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

[Raag Jadav <raag.jadav@intel.com>]

On Tue, Apr 08, 2025 at 11:46:59AM +0300, Andy Shevchenko wrote:
> 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().

Indeed. But considering the regions are for configuration and low speed
I/O, I'm wondering if IORESOURCE_MUXED could be of any use here?

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

Sure, let me see what can be done here. Thanks for pointing it out.

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

You mean something like we originally had in v1? I thought the idea was
to not deal with that level of complexity?

Raag