| Message ID | 20181205014900.4827-1-jeremyfertic@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0" | expand |
On Tue, 2018-12-04 at 18:49 -0700, Jeremy Fertic wrote: > This reverts commit 00426e99789357dbff7e719a092ce36a3ce49d94. > > i2c_smbus_read_byte() returns 0 when a byte with the value 0 is read > from > the device. This is a valid read so revert the check for 0. > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> > --- Hi Jeremy, As per my understanding, 0 value indicates no error but no data read. Then how can this be a valid case? Can you please make me understand that how can we consider this as a valid case even when no data has been read? Thanks > drivers/staging/iio/addac/adt7316-i2c.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/staging/iio/addac/adt7316-i2c.c > b/drivers/staging/iio/addac/adt7316-i2c.c > index ac91163656b5..2d51bd425662 100644 > --- a/drivers/staging/iio/addac/adt7316-i2c.c > +++ b/drivers/staging/iio/addac/adt7316-i2c.c > @@ -30,10 +30,6 @@ static int adt7316_i2c_read(void *client, u8 reg, > u8 *data) > } > > ret = i2c_smbus_read_byte(client); > - > - if (!ret) > - return -EIO; > - > if (ret < 0) { > dev_err(&cl->dev, "I2C read error\n"); > return ret;
On Thu, Dec 06, 2018 at 01:25:55AM +0530, Shreeya Patel wrote: > On Tue, 2018-12-04 at 18:49 -0700, Jeremy Fertic wrote: > > This reverts commit 00426e99789357dbff7e719a092ce36a3ce49d94. > > > > i2c_smbus_read_byte() returns 0 when a byte with the value 0 is read > > from > > the device. This is a valid read so revert the check for 0. > > > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> > > --- > > Hi Jeremy, > > As per my understanding, 0 value indicates no error but no data read. > Then how can this be a valid case? > > Can you please make me understand that how can we consider this as a > valid case even when no data has been read? > > > Thanks I'm not sure I understand why the value 0 would indicate no data read. Doesn't that just mean a byte was read with the value 0. For instance, if the input to the adc is 0V. Can you point me to where you're seeing that this would indicate no data read? > > > drivers/staging/iio/addac/adt7316-i2c.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/drivers/staging/iio/addac/adt7316-i2c.c > > b/drivers/staging/iio/addac/adt7316-i2c.c > > index ac91163656b5..2d51bd425662 100644 > > --- a/drivers/staging/iio/addac/adt7316-i2c.c > > +++ b/drivers/staging/iio/addac/adt7316-i2c.c > > @@ -30,10 +30,6 @@ static int adt7316_i2c_read(void *client, u8 reg, > > u8 *data) > > } > > > > ret = i2c_smbus_read_byte(client); > > - > > - if (!ret) > > - return -EIO; > > - > > if (ret < 0) { > > dev_err(&cl->dev, "I2C read error\n"); > > return ret;
On Wed, Dec 05, 2018 at 02:59:53PM -0700, Jeremy Fertic wrote: > On Thu, Dec 06, 2018 at 01:25:55AM +0530, Shreeya Patel wrote: > > On Tue, 2018-12-04 at 18:49 -0700, Jeremy Fertic wrote: > > > This reverts commit 00426e99789357dbff7e719a092ce36a3ce49d94. > > > > > > i2c_smbus_read_byte() returns 0 when a byte with the value 0 is read > > > from > > > the device. This is a valid read so revert the check for 0. > > > > > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> > > > --- > > > > Hi Jeremy, > > > > As per my understanding, 0 value indicates no error but no data read. > > Then how can this be a valid case? > > > > Can you please make me understand that how can we consider this as a > > valid case even when no data has been read? It's not reading no data. It's reading one byte of data and returning it. > > > > > > Thanks > > I'm not sure I understand why the value 0 would indicate no data read. > Doesn't that just mean a byte was read with the value 0. Yes. It does mean that. Please don't ask rhetorical questions... :( This list is full of people who can't resist answering every question. > For instance, if the input to the adc is 0V. Can you point me to where > you're seeing that this would indicate no data read? drivers/i2c/i2c-core-smbus.c 88 /** 89 * i2c_smbus_read_byte - SMBus "receive byte" protocol 90 * @client: Handle to slave device 91 * 92 * This executes the SMBus "receive byte" protocol, returning negative errno 93 * else the byte received from the device. 94 */ 95 s32 i2c_smbus_read_byte(const struct i2c_client *client) 96 { 97 union i2c_smbus_data data; 98 int status; 99 100 status = i2c_smbus_xfer(client->adapter, client->addr, client->flags, 101 I2C_SMBUS_READ, 0, 102 I2C_SMBUS_BYTE, &data); 103 return (status < 0) ? status : data.byte; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 104 } 105 EXPORT_SYMBOL(i2c_smbus_read_byte); You are right. Commit 00426e997893 ("Staging: iio: adt7316: Add an extra check for 'ret' equals to 0") needs to be reverted... regards, dan carpenter
On Thu, 2018-12-06 at 15:40 +0300, Dan Carpenter wrote: > On Wed, Dec 05, 2018 at 02:59:53PM -0700, Jeremy Fertic wrote: > > On Thu, Dec 06, 2018 at 01:25:55AM +0530, Shreeya Patel wrote: > > > On Tue, 2018-12-04 at 18:49 -0700, Jeremy Fertic wrote: > > > > This reverts commit 00426e99789357dbff7e719a092ce36a3ce49d94. > > > > > > > > i2c_smbus_read_byte() returns 0 when a byte with the value 0 is > > > > read > > > > from > > > > the device. This is a valid read so revert the check for 0. > > > > > > > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> > > > > --- > > > > > > Hi Jeremy, > > > > > > As per my understanding, 0 value indicates no error but no data > > > read. > > > Then how can this be a valid case? > > > > > > Can you please make me understand that how can we consider this > > > as a > > > valid case even when no data has been read? > > It's not reading no data. It's reading one byte of data and > returning > it. > > > > > > > > > > Thanks > > > > I'm not sure I understand why the value 0 would indicate no data > > read. > > Doesn't that just mean a byte was read with the value 0. > > Yes. It does mean that. Please don't ask rhetorical > questions... :( > This list is full of people who can't resist answering every > question. > > > For instance, if the input to the adc is 0V. Can you point me to > > where > > you're seeing that this would indicate no data read? > > drivers/i2c/i2c-core-smbus.c > 88 /** > 89 * i2c_smbus_read_byte - SMBus "receive byte" protocol > 90 * @client: Handle to slave device > 91 * > 92 * This executes the SMBus "receive byte" protocol, returning > negative errno > 93 * else the byte received from the device. > 94 */ > 95 s32 i2c_smbus_read_byte(const struct i2c_client *client) > 96 { > 97 union i2c_smbus_data data; > 98 int status; > 99 > 100 status = i2c_smbus_xfer(client->adapter, client- > >addr, client->flags, > 101 I2C_SMBUS_READ, 0, > 102 I2C_SMBUS_BYTE, &data); > 103 return (status < 0) ? status : data.byte; > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > 104 } > 105 EXPORT_SYMBOL(i2c_smbus_read_byte); Even I had sent the same code to Jonathan and we had a discussion on this. I asked him that this code clearly shows that there is an error condition only when status < 0 then why do we need a check for status = 0. Then he explained me that 0 isn't an error. The issue is the silliness of the i2c interface. Pretty much every other bus returns an error (negative) if less data is received than expected. Most i2c bus master's do as well but in theory it can return 0 to indicate no error but no data read (which doesn't make any sense) 0 doesn't ever happen in reality but it should be handled for correctness though. So we should wait for what Jonathan has to say on this :) Thanks > You are right. Commit 00426e997893 ("Staging: iio: adt7316: Add an > extra check for 'ret' equals to 0") needs to be reverted... > > regards, > dan carpenter > >
On Sat, 08 Dec 2018 00:07:21 +0530 Shreeya Patel <shreeya.patel23498@gmail.com> wrote: > On Thu, 2018-12-06 at 15:40 +0300, Dan Carpenter wrote: > > On Wed, Dec 05, 2018 at 02:59:53PM -0700, Jeremy Fertic wrote: > > > On Thu, Dec 06, 2018 at 01:25:55AM +0530, Shreeya Patel wrote: > > > > On Tue, 2018-12-04 at 18:49 -0700, Jeremy Fertic wrote: > > > > > This reverts commit 00426e99789357dbff7e719a092ce36a3ce49d94. > > > > > > > > > > i2c_smbus_read_byte() returns 0 when a byte with the value 0 is > > > > > read > > > > > from > > > > > the device. This is a valid read so revert the check for 0. > > > > > > > > > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> > > > > > --- > > > > > > > > Hi Jeremy, > > > > > > > > As per my understanding, 0 value indicates no error but no data > > > > read. > > > > Then how can this be a valid case? > > > > > > > > Can you please make me understand that how can we consider this > > > > as a > > > > valid case even when no data has been read? > > > > It's not reading no data. It's reading one byte of data and > > returning > > it. > > > > > > > > > > > > > > Thanks > > > > > > I'm not sure I understand why the value 0 would indicate no data > > > read. > > > Doesn't that just mean a byte was read with the value 0. > > > > Yes. It does mean that. Please don't ask rhetorical > > questions... :( > > This list is full of people who can't resist answering every > > question. > > > > > For instance, if the input to the adc is 0V. Can you point me to > > > where > > > you're seeing that this would indicate no data read? > > > > drivers/i2c/i2c-core-smbus.c > > 88 /** > > 89 * i2c_smbus_read_byte - SMBus "receive byte" protocol > > 90 * @client: Handle to slave device > > 91 * > > 92 * This executes the SMBus "receive byte" protocol, returning > > negative errno > > 93 * else the byte received from the device. > > 94 */ > > 95 s32 i2c_smbus_read_byte(const struct i2c_client *client) > > 96 { > > 97 union i2c_smbus_data data; > > 98 int status; > > 99 > > 100 status = i2c_smbus_xfer(client->adapter, client- > > >addr, client->flags, > > 101 I2C_SMBUS_READ, 0, > > 102 I2C_SMBUS_BYTE, &data); > > 103 return (status < 0) ? status : data.byte; > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > 104 } > > 105 EXPORT_SYMBOL(i2c_smbus_read_byte); > > Even I had sent the same code to Jonathan and we had a discussion on > this. > I asked him that this code clearly shows that there is an error > condition only when status < 0 then why do we need a check for status = > 0. > > Then he explained me that 0 isn't an error. The issue is the silliness > of the i2c interface. > > Pretty much every other bus returns an error (negative) if less data is > received than expected. Most i2c > bus master's do as well but in theory it can return 0 to indicate no > error but no data read (which doesn't make any sense) > > 0 doesn't ever happen in reality but it should be handled for > correctness though. > > So we should wait for what Jonathan has to say on this :) Yup, I was being an idiot. Sorry about that! For some reason I'd gotten it into my head that the particular function we were talking about was i2c_master_send which does indeed do as discussed above. Apologies for misleading you on this. Definitely a proper idiot moment of me not reading what the code actually was properly, even when you questioned what I was going on about. Thanks to Jeremy for catching this one. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Jonathan > > Thanks > > > You are right. Commit 00426e997893 ("Staging: iio: adt7316: Add an > > extra check for 'ret' equals to 0") needs to be reverted... > > > > regards, > > dan carpenter > > > >
On Sat, 2018-12-08 at 11:17 +0000, Jonathan Cameron wrote: > On Sat, 08 Dec 2018 00:07:21 +0530 > Shreeya Patel <shreeya.patel23498@gmail.com> wrote: > > > On Thu, 2018-12-06 at 15:40 +0300, Dan Carpenter wrote: > > > On Wed, Dec 05, 2018 at 02:59:53PM -0700, Jeremy Fertic wrote: > > > > On Thu, Dec 06, 2018 at 01:25:55AM +0530, Shreeya Patel > > > > wrote: > > > > > On Tue, 2018-12-04 at 18:49 -0700, Jeremy Fertic wrote: > > > > > > This reverts commit > > > > > > 00426e99789357dbff7e719a092ce36a3ce49d94. > > > > > > > > > > > > i2c_smbus_read_byte() returns 0 when a byte with the value > > > > > > 0 is > > > > > > read > > > > > > from > > > > > > the device. This is a valid read so revert the check for 0. > > > > > > > > > > > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> > > > > > > --- > > > > > > > > > > Hi Jeremy, > > > > > > > > > > As per my understanding, 0 value indicates no error but no > > > > > data > > > > > read. > > > > > Then how can this be a valid case? > > > > > > > > > > Can you please make me understand that how can we consider > > > > > this > > > > > as a > > > > > valid case even when no data has been read? > > > > > > It's not reading no data. It's reading one byte of data and > > > returning > > > it. > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > I'm not sure I understand why the value 0 would indicate no > > > > data > > > > read. > > > > Doesn't that just mean a byte was read with the value 0. > > > > > > Yes. It does mean that. Please don't ask rhetorical > > > questions... :( > > > This list is full of people who can't resist answering every > > > question. > > > > > > > For instance, if the input to the adc is 0V. Can you point me > > > > to > > > > where > > > > you're seeing that this would indicate no data read? > > > > > > drivers/i2c/i2c-core-smbus.c > > > 88 /** > > > 89 * i2c_smbus_read_byte - SMBus "receive byte" protocol > > > 90 * @client: Handle to slave device > > > 91 * > > > 92 * This executes the SMBus "receive byte" protocol, > > > returning > > > negative errno > > > 93 * else the byte received from the device. > > > 94 */ > > > 95 s32 i2c_smbus_read_byte(const struct i2c_client *client) > > > 96 { > > > 97 union i2c_smbus_data data; > > > 98 int status; > > > 99 > > > 100 status = i2c_smbus_xfer(client->adapter, > > > client- > > > > addr, client->flags, > > > > > > 101 I2C_SMBUS_READ, 0, > > > 102 I2C_SMBUS_BYTE, &data); > > > 103 return (status < 0) ? status : data.byte; > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > 104 } > > > 105 EXPORT_SYMBOL(i2c_smbus_read_byte); > > > > Even I had sent the same code to Jonathan and we had a discussion > > on > > this. > > I asked him that this code clearly shows that there is an error > > condition only when status < 0 then why do we need a check for > > status = > > 0. > > > > Then he explained me that 0 isn't an error. The issue is the > > silliness > > of the i2c interface. > > > > Pretty much every other bus returns an error (negative) if less > > data is > > received than expected. Most i2c > > bus master's do as well but in theory it can return 0 to indicate > > no > > error but no data read (which doesn't make any sense) > > > > 0 doesn't ever happen in reality but it should be handled for > > correctness though. > > > > So we should wait for what Jonathan has to say on this :) > > Yup, I was being an idiot. Sorry about that! For some reason I'd > gotten it into my head that the particular function we were talking > about was i2c_master_send which does indeed do as discussed above. > > Apologies for misleading you on this. Definitely a proper idiot > moment of me not reading what the code actually was properly, even > when you questioned what I was going on about. It was not your mistake! There was a confusion because of delay in replying to you from my side. So it was just the case of human error :) > > Thanks to Jeremy for catching this one. > > Applied to the togreg branch of iio.git and pushed out as testing for > the autobuilders to play with it. > > Jonathan > > > > > Thanks > > > > > You are right. Commit 00426e997893 ("Staging: iio: adt7316: Add > > > an > > > extra check for 'ret' equals to 0") needs to be reverted... > > > > > > regards, > > > dan carpenter > > > > > > > >
diff --git a/drivers/staging/iio/addac/adt7316-i2c.c b/drivers/staging/iio/addac/adt7316-i2c.c index ac91163656b5..2d51bd425662 100644 --- a/drivers/staging/iio/addac/adt7316-i2c.c +++ b/drivers/staging/iio/addac/adt7316-i2c.c @@ -30,10 +30,6 @@ static int adt7316_i2c_read(void *client, u8 reg, u8 *data) } ret = i2c_smbus_read_byte(client); - - if (!ret) - return -EIO; - if (ret < 0) { dev_err(&cl->dev, "I2C read error\n"); return ret;
This reverts commit 00426e99789357dbff7e719a092ce36a3ce49d94. i2c_smbus_read_byte() returns 0 when a byte with the value 0 is read from the device. This is a valid read so revert the check for 0. Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> --- drivers/staging/iio/addac/adt7316-i2c.c | 4 ---- 1 file changed, 4 deletions(-)