Re: [PATCH v2 4/4] iio: light: ltr501: Add support for the LTR-659PS-01 proximity sensor

[Jonathan Cameron]

On Sat, 7 Nov 2026 12:15:07 +0200
Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:

> сб, 7 лист. 2026 р. о 05:33 Jonathan Cameron <jic23@xxxxxxxxxx> пише:
> >
> > On Fri,  6 Nov 2026 11:19:43 +0200
> > Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:
> >  
> > > Add support for the Lite-On LTR-659PS-01 proximity sensor. This device is
> > > nearly identical to the LTR-559ALS-01, but omits all elements related to
> > > ambient light sensing.
> > >
> > > Add a no_als_support flag, similar to how no_irq_support is used, to
> > > isolate ALS register access for the LTR-659PS-01, since ALS registers are
> > > marked as reserved and interactions with them are discouraged.
> > >
> > > The ltr501_trigger_handler relied on the scan index to identify channels.
> > > This approach is fragile regarding channel status flags and can break if
> > > the index is not strictly set. Switch to checking the channel type
> > > instead.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > > ---
> > >  drivers/iio/light/ltr501.c | 73 +++++++++++++++++++++++++++++++-------
> > >  1 file changed, 61 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> > > index d5ce6468a229b..e7762115df913 100644
> > > --- a/drivers/iio/light/ltr501.c
> > > +++ b/drivers/iio/light/ltr501.c
> > > @@ -151,6 +151,7 @@ struct ltr501_chip_info {
> > >       u8 als_gain_mask;
> > >       u8 als_gain_shift;
> > >       bool no_irq_support;
> > > +     bool no_als_support;
> > >       struct iio_chan_spec const *channels;
> > >       const int no_channels;
> > >  };
> > > @@ -192,6 +193,11 @@ static bool ltr501_has_irq_support(const struct ltr501_chip_info *info)
> > >       return !info->no_irq_support;
> > >  }
> > >
> > > +static bool ltr501_has_als_support(const struct ltr501_chip_info *info)  
> >
> > Is the helper bring enough value?  The !info->no_als_support
> > where this is used seems clear enough to me.
> >  
> 
> Noted, if you are fine with !info->no_als_support I have no objections.
> 
> > > +{
> > > +     return !info->no_als_support;
> > > +}
> > > +
> > >  static int ltr501_match_samp_freq(const struct ltr501_samp_table *tab,
> > >                                          int len, int val, int val2)
> > >  {
> > > @@ -656,6 +662,29 @@ static const struct iio_chan_spec ltr301_channels[] = {
> > >       IIO_CHAN_SOFT_TIMESTAMP(2),
> > >  };
> > >
> > > +static const struct iio_chan_spec ltr659_channels[] = {
> > > +     {
> > > +             .type = IIO_PROXIMITY,
> > > +             .address = LTR501_PS_DATA,
> > > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > +                                   BIT(IIO_CHAN_INFO_SCALE) |
> > > +                                   BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > +             .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE) |
> > > +                                             BIT(IIO_CHAN_INFO_SAMP_FREQ),  
> >
> > The sampling frequency applies to both channels here. I'd just make that
> > info_mask_shared_by_all
> >  
> 
> so IIO_CHAN_INFO_SCALE is separate
> and IIO_CHAN_INFO_SAMP_FREQ is shared_by_all
> 
> Noted.
> 
Roughly speaking look at what is being described and consider what it applies to.

Here the timestamp is grabbed for each single channel read, so sampling_frequency
it applies to the timestamp as well as the channel.  There are messier corners
with sampling for devices with more channels where it would be per channel in
the case that they are sampled one by one.
That usually indicates the sampling frequency of a scan is 1/(1/sampA + 1/sampB ...)
If the sampling of a scan is independent of channels enabled (because we read
them all anyway, parallel read out in the hardware or the sampling is clocked at
a rate such that they all get sampled in less time than the inter sample frequency
then sampling_frequency may still be shared_by_all.

The scale doesn't apply to the timestamp.  Now you only have one channel
so you could use shared_by_type, but for this I'd just go with most common
pattern for related drivers and separate is also a valid ABI choice.

Userspace should always cope with figuring out whether the broader scoped
parameters apply to a specific channel - so separate and shared_by_type
are equivalent when there is only one channel of the type.

Jonathan


> 
> > > +             .scan_index = 0,
> > > +             .scan_type = {
> > > +                     .sign = 'u',
> > > +                     .realbits = 11,
> > > +                     .storagebits = 16,
> > > +                     .endianness = IIO_CPU,
> > > +             },
> > > +             .event_spec = ltr501_pxs_event_spec,
> > > +             .num_event_specs = ARRAY_SIZE(ltr501_pxs_event_spec),
> > > +             .ext_info = ltr501_ext_info,
> > > +     },
> > > +     IIO_CHAN_SOFT_TIMESTAMP(1),
> > > +};  
> >