Re: [PATCH v2 3/4] iio: light: ltr501: Add ltr329 driver support

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Wed, 15 Jul 2026 14:27:25 +0200
Esben Haabendal <esben@geanix.com> wrote:

> This adds support for the LTR-329ALS-01 chip, which is similar to
> LTR-303ALS-01, except for interrupt, which LTR-329ALS-01 chip does not
> have.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
Hi Esben

A few comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/light/ltr501.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index 7d045be78c6d..379e57ac5f5b 100644
> --- a/drivers/iio/light/ltr501.c
> +++ b/drivers/iio/light/ltr501.c
> @@ -15,6 +15,7 @@
>  #include <linux/delay.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/array_size.h> // for ARRAY_SIZE
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/events.h>
> @@ -94,6 +95,7 @@ enum {
>  	ltr559,
>  	ltr301,
>  	ltr303,
> +	ltr329,
>  };
>  
>  struct ltr501_gain {
> @@ -178,6 +180,11 @@ static const struct ltr501_samp_table ltr501_ps_samp_table[] = {
>  			{500000, 2000000}
>  };
>  
> +static bool ltr501_has_irq_support(const struct ltr501_chip_info *chip_info)
> +{
> +	return chip_info->info != chip_info->info_no_irq;
> +}
> +
>  static int ltr501_match_samp_freq(const struct ltr501_samp_table *tab,
>  					   int len, int val, int val2)
>  {
> @@ -428,6 +435,9 @@ static int ltr501_read_intr_prst(const struct ltr501_data *data,
>  {
>  	int ret, samp_period, prst;
>  
> +	if (!ltr501_has_irq_support(data->chip_info))
> +		return 0;

This is only called in two places. One of those is events infrastructure
that I would assume is not registered.   For the other in _init I'd
push the check to the caller.  Would avoid oddity that we seem to read
this and get an 'all good' return when there is no such thing to read.

> +
>  	switch (type) {
>  	case IIO_INTENSITY:
>  		ret = regmap_field_read(data->reg_als_prst, &prst);
> @@ -466,6 +476,9 @@ static int ltr501_write_intr_prst(struct ltr501_data *data,
>  	int ret, samp_period, new_val;
>  	unsigned long period;
>  
> +	if (!ltr501_has_irq_support(data->chip_info))

This one is called when setting sampling frequency. I'd gate whether
it is called in __ltr501_write_raw() rather than down here for same reason
as the read side.

> +		return 0;
> +
>  	if (val < 0 || val2 < 0)
>  		return -EINVAL;
>  
> @@ -1257,6 +1270,18 @@ static const struct ltr501_chip_info ltr501_chip_info_tbl[] = {
>  		.channels = ltr301_channels,
>  		.no_channels = ARRAY_SIZE(ltr301_channels),
>  	},
> +	[ltr329] = {
> +		.partid = 0x0A,
> +		.als_gain = ltr559_als_gain_tbl,
> +		.als_gain_tbl_size = ARRAY_SIZE(ltr559_als_gain_tbl),
> +		.als_mode_active = BIT(0),
> +		.als_gain_mask = BIT(2) | BIT(3) | BIT(4),
> +		.als_gain_shift = 2,
> +		.info = &ltr301_info_no_irq,
> +		.info_no_irq = &ltr301_info_no_irq,

The suggestion about a flag in your discussion with Nuno makes sense to me.

> +		.channels = ltr301_channels,
> +		.no_channels = ARRAY_SIZE(ltr301_channels),
> +	},
>  };
>  
>  static int ltr501_write_contr(struct ltr501_data *data, u8 als_val, u8 ps_val)
> @@ -1531,6 +1556,12 @@ static int ltr501_probe(struct i2c_client *client)
>  		return ret;
>  
>  	if (client->irq > 0) {
> +		if (!ltr501_has_irq_support(data->chip_info)) {
> +			dev_err(&client->dev, "chip does not support irq\n");
> +			ret = -EINVAL;
> +			goto powerdown_on_error;
> +		}
> +
>  		ret = devm_request_threaded_irq(&client->dev, client->irq,
>  						NULL, ltr501_interrupt_handler,
>  						IRQF_TRIGGER_FALLING |
> @@ -1604,6 +1635,7 @@ static const struct i2c_device_id ltr501_id[] = {
>  	{ .name = "ltr559", .driver_data = ltr559 },
>  	{ .name = "ltr301", .driver_data = ltr301 },
>  	{ .name = "ltr303", .driver_data = ltr303 },
> +	{ .name = "ltr329", .driver_data = ltr329 },

Please put these in numeric order in a precursor patch.

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, ltr501_id);
> @@ -1613,6 +1645,7 @@ static const struct of_device_id ltr501_of_match[] = {
>  	{ .compatible = "liteon,ltr559", },
>  	{ .compatible = "liteon,ltr301", },
>  	{ .compatible = "liteon,ltr303", },
> +	{ .compatible = "liteon,ltr329", },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ltr501_of_match);
>