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

[Jonathan Cameron]

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

> сб, 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!

It is common practice to ask anyone who is modifying the code to
tidy up inconsistencies, modernise and make minor general improvements.
If nothing else, if someone else makes those improvements within
a short window of you working on the driver I'm going ask you to
test them anyway.

I very rarely make a request for major refactoring because that
burden is unreasonable. This would fall under the scope of what
I'd normally consider reasonable.

Leaving that cleanup for future is fine. Just state that in
you have chosen to do so in the cover letter for the next version.

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

Yes. I think that should work.