Re: [PATCH v3 2/2] mfd: lm3533: convert to use OF

[Svyatoslav Ryhel <clamor95@gmail.com>]

ср, 5 бер. 2025 р. о 15:45 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Fri, 28 Feb 2025 11:30:51 +0200
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > пт, 28 лют. 2025 р. о 10:59 Lee Jones <lee@kernel.org> пише:
> > >
> > > On Mon, 24 Feb 2025, Svyatoslav Ryhel wrote:
> > >
> > > > Remove platform data and fully relay on OF and device tree
> > > > parsing and binding devices.
> > > >
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > ---
> > > >  drivers/iio/light/lm3533-als.c      |  40 ++++---
> > > >  drivers/leds/leds-lm3533.c          |  46 +++++---
> > > >  drivers/mfd/lm3533-core.c           | 159 ++++++++--------------------
> > > >  drivers/video/backlight/lm3533_bl.c |  71 ++++++++++---
> > > >  include/linux/mfd/lm3533.h          |  35 +-----
> > > >  5 files changed, 164 insertions(+), 187 deletions(-)
> > > >
...
> > > >       /* ALS input is always high impedance in PWM-mode. */
> > > > -     if (!pdata->pwm_mode) {
> > > > -             ret = lm3533_als_set_resistor(als, pdata->r_select);
> > > > +     if (!als->pwm_mode) {
> > > > +             ret = lm3533_als_set_resistor(als, als->r_select);
> > >
> > > You're already passing 'als'.
> > >
> > > Just teach lm3533_als_set_resistor that 'r_select' is now contained.
> > >
> >
> > This is not scope of this patchset. I was already accused in too much
> > changes which make it unreadable. This patchset is dedicated to
> > swapping platform data to use of the device tree. NOT improving
> > functions, NOT rewriting arbitrary mechanics. If you feed a need for
> > this change, then propose a followup. I need from this driver only one
> > thing, that it could work with device tree. But it seems that it is
> > better that it just rots in the garbage bin until removed cause no one
> > cared.
>
> This is not an unreasonable request as you added r_select to als.
> Perhaps it belongs in a separate follow up patch.

I have just moved values used in pdata to private structs of each
driver. Without changing names or purpose.

> However
> it is worth remembering the motivation here is that you want get
> this code upstream, the maintainers don't have that motivation.

This driver is already upstream and it is useless and incompatible
with majority of supported devices. Maintainers should encourage those
who try to help and instead we have what? A total discouragement. Well
defined path into nowhere.

>
> Greg KH has given various talks on the different motivations in the
> past. It maybe worth a watch.
>
>
> >
> > > >               if (ret)
> > > >                       return ret;
> > > >       }
> > > > @@ -828,22 +833,16 @@ static const struct iio_info lm3533_als_info = {
> > > >
> > > >  static int lm3533_als_probe(struct platform_device *pdev)
> > > >  {
> > > > -     const struct lm3533_als_platform_data *pdata;
> > > >       struct lm3533 *lm3533;
> > > >       struct lm3533_als *als;
> > > >       struct iio_dev *indio_dev;
> > > > +     u32 val;
> > >
> > > Value of what, potatoes?
> > >
> >
> > Oranges.
>
> A well named variable would avoid need for any discussion of
> what it is the value of.
>

This is temporary placeholder used to get values from the tree and
then pass it driver struct.

> >
> > > >       int ret;
> > > >
> > > >       lm3533 = dev_get_drvdata(pdev->dev.parent);
> > > >       if (!lm3533)
> > > >               return -EINVAL;
> > > >
> > > > -     pdata = dev_get_platdata(&pdev->dev);
> > > > -     if (!pdata) {
> > > > -             dev_err(&pdev->dev, "no platform data\n");
> > > > -             return -EINVAL;
> > > > -     }
> > > > -
> > > >       indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));
> > > >       if (!indio_dev)
> > > >               return -ENOMEM;
> > > > @@ -864,13 +863,21 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > > >
> > > >       platform_set_drvdata(pdev, indio_dev);
> > > >
> > > > +     val = 200 * KILO; /* 200kOhm */
> > >
> > > Better to #define magic numbers; DEFAULT_{DESCRIPTION}_OHMS
> > >
> >
> > Why? that is not needed.
> If this variable had a more useful name there would be no need for
> the comment either.
>
>         val_resitor_ohms = 200 * KILLO;
>
> or similar.
>

So I have to add a "reasonably" named variable for each property I
want to get from device tree? Why? It seems to be a bit of overkill,
no? Maybe I am not aware, have variables stopped being reusable?

> >
> > > > +     device_property_read_u32(&pdev->dev, "ti,resistor-value-ohm", &val);
> > > > +
> > > > +     /* Convert resitance into R_ALS value with 2v / 10uA * R */
> > >
> > > Because ...
> > >
> >
> > BACAUSE the device DOES NOT understand human readable values, only 0s
> > and 1s, hence mOhms must be converted into value lm3533 chip can
> > understand.
> A comment that gave the motivation would be much more useful than
> repeating the maths.
>
> /* Convert resistance to equivalent register value */
>

ok, this is reasonable.

> >
> > > > +     als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * val);
> > > > +
> > > > +     als->pwm_mode = device_property_read_bool(&pdev->dev, "ti,pwm-mode");
> > > > +
> > > >       if (als->irq) {
> > > >               ret = lm3533_als_setup_irq(als, indio_dev);
> > > >               if (ret)
> > > >                       return ret;
> > > >       }
> > > >
> > > > -     ret = lm3533_als_setup(als, pdata);
> > > > +     ret = lm3533_als_setup(als);
> > > >       if (ret)
> > > >               goto err_free_irq;
> > > >
> > > > @@ -907,9 +914,16 @@ static void lm3533_als_remove(struct platform_device *pdev)
> > > >               free_irq(als->irq, indio_dev);
> > > >  }
> > > >
> > > > +static const struct of_device_id lm3533_als_match_table[] = {
> > > > +     { .compatible = "ti,lm3533-als" },
> > > > +     { }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, lm3533_als_match_table);
> > > > +
> > > >  static struct platform_driver lm3533_als_driver = {
> > > >       .driver = {
> > > >               .name   = "lm3533-als",
> > > > +             .of_match_table = lm3533_als_match_table,
> > > >       },
> > > >       .probe          = lm3533_als_probe,
> > > >       .remove         = lm3533_als_remove,
>
> Anyhow, I'm short on time so only looking at the IIO related part.
>
> Jonathan