[PATCH v5 0/2] Auxiliary device support for MFD

[PATCH v5 0/2] Auxiliary device support for MFD

[Raag Jadav]

This series adds auxiliary child device support for discoverable MFD and
as a prerequisite, introduces auxiliary device specific resource APIs to
to allow independent resource management.

v2: Introduce a shared struct mfd_aux_device
    Introduce auxiliary device opt-in flag (Greg)

v3: Fix device_type ABI breakage (Andy)
    Aesthetic adjustments (Andy)

v4: s/mfd_aux/maux
    Allow num_resources for child device (Andy)
    Fix build warning (Andy)

v5: Move resource() helpers from MFD to auxiliary subsystem (Lee)

Raag Jadav (2):
  driver core: auxiliary bus: Introduce auxiliary device resource
    management
  mfd: core: Support auxiliary device

 drivers/base/auxiliary.c      | 145 ++++++++++++++++++++++++++
 drivers/mfd/Kconfig           |   2 +-
 drivers/mfd/mfd-core.c        | 185 +++++++++++++++++++++++++---------
 include/linux/auxiliary_bus.h |  18 ++++
 include/linux/mfd/core.h      |   3 +
 5 files changed, 305 insertions(+), 48 deletions(-)

-- 
2.34.1

[PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[Raag Jadav]

With more and more drivers adopting to auxiliary bus infrastructure comes
the need for managing resources at auxiliary device level. This is useful
for cases where parent device shares variable number and type of resources
with auxiliary child device but doesn't require any active involvement in
managing them.

This reduces potential duplication of resource APIs that may be required by
parent device driver. With this in place parent driver will be responsible
for filling up respective resources and its count in auxiliary device
structure before registering it, so that the leaf drivers can utilize in
their probe function. Lifecycle of these resources will be as long as the
auxiliary device exists.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/base/auxiliary.c      | 145 ++++++++++++++++++++++++++++++++++
 include/linux/auxiliary_bus.h |  18 +++++
 2 files changed, 163 insertions(+)

diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
index 95717d509ca9..86ae51ef50ff 100644
--- a/drivers/base/auxiliary.c
+++ b/drivers/base/auxiliary.c
@@ -9,6 +9,8 @@
 
 #include <linux/device.h>
 #include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/irq.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/pm_domain.h>
@@ -384,6 +386,149 @@ int __auxiliary_driver_register(struct auxiliary_driver *auxdrv,
 }
 EXPORT_SYMBOL_GPL(__auxiliary_driver_register);
 
+/**
+ * auxiliary_get_resource - get a resource for auxiliary device
+ * @auxdev: auxiliary device
+ * @type: resource type
+ * @num: resource index
+ *
+ * Return: a pointer to the resource or NULL on failure.
+ */
+struct resource *auxiliary_get_resource(struct auxiliary_device *auxdev, unsigned int type,
+					unsigned int num)
+{
+	u32 i;
+
+	for (i = 0; i < auxdev->num_resources; i++) {
+		struct resource *r = &auxdev->resource[i];
+
+		if (type == resource_type(r) && num-- == 0)
+			return r;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(auxiliary_get_resource);
+
+#ifdef CONFIG_HAS_IOMEM
+/**
+ * devm_auxiliary_get_and_ioremap_resource - get resource and call devm_ioremap_resource()
+ *					     for auxiliary device
+ *
+ * @auxdev: auxiliary device to use both for memory resource lookup as well as
+ *	    resource management
+ * @index: resource index
+ * @res: optional output parameter to store a pointer to the obtained resource.
+ *
+ * Return: a pointer to the remapped memory or an ERR_PTR() encoded error code
+ * on failure.
+ */
+void __iomem *devm_auxiliary_get_and_ioremap_resource(struct auxiliary_device *auxdev,
+						      unsigned int index, struct resource **res)
+{
+	struct resource *r;
+
+	r = auxiliary_get_resource(auxdev, IORESOURCE_MEM, index);
+	if (res)
+		*res = r;
+	return devm_ioremap_resource(&auxdev->dev, r);
+}
+EXPORT_SYMBOL_GPL(devm_auxiliary_get_and_ioremap_resource);
+
+/**
+ * devm_auxiliary_ioremap_resource - call devm_ioremap_resource() for auxiliary device
+ *
+ * @auxdev: auxiliary device to use both for memory resource lookup as well as
+ *	    resource management
+ * @index: resource index
+ *
+ * Return: a pointer to the remapped memory or an ERR_PTR() encoded error code
+ * on failure.
+ */
+void __iomem *devm_auxiliary_ioremap_resource(struct auxiliary_device *auxdev, unsigned int index)
+{
+	return devm_auxiliary_get_and_ioremap_resource(auxdev, index, NULL);
+}
+EXPORT_SYMBOL_GPL(devm_auxiliary_ioremap_resource);
+#endif
+
+/**
+ * auxiliary_get_irq_optional - get an optional IRQ for auxiliary device
+ * @auxdev: auxiliary device
+ * @num: IRQ number index
+ *
+ * Gets an IRQ for a auxiliary device. Device drivers should check the return value
+ * for errors so as to not pass a negative integer value to the request_irq()
+ * APIs. This is the same as auxiliary_get_irq(), except that it does not print an
+ * error message if an IRQ can not be obtained.
+ *
+ * For example::
+ *
+ *		int irq = auxiliary_get_irq_optional(auxdev, 0);
+ *		if (irq < 0)
+ *			return irq;
+ *
+ * Return: non-zero IRQ number on success, negative error number on failure.
+ */
+int auxiliary_get_irq_optional(struct auxiliary_device *auxdev, unsigned int num)
+{
+	struct resource *r;
+	int ret = -ENXIO;
+
+	r = auxiliary_get_resource(auxdev, IORESOURCE_IRQ, num);
+	if (!r)
+		goto out;
+
+	/*
+	 * The resources may pass trigger flags to the irqs that need to be
+	 * set up. It so happens that the trigger flags for IORESOURCE_BITS
+	 * correspond 1-to-1 to the IRQF_TRIGGER* settings.
+	 */
+	if (r->flags & IORESOURCE_BITS) {
+		struct irq_data *irqd;
+
+		irqd = irq_get_irq_data(r->start);
+		if (!irqd)
+			goto out;
+		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
+	}
+
+	ret = r->start;
+	if (WARN(!ret, "0 is an invalid IRQ number\n"))
+		ret = -EINVAL;
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(auxiliary_get_irq_optional);
+
+/**
+ * auxiliary_get_irq - get an IRQ for auxiliary device
+ * @auxdev: auxiliary device
+ * @num: IRQ number index
+ *
+ * Gets an IRQ for a auxiliary device and prints an error message if finding the IRQ
+ * fails. Device drivers should check the return value for errors so as to not pass
+ * a negative integer value to the request_irq() APIs.
+ *
+ * For example::
+ *
+ *		int irq = auxiliary_get_irq(auxdev, 0);
+ *		if (irq < 0)
+ *			return irq;
+ *
+ * Return: non-zero IRQ number on success, negative error number on failure.
+ */
+int auxiliary_get_irq(struct auxiliary_device *auxdev, unsigned int num)
+{
+	int ret;
+
+	ret = auxiliary_get_irq_optional(auxdev, num);
+	if (ret < 0)
+		return dev_err_probe(&auxdev->dev, ret, "IRQ index %u not found\n", num);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(auxiliary_get_irq);
+
 /**
  * auxiliary_driver_unregister - unregister a driver
  * @auxdrv: auxiliary_driver structure
diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
index 65dd7f154374..7d7e23313b63 100644
--- a/include/linux/auxiliary_bus.h
+++ b/include/linux/auxiliary_bus.h
@@ -11,6 +11,8 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 
+struct resource;
+
 /**
  * DOC: DEVICE_LIFESPAN
  *
@@ -148,6 +150,8 @@ struct auxiliary_device {
 		struct mutex lock; /* Synchronize irq sysfs creation */
 		bool irq_dir_exists;
 	} sysfs;
+	u32 num_resources;
+	struct resource	*resource;
 };
 
 /**
@@ -238,6 +242,9 @@ auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq) {}
 
 static inline void auxiliary_device_uninit(struct auxiliary_device *auxdev)
 {
+	if (auxdev->resource)
+		kfree(auxdev->resource);
+
 	mutex_destroy(&auxdev->sysfs.lock);
 	put_device(&auxdev->dev);
 }
@@ -269,4 +276,15 @@ void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv);
 #define module_auxiliary_driver(__auxiliary_driver) \
 	module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
 
+struct resource *auxiliary_get_resource(struct auxiliary_device *auxdev, unsigned int type,
+					unsigned int num);
+int auxiliary_get_irq_optional(struct auxiliary_device *auxdev, unsigned int num);
+int auxiliary_get_irq(struct auxiliary_device *auxdev, unsigned int num);
+
+#ifdef CONFIG_HAS_IOMEM
+void __iomem *devm_auxiliary_get_and_ioremap_resource(struct auxiliary_device *auxdev,
+						      unsigned int index, struct resource **res);
+void __iomem *devm_auxiliary_ioremap_resource(struct auxiliary_device *auxdev, unsigned int index);
+#endif
+
 #endif /* _AUXILIARY_BUS_H_ */
-- 
2.34.1

[PATCH v5 2/2] mfd: core: Support auxiliary device

[Raag Jadav]

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 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>
---
 drivers/mfd/Kconfig      |   2 +-
 drivers/mfd/mfd-core.c   | 185 +++++++++++++++++++++++++++++----------
 include/linux/mfd/core.h |   3 +
 3 files changed, 142 insertions(+), 48 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 93773201a517..4c71a3f962c9 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -8,8 +8,8 @@ menu "Multifunction device drivers"
 
 config MFD_CORE
 	tristate
+	select AUXILIARY_BUS
 	select IRQ_DOMAIN
-	default n
 
 config MFD_CS5535
 	tristate "AMD CS5535 and CS5536 southbridge core functions"
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 76bd316a50af..174925bb7bf6 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -10,9 +10,11 @@
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 #include <linux/acpi.h>
+#include <linux/auxiliary_bus.h>
 #include <linux/list.h>
 #include <linux/property.h>
 #include <linux/mfd/core.h>
+#include <linux/pci.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/module.h>
@@ -136,10 +138,108 @@ 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 int mfd_fill_device_resources(struct device *dev, const struct mfd_cell *cell,
+				     struct resource *mem_base, int irq_base,
+				     struct irq_domain *domain, struct resource *res)
+{
+	int r, ret;
+
+	for (r = 0; r < cell->num_resources; r++) {
+		res[r].name  = cell->resources[r].name;
+		res[r].flags = cell->resources[r].flags;
+
+		/* Find out base to use */
+		if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
+			res[r].parent = mem_base;
+			res[r].start  = mem_base->start + cell->resources[r].start;
+			res[r].end    = mem_base->start + cell->resources[r].end;
+		} else if (cell->resources[r].flags & IORESOURCE_IRQ) {
+			if (domain) {
+				/* Unable to create mappings for IRQ ranges. */
+				WARN_ON(cell->resources[r].start != cell->resources[r].end);
+				res[r].start = res[r].end = irq_create_mapping(domain,
+						cell->resources[r].start);
+			} else {
+				res[r].start = irq_base + cell->resources[r].start;
+				res[r].end   = irq_base + cell->resources[r].end;
+			}
+		} else {
+			res[r].parent = cell->resources[r].parent;
+			res[r].start  = cell->resources[r].start;
+			res[r].end    = cell->resources[r].end;
+		}
+
+		if (!cell->ignore_resource_conflicts) {
+			if (has_acpi_companion(dev)) {
+				ret = acpi_check_resource_conflict(&res[r]);
+				if (ret)
+					return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static void mfd_release_auxiliary_device(struct device *dev)
+{
+	kfree(to_auxiliary_dev(dev));
+}
+
+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 auxiliary_device *auxdev;
+	struct resource *res;
+	int ret = -ENOMEM;
+
+	auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL);
+	if (!auxdev)
+		return ret;
+
+	res = kcalloc(cell->num_resources, sizeof(*res), GFP_KERNEL);
+	if (!res)
+		goto fail_alloc_res;
+
+	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;
+	auxdev->dev.release = mfd_release_auxiliary_device;
+
+	ret = auxiliary_device_init(auxdev);
+	if (ret)
+		goto fail_aux_init;
+
+	ret = mfd_fill_device_resources(&auxdev->dev, cell, mem_base, irq_base, domain, res);
+	if (ret)
+		goto fail_aux_add;
+
+	auxdev->resource = res;
+	auxdev->num_resources = cell->num_resources;
+
+	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(res);
+fail_alloc_res:
+	kfree(auxdev);
+	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;
@@ -148,7 +248,6 @@ static int mfd_add_device(struct device *parent, int id,
 	bool disabled = false;
 	int ret = -ENOMEM;
 	int platform_id;
-	int r;
 
 	if (id == PLATFORM_DEVID_AUTO)
 		platform_id = id;
@@ -227,44 +326,9 @@ static int mfd_add_device(struct device *parent, int id,
 			goto fail_of_entry;
 	}
 
-	for (r = 0; r < cell->num_resources; r++) {
-		res[r].name = cell->resources[r].name;
-		res[r].flags = cell->resources[r].flags;
-
-		/* Find out base to use */
-		if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
-			res[r].parent = mem_base;
-			res[r].start = mem_base->start +
-				cell->resources[r].start;
-			res[r].end = mem_base->start +
-				cell->resources[r].end;
-		} else if (cell->resources[r].flags & IORESOURCE_IRQ) {
-			if (domain) {
-				/* Unable to create mappings for IRQ ranges. */
-				WARN_ON(cell->resources[r].start !=
-					cell->resources[r].end);
-				res[r].start = res[r].end = irq_create_mapping(
-					domain, cell->resources[r].start);
-			} else {
-				res[r].start = irq_base +
-					cell->resources[r].start;
-				res[r].end   = irq_base +
-					cell->resources[r].end;
-			}
-		} else {
-			res[r].parent = cell->resources[r].parent;
-			res[r].start = cell->resources[r].start;
-			res[r].end   = cell->resources[r].end;
-		}
-
-		if (!cell->ignore_resource_conflicts) {
-			if (has_acpi_companion(&pdev->dev)) {
-				ret = acpi_check_resource_conflict(&res[r]);
-				if (ret)
-					goto fail_res_conflict;
-			}
-		}
-	}
+	ret = mfd_fill_device_resources(&pdev->dev, cell, mem_base, irq_base, domain, res);
+	if (ret)
+		goto fail_res_conflict;
 
 	ret = platform_device_add_resources(pdev, res, cell->num_resources);
 	if (ret)
@@ -302,6 +366,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 platform device abusers and remove this flag */
+	if (dev_is_pci(parent) && id == MAUX_TYPE)
+		return mfd_add_auxiliary_device(parent, id, cells, mem_base, irq_base, domain);
+
+	return mfd_add_platform_device(parent, id, cells, mem_base, irq_base, domain);
+}
+
 /**
  * mfd_add_devices - register child devices
  *
@@ -340,16 +414,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, void *data)
+{
+	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 +452,17 @@ 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)
+		return 0;
+
+	if (dev_is_platform(dev))
+		return mfd_remove_platform_device(dev, data);
+
+	return mfd_remove_auxiliary_device(dev, data);
+}
+
 void mfd_remove_devices_late(struct device *parent)
 {
 	int level = MFD_DEP_LEVEL_HIGH;
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index faeea7abd688..85ca273b3873 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -12,6 +12,9 @@
 
 #include <linux/platform_device.h>
 
+/* TODO: Convert platform device abusers and remove this flag */
+#define MAUX_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

Re: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[Greg KH]

On Wed, May 14, 2025 at 05:54:31PM +0530, Raag Jadav wrote:
> With more and more drivers adopting to auxiliary bus infrastructure comes
> the need for managing resources at auxiliary device level. This is useful
> for cases where parent device shares variable number and type of resources
> with auxiliary child device but doesn't require any active involvement in
> managing them.
> 
> This reduces potential duplication of resource APIs that may be required by
> parent device driver. With this in place parent driver will be responsible
> for filling up respective resources and its count in auxiliary device
> structure before registering it, so that the leaf drivers can utilize in
> their probe function. Lifecycle of these resources will be as long as the
> auxiliary device exists.
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/base/auxiliary.c      | 145 ++++++++++++++++++++++++++++++++++
>  include/linux/auxiliary_bus.h |  18 +++++
>  2 files changed, 163 insertions(+)

Sorry, but again, you are not following the required rules for Intel
kernel submissions for this subsystem.  Please work with the Intel
kernel team to fix this up before you resend.

greg k-h

Re: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[Andy Shevchenko]

On Wed, May 14, 2025 at 05:54:31PM +0530, Raag Jadav wrote:
> With more and more drivers adopting to auxiliary bus infrastructure comes
> the need for managing resources at auxiliary device level. This is useful
> for cases where parent device shares variable number and type of resources
> with auxiliary child device but doesn't require any active involvement in
> managing them.
> 
> This reduces potential duplication of resource APIs that may be required by
> parent device driver. With this in place parent driver will be responsible
> for filling up respective resources and its count in auxiliary device
> structure before registering it, so that the leaf drivers can utilize in
> their probe function. Lifecycle of these resources will be as long as the
> auxiliary device exists.

...

> +/**
> + * auxiliary_get_irq_optional - get an optional IRQ for auxiliary device
> + * @auxdev: auxiliary device
> + * @num: IRQ number index
> + *
> + * Gets an IRQ for a auxiliary device. Device drivers should check the return value
> + * for errors so as to not pass a negative integer value to the request_irq()
> + * APIs. This is the same as auxiliary_get_irq(), except that it does not print an
> + * error message if an IRQ can not be obtained.
> + *
> + * For example::
> + *
> + *		int irq = auxiliary_get_irq_optional(auxdev, 0);
> + *		if (irq < 0)
> + *			return irq;
> + *
> + * Return: non-zero IRQ number on success, negative error number on failure.
> + */
> +int auxiliary_get_irq_optional(struct auxiliary_device *auxdev, unsigned int num)
> +{
> +	struct resource *r;
> +	int ret = -ENXIO;
> +
> +	r = auxiliary_get_resource(auxdev, IORESOURCE_IRQ, num);
> +	if (!r)
> +		goto out;
> +
> +	/*
> +	 * The resources may pass trigger flags to the irqs that need to be
> +	 * set up. It so happens that the trigger flags for IORESOURCE_BITS
> +	 * correspond 1-to-1 to the IRQF_TRIGGER* settings.
> +	 */
> +	if (r->flags & IORESOURCE_BITS) {
> +		struct irq_data *irqd;
> +
> +		irqd = irq_get_irq_data(r->start);
> +		if (!irqd)
> +			goto out;
> +		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> +	}
> +
> +	ret = r->start;
> +	if (WARN(!ret, "0 is an invalid IRQ number\n"))
> +		ret = -EINVAL;
> +out:
> +	return ret;
> +}

Please, do not inherit the issues that the respective platform device API has.
And after all, why do you need this? What's wrong with plain fwnode_irq_get()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[kernel test robot]

Hi Raag,

kernel test robot noticed the following build warnings:

[auto build test WARNING on lee-mfd/for-mfd-next]
[also build test WARNING on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus staging/staging-testing staging/staging-next staging/staging-linus linus/master v6.15-rc6 next-20250514]
[cannot apply to lee-mfd/for-mfd-fixes]
[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/driver-core-auxiliary-bus-Introduce-auxiliary-device-resource-management/20250514-202748
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link:    https://lore.kernel.org/r/20250514122432.4019606-2-raag.jadav%40intel.com
patch subject: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management
config: arm-randconfig-003-20250514 (https://download.01.org/0day-ci/archive/20250514/202505142347.7wqzaBvR-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250514/202505142347.7wqzaBvR-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/202505142347.7wqzaBvR-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/bridge/ti-sn65dsi86.c:8:0:
   include/linux/auxiliary_bus.h: In function 'auxiliary_device_uninit':
   include/linux/auxiliary_bus.h:246:3: error: implicit declaration of function 'kfree'; did you mean '__free'? [-Werror=implicit-function-declaration]
      kfree(auxdev->resource);
      ^~~~~
      __free
   In file included from arch/arm/include/asm/pgtable-nommu.h:13:0,
                    from arch/arm/include/asm/pgtable.h:25,
                    from arch/arm/include/asm/uaccess.h:17,
                    from include/linux/uaccess.h:12,
                    from include/linux/sched/task.h:13,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:34,
                    from include/linux/debugfs.h:15,
                    from drivers/gpu/drm/bridge/ti-sn65dsi86.c:12:
   include/linux/slab.h: At top level:
>> include/linux/slab.h:472:6: warning: conflicting types for 'kfree'
    void kfree(const void *objp);
         ^~~~~
   In file included from drivers/gpu/drm/bridge/ti-sn65dsi86.c:8:0:
   include/linux/auxiliary_bus.h:246:3: note: previous implicit declaration of 'kfree' was here
      kfree(auxdev->resource);
      ^~~~~
   cc1: some warnings being treated as errors


vim +/kfree +472 include/linux/slab.h

7bd230a26648ac Suren Baghdasaryan 2024-03-21  471  
72d67229f522e3 Kees Cook          2021-11-05 @472  void kfree(const void *objp);
72d67229f522e3 Kees Cook          2021-11-05  473  void kfree_sensitive(const void *objp);
72d67229f522e3 Kees Cook          2021-11-05  474  size_t __ksize(const void *objp);
05a940656e1eb2 Kees Cook          2022-09-23  475  

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

Re: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[Raag Jadav]

On Wed, May 14, 2025 at 02:35:01PM +0200, Greg KH wrote:
> On Wed, May 14, 2025 at 05:54:31PM +0530, Raag Jadav wrote:
> > With more and more drivers adopting to auxiliary bus infrastructure comes
> > the need for managing resources at auxiliary device level. This is useful
> > for cases where parent device shares variable number and type of resources
> > with auxiliary child device but doesn't require any active involvement in
> > managing them.
> > 
> > This reduces potential duplication of resource APIs that may be required by
> > parent device driver. With this in place parent driver will be responsible
> > for filling up respective resources and its count in auxiliary device
> > structure before registering it, so that the leaf drivers can utilize in
> > their probe function. Lifecycle of these resources will be as long as the
> > auxiliary device exists.
> > 
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> >  drivers/base/auxiliary.c      | 145 ++++++++++++++++++++++++++++++++++
> >  include/linux/auxiliary_bus.h |  18 +++++
> >  2 files changed, 163 insertions(+)
> 
> Sorry, but again, you are not following the required rules for Intel
> kernel submissions for this subsystem.  Please work with the Intel
> kernel team to fix this up before you resend.

Apologies again if I have broken any rules here. Since this is following
the recommendations[1][2] of community maintainers, my understanding is
that this needed their attention.

[1] https://lore.kernel.org/r/2025032514-ipad-schilling-9928@gregkh
[2] https://lore.kernel.org/r/20250501125028.GM1567507@google.com

Thanks for guidance, let me fix this.

Raag

Re: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[kernel test robot]

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 driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus staging/staging-testing staging/staging-next staging/staging-linus linus/master v6.15-rc6 next-20250514]
[cannot apply to lee-mfd/for-mfd-fixes]
[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/driver-core-auxiliary-bus-Introduce-auxiliary-device-resource-management/20250514-202748
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link:    https://lore.kernel.org/r/20250514122432.4019606-2-raag.jadav%40intel.com
patch subject: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management
config: arm-randconfig-002-20250514 (https://download.01.org/0day-ci/archive/20250514/202505142345.jLaHG90C-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project f819f46284f2a79790038e1f6649172789734ae8)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250514/202505142345.jLaHG90C-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/202505142345.jLaHG90C-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/base/auxiliary_sysfs.c:6:
>> include/linux/auxiliary_bus.h:246:3: error: call to undeclared function 'kfree'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     246 |                 kfree(auxdev->resource);
         |                 ^
   In file included from drivers/base/auxiliary_sysfs.c:7:
>> include/linux/slab.h:472:6: error: conflicting types for 'kfree'
     472 | void kfree(const void *objp);
         |      ^
   include/linux/auxiliary_bus.h:246:3: note: previous implicit declaration is here
     246 |                 kfree(auxdev->resource);
         |                 ^
   2 errors generated.


vim +/kfree +246 include/linux/auxiliary_bus.h

   242	
   243	static inline void auxiliary_device_uninit(struct auxiliary_device *auxdev)
   244	{
   245		if (auxdev->resource)
 > 246			kfree(auxdev->resource);
   247	
   248		mutex_destroy(&auxdev->sysfs.lock);
   249		put_device(&auxdev->dev);
   250	}
   251	

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

Re: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[Raag Jadav]

On Wed, May 14, 2025 at 03:36:49PM +0300, Andy Shevchenko wrote:
> On Wed, May 14, 2025 at 05:54:31PM +0530, Raag Jadav wrote:
> > With more and more drivers adopting to auxiliary bus infrastructure comes
> > the need for managing resources at auxiliary device level. This is useful
> > for cases where parent device shares variable number and type of resources
> > with auxiliary child device but doesn't require any active involvement in
> > managing them.
> > 
> > This reduces potential duplication of resource APIs that may be required by
> > parent device driver. With this in place parent driver will be responsible
> > for filling up respective resources and its count in auxiliary device
> > structure before registering it, so that the leaf drivers can utilize in
> > their probe function. Lifecycle of these resources will be as long as the
> > auxiliary device exists.
> 
> ...
> 
> > +/**
> > + * auxiliary_get_irq_optional - get an optional IRQ for auxiliary device
> > + * @auxdev: auxiliary device
> > + * @num: IRQ number index
> > + *
> > + * Gets an IRQ for a auxiliary device. Device drivers should check the return value
> > + * for errors so as to not pass a negative integer value to the request_irq()
> > + * APIs. This is the same as auxiliary_get_irq(), except that it does not print an
> > + * error message if an IRQ can not be obtained.
> > + *
> > + * For example::
> > + *
> > + *		int irq = auxiliary_get_irq_optional(auxdev, 0);
> > + *		if (irq < 0)
> > + *			return irq;
> > + *
> > + * Return: non-zero IRQ number on success, negative error number on failure.
> > + */
> > +int auxiliary_get_irq_optional(struct auxiliary_device *auxdev, unsigned int num)
> > +{
> > +	struct resource *r;
> > +	int ret = -ENXIO;
> > +
> > +	r = auxiliary_get_resource(auxdev, IORESOURCE_IRQ, num);
> > +	if (!r)
> > +		goto out;
> > +
> > +	/*
> > +	 * The resources may pass trigger flags to the irqs that need to be
> > +	 * set up. It so happens that the trigger flags for IORESOURCE_BITS
> > +	 * correspond 1-to-1 to the IRQF_TRIGGER* settings.
> > +	 */
> > +	if (r->flags & IORESOURCE_BITS) {
> > +		struct irq_data *irqd;
> > +
> > +		irqd = irq_get_irq_data(r->start);
> > +		if (!irqd)
> > +			goto out;
> > +		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> > +	}
> > +
> > +	ret = r->start;
> > +	if (WARN(!ret, "0 is an invalid IRQ number\n"))
> > +		ret = -EINVAL;
> > +out:
> > +	return ret;
> > +}
> 
> Please, do not inherit the issues that the respective platform device API has.
> And after all, why do you need this? What's wrong with plain fwnode_irq_get()?

Can you please elaborate? Are we expecting fwnode to be supported by auxiliary
device?

Raag

Re: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[Andy Shevchenko]

On Thu, May 15, 2025 at 03:52:38PM +0300, Raag Jadav wrote:
> On Wed, May 14, 2025 at 03:36:49PM +0300, Andy Shevchenko wrote:
> > On Wed, May 14, 2025 at 05:54:31PM +0530, Raag Jadav wrote:

...

> > > +/**
> > > + * auxiliary_get_irq_optional - get an optional IRQ for auxiliary device
> > > + * @auxdev: auxiliary device
> > > + * @num: IRQ number index
> > > + *
> > > + * Gets an IRQ for a auxiliary device. Device drivers should check the return value
> > > + * for errors so as to not pass a negative integer value to the request_irq()
> > > + * APIs. This is the same as auxiliary_get_irq(), except that it does not print an
> > > + * error message if an IRQ can not be obtained.
> > > + *
> > > + * For example::
> > > + *
> > > + *		int irq = auxiliary_get_irq_optional(auxdev, 0);
> > > + *		if (irq < 0)
> > > + *			return irq;
> > > + *
> > > + * Return: non-zero IRQ number on success, negative error number on failure.
> > > + */
> > > +int auxiliary_get_irq_optional(struct auxiliary_device *auxdev, unsigned int num)
> > > +{
> > > +	struct resource *r;
> > > +	int ret = -ENXIO;
> > > +
> > > +	r = auxiliary_get_resource(auxdev, IORESOURCE_IRQ, num);
> > > +	if (!r)
> > > +		goto out;
> > > +
> > > +	/*
> > > +	 * The resources may pass trigger flags to the irqs that need to be
> > > +	 * set up. It so happens that the trigger flags for IORESOURCE_BITS
> > > +	 * correspond 1-to-1 to the IRQF_TRIGGER* settings.
> > > +	 */
> > > +	if (r->flags & IORESOURCE_BITS) {
> > > +		struct irq_data *irqd;
> > > +
> > > +		irqd = irq_get_irq_data(r->start);
> > > +		if (!irqd)
> > > +			goto out;
> > > +		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> > > +	}
> > > +
> > > +	ret = r->start;
> > > +	if (WARN(!ret, "0 is an invalid IRQ number\n"))
> > > +		ret = -EINVAL;
> > > +out:
> > > +	return ret;
> > > +}
> > 
> > Please, do not inherit the issues that the respective platform device API has.
> > And after all, why do you need this? What's wrong with plain fwnode_irq_get()?
> 
> Can you please elaborate? Are we expecting fwnode to be supported by auxiliary
> device?

Platform IRQ getter is legacy for the board files, but it has support for fwnode.
Why do you need to inherit all that legacy? What's the point?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[Raag Jadav]

On Thu, May 15, 2025 at 04:06:47PM +0300, Andy Shevchenko wrote:
> On Thu, May 15, 2025 at 03:52:38PM +0300, Raag Jadav wrote:
> > On Wed, May 14, 2025 at 03:36:49PM +0300, Andy Shevchenko wrote:
> > > On Wed, May 14, 2025 at 05:54:31PM +0530, Raag Jadav wrote:
> 
> ...
> 
> > > > +/**
> > > > + * auxiliary_get_irq_optional - get an optional IRQ for auxiliary device
> > > > + * @auxdev: auxiliary device
> > > > + * @num: IRQ number index
> > > > + *
> > > > + * Gets an IRQ for a auxiliary device. Device drivers should check the return value
> > > > + * for errors so as to not pass a negative integer value to the request_irq()
> > > > + * APIs. This is the same as auxiliary_get_irq(), except that it does not print an
> > > > + * error message if an IRQ can not be obtained.
> > > > + *
> > > > + * For example::
> > > > + *
> > > > + *		int irq = auxiliary_get_irq_optional(auxdev, 0);
> > > > + *		if (irq < 0)
> > > > + *			return irq;
> > > > + *
> > > > + * Return: non-zero IRQ number on success, negative error number on failure.
> > > > + */
> > > > +int auxiliary_get_irq_optional(struct auxiliary_device *auxdev, unsigned int num)
> > > > +{
> > > > +	struct resource *r;
> > > > +	int ret = -ENXIO;
> > > > +
> > > > +	r = auxiliary_get_resource(auxdev, IORESOURCE_IRQ, num);
> > > > +	if (!r)
> > > > +		goto out;
> > > > +
> > > > +	/*
> > > > +	 * The resources may pass trigger flags to the irqs that need to be
> > > > +	 * set up. It so happens that the trigger flags for IORESOURCE_BITS
> > > > +	 * correspond 1-to-1 to the IRQF_TRIGGER* settings.
> > > > +	 */
> > > > +	if (r->flags & IORESOURCE_BITS) {
> > > > +		struct irq_data *irqd;
> > > > +
> > > > +		irqd = irq_get_irq_data(r->start);
> > > > +		if (!irqd)
> > > > +			goto out;
> > > > +		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> > > > +	}
> > > > +
> > > > +	ret = r->start;
> > > > +	if (WARN(!ret, "0 is an invalid IRQ number\n"))
> > > > +		ret = -EINVAL;
> > > > +out:
> > > > +	return ret;
> > > > +}
> > > 
> > > Please, do not inherit the issues that the respective platform device API has.
> > > And after all, why do you need this? What's wrong with plain fwnode_irq_get()?
> > 
> > Can you please elaborate? Are we expecting fwnode to be supported by auxiliary
> > device?
> 
> Platform IRQ getter is legacy for the board files, but it has support for fwnode.
> Why do you need to inherit all that legacy? What's the point?

This is just to abstract get_resource(IRQ) which has been carved up by the
parent device. And since this is an auxiliary child device, I'm not sure if
we have a firmware to work with.

Please correct me if I've misunderstood your question.

Raag

Re: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[Andy Shevchenko]

On Fri, May 16, 2025 at 10:20:02PM +0300, Raag Jadav wrote:
> On Thu, May 15, 2025 at 04:06:47PM +0300, Andy Shevchenko wrote:
> > On Thu, May 15, 2025 at 03:52:38PM +0300, Raag Jadav wrote:
> > > On Wed, May 14, 2025 at 03:36:49PM +0300, Andy Shevchenko wrote:
> > > > On Wed, May 14, 2025 at 05:54:31PM +0530, Raag Jadav wrote:

...

> > > > > +int auxiliary_get_irq_optional(struct auxiliary_device *auxdev, unsigned int num)
> > > > > +{
> > > > > +	struct resource *r;
> > > > > +	int ret = -ENXIO;
> > > > > +
> > > > > +	r = auxiliary_get_resource(auxdev, IORESOURCE_IRQ, num);
> > > > > +	if (!r)
> > > > > +		goto out;
> > > > > +
> > > > > +	/*
> > > > > +	 * The resources may pass trigger flags to the irqs that need to be
> > > > > +	 * set up. It so happens that the trigger flags for IORESOURCE_BITS
> > > > > +	 * correspond 1-to-1 to the IRQF_TRIGGER* settings.
> > > > > +	 */
> > > > > +	if (r->flags & IORESOURCE_BITS) {
> > > > > +		struct irq_data *irqd;
> > > > > +
> > > > > +		irqd = irq_get_irq_data(r->start);
> > > > > +		if (!irqd)
> > > > > +			goto out;
> > > > > +		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> > > > > +	}
> > > > > +
> > > > > +	ret = r->start;
> > > > > +	if (WARN(!ret, "0 is an invalid IRQ number\n"))
> > > > > +		ret = -EINVAL;
> > > > > +out:
> > > > > +	return ret;
> > > > > +}
> > > > 
> > > > Please, do not inherit the issues that the respective platform device API has.
> > > > And after all, why do you need this? What's wrong with plain fwnode_irq_get()?
> > > 
> > > Can you please elaborate? Are we expecting fwnode to be supported by auxiliary
> > > device?
> > 
> > Platform IRQ getter is legacy for the board files, but it has support for fwnode.
> > Why do you need to inherit all that legacy? What's the point?
> 
> This is just to abstract get_resource(IRQ) which has been carved up by the
> parent device. And since this is an auxiliary child device, I'm not sure if
> we have a firmware to work with.

To make get_resource() work, someone has to add those resources to the list.
The question is, why do we need this for AUX devices? Are you expecting
several IRQs to be dedicated for several devices (no sharing)? If now, why
is the fwnode version of IRQ getter not enough?

> Please correct me if I've misunderstood your question.

For the memory and port resources it might be indeed needed to have a split.
But then, since it's a lot of the copy from platform code, I would expect
the common library for both rather than reinventing the wheel. To achieve
that one might need to abstract away from the certain device container when
handling resources (no platform_device nor auxiliary_device). Would that
approach work?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] driver core: auxiliary bus: Introduce auxiliary device resource management

[Raag Jadav]

On Mon, May 19, 2025 at 01:44:49PM +0300, Andy Shevchenko wrote:
> On Fri, May 16, 2025 at 10:20:02PM +0300, Raag Jadav wrote:
> > On Thu, May 15, 2025 at 04:06:47PM +0300, Andy Shevchenko wrote:
> > > On Thu, May 15, 2025 at 03:52:38PM +0300, Raag Jadav wrote:
> > > > On Wed, May 14, 2025 at 03:36:49PM +0300, Andy Shevchenko wrote:
> > > > > On Wed, May 14, 2025 at 05:54:31PM +0530, Raag Jadav wrote:
> 
> ...
> 
> > > > > > +int auxiliary_get_irq_optional(struct auxiliary_device *auxdev, unsigned int num)
> > > > > > +{
> > > > > > +	struct resource *r;
> > > > > > +	int ret = -ENXIO;
> > > > > > +
> > > > > > +	r = auxiliary_get_resource(auxdev, IORESOURCE_IRQ, num);
> > > > > > +	if (!r)
> > > > > > +		goto out;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * The resources may pass trigger flags to the irqs that need to be
> > > > > > +	 * set up. It so happens that the trigger flags for IORESOURCE_BITS
> > > > > > +	 * correspond 1-to-1 to the IRQF_TRIGGER* settings.
> > > > > > +	 */
> > > > > > +	if (r->flags & IORESOURCE_BITS) {
> > > > > > +		struct irq_data *irqd;
> > > > > > +
> > > > > > +		irqd = irq_get_irq_data(r->start);
> > > > > > +		if (!irqd)
> > > > > > +			goto out;
> > > > > > +		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> > > > > > +	}
> > > > > > +
> > > > > > +	ret = r->start;
> > > > > > +	if (WARN(!ret, "0 is an invalid IRQ number\n"))
> > > > > > +		ret = -EINVAL;
> > > > > > +out:
> > > > > > +	return ret;
> > > > > > +}
> > > > > 
> > > > > Please, do not inherit the issues that the respective platform device API has.
> > > > > And after all, why do you need this? What's wrong with plain fwnode_irq_get()?
> > > > 
> > > > Can you please elaborate? Are we expecting fwnode to be supported by auxiliary
> > > > device?
> > > 
> > > Platform IRQ getter is legacy for the board files, but it has support for fwnode.
> > > Why do you need to inherit all that legacy? What's the point?
> > 
> > This is just to abstract get_resource(IRQ) which has been carved up by the
> > parent device. And since this is an auxiliary child device, I'm not sure if
> > we have a firmware to work with.
> 
> To make get_resource() work, someone has to add those resources to the list.
> The question is, why do we need this for AUX devices? Are you expecting
> several IRQs to be dedicated for several devices (no sharing)? If now, why
> is the fwnode version of IRQ getter not enough?

With PCI type MFDs, MSIX would be a fair possibility, if not now atleast
in the future.

> > Please correct me if I've misunderstood your question.
> 
> For the memory and port resources it might be indeed needed to have a split.
> But then, since it's a lot of the copy from platform code, I would expect
> the common library for both rather than reinventing the wheel. To achieve
> that one might need to abstract away from the certain device container when
> handling resources (no platform_device nor auxiliary_device). Would that
> approach work?

Sure, let me explore this. Thanks for the suggestions.

Raag