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 17:19 +0200, Laurent Pinchart wrote:
> On Tue, May 13, 2025 at 04:02:11PM +0100, Nuno Sá wrote:
> > 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..02f9e8c1c6a1d8b9516c060e0024d6
> > > > 9886
> > > > 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
> 
> The properties are not random.
> 
> > > 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.
> 
> Registering them all means we'll get error messages in the kernel log
> when the corresponding drivers will probe, while nothing is actually
> wrong. That's fairly confusing for the user.
> 
> In an ideal situation we would have child nodes in DT and only register
> child devices for existing child nodes. Unfortunately the DT bindings
> were not designed that way, so we have to live with the current
> situation.
> 
> > > > +	*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
> 
> I don't find it that bad either. I don't think you should use
> devm_kcalloc() though, as the memory should be freed as soon as it's not
> needed anymore.
> 

Agreed... Sometimes I do forget we also have __free(kfree) even though I used it
in the keyboard driver.

> > But... I was actually being very stupid. First I did looked at an API to
> > only
> 
> Occasionally overseeing a possible solution isn't being stupid. Or at
> least I hope it isn't, otherwise I would be very stupid too.
> 

I know :)... 

> > 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");
> > > >