Re: [PATCH v3 3/4] mfd: aat2870: Convert to use OF bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] video: missing default value for optional led-max-microamp property
- [Medium] mfd: aat2870: WARNING triggered by gpiod_set_value on sleepable GPIOs
- [Low] video: incompatible pointer type in device_property_read_u32
- [Low] regulator: missing const on aat2870_regulators array
--

commit 44f47c7fe6dfacca37d21c28bab7fce4a68f379a
Author: Svyatoslav Ryhel <clamor95@gmail.com>

mfd: aat2870: Convert to use OF bindings

This commit converts the AAT2870 driver to use OF bindings by replacing
mfd_add_devices() with of_platform_populate() and adding separate compatible
strings for its backlight and regulator sub-devices. It also updates the
GPIO enable logic to use gpiod helpers and modifies the child drivers to parse
properties directly from the Device Tree, allowing the removal of platform data.

> diff --git a/drivers/mfd/aat2870-core.c b/drivers/mfd/aat2870-core.c
> index 0d56cd6fbc6a5..909c14ee60323 100644
> --- a/drivers/mfd/aat2870-core.c
> +++ b/drivers/mfd/aat2870-core.c
[ ... ]
> @@ -348,60 +309,29 @@ static int aat2870_i2c_probe(struct i2c_client *client)
>  
>  	aat2870->reg_cache = aat2870_regs;
>  
> -	if (pdata->en_pin < 0)
> -		aat2870->en_pin = -1;
> -	else
> -		aat2870->en_pin = pdata->en_pin;
> +	aat2870->en_pin = devm_gpiod_get_optional(&client->dev, "enable",
> +						  GPIOD_OUT_HIGH);
> +	if (IS_ERR(aat2870->en_pin))
> +		return dev_err_probe(&client->dev, PTR_ERR(aat2870->en_pin),
> +				     "Failed to get EN GPIO\n");
>  
> -	aat2870->init = pdata->init;
> -	aat2870->uninit = pdata->uninit;
>  	aat2870->read = aat2870_read;
>  	aat2870->write = aat2870_write;
>  	aat2870->update = aat2870_update;
>  
>  	mutex_init(&aat2870->io_lock);
>  
> -	if (aat2870->init)
> -		aat2870->init(aat2870);
> -
> -	if (aat2870->en_pin >= 0) {
> -		ret = devm_gpio_request_one(&client->dev, aat2870->en_pin,
> -					GPIOF_OUT_INIT_HIGH, "aat2870-en");
> -		if (ret < 0) {
> -			dev_err(&client->dev,
> -				"Failed to request GPIO %d\n", aat2870->en_pin);
> -			return ret;
> -		}
> -	}
> -
> -	aat2870_enable(aat2870);
> -
> -	for (i = 0; i < pdata->num_subdevs; i++) {
> -		for (j = 0; j < ARRAY_SIZE(aat2870_devs); j++) {
> -			if ((pdata->subdevs[i].id == aat2870_devs[j].id) &&
> -					!strcmp(pdata->subdevs[i].name,
> -						aat2870_devs[j].name)) {
> -				aat2870_devs[j].platform_data =
> -					pdata->subdevs[i].platform_data;
> -				break;
> -			}
> -		}
> -	}
> +	gpiod_set_value(aat2870->en_pin, 1);

[Severity: Medium]
Should this use gpiod_set_value_cansleep() instead?

Since this device is an I2C MFD, the enable GPIO could be attached to an
I2C or SPI expander, which means toggling it might sleep. Using the
non-cansleep gpiod_set_value() here, as well as in the probe error path and
suspend/resume callbacks, could trigger a WARN_ON() in gpiolib if the
underlying controller requires sleeping.

>  
> -	ret = mfd_add_devices(aat2870->dev, 0, aat2870_devs,
> -			      ARRAY_SIZE(aat2870_devs), NULL, 0, NULL);
> -	if (ret != 0) {
> -		dev_err(aat2870->dev, "Failed to add subdev: %d\n", ret);
> -		goto out_disable;
> +	ret = devm_of_platform_populate(&client->dev);
> +	if (ret) {
> +		gpiod_set_value(aat2870->en_pin, 0);

[Severity: Medium]
Similar to the above, would gpiod_set_value_cansleep() be safer here on the
error path to avoid a potential gpiolib WARN_ON()?

> +		return dev_err_probe(&client->dev, ret, "Failed to populate cells\n");
>  	}
>  
>  	aat2870_init_debugfs(aat2870);
>  
>  	return 0;
> -
> -out_disable:
> -	aat2870_disable(aat2870);
> -	return ret;
>  }
>  
>  static int aat2870_i2c_suspend(struct device *dev)
> @@ -409,7 +339,7 @@ static int aat2870_i2c_suspend(struct device *dev)
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct aat2870_data *aat2870 = i2c_get_clientdata(client);
>  
> -	aat2870_disable(aat2870);
> +	gpiod_set_value(aat2870->en_pin, 0);

[Severity: Medium]
Should we use gpiod_set_value_cansleep() here in the suspend handler?

>  
>  	return 0;
>  }
> @@ -421,7 +351,7 @@ static int aat2870_i2c_resume(struct device *dev)
>  	struct aat2870_register *reg = NULL;
>  	int i;
>  
> -	aat2870_enable(aat2870);
> +	gpiod_set_value(aat2870->en_pin, 1);

[Severity: Medium]
Should we use gpiod_set_value_cansleep() here in the resume handler?

