Re: [PATCH v2 2/6] mfd: lm3533: Convert to use OF bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The `lm3533_cells` array is allocated on the stack but never zero-initialized, causing it to contain random garbage data.
- [High] The cell iterator logic increments the `count` variable for every child node unconditionally, even if the node does not match any expected compatible strings.
- [Medium] The ALS driver attempts to free an invalid IRQ number on error paths and during device removal when no IRQ is present.
- [Medium] Uninitialized stack memory is printed to the kernel log if `fwnode_property_read_u32()` fails to read the "reg" property.
- [Low] The patch introduces basic syntax and scoping errors in `lm3533_als_setup()` that completely prevent compilation.
- [Low] The backlight device tree `reg` property is validated against the wrong maximum limit in the MFD core.
- [Low] Passing an `int *` to a function expecting a `u32 *` causes compiler warnings/errors and breaks the build.
--

commit 9af50d17aae60cc268d70b24e00ddaa3a0500388
Author: Svyatoslav Ryhel <clamor95@gmail.com>

mfd: lm3533: Convert to use OF bindings

Since there are no users of this driver via platform data, remove the
platform data support and switch to using Device Tree bindings.
Additionally, optimize functions used only by platform data.

> diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> index 99f0b903018cf..cbd337b73bd9f 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c

[ ... ]

