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

[Lee Jones <lee@kernel.org>]

On Tue, 13 May 2025, 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..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
> 
> 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.
> 
> > 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.

Yes, likewise.  Never worry about that.

In general let's try to simplify things by not using pointers to
pointers and pointers to functions.  There are usually much nicer,
cleaner and simpler solutions.

IMHO, the above is C-hackery at its best.

Let's see where v4 takes us.

-- 
Lee Jones [李琼斯]