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.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---

v2: Introduce a shared struct mfd_aux_device
    Introduce MFD_AUX_TYPE flag for auxiliary device opt-in

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.

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.

Open to feedback/suggestions if it's still worth pursuing here.

 drivers/mfd/mfd-core.c   | 135 +++++++++++++++++++++++++++++++++++----
 include/linux/mfd/aux.h  |  30 +++++++++
 include/linux/mfd/core.h |   3 +
 3 files changed, 157 insertions(+), 11 deletions(-)
 create mode 100644 include/linux/mfd/aux.h

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 76bd316a50af..f38b5da6637e 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -10,8 +10,11 @@
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 #include <linux/acpi.h>
+#include <linux/auxiliary_bus.h>
+#include <linux/pci.h>
 #include <linux/list.h>
 #include <linux/property.h>
+#include <linux/mfd/aux.h>
 #include <linux/mfd/core.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
@@ -29,8 +32,15 @@ struct mfd_of_node_entry {
 	struct device_node *np;
 };
 
-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" },
 };
 
 #if IS_ENABLED(CONFIG_ACPI)
@@ -136,10 +146,87 @@ static int mfd_match_of_node_to_dev(struct platform_device *pdev,
 	return 0;
 }
 
-static int mfd_add_device(struct device *parent, int id,
-			  const struct mfd_cell *cell,
-			  struct resource *mem_base,
-			  int irq_base, struct irq_domain *domain)
+static void mfd_release_auxiliary_device(struct device *dev)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct mfd_aux_device *mfd_aux = auxiliary_dev_to_mfd_aux_dev(auxdev);
+
+	kfree(mfd_aux);
+}
+
+static int mfd_add_auxiliary_device(struct device *parent, int id, const struct mfd_cell *cell,
+				    struct resource *mem_base, int irq_base,
+				    struct irq_domain *domain)
+{
+	struct mfd_aux_device *mfd_aux;
+	struct auxiliary_device *auxdev;
+	int r, ret;
+
+	mfd_aux = kzalloc(sizeof(*mfd_aux), GFP_KERNEL);
+	if (!mfd_aux)
+		return -ENOMEM;
+
+	for (r = 0; r < cell->num_resources; r++) {
+		/* Find out base to use */
+		if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
+			mfd_aux->mem.name = cell->resources[r].name;
+			mfd_aux->mem.flags = cell->resources[r].flags;
+
+			mfd_aux->mem.parent = mem_base;
+			mfd_aux->mem.start = mem_base->start + cell->resources[r].start;
+			mfd_aux->mem.end = mem_base->start + cell->resources[r].end;
+		} else if (cell->resources[r].flags & IORESOURCE_IRQ) {
+			mfd_aux->irq.name = cell->resources[r].name;
+			mfd_aux->irq.flags = cell->resources[r].flags;
+
+			if (domain) {
+				/* Unable to create mappings for IRQ ranges */
+				WARN_ON(cell->resources[r].start != cell->resources[r].end);
+				mfd_aux->irq.start = mfd_aux->irq.end = irq_create_mapping(
+						domain, cell->resources[r].start);
+			} else {
+				mfd_aux->irq.start = irq_base + cell->resources[r].start;
+				mfd_aux->irq.end = irq_base + cell->resources[r].end;
+			}
+		} else {
+			mfd_aux->ext.name = cell->resources[r].name;
+			mfd_aux->ext.flags = cell->resources[r].flags;
+			mfd_aux->ext.parent = cell->resources[r].parent;
+			mfd_aux->ext.start = cell->resources[r].start;
+			mfd_aux->ext.end = cell->resources[r].end;
+		}
+	}
+
+	auxdev = &mfd_aux->auxdev;
+	auxdev->name = cell->name;
+	/* Use parent id for discoverable devices */
+	auxdev->id = dev_is_pci(parent) ? pci_dev_id(to_pci_dev(parent)) : cell->id;
+
+	auxdev->dev.parent = parent;
+	auxdev->dev.type = &mfd_dev_type[MFD_AUX_DEV];
+	auxdev->dev.release = mfd_release_auxiliary_device;
+
+	ret = auxiliary_device_init(auxdev);
+	if (ret)
+		goto fail_aux_init;
+
+	ret = __auxiliary_device_add(auxdev, parent->driver->name);
+	if (ret)
+		goto fail_aux_add;
+
+	return 0;
+
+fail_aux_add:
+	/* auxdev will be freed with the put_device() and .release sequence */
+	auxiliary_device_uninit(auxdev);
+fail_aux_init:
+	kfree(mfd_aux);
+	return ret;
+}
+
+static int mfd_add_platform_device(struct device *parent, int id, const struct mfd_cell *cell,
+				   struct resource *mem_base, int irq_base,
+				   struct irq_domain *domain)
 {
 	struct resource *res;
 	struct platform_device *pdev;
@@ -168,7 +255,7 @@ static int mfd_add_device(struct device *parent, int id,
 		goto fail_device;
 
 	pdev->dev.parent = parent;
-	pdev->dev.type = &mfd_dev_type;
+	pdev->dev.type = &mfd_dev_type[MFD_PLAT_DEV];
 	pdev->dev.dma_mask = parent->dma_mask;
 	pdev->dev.dma_parms = parent->dma_parms;
 	pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
@@ -302,6 +389,16 @@ static int mfd_add_device(struct device *parent, int id,
 	return ret;
 }
 
+static int mfd_add_device(struct device *parent, int id, const struct mfd_cell *cells,
+			  struct resource *mem_base, int irq_base, struct irq_domain *domain)
+{
+	/* TODO: Convert the platform device abusers and remove this flag */
+	if (dev_is_pci(parent) && id == MFD_AUX_TYPE)
+		return mfd_add_auxiliary_device(parent, id, cells, mem_base, irq_base, domain);
+	else
+		return mfd_add_platform_device(parent, id, cells, mem_base, irq_base, domain);
+}
+
 /**
  * mfd_add_devices - register child devices
  *
@@ -340,16 +437,22 @@ int mfd_add_devices(struct device *parent, int id,
 }
 EXPORT_SYMBOL(mfd_add_devices);
 
-static int mfd_remove_devices_fn(struct device *dev, void *data)
+static int mfd_remove_auxiliary_device(struct device *dev)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+
+	auxiliary_device_delete(auxdev);
+	auxiliary_device_uninit(auxdev);
+	return 0;
+}
+
+static int mfd_remove_platform_device(struct device *dev, void *data)
 {
 	struct platform_device *pdev;
 	const struct mfd_cell *cell;
 	struct mfd_of_node_entry *of_entry, *tmp;
 	int *level = data;
 
-	if (dev->type != &mfd_dev_type)
-		return 0;
-
 	pdev = to_platform_device(dev);
 	cell = mfd_get_cell(pdev);
 
@@ -372,6 +475,16 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
 	return 0;
 }
 
+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])
+		return mfd_remove_platform_device(dev, data);
+
+	return 0;
+}
+
 void mfd_remove_devices_late(struct device *parent)
 {
 	int level = MFD_DEP_LEVEL_HIGH;
diff --git a/include/linux/mfd/aux.h b/include/linux/mfd/aux.h
new file mode 100644
index 000000000000..ebc385203184
--- /dev/null
+++ b/include/linux/mfd/aux.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * MFD auxiliary device
+ *
+ * Copyright (c) 2025 Raag Jadav <raag.jadav@intel.com>
+ */
+
+#ifndef MFD_AUX_H
+#define MFD_AUX_H
+
+#include <linux/auxiliary_bus.h>
+#include <linux/ioport.h>
+#include <linux/types.h>
+
+#define auxiliary_dev_to_mfd_aux_dev(auxiliary_dev) \
+	container_of(auxiliary_dev, struct mfd_aux_device, auxdev)
+
+/*
+ * 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;
+};
+
+#endif
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index faeea7abd688..ab516fc750e0 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -12,6 +12,9 @@
 
 #include <linux/platform_device.h>
 
+/* TODO: Convert the platform device abusers and remove this flag */
+#define MFD_AUX_TYPE	INT_MIN
+
 #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
 
 #define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, _of_reg, _use_of_reg, _match) \
-- 
2.34.1

Hi Raag,

kernel test robot noticed the following build errors:

[auto build test ERROR on lee-mfd/for-mfd-next]
[also build test ERROR on lee-mfd/for-mfd-fixes linus/master v6.15-rc1 next-20250407]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Raag-Jadav/mfd-core-Support-auxiliary-device/20250407-154807
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link:    https://lore.kernel.org/r/20250407074614.1665575-1-raag.jadav%40intel.com
patch subject: [PATCH v2] mfd: core: Support auxiliary device
config: arm-socfpga_defconfig (https://download.01.org/0day-ci/archive/20250407/202504071910.eL0mjen2-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250407/202504071910.eL0mjen2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504071910.eL0mjen2-lkp@intel.com/

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/mfd/mfd-core.o: in function `mfd_add_auxiliary_device':
>> drivers/mfd/mfd-core.c:209:(.text+0x360): undefined reference to `auxiliary_device_init'
>> arm-linux-gnueabi-ld: drivers/mfd/mfd-core.c:213:(.text+0x3d0): undefined reference to `__auxiliary_device_add'


vim +209 drivers/mfd/mfd-core.c

   156	
   157	static int mfd_add_auxiliary_device(struct device *parent, int id, const struct mfd_cell *cell,
   158					    struct resource *mem_base, int irq_base,
   159					    struct irq_domain *domain)
   160	{
   161		struct mfd_aux_device *mfd_aux;
   162		struct auxiliary_device *auxdev;
   163		int r, ret;
   164	
   165		mfd_aux = kzalloc(sizeof(*mfd_aux), GFP_KERNEL);
   166		if (!mfd_aux)
   167			return -ENOMEM;
   168	
   169		for (r = 0; r < cell->num_resources; r++) {
   170			/* Find out base to use */
   171			if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
   172				mfd_aux->mem.name = cell->resources[r].name;
   173				mfd_aux->mem.flags = cell->resources[r].flags;
   174	
   175				mfd_aux->mem.parent = mem_base;
   176				mfd_aux->mem.start = mem_base->start + cell->resources[r].start;
   177				mfd_aux->mem.end = mem_base->start + cell->resources[r].end;
   178			} else if (cell->resources[r].flags & IORESOURCE_IRQ) {
   179				mfd_aux->irq.name = cell->resources[r].name;
   180				mfd_aux->irq.flags = cell->resources[r].flags;
   181	
   182				if (domain) {
   183					/* Unable to create mappings for IRQ ranges */
   184					WARN_ON(cell->resources[r].start != cell->resources[r].end);
   185					mfd_aux->irq.start = mfd_aux->irq.end = irq_create_mapping(
   186							domain, cell->resources[r].start);
   187				} else {
   188					mfd_aux->irq.start = irq_base + cell->resources[r].start;
   189					mfd_aux->irq.end = irq_base + cell->resources[r].end;
   190				}
   191			} else {
   192				mfd_aux->ext.name = cell->resources[r].name;
   193				mfd_aux->ext.flags = cell->resources[r].flags;
   194				mfd_aux->ext.parent = cell->resources[r].parent;
   195				mfd_aux->ext.start = cell->resources[r].start;
   196				mfd_aux->ext.end = cell->resources[r].end;
   197			}
   198		}
   199	
   200		auxdev = &mfd_aux->auxdev;
   201		auxdev->name = cell->name;
   202		/* Use parent id for discoverable devices */
   203		auxdev->id = dev_is_pci(parent) ? pci_dev_id(to_pci_dev(parent)) : cell->id;
   204	
   205		auxdev->dev.parent = parent;
   206		auxdev->dev.type = &mfd_dev_type[MFD_AUX_DEV];
   207		auxdev->dev.release = mfd_release_auxiliary_device;
   208	
 > 209		ret = auxiliary_device_init(auxdev);
   210		if (ret)
   211			goto fail_aux_init;
   212	
 > 213		ret = __auxiliary_device_add(auxdev, parent->driver->name);
   214		if (ret)
   215			goto fail_aux_add;
   216	
   217		return 0;
   218	
   219	fail_aux_add:
   220		/* auxdev will be freed with the put_device() and .release sequence */
   221		auxiliary_device_uninit(auxdev);
   222	fail_aux_init:
   223		kfree(mfd_aux);
   224		return ret;
   225	}
   226	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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.

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

...

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

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

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

> +};
> +
> +#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.

-- 
With Best Regards,
Andy Shevchenko

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

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

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

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

And yes, I still consider enforcing regmap is the right step to go.

-- 
With Best Regards,
Andy Shevchenko

On Mon, Apr 07, 2025 at 11:45:30AM +0300, Andy Shevchenko 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:
> > > 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.
> > 
> > > 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.
> 
> And yes, I still consider enforcing regmap is the right step to go.

Unless there's an explicit need for it, I think we should leave that
choice to the individial drivers instead of forcing them to revamp.
But let's see what Lee and Greg have to say about this.

Raag

On Tue, Apr 08, 2025 at 11:04:59AM +0300, Raag Jadav wrote:
> On Mon, Apr 07, 2025 at 11:45:30AM +0300, Andy Shevchenko 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:
> > > > 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.
> > > 
> > > > 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.
> > 
> > And yes, I still consider enforcing regmap is the right step to go.
> 
> Unless there's an explicit need for it, I think we should leave that
> choice to the individial drivers instead of forcing them to revamp.
> But let's see what Lee and Greg have to say about this.

Doing that is call to inherit all issues with shared resources and locking
as I already said.

-- 
With Best Regards,
Andy Shevchenko