> @@ -714,59 +720,33 @@ static const struct attribute_group lm3533_als_attribute_group = {
>  	.attrs = lm3533_als_attributes
>  };
>  
> -static int lm3533_als_set_input_mode(struct lm3533_als *als, bool pwm_mode)
> +static int lm3533_als_setup(struct lm3533_als *als)
>  {
> -	u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> -	u8 val;
> +	struct device *dev = &als->pdev.dev;

[Severity: Low]
Does this syntax cause a build error? The pointer als->pdev requires
the -> operator to access dev, like &als->pdev->dev.

>  	int ret;
>  
> -	if (pwm_mode)
> -		val = mask;	/* pwm input */
> -	else
> -		val = 0;	/* analog input */
> -
> -	ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
> -	if (ret) {
> -		dev_err(&als->pdev->dev, "failed to set input mode %d\n",
> -								pwm_mode);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int lm3533_als_set_resistor(struct lm3533_als *als, u8 val)
> -{
> -	int ret;
> -
> -	if (val < LM3533_ALS_RESISTOR_MIN || val > LM3533_ALS_RESISTOR_MAX) {
> -		dev_err(&als->pdev->dev, "invalid resistor value\n");
> -		return -EINVAL;
> -	}
> -
> -	ret = lm3533_write(als->lm3533, LM3533_REG_ALS_RESISTOR_SELECT, val);
> -	if (ret) {
> -		dev_err(&als->pdev->dev, "failed to set resistor\n");
> -		return ret;
> -	}
> +	device_property_read_u32(dev, "ti,resistor-value-ohm",
> +				 &als->r_select);
>  
> -	return 0;
> -}
> +	als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> +			      LM3533_ALS_RESISTOR_MAX);
> +	als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
>  
> -static int lm3533_als_setup(struct lm3533_als *als,
> -			    const struct lm3533_als_platform_data *pdata)
> -{
> -	int ret;
> +	als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
>  
> -	ret = lm3533_als_set_input_mode(als, pdata->pwm_mode);
> +	ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, als->pwm_mode ?

[Severity: Low]
Will this fail to compile since lm3533 is undeclared in this scope? It
appears this should use als->lm3533 instead.

[ ... ]

> @@ -838,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;
> @@ -852,25 +825,28 @@ 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);
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	als = iio_priv(indio_dev);
>  	als->lm3533 = lm3533;
>  	als->pdev = pdev;
> -	als->irq = lm3533->irq;
> +	als->irq = platform_get_irq_optional(pdev, 0);

[Severity: Medium]
Will this cause problems during error handling or device removal?
platform_get_irq_optional() returns negative error codes when no IRQ
is present. The existing cleanup paths check if (als->irq) which evaluates
to true for negative numbers, causing it to call free_irq() with an
invalid IRQ number. Should the cleanup checks be updated to
if (als->irq > 0)?

[ ... ]

> diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> index 0a2409d00b2e0..8495e91198711 100644
> --- a/drivers/mfd/lm3533-core.c
> +++ b/drivers/mfd/lm3533-core.c

[ ... ]

> -static int lm3533_device_init(struct lm3533 *lm3533)
> +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_cells[LM3533_CELLS_MAX];

[Severity: High]
Does this array need to be zero-initialized? Because it's allocated on
the stack, fields like num_resources and resources will contain garbage
memory. When mfd_add_devices() processes it, could this lead to wild
pointer dereferences?

> +	u32 count = 0, reg;
>  	int ret;
>  
> -	dev_dbg(lm3533->dev, "%s\n", __func__);
> -
> -	if (!pdata) {
> -		dev_err(lm3533->dev, "no platform data\n");
> -		return -EINVAL;
> +	lm3533_enable(lm3533);
>  
> -	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");
> -
> -	lm3533_enable(lm3533);
> -
> -	ret = lm3533_device_setup(lm3533, pdata);
> -	if (ret)
> +	device_for_each_child_node_scoped(lm3533->dev, child) {
> +		if (!fwnode_device_is_available(child))
> +			continue;
>  
> -	lm3533_device_als_init(lm3533);
> -	lm3533_device_bl_init(lm3533);
> -	lm3533_device_led_init(lm3533);
> -
> -	ret = sysfs_create_group(&lm3533->dev->kobj, &lm3533_attribute_group);
> -	if (ret < 0) {
> -		dev_err(lm3533->dev, "failed to create sysfs attributes\n");
> -		goto err_unregister;
> +		if (count >= LM3533_CELLS_MAX)
> +			break;
>  
> +		if (fwnode_device_is_compatible(child, "ti,lm3533-als")) {
> +			lm3533_cells[count].name = "lm3533-als";
> +			lm3533_cells[count].id = PLATFORM_DEVID_NONE;
> +			lm3533_cells[count].of_compatible = "ti,lm3533-als";
>  
> +			lm3533->have_als = true;
> +		}
>  
> +		if (fwnode_device_is_compatible(child, "ti,lm3533-backlight")) {
> +			ret = fwnode_property_read_u32(child, "reg", &reg);
> +			if (ret || reg > LM3533_HVLED_ID_MAX) {

[Severity: Low]
Is LM3533_HVLED_ID_MAX the correct boundary to check here? The backlight IDs
should typically be validated against LM3533_BL_ID_MAX. Since
LM3533_HVLED_ID_MAX is 2, it would allow invalid backlight IDs to bypass
this check.

> +				dev_err(dev, "invalid backlight reg %d\n", reg);

[Severity: Medium]
If fwnode_property_read_u32() returns an error, reg remains uninitialized.
Could this print stack garbage memory to the kernel log?

> +				continue;
> +			}
>  
> +			lm3533_cells[count].name = "lm3533-backlight";
> +			lm3533_cells[count].id = reg;
> +			lm3533_cells[count].of_compatible = "ti,lm3533-backlight";
>  
> +			lm3533->have_backlights = true;
> +		}
>  
> +		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 reg %d\n", reg);
> +				continue;
> +			}
>  
> +			lm3533_cells[count].name = "lm3533-leds";
> +			lm3533_cells[count].id = reg - LM3533_HVLED_ID_MAX;
> +			lm3533_cells[count].of_compatible = "ti,lm3533-leds";
>  
> +			lm3533->have_leds = true;
> +		}
>  
> +		count++;

[Severity: High]
Does count increment unconditionally even if the child node didn't match
any of the compatible strings? If an unrelated child node is present, it
would increment count while leaving the lm3533_cells[count] entirely
unpopulated. When mfd_add_devices() is called, this could result in a NULL
pointer dereference on the .name field.

[ ... ]

> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> index babfd3ceec866..42da652df58d9 100644
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c

[ ... ]

> @@ -295,13 +293,20 @@ static int lm3533_bl_probe(struct platform_device *pdev)
>  	bl->cb.id = lm3533_bl_get_ctrlbank_id(bl);
>  	bl->cb.dev = NULL;			/* until registered */
>  
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%d",
> +			      pdev->name, pdev->id);
> +	if (!name)
> +		return -ENOMEM;
> +
>  	memset(&props, 0, sizeof(props));
>  	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);
> +	props.brightness = LM3533_BL_MAX_BRIGHTNESS;
> +	device_property_read_u32(&pdev->dev, "default-brightness",
> +				 &props.brightness);

[Severity: Low]
Will this pass an incompatible pointer type? props.brightness is defined as
an int in struct backlight_properties, but device_property_read_u32() expects
a u32 pointer. This could cause compiler warnings or errors.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528135123.103745-1-clamor95@gmail.com?part=2