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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Apr 08, 2025 at 10:58:06AM +0300, Raag Jadav wrote:
> 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:

...

> > > 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.

We have very "nice" MFD user, which blows up all issues with shared resources
and so on, look at drivers/mfd/sm501.c. The most problematic part there is
request_region().

> > > 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?

drivers/base/core.c

        if (dev->type && dev->type->name)
		add_uevent_var(env, "DEVTYPE=%s", dev->type->name);

You broke ABI, it's no go.

...

> > > +/*
> > > + * 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.

Yes, and it can be done by calling resource_type() over each and checked
respectively. So, why do you need them to open code?

> > > +};

-- 
With Best Regards,
Andy Shevchenko