GPIO controller often have support for IRQ: allow to easily allocate
both gpio-regmap and regmap-irq in one operation.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/gpio/gpio-regmap.c  | 23 +++++++++++++++++++++--
 include/linux/gpio/regmap.h | 15 +++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 05f8781b5204..61d5f48b445d 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -203,6 +203,7 @@ EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata);
  */
 struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config)
 {
+	struct irq_domain *irq_domain;
 	struct gpio_regmap *gpio;
 	struct gpio_chip *chip;
 	int ret;
@@ -280,8 +281,26 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	if (ret < 0)
 		goto err_free_gpio;
 
-	if (config->irq_domain) {
-		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);
+	irq_domain = config->irq_domain;
+#ifdef CONFIG_GPIOLIB_IRQCHIP
+	if (config->regmap_irq_chip) {
+		struct regmap_irq_chip_data *irq_chip_data;
+
+		ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent),
+						      config->regmap, config->regmap_irq_irqno,
+						      config->regmap_irq_flags, 0,
+						      config->regmap_irq_chip, &irq_chip_data);
+		if (ret)
+			goto err_free_gpio;
+
+		irq_domain = regmap_irq_get_domain(irq_chip_data);
+		if (config->regmap_irq_chip_data)
+			*config->regmap_irq_chip_data = irq_chip_data;
+	}
+#endif
+
+	if (irq_domain) {
+		ret = gpiochip_irqchip_add_domain(chip, irq_domain);
 		if (ret)
 			goto err_remove_gpiochip;
 	}
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index a9f7b7faf57b..55df2527b982 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -40,6 +40,14 @@ struct regmap;
  * @drvdata:		(Optional) Pointer to driver specific data which is
  *			not used by gpio-remap but is provided "as is" to the
  *			driver callback(s).
+ * @regmap_irq_chip:	(Optional) Pointer on an regmap_irq_chip structure. If
+ *			set, a regmap-irq device will be created and the IRQ
+ *			domain will be set accordingly.
+ * @regmap_irq_chip_data: (Optional) Pointer on an regmap_irq_chip_data
+ *                      structure pointer. If set, it will be populated with a
+ *                      pointer on allocated regmap_irq data.
+ * @regmap_irq_irqno	(Optional) The IRQ the device uses to signal interrupts.
+ * @regmap_irq_flags	(Optional) The IRQF_ flags to use for the interrupt.
  *
  * The ->reg_mask_xlate translates a given base address and GPIO offset to
  * register and mask pair. The base address is one of the given register
@@ -78,6 +86,13 @@ struct gpio_regmap_config {
 	int ngpio_per_reg;
 	struct irq_domain *irq_domain;
 
+#ifdef CONFIG_GPIOLIB_IRQCHIP
+	struct regmap_irq_chip *regmap_irq_chip;
+	struct regmap_irq_chip_data **regmap_irq_chip_data;
+	int regmap_irq_irqno;
+	unsigned long regmap_irq_flags;
+#endif
+
 	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
 			      unsigned int offset, unsigned int *reg,
 			      unsigned int *mask);

-- 
2.39.5

Hi,

> GPIO controller often have support for IRQ: allow to easily allocate
> both gpio-regmap and regmap-irq in one operation.
>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> ---
>  drivers/gpio/gpio-regmap.c  | 23 +++++++++++++++++++++--
>  include/linux/gpio/regmap.h | 15 +++++++++++++++
>  2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index 05f8781b5204..61d5f48b445d 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -203,6 +203,7 @@ EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata);
>   */
>  struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config)
>  {
> +	struct irq_domain *irq_domain;
>  	struct gpio_regmap *gpio;
>  	struct gpio_chip *chip;
>  	int ret;
> @@ -280,8 +281,26 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
>  	if (ret < 0)
>  		goto err_free_gpio;
>  
> -	if (config->irq_domain) {
> -		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);
> +	irq_domain = config->irq_domain;
> +#ifdef CONFIG_GPIOLIB_IRQCHIP

Why do we need this ifdef?

> +	if (config->regmap_irq_chip) {
> +		struct regmap_irq_chip_data *irq_chip_data;
> +
> +		ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent),
> +						      config->regmap, config->regmap_irq_irqno,
> +						      config->regmap_irq_flags, 0,
> +						      config->regmap_irq_chip, &irq_chip_data);
> +		if (ret)
> +			goto err_free_gpio;
> +
> +		irq_domain = regmap_irq_get_domain(irq_chip_data);
> +		if (config->regmap_irq_chip_data)
> +			*config->regmap_irq_chip_data = irq_chip_data;

