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

[Lee Jones <lee@kernel.org>]

On Fri, 23 May 2025, Nuno Sá wrote:

> On Fri, 2025-05-23 at 15:51 +0100, Lee Jones wrote:
> > 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..806867c56d6fb4ef1f461af26a424a3a05
> > > f46575 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_*()?
> 
> I do mention in the cover why I did not used devm_mfd_add_devices(). We would be
> adding an action per device and mfd_remove_devices() removes all of them in one
> call. Not that is an issue (I believe subsequent calls with be kind of no-ops)
> but this way felt more correct.

I haven't seen another device add a .remove() equivalent per device.

Why do you need it?  What's the use-case where this would become critical?

-- 
Lee Jones [李琼斯]