[U-Boot] [PATCH v3 0/5] ARM: omap-common: Add board detection support for TI EVMs

[Nishanth Menon]

On 11/04/2015 04:00 PM, Steve Kipisz wrote:
> Several TI EVMs have onboard EEPROM that contain board description
> information. The onboard EEPROM on Beaglebone, Beaglebone Black, AM335x
> EVM, AM43x EVM, AM57xx EVM, Beagleboard-x15 all share the same format.
> 
> This series of patches introduces code which is generic among these
> platforms. The boards can use the data for any operations they might
> choose.
> 
> Lokesh Vutla (1):
>   ARM: omap-common: Add standard access for board description EEPROM
> 
> Steve Kipisz (4):
>   ARM: OMAP4/5: Centralize early clock initialization
>   ARM: OMAP4/5: Centralize gpi2c_init
>   ARM: OMAP4/5: Add generic board detection hook
>   board: ti: AM57xx: Add detection logic for AM57xx-evm
> 
>  arch/arm/cpu/armv7/omap-common/clocks-common.c |  21 +++-
>  arch/arm/cpu/armv7/omap-common/hwinit-common.c |  14 ++-
>  arch/arm/include/asm/arch-omap4/sys_proto.h    |   4 +-
>  arch/arm/include/asm/arch-omap5/sys_proto.h    |   4 +-
>  arch/arm/include/asm/omap_common.h             |   8 +-
>  board/ti/am57xx/Makefile                       |   2 +
>  board/ti/am57xx/board.c                        |  52 +++++++++
>  board/ti/common/board.c                        |  54 +++++++++
>  board/ti/common/board.h                        | 117 +++++++++++++++++++
>  board/ti/common/ti-i2c-eeprom.c                | 148 +++++++++++++++++++++++++
>  include/configs/am57xx_evm.h                   |   4 +
>  11 files changed, 419 insertions(+), 9 deletions(-)
>  create mode 100644 board/ti/common/board.c
>  create mode 100644 board/ti/common/board.h
>  create mode 100644 board/ti/common/ti-i2c-eeprom.c
> 

In the future, please:
a) CC beagleboard-x15 mailing list (beagleboard-x15
<beagleboard-x15 at googlegroups.com>)
b) CC previous reviewers: I think I dont see Igor
(grinberg at compulab.co.il) in CC list. it is a good practice to cc
previous reviewers because, we dont always watch all emails coming to
the list and you'd like to respect the time and effort reviewers put
in when sending newer revisions.
c) even though I see that you have provided in-context of each patch
what the changes are (that is good), it is usually better to add to
the covering letter what the v1,v2 links were, and also what the
baseline for this series is - in this case, you do depend on
https://patchwork.ozlabs.org/patch/538046/ - and should be highlighted
in cover-letter and reiterated in the corresponding patch.
d) provide links to test logs in cover-letter. most of folks do not
possess x15 and evms - While I do realize that you have covered that
data in the patch #5, personally, I dont usually review patches that
are'nt even tested - and I personally respect the patches that tell me
upfront in cover letter than the patches have been tested and links to
the same.
e) provide the baseline for the patches in cover letter, instead of
repeating them over and over in each of the patches. it is a series,
and patches following will depend on previous patches. For example,
your patch #5 claims:
"v3 Based on:
 master     83bf0057 arm: at91: reworked meesc board support"

That is not really true - because you do depend on x15 rename patch.
If I attempt to apply this series on current u-boot master, I will
fail and that does not give me a good impression of the series overall.


Apologies on ranting about this, but I'd thought it might help you
give a better perspective of what, at least I, as a reviewer would
like to see in a series.


-- 
Regards,
Nishanth Menon