2023-06-21 17:52:42

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v1 5/5] gpiolib: Replace open coded gpiochip_irqchip_add_allocated_domain()

Replace open coded variant of gpiochip_irqchip_add_allocated_domain()
in gpiochip_add_irqchip().

Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpiolib.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 59d87e60b108..bc8b9d6afe0e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1642,6 +1642,9 @@ static int gpiochip_irqchip_add_allocated_domain(struct gpio_chip *gc,
if (!domain)
return -EINVAL;

+ if (gc->to_irq)
+ chip_warn(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__);
+
gc->to_irq = gpiochip_to_irq;
gc->irq.domain = domain;
gc->irq.domain_is_allocated_externally = allocated_externally;
@@ -1672,6 +1675,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
struct irq_domain *domain;
unsigned int type;
unsigned int i;
+ int ret;

if (!irqchip)
return 0;
@@ -1692,10 +1696,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
"%pfw: Ignoring %u default trigger\n", fwnode, type))
type = IRQ_TYPE_NONE;

- if (gc->to_irq)
- chip_warn(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__);
-
- gc->to_irq = gpiochip_to_irq;
gc->irq.default_type = type;
gc->irq.lock_key = lock_key;
gc->irq.request_key = request_key;
@@ -1708,7 +1708,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
}
if (IS_ERR(domain))
return PTR_ERR(domain);
- gc->irq.domain = domain;

if (gc->irq.parent_handler) {
for (i = 0; i < gc->irq.num_parents; i++) {
@@ -1732,14 +1731,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,

gpiochip_set_irq_hooks(gc);

- /*
- * Using barrier() here to prevent compiler from reordering
- * gc->irq.initialized before initialization of above
- * GPIO chip irq members.
- */
- barrier();
-
- gc->irq.initialized = true;
+ ret = gpiochip_irqchip_add_allocated_domain(gc, domain, false);
+ if (ret)
+ return ret;

acpi_gpiochip_request_interrupts(gc);

--
2.40.0.1.gaa8946217a0b



2023-06-30 12:30:14

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v1 5/5] gpiolib: Replace open coded gpiochip_irqchip_add_allocated_domain()

On Wed, Jun 21, 2023 at 7:49 PM Andy Shevchenko
<[email protected]> wrote:

> Replace open coded variant of gpiochip_irqchip_add_allocated_domain()
> in gpiochip_add_irqchip().
>
> Signed-off-by: Andy Shevchenko <[email protected]>

and this concludes patches 4,5 very nicely as well.
Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2023-06-30 14:13:59

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v1 5/5] gpiolib: Replace open coded gpiochip_irqchip_add_allocated_domain()

On Fri, Jun 30, 2023 at 2:52 PM Linus Walleij <[email protected]> wrote:
>
> On Wed, Jun 21, 2023 at 7:49 PM Andy Shevchenko
> <[email protected]> wrote:
>
> > Replace open coded variant of gpiochip_irqchip_add_allocated_domain()
> > in gpiochip_add_irqchip().
> >
> > Signed-off-by: Andy Shevchenko <[email protected]>
>
> and this concludes patches 4,5 very nicely as well.

Yep!

> Reviewed-by: Linus Walleij <[email protected]>

Thank you for the review!

While at it, I would like to ask on ->to_irq() callback. IIUC
assigning it with an IRQ chip makes a dead code in the driver. Am I
correct? If not, can somebody shed some light on how the RT5677
driver, for example, works with GPIO IRQ?


--
With Best Regards,
Andy Shevchenko

2023-07-03 22:45:20

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v1 5/5] gpiolib: Replace open coded gpiochip_irqchip_add_allocated_domain()

On Fri, Jun 30, 2023 at 3:58 PM Andy Shevchenko
<[email protected]> wrote:

> While at it, I would like to ask on ->to_irq() callback. IIUC
> assigning it with an IRQ chip makes a dead code in the driver. Am I
> correct?

It's fine to assign it with an IRQ chip but not with GPIOLIB_IRQCHIP,
i.e. gc->irq better be NULL.

> If not, can somebody shed some light on how the RT5677
> driver, for example, works with GPIO IRQ?

That is theoretically fine (I don't know this HW in particular).
It looks a bit fragile...

It's just a helper to translate a GPIO line to the corresponding Linux
IRQ number and when using GPIOLIB_IRQCHIP the GPIO core
will do this using the irqdomain, else it is up to the driver.

Yours,
Linus Walleij