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

[Nuno Sá]

On Wed, Jul 15, 2026 at 03:43:10PM +0200, Esben Haabendal wrote:
> Nuno Sá <noname.nuno@xxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >
> > 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@xxxxxxxxxx>
> >
> >>  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_* :)

- Nuno Sá

> 
> I will give it a spin.
> 
> /Esben