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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250216144033.47852a60@jic23-huawei>
Date: Sun, 16 Feb 2025 14:40:33 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Svyatoslav Ryhel <clamor95@...il.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Lars-Peter Clausen <lars@...afoo.de>, Pavel Machek <pavel@....cz>, Daniel
 Thompson <danielt@...nel.org>, Jingoo Han <jingoohan1@...il.com>, Helge
 Deller <deller@....de>, Andy Shevchenko
 <andriy.shevchenko@...ux.intel.com>, Uwe Kleine-König
 <u.kleine-koenig@...libre.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
 linux-leds@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v1 2/2] mfd: lm3533: convert to use OF

On Wed, 12 Feb 2025 09:58:42 +0200
Svyatoslav Ryhel <clamor95@...il.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@...il.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));