Re: [PATCH v5 08/14] mfd: lm3533: Convert to use OF bindings

On Tue, Jul 14, 2026 at 04:57:01PM +0300, Svyatoslav Ryhel wrote:

пт, 3 лип. 2026 р. о 14:03 Johan Hovold [off-list ref] пише:

quoted

On Wed, Jun 17, 2026 at 11:00:25AM +0300, Svyatoslav Ryhel 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.

Signed-off-by: Svyatoslav Ryhel <redacted>
Reviewed-by: Daniel Thompson (RISCstar) <danielt@kernel.org> #for backlight
---
 drivers/iio/light/lm3533-als.c      |  67 +++++---
 drivers/leds/leds-lm3533.c          |  50 ++++--
 drivers/mfd/lm3533-core.c           | 236 ++++++++++++----------------
 drivers/mfd/lm3533-ctrlbank.c       |   5 -
 drivers/video/backlight/lm3533_bl.c |  55 +++++--
 include/linux/mfd/lm3533.h          |  52 +-----
 6 files changed, 220 insertions(+), 245 deletions(-)

quoted

 static int lm3533_als_probe(struct platform_device *pdev)
 {
-     const struct lm3533_als_platform_data *pdata;
      struct lm3533 *lm3533;
      struct lm3533_als *als;
      struct iio_dev *indio_dev;

@@ -803,12 +817,6 @@ static int lm3533_als_probe(struct platform_device *pdev)
      if (!lm3533)
              return -EINVAL;

-     pdata = dev_get_platdata(&pdev->dev);
-     if (!pdata) {
-             dev_err(&pdev->dev, "no platform data\n");
-             return -EINVAL;
-     }
-
      indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));
      if (!indio_dev)
              return -ENOMEM;

@@ -817,25 +825,27 @@ 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);

Why are you reparenting the iio device here?

Because every cell has its own binding now and using phandle to parent
when device has its own node is not a good practice.

quoted

That's an ABI break.

This driver does not have any active users in the kernel and no
activity for more then 2 years.

We have never required board files to be upstream.

And a working driver does not need to be changed every year.

In any case, something like this at a minimum needs to be highlighted in
the commit message.

quoted

quoted

