Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings

[Jonathan Cameron]

On Tue, 2 Jun 2026 16:50:16 +0300
Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:

> вт, 2 черв. 2026 р. о 16:46 Jonathan Cameron <jic23@xxxxxxxxxx> пише:
> >
> > On Mon,  1 Jun 2026 18:18:25 +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.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>  
> >
> >  
> > > index 52136ca1abc9..55b35467a722 100644
> > > --- a/drivers/iio/light/lm3533-als.c
> > > +++ b/drivers/iio/light/lm3533-als.c
> > > @@ -16,16 +16,19 @@
> > >  #include <linux/module.h>
> > >  #include <linux/mutex.h>
> > >  #include <linux/mfd/core.h>
> > > +#include <linux/mod_devicetable.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/property.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/uaccess.h>
> > > +#include <linux/units.h>
> > >
> > >  #include <linux/mfd/lm3533.h>
> > >
> > >
> > > -#define LM3533_ALS_RESISTOR_MIN                      1
> > > -#define LM3533_ALS_RESISTOR_MAX                      127
> > > +#define LM3533_ALS_RESISTOR_MIN                      1575
> > > +#define LM3533_ALS_RESISTOR_MAX                      200000
> > >  #define LM3533_ALS_CHANNEL_CURRENT_MAX               2
> > >  #define LM3533_ALS_THRESH_MAX                        3
> > >  #define LM3533_ALS_ZONE_MAX                  4
> > > @@ -57,6 +60,9 @@ struct lm3533_als {
> > >
> > >       atomic_t zone;
> > >       struct mutex thresh_mutex;
> > > +
> > > +     bool pwm_mode;
> > > +     u32 r_select;
> > >  };
> > >
> > >
> > > @@ -411,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev,
> > >       int enable;
> > >       int ret;
> > >
> > > -     if (als->irq) {
> > > +     if (als->irq > 0) {
> > >               ret = lm3533_als_get_int_mode(indio_dev, &enable);
> > >               if (ret)
> > >                       return ret;
> > > @@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
> > >       .attrs = lm3533_als_attributes
> > >  };
> > >
> > > -static int lm3533_als_setup(struct lm3533_als *als,
> > > -                         const struct lm3533_als_platform_data *pdata)
> > > +static int lm3533_als_setup(struct lm3533_als *als)
> > >  {
> > >       struct device *dev = &als->pdev->dev;
> > >       int ret;
> > >
> > > +     als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> > > +
> > >       ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> > >                                LM3533_ALS_INPUT_MODE_MASK,
> > > -                              pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > > +                              als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> > >       if (ret)
> > >               return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> > > -                                  pdata->pwm_mode);
> > > -
> > > +                                  als->pwm_mode);
> > >
> > >       /* ALS input is always high impedance in PWM-mode. */
> > > -     if (!pdata->pwm_mode) {
> > > -             if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> > > -                 pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> > > -                     dev_err(&als->pdev->dev, "invalid resistor value\n");
> > > -                     return -EINVAL;
> > > -             }
> > > +     if (!als->pwm_mode) {
> > > +             ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
> > > +                                            &als->r_select);
> > > +             if (ret)
> > > +                     return dev_err_probe(dev, ret,
> > > +                                          "failed to ger resistor value\n");
> > > +
> > > +             als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> > > +                                   LM3533_ALS_RESISTOR_MAX);  
> >
> > If we are getting garbage from DT I think I'd rather error out that paper over
> > that problem.  So similar to before, check valid value and if not fail probe
> > so that hopefully someone goes and fixes it!
> >  
> 
> sure
> 
> > > +             als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);  
> > Why do we need this when we didn't before?  The range checks are the same
> > so it smells like it shouldn't need transforming. I'd also rather we didn't do
> > rewriting of the meaning of r_select like this.  Just use a local variable for
> > the intermediate result.
> >  
> 
> before pdata passed resistor value as actual register value, not we
> are getting the actual resistance in ohms from the tree and must
> convert it into register value.

ah. I missed the change of values.  Can you make them explicitly now _OHMS or something
along those lines rather than reusing the macro name for a different thing.

> 
> > >
> > >               ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> > > -                                pdata->r_select);
> > > +                                als->r_select);
> > >               if (ret)
> > >                       return dev_err_probe(dev, ret, "failed to set resistor\n");