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

[Jonathan Cameron]

On Fri, 29 May 2026 12:39:56 +0300
Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:

> пт, 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.

Sounds good.

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

No don't do that.  If you flatten the two helpers as a precursor patch
then the changes in here will be easier to review anyway.

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

Ah ok. Even better.

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

So, just to check, are you a user of this code or is this more trying to
help out with old code?

Jonathan

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