Re: [PATCH v9 5/6] arm64: dts: amlogic: t7: Add i2c controller node

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Probe fails for generic Khadas MCU devices because match data evaluates to NULL.
- [Medium] The fan regulator is leaked on probe error and driver unbind.
--

--- Patch [5]: [PATCH v9 5/6] arm64: dts: amlogic: t7: Add i2c controller node ---
commit b6b3cf68d983172e0dc3e121aff82af7de674ddb
Author: Ronald Claveau <[email protected]>

arm64: dts: amlogic: t7: Add i2c controller node

Add the T7 i2c controller node used by the Khadas VIM4 for MCU communication.
Use amlogic,meson-axg-i2c as fallback compatible.

> diff --git a/drivers/mfd/khadas-mcu.c b/drivers/mfd/khadas-mcu.c
> index ba981a7886921..4cb305ac915e1 100644
> --- a/drivers/mfd/khadas-mcu.c
> +++ b/drivers/mfd/khadas-mcu.c
[ ... ]
> @@ -75,47 +75,138 @@ static const struct regmap_config khadas_mcu_regmap_config = {
[ ... ]
>  static int khadas_mcu_probe(struct i2c_client *client)
>  {
> +	const struct mfd_cell *cells, *fan_cells;
> +	const struct regmap_config *regmap_cfg;
>  	struct device *dev = &client->dev;
> +	int ncells, nfan_cells, ret;
>  	struct khadas_mcu *ddata;
> -	int ret;
> +	const void *match;
>  
>  	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
>  	if (!ddata)
>  		return -ENOMEM;
>  
> +	match = i2c_get_match_data(client);
> +	if (!match)
> +		return -ENODEV;
> +
> +	switch ((uintptr_t)match) {
> +	case KHADAS_MCU_GENERIC:

[Severity: High]
Does this check unintentionally abort probe for generic Khadas MCU devices?

Because KHADAS_MCU_GENERIC is 0, the data field in the match table is
effectively a null pointer:

    { .compatible = "khadas,mcu", .data = (void *)KHADAS_MCU_GENERIC }

When khadas_mcu_probe() is called for a generic device, i2c_get_match_data()
returns NULL. The check for (!match) is met, and the driver unconditionally
returns -ENODEV. This causes a regression for all generic Khadas MCU boards.

[ ... ]
> diff --git a/drivers/thermal/khadas_mcu_fan.c b/drivers/thermal/khadas_mcu_fan.c
> index 21b3d0a71bd0d..c7f74394b823f 100644
> --- a/drivers/thermal/khadas_mcu_fan.c
> +++ b/drivers/thermal/khadas_mcu_fan.c
[ ... ]
> @@ -78,6 +81,7 @@ static const struct thermal_cooling_device_ops khadas_mcu_fan_cooling_ops = {
[ ... ]
>  static int khadas_mcu_fan_probe(struct platform_device *pdev)
>  {
> +	const struct khadas_mcu_fan_pdata *pdata = dev_get_platdata(&pdev->dev);
>  	struct khadas_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
>  	struct thermal_cooling_device *cdev;
>  	struct device *dev = &pdev->dev;
> @@ -87,7 +91,22 @@ static int khadas_mcu_fan_probe(struct platform_device *pdev)
>  	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>  	if (!ctx)
>  		return -ENOMEM;
> +
>  	ctx->mcu = mcu;
> +	ctx->fan_reg = pdata->fan_reg;
> +	ctx->levels = pdata->levels;
> +	ctx->nlevels = pdata->nlevels;
> +
> +	ctx->power = devm_regulator_get(dev->parent, "fan");
> +	if (IS_ERR(ctx->power))
> +		return PTR_ERR(ctx->power);
> +
> +	ret = regulator_enable(ctx->power);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable fan power supply: %d\n", ret);
> +		return ret;
> +	}
> +
>  	platform_set_drvdata(pdev, ctx);
>  
>  	cdev = devm_thermal_of_child_cooling_device_register(dev->parent,

[Severity: Medium]
Could this leak the regulator on probe error or driver unbind?

If devm_thermal_of_child_cooling_device_register() fails later in
khadas_mcu_fan_probe(), the function returns without calling
regulator_disable().

Additionally, there is no remove callback in khadas_mcu_fan_driver:

static struct platform_driver khadas_mcu_fan_driver = {
    .probe      = khadas_mcu_fan_probe,
    .shutdown   = khadas_mcu_fan_shutdown,
    .driver = { ...

This means the regulator is not cleanly disabled when the driver is unbound,
which could leave the reference leaked and prevent the regulator from ever
being powered down.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic