[PATCH 1/4] iio: ltr501: fix regmap, initialization of ltr559, endianness and mark structs as const

[PATCH 1/4] iio: ltr501: fix regmap, initialization of ltr559, endianness and mark structs as const

[Marc Kleine-Budde]

Hello,

here are 3 bug-files (probably stable material) and 1 enhancement for
the ltr501 driver.

regards,
Marc

[PATCH 1/4] iio: ltr501: mark register holding upper 8 bits of ALS_DATA{0,1} and PS_DATA as volatile, too

[Marc Kleine-Budde]

The regmap is configured for 8 bit registers, uses a RB-Tree cache and
marks several registers as volatile (i.e. do not cache).

The ALS and PS data registers in the chip are 16 bit wide and spans
two regmap registers. In the current driver only the base register is
marked as volatile, resulting in the upper register only read once.

Further the data sheet notes:

| When the I2C read operation starts, all four ALS data registers are
| locked until the I2C read operation of register 0x8B is completed.

Which results in the registers never update after the 2nd read.

This patch fixes the problem by marking the upper 8 bits of the ALS
and PS registers as volatile, too.

Fixes: 2f2c96338afc ("iio: ltr501: Add regmap support.")
Reported-by: Oliver Lang <Oliver.Lang@gossenmetrawatt.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/iio/light/ltr501.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index b4323d2db0b1..0ed3392a33cf 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -32,9 +32,12 @@
 #define LTR501_PART_ID 0x86
 #define LTR501_MANUFAC_ID 0x87
 #define LTR501_ALS_DATA1 0x88 /* 16-bit, little endian */
+#define LTR501_ALS_DATA1_UPPER 0x89 /* upper 8 bits of LTR501_ALS_DATA1 */
 #define LTR501_ALS_DATA0 0x8a /* 16-bit, little endian */
+#define LTR501_ALS_DATA0_UPPER 0x8b /* upper 8 bits of LTR501_ALS_DATA0 */
 #define LTR501_ALS_PS_STATUS 0x8c
 #define LTR501_PS_DATA 0x8d /* 16-bit, little endian */
+#define LTR501_PS_DATA_UPPER 0x8e /* upper 8 bits of LTR501_PS_DATA */
 #define LTR501_INTR 0x8f /* output mode, polarity, mode */
 #define LTR501_PS_THRESH_UP 0x90 /* 11 bit, ps upper threshold */
 #define LTR501_PS_THRESH_LOW 0x92 /* 11 bit, ps lower threshold */
@@ -1354,9 +1357,12 @@ static bool ltr501_is_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
 	case LTR501_ALS_DATA1:
+	case LTR501_ALS_DATA1_UPPER:
 	case LTR501_ALS_DATA0:
+	case LTR501_ALS_DATA0_UPPER:
 	case LTR501_ALS_PS_STATUS:
 	case LTR501_PS_DATA:
+	case LTR501_PS_DATA_UPPER:
 		return true;
 	default:
 		return false;
-- 
2.30.2

[PATCH 2/4] iio: ltr501: ltr559: fix initialization of LTR501_ALS_CONTR

[Marc Kleine-Budde]

From: Oliver Lang <Oliver.Lang@gossenmetrawatt.com>

The ltr559 chip uses only the lowest bit of the ALS_CONTR register to
configure between active and stand-by mode. In the original driver
BIT(1) is used, which does a software reset instead.

This patch fixes the problem by using BIT(0) as als_mode_active for
the ltr559 chip.

Fixes: 8592a7eefa54 ("iio: ltr501: Add support for ltr559 chip")
Signed-off-by: Oliver Lang <Oliver.Lang@gossenmetrawatt.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/iio/light/ltr501.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 0ed3392a33cf..79898b72fe73 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -1208,7 +1208,7 @@ static struct ltr501_chip_info ltr501_chip_info_tbl[] = {
 		.als_gain_tbl_size = ARRAY_SIZE(ltr559_als_gain_tbl),
 		.ps_gain = ltr559_ps_gain_tbl,
 		.ps_gain_tbl_size = ARRAY_SIZE(ltr559_ps_gain_tbl),
-		.als_mode_active = BIT(1),
+		.als_mode_active = BIT(0),
 		.als_gain_mask = BIT(2) | BIT(3) | BIT(4),
 		.als_gain_shift = 2,
 		.info = &ltr501_info,
-- 
2.30.2

[PATCH 3/4] iio: ltr501: ltr501_read_ps(): add missing endianness conversion

[Marc Kleine-Budde]

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));
 }
 
 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