I'm not a fan of misusing the config to return any data. Can we have
a normal gpio_regmap_get_...()? Usually, the config is on the stack
of the caller, what if you need to get irq_chip_data afterwards?
Then your caller has to save it somewhere.

Also, what is the advantage of this? Your caller doesn't have to
call devm_regmap_add_irq_chip_fwnode(), but on the flip side you
have to cram all its parameters in the gpio_regmap config. I'd like
to keep that small and simple (but still extensible!). IMHO just
setting the irq_domain is enough to achieve that.

-michael

> +	}
> +#endif
> +
> +	if (irq_domain) {
> +		ret = gpiochip_irqchip_add_domain(chip, irq_domain);
>  		if (ret)
>  			goto err_remove_gpiochip;
>  	}
> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> index a9f7b7faf57b..55df2527b982 100644
> --- a/include/linux/gpio/regmap.h
> +++ b/include/linux/gpio/regmap.h
> @@ -40,6 +40,14 @@ struct regmap;
>   * @drvdata:		(Optional) Pointer to driver specific data which is
>   *			not used by gpio-remap but is provided "as is" to the
>   *			driver callback(s).
> + * @regmap_irq_chip:	(Optional) Pointer on an regmap_irq_chip structure. If
> + *			set, a regmap-irq device will be created and the IRQ
> + *			domain will be set accordingly.
> + * @regmap_irq_chip_data: (Optional) Pointer on an regmap_irq_chip_data
> + *                      structure pointer. If set, it will be populated with a
> + *                      pointer on allocated regmap_irq data.
> + * @regmap_irq_irqno	(Optional) The IRQ the device uses to signal interrupts.
> + * @regmap_irq_flags	(Optional) The IRQF_ flags to use for the interrupt.
>   *
>   * The ->reg_mask_xlate translates a given base address and GPIO offset to
>   * register and mask pair. The base address is one of the given register
> @@ -78,6 +86,13 @@ struct gpio_regmap_config {
>  	int ngpio_per_reg;
>  	struct irq_domain *irq_domain;
>  
> +#ifdef CONFIG_GPIOLIB_IRQCHIP
> +	struct regmap_irq_chip *regmap_irq_chip;
> +	struct regmap_irq_chip_data **regmap_irq_chip_data;
> +	int regmap_irq_irqno;
> +	unsigned long regmap_irq_flags;
> +#endif
> +
>  	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
>  			      unsigned int offset, unsigned int *reg,
>  			      unsigned int *mask);