+static const struct of_device_id lm3533_als_match_table[] = {
+     { .compatible = "ti,lm3533-als" },
+     { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_als_match_table);
+
 static struct platform_driver lm3533_als_driver = {
      .driver = {
              .name   = "lm3533-als",
+             .of_match_table = lm3533_als_match_table,
      },
      .probe          = lm3533_als_probe,
      .remove         = lm3533_als_remove,

You should also remove the platform module alias below.

Why?

Because your change makes it obsolete. The driver now only supports OF
probing.

quoted

quoted

@@ -680,15 +684,22 @@ static int lm3533_led_probe(struct platform_device *pdev)

      platform_set_drvdata(pdev, led);

-     ret = led_classdev_register(pdev->dev.parent, &led->cdev);
+     ret = led_classdev_register(&pdev->dev, &led->cdev);

Here too you appear to be reparenting the class devices.

quoted

      if (ret) {
-             dev_err(&pdev->dev, "failed to register LED %d\n", pdev->id);
+             dev_err(&pdev->dev, "failed to register LED %d\n", led->id);

This does not seem to be necessary.

Agreed.

quoted

quoted

              return ret;
      }

      led->cb.dev = led->cdev.dev;

-     ret = lm3533_led_setup(led, pdata);
+     device_property_read_u32(&pdev->dev, "led-max-microamp",
+                              &led->max_current);
+     led->max_current = clamp(led->max_current, LM3533_MAX_CURRENT_MIN,
+                              LM3533_MAX_CURRENT_MAX);

Why clamp instead of having lm3533_led_setup() fail below?

According to OF schema default lower margin is set to
LM3533_MAX_CURRENT_MIN so clamping seems a good option here, even
though it will clamp max value.

Just let it fail to probe. The driver already have the necessary checks.

quoted

quoted

+
+     device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
+
+     ret = lm3533_led_setup(led);
      if (ret)
              goto err_deregister;

@@ -725,9 +736,16 @@ static void lm3533_led_shutdown(struct platform_device *pdev)
      lm3533_led_set(&led->cdev, LED_OFF);            /* disable blink */
 }

+static const struct of_device_id lm3533_led_match_table[] = {
+     { .compatible = "ti,lm3533-leds" },
+     { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_led_match_table);
+
 static struct platform_driver lm3533_led_driver = {
      .driver = {
              .name = "lm3533-leds",
+             .of_match_table = lm3533_led_match_table,
      },
      .probe          = lm3533_led_probe,
      .remove         = lm3533_led_remove,

Remove platform alias below as well.

Why?

Same reason as above.

quoted

quoted

 static int lm3533_device_init(struct lm3533 *lm3533)
 {
-     struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev);
+     struct device *dev = lm3533->dev;
+     struct mfd_cell *lm3533_devices;
+     u32 count = 0, reg, nchilds;

Don't mix multiple declarations with initialisation like this.

Checkpatch does not complain on style issue, hence this is not prohibited.

Checkpatch does not define good style.

quoted

quoted

      int ret;

-     dev_dbg(lm3533->dev, "%s\n", __func__);
+     nchilds = device_get_child_node_count(dev);
+     if (!nchilds || nchilds > LM3533_CELLS_MAX)
+             return dev_err_probe(dev, -ENODEV,
+                                  "num of child nodes is not supported\n");

-     if (!pdata) {
-             dev_err(lm3533->dev, "no platform data\n");
-             return -EINVAL;
-     }
+     lm3533_devices = devm_kcalloc(dev, nchilds, sizeof(*lm3533_devices),
+                                   GFP_KERNEL);
+     if (!lm3533_devices)
+             return -ENOMEM;

-     lm3533->hwen = devm_gpiod_get(lm3533->dev, NULL, GPIOD_OUT_LOW);
-     if (IS_ERR(lm3533->hwen))
-             return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen), "failed to request HWEN GPIO\n");
-     gpiod_set_consumer_name(lm3533->hwen, "lm3533-hwen");
+     device_for_each_child_node_scoped(dev, child) {
+             if (count >= nchilds)
+                     break;

How could count be larger than nchilds?

Only if the tree is malformed, hence this check was added.

But you've just retrieved nchilds by parsing the tree and counting the
child nodes. So how can count possibly be larger than nchilds here?

quoted

quoted

+
+             if (fwnode_device_is_compatible(child, "ti,lm3533-als")) {
+                     lm3533_devices[count].name = "lm3533-als";
+                     lm3533_devices[count].of_compatible = "ti,lm3533-als";
+                     lm3533_devices[count].id = PLATFORM_DEVID_NONE;
+
+                     lm3533->have_als = true;
+                     count++;
+             } else if (fwnode_device_is_compatible(child, "ti,lm3533-backlight")) {
+                     ret = fwnode_property_read_u32(child, "reg", &reg);
+                     if (ret || reg >= LM3533_HVLED_ID_MAX) {
+                             dev_err(dev, "invalid backlight node %pfw\n", child);
+                             continue;
+                     }
+
+                     lm3533_devices[count].name = "lm3533-backlight";
+                     lm3533_devices[count].of_compatible = "ti,lm3533-backlight";
+                     lm3533_devices[count].id = reg;
+                     lm3533_devices[count].of_reg = reg;
+                     lm3533_devices[count].use_of_reg = true;
+
+                     lm3533->have_backlights = true;
+                     count++;
+             } else if (fwnode_device_is_compatible(child, "ti,lm3533-leds")) {
+                     ret = fwnode_property_read_u32(child, "reg", &reg);
+                     if (ret || reg < LM3533_HVLED_ID_MAX ||
+                         reg > LM3533_LVLED_ID_MAX) {
+                             dev_err(dev, "invalid LED node %pfw\n", child);
+                             continue;
+                     }
+
+                     lm3533_devices[count].name = "lm3533-leds";
+                     lm3533_devices[count].of_compatible = "ti,lm3533-leds";
+                     lm3533_devices[count].id = reg - LM3533_HVLED_ID_MAX;
+                     lm3533_devices[count].of_reg = reg;
+                     lm3533_devices[count].use_of_reg = true;
+
+                     lm3533->have_leds = true;
+                     count++;
+             }
+     }

Why do you need the above at all? Shouldn't you be able to just use
of_platform_populate().

of_platform_populate() is not a part of mfd framework.

We have several MFD drivers using of_platform_populate().

quoted

quoted

      lm3533_enable(lm3533);

      ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
                               LM3533_BOOST_FREQ_MASK,
-                              pdata->boost_freq << LM3533_BOOST_FREQ_SHIFT);
+                              lm3533->boost_freq << LM3533_BOOST_FREQ_SHIFT);
      if (ret) {
-             dev_err(lm3533->dev, "failed to set boost frequency\n");
+             dev_err(dev, "failed to set boost frequency\n");
              goto err_disable;
      }

      ret = regmap_update_bits(lm3533->regmap, LM3533_REG_BOOST_PWM,
                               LM3533_BOOST_OVP_MASK,
-                              pdata->boost_ovp << LM3533_BOOST_OVP_SHIFT);
+                              lm3533->boost_ovp << LM3533_BOOST_OVP_SHIFT);
      if (ret) {
-             dev_err(lm3533->dev, "failed to set boost ovp\n");
+             dev_err(dev, "failed to set boost ovp\n");
              goto err_disable;
      }

