| Message ID | 20231020070128.28320-1-p-mantena@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [RESEND] board: ti: common: board_detect: Fix EEPROM offset read for 1-byte | expand |
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 > > Signed-off-by: Prasanth Babu Mantena <p-mantena@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 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??? */
Hi Prasanth On 20-Oct-23 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"). > > 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. > > Signed-off-by: Prasanth Babu Mantena <p-mantena@ti.com> > Fixes: 9f393a2d7af8 (board: ti: common: board_detect: Fix EEPROM read quirk for 2-byte) > --- > 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??? */ Thanks for catching this! Reviewed-by: Neha Malcom Francis <n-francis@ti.com>
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-mantena@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.kornilov@gmail.com> , Jason Kridner <jkridner@beagleboard.org> and Robert Nelson <robertcnelson@gmail.com> So that we don't have regressions on their boards. https://lore.kernel.org/all/CAJs94Ebdd4foOjhGFu9Bop0v=B1US9neDLxfhgcY23ukgLzFOQ@mail.gmail.com/
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-mantena@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.kornilov@gmail.com> , Jason > Kridner <jkridner@beagleboard.org> and Robert Nelson <robertcnelson@gmail.com> > > So that we don't have regressions on their boards. > > https://lore.kernel.org/all/CAJs94Ebdd4foOjhGFu9Bop0v=B1US9neDLxfhgcY23ukgLzFOQ@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
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 > > > > Signed-off-by: Prasanth Babu Mantena <p-mantena@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 > > > Please copy Nishanth in patch as well . > Thanks for the review comments Udit. Will send a v2 addressing them. Regards, Prasanth > > --- > > 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??? */
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??? */
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"). 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. Signed-off-by: Prasanth Babu Mantena <p-mantena@ti.com> Fixes: 9f393a2d7af8 (board: ti: common: board_detect: Fix EEPROM read quirk for 2-byte) --- Resending due to incorrect patch tag last time. board/ti/common/board_detect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)