Re: [U-Boot] [PATCH v2 0/9] Added support for SPEAr SoCs
On Fri, 2010-01-08 at 10:05 +0530, Vipin KUMAR wrote:
Hello Peter,
This patch set is a reworked patch which incorporates all review
feedbacks from
earlier earlier mails
This patch set contains the support for 4 SoCs SPEAr600 SPEAr300 SPEAr310 SPEAr320
SPEAr is an ARM based SoC which supports rich set of peripherals like
Ethernet,
USB Host, USB Device etc to support various generalapplications
For further info on SPEAr SoC, please see README.spear also contained
in the
patch set.
Vipin (9): Added README.spear SPEAr600 SoC support added SPEAr300 SoC support added SPEAr310 SoC support added SPEAr320 SoC support added i2c driver support for SPEAr SoCs smi driver support for SPEAr SoCs nand(fsmc) driver support for SPEAr SoCs usbd driver support for SPEAr SoCs
It would be nice if you reordered the patches. At a glance, some of the boards added first rely on drivers added later. Eg patch 1 uses the I2C driver in patch 6. This will cause 'git bisects' to break. To prevent this you should add SOC + driver code first, then boards last. Something like:
- Initial SPEAr SOC support
- SPEAr: Add I2C support
- SPEAr: Add SMI support
.... 6. Add SPEAr 600 board support 7. Add SPEAr 300 board support 8. Add SPEAr 310 board support 9. Add SPEAr 320 board support
I am planning to keep the patch order as it is but I would remove dependencies eg. SPEAr600 would not contain code that depends on a driver added later and so on.
This should also be OK. Right?
It should remove the 'git bisect' issue, which is the important thing to me. The way you did it in v3 should work, its just a bit uglier than doing it the "right" way that I outlined above. Eg you modify spear.h and config.mk each time you add a new driver, basically tweaking the board support as the patch sequence progresses instead of just adding proper board support in 1 patch at the end.
Also, its a bit ugly to add both spear arch support as well as board support in patch 2/11. Ideally you'd have 1 patch adding spear architectural support, then additional patches for drivers, then patches for boards.
In any case, my comments are mostly aesthetic and I don't really care how you split up the patches as long as you don't break bisection:) Wolfgang or Tom can force you to reorganize the patches if they care about it enough.
Best, Peter
Peter Tyser wrote:
On Fri, 2010-01-08 at 10:05 +0530, Vipin KUMAR wrote:
Hello Peter,
<snip>
I am planning to keep the patch order as it is but I would remove dependencies eg. SPEAr600 would not contain code that depends on a driver added later and so on.
This should also be OK. Right?
It should remove the 'git bisect' issue, which is the important thing to me. The way you did it in v3 should work, its just a bit uglier than doing it the "right" way that I outlined above. Eg you modify spear.h and config.mk each time you add a new driver, basically tweaking the board support as the patch sequence progresses instead of just adding proper board support in 1 patch at the end.
Also, its a bit ugly to add both spear arch support as well as board support in patch 2/11. Ideally you'd have 1 patch adding spear architectural support, then additional patches for drivers, then patches for boards.
On 2/11, this patch must be split. Arch support split from board support.
Moving the driver support before board support would simplify the review. The subsystem custodians would only have to review what they are responsible for. I would also recommend doing this.
Tom
In any case, my comments are mostly aesthetic and I don't really care how you split up the patches as long as you don't break bisection:) Wolfgang or Tom can force you to reorganize the patches if they care about it enough.
Best, Peter
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hello Tom,
I am planning to keep the patch order as it is but I would remove dependencies eg. SPEAr600 would not contain code that depends on a driver added later and so on.
This should also be OK. Right?
It should remove the 'git bisect' issue, which is the important thing to me. The way you did it in v3 should work, its just a bit uglier than doing it the "right" way that I outlined above. Eg you modify spear.h and config.mk each time you add a new driver, basically tweaking the board support as the patch sequence progresses instead of just adding proper board support in 1 patch at the end.
Also, its a bit ugly to add both spear arch support as well as board support in patch 2/11. Ideally you'd have 1 patch adding spear architectural support, then additional patches for drivers, then patches for boards.
On 2/11, this patch must be split. Arch support split from board support.
Moving the driver support before board support would simplify the review. The subsystem custodians would only have to review what they are responsible for. I would also recommend doing this.
Changes done as you suggested in patch set version4
Regards Vipin
Vipin Kumar wrote:
Hello Tom,
I am planning to keep the patch order as it is but I would remove dependencies eg. SPEAr600 would not contain code that depends on a driver added later and so on.
This should also be OK. Right?
It should remove the 'git bisect' issue, which is the important thing to me. The way you did it in v3 should work, its just a bit uglier than doing it the "right" way that I outlined above. Eg you modify spear.h and config.mk each time you add a new driver, basically tweaking the board support as the patch sequence progresses instead of just adding proper board support in 1 patch at the end.
Also, its a bit ugly to add both spear arch support as well as board support in patch 2/11. Ideally you'd have 1 patch adding spear architectural support, then additional patches for drivers, then patches for boards.
On 2/11, this patch must be split. Arch support split from board support.
Moving the driver support before board support would simplify the review. The subsystem custodians would only have to review what they are responsible for. I would also recommend doing this.
Changes done as you suggested in patch set version4
Thanks, I am reviewing this patch set now. So far it looks ok wrt mechanical checking. Tom
Regards Vipin
participants (3)
-
Peter Tyser -
Tom -
Vipin Kumar