| Message ID | 20250212075845.11338-3-clamor95@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: lm3533: convert to use OF | expand |
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.
чт, 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.
On Wed, 12 Feb 2025 09:58:42 +0200 Svyatoslav Ryhel <clamor95@gmail.com> 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. > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> Focusing on just the IIO bit. (backlog of review is a bit high for me this weekend!) > --- > drivers/iio/light/lm3533-als.c | 58 ++++++++++++++++++++- > drivers/leds/leds-lm3533.c | 69 +++++++++++++++++++++++-- > drivers/mfd/lm3533-core.c | 79 ++++++++++++++++++++++++----- > drivers/video/backlight/lm3533_bl.c | 72 ++++++++++++++++++++++++-- > include/linux/mfd/lm3533.h | 1 + > 5 files changed, 256 insertions(+), 23 deletions(-) > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c > index 99f0b903018c..21fc5f215c80 100644 > --- a/drivers/iio/light/lm3533-als.c > +++ b/drivers/iio/light/lm3533-als.c > @@ -16,7 +16,9 @@ > #include <linux/module.h> > #include <linux/mutex.h> > #include <linux/mfd/core.h> > +#include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/property.h> > #include <linux/slab.h> > #include <linux/uaccess.h> > > @@ -826,6 +828,50 @@ static const struct iio_info lm3533_als_info = { > .read_raw = &lm3533_als_read_raw, > }; > > +static void lm3533_parse_als(struct platform_device *pdev, > + struct lm3533_als_platform_data *pdata) > +{ > + struct device *dev = &pdev->dev; > + int val, ret; > + > + /* 1 - 127 (ignored in PWM-mode) */ > + ret = device_property_read_u32(dev, "resistor-value", &val); > + if (ret) > + val = 1; For defaults that apply on any error, the pattern val = 1; device_property_read_u32(dev, "resistor-value", &val); is cleaner. What are the units? If it's a resistor then they should be ohms or similar and that should be part of the naming. You should be taking whatever the value is in ohms and mapping it to appropriate r_select in this function. > + pdata->r_select = val; > + > + pdata->pwm_mode = device_property_read_bool(dev, "pwm-mode"); > +} > + > +static int lm3533_pass_of_node(struct platform_device *pdev, > + 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) { This devices should have direct access to the correct child node. Otherwise something odd is going on... > + fwnode_property_read_string(node, "compatible", &label); > + > + if (!strcmp(label, pdev->name)) { > + ret = fwnode_property_read_u32(node, "reg", &val); > + if (ret) { > + dev_err(dev, "reg property is missing: ret %d\n", ret); use return dev_err_probe() for these. > + return ret; > + } > + > + if (val == pdev->id) { > + dev->fwnode = node; > + dev->of_node = to_of_node(node); > + } > + } > + } > + > + return 0; > +} > + > static int lm3533_als_probe(struct platform_device *pdev) > { > const struct lm3533_als_platform_data *pdata; > @@ -840,8 +886,16 @@ static int lm3533_als_probe(struct platform_device *pdev) > > 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"); > + > + lm3533_parse_als(pdev, pdata); > } > > indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als));
diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c index 99f0b903018c..21fc5f215c80 100644 --- a/drivers/iio/light/lm3533-als.c +++ b/drivers/iio/light/lm3533-als.c @@ -16,7 +16,9 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/mfd/core.h> +#include <linux/of.h> #include <linux/platform_device.h> +#include <linux/property.h> #include <linux/slab.h> #include <linux/uaccess.h> @@ -826,6 +828,50 @@ static const struct iio_info lm3533_als_info = { .read_raw = &lm3533_als_read_raw, }; +static void lm3533_parse_als(struct platform_device *pdev, + struct lm3533_als_platform_data *pdata) +{ + struct device *dev = &pdev->dev; + int val, ret; + + /* 1 - 127 (ignored in PWM-mode) */ + ret = device_property_read_u32(dev, "resistor-value", &val); + if (ret) + val = 1; + pdata->r_select = val; + + pdata->pwm_mode = device_property_read_bool(dev, "pwm-mode"); +} + +static int lm3533_pass_of_node(struct platform_device *pdev, + 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)) { + 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); + } + } + } + + return 0; +} + static int lm3533_als_probe(struct platform_device *pdev) { const struct lm3533_als_platform_data *pdata; @@ -840,8 +886,16 @@ static int lm3533_als_probe(struct platform_device *pdev) 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"); + + lm3533_parse_als(pdev, pdata); } indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*als)); diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c index 45795f2a1042..587bc27b17c3 100644 --- a/drivers/leds/leds-lm3533.c +++ b/drivers/leds/leds-lm3533.c @@ -11,7 +11,9 @@ #include <linux/leds.h> #include <linux/mfd/core.h> #include <linux/mutex.h> +#include <linux/of.h> #include <linux/platform_device.h> +#include <linux/property.h> #include <linux/slab.h> #include <linux/mfd/lm3533.h> @@ -644,6 +646,59 @@ static int lm3533_led_setup(struct lm3533_led *led, return lm3533_ctrlbank_set_pwm(&led->cb, pdata->pwm); } +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"; + + /* 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) +{ + 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)) { + ret = fwnode_property_read_u32(node, "reg", &val); + if (ret) { + dev_info(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); + } + } + } + + return 0; +} + static int lm3533_led_probe(struct platform_device *pdev) { struct lm3533 *lm3533; @@ -659,8 +714,16 @@ static int lm3533_led_probe(struct platform_device *pdev) 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); } if (pdev->id < 0 || pdev->id >= LM3533_LVCTRLBANK_COUNT) { @@ -673,7 +736,7 @@ static int lm3533_led_probe(struct platform_device *pdev) return -ENOMEM; led->lm3533 = lm3533; - led->cdev.name = pdata->name; + led->cdev.name = dev_name(&pdev->dev); led->cdev.default_trigger = pdata->default_trigger; led->cdev.brightness_set_blocking = lm3533_led_set; led->cdev.brightness_get = lm3533_led_get; diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c index 0a2409d00b2e..cab60c25e3c7 100644 --- a/drivers/mfd/lm3533-core.c +++ b/drivers/mfd/lm3533-core.c @@ -381,11 +381,16 @@ static int lm3533_device_als_init(struct lm3533 *lm3533) struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev); int ret; - 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); + + if (pdata->als) { + lm3533_als_devs[0].platform_data = pdata->als; + lm3533_als_devs[0].pdata_size = sizeof(*pdata->als); + } ret = mfd_add_devices(lm3533->dev, 0, lm3533_als_devs, 1, NULL, 0, NULL); @@ -405,15 +410,17 @@ static int lm3533_device_bl_init(struct lm3533 *lm3533) int i; int ret; - if (!pdata->backlights || pdata->num_backlights == 0) + if (!pdata->num_backlights) return 0; if (pdata->num_backlights > ARRAY_SIZE(lm3533_bl_devs)) pdata->num_backlights = ARRAY_SIZE(lm3533_bl_devs); - for (i = 0; i < pdata->num_backlights; ++i) { - lm3533_bl_devs[i].platform_data = &pdata->backlights[i]; - lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]); + if (pdata->backlights) { + for (i = 0; i < pdata->num_backlights; ++i) { + lm3533_bl_devs[i].platform_data = &pdata->backlights[i]; + lm3533_bl_devs[i].pdata_size = sizeof(pdata->backlights[i]); + } } ret = mfd_add_devices(lm3533->dev, 0, lm3533_bl_devs, @@ -434,15 +441,17 @@ static int lm3533_device_led_init(struct lm3533 *lm3533) int i; int ret; - if (!pdata->leds || pdata->num_leds == 0) + if (!pdata->num_leds) return 0; if (pdata->num_leds > ARRAY_SIZE(lm3533_led_devs)) pdata->num_leds = ARRAY_SIZE(lm3533_led_devs); - 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]); + if (pdata->leds) { + 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]); + } } ret = mfd_add_devices(lm3533->dev, 0, lm3533_led_devs, @@ -469,6 +478,26 @@ static int lm3533_device_setup(struct lm3533 *lm3533, return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp); } +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++; + } +} + static int lm3533_device_init(struct lm3533 *lm3533) { struct lm3533_platform_data *pdata = dev_get_platdata(lm3533->dev); @@ -477,8 +506,25 @@ static int lm3533_device_init(struct lm3533 *lm3533) dev_dbg(lm3533->dev, "%s\n", __func__); if (!pdata) { - 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; } lm3533->hwen = devm_gpiod_get(lm3533->dev, NULL, GPIOD_OUT_LOW); @@ -603,6 +649,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" }, { } @@ -612,6 +664,7 @@ MODULE_DEVICE_TABLE(i2c, lm3533_i2c_ids); static struct i2c_driver lm3533_i2c_driver = { .driver = { .name = "lm3533", + .of_match_table = lm3533_match_table, }, .id_table = lm3533_i2c_ids, .probe = lm3533_i2c_probe, diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c index babfd3ceec86..bc568916f1b1 100644 --- a/drivers/video/backlight/lm3533_bl.c +++ b/drivers/video/backlight/lm3533_bl.c @@ -258,6 +258,60 @@ static int lm3533_bl_setup(struct lm3533_bl *bl, return lm3533_ctrlbank_set_pwm(&bl->cb, pdata->pwm); } +static void lm3533_parse_backlight(struct platform_device *pdev, + 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; + + /* 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) +{ + 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)) { + ret = fwnode_property_read_u32(node, "reg", &val); + if (ret) { + dev_info(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); + } + } + } + + return 0; +} + static int lm3533_bl_probe(struct platform_device *pdev) { struct lm3533 *lm3533; @@ -275,8 +329,16 @@ static int lm3533_bl_probe(struct platform_device *pdev) 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); } if (pdev->id < 0 || pdev->id >= LM3533_HVCTRLBANK_COUNT) { @@ -299,9 +361,9 @@ static int lm3533_bl_probe(struct platform_device *pdev) props.type = BACKLIGHT_RAW; props.max_brightness = LM3533_BL_MAX_BRIGHTNESS; props.brightness = pdata->default_brightness; - 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), + pdev->dev.parent, bl, + &lm3533_bl_ops, &props); if (IS_ERR(bd)) { dev_err(&pdev->dev, "failed to register backlight device\n"); return PTR_ERR(bd); diff --git a/include/linux/mfd/lm3533.h b/include/linux/mfd/lm3533.h index 69059a7a2ce5..e22fd2093a95 100644 --- a/include/linux/mfd/lm3533.h +++ b/include/linux/mfd/lm3533.h @@ -74,6 +74,7 @@ struct lm3533_platform_data { enum lm3533_boost_freq boost_freq; struct lm3533_als_platform_data *als; + int num_als; struct lm3533_bl_platform_data *backlights; int num_backlights;
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. Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> --- drivers/iio/light/lm3533-als.c | 58 ++++++++++++++++++++- drivers/leds/leds-lm3533.c | 69 +++++++++++++++++++++++-- drivers/mfd/lm3533-core.c | 79 ++++++++++++++++++++++++----- drivers/video/backlight/lm3533_bl.c | 72 ++++++++++++++++++++++++-- include/linux/mfd/lm3533.h | 1 + 5 files changed, 256 insertions(+), 23 deletions(-)