Re: [PATCH 3/4] iio: ltr501: ltr501_read_ps(): add missing endianness conversion

[Andy Shevchenko]

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


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] iio: ltr501: ltr501_read_ps(): add missing endianness conversion

[Marc Kleine-Budde]

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

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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] iio: ltr501: ltr501_read_ps(): add missing endianness conversion

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

[PATCH 4/4] iio: ltr501: mark ltr501_chip_info as const

[Marc Kleine-Budde]

This patch marks the struct ltr501_chip_info as constant.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/iio/light/ltr501.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 0e3f97ef3cdd..e2b4d20e33dc 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -152,7 +152,7 @@ struct ltr501_chip_info {
 struct ltr501_data {
 	struct i2c_client *client;
 	struct mutex lock_als, lock_ps;
-	struct ltr501_chip_info *chip_info;
+	const struct ltr501_chip_info *chip_info;
 	u8 als_contr, ps_contr;
 	int als_period, ps_period; /* period in micro seconds */
 	struct regmap *regmap;
@@ -734,7 +734,7 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
 {
 	struct ltr501_data *data = iio_priv(indio_dev);
 	int i, ret, freq_val, freq_val2;
-	struct ltr501_chip_info *info = data->chip_info;
+	const struct ltr501_chip_info *info = data->chip_info;
 
 	ret = iio_device_claim_direct_mode(indio_dev);
 	if (ret)
@@ -1081,7 +1081,7 @@ static ssize_t ltr501_show_proximity_scale_avail(struct device *dev,
 						 char *buf)
 {
 	struct ltr501_data *data = iio_priv(dev_to_iio_dev(dev));
-	struct ltr501_chip_info *info = data->chip_info;
+	const struct ltr501_chip_info *info = data->chip_info;
 	ssize_t len = 0;
 	int i;
 
@@ -1103,7 +1103,7 @@ static ssize_t ltr501_show_intensity_scale_avail(struct device *dev,
 						 char *buf)
 {
 	struct ltr501_data *data = iio_priv(dev_to_iio_dev(dev));
-	struct ltr501_chip_info *info = data->chip_info;
+	const struct ltr501_chip_info *info = data->chip_info;
 	ssize_t len = 0;
 	int i;
 
@@ -1183,7 +1183,7 @@ static const struct iio_info ltr301_info = {
 	.write_event_config	= &ltr501_write_event_config,
 };
 
-static struct ltr501_chip_info ltr501_chip_info_tbl[] = {
+static const struct ltr501_chip_info ltr501_chip_info_tbl[] = {
 	[ltr501] = {
 		.partid = 0x08,
 		.als_gain = ltr501_als_gain_tbl,
-- 
2.30.2

Re: [PATCH 1/4] iio: ltr501: fix regmap, initialization of ltr559, endianness and mark structs as const

[Andy Shevchenko]

On Thu, Jun 10, 2021 at 3:55 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> Hello,
>
> here are 3 bug-files (probably stable material) and 1 enhancement for
> the ltr501 driver.
>
> regards,
> Marc

By some reason this is marked as patch 1/4 (should be the cover letter
as patch 0/4).
Hint: --cover-letter parameter will do a template for you (`git
format-patch --cover-letter --thread ...`).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] iio: ltr501: fix regmap, initialization of ltr559, endianness and mark structs as const

[Andy Shevchenko]

On Thu, Jun 10, 2021 at 3:55 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> Hello,
>
> here are 3 bug-files (probably stable material) and 1 enhancement for
> the ltr501 driver.

After addressing comment to patch 3, feel free to add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
to the entire series.

Thanks for fixing the issues!

-- 
With Best Regards,
Andy Shevchenko