[PATCH v3 11/16] board: ti: j784s4: Add support for board detection by EEPROM read

[Nishanth Menon]

On 12:20-20230908, Andrew Davis wrote:
> On 9/8/23 12:03 PM, Nishanth Menon wrote:
> > On 10:59-20230908, Andrew Davis wrote:
> > > On 9/8/23 9:42 AM, Nishanth Menon wrote:
> > > > On 16:35-20230908, Apurva Nandan wrote:
> > > > > From: Dasnavis Sabiya <sabiya.d at ti.com>
> > > > > 
> > > > > The board name is programmed in the EEPROM. Add support for board
> > > > > detection and choose the right dtb based on the board name read
> > > > > from EEPROM.
> > > > > 
> > > > > The J784S4/AM69 has two platforms naming J784S4-EVM and AM69-SK. The
> > > > > J784S4 has EEPROM populated at 0x50. AM69 SK has EEPROM
> > > > > populated at next address 0x51. So start looking for TI specific EEPROM
> > > > > at 0x50, if not found look for EEPROM at 0x51.
> > > > > 
> > > > > Also, add support for setup_serial() to read the serial number from EEPROM
> > > > > and update the serial environment variable with the same.
> > > > > 
> > > > > Signed-off-by: Dasnavis Sabiya <sabiya.d at ti.com>
> > > > > Signed-off-by: Apurva Nandan <a-nandan at ti.com>
> > > > 
> > > > 
> > > > NAK. go with the approach similar to AM62x. AM69-SK has it's own
> > > > fragment.
> > > > 
> > > 
> > > NAK to your NAK, there is no reason to introduce more fragments here
> > > when both boards can be supported with one defconfig.
> > 
> > Sorry, but no. the evm.c has become a mess. Time to stop it is in the
> > introduction itself
> > 
> > Need the eeprom stuff clearly pulled out of evm.c -> evm.c should still
> > work with CONFIG_EEPROM_DETECT disabled and should be board independent.
> > 
> 
> Sure evm.c is a mess, but that is the price for one config/build working
> on multiple boards. That is why we have EEPROM on *every* EVM, you throw
> that away if you got to "just have a different config/build for each EVM".

I understand, it also comes in the way of downstream customers creating
board variants and using this code as the reference (there is only this
code that is the starting point).

> 
> Honestly I'm fine either way (I never did think we got much out of
> multi-board detection support), but that decision should come with more
> reasoning than "NAK"..

Sure and it has been discussed over and over again on the mailing list
with Tom reacting to the mess created and which is how am62 was trimmed
down in the first place and highlighted internally as well, multiple
times.

So, I believe the rationale is sufficient.
If we don't learn from the feedback[1], it shows badly on us.

Long story short, keep it clean, simple reference code, or sorry, NAK
stands.

[1] https://jaycarlson.net/embedded-linux/#am335x
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D