On Fri, 29 May 2026 12:39:56 +0300 Svyatoslav Ryhel <[email protected]> wrote:
> пт, 29 трав. 2026 р. о 12:08 Jonathan Cameron <[email protected]> пише: > > > > 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. > > > > 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/[email protected]/ > > 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!) > > > > >
