* [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"
@ 2018-12-05 1:49 Jeremy Fertic
2018-12-05 19:55 ` Shreeya Patel
0 siblings, 1 reply; 7+ messages in thread
From: Jeremy Fertic @ 2018-12-05 1:49 UTC (permalink / raw)
To: jic23, linux-iio
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, devel,
linux-kernel, Jeremy Fertic
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(-)
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;
--
2.19.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"
2018-12-05 1:49 [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0" Jeremy Fertic
@ 2018-12-05 19:55 ` Shreeya Patel
2018-12-05 21:59 ` Jeremy Fertic
0 siblings, 1 reply; 7+ messages in thread
From: Shreeya Patel @ 2018-12-05 19:55 UTC (permalink / raw)
To: Jeremy Fertic, jic23, linux-iio
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, devel, linux-kernel
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"
2018-12-05 19:55 ` Shreeya Patel
@ 2018-12-05 21:59 ` Jeremy Fertic
2018-12-06 12:40 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Jeremy Fertic @ 2018-12-05 21:59 UTC (permalink / raw)
To: Shreeya Patel
Cc: jic23, linux-iio, lars, Michael.Hennerich, knaack.h, pmeerw,
gregkh, devel, linux-kernel
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"
2018-12-05 21:59 ` Jeremy Fertic
@ 2018-12-06 12:40 ` Dan Carpenter
2018-12-07 18:37 ` Shreeya Patel
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2018-12-06 12:40 UTC (permalink / raw)
To: Jeremy Fertic
Cc: Shreeya Patel, devel, lars, Michael.Hennerich, linux-iio, gregkh,
linux-kernel, pmeerw, knaack.h, jic23
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"
2018-12-06 12:40 ` Dan Carpenter
@ 2018-12-07 18:37 ` Shreeya Patel
2018-12-08 11:17 ` Jonathan Cameron
0 siblings, 1 reply; 7+ messages in thread
From: Shreeya Patel @ 2018-12-07 18:37 UTC (permalink / raw)
To: Dan Carpenter, Jeremy Fertic
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, linux-kernel,
pmeerw, knaack.h, jic23
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
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"
2018-12-07 18:37 ` Shreeya Patel
@ 2018-12-08 11:17 ` Jonathan Cameron
2018-12-08 15:33 ` Shreeya Patel
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2018-12-08 11:17 UTC (permalink / raw)
To: Shreeya Patel
Cc: Dan Carpenter, Jeremy Fertic, devel, lars, Michael.Hennerich,
linux-iio, gregkh, linux-kernel, pmeerw, knaack.h
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
> >
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"
2018-12-08 11:17 ` Jonathan Cameron
@ 2018-12-08 15:33 ` Shreeya Patel
0 siblings, 0 replies; 7+ messages in thread
From: Shreeya Patel @ 2018-12-08 15:33 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dan Carpenter, Jeremy Fertic, devel, lars, Michael.Hennerich,
linux-iio, gregkh, linux-kernel, pmeerw, knaack.h
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
> > >
> > >
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-08 15:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-05 1:49 [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0" Jeremy Fertic
2018-12-05 19:55 ` Shreeya Patel
2018-12-05 21:59 ` Jeremy Fertic
2018-12-06 12:40 ` Dan Carpenter
2018-12-07 18:37 ` Shreeya Patel
2018-12-08 11:17 ` Jonathan Cameron
2018-12-08 15:33 ` Shreeya Patel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).