| Message ID | 20250318-mdb-max7360-support-v5-6-fb20baf97da0@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for MAX7360 | expand |
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
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 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.
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
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!
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.
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
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
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);
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(-)