Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings

[Svyatoslav Ryhel]

пт, 29 трав. 2026 р. о 12:08 Jonathan Cameron <jic23@xxxxxxxxxx> пише:
>
> On Thu, 28 May 2026 18:03:31 +0300
> Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:
>
> > чт, 28 трав. 2026 р. о 17:50 Jonathan Cameron <jic23@xxxxxxxxxx> пише:
> > >
> > > On Thu, 28 May 2026 16:51:19 +0300
> > > Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:
> > >
> > > > Since there are no users of this driver via platform data, remove the
> > > > platform data support and switch to using Device Tree bindings.
> > > > Additionally, optimize functions used only by platform data.
> > >
> > >
> > > At least the IIO ones would have made much the same amount of sense for
> > > dt, just that they weren't having in the first place. I'd prefer that
>
> Gah. I write gibberish after too much reviewing.  having/helping!
>
> > > as a precursor patch to make the rest much more readable.
> > >
> >
> > I can add you preferences into this commit, I don't mind.
> >
> > > >
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > >
> > > I only looked in detail at the iio bit. A few changes requested.
> > >
> > > > ---
> > > >  drivers/iio/light/lm3533-als.c      |  95 ++++------
> > > >  drivers/leds/leds-lm3533.c          |  51 ++++--
> > > >  drivers/mfd/lm3533-core.c           | 268 ++++++++++------------------
> > > >  drivers/video/backlight/lm3533_bl.c |  52 ++++--
> > > >  include/linux/mfd/lm3533.h          |  51 +-----
> > > >  5 files changed, 212 insertions(+), 305 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > > index 99f0b903018c..cbd337b73bd9 100644
> > > > --- a/drivers/iio/light/lm3533-als.c
> > > > +++ b/drivers/iio/light/lm3533-als.c
> > >
> > > > @@ -714,59 +720,33 @@ static const struct attribute_group lm3533_als_attribute_group = {
> > > >       .attrs = lm3533_als_attributes
> > > >  };
> > > >
> > > > -static int lm3533_als_set_input_mode(struct lm3533_als *als, bool pwm_mode)
> > > > +static int lm3533_als_setup(struct lm3533_als *als)
> > > >  {
> > > > -     u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> > > > -     u8 val;
> > > > +     struct device *dev = &als->pdev.dev;
> > > >       int ret;
> > > >
> > > > -     if (pwm_mode)
> > > > -             val = mask;     /* pwm input */
> > > > -     else
> > > > -             val = 0;        /* analog input */
> > > > -
> > > > -     ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
> > > > -     if (ret) {
> > > > -             dev_err(&als->pdev->dev, "failed to set input mode %d\n",
> > > > -                                                             pwm_mode);
> > > > -             return ret;
> > > > -     }
> > > > -
> > > > -     return 0;
> > > > -}
> > > > -
> > > > -static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
> > > > -{
> > > > -     int ret;
> > > > -
> > > > -     if (val < LM3533_ALS_RESISTOR_MIN || val > LM3533_ALS_RESISTOR_MAX) {
> > > > -             dev_err(&als->pdev->dev, "invalid resistor value\n");
> > > > -             return -EINVAL;
> > > > -     }
> > > > -
> > > > -     ret = lm3533_write(als->lm3533, LM3533_REG_ALS_RESISTOR_SELECT, val);
> > > > -     if (ret) {
> > > > -             dev_err(&als->pdev->dev, "failed to set resistor\n");
> > > > -             return ret;
> > > > -     }
> > > > +     device_property_read_u32(dev, "ti,resistor-value-ohm",
> > > > +                              &als->r_select);
> > > Does this have a default?  If so the pattern we've recently be setting on for IIO
> > > is
> > >         if (device_property_present(dev, "ti,resistor-value-ohm"))
> > >                 ret = device_property_read_u32();
> > >                 if (ret) //corrupt property in some fashion
> > >                         return ret;
> > >         } else {
> > >                 //set default
> > >         }
> > > If there is no default then check it unconditionally.
> >
> > default value is LM3533_ALS_RESISTOR_MIN and if no property is present
> > clamp will ensure that als->r_select will be set to
> > LM3533_ALS_RESISTOR_MIN
>
> I don't see that default in the binding doc and relying in the 0 being clamped
> isn't particularly readable - I'd set it explicitly.
>

Oh, ye, my bad. Schema enforces one of props to be present and if pwn
is present then resistor is ignored. What if I move resistor reading,
clamping and conversion under !als->pwm_mode check? Then resistor must
be present and hence must be checked unconditionally.

Additionally, I can comment original lm3533_als_setup with #if 0
#endif then git formatting will be much cleaner and easier to review,
and once we all come to result I will just remove entire commented
block and Lee can pick clean commits.

>
> >
> > >
> > > >
> > > > -     return 0;
> > > > -}
> > > > +     als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> > > > +                           LM3533_ALS_RESISTOR_MAX);
> > > > +     als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
> > > >
> > > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > > -                         const struct lm3533_als_platform_data *pdata)
> > > > -{
> > > > -     int ret;
> > > > +     als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > > >
> > > > -     ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> > > > +     ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?
> > > > +                         LM3533_ALS_INPUT_MODE_MASK : 0,
> > >
> > > That's ugly.  Better as
> > >
> > >         ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF,
> > >                             als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0,
> > >
> >
> > Yes sure, just followed 80 char limit.
> >
> > > Though if there wasn't a layer hiding the regmap, it could just have been
> > >
> > >         ret = regmap_assign_bits(lm3533->regmap, LM3533_REG_ALS_CONF,
> > >                                  LM3533_ALS_INPUT_MODE_MASK, als->pwm_mode);;
> > >
> > > which would have been nicer.
> > >
> > > I'm not particularly keen on the swashing of the helpers being in a patch
>
> smashing.  (this definitely wasn't my best effort at English!)
>
> > > that is about switching the binding type as feels largely unrelated.
> > > Should really have been a precursor, easier to review patch.
> > >
> >
> > Removing of lm3533_update layer is not the scope of this patchset.
>
> Understood.  I'm fine with just the refactor you are doing brought out as a precursor
> patch.
>

I have looked into removing wrappers too. That seems to be less a
hassle that I anticipated, so I will include regmap switch in the v2.

> >
> > >
> > > > +                         LM3533_ALS_INPUT_MODE_MASK);
> > > >       if (ret)
> > > > -             return ret;
> > > > +             return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > > +                                  als->pwm_mode);
> > > >
> > > >       /* 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_write(lm3533, LM3533_REG_ALS_RESISTOR_SELECT,
> > > > +                                (u8)als->r_select);
> > >
> > > Same applies here. Mostly an unrelated change as the only thing switching that
> > > is related to the patch is one parameter.
> > >
> >
> > Removing of lm3533_write layer is not the scope of this patchset.
> >
> > > >               if (ret)
> > > > -                     return ret;
> > > > +                     return dev_err_probe(dev, ret, "failed to set resistor\n");
> > > >       }
> > > >
> > > >       return 0;
> > >
> > > > @@ -852,25 +825,28 @@ static int lm3533_als_probe(struct platform_device *pdev)
> > > >       indio_dev->channels = lm3533_als_channels;
> > > >       indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
> > > >       indio_dev->name = dev_name(&pdev->dev);
> > > > -     iio_device_set_parent(indio_dev, pdev->dev.parent);
> > >
> > > I'm not sure why this was there in the first place.  Hence not sure if it
> > > is safe to remove.
> > >
> >
> > This is directly related to OF conversion. The iio_device_set_parent
> > bound indio_dev to parent, and it causes problems with OF now since
> > als output has its own node and binding it to parent if wrong. Same
> > story for backlight and leds btw.
>
> Is there any risk anyone was using the canonical path to get to the iio dev?
> /sys/bus/platform/devices/..../iio\:deviceX
> This is technically an ABI change be it a subtle one.
>

Linux kernel has no users of this driver, and it is in "stale" state
for more then 2 years (maybe even longer). I have cc'd Johan Hovold.

https://lore.kernel.org/lkml/ZmBcvtLCzllQDWVX@xxxxxxxxxxxxxxxxxxxx/

This this 2 y. o. discussion and there were no actions ore movements.
I assume this driver in its current form has no more users. This does
not mean that it cannot be revived though.

>
> >
> > >
> > > > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> > > > index 45795f2a1042..d707d43d5526 100644
> > > > --- a/drivers/leds/leds-lm3533.c
> > > > +++ b/drivers/leds/leds-lm3533.c
> > >
> > > >
> > > >       led->cb.dev = led->cdev.dev;
> > > >
> > > > -     ret = lm3533_led_setup(led, pdata);
> > > > +     device_property_read_u32(&pdev->dev, "led-max-microamp",
> > > > +                              &led->max_current);
> > >
> > > I'd prefer explicit setting of the default to be visible before this, or
> > > the property_present pattern I mention in the IIO review above.
> > >
> >
> > clamp will ensure that led->max_current will be set to
> > LM3533_LED_MAX_CURRENT_MIN regardless if it it present
>
> As above, I'd prefer it set explicitly.
>

I understand your position and I am not denying it for ALS part, but
LEDs don't belong to IIO subsystem and different subsystem maintainers
may have drastically different preferences and requirements (ugh, PTSD
in its full glory).

> >
> > > > +     led->max_current = clamp(led->max_current, LM3533_LED_MAX_CURRENT_MIN,
> > > > +                              LM3533_LED_MAX_CURRENT_MAX);
> > >
> > > I didn't look any further (busy day!)
> >
>