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

On Tue, Aug 04, 2026 at 04:44:02PM +0200, Esben Haabendal wrote:
> 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 :)
> 

No strong feeling. I'm fine with it.

- Nuno Sá