Re: [PATCH v2] mfd: core: Support auxiliary device

[Raag Jadav]

On Mon, Apr 07, 2025 at 11:44:50AM +0300, Andy Shevchenko wrote:
> On Mon, Apr 07, 2025 at 01:16:14PM +0530, Raag Jadav wrote:
> > Extend MFD subsystem to support auxiliary child device. This is useful
> > for MFD usecases where parent device is on a discoverable bus and doesn't
> > fit into the platform device criteria. Purpose of this implementation is
> > to provide discoverable MFDs just enough infrastructure to register
> > independent child devices with their own memory and interrupt resources
> > without abusing the platform device.
> > 
> > Current support is limited to just PCI type MFDs, but this can be further
> > extended to support other types like USB in the future.
> 
> > PS: I'm leaning towards not doing any of the ioremap or regmap on MFD
> > side and think that we should enforce child devices to not overlap.
> 
> Yes, but we will have the cases in the future, whatever,
> for the first step it's okay.

I've always found such devices to have a parent specific functionality
that fall under a specific subsystem instead of needing a generic MFD for
it. But I'd love to be surprised.

> > If there's a need to handle common register access by parent device,
> > then I think it warrants its own driver which adds auxiliary devices
> > along with a custom interface to communicate with them, and MFD on
> > AUX is not the right solution for it.
> 
> ...
> 
> > -static const struct device_type mfd_dev_type = {
> > -	.name	= "mfd_device",
> > +enum mfd_dev {
> > +	MFD_AUX_DEV,
> > +	MFD_PLAT_DEV,
> > +	MFD_MAX_DEV
> > +};
> > +
> > +static const struct device_type mfd_dev_type[MFD_MAX_DEV] = {
> > +	[MFD_AUX_DEV]	= { .name = "mfd_auxiliary_device" },
> > +	[MFD_PLAT_DEV]	= { .name = "mfd_platform_device" },
> >  };
> 
> This is likely an ABI breakage if anything looks in sysfs for mfd_device.

I have no insight on the usecase here. Can you please elaborate?

> > +static int mfd_remove_devices_fn(struct device *dev, void *data)
> > +{
> > +	if (dev->type == &mfd_dev_type[MFD_AUX_DEV])
> > +		return mfd_remove_auxiliary_device(dev);
> 
> > +	else if (dev->type == &mfd_dev_type[MFD_PLAT_DEV])
> 
> Redundant 'else'
> 
> > +		return mfd_remove_platform_device(dev, data);
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +#ifndef MFD_AUX_H
> > +#define MFD_AUX_H
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/ioport.h>
> 
> > +#include <linux/types.h>
> 
> How is this one being used?

Ah, since it's not so easy to come across a file without a type, I've grown
a habit of throwing this in without a thought. Thanks for catching it.

> > +#define auxiliary_dev_to_mfd_aux_dev(auxiliary_dev) \
> > +	container_of(auxiliary_dev, struct mfd_aux_device, auxdev)
> 
> Missing container_of.h and better to define after the data type as it can be
> converted to static inline, if required.

Sure.

> > +/*
> > + * Common structure between MFD parent and auxiliary child device.
> > + * To be used by leaf drivers to access child device resources.
> > + */
> > +struct mfd_aux_device {
> > +	struct auxiliary_device auxdev;
> 
> > +	struct resource	mem;
> > +	struct resource	irq;
> > +	/* Place holder for other types */
> > +	struct resource	ext;
> 
> Why this can't be simply a VLA?

Because it requires resouce identification, and with that we're back to
platform style get_resource() and friends.

> > +};
> > +
> > +#endif
> 
> ...
> 
> > +/* TODO: Convert the platform device abusers and remove this flag */
> > +#define MFD_AUX_TYPE	INT_MIN
> 
> INT_MIN?! This is a bit unintuitive. BIT(31) sounds better to me.
> Or even a plain (decimal) number as PLATFORM_DEVID_* done, for example.

I thought a specific number would rather raise more questions, but sure.

Raag