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

Nuno Sá <[email protected]> writes:

> On Wed, Jul 15, 2026 at 03:43:10PM +0200, Esben Haabendal wrote:
>> Nuno Sá <[email protected]> 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 <[email protected]>
>> >> ---
>> >
>> > 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á <[email protected]>
>> >
>> >>  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).
>
> Yeps, that´s is why I proposed has_no_* :)

The has_no_irq could also be read as a variable saying if there is an
irq (pending) right now. For that reason, I will go with no_irq_support
instead. I hope you agree with that as well :)

/Esben