>  
>  	/* restore registers */
>  	for (i = 0; i < AAT2870_REG_NUM; i++) {
> diff --git a/drivers/regulator/aat2870-regulator.c b/drivers/regulator/aat2870-regulator.c
> index 970d86f2bbb81..9b8eb8df220a7 100644
> --- a/drivers/regulator/aat2870-regulator.c
> +++ b/drivers/regulator/aat2870-regulator.c
[ ... ]
> @@ -101,84 +102,60 @@ static const unsigned int aat2870_ldo_voltages[] = {
>  	3000000, 3100000, 3200000, 3300000,
>  };
>  
> -#define AAT2870_LDO(ids)				\
> -	{						\
> -		.desc = {				\
> -			.name = #ids,			\
> -			.id = AAT2870_ID_##ids,		\
> -			.n_voltages = ARRAY_SIZE(aat2870_ldo_voltages),	\
> -			.volt_table = aat2870_ldo_voltages, \
> -			.ops = &aat2870_ldo_ops,	\
> -			.type = REGULATOR_VOLTAGE,	\
> -			.owner = THIS_MODULE,		\
> -		},					\
> -	}
> +#define AAT2870_LDO(ids, match, vsel, vmask) {		\
> +	.name = #ids,					\
> +	.of_match = (match),				\
> +	.id = AAT2870_ID_##ids,				\
> +	.n_voltages = ARRAY_SIZE(aat2870_ldo_voltages),	\
> +	.volt_table = aat2870_ldo_voltages,		\
> +	.ops = &aat2870_ldo_ops,			\
> +	.type = REGULATOR_VOLTAGE,			\
> +	.owner = THIS_MODULE,				\
> +	.vsel_reg = (vsel),				\
> +	.vsel_mask = (vmask),				\
> +	.enable_reg = AAT2870_LDO_EN,			\
> +	.enable_mask = BIT(AAT2870_ID_##ids),		\
> +}
>  
> -static struct aat2870_regulator aat2870_regulators[] = {
> -	AAT2870_LDO(LDOA),
> -	AAT2870_LDO(LDOB),
> -	AAT2870_LDO(LDOC),
> -	AAT2870_LDO(LDOD),
> +static struct regulator_desc aat2870_regulators[] = {

[Severity: Low]
Could we add the const qualifier to this array?

Since the descriptions are static and never modified at runtime, and
devm_regulator_register() expects a const struct regulator_desc *, adding
const would allow this array to be placed in read-only .rodata memory
instead of mutable .data.

> +	AAT2870_LDO(LDOA, "ldo-a", AAT2870_LDO_AB, AAT2870_LDO_AB_A_MASK),
> +	AAT2870_LDO(LDOB, "ldo-b", AAT2870_LDO_AB, AAT2870_LDO_AB_B_MASK),
> +	AAT2870_LDO(LDOC, "ldo-c", AAT2870_LDO_CD, AAT2870_LDO_CD_C_MASK),
> +	AAT2870_LDO(LDOD, "ldo-d", AAT2870_LDO_CD, AAT2870_LDO_CD_D_MASK),
>  };
>  
> diff --git a/drivers/video/backlight/aat2870_bl.c b/drivers/video/backlight/aat2870_bl.c
> index eb686e315921d..705a8d32d224f 100644
> --- a/drivers/video/backlight/aat2870_bl.c
> +++ b/drivers/video/backlight/aat2870_bl.c
[ ... ]
> @@ -129,21 +124,20 @@ static int aat2870_bl_probe(struct platform_device *pdev)
>  
>  	aat2870_bl->bd = bd;
>  
> -	if (pdata->channels > 0)
> -		aat2870_bl->channels = pdata->channels;
> -	else
> -		aat2870_bl->channels = AAT2870_BL_CH_ALL;
> +	aat2870_bl->channels = AAT2870_BL_CH_ALL;
> +	device_property_read_u8(&pdev->dev, "skyworks,channels", &aat2870_bl->channels);
>  
> -	if (pdata->max_current > 0)
> -		aat2870_bl->max_current = pdata->max_current;
> -	else
> -		aat2870_bl->max_current = AAT2870_CURRENT_27_9;
> +	device_property_read_u32(&pdev->dev, "led-max-microamp", &aat2870_bl->max_current);

[Severity: Low]
Does this cause an incompatible pointer type warning?

aat2870_bl->max_current is defined as a signed int, but
device_property_read_u32() expects a u32 pointer. This could break
builds that treat -Wincompatible-pointer-types as an error.

[Severity: Medium]
What happens if the optional "led-max-microamp" property is missing from
the device tree?

If the property is missing, device_property_read_u32() leaves max_current
at 0 (its initial value from devm_kzalloc). When passed into clamp(0, 450,
27900), it gets forced to 450, and dividing by 900 evaluates to 0.

This sets bd->props.max_brightness to 1 (0 + 1), silently degrading the
backlight limits to an almost invisible state, whereas the previous behavior
correctly defaulted to 27.9mA (32 steps). Should max_current be initialized
to a default value before reading the property?

> +	aat2870_bl->max_current = clamp(aat2870_bl->max_current, AAT2870_CURRENT_MIN,
> +					AAT2870_CURRENT_MAX);
> +	aat2870_bl->max_current /= AAT2870_CURRENT_STEP;
>  
>  	/*
>  	 * The maximum number of brightness steps is determined by the max current.
> -	 * The aat2870_current enum maps directly to the allowed brightness steps.
> +	 * The aat2870_bl->max_current value maps to the allowed brightness steps
> +	 * with +1 offset since max_current 0 is still 450uA.
>  	 */
> -	bd->props.max_brightness = pdata->max_current;
> +	bd->props.max_brightness = aat2870_bl->max_current + 1;
>  
>  	aat2870_bl->brightness = 0;
>  	bd->props.power = BACKLIGHT_POWER_ON;

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