* [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data @ 2016-08-04 3:33 Alison Schofield 2016-08-04 4:58 ` Matt Ranostay 0 siblings, 1 reply; 8+ messages in thread From: Alison Schofield @ 2016-08-04 3:33 UTC (permalink / raw) To: jic23; +Cc: mranostay, knaack.h, lars, pmeerw, linux-iio, linux-kernel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data 2016-08-04 3:33 [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data Alison Schofield @ 2016-08-04 4:58 ` Matt Ranostay 2016-08-04 5:19 ` Peter Meerwald-Stadler 0 siblings, 1 reply; 8+ messages in thread From: Matt Ranostay @ 2016-08-04 4:58 UTC (permalink / raw) To: Alison Schofield Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data 2016-08-04 4:58 ` Matt Ranostay @ 2016-08-04 5:19 ` Peter Meerwald-Stadler 2016-08-04 5:50 ` Matt Ranostay 0 siblings, 1 reply; 8+ messages in thread From: Peter Meerwald-Stadler @ 2016-08-04 5:19 UTC (permalink / raw) To: Matt Ranostay Cc: Alison Schofield, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, linux-iio, linux-kernel > > 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data 2016-08-04 5:19 ` Peter Meerwald-Stadler @ 2016-08-04 5:50 ` Matt Ranostay 2016-08-04 15:35 ` Alison Schofield 0 siblings, 1 reply; 8+ messages in thread From: Matt Ranostay @ 2016-08-04 5:50 UTC (permalink / raw) To: Peter Meerwald-Stadler Cc: Alison Schofield, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, linux-iio, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data 2016-08-04 5:50 ` Matt Ranostay @ 2016-08-04 15:35 ` Alison Schofield 2016-08-04 22:21 ` Matt Ranostay 0 siblings, 1 reply; 8+ messages in thread From: Alison Schofield @ 2016-08-04 15:35 UTC (permalink / raw) To: Matt Ranostay Cc: Peter Meerwald-Stadler, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, linux-iio, linux-kernel 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) > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data 2016-08-04 15:35 ` Alison Schofield @ 2016-08-04 22:21 ` Matt Ranostay 2016-08-04 23:37 ` Alison Schofield 0 siblings, 1 reply; 8+ messages in thread From: Matt Ranostay @ 2016-08-04 22:21 UTC (permalink / raw) To: Alison Schofield Cc: Peter Meerwald-Stadler, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, linux-iio, linux-kernel 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) >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data 2016-08-04 22:21 ` Matt Ranostay @ 2016-08-04 23:37 ` Alison Schofield 2016-08-05 9:35 ` Lars-Peter Clausen 0 siblings, 1 reply; 8+ messages in thread From: Alison Schofield @ 2016-08-04 23:37 UTC (permalink / raw) To: Matt Ranostay Cc: Peter Meerwald-Stadler, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, linux-iio, linux-kernel 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) > >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data 2016-08-04 23:37 ` Alison Schofield @ 2016-08-05 9:35 ` Lars-Peter Clausen 0 siblings, 0 replies; 8+ messages in thread From: Lars-Peter Clausen @ 2016-08-05 9:35 UTC (permalink / raw) To: Alison Schofield, Matt Ranostay Cc: Peter Meerwald-Stadler, Jonathan Cameron, Hartmut Knaack, linux-iio, linux-kernel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-08-05 9:35 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-04 3:33 [PATCH] iio: humidity: hdc100x: use i2c_master_recv to read sensor data Alison Schofield 2016-08-04 4:58 ` Matt Ranostay 2016-08-04 5:19 ` Peter Meerwald-Stadler 2016-08-04 5:50 ` Matt Ranostay 2016-08-04 15:35 ` Alison Schofield 2016-08-04 22:21 ` Matt Ranostay 2016-08-04 23:37 ` Alison Schofield 2016-08-05 9:35 ` Lars-Peter Clausen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.