-     lm3533_device_als_init(lm3533);
-     lm3533_device_bl_init(lm3533);
-     lm3533_device_led_init(lm3533);
+     ret = mfd_add_devices(dev, 0, lm3533_devices, count, NULL, 0, NULL);
+     if (ret) {
+             dev_err(dev, "failed to add MFD devices: %d\n", ret);
+             goto err_disable;
+     }

      return 0;

@@ -504,7 +440,26 @@ static int lm3533_i2c_probe(struct i2c_client *i2c)
              return PTR_ERR(lm3533->regmap);

      lm3533->dev = &i2c->dev;
-     lm3533->irq = i2c->irq;
+
+     lm3533->hwen = devm_gpiod_get_optional(lm3533->dev, "enable",
+                                            GPIOD_OUT_LOW);
+     if (IS_ERR(lm3533->hwen))
+             return dev_err_probe(lm3533->dev, PTR_ERR(lm3533->hwen),
+                                  "failed to get HWEN GPIO\n");

Please use brackets around multline statements for readability
throughout.

Checkpatch does not complain on style issue, hence this is not prohibited.

Checkpatch is irrelevant.

quoted

quoted

+
+     device_property_read_u32(lm3533->dev, "ti,boost-ovp-microvolt",
+                              &lm3533->boost_ovp);
+
+     lm3533->boost_ovp = clamp(lm3533->boost_ovp, LM3533_BOOST_OVP_MIN,
+                               LM3533_BOOST_OVP_MAX);
+     lm3533->boost_ovp = lm3533->boost_ovp / (8 * MICRO) - 2;
+
+     device_property_read_u32(lm3533->dev, "ti,boost-freq-hz",
+                              &lm3533->boost_freq);
+
+     lm3533->boost_freq = clamp(lm3533->boost_freq, LM3533_BOOST_FREQ_MIN,
+                                LM3533_BOOST_FREQ_MAX);
+     lm3533->boost_freq = lm3533->boost_freq / (500 * KILO) - 1;

Again, why clamp instead of failing probe?

According to OF schema default lower margin is set to
LM3533_BOOST_FREQ_MIN so clamping seems a good option here, even
though it will clamp max value.

Just let the driver fail to probe as the sanity checks are already
there.

quoted

quoted

      return lm3533_device_init(lm3533);
 }

@@ -518,6 +473,12 @@ static void lm3533_i2c_remove(struct i2c_client *i2c)
      lm3533_device_exit(lm3533);
 }

+static const struct of_device_id lm3533_match_table[] = {
+     { .compatible = "ti,lm3533" },
+     { }
+};
+MODULE_DEVICE_TABLE(of, lm3533_match_table);
+
 static const struct i2c_device_id lm3533_i2c_ids[] = {
      { "lm3533" },
      { }

Shouldn't you drop i2c probing now as well?

quoted

quoted

@@ -528,6 +489,7 @@ static struct i2c_driver lm3533_i2c_driver = {
      .driver = {
                 .name = "lm3533",
                 .dev_groups = lm3533_attribute_groups,
+                .of_match_table = lm3533_match_table,
      },
      .id_table       = lm3533_i2c_ids,
      .probe          = lm3533_i2c_probe,

Johan