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