Re: [PATCH v3 02/22] mfd: adp5585: only add devices given in FW

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

On Tue, 2025-05-13 at 15:34 +0100, Lee Jones wrote:
> On Mon, 12 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 | 45 +++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 41 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
> > index
> > 160e0b38106a6d78f7d4b7c866cb603d96ea673e..02f9e8c1c6a1d8b9516c060e0024d69886
> > e9fb7a 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,12 +116,40 @@ static const struct regmap_config
> > adp5585_regmap_configs[] = {
> >  	},
> >  };
> >  
> > +static int adp5585_parse_fw(struct device *dev, struct adp5585_dev
> > *adp5585,
> > +			    struct mfd_cell **devs)
> > +{
> > +	unsigned int has_pwm = 0, has_gpio = 0, rc = 0;
> > +
> > +	if (device_property_present(dev, "#pwm-cells"))
> > +		has_pwm = 1;
> 
> This is a little sloppy.  Instead of using throwaway local variables, do
> what you're going to do in the if statement.
> 

Then I would need to realloc my device cells... But as I realized below, this is
indeed not needed.

> > +	if (device_property_present(dev, "#gpio-cells"))
> > +		has_gpio = 1;
> > +
> > +	if (!has_pwm && !has_gpio)
> > +		return -ENODEV;
> 
> Are we really dictating which child devices to register based on random
> DT properties?  Why not register them anyway and have them fail if the
> information they need is not available?  Missing / incorrect properties
> usually get a -EINVAL.

Well, this was something Laurent asked for... In the previous version I was
registering all the devices unconditionally.
 
> 
> > +	*devs = devm_kcalloc(dev, has_pwm + has_gpio, sizeof(struct
> > mfd_cell),
> > +			     GFP_KERNEL);
> > +	if (!*devs)
> > +		return -ENOMEM;
> > +
> > +	if (has_pwm)
> > +		(*devs)[rc++] = adp5585_devs[ADP5585_DEV_PWM];
> > +	if (has_gpio)
> > +		(*devs)[rc++] = adp5585_devs[ADP5585_DEV_GPIO];
> 
> Passing around pointers to pointers for allocation (and later, pointer
> to functions) is not the way we wish to operate.  See how all of the
> other MFD drivers handle selective sub-drivers.

Any pointer from the top of your head (example driver)? Honestly, I do not see
this being that bad. Pretty much is a dynamic array of struct mfd_cel but
anyways, no strong feelings

But... I was actually being very stupid. First I did looked at an API to only
add one mfd device and failed to realize that I can use devm_mfd_add_devices()
with n_devs = 1

Nevermind, will refactor in v4

> 
> > +	return rc;
> > +}
> > +
> >  static int adp5585_i2c_probe(struct i2c_client *i2c)
> >  {
> >  	const struct regmap_config *regmap_config;
> >  	struct adp5585_dev *adp5585;
> > +	struct mfd_cell *devs;
> >  	unsigned int id;
> > -	int ret;
> > +	int ret, n_devs;
> >  
> >  	adp5585 = devm_kzalloc(&i2c->dev, sizeof(*adp5585), GFP_KERNEL);
> >  	if (!adp5585)
> > @@ -138,9 +172,12 @@ static int adp5585_i2c_probe(struct i2c_client *i2c)
> >  		return dev_err_probe(&i2c->dev, -ENODEV,
> >  				     "Invalid device ID 0x%02x\n", id);
> >  
> > +	n_devs = adp5585_parse_fw(&i2c->dev, adp5585, &devs);
> > +	if (n_devs < 0)
> > +		return n_devs;
> > +
> >  	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> > -				   adp5585_devs, ARRAY_SIZE(adp5585_devs),
> > -				   NULL, 0, NULL);
> > +				   devs, n_devs, NULL, 0, NULL);
> >  	if (ret)
> >  		return dev_err_probe(&i2c->dev, ret,
> >  				     "Failed to add child devices\n");
> > 
> > -- 
> > 2.49.0
> > 
> >