On Thu, 28 May 2026 18:03:31 +0300 Svyatoslav Ryhel <[email protected]> wrote:
> чт, 28 трав. 2026 р. о 17:50 Jonathan Cameron <[email protected]> пише: > > > > On Thu, 28 May 2026 16:51:19 +0300 > > Svyatoslav Ryhel <[email protected]> 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 <[email protected]> > > > > 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!) >
