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

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

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

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

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

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

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

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

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

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

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