On Wed Mar 19, 2025 at 8:15 AM CET, Michael Walle wrote:
> Hi,
>
> > GPIO controller often have support for IRQ: allow to easily allocate
> > both gpio-regmap and regmap-irq in one operation.
> >
> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> > ---
> >  drivers/gpio/gpio-regmap.c  | 23 +++++++++++++++++++++--
> >  include/linux/gpio/regmap.h | 15 +++++++++++++++
> >  2 files changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> > index 05f8781b5204..61d5f48b445d 100644
> > --- a/drivers/gpio/gpio-regmap.c
> > +++ b/drivers/gpio/gpio-regmap.c
> > @@ -203,6 +203,7 @@ EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata);
> >   */
> >  struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config)
> >  {
> > +	struct irq_domain *irq_domain;
> >  	struct gpio_regmap *gpio;
> >  	struct gpio_chip *chip;
> >  	int ret;
> > @@ -280,8 +281,26 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
> >  	if (ret < 0)
> >  		goto err_free_gpio;
> >  
> > -	if (config->irq_domain) {
> > -		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);
> > +	irq_domain = config->irq_domain;
> > +#ifdef CONFIG_GPIOLIB_IRQCHIP
>
> Why do we need this ifdef?
>

Hum yes, on second thought we probably need to depend on
CONFIG_REGMAP_IRQ here.

> > +	if (config->regmap_irq_chip) {
> > +		struct regmap_irq_chip_data *irq_chip_data;
> > +
> > +		ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent),
> > +						      config->regmap, config->regmap_irq_irqno,
> > +						      config->regmap_irq_flags, 0,
> > +						      config->regmap_irq_chip, &irq_chip_data);
> > +		if (ret)
> > +			goto err_free_gpio;
> > +
> > +		irq_domain = regmap_irq_get_domain(irq_chip_data);
> > +		if (config->regmap_irq_chip_data)
> > +			*config->regmap_irq_chip_data = irq_chip_data;
>
> I'm not a fan of misusing the config to return any data. Can we have
> a normal gpio_regmap_get_...()? Usually, the config is on the stack
> of the caller, what if you need to get irq_chip_data afterwards?
> Then your caller has to save it somewhere.
>

Yes, makes sense. As suggested by Andy Shevchenko, I will remove this
parameter as there is no user today: a way to retrieve it can be added
later if needed.

> Also, what is the advantage of this? Your caller doesn't have to
> call devm_regmap_add_irq_chip_fwnode(), but on the flip side you
> have to cram all its parameters in the gpio_regmap config. I'd like
> to keep that small and simple (but still extensible!). IMHO just
> setting the irq_domain is enough to achieve that.

This was a request from Andy on my previous series.

>
> -michael
>

Thanks for your review.
Mathieu

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Hi,

> > > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> >
> > Why do we need this ifdef?
> >
>
> Hum yes, on second thought we probably need to depend on
> CONFIG_REGMAP_IRQ here.

But then, you'd also require the regmap_irq support for chips that
don't support IRQs at all. devm_regmap_add_irq_fwnode() seems to be
missing a stub version.

-michael

On Tue Mar 25, 2025 at 8:50 AM CET, Michael Walle wrote:
> > > > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > >
> > > Why do we need this ifdef?
> > >
> >
> > Hum yes, on second thought we probably need to depend on
> > CONFIG_REGMAP_IRQ here.
>
> But then, you'd also require the regmap_irq support for chips that
> don't support IRQs at all. devm_regmap_add_irq_fwnode() seems to be
> missing a stub version.
>

Sorry, maybe my previous message was not clear, when I said "depend",
what I meant is having an "#ifdef CONFIG_REGMAP_IRQ" here in place of
"#ifdef CONFIG_GPIOLIB_IRQCHIP"

If CONFIG_REGMAP_IRQ is enabled, drivers/base/regmap/regmap-irq.c is
built, so we do have both devm_regmap_add_irq_chip_fwnode() and
regmap_irq_get_domain(). So this code block should compile and link
correctly.

I did some build tests with and without CONFIG_GPIOLIB_IRQCHIP and I
believe this is fine.

Or am I missing something?


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

On Wed Mar 26, 2025 at 12:00 PM CET, Mathieu Dubois-Briand wrote:
> On Tue Mar 25, 2025 at 8:50 AM CET, Michael Walle wrote:
> > > > > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > > >
> > > > Why do we need this ifdef?
> > > >
> > >
> > > Hum yes, on second thought we probably need to depend on
> > > CONFIG_REGMAP_IRQ here.
> >
> > But then, you'd also require the regmap_irq support for chips that
> > don't support IRQs at all. devm_regmap_add_irq_fwnode() seems to be
> > missing a stub version.
> >
>
> Sorry, maybe my previous message was not clear, when I said "depend",
> what I meant is having an "#ifdef CONFIG_REGMAP_IRQ" here in place of
> "#ifdef CONFIG_GPIOLIB_IRQCHIP"
>
> If CONFIG_REGMAP_IRQ is enabled, drivers/base/regmap/regmap-irq.c is
> built, so we do have both devm_regmap_add_irq_chip_fwnode() and
> regmap_irq_get_domain(). So this code block should compile and link
> correctly.

