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

[Jonathan Cameron <jic23@kernel.org>]

On Sun,  1 Nov 2026 16:15:07 +0200
Svyatoslav Ryhel <clamor95@gmail.com> 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@gmail.com>
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.

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.

> +
>  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	= &ltr501_write_event_config,
>  };
>  
> +static const struct iio_info ltr659_info_no_irq = {
> +	.read_raw = ltr501_read_raw,
> +	.write_raw = ltr501_write_raw,
> +	.attrs = &ltr659_attribute_group,
> +};
> +
> +static const struct iio_info ltr659_info = {
> +	.read_raw = ltr501_read_raw,
> +	.write_raw = ltr501_write_raw,
> +	.attrs = &ltr659_attribute_group,
> +	.read_event_value	= &ltr501_read_event,
> +	.write_event_value	= &ltr501_write_event,
> +	.read_event_config	= &ltr501_read_event_config,
> +	.write_event_config	= &ltr501_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.

> +};
> +
>  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.

> +	[ltr659] = {
> +		.partid = 0x09,
> +		.ps_gain = ltr559_ps_gain_tbl,
> +		.ps_gain_tbl_size = ARRAY_SIZE(ltr559_ps_gain_tbl),
> +		.info = &ltr659_info,
> +		.info_no_irq = &ltr659_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);