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

[Esben Haabendal <esben@geanix.com>]

Nuno Sá <noname.nuno@gmail.com> writes:

> On Wed, Jul 15, 2026 at 02:27:25PM +0200, Esben Haabendal 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, I have a small not below. Kind of personal preference though. But
> what Joshua mentioned should be addressed. With that:
>
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>
>
>>  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
>>
>
> ...
>
>>
>> +	if (!ltr501_has_irq_support(data->chip_info))
>> +		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,
>> +		.channels = ltr301_channels,
>> +		.no_channels = ARRAY_SIZE(ltr301_channels),
>
> Instead of playing the above game with info vs info_no_irq, an explicit
> has_no_irq would probably be better. I mean conceptually if the pointers
> are the same, it could also mean that both are with IRQ support. With
> it, I think it would be safe to leave the .info pointer as NULL as it
> would be always overwritten.
>
> Having said the above, so strong feelings about it so up to you :)

Calling it has_irq would avoid double negation. But we would then have
to set it to true in most of the entries (all except ltr329 for now).

I will give it a spin.

/Esben