Yes.

> I did some build tests with and without CONFIG_GPIOLIB_IRQCHIP and I
> believe this is fine.
>
> Or am I missing something?

I'd like to avoid the ifdef macros if possible. Thus you'd need
stubs for devm_regmap_add_irq_chip_fwnode() and
regmap_irq_get_domain() if CONFIG_REGMAP_IRQ is not defined.

Not sure if broonie agrees though (?).

-michael

On Thu, Mar 20, 2025 at 09:35:15AM +0100, Mathieu Dubois-Briand wrote:
> On Wed Mar 19, 2025 at 8:15 AM CET, Michael Walle wrote:

...

> > Also, what is the advantage of this? Your caller doesn't have to
> > call devm_regmap_add_irq_chip_fwnode(), but on the flip side you
> > have to cram all its parameters in the gpio_regmap config. I'd like
> > to keep that small and simple (but still extensible!). IMHO just
> > setting the irq_domain is enough to achieve that.
> 
> This was a request from Andy on my previous series.

The benefit is deduplication of a lot of code. You may consider it the same as
GPIO library does with IRQ chip. This is just the same on a different level.

Besides the driver in this series, I would think of other GPIO drivers that
are not (yet) converted to regmap (partially because of this is being absent)
or existing drivers, if any, that may utilise it.

-- 
With Best Regards,
Andy Shevchenko

Hi,

> > > Also, what is the advantage of this? Your caller doesn't have to
> > > call devm_regmap_add_irq_chip_fwnode(), but on the flip side you
> > > have to cram all its parameters in the gpio_regmap config. I'd like
> > > to keep that small and simple (but still extensible!). IMHO just
> > > setting the irq_domain is enough to achieve that.
> > 
> > This was a request from Andy on my previous series.
>
> The benefit is deduplication of a lot of code. You may consider it the same as
> GPIO library does with IRQ chip. This is just the same on a different level.

I'd say "a lot of code" is slightly exaggerated :-) I was hesitant
because it sounded like a one-off for the regmap_irq support. There
could theoretically be other irq_domain providers (I think).

I just had a quick look at all the gpio_regmap drivers and they all
use regmap_irq. So maybe it's fair to say that one could be directly
supported within gpio_regmap.

> Besides the driver in this series, I would think of other GPIO drivers that
> are not (yet) converted to regmap (partially because of this is being absent)
> or existing drivers, if any, that may utilise it.

Yes probably all of the existing ones :)

-michael

On Tue, Mar 18, 2025 at 05:26:22PM +0100, Mathieu Dubois-Briand wrote:
> GPIO controller often have support for IRQ: allow to easily allocate
> both gpio-regmap and regmap-irq in one operation.

...

> -	if (config->irq_domain) {
> -		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);

> +	irq_domain = config->irq_domain;

Better to move it into #else, so we avoid double assignment (see below).

> +#ifdef CONFIG_GPIOLIB_IRQCHIP
> +	if (config->regmap_irq_chip) {
> +		struct regmap_irq_chip_data *irq_chip_data;
> +
> +		ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent),
> +						      config->regmap, config->regmap_irq_irqno,
> +						      config->regmap_irq_flags, 0,
> +						      config->regmap_irq_chip, &irq_chip_data);
> +		if (ret)
> +			goto err_free_gpio;
> +
> +		irq_domain = regmap_irq_get_domain(irq_chip_data);
> +		if (config->regmap_irq_chip_data)
> +			*config->regmap_irq_chip_data = irq_chip_data;

