Re: [PATCH v1 2/2] mfd: lm3533: convert to use OF

[Svyatoslav Ryhel <clamor95@gmail.com>]

чт, 13 лют. 2025 р. о 16:57 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> пише:
>
> On Wed, Feb 12, 2025 at 09:58:42AM +0200, Svyatoslav Ryhel wrote:
> > Add ability to fill pdata from device tree. Common stuff is
> > filled from core driver and then pdata is filled per-device
> > since all cells are optional.
>
> ...
>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/mfd/core.h>
>
> > +#include <linux/of.h>
>
> Is it used? In any case, please no OF-centric APIs in a new (feature) code.
>
> >  #include <linux/platform_device.h>
> > +#include <linux/property.h>
> >  #include <linux/slab.h>
> >  #include <linux/uaccess.h>
>
> ...
>
> > +static int lm3533_pass_of_node(struct platform_device *pdev,
>
> pass_of_node sounds a bit awkward.
> Perhaps you thought about parse_fwnode ?
>
> > +                            struct lm3533_als_platform_data *pdata)
> > +{
> > +     struct device *parent_dev = pdev->dev.parent;
> > +     struct device *dev = &pdev->dev;
> > +     struct fwnode_handle *node;
> > +     const char *label;
> > +     int val, ret;
> > +
> > +     device_for_each_child_node(parent_dev, node) {
> > +             fwnode_property_read_string(node, "compatible", &label);
> > +
> > +             if (!strcmp(label, pdev->name)) {
>
> This is a bit strange. Why one need to compare platform device instance (!)
> name with compatible which is part of ABI. This looks really wrong approach.
> Needs a very good explanation on what's going on here.
>
> Besides that the label is usually filled by LEDS core, why do we need to handle
> it in a special way?
>
> > +                     ret = fwnode_property_read_u32(node, "reg", &val);
> > +                     if (ret) {
> > +                             dev_err(dev, "reg property is missing: ret %d\n", ret);
> > +                             return ret;
> > +                     }
> > +
> > +                     if (val == pdev->id) {
>
> > +                             dev->fwnode = node;
> > +                             dev->of_node = to_of_node(node);
>
> No direct access to fwnode in struct device, please use device_set_node().
>
> > +                     }
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
>
> ...
>
> >       pdata = dev_get_platdata(&pdev->dev);
> >       if (!pdata) {
> > -             dev_err(&pdev->dev, "no platform data\n");
> > -             return -EINVAL;
> > +             pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > +             if (!pdata)
> > +                     return -ENOMEM;
> > +
> > +             ret = lm3533_pass_of_node(pdev, pdata);
> > +             if (ret)
> > +                     return dev_err_probe(&pdev->dev, ret,
> > +                                          "failed to get als device node\n");
>
> With
>
>         struct device *dev = &pdev->dev;
>
> at the top of the function will help a lot in making the code neater and easier
> to read.
>
> > +             lm3533_parse_als(pdev, pdata);
> >       }
>
> ...
>
> >  #include <linux/leds.h>
> >  #include <linux/mfd/core.h>
> >  #include <linux/mutex.h>
>
> > +#include <linux/of.h>
>
> Cargo cult? "Proxy" header? Please follow IWYU principle.
>
> >  #include <linux/platform_device.h>
> > +#include <linux/property.h>
> >  #include <linux/slab.h>
>
> ...
>
> > +static void lm3533_parse_led(struct platform_device *pdev,
> > +                          struct lm3533_led_platform_data *pdata)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     int val, ret;
> > +
> > +     ret = device_property_read_string(dev, "default-trigger",
> > +                                       &pdata->default_trigger);
> > +     if (ret)
> > +             pdata->default_trigger = "none";
>
> Isn't this done already in LEDS core?
>
> > +     /* 5000 - 29800 uA (800 uA step) */
> > +     ret = device_property_read_u32(dev, "max-current-microamp", &val);
> > +     if (ret)
> > +             val = 5000;
> > +     pdata->max_current = val;
> > +
> > +     /* 0 - 0x3f */
> > +     ret = device_property_read_u32(dev, "pwm", &val);
> > +     if (ret)
> > +             val = 0;
> > +     pdata->pwm = val;
> > +}
>
> ...
>
> > +static int lm3533_pass_of_node(struct platform_device *pdev,
> > +                            struct lm3533_led_platform_data *pdata)
>
> I think I already saw exactly the same piece of code. Please make sure
> you have no duplications.
>
> > +}
>
> ...
>
> >       pdata = dev_get_platdata(&pdev->dev);
> >       if (!pdata) {
> > -             dev_err(&pdev->dev, "no platform data\n");
> > -             return -EINVAL;
> > +             pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > +             if (!pdata)
> > +                     return -ENOMEM;
> > +
> > +             ret = lm3533_pass_of_node(pdev, pdata);
> > +             if (ret)
> > +                     return dev_err_probe(&pdev->dev, ret,
> > +                                          "failed to get led device node\n");
> > +
> > +             lm3533_parse_led(pdev, pdata);
>
> Ditto.
>
> >       }
>
> ...
>
> > -     led->cdev.name = pdata->name;
> > +     led->cdev.name = dev_name(&pdev->dev);
>
> Are you sure it's a good idea?
>
> ...
>
> > -     if (!pdata->als)
> > +     if (!pdata->num_als)
> >               return 0;
> >
> > -     lm3533_als_devs[0].platform_data = pdata->als;
> > -     lm3533_als_devs[0].pdata_size = sizeof(*pdata->als);
> > +     if (pdata->num_als > ARRAY_SIZE(lm3533_als_devs))
> > +             pdata->num_als = ARRAY_SIZE(lm3533_als_devs);
>
> Looks like you want
>
>         pdata->num_als = clamp(pdata->num_als, 0, ARRAY_SIZE(lm3533_als_devs));
>         if (!pdata->num_als)
>                 return 0;
>
> instead of the above. You would need minmax.h for that.
>
> ...
>
> > +     if (pdata->leds) {
>
> This is strange. I would expect num_leds == 0 in this case
>
> > +             for (i = 0; i < pdata->num_leds; ++i) {
> > +                     lm3533_led_devs[i].platform_data = &pdata->leds[i];
> > +                     lm3533_led_devs[i].pdata_size = sizeof(pdata->leds[i]);
> > +             }
> >       }
>
> ...
>
> > +static void lm3533_parse_nodes(struct lm3533 *lm3533,
> > +                            struct lm3533_platform_data *pdata)
> > +{
> > +     struct fwnode_handle *node;
> > +     const char *label;
> > +
> > +     device_for_each_child_node(lm3533->dev, node) {
> > +             fwnode_property_read_string(node, "compatible", &label);
> > +
> > +             if (!strcmp(label, lm3533_bl_devs[pdata->num_backlights].name))
> > +                     pdata->num_backlights++;
> > +
> > +             if (!strcmp(label, lm3533_led_devs[pdata->num_leds].name))
> > +                     pdata->num_leds++;
> > +
> > +             if (!strcmp(label, lm3533_als_devs[pdata->num_als].name))
> > +                     pdata->num_als++;
> > +     }
> > +}
>
> Oh, I don't like this approach. If you have compatible, you have driver_data
> available, all this is not needed as it may be hard coded.
>
> ...
>
> >       if (!pdata) {
>
> I would expect actually that legacy platform data support will be simply killed
> by this patch(es). Do we have in-kernel users? If so, they can be easily
> converted to use software nodes, otherwise we even don't need to care.
>
> > -             dev_err(lm3533->dev, "no platform data\n");
> > -             return -EINVAL;
> > +             pdata = devm_kzalloc(lm3533->dev, sizeof(*pdata), GFP_KERNEL);
> > +             if (!pdata)
> > +                     return -ENOMEM;
> > +
> > +             ret = device_property_read_u32(lm3533->dev,
> > +                                            "ti,boost-ovp",
> > +                                            &pdata->boost_ovp);
> > +             if (ret)
> > +                     pdata->boost_ovp = LM3533_BOOST_OVP_16V;
> > +
> > +             ret = device_property_read_u32(lm3533->dev,
> > +                                            "ti,boost-freq",
> > +                                            &pdata->boost_freq);
> > +             if (ret)
> > +                     pdata->boost_freq = LM3533_BOOST_FREQ_500KHZ;
> > +
> > +             lm3533_parse_nodes(lm3533, pdata);
> > +
> > +             lm3533->dev->platform_data = pdata;
> >       }
>
> ...
>
> > +static const struct of_device_id lm3533_match_table[] = {
> > +     { .compatible = "ti,lm3533" },
> > +     { },
>
> No comma in the terminator entry.
>
> > +};
>
> ...
>
> > +static void lm3533_parse_backlight(struct platform_device *pdev,
>
> pdev is not actually used, just pass struct device *dev directly.
> Same comment to other functions in this change. It will make code more
> bus independent and reusable.
>
> > +                                struct lm3533_bl_platform_data *pdata)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     int val, ret;
> > +
> > +     /* 5000 - 29800 uA (800 uA step) */
> > +     ret = device_property_read_u32(dev, "max-current-microamp", &val);
> > +     if (ret)
> > +             val = 5000;
> > +     pdata->max_current = val;
>
> > +     /* 0 - 255 */
> > +     ret = device_property_read_u32(dev, "default-brightness", &val);
> > +     if (ret)
> > +             val = LM3533_BL_MAX_BRIGHTNESS;
> > +     pdata->default_brightness = val;
>
> Isn't handled by LEDS core?
>
> > +     /* 0 - 0x3f */
> > +     ret = device_property_read_u32(dev, "pwm", &val);
> > +     if (ret)
> > +             val = 0;
> > +     pdata->pwm = val;
> > +}
>
> ...
>
> > +static int lm3533_pass_of_node(struct platform_device *pdev,
> > +                            struct lm3533_bl_platform_data *pdata)
> > +{
>
> 3rd dup?
>
> > +}
>
> ...
>
> >       pdata = dev_get_platdata(&pdev->dev);
> >       if (!pdata) {
> > -             dev_err(&pdev->dev, "no platform data\n");
> > -             return -EINVAL;
> > +             pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > +             if (!pdata)
> > +                     return -ENOMEM;
> > +
> > +             ret = lm3533_pass_of_node(pdev, pdata);
> > +             if (ret)
> > +                     return dev_err_probe(&pdev->dev, ret,
> > +                                          "failed to get backlight device node\n");
> > +
> > +             lm3533_parse_backlight(pdev, pdata);
> >       }
>
> Ditto.
>
> > -     bd = devm_backlight_device_register(&pdev->dev, pdata->name,
> > -                                     pdev->dev.parent, bl, &lm3533_bl_ops,
> > -                                     &props);
> > +     bd = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev),
>
> I'm not sure the dev_name() is a good idea. We usually try to rely on the
> predictable outcome, something like what "%pfw" prints against a certain fwnode.
>
> > +                                         pdev->dev.parent, bl,
> > +                                         &lm3533_bl_ops, &props);
>
> ...
>
> Also I feel that this change may be split to a few separate logical changes.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Acknowledged, thank you.