Re: [PATCH v4 02/20] mfd: adp5585: only add devices given in FW

[Lee Jones <lee@kernel.org>]

On Wed, 21 May 2025, Nuno Sá via B4 Relay wrote:

> From: Nuno Sá <nuno.sa@analog.com>
> 
> Not all devices (features) of the adp5585 device are mandatory to be
> used in all platforms. Hence, check what's given in FW and dynamically
> create the mfd_cell array to be given to devm_mfd_add_devices().
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/mfd/adp5585.c | 48 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
> index 160e0b38106a6d78f7d4b7c866cb603d96ea673e..806867c56d6fb4ef1f461af26a424a3a05f46575 100644
> --- a/drivers/mfd/adp5585.c
> +++ b/drivers/mfd/adp5585.c
> @@ -17,7 +17,13 @@
>  #include <linux/regmap.h>
>  #include <linux/types.h>
>  
> -static const struct mfd_cell adp5585_devs[] = {
> +enum {
> +	ADP5585_DEV_GPIO,
> +	ADP5585_DEV_PWM,
> +	ADP5585_DEV_MAX
> +};
> +
> +static const struct mfd_cell adp5585_devs[ADP5585_DEV_MAX] = {
>  	{ .name = "adp5585-gpio", },
>  	{ .name = "adp5585-pwm", },
>  };
> @@ -110,6 +116,37 @@ static const struct regmap_config adp5585_regmap_configs[] = {
>  	},
>  };
>  
> +static void adp5585_remove_devices(void *dev)
> +{
> +	mfd_remove_devices(dev);
> +}
> +
> +static int adp5585_add_devices(struct device *dev)
> +{
> +	int ret;
> +
> +	if (device_property_present(dev, "#pwm-cells")) {
> +		ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> +				      &adp5585_devs[ADP5585_DEV_PWM], 1, NULL, 0, NULL);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to add pwm device\n");

PWM is an acronym, it should be capitalised.

> +	}
> +
> +	if (device_property_present(dev, "#gpio-cells")) {
> +		ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> +				      &adp5585_devs[ADP5585_DEV_GPIO], 1, NULL, 0, NULL);
> +		if (ret) {
> +			ret = dev_err_probe(dev, ret, "Failed to add gpio device\n");

Same with GPIO.

> +			goto out_error;
> +		}
> +	}
> +
> +	return devm_add_action_or_reset(dev, adp5585_remove_devices, dev);

We have 2 of these now.

Why do we need lots of unbinding functions?

What's wrong .remove() or devm_*()?

> +out_error:
> +	mfd_remove_devices(dev);
> +	return ret;
> +}
> +
>  static int adp5585_i2c_probe(struct i2c_client *i2c)
>  {
>  	const struct regmap_config *regmap_config;
> @@ -138,14 +175,7 @@ static int adp5585_i2c_probe(struct i2c_client *i2c)
>  		return dev_err_probe(&i2c->dev, -ENODEV,
>  				     "Invalid device ID 0x%02x\n", id);
>  
> -	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> -				   adp5585_devs, ARRAY_SIZE(adp5585_devs),
> -				   NULL, 0, NULL);
> -	if (ret)
> -		return dev_err_probe(&i2c->dev, ret,
> -				     "Failed to add child devices\n");
> -
> -	return 0;
> +	return adp5585_add_devices(&i2c->dev);
>  }
>  
>  static int adp5585_suspend(struct device *dev)
> 
> -- 
> 2.49.0
> 
> 

-- 
Lee Jones [李琼斯]