On Thu, Jun 10, 2021 at 3:55 PM Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
>
> From: Oliver Lang <Oliver.Lang@xxxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> ---
> 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
>
>
--
With Best Regards,
Andy Shevchenko
