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

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