| Message ID | 20200512184513.86883-3-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/4] gpio: dwapb: avoid error message for optional IRQ | expand |
On Tue, May 12, 2020 at 09:45:12PM +0300, Andy Shevchenko wrote: > There is no need to have an additional check to call > acpi_gpiochip_request_interrupts(). Even without any interrupts available > the registered ACPI Event handlers can be useful for debugging purposes. > > While at it, add missed acpi_gpiochip_free_interrupts() call when > unregistering ports. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Serge Semin <fancer.lancer@gmail.com> > --- > drivers/gpio/gpio-dwapb.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 78662d6d73634e..2975f2d369874a 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -505,26 +505,33 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > dwapb_configure_irqs(gpio, port, pp); > > err = gpiochip_add_data(&port->gc, port); > - if (err) > + if (err) { > dev_err(gpio->dev, "failed to register gpiochip for port%d\n", > port->idx); > - else > - port->is_registered = true; > + return err; > + } > > /* Add GPIO-signaled ACPI event support */ > - if (pp->has_irq) > - acpi_gpiochip_request_interrupts(&port->gc); > + acpi_gpiochip_request_interrupts(&port->gc); Hm, perhaps replacing it with: + if (pp->idx == 0) + acpi_gpiochip_request_interrupts(&port->gc); could be more appropriate seeing Port A only supports IRQs, which we'd point out by the (idx == 0) conditional statement. So we don't have to call the method at most four times for each available port. Though judging by the acpi_gpiochip_request_interrupts() function internals it will just ignore GPIO chips with no IRQ support. Andy, It's up to you to decide. I'm not against the change the way it is, but if you agree that signifying the IRQs affiliation would be better, then please fill free to add the conditional statement I suggested. > > - return err; > + port->is_registered = true; > + > + return 0; > } > > static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) > { > unsigned int m; > > - for (m = 0; m < gpio->nr_ports; ++m) > - if (gpio->ports[m].is_registered) > - gpiochip_remove(&gpio->ports[m].gc); > + for (m = 0; m < gpio->nr_ports; ++m) { > + struct dwapb_gpio_port *port = &gpio->ports[m]; > + > + if (!port->is_registered) > + continue; > + > + acpi_gpiochip_free_interrupts(&port->gc); > + gpiochip_remove(&port->gc); > + } > } Could you please move this change to a dedicated patch? It seems to me this alteration might be appropriate to be ported to the stable kernels seeing it fixes e6cb3486f5a1 ("gpio: dwapb: add gpio-signaled acpi event support"). Linus, what do you think? -Sergey > > static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode, > -- > 2.26.2 >
On Sun, May 17, 2020 at 04:55:24PM +0300, Serge Semin wrote: > On Tue, May 12, 2020 at 09:45:12PM +0300, Andy Shevchenko wrote: > > There is no need to have an additional check to call > > acpi_gpiochip_request_interrupts(). Even without any interrupts available > > the registered ACPI Event handlers can be useful for debugging purposes. > > > > While at it, add missed acpi_gpiochip_free_interrupts() call when > > unregistering ports. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: Serge Semin <fancer.lancer@gmail.com> > > --- [nip] > > - if (pp->has_irq) > > - acpi_gpiochip_request_interrupts(&port->gc); > > + acpi_gpiochip_request_interrupts(&port->gc); > > Hm, perhaps replacing it with: > + if (pp->idx == 0) > + acpi_gpiochip_request_interrupts(&port->gc); > could be more appropriate seeing Port A only supports IRQs, which we'd point > out by the (idx == 0) conditional statement. So we don't have to call > the method at most four times for each available port. Though judging by the > acpi_gpiochip_request_interrupts() function internals it will just ignore > GPIO chips with no IRQ support. Andy, It's up to you to decide. I'm not against > the change the way it is, but if you agree that signifying the IRQs affiliation > would be better, then please fill free to add the conditional statement I > suggested. > Please see my comment below, before you decide what to do with this part of the patch. [nip] > > > static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) > > { > > unsigned int m; > > > > - for (m = 0; m < gpio->nr_ports; ++m) > > - if (gpio->ports[m].is_registered) > > - gpiochip_remove(&gpio->ports[m].gc); > > + for (m = 0; m < gpio->nr_ports; ++m) { > > + struct dwapb_gpio_port *port = &gpio->ports[m]; > > + > > + if (!port->is_registered) > > + continue; > > + > > + acpi_gpiochip_free_interrupts(&port->gc); > > + gpiochip_remove(&port->gc); > > + } > > } > > Could you please move this change to a dedicated patch? It seems to me this > alteration might be appropriate to be ported to the stable kernels seeing it > fixes e6cb3486f5a1 ("gpio: dwapb: add gpio-signaled acpi event support"). > Linus, what do you think? > > -Sergey > BTW after moving the change with acpi_gpiochip_free_interrupts() into a dedicated patch, you can freely merge the rest of this patch into the last one of this series. So the has_irq flag cleanup would be performed in a single commit. Especially if you implement the comment I provided above regarding conditional (idx == 0) calling of the acpi_gpiochip_request_interrupts() method. So your series will look like this: gpio: dwapb: avoid error message for optional IRQ gpio: dwapb: Don't use 0 as valid Linux interrupt number gpio: dwapb: Call acpi_gpiochip_free_interrupts() on GPIO chip de-registration (<= This commit can be moved to the head of the series as being marked by the Fixes tag) gpio: dwapb: Remove redundant has_irq flag support -Sergey
On Sun, May 17, 2020 at 05:47:37PM +0300, Serge Semin wrote: > On Sun, May 17, 2020 at 04:55:24PM +0300, Serge Semin wrote: > > On Tue, May 12, 2020 at 09:45:12PM +0300, Andy Shevchenko wrote: ... > > > static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) > > > { > > > unsigned int m; > > > > > > - for (m = 0; m < gpio->nr_ports; ++m) > > > - if (gpio->ports[m].is_registered) > > > - gpiochip_remove(&gpio->ports[m].gc); > > > + for (m = 0; m < gpio->nr_ports; ++m) { > > > + struct dwapb_gpio_port *port = &gpio->ports[m]; > > > + > > > + if (!port->is_registered) > > > + continue; > > > + > > > + acpi_gpiochip_free_interrupts(&port->gc); > > > + gpiochip_remove(&port->gc); > > > + } > > > } > > > > Could you please move this change to a dedicated patch? It seems to me this > > alteration might be appropriate to be ported to the stable kernels seeing it > > fixes e6cb3486f5a1 ("gpio: dwapb: add gpio-signaled acpi event support"). > > Linus, what do you think? > > > > -Sergey > > > > BTW after moving the change with acpi_gpiochip_free_interrupts() into a > dedicated patch, you can freely merge the rest of this patch into the > last one of this series. So the has_irq flag cleanup would be performed in a > single commit. Especially if you implement the comment I provided above regarding > conditional (idx == 0) calling of the acpi_gpiochip_request_interrupts() method. I was thinking about this split and came to the conclusion that it will be a bit awkward to introduce additional check in the ->dwapb_gpio_unregister() for freeing ACPI Event handling which will be removed by one of following patch (even taking into consideration backporting). What I propose here is an alternative, i.e. I make this patch as first in the series and will focus / dedicate it as a fix rather than clean up. > So your series will look like this: > gpio: dwapb: avoid error message for optional IRQ > gpio: dwapb: Don't use 0 as valid Linux interrupt number > gpio: dwapb: Call acpi_gpiochip_free_interrupts() on GPIO chip de-registration (<= This commit can be moved to the head of the series as being marked by the > Fixes tag) Yes, something like this, but keeping this form of the patch. Thanks for the suggestion! > gpio: dwapb: Remove redundant has_irq flag support
On Sun, May 17, 2020 at 04:55:21PM +0300, Serge Semin wrote: > On Tue, May 12, 2020 at 09:45:12PM +0300, Andy Shevchenko wrote: > > There is no need to have an additional check to call > > acpi_gpiochip_request_interrupts(). Even without any interrupts available > > the registered ACPI Event handlers can be useful for debugging purposes. ... > > - if (pp->has_irq) > > - acpi_gpiochip_request_interrupts(&port->gc); > > + acpi_gpiochip_request_interrupts(&port->gc); > > Hm, perhaps replacing it with: > + if (pp->idx == 0) > + acpi_gpiochip_request_interrupts(&port->gc); > could be more appropriate seeing Port A only supports IRQs, which we'd point > out by the (idx == 0) conditional statement. So we don't have to call > the method at most four times for each available port. Though judging by the > acpi_gpiochip_request_interrupts() function internals it will just ignore > GPIO chips with no IRQ support. Andy, It's up to you to decide. I'm not against > the change the way it is, but if you agree that signifying the IRQs affiliation > would be better, then please fill free to add the conditional statement I > suggested. It's really harmless to call it for each port. It allows as a side effect see issues with ACPI tables which may refer to a wrong port / device and thus getting no certain event handled. I prefer to unconditionally call it.
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 78662d6d73634e..2975f2d369874a 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -505,26 +505,33 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, dwapb_configure_irqs(gpio, port, pp); err = gpiochip_add_data(&port->gc, port); - if (err) + if (err) { dev_err(gpio->dev, "failed to register gpiochip for port%d\n", port->idx); - else - port->is_registered = true; + return err; + } /* Add GPIO-signaled ACPI event support */ - if (pp->has_irq) - acpi_gpiochip_request_interrupts(&port->gc); + acpi_gpiochip_request_interrupts(&port->gc); - return err; + port->is_registered = true; + + return 0; } static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) { unsigned int m; - for (m = 0; m < gpio->nr_ports; ++m) - if (gpio->ports[m].is_registered) - gpiochip_remove(&gpio->ports[m].gc); + for (m = 0; m < gpio->nr_ports; ++m) { + struct dwapb_gpio_port *port = &gpio->ports[m]; + + if (!port->is_registered) + continue; + + acpi_gpiochip_free_interrupts(&port->gc); + gpiochip_remove(&port->gc); + } } static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
There is no need to have an additional check to call acpi_gpiochip_request_interrupts(). Even without any interrupts available the registered ACPI Event handlers can be useful for debugging purposes. While at it, add missed acpi_gpiochip_free_interrupts() call when unregistering ports. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Serge Semin <fancer.lancer@gmail.com> --- drivers/gpio/gpio-dwapb.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)