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