lkml.org 
[lkml]   [2018]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v5] gpio: dwapb: Add support for 1 interrupt per port A GPIO
On Thu, Apr 26, 2018 at 7:19 PM, Phil Edworthy
<phil.edworthy@renesas.com> wrote:

Sotty fo a late response. Consider follow up fixes for below.

> if (!pp->irq_shared) {
> + int i;
> +
> + for (i = 0; i < pp->ngpio; i++) {
> + if (pp->irq[i])
> + irq_set_chained_handler_and_data(pp->irq[i],
> + dwapb_irq_handler, gpio);
> + }
> } else {
> /*
> * Request a shared IRQ since where MFD would have devices
> * using the same irq pin
> */
> + err = devm_request_irq(gpio->dev, pp->irq[0],
> dwapb_irq_handler_mfd,
> IRQF_SHARED, "gpio-dwapb-mfd", gpio);

> + if (pp->has_irq)
> dwapb_configure_irqs(gpio, port, pp);

I would rather make irq array a type of signed int and move
conditional into the function to test per IRQ based.

> /* Add GPIO-signaled ACPI event support */
> + if (pp->has_irq)
> acpi_gpiochip_request_interrupts(&port->gc);

Perhaps something similar.

> if (dev->of_node && pp->idx == 0 &&
> fwnode_property_read_bool(fwnode,
> "interrupt-controller")) {

> + struct device_node *np = to_of_node(fwnode);
> + unsigned int j;
> +
> + /*
> + * The IP has configuration options to allow a single
> + * combined interrupt or one per gpio. If one per gpio,
> + * some might not be used.
> + */
> + for (j = 0; j < pp->ngpio; j++) {
> + int irq = of_irq_get(np, j);
> + if (irq < 0)
> + continue;
> +
> + pp->irq[j] = irq;
> + pp->has_irq = true;
> + }

for (...)
pp->irq = of_irq_get();

> }

> + if (has_acpi_companion(dev) && pp->idx == 0) {
> + unsigned int j;
> +
> + for (j = 0; j < pp->ngpio; j++) {
> + pp->irq[j] = platform_get_irq(to_platform_device(dev), j);
> + if (pp->irq[j])
> + pp->has_irq = true;
> + }

Ditto.
Moreover you have a bug here. See my proposal at the top of this message.

And now even better to ask, why platform_get_irq() wouldn't work for DT case?

> +
> + if (!pp->has_irq)
> dev_warn(dev, "no irq for port%d\n", pp->idx);

This could be issued in the actual function which will try to allocate
IRQs (perhaps on debug level)


P.S. Just think about it, perhaps you find even better solutions.

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2018-05-05 12:49    [W:0.055 / U:2.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site