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

[Jonathan Cameron <jic23@kernel.org>]

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