Re: [PATCH v2 2/4] iio: light: ltr501: Switch to modern match mechanism

[Jonathan Cameron]

On Fri,  6 Nov 2026 11:19:41 +0200
Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:

> Remove the chip enum, break the chip list structure into individual
> elements, and pass each element using i2c_get_match_data().

Sashiko calls out that for ACPI this results in an ABI change.
The original code did the ABI wrong but that unfortunately doesn't
mean we can change this one as it is an artefact of a time when
we weren't so careful about those names.

So you'll need to put special handling back in for the ACPI parts
and that name. Some other things in the existing code noted inline
that would be nice to clean up whilst you are here.

> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> ---
>  drivers/iio/light/ltr501.c | 165 +++++++++++++++++--------------------
>  1 file changed, 74 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index cc5d7af44d77c..d5ce6468a229b 100644
> --- a/drivers/iio/light/ltr501.c
> +++ b/drivers/iio/light/ltr501.c

> +static const struct ltr501_chip_info ltr301_chip_info_tbl = {
> +	.partid = 0x08,
> +	.als_gain = ltr501_als_gain_tbl,
> +	.als_gain_tbl_size = ARRAY_SIZE(ltr501_als_gain_tbl),
> +	.als_mode_active = BIT(0) | BIT(1),
> +	.als_gain_mask = BIT(3),
> +	.als_gain_shift = 3,
> +	.channels = ltr301_channels,
> +	.no_channels = ARRAY_SIZE(ltr301_channels),
> +};
> +
> +static const struct ltr501_chip_info ltr303_chip_info_tbl = {
> +	.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,
> +	.channels = ltr301_channels,
> +	.no_channels = ARRAY_SIZE(ltr301_channels),
> +};
> +
> +static const struct ltr501_chip_info ltr329_chip_info_tbl = {
> +	.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),

As below.  Same for all the others.  That would be a separate
cleanup patch though -> perhaps do it with getting rid of
the als_gain_shift.

> +	.als_gain_shift = 2,
> +	.no_irq_support = true,
> +	.channels = ltr301_channels,
> +	.no_channels = ARRAY_SIZE(ltr301_channels),
> +};
> +
> +static const struct ltr501_chip_info ltr501_chip_info_tbl = {
> +	.partid = 0x08,
> +	.als_gain = ltr501_als_gain_tbl,
> +	.als_gain_tbl_size = ARRAY_SIZE(ltr501_als_gain_tbl),
> +	.ps_gain = ltr501_ps_gain_tbl,
> +	.ps_gain_tbl_size = ARRAY_SIZE(ltr501_ps_gain_tbl),
> +	.als_mode_active = BIT(0) | BIT(1),
As below
> +	.als_gain_mask = BIT(3),
> +	.als_gain_shift = 3,
> +	.channels = ltr501_channels,
> +	.no_channels = ARRAY_SIZE(ltr501_channels),
> +};
> +
> +static const struct ltr501_chip_info ltr559_chip_info_tbl = {
> +	.partid = 0x09,
> +	.als_gain = ltr559_als_gain_tbl,
> +	.als_gain_tbl_size = ARRAY_SIZE(ltr559_als_gain_tbl),
> +	.ps_gain = ltr559_ps_gain_tbl,
> +	.ps_gain_tbl_size = ARRAY_SIZE(ltr559_ps_gain_tbl),
> +	.als_mode_active = BIT(0),
> +	.als_gain_mask = BIT(2) | BIT(3) | BIT(4),
Is it really 3 separate bits, or GENMASK(4, 2) to
represent a 3 bit shift?  

Whilst you are touching the driver can we get rid of the
separate shift by using field_prep() field_get() using
just the mask?

> +	.als_gain_shift = 2,
> +	.channels = ltr501_channels,
> +	.no_channels = ARRAY_SIZE(ltr501_channels),
>  };
>  
>  static int ltr501_write_contr(struct ltr501_data *data, u8 als_val, u8 ps_val)
> @@ -1393,14 +1387,11 @@ static int ltr501_powerdown(struct ltr501_data *data)
>  
>  static int ltr501_probe(struct i2c_client *client)
>  {
> -	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>  	static const char * const regulator_names[] = { "vdd", "vddio" };
>  	struct ltr501_data *data;
>  	struct iio_dev *indio_dev;
>  	struct regmap *regmap;
> -	const void *ddata = NULL;
> -	int partid, chip_idx;
> -	const char *name;
> +	int partid;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> @@ -1480,18 +1471,10 @@ static int ltr501_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (id) {
> -		name = id->name;
> -		chip_idx = id->driver_data;
> -	} else {
> -		name = iio_get_acpi_device_name_and_data(&client->dev, &ddata);
> -		chip_idx = (intptr_t)ddata;
> -	}
> -	if (!name)
> +	data->chip_info = i2c_get_match_data(client);
> +	if (!data->chip_info)
>  		return -ENODEV;

As above. Unfortunately at this point we'll still need to
do some special handling to match the existing indio_dev->name.

>  
> -	data->chip_info = &ltr501_chip_info_tbl[chip_idx];
> -
>  	if ((partid >> 4) != data->chip_info->partid)
>  		return -ENODEV;
>  
> @@ -1502,7 +1485,7 @@ static int ltr501_probe(struct i2c_client *client)
>  	indio_dev->info = &ltr501_info;
>  	indio_dev->channels = data->chip_info->channels;
>  	indio_dev->num_channels = data->chip_info->no_channels;
> -	indio_dev->name = name;
> +	indio_dev->name = client->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>