[PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"

[PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"

[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

Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"

[Shreeya Patel]

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;

Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"

[Jeremy Fertic]

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;

Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"

[Dan Carpenter]

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

Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"

[Shreeya Patel]

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
> 
> 

Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"

[Jonathan Cameron]

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
> > 
> >   

Re: [PATCH] Revert "Staging: iio: adt7316: Add an extra check for 'ret' equals to 0"

[Shreeya Patel]

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
> > > 
> > >   
> 
>