Re: [PATCH v1 2/2] iio: light: ltr501: Add support for the LTR-659PS-01 proximity sensor
From: Svyatoslav Ryhel
Date: Sun Nov 01 2026 - 12:56:12 EST
нд, 1 лист. 2026 р. о 19:41 Jonathan Cameron <jic23@xxxxxxxxxx> пише:
>
> On Sun, 1 Nov 2026 16:15:07 +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 (Even the regmap is identical, but ALS registers are
> > marked as reserved and interactions with them are discouraged).
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> Hi Svyatoslav,
>
> Various more general comments on the driver in here. Only real request
> is to not copy the rather ugly existing indentation for new code.
>
> If nothing else comes up in review, I can just tidy that up whilst applying.
> For now I'm going to leave this on list to give others time to take a
> look.
>
I can address some of your suggestions in this series. I would like to
send v2 anyway since Sashiko found a few legit places needing a fix.
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/light/ltr501.c | 58 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 58 insertions(+)
> >
> > diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> > index 302be2fc314c5..5cc1141322ad5 100644
> > --- a/drivers/iio/light/ltr501.c
> > +++ b/drivers/iio/light/ltr501.c
> > @@ -96,6 +96,7 @@ enum {
> > ltr329,
> > ltr501,
> > ltr559,
> > + ltr659,
> > };
> >
> > struct ltr501_gain {
> > @@ -649,6 +650,26 @@ 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),
> > + .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),
> > +};
> > +
> > static int ltr501_read_info_raw(struct ltr501_data *data,
> > struct iio_chan_spec const *chan,
> > int *val)
> > @@ -1178,6 +1199,12 @@ static struct attribute *ltr301_attributes[] = {
> > NULL
> > };
> >
> > +static struct attribute *ltr659_attributes[] = {
> > + &iio_dev_attr_in_proximity_scale_available.dev_attr.attr,
> > + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
> > + NULL
> > +};
> Not something I'm going to ask for as part of this series, but
> if anyone working with this driver has time it would be good to
> move the available handling over to the read_avail() callback
> and masks. This is an old driver and so far not been converted
> to that newer infrastructure.
>
This I don't promise, I will see what I can do. Maybe you have a good example?
> > +
> > static const struct attribute_group ltr501_attribute_group = {
> > .attrs = ltr501_attributes,
> > };
> > @@ -1186,6 +1213,10 @@ static const struct attribute_group ltr301_attribute_group = {
> > .attrs = ltr301_attributes,
> > };
> >
> > +static const struct attribute_group ltr659_attribute_group = {
> > + .attrs = ltr659_attributes,
> > +};
> > +
> > static const struct iio_info ltr501_info_no_irq = {
> > .read_raw = ltr501_read_raw,
> > .write_raw = ltr501_write_raw,
> > @@ -1218,6 +1249,22 @@ static const struct iio_info ltr301_info = {
> > .write_event_config = <r501_write_event_config,
> > };
> >
> > +static const struct iio_info ltr659_info_no_irq = {
> > + .read_raw = ltr501_read_raw,
> > + .write_raw = ltr501_write_raw,
> > + .attrs = <r659_attribute_group,
> > +};
> > +
> > +static const struct iio_info ltr659_info = {
> > + .read_raw = ltr501_read_raw,
> > + .write_raw = ltr501_write_raw,
> > + .attrs = <r659_attribute_group,
> > + .read_event_value = <r501_read_event,
> > + .write_event_value = <r501_write_event,
> > + .read_event_config = <r501_read_event_config,
> > + .write_event_config = <r501_write_event_config,
>
> The indenting of the existing structure is weird and could do with a tidy up.
> I'd suggest just dropping any attempt to force alignment. As is shown
> here, doing that tends to lead to inconsistency over time as a driver
> evolves.
>
This I will address as a separate commit before adding ltr659
> > +};
> > +
> > static const struct ltr501_chip_info ltr501_chip_info_tbl[] = {
> > [ltr301] = {
> > .partid = 0x08,
> > @@ -1283,6 +1330,15 @@ static const struct ltr501_chip_info ltr501_chip_info_tbl[] = {
> > .channels = ltr501_channels,
> > .no_channels = ARRAY_SIZE(ltr501_channels),
> > },
>
> Another nice to have but not a request for this series.
> Would be good to move away from handling these as an array with the enum
> value in the firmware tables. That scales badly and runs into NULL check
> problems when we use the generic firmware accessors (which we should really
> be using here). i2c_get_match_data(). The standard way to do this
> today is to use separate named structures and put pointers to those in the
> tables.
>
This I will address as well.
> > + [ltr659] = {
> > + .partid = 0x09,
> > + .ps_gain = ltr559_ps_gain_tbl,
> > + .ps_gain_tbl_size = ARRAY_SIZE(ltr559_ps_gain_tbl),
> > + .info = <r659_info,
> > + .info_no_irq = <r659_info_no_irq,
> > + .channels = ltr659_channels,
> > + .no_channels = ARRAY_SIZE(ltr659_channels),
> > + },
> > };
> >
> > static int ltr501_write_contr(struct ltr501_data *data, u8 als_val, u8 ps_val)
> > @@ -1635,6 +1691,7 @@ static const struct i2c_device_id ltr501_id[] = {
> > { .name = "ltr329", .driver_data = ltr329 },
> > { .name = "ltr501", .driver_data = ltr501 },
> > { .name = "ltr559", .driver_data = ltr559 },
> > + { .name = "ltr659", .driver_data = ltr659 },
> > { }
> > };
> > MODULE_DEVICE_TABLE(i2c, ltr501_id);
> > @@ -1645,6 +1702,7 @@ static const struct of_device_id ltr501_of_match[] = {
> > { .compatible = "liteon,ltr329", },
> > { .compatible = "liteon,ltr501", },
> > { .compatible = "liteon,ltr559", },
> > + { .compatible = "liteon,ltr659", },
> > { }
> > };
> > MODULE_DEVICE_TABLE(of, ltr501_of_match);
>