| Message ID | 20210610125358.2096497-4-mkl@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] iio: ltr501: mark register holding upper 8 bits of ALS_DATA{0,1} and PS_DATA as volatile, too | expand |
On Thu, Jun 10, 2021 at 3:55 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > From: Oliver Lang <Oliver.Lang@gossenmetrawatt.com> > > The PS ADC Channel data is spread over 2 registers in little-endian > form. This patch adds the missing endianness conversion. > > Fixes: 2690be905123 ("iio: Add Lite-On ltr501 ambient light / proximity sensor driver") > Signed-off-by: Oliver Lang <Oliver.Lang@gossenmetrawatt.com> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > --- > drivers/iio/light/ltr501.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c > index 79898b72fe73..0e3f97ef3cdd 100644 > --- a/drivers/iio/light/ltr501.c > +++ b/drivers/iio/light/ltr501.c > @@ -407,20 +407,16 @@ static int ltr501_read_als(const struct ltr501_data *data, __le16 buf[2]) > buf, 2 * sizeof(__le16)); > } > > -static int ltr501_read_ps(const struct ltr501_data *data) > +static int ltr501_read_ps(const struct ltr501_data *data, __le16 *buf) > { > - int ret, status; > + int ret; > > ret = ltr501_drdy(data, LTR501_STATUS_PS_RDY); > if (ret < 0) > return ret; > > - ret = regmap_bulk_read(data->regmap, LTR501_PS_DATA, > - &status, 2); > - if (ret < 0) > - return ret; > - > - return status; > + return regmap_bulk_read(data->regmap, LTR501_PS_DATA, > + buf, sizeof(__le16)); This is slightly weird since we pass a pointer to a buffer of one element (buffer can be longer, but here it's always one element is in use). The original code is better (initially). Also making caller to do endiannes conversion each time is not good. What I suggest is to leave the caller as is and change here only the followng int status ==> __le16 status; ret = regmap_bulk_read(data->regmap, LTR501_PS_DATA, &status, sizeof(status)); ... return le16_to_cpu(status); > } > > static int ltr501_read_intr_prst(const struct ltr501_data *data, > @@ -668,11 +664,11 @@ static int ltr501_read_raw(struct iio_dev *indio_dev, > break; > case IIO_PROXIMITY: > mutex_lock(&data->lock_ps); > - ret = ltr501_read_ps(data); > + ret = ltr501_read_ps(data, buf); > mutex_unlock(&data->lock_ps); > if (ret < 0) > break; > - *val = ret & LTR501_PS_DATA_MASK; > + *val = le16_to_cpu(buf[0]) & LTR501_PS_DATA_MASK; > ret = IIO_VAL_INT; > break; > default: > -- > 2.30.2 > >
On 10.06.2021 16:21:30, Andy Shevchenko wrote: > On Thu, Jun 10, 2021 at 3:55 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > From: Oliver Lang <Oliver.Lang@gossenmetrawatt.com> > > > > The PS ADC Channel data is spread over 2 registers in little-endian > > form. This patch adds the missing endianness conversion. > > > > Fixes: 2690be905123 ("iio: Add Lite-On ltr501 ambient light / proximity sensor driver") > > Signed-off-by: Oliver Lang <Oliver.Lang@gossenmetrawatt.com> > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > > --- > > drivers/iio/light/ltr501.c | 16 ++++++---------- > > 1 file changed, 6 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c > > index 79898b72fe73..0e3f97ef3cdd 100644 > > --- a/drivers/iio/light/ltr501.c > > +++ b/drivers/iio/light/ltr501.c > > @@ -407,20 +407,16 @@ static int ltr501_read_als(const struct ltr501_data *data, __le16 buf[2]) > > buf, 2 * sizeof(__le16)); > > } > > > > -static int ltr501_read_ps(const struct ltr501_data *data) > > +static int ltr501_read_ps(const struct ltr501_data *data, __le16 *buf) > > { > > - int ret, status; > > + int ret; > > > > ret = ltr501_drdy(data, LTR501_STATUS_PS_RDY); > > if (ret < 0) > > return ret; > > > > - ret = regmap_bulk_read(data->regmap, LTR501_PS_DATA, > > - &status, 2); > > - if (ret < 0) > > - return ret; > > - > > - return status; > > + return regmap_bulk_read(data->regmap, LTR501_PS_DATA, > > + buf, sizeof(__le16)); > > This is slightly weird since we pass a pointer to a buffer of one > element (buffer can be longer, but here it's always one element is in > use). The original code is better (initially). Also making caller to > do endiannes conversion each time is not good. We decided to implement the same semantics as ltr501_read_als(), where the caller does the endianness conversion. I'll change the code and send a v2 (with a proper cover letter subject :)) regards, Marc
On Thu, Jun 10, 2021 at 4:31 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > On 10.06.2021 16:21:30, Andy Shevchenko wrote: > > On Thu, Jun 10, 2021 at 3:55 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: ... > > > + int ret; > > > > > > ret = ltr501_drdy(data, LTR501_STATUS_PS_RDY); > > > if (ret < 0) > > > return ret; > > > > > > - ret = regmap_bulk_read(data->regmap, LTR501_PS_DATA, > > > - &status, 2); > > > - if (ret < 0) > > > - return ret; > > > - > > > - return status; > > > + return regmap_bulk_read(data->regmap, LTR501_PS_DATA, > > > + buf, sizeof(__le16)); > > > > This is slightly weird since we pass a pointer to a buffer of one > > element (buffer can be longer, but here it's always one element is in > > use). The original code is better (initially). Also making caller to > > do endiannes conversion each time is not good. > > We decided to implement the same semantics as ltr501_read_als(), where > the caller does the endianness conversion. I'm fully in favour of consistency, but in this case I think it would gain from converting the old code to the above mentioned schema. > I'll change the code and send > a v2 (with a proper cover letter subject :)) Thank you!
diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c index 79898b72fe73..0e3f97ef3cdd 100644 --- a/drivers/iio/light/ltr501.c +++ b/drivers/iio/light/ltr501.c @@ -407,20 +407,16 @@ static int ltr501_read_als(const struct ltr501_data *data, __le16 buf[2]) buf, 2 * sizeof(__le16)); } -static int ltr501_read_ps(const struct ltr501_data *data) +static int ltr501_read_ps(const struct ltr501_data *data, __le16 *buf) { - int ret, status; + int ret; ret = ltr501_drdy(data, LTR501_STATUS_PS_RDY); if (ret < 0) return ret; - ret = regmap_bulk_read(data->regmap, LTR501_PS_DATA, - &status, 2); - if (ret < 0) - return ret; - - return status; + return regmap_bulk_read(data->regmap, LTR501_PS_DATA, + buf, sizeof(__le16)); } static int ltr501_read_intr_prst(const struct ltr501_data *data, @@ -668,11 +664,11 @@ static int ltr501_read_raw(struct iio_dev *indio_dev, break; case IIO_PROXIMITY: mutex_lock(&data->lock_ps); - ret = ltr501_read_ps(data); + ret = ltr501_read_ps(data, buf); mutex_unlock(&data->lock_ps); if (ret < 0) break; - *val = ret & LTR501_PS_DATA_MASK; + *val = le16_to_cpu(buf[0]) & LTR501_PS_DATA_MASK; ret = IIO_VAL_INT; break; default: