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

[Svyatoslav Ryhel]

сб, 7 лист. 2026 р. о 05:30 Jonathan Cameron <jic23@xxxxxxxxxx> пише:
>
> 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.
>

This is all out of scope of my series. I am not a code janitor!

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

const struct i2c_device_id *id = i2c_client_get_device_id(client);

...

indio_dev->name = id ? id->name :
iio_get_acpi_device_name_and_data(&client->dev, NULL);

Will this be acceptible?

> >
> > -     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;
> >