пт, 29 трав. 2026 р. о 13:48 Jonathan Cameron [off-list ref] пише:

On Fri, 29 May 2026 12:39:56 +0300
Svyatoslav Ryhel [off-list ref] wrote:

quoted

пт, 29 трав. 2026 р. о 12:08 Jonathan Cameron [off-list ref] пише:

quoted

On Thu, 28 May 2026 18:03:31 +0300
Svyatoslav Ryhel [off-list ref] wrote:

quoted

чт, 28 трав. 2026 р. о 17:50 Jonathan Cameron [off-list ref] пише:

quoted

On Thu, 28 May 2026 16:51:19 +0300
Svyatoslav Ryhel [off-list ref] wrote:

quoted

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!

quoted

quoted

as a precursor patch to make the rest much more readable.

I can add you preferences into this commit, I don't mind.

quoted

quoted

Signed-off-by: Svyatoslav Ryhel <redacted>

I only looked in detail at the iio bit. A few changes requested.

quoted

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

quoted

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

quoted

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.

By "flatten the two helpers" you mean incorporate
lm3533_als_set_input_mode and lm3533_als_set_resistor into
lm3533_als_setup first and then convert it to use DT? I am asking,
just to be sure.

quoted

quoted

quoted

quoted

quoted

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

quoted

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

quoted

quoted

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.

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

              if (ret)
-                     return ret;
+                     return dev_err_probe(dev, ret, "failed to set resistor\n");
      }

      return 0;

quoted

@@ -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@hovoldconsulting.com/ (local)

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?

I am not insane enough to get myself into all this conversion if I did
not need it. This is one of 2 remaining gaps in support of LG
P880/P895 I own and support. I would really like to finally mainline
all the patches I have locally since maintaining them becomes quite
troublesome with time and additional patches layering on top.

Jonathan

quoted

quoted

quoted

quoted

quoted

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

quoted

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

quoted

quoted

quoted

quoted

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