Hmm... I was under impression that we don't need this to be returned.
Do we have any user of it right now? If not, no need to export until
it is needed.

> +	}

	} else

> +#endif

	irq_domain = config->irq_domain;

> +
> +	if (irq_domain) {
> +		ret = gpiochip_irqchip_add_domain(chip, irq_domain);
>  		if (ret)
>  			goto err_remove_gpiochip;
>  	}

...

> +#ifdef CONFIG_GPIOLIB_IRQCHIP
> +	struct regmap_irq_chip *regmap_irq_chip;
> +	struct regmap_irq_chip_data **regmap_irq_chip_data;

But why double pointer?

> +	int regmap_irq_irqno;
> +	unsigned long regmap_irq_flags;
> +#endif

-- 
With Best Regards,
Andy Shevchenko

On Tue Mar 18, 2025 at 5:52 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:22PM +0100, Mathieu Dubois-Briand wrote:
> > GPIO controller often have support for IRQ: allow to easily allocate
> > both gpio-regmap and regmap-irq in one operation.
>
> ...
>
> > -	if (config->irq_domain) {
> > -		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);
>
> > +	irq_domain = config->irq_domain;
>
> Better to move it into #else, so we avoid double assignment (see below).
>

OK

> > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > +	if (config->regmap_irq_chip) {
> > +		struct regmap_irq_chip_data *irq_chip_data;
> > +
> > +		ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent),
> > +						      config->regmap, config->regmap_irq_irqno,
> > +						      config->regmap_irq_flags, 0,
> > +						      config->regmap_irq_chip, &irq_chip_data);
> > +		if (ret)
> > +			goto err_free_gpio;
> > +
> > +		irq_domain = regmap_irq_get_domain(irq_chip_data);
> > +		if (config->regmap_irq_chip_data)
> > +			*config->regmap_irq_chip_data = irq_chip_data;
>
> Hmm... I was under impression that we don't need this to be returned.
> Do we have any user of it right now? If not, no need to export until
> it is needed.
>

Right, I will remove it.

> > +	}
>
> 	} else
>
> > +#endif
>
> 	irq_domain = config->irq_domain;
>
> > +
> > +	if (irq_domain) {
> > +		ret = gpiochip_irqchip_add_domain(chip, irq_domain);
> >  		if (ret)
> >  			goto err_remove_gpiochip;
> >  	}
>
> ...
>
> > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > +	struct regmap_irq_chip *regmap_irq_chip;
> > +	struct regmap_irq_chip_data **regmap_irq_chip_data;
>
> But why double pointer?
>

I believe this has to be a double pointer, as it is going to be assigned
a pointer value: data buffer is allocated inside of
devm_regmap_add_irq_chip_fwnode().

But as you said, it's better to remove it and add it later if there is
an use case.

> > +	int regmap_irq_irqno;
> > +	unsigned long regmap_irq_flags;
> > +#endif

Thanks for your review.


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

On Thu, Mar 20, 2025 at 08:55:57AM +0100, Mathieu Dubois-Briand wrote:
> On Tue Mar 18, 2025 at 5:52 PM CET, Andy Shevchenko wrote:
> > On Tue, Mar 18, 2025 at 05:26:22PM +0100, Mathieu Dubois-Briand wrote:

...

> > > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > > +	struct regmap_irq_chip *regmap_irq_chip;
> > > +	struct regmap_irq_chip_data **regmap_irq_chip_data;
> >
> > But why double pointer?
> 
> I believe this has to be a double pointer, as it is going to be assigned
> a pointer value: data buffer is allocated inside of
> devm_regmap_add_irq_chip_fwnode().

Yes, but it doesn't need to be a double one in the data structrure, right?

> But as you said, it's better to remove it and add it later if there is
> an use case.

This would be even better for now, thanks!

-- 
With Best Regards,
Andy Shevchenko