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

[Jonathan Cameron]

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.


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

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


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

> 
> > > +     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!)  
>