On 13:05-20231020, Nishanth Menon wrote:
> On 20:52-20231020, Kumar, Udit wrote:
> > Thanks Prasanth
> >
> > On 10/20/2023 12:31 PM, Prasanth Babu Mantena wrote:
> > > EEPROM detection logic in ti_i2c_eeprom_get() involves reading the total
> > > size followed by reading 1-byte size with an offset 1. This commit fixes
> > > the header matching issue in commit 9f393a2d7af8 ("board: ti: common:
> > > board_detect: Fix EEPROM read quirk for 2-byte").
> > You can fixes below as well. I think you can avoid this in commit message
> > >
> > > In the previous commit, the value with one offset is being read into
> > > offset_test, but the pointer used to match was still ep. After reading
> > > with an offset 1, the second byte of the header is compared with the
> > > 1-byte
> > > data read from EEPROM. This is taken care by comparing proper first byte
> > > value from the header.
> > Nice catch
>
> Yes indeed.. here is a possible improvement of commit message:
>
> EEPROM detection logic in ti_i2c_eeprom_get() involves reading the
> total size and the 1-byte size with an offset 1.
>
> The value with one offset is read into "offset_test," but the pointer
> used to match was still "ep". This results in comparing results
> against the previously read data while the intent is to ensure the
> detection of the bad 2-byte addressing eeproms that get stuck after
> responding initially to 1- byte accesses. This behavior was detected
> on BeagleBone-AI64.
>
> > >
> > > Signed-off-by: Prasanth Babu Mantena <p-mant...@ti.com>
> > > Fixes: 9f393a2d7af8 (board: ti: common: board_detect: Fix EEPROM read
> > > quirk for 2-byte)
> > Please consider Fixes line, first than Signed-off-by
>
> Yup, fixes and SoB (checkpatch typically warns)
> >
> >
> > Please copy Nishanth in patch as well .
> >
> > > ---
> > > Resending due to incorrect patch tag last time.
> > >
> > > board/ti/common/board_detect.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/board/ti/common/board_detect.c
> > > b/board/ti/common/board_detect.c
> > > index 9a53884c98..17fe8f8069 100644
> > > --- a/board/ti/common/board_detect.c
> > > +++ b/board/ti/common/board_detect.c
> > > @@ -128,7 +128,7 @@ static int __maybe_unused ti_i2c_eeprom_get(int
> > > bus_addr, int dev_addr,
> > > rc = dm_i2c_read(dev, 0x1, &offset_test, sizeof(offset_test));
> > > - if (*((u32 *)ep) != (header & 0xFF))
> > > + if (offset_test != ((header >> 8) & 0xFF))
> > > one_byte_addressing = false;
> > > /* Corrupted data??? */
>
>
> You missed the else CONFIG_IS_ENABLED(DM_I2C) case with the same
> bug?
>
> Also please CC Matwey V. Kornilov <matwey.korni...@gmail.com> , Jason
> Kridner <jkrid...@beagleboard.org> and Robert Nelson <robertcnel...@gmail.com>
>
> So that we don't have regressions on their boards.
>
> https://lore.kernel.org/all/CAJs94Ebdd4foOjhGFu9Bop0v=b1us9nedlxfhgcy23ukglz...@mail.gmail.com/
Thanks for the comments Nishanth.
Will send a v2 addressing them.
Regards,
Prasanth
>
> --
> Regards,
> Nishanth Menon
> Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5
> 849D 1736 249D
