[PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data

[PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data

[Alison Schofield]

Replace the i2c_smbus_read_byte commmands used to retrieve the sensor
data with an i2c_master_recv command.

The smbus read byte method fails because the device does not expect a
stop condition after sending the first byte. When we issue the second
read, we are getting the first byte again. Net effect is that of the 14
bits used for the measurement, the 8 most significant bits are correct,
the lower 6 are not.

None of the smbus read protocols follow the pattern this device requires
(S Addr Rd [A] Data [A] Data NA P), hence the switch to an i2c receive
transaction.

Signed-off-by: Alison Schofield <amsfield22@gmail.com>
Cc: Daniel Baluta <daniel.baluta@gmail.com>
---
 drivers/iio/humidity/hdc100x.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
index a03832a..643a42d 100644
--- a/drivers/iio/humidity/hdc100x.c
+++ b/drivers/iio/humidity/hdc100x.c
@@ -142,7 +142,7 @@ static int hdc100x_get_measurement(struct hdc100x_data *data,
 	struct i2c_client *client = data->client;
 	int delay = data->adc_int_us[chan->address];
 	int ret;
-	int val;
+	u8 buf[2];
 
 	/* start measurement */
 	ret = i2c_smbus_write_byte(client, chan->address);
@@ -154,26 +154,13 @@ static int hdc100x_get_measurement(struct hdc100x_data *data,
 	/* wait for integration time to pass */
 	usleep_range(delay, delay + 1000);
 
-	/*
-	 * i2c_smbus_read_word_data cannot() be used here due to the command
-	 * value not being understood and causes NAKs preventing any reading
-	 * from being accessed.
-	 */
-	ret = i2c_smbus_read_byte(client);
+	/* read the 2 byte measurement */
+	ret = i2c_master_recv(data->client, buf, 2);
 	if (ret < 0) {
-		dev_err(&client->dev, "cannot read high byte measurement");
+		dev_err(&client->dev, "cannot read sensor data\n");
 		return ret;
 	}
-	val = ret << 8;
-
-	ret = i2c_smbus_read_byte(client);
-	if (ret < 0) {
-		dev_err(&client->dev, "cannot read low byte measurement");
-		return ret;
-	}
-	val |= ret;
-
-	return val;
+	return (int)(buf[0] << 8 | buf[1]);
 }
 
 static int hdc100x_get_heater_status(struct hdc100x_data *data)
@@ -272,8 +259,8 @@ static int hdc100x_probe(struct i2c_client *client,
 	struct iio_dev *indio_dev;
 	struct hdc100x_data *data;
 
-	if (!i2c_check_functionality(client->adapter,
-				I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_BYTE))
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA |
+				     I2C_FUNC_SMBUS_BYTE | I2C_FUNC_I2C))
 		return -EOPNOTSUPP;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
-- 
2.1.4

Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data

[Matt Ranostay]

On Wed, Aug 3, 2016 at 8:33 PM, Alison Schofield <amsfield22@gmail.com> wrote:
> Replace the i2c_smbus_read_byte commmands used to retrieve the sensor
> data with an i2c_master_recv command.
>
> The smbus read byte method fails because the device does not expect a
> stop condition after sending the first byte. When we issue the second
> read, we are getting the first byte again. Net effect is that of the 14
> bits used for the measurement, the 8 most significant bits are correct,
> the lower 6 are not.
>
> None of the smbus read protocols follow the pattern this device requires
> (S Addr Rd [A] Data [A] Data NA P), hence the switch to an i2c receive
> transaction.
>
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> Cc: Daniel Baluta <daniel.baluta@gmail.com>
> ---
>  drivers/iio/humidity/hdc100x.c | 27 +++++++--------------------
>  1 file changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
> index a03832a..643a42d 100644
> --- a/drivers/iio/humidity/hdc100x.c
> +++ b/drivers/iio/humidity/hdc100x.c
> @@ -142,7 +142,7 @@ static int hdc100x_get_measurement(struct hdc100x_data *data,
>         struct i2c_client *client = data->client;
>         int delay = data->adc_int_us[chan->address];
>         int ret;
> -       int val;
> +       u8 buf[2];
>
>         /* start measurement */
>         ret = i2c_smbus_write_byte(client, chan->address);
> @@ -154,26 +154,13 @@ static int hdc100x_get_measurement(struct hdc100x_data *data,
>         /* wait for integration time to pass */
>         usleep_range(delay, delay + 1000);
>
> -       /*
> -        * i2c_smbus_read_word_data cannot() be used here due to the command
> -        * value not being understood and causes NAKs preventing any reading
> -        * from being accessed.
> -        */
> -       ret = i2c_smbus_read_byte(client);
> +       /* read the 2 byte measurement */
> +       ret = i2c_master_recv(data->client, buf, 2);
>         if (ret < 0) {
> -               dev_err(&client->dev, "cannot read high byte measurement");
> +               dev_err(&client->dev, "cannot read sensor data\n");
>                 return ret;
>         }
> -       val = ret << 8;
> -
> -       ret = i2c_smbus_read_byte(client);
> -       if (ret < 0) {
> -               dev_err(&client->dev, "cannot read low byte measurement");
> -               return ret;
> -       }
> -       val |= ret;
> -
> -       return val;
> +       return (int)(buf[0] << 8 | buf[1]);

Pretty sure you don't need to cast to int type here.

>  }
>
>  static int hdc100x_get_heater_status(struct hdc100x_data *data)
> @@ -272,8 +259,8 @@ static int hdc100x_probe(struct i2c_client *client,
>         struct iio_dev *indio_dev;
>         struct hdc100x_data *data;
>
> -       if (!i2c_check_functionality(client->adapter,
> -                               I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_BYTE))
> +       if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA |
> +                                    I2C_FUNC_SMBUS_BYTE | I2C_FUNC_I2C))

Not sure we want to kill smbus support for this device...  iwe should
have two read methods for i2c and smbus (see the PulsedLight LIDAR
driver) in this case.

Ideally I wouldn't have written it with only smbus in mind, but can
kill backwards compatibility unless we have a good reason.

Thanks,

Matt

>                 return -EOPNOTSUPP;
>
>         indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> --
> 2.1.4
>

Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data

[Peter Meerwald-Stadler]

> > Replace the i2c_smbus_read_byte commmands used to retrieve the sensor
> > data with an i2c_master_recv command.
> >
> > The smbus read byte method fails because the device does not expect a
> > stop condition after sending the first byte. When we issue the second
> > read, we are getting the first byte again. Net effect is that of the 14
> > bits used for the measurement, the 8 most significant bits are correct,
> > the lower 6 are not.
> >
> > None of the smbus read protocols follow the pattern this device requires
> > (S Addr Rd [A] Data [A] Data NA P), hence the switch to an i2c receive
> > transaction.
> >
> > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > Cc: Daniel Baluta <daniel.baluta@gmail.com>
> > ---
> >  drivers/iio/humidity/hdc100x.c | 27 +++++++--------------------
> >  1 file changed, 7 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
> > index a03832a..643a42d 100644
> > --- a/drivers/iio/humidity/hdc100x.c
> > +++ b/drivers/iio/humidity/hdc100x.c
> > @@ -142,7 +142,7 @@ static int hdc100x_get_measurement(struct hdc100x_data *data,
> >         struct i2c_client *client = data->client;
> >         int delay = data->adc_int_us[chan->address];
> >         int ret;
> > -       int val;
> > +       u8 buf[2];

__le16 val;

> >
> >         /* start measurement */
> >         ret = i2c_smbus_write_byte(client, chan->address);
> > @@ -154,26 +154,13 @@ static int hdc100x_get_measurement(struct hdc100x_data *data,
> >         /* wait for integration time to pass */
> >         usleep_range(delay, delay + 1000);
> >
> > -       /*
> > -        * i2c_smbus_read_word_data cannot() be used here due to the command
> > -        * value not being understood and causes NAKs preventing any reading
> > -        * from being accessed.
> > -        */
> > -       ret = i2c_smbus_read_byte(client);
> > +       /* read the 2 byte measurement */
> > +       ret = i2c_master_recv(data->client, buf, 2);
> >         if (ret < 0) {
> > -               dev_err(&client->dev, "cannot read high byte measurement");
> > +               dev_err(&client->dev, "cannot read sensor data\n");
> >                 return ret;
> >         }
> > -       val = ret << 8;
> > -
> > -       ret = i2c_smbus_read_byte(client);
> > -       if (ret < 0) {
> > -               dev_err(&client->dev, "cannot read low byte measurement");
> > -               return ret;
> > -       }
> > -       val |= ret;
> > -
> > -       return val;
> > +       return (int)(buf[0] << 8 | buf[1]);
> 
> Pretty sure you don't need to cast to int type here.

return le16_to_cpu(val);

> 
> >  }
> >
> >  static int hdc100x_get_heater_status(struct hdc100x_data *data)
> > @@ -272,8 +259,8 @@ static int hdc100x_probe(struct i2c_client *client,
> >         struct iio_dev *indio_dev;
> >         struct hdc100x_data *data;
> >
> > -       if (!i2c_check_functionality(client->adapter,
> > -                               I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_BYTE))
> > +       if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA |
> > +                                    I2C_FUNC_SMBUS_BYTE | I2C_FUNC_I2C))
> 
> Not sure we want to kill smbus support for this device...  iwe should
> have two read methods for i2c and smbus (see the PulsedLight LIDAR
> driver) in this case.
> 
> Ideally I wouldn't have written it with only smbus in mind, but can
> kill backwards compatibility unless we have a good reason.
> 
> Thanks,
> 
> Matt
> 
> >                 return -EOPNOTSUPP;
> >
> >         indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > --
> > 2.1.4
> >
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data

[Matt Ranostay]

[-- Attachment #1: Type: text/plain, Size: 4073 bytes --]

On Wed, Aug 3, 2016 at 10:19 PM, Peter Meerwald-Stadler <pmeerw@pmeerw.net>
wrote:

>
> > > Replace the i2c_smbus_read_byte commmands used to retrieve the sensor
> > > data with an i2c_master_recv command.
> > >
> > > The smbus read byte method fails because the device does not expect a
> > > stop condition after sending the first byte. When we issue the second
> > > read, we are getting the first byte again. Net effect is that of the 14
> > > bits used for the measurement, the 8 most significant bits are correct,
> > > the lower 6 are not.
> > >
> > > None of the smbus read protocols follow the pattern this device
> requires
> > > (S Addr Rd [A] Data [A] Data NA P), hence the switch to an i2c receive
> > > transaction.
> > >
> > > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > > Cc: Daniel Baluta <daniel.baluta@gmail.com>
> > > ---
> > >  drivers/iio/humidity/hdc100x.c | 27 +++++++--------------------
> > >  1 file changed, 7 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/iio/humidity/hdc100x.c
> b/drivers/iio/humidity/hdc100x.c
> > > index a03832a..643a42d 100644
> > > --- a/drivers/iio/humidity/hdc100x.c
> > > +++ b/drivers/iio/humidity/hdc100x.c
> > > @@ -142,7 +142,7 @@ static int hdc100x_get_measurement(struct
> hdc100x_data *data,
> > >         struct i2c_client *client = data->client;
> > >         int delay = data->adc_int_us[chan->address];
> > >         int ret;
> > > -       int val;
> > > +       u8 buf[2];
>
> __le16 val;
>
> > >
> > >         /* start measurement */
> > >         ret = i2c_smbus_write_byte(client, chan->address);
> > > @@ -154,26 +154,13 @@ static int hdc100x_get_measurement(struct
> hdc100x_data *data,
> > >         /* wait for integration time to pass */
> > >         usleep_range(delay, delay + 1000);
> > >
> > > -       /*
> > > -        * i2c_smbus_read_word_data cannot() be used here due to the
> command
> > > -        * value not being understood and causes NAKs preventing any
> reading
> > > -        * from being accessed.
> > > -        */
> > > -       ret = i2c_smbus_read_byte(client);
> > > +       /* read the 2 byte measurement */
> > > +       ret = i2c_master_recv(data->client, buf, 2);
> > >         if (ret < 0) {
> > > -               dev_err(&client->dev, "cannot read high byte
> measurement");
> > > +               dev_err(&client->dev, "cannot read sensor data\n");
> > >                 return ret;
> > >         }
> > > -       val = ret << 8;
> > > -
> > > -       ret = i2c_smbus_read_byte(client);
> > > -       if (ret < 0) {
> > > -               dev_err(&client->dev, "cannot read low byte
> measurement");
> > > -               return ret;
> > > -       }
> > > -       val |= ret;
> > > -
> > > -       return val;
> > > +       return (int)(buf[0] << 8 | buf[1]);
> >
> > Pretty sure you don't need to cast to int type here.
>
> return le16_to_cpu(val);
>
>
You mean le16_to_cpu(&buf)  I assume?


> >
> > >  }
> > >
> > >  static int hdc100x_get_heater_status(struct hdc100x_data *data)
> > > @@ -272,8 +259,8 @@ static int hdc100x_probe(struct i2c_client *client,
> > >         struct iio_dev *indio_dev;
> > >         struct hdc100x_data *data;
> > >
> > > -       if (!i2c_check_functionality(client->adapter,
> > > -                               I2C_FUNC_SMBUS_WORD_DATA |
> I2C_FUNC_SMBUS_BYTE))
> > > +       if (!i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_WORD_DATA |
> > > +                                    I2C_FUNC_SMBUS_BYTE |
> I2C_FUNC_I2C))
> >
> > Not sure we want to kill smbus support for this device...  iwe should
> > have two read methods for i2c and smbus (see the PulsedLight LIDAR
> > driver) in this case.
> >
> > Ideally I wouldn't have written it with only smbus in mind, but can
> > kill backwards compatibility unless we have a good reason.
> >
> > Thanks,
> >
> > Matt
> >
> > >                 return -EOPNOTSUPP;
> > >
> > >         indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > > --
> > > 2.1.4
> > >
> >
>
> --
>
> Peter Meerwald-Stadler
> +43-664-2444418 (mobile)
>

[-- Attachment #2: Type: text/html, Size: 6055 bytes --]

Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data

[Alison Schofield]

On Wed, Aug 03, 2016 at 10:50:54PM -0700, Matt Ranostay wrote:
> On Wed, Aug 3, 2016 at 10:19 PM, Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> wrote:
> 
> >
> > > > Replace the i2c_smbus_read_byte commmands used to retrieve the sensor
> > > > data with an i2c_master_recv command.
> > > >
> > > > The smbus read byte method fails because the device does not expect a
> > > > stop condition after sending the first byte. When we issue the second
> > > > read, we are getting the first byte again. Net effect is that of the 14
> > > > bits used for the measurement, the 8 most significant bits are correct,
> > > > the lower 6 are not.
> > > >
> > > > None of the smbus read protocols follow the pattern this device
> > requires
> > > > (S Addr Rd [A] Data [A] Data NA P), hence the switch to an i2c receive
> > > > transaction.
> > > >
> > > > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > > > Cc: Daniel Baluta <daniel.baluta@gmail.com>
> > > > ---
> > > >  drivers/iio/humidity/hdc100x.c | 27 +++++++--------------------
> > > >  1 file changed, 7 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/humidity/hdc100x.c
> > b/drivers/iio/humidity/hdc100x.c
> > > > index a03832a..643a42d 100644
> > > > --- a/drivers/iio/humidity/hdc100x.c
> > > > +++ b/drivers/iio/humidity/hdc100x.c
> > > > @@ -142,7 +142,7 @@ static int hdc100x_get_measurement(struct
> > hdc100x_data *data,
> > > >         struct i2c_client *client = data->client;
> > > >         int delay = data->adc_int_us[chan->address];
> > > >         int ret;
> > > > -       int val;
> > > > +       u8 buf[2];
> >
> > __le16 val;
> >
> > > >
> > > >         /* start measurement */
> > > >         ret = i2c_smbus_write_byte(client, chan->address);
> > > > @@ -154,26 +154,13 @@ static int hdc100x_get_measurement(struct
> > hdc100x_data *data,
> > > >         /* wait for integration time to pass */
> > > >         usleep_range(delay, delay + 1000);
> > > >
> > > > -       /*
> > > > -        * i2c_smbus_read_word_data cannot() be used here due to the
> > command
> > > > -        * value not being understood and causes NAKs preventing any
> > reading
> > > > -        * from being accessed.
> > > > -        */
> > > > -       ret = i2c_smbus_read_byte(client);
> > > > +       /* read the 2 byte measurement */
> > > > +       ret = i2c_master_recv(data->client, buf, 2);
> > > >         if (ret < 0) {
> > > > -               dev_err(&client->dev, "cannot read high byte
> > measurement");
> > > > +               dev_err(&client->dev, "cannot read sensor data\n");
> > > >                 return ret;
> > > >         }
> > > > -       val = ret << 8;
> > > > -
> > > > -       ret = i2c_smbus_read_byte(client);
> > > > -       if (ret < 0) {
> > > > -               dev_err(&client->dev, "cannot read low byte
> > measurement");
> > > > -               return ret;
> > > > -       }
> > > > -       val |= ret;
> > > > -
> > > > -       return val;
> > > > +       return (int)(buf[0] << 8 | buf[1]);
> > >
> > > Pretty sure you don't need to cast to int type here.
> >
> > return le16_to_cpu(val);
> >
> >
> You mean le16_to_cpu(&buf)  I assume?
> 
> 
> > >
> > > >  }
> > > >
> > > >  static int hdc100x_get_heater_status(struct hdc100x_data *data)
> > > > @@ -272,8 +259,8 @@ static int hdc100x_probe(struct i2c_client *client,
> > > >         struct iio_dev *indio_dev;
> > > >         struct hdc100x_data *data;
> > > >
> > > > -       if (!i2c_check_functionality(client->adapter,
> > > > -                               I2C_FUNC_SMBUS_WORD_DATA |
> > I2C_FUNC_SMBUS_BYTE))
> > > > +       if (!i2c_check_functionality(client->adapter,
> > I2C_FUNC_SMBUS_WORD_DATA |
> > > > +                                    I2C_FUNC_SMBUS_BYTE |
> > I2C_FUNC_I2C))
> > >
> > > Not sure we want to kill smbus support for this device...  iwe should
> > > have two read methods for i2c and smbus (see the PulsedLight LIDAR
> > > driver) in this case.
> > >
> > > Ideally I wouldn't have written it with only smbus in mind, but can
> > > kill backwards compatibility unless we have a good reason.
> > >
> > > Thanks,
> > >
> > > Matt

Thanks for the reviews. Let me clarify:

This is a fix for a bug in the current driver.  See the changelog.

The relationship between this patch and my triggered-buffer work
is that I found this bug while trying to do just what you say above.
I tried to have 2 methods for reading data - smbus and plain i2c. 
That's the point where I found that smbus reads do not work, never did.

I know we want to stay on the smbus, so I'm looking for suggestions.
As I noted in changelog and my 'smbus help' email - every defined smbus
read command fails.  

alisons

> > >
> > > >                 return -EOPNOTSUPP;
> > > >
> > > >         indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > > > --
> > > > 2.1.4
> > > >
> > >
> >
> > --
> >
> > Peter Meerwald-Stadler
> > +43-664-2444418 (mobile)
> >

Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data

[Matt Ranostay]

On Thu, Aug 4, 2016 at 8:35 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> On Wed, Aug 03, 2016 at 10:50:54PM -0700, Matt Ranostay wrote:
>> On Wed, Aug 3, 2016 at 10:19 PM, Peter Meerwald-Stadler <pmeerw@pmeerw.net>
>> wrote:
>>
>> >
>> > > > Replace the i2c_smbus_read_byte commmands used to retrieve the sensor
>> > > > data with an i2c_master_recv command.
>> > > >
>> > > > The smbus read byte method fails because the device does not expect a
>> > > > stop condition after sending the first byte. When we issue the second
>> > > > read, we are getting the first byte again. Net effect is that of the 14
>> > > > bits used for the measurement, the 8 most significant bits are correct,
>> > > > the lower 6 are not.
>> > > >
>> > > > None of the smbus read protocols follow the pattern this device
>> > requires
>> > > > (S Addr Rd [A] Data [A] Data NA P), hence the switch to an i2c receive
>> > > > transaction.
>> > > >
>> > > > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
>> > > > Cc: Daniel Baluta <daniel.baluta@gmail.com>
>> > > > ---
>> > > >  drivers/iio/humidity/hdc100x.c | 27 +++++++--------------------
>> > > >  1 file changed, 7 insertions(+), 20 deletions(-)
>> > > >
>> > > > diff --git a/drivers/iio/humidity/hdc100x.c
>> > b/drivers/iio/humidity/hdc100x.c
>> > > > index a03832a..643a42d 100644
>> > > > --- a/drivers/iio/humidity/hdc100x.c
>> > > > +++ b/drivers/iio/humidity/hdc100x.c
>> > > > @@ -142,7 +142,7 @@ static int hdc100x_get_measurement(struct
>> > hdc100x_data *data,
>> > > >         struct i2c_client *client = data->client;
>> > > >         int delay = data->adc_int_us[chan->address];
>> > > >         int ret;
>> > > > -       int val;
>> > > > +       u8 buf[2];
>> >
>> > __le16 val;
>> >
>> > > >
>> > > >         /* start measurement */
>> > > >         ret = i2c_smbus_write_byte(client, chan->address);
>> > > > @@ -154,26 +154,13 @@ static int hdc100x_get_measurement(struct
>> > hdc100x_data *data,
>> > > >         /* wait for integration time to pass */
>> > > >         usleep_range(delay, delay + 1000);
>> > > >
>> > > > -       /*
>> > > > -        * i2c_smbus_read_word_data cannot() be used here due to the
>> > command
>> > > > -        * value not being understood and causes NAKs preventing any
>> > reading
>> > > > -        * from being accessed.
>> > > > -        */
>> > > > -       ret = i2c_smbus_read_byte(client);
>> > > > +       /* read the 2 byte measurement */
>> > > > +       ret = i2c_master_recv(data->client, buf, 2);
>> > > >         if (ret < 0) {
>> > > > -               dev_err(&client->dev, "cannot read high byte
>> > measurement");
>> > > > +               dev_err(&client->dev, "cannot read sensor data\n");
>> > > >                 return ret;
>> > > >         }
>> > > > -       val = ret << 8;
>> > > > -
>> > > > -       ret = i2c_smbus_read_byte(client);
>> > > > -       if (ret < 0) {
>> > > > -               dev_err(&client->dev, "cannot read low byte
>> > measurement");
>> > > > -               return ret;
>> > > > -       }
>> > > > -       val |= ret;
>> > > > -
>> > > > -       return val;
>> > > > +       return (int)(buf[0] << 8 | buf[1]);
>> > >
>> > > Pretty sure you don't need to cast to int type here.
>> >
>> > return le16_to_cpu(val);
>> >
>> >
>> You mean le16_to_cpu(&buf)  I assume?
>>
>>
>> > >
>> > > >  }
>> > > >
>> > > >  static int hdc100x_get_heater_status(struct hdc100x_data *data)
>> > > > @@ -272,8 +259,8 @@ static int hdc100x_probe(struct i2c_client *client,
>> > > >         struct iio_dev *indio_dev;
>> > > >         struct hdc100x_data *data;
>> > > >
>> > > > -       if (!i2c_check_functionality(client->adapter,
>> > > > -                               I2C_FUNC_SMBUS_WORD_DATA |
>> > I2C_FUNC_SMBUS_BYTE))
>> > > > +       if (!i2c_check_functionality(client->adapter,
>> > I2C_FUNC_SMBUS_WORD_DATA |
>> > > > +                                    I2C_FUNC_SMBUS_BYTE |
>> > I2C_FUNC_I2C))
>> > >
>> > > Not sure we want to kill smbus support for this device...  iwe should
>> > > have two read methods for i2c and smbus (see the PulsedLight LIDAR
>> > > driver) in this case.
>> > >
>> > > Ideally I wouldn't have written it with only smbus in mind, but can
>> > > kill backwards compatibility unless we have a good reason.
>> > >
>> > > Thanks,
>> > >
>> > > Matt
>
> Thanks for the reviews. Let me clarify:
>
> This is a fix for a bug in the current driver.  See the changelog.
>
> The relationship between this patch and my triggered-buffer work
> is that I found this bug while trying to do just what you say above.
> I tried to have 2 methods for reading data - smbus and plain i2c.
> That's the point where I found that smbus reads do not work, never did.
>
> I know we want to stay on the smbus, so I'm looking for suggestions.
> As I noted in changelog and my 'smbus help' email - every defined smbus
> read command fails.

Of course this depends on what dev board you are using and if the i2c
controller supports both.

Ideally you check for i2c support first then point to that xfer
transfer function for it, and then check for smbus support. Suspect
you did something similar to ->
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c?id=refs/tags/v4.7#n274
, correct?

Thanks,

Matt

>
> alisons
>
>> > >
>> > > >                 return -EOPNOTSUPP;
>> > > >
>> > > >         indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> > > > --
>> > > > 2.1.4
>> > > >
>> > >
>> >
>> > --
>> >
>> > Peter Meerwald-Stadler
>> > +43-664-2444418 (mobile)
>> >

Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data

[Alison Schofield]

On Thu, Aug 04, 2016 at 03:21:13PM -0700, Matt Ranostay wrote:
> On Thu, Aug 4, 2016 at 8:35 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> > On Wed, Aug 03, 2016 at 10:50:54PM -0700, Matt Ranostay wrote:
> >> On Wed, Aug 3, 2016 at 10:19 PM, Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> >> wrote:
> >>
> >> >
> >> > > > Replace the i2c_smbus_read_byte commmands used to retrieve the sensor
> >> > > > data with an i2c_master_recv command.
> >> > > >
> >> > > > The smbus read byte method fails because the device does not expect a
> >> > > > stop condition after sending the first byte. When we issue the second
> >> > > > read, we are getting the first byte again. Net effect is that of the 14
> >> > > > bits used for the measurement, the 8 most significant bits are correct,
> >> > > > the lower 6 are not.
> >> > > >
> >> > > > None of the smbus read protocols follow the pattern this device
> >> > requires
> >> > > > (S Addr Rd [A] Data [A] Data NA P), hence the switch to an i2c receive
> >> > > > transaction.
> >> > > >
> >> > > > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> >> > > > Cc: Daniel Baluta <daniel.baluta@gmail.com>
> >> > > > ---
> >> > > >  drivers/iio/humidity/hdc100x.c | 27 +++++++--------------------
> >> > > >  1 file changed, 7 insertions(+), 20 deletions(-)
> >> > > >
> >> > > > diff --git a/drivers/iio/humidity/hdc100x.c
> >> > b/drivers/iio/humidity/hdc100x.c
> >> > > > index a03832a..643a42d 100644
> >> > > > --- a/drivers/iio/humidity/hdc100x.c
> >> > > > +++ b/drivers/iio/humidity/hdc100x.c
> >> > > > @@ -142,7 +142,7 @@ static int hdc100x_get_measurement(struct
> >> > hdc100x_data *data,
> >> > > >         struct i2c_client *client = data->client;
> >> > > >         int delay = data->adc_int_us[chan->address];
> >> > > >         int ret;
> >> > > > -       int val;
> >> > > > +       u8 buf[2];
> >> >
> >> > __le16 val;
> >> >
> >> > > >
> >> > > >         /* start measurement */
> >> > > >         ret = i2c_smbus_write_byte(client, chan->address);
> >> > > > @@ -154,26 +154,13 @@ static int hdc100x_get_measurement(struct
> >> > hdc100x_data *data,
> >> > > >         /* wait for integration time to pass */
> >> > > >         usleep_range(delay, delay + 1000);
> >> > > >
> >> > > > -       /*
> >> > > > -        * i2c_smbus_read_word_data cannot() be used here due to the
> >> > command
> >> > > > -        * value not being understood and causes NAKs preventing any
> >> > reading
> >> > > > -        * from being accessed.
> >> > > > -        */
> >> > > > -       ret = i2c_smbus_read_byte(client);
> >> > > > +       /* read the 2 byte measurement */
> >> > > > +       ret = i2c_master_recv(data->client, buf, 2);
> >> > > >         if (ret < 0) {
> >> > > > -               dev_err(&client->dev, "cannot read high byte
> >> > measurement");
> >> > > > +               dev_err(&client->dev, "cannot read sensor data\n");
> >> > > >                 return ret;
> >> > > >         }
> >> > > > -       val = ret << 8;
> >> > > > -
> >> > > > -       ret = i2c_smbus_read_byte(client);
> >> > > > -       if (ret < 0) {
> >> > > > -               dev_err(&client->dev, "cannot read low byte
> >> > measurement");
> >> > > > -               return ret;
> >> > > > -       }
> >> > > > -       val |= ret;
> >> > > > -
> >> > > > -       return val;
> >> > > > +       return (int)(buf[0] << 8 | buf[1]);
> >> > >
> >> > > Pretty sure you don't need to cast to int type here.
> >> >
> >> > return le16_to_cpu(val);
> >> >
> >> >
> >> You mean le16_to_cpu(&buf)  I assume?
> >>
> >>
> >> > >
> >> > > >  }
> >> > > >
> >> > > >  static int hdc100x_get_heater_status(struct hdc100x_data *data)
> >> > > > @@ -272,8 +259,8 @@ static int hdc100x_probe(struct i2c_client *client,
> >> > > >         struct iio_dev *indio_dev;
> >> > > >         struct hdc100x_data *data;
> >> > > >
> >> > > > -       if (!i2c_check_functionality(client->adapter,
> >> > > > -                               I2C_FUNC_SMBUS_WORD_DATA |
> >> > I2C_FUNC_SMBUS_BYTE))
> >> > > > +       if (!i2c_check_functionality(client->adapter,
> >> > I2C_FUNC_SMBUS_WORD_DATA |
> >> > > > +                                    I2C_FUNC_SMBUS_BYTE |
> >> > I2C_FUNC_I2C))
> >> > >
> >> > > Not sure we want to kill smbus support for this device...  iwe should
> >> > > have two read methods for i2c and smbus (see the PulsedLight LIDAR
> >> > > driver) in this case.
> >> > >
> >> > > Ideally I wouldn't have written it with only smbus in mind, but can
> >> > > kill backwards compatibility unless we have a good reason.
> >> > >
> >> > > Thanks,
> >> > >
> >> > > Matt
> >
> > Thanks for the reviews. Let me clarify:
> >
> > This is a fix for a bug in the current driver.  See the changelog.
> >
> > The relationship between this patch and my triggered-buffer work
> > is that I found this bug while trying to do just what you say above.
> > I tried to have 2 methods for reading data - smbus and plain i2c.
> > That's the point where I found that smbus reads do not work, never did.
> >
> > I know we want to stay on the smbus, so I'm looking for suggestions.
> > As I noted in changelog and my 'smbus help' email - every defined smbus
> > read command fails.
> 
> Of course this depends on what dev board you are using and if the i2c
> controller supports both.
> 
> Ideally you check for i2c support first then point to that xfer
> transfer function for it, and then check for smbus support. Suspect
> you did something similar to ->
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c?id=refs/tags/v4.7#n274
> , correct?

No, that's not what this bug & patch is about.

This patch is about the fact that we are reading the temp &
humid data registers incorrectly in the current driver and
we are exposing incorrect numbers via sysfs raw reads.

I have tried by testing and inspection each read protocol.
It NAKs all of them - except for read_byte. Problem with
read_byte is that we are always getting the MSB.   We do 2
consecutive read_bytes, expecting MSB,LSB, but get MSB,MSB.

I have been doing msmts w this driver for quite awhile, and
to my human eye they looked reasonable in integer format.
But, once I started looking at the byte level, to test moving
those bytes into buffers, I saw the unmistakable pattern: 
MSB == LSB always.

Sorry to be the bearer of bad news.  I still hold out hope that
we can fix this with some smbus magic and not 'kill backward
compatibility'.  

Who's the resident SMBUS expert(s)?

I don't have a session logged, but I can set up and send a 
demo if wanted.  Let me know.

alisons

> 
> Thanks,
> 
> Matt > 
> >
> > alisons
> >
> >> > >
> >> > > >                 return -EOPNOTSUPP;
> >> > > >
> >> > > >         indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> >> > > > --
> >> > > > 2.1.4
> >> > > >
> >> > >
> >> >
> >> > --
> >> >
> >> > Peter Meerwald-Stadler
> >> > +43-664-2444418 (mobile)
> >> >

Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data

[Lars-Peter Clausen]

On 08/05/2016 01:37 AM, Alison Schofield wrote:
[...]
> 
> No, that's not what this bug & patch is about.
> 
> This patch is about the fact that we are reading the temp &
> humid data registers incorrectly in the current driver and
> we are exposing incorrect numbers via sysfs raw reads.
> 
> I have tried by testing and inspection each read protocol.
> It NAKs all of them - except for read_byte. Problem with
> read_byte is that we are always getting the MSB.   We do 2
> consecutive read_bytes, expecting MSB,LSB, but get MSB,MSB.
> 
> I have been doing msmts w this driver for quite awhile, and
> to my human eye they looked reasonable in integer format.
> But, once I started looking at the byte level, to test moving
> those bytes into buffers, I saw the unmistakable pattern: 
> MSB == LSB always.
> 
> Sorry to be the bearer of bad news.  I still hold out hope that
> we can fix this with some smbus magic and not 'kill backward
> compatibility'.  

If the part is not SMBUS compatible I don't see a reason to try to force it.
It's also not affecting backwards compatibility, the driver never worked
correctly in the first place with SMBUS only, so there is no regression.
After the patch it sill does not work.