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

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2025-05-23 at 16:19 +0100, Lee Jones wrote:
> 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..806867c56d6fb4ef1f461af26a424a
> > > > 3a05
> > > > 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?

No sure I'm following you. I don't need a .remove() per device (or it is not
critical to have one). I just went with this because devm_mfd_add_devices()
would be adding more devres_add() than what we need given that
mfd_remove_devices() removes all child devices at once. So, logically, the above
makes sense to me. Now, I'm ok if you say, don't bother with this and just use 
devm_mfd_add_devices() on every device we want to add.

- Nuno Sá