| Message ID | 1523609472-4481-1-git-send-email-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Phil, On Fri, Apr 13, 2018 at 1:51 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote: > The DesignWare GPIO IP can be configured for either 1 interrupt or 1 > per GPIO in port A, but the driver currently only supports 1 interrupt. > See the DesignWare DW_apb_gpio Databook description of the > 'GPIO_INTR_IO' parameter. > > This change allows the driver to work with up to 32 interrupts, it will > get as many interrupts as specified in the DT 'interrupts' property. > It doesn't do anything clever with the different interrupts, it just calls > the same handler used for single interrupt hardware. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > One point to mention is that I have made it possible for users to have > unconncted interrupts by specifying holes in the list of interrupts. This is > done by supporting the interrupts-extended DT prop. > However, I have no use for this and had to hack some test case for this. > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts? > > v3: > - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid bisect problems > v2: > - Replaced interrupt-mask DT prop with support for the interrupts-extended > prop. This means replacing the call to irq_of_parse_and_map() with calls > to of_irq_parse_one() and irq_create_of_mapping(). > > Note: There are a few *code* lines over 80 chars, but this is just guidance, > right? Especially as there are already some lines over 80 chars. > --- > .../devicetree/bindings/gpio/snps-dwapb-gpio.txt | 9 ++++- > drivers/gpio/gpio-dwapb.c | 43 +++++++++++++++++----- > drivers/mfd/intel_quark_i2c_gpio.c | 3 +- > include/linux/platform_data/gpio-dwapb.h | 3 +- > 4 files changed, 45 insertions(+), 13 deletions(-) > > diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > index 4a75da7..3c1118b 100644 > --- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > @@ -26,8 +26,13 @@ controller. > the second encodes the triger flags encoded as described in > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > - interrupt-parent : The parent interrupt controller. > -- interrupts : The interrupt to the parent controller raised when GPIOs > - generate the interrupts. > +- interrupts : The interrupts to the parent controller raised when GPIOs > + generate the interrupts. If the controller provides one combined interrupt > + for all GPIOs, specify a single interrupt. If the controller provides one > + interrupt for each GPIO, provide a list of interrupts that correspond to each > + of the GPIO pins. When specifying multiple interrupts, if any are unconnected, > + use the interrupts-extended property to specify the interrupts and set the > + interrupt controller handle for unused interrupts to 0. > - snps,nr-gpios : The number of pins in the port, a single cell. > - resets : Reset line for the controller. > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 226977f..3273504 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > irq_gc->chip_types[1].handler = handle_edge_irq; > > if (!pp->irq_shared) { > - irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler, > - gpio); > + 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, > + err = devm_request_irq(gpio->dev, pp->irq[0], > dwapb_irq_handler_mfd, > IRQF_SHARED, "gpio-dwapb-mfd", gpio); > if (err) { > @@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > if (pp->idx == 0) > port->gc.set_config = dwapb_gpio_set_config; > > - if (pp->irq) > + if (pp->has_irq) > dwapb_configure_irqs(gpio, port, pp); > > err = gpiochip_add_data(&port->gc, port); > @@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > port->is_registered = true; > > /* Add GPIO-signaled ACPI event support */ > - if (pp->irq) > + if (pp->has_irq) > acpi_gpiochip_request_interrupts(&port->gc); > > return err; > @@ -601,13 +606,33 @@ dwapb_gpio_get_pdata(struct device *dev) > if (dev->of_node && pp->idx == 0 && > fwnode_property_read_bool(fwnode, > "interrupt-controller")) { > - pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); > - if (!pp->irq) > + struct device_node *np = to_of_node(fwnode); > + struct of_phandle_args oirq; > + 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++) { > + if (of_irq_parse_one(np, j, &oirq)) > + continue; > + > + pp->irq[j] = irq_create_of_mapping(&oirq); > + if (pp->irq[j]) > + pp->has_irq = true; > + } > + > + if (!pp->has_irq) > dev_warn(dev, "no irq for port%d\n", pp->idx); > } > > - if (has_acpi_companion(dev) && pp->idx == 0) > - pp->irq = platform_get_irq(to_platform_device(dev), 0); > + if (has_acpi_companion(dev) && pp->idx == 0) { > + pp->irq[0] = platform_get_irq(to_platform_device(dev), 0); > + if (pp->irq[0]) > + pp->has_irq = true; > + } It doesn't work for ACPI. Could you do the same logic for ACPI? Thanks Hoan > > pp->irq_shared = false; > pp->gpio_base = -1; > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c > index 90e35de..5bddb84 100644 > --- a/drivers/mfd/intel_quark_i2c_gpio.c > +++ b/drivers/mfd/intel_quark_i2c_gpio.c > @@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell) > pdata->properties->idx = 0; > pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO; > pdata->properties->gpio_base = INTEL_QUARK_MFD_GPIO_BASE; > - pdata->properties->irq = pdev->irq; > + pdata->properties->irq[0] = pdev->irq; > + pdata->properties->has_irq = true; > pdata->properties->irq_shared = true; > > cell->platform_data = pdata; > diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h > index 2dc7f4a..5a52d69 100644 > --- a/include/linux/platform_data/gpio-dwapb.h > +++ b/include/linux/platform_data/gpio-dwapb.h > @@ -19,7 +19,8 @@ struct dwapb_port_property { > unsigned int idx; > unsigned int ngpio; > unsigned int gpio_base; > - unsigned int irq; > + unsigned int irq[32]; > + bool has_irq; > bool irq_shared; > }; > > -- > 2.7.4 >
Hi Hoan, On 13 April 2018 17:37 Hoan Tran wrote: > On Fri, Apr 13, 2018 at 1:51 AM, Phil Edworthy wrote: > > The DesignWare GPIO IP can be configured for either 1 interrupt or 1 > > per GPIO in port A, but the driver currently only supports 1 interrupt. > > See the DesignWare DW_apb_gpio Databook description of the > > 'GPIO_INTR_IO' parameter. > > > > This change allows the driver to work with up to 32 interrupts, it > > will get as many interrupts as specified in the DT 'interrupts' property. > > It doesn't do anything clever with the different interrupts, it just > > calls the same handler used for single interrupt hardware. > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > --- > > One point to mention is that I have made it possible for users to have > > unconncted interrupts by specifying holes in the list of interrupts. > > This is done by supporting the interrupts-extended DT prop. > > However, I have no use for this and had to hack some test case for this. > > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts? > > > > v3: > > - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid > > bisect problems > > v2: > > - Replaced interrupt-mask DT prop with support for the interrupts- > extended > > prop. This means replacing the call to irq_of_parse_and_map() with calls > > to of_irq_parse_one() and irq_create_of_mapping(). > > > > Note: There are a few *code* lines over 80 chars, but this is just guidance, > > right? Especially as there are already some lines over 80 chars. > > --- [snip] > > - if (has_acpi_companion(dev) && pp->idx == 0) > > - pp->irq = platform_get_irq(to_platform_device(dev), 0); > > + if (has_acpi_companion(dev) && pp->idx == 0) { > > + pp->irq[0] = platform_get_irq(to_platform_device(dev), 0); > > + if (pp->irq[0]) > > + pp->has_irq = true; > > + } > > It doesn't work for ACPI. Could you do the same logic for ACPI? I don’t have access to any device that was baked (i.e. fabbed) with multiple output interrupts from the Synopsys GPIO blocks and use ACPI. I don't know if any such device exists. I would prefer not writing code that can be tested easily. I cannot even test the current, albeit small, changes to the Intel Quark MFD. Regards Phil > Thanks > Hoan > > > > > pp->irq_shared = false; > > pp->gpio_base = -1; > > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c > > b/drivers/mfd/intel_quark_i2c_gpio.c > > index 90e35de..5bddb84 100644 > > --- a/drivers/mfd/intel_quark_i2c_gpio.c > > +++ b/drivers/mfd/intel_quark_i2c_gpio.c > > @@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct pci_dev > *pdev, struct mfd_cell *cell) > > pdata->properties->idx = 0; > > pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO; > > pdata->properties->gpio_base = INTEL_QUARK_MFD_GPIO_BASE; > > - pdata->properties->irq = pdev->irq; > > + pdata->properties->irq[0] = pdev->irq; > > + pdata->properties->has_irq = true; > > pdata->properties->irq_shared = true; > > > > cell->platform_data = pdata; > > diff --git a/include/linux/platform_data/gpio-dwapb.h > > b/include/linux/platform_data/gpio-dwapb.h > > index 2dc7f4a..5a52d69 100644 > > --- a/include/linux/platform_data/gpio-dwapb.h > > +++ b/include/linux/platform_data/gpio-dwapb.h > > @@ -19,7 +19,8 @@ struct dwapb_port_property { > > unsigned int idx; > > unsigned int ngpio; > > unsigned int gpio_base; > > - unsigned int irq; > > + unsigned int irq[32]; > > + bool has_irq; > > bool irq_shared; > > }; > > > > -- > > 2.7.4 > >
On Fri, Apr 13, 2018 at 09:51:12AM +0100, Phil Edworthy wrote: > The DesignWare GPIO IP can be configured for either 1 interrupt or 1 > per GPIO in port A, but the driver currently only supports 1 interrupt. > See the DesignWare DW_apb_gpio Databook description of the > 'GPIO_INTR_IO' parameter. > > This change allows the driver to work with up to 32 interrupts, it will > get as many interrupts as specified in the DT 'interrupts' property. > It doesn't do anything clever with the different interrupts, it just calls > the same handler used for single interrupt hardware. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > One point to mention is that I have made it possible for users to have > unconncted interrupts by specifying holes in the list of interrupts. This is > done by supporting the interrupts-extended DT prop. > However, I have no use for this and had to hack some test case for this. > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts? > > v3: > - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid bisect problems > v2: > - Replaced interrupt-mask DT prop with support for the interrupts-extended > prop. This means replacing the call to irq_of_parse_and_map() with calls > to of_irq_parse_one() and irq_create_of_mapping(). > > Note: There are a few *code* lines over 80 chars, but this is just guidance, > right? Especially as there are already some lines over 80 chars. > --- > .../devicetree/bindings/gpio/snps-dwapb-gpio.txt | 9 ++++- > drivers/gpio/gpio-dwapb.c | 43 +++++++++++++++++----- > drivers/mfd/intel_quark_i2c_gpio.c | 3 +- > include/linux/platform_data/gpio-dwapb.h | 3 +- > 4 files changed, 45 insertions(+), 13 deletions(-) > > diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > index 4a75da7..3c1118b 100644 > --- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt > @@ -26,8 +26,13 @@ controller. > the second encodes the triger flags encoded as described in > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > - interrupt-parent : The parent interrupt controller. > -- interrupts : The interrupt to the parent controller raised when GPIOs > - generate the interrupts. > +- interrupts : The interrupts to the parent controller raised when GPIOs > + generate the interrupts. If the controller provides one combined interrupt > + for all GPIOs, specify a single interrupt. If the controller provides one > + interrupt for each GPIO, provide a list of interrupts that correspond to each > + of the GPIO pins. When specifying multiple interrupts, if any are unconnected, > + use the interrupts-extended property to specify the interrupts and set the > + interrupt controller handle for unused interrupts to 0. > - snps,nr-gpios : The number of pins in the port, a single cell. > - resets : Reset line for the controller. > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 226977f..3273504 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > irq_gc->chip_types[1].handler = handle_edge_irq; > > if (!pp->irq_shared) { > - irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler, > - gpio); > + 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, > + err = devm_request_irq(gpio->dev, pp->irq[0], > dwapb_irq_handler_mfd, > IRQF_SHARED, "gpio-dwapb-mfd", gpio); > if (err) { > @@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > if (pp->idx == 0) > port->gc.set_config = dwapb_gpio_set_config; > > - if (pp->irq) > + if (pp->has_irq) > dwapb_configure_irqs(gpio, port, pp); > > err = gpiochip_add_data(&port->gc, port); > @@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > port->is_registered = true; > > /* Add GPIO-signaled ACPI event support */ > - if (pp->irq) > + if (pp->has_irq) > acpi_gpiochip_request_interrupts(&port->gc); > > return err; > @@ -601,13 +606,33 @@ dwapb_gpio_get_pdata(struct device *dev) > if (dev->of_node && pp->idx == 0 && > fwnode_property_read_bool(fwnode, > "interrupt-controller")) { > - pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); > - if (!pp->irq) > + struct device_node *np = to_of_node(fwnode); > + struct of_phandle_args oirq; > + 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++) { > + if (of_irq_parse_one(np, j, &oirq)) > + continue; > + > + pp->irq[j] = irq_create_of_mapping(&oirq); I'm hoping to not have new users of of_irq_parse_one and irq_create_of_mapping. Can you use of_irq_get instead? It will base back error codes so you can distinguish different conditions. > + if (pp->irq[j]) > + pp->has_irq = true; > + } > + > + if (!pp->has_irq) > dev_warn(dev, "no irq for port%d\n", pp->idx); > } > > - if (has_acpi_companion(dev) && pp->idx == 0) > - pp->irq = platform_get_irq(to_platform_device(dev), 0); > + if (has_acpi_companion(dev) && pp->idx == 0) { > + pp->irq[0] = platform_get_irq(to_platform_device(dev), 0); > + if (pp->irq[0]) > + pp->has_irq = true; > + } > > pp->irq_shared = false; > pp->gpio_base = -1;
Hi Rob, On 16 April 2018 21:03 Rob Herring wrote: > On Fri, Apr 13, 2018 at 09:51:12AM +0100, Phil Edworthy wrote: > > The DesignWare GPIO IP can be configured for either 1 interrupt or 1 > > per GPIO in port A, but the driver currently only supports 1 interrupt. > > See the DesignWare DW_apb_gpio Databook description of the > > 'GPIO_INTR_IO' parameter. > > > > This change allows the driver to work with up to 32 interrupts, it > > will get as many interrupts as specified in the DT 'interrupts' property. > > It doesn't do anything clever with the different interrupts, it just > > calls the same handler used for single interrupt hardware. > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > --- > > One point to mention is that I have made it possible for users to have > > unconncted interrupts by specifying holes in the list of interrupts. > > This is done by supporting the interrupts-extended DT prop. > > However, I have no use for this and had to hack some test case for this. > > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts? > > > > v3: > > - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid > > bisect problems > > v2: > > - Replaced interrupt-mask DT prop with support for the interrupts- > extended > > prop. This means replacing the call to irq_of_parse_and_map() with calls > > to of_irq_parse_one() and irq_create_of_mapping(). > > > > Note: There are a few *code* lines over 80 chars, but this is just guidance, > > right? Especially as there are already some lines over 80 chars. > > --- [snip] > > + for (j = 0; j < pp->ngpio; j++) { > > + if (of_irq_parse_one(np, j, &oirq)) > > + continue; > > + > > + pp->irq[j] = irq_create_of_mapping(&oirq); > > I'm hoping to not have new users of of_irq_parse_one and > irq_create_of_mapping. Can you use of_irq_get instead? It will base back > error codes so you can distinguish different conditions. Sure, I hadn't noticed that particular variant! Thanks Phil > > + if (pp->irq[j]) > > + pp->has_irq = true; > > + } > > + > > + if (!pp->has_irq) > > dev_warn(dev, "no irq for port%d\n", pp- > >idx); > > } > > > > - if (has_acpi_companion(dev) && pp->idx == 0) > > - pp->irq = > platform_get_irq(to_platform_device(dev), 0); > > + if (has_acpi_companion(dev) && pp->idx == 0) { > > + pp->irq[0] = > platform_get_irq(to_platform_device(dev), 0); > > + if (pp->irq[0]) > > + pp->has_irq = true; > > + } > > > > pp->irq_shared = false; > > pp->gpio_base = -1;
Hi Phil, On Fri, Apr 13, 2018 at 9:47 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote: > Hi Hoan, > > On 13 April 2018 17:37 Hoan Tran wrote: >> On Fri, Apr 13, 2018 at 1:51 AM, Phil Edworthy wrote: >> > The DesignWare GPIO IP can be configured for either 1 interrupt or 1 >> > per GPIO in port A, but the driver currently only supports 1 interrupt. >> > See the DesignWare DW_apb_gpio Databook description of the >> > 'GPIO_INTR_IO' parameter. >> > >> > This change allows the driver to work with up to 32 interrupts, it >> > will get as many interrupts as specified in the DT 'interrupts' property. >> > It doesn't do anything clever with the different interrupts, it just >> > calls the same handler used for single interrupt hardware. >> > >> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> >> > --- >> > One point to mention is that I have made it possible for users to have >> > unconncted interrupts by specifying holes in the list of interrupts. >> > This is done by supporting the interrupts-extended DT prop. >> > However, I have no use for this and had to hack some test case for this. >> > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts? >> > >> > v3: >> > - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid >> > bisect problems >> > v2: >> > - Replaced interrupt-mask DT prop with support for the interrupts- >> extended >> > prop. This means replacing the call to irq_of_parse_and_map() with calls >> > to of_irq_parse_one() and irq_create_of_mapping(). >> > >> > Note: There are a few *code* lines over 80 chars, but this is just guidance, >> > right? Especially as there are already some lines over 80 chars. >> > --- > [snip] > >> > - if (has_acpi_companion(dev) && pp->idx == 0) >> > - pp->irq = platform_get_irq(to_platform_device(dev), 0); >> > + if (has_acpi_companion(dev) && pp->idx == 0) { >> > + pp->irq[0] = platform_get_irq(to_platform_device(dev), 0); >> > + if (pp->irq[0]) >> > + pp->has_irq = true; >> > + } >> >> It doesn't work for ACPI. Could you do the same logic for ACPI? > I don’t have access to any device that was baked (i.e. fabbed) with multiple > output interrupts from the Synopsys GPIO blocks and use ACPI. I don't > know if any such device exists. Below code is tested on X-Gene system which supports 1 interrupt per GPIO on Port A. You can update it into your patch. - if (has_acpi_companion(dev) && pp->idx == 0) - pp->irq = platform_get_irq(to_platform_device(dev), 0); + 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; + } + } Thanks Hoan > > I would prefer not writing code that can be tested easily. I cannot even > test the current, albeit small, changes to the Intel Quark MFD. > > Regards > Phil > >> Thanks >> Hoan >> >> > >> > pp->irq_shared = false; >> > pp->gpio_base = -1; >> > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c >> > b/drivers/mfd/intel_quark_i2c_gpio.c >> > index 90e35de..5bddb84 100644 >> > --- a/drivers/mfd/intel_quark_i2c_gpio.c >> > +++ b/drivers/mfd/intel_quark_i2c_gpio.c >> > @@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct pci_dev >> *pdev, struct mfd_cell *cell) >> > pdata->properties->idx = 0; >> > pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO; >> > pdata->properties->gpio_base = INTEL_QUARK_MFD_GPIO_BASE; >> > - pdata->properties->irq = pdev->irq; >> > + pdata->properties->irq[0] = pdev->irq; >> > + pdata->properties->has_irq = true; >> > pdata->properties->irq_shared = true; >> > >> > cell->platform_data = pdata; >> > diff --git a/include/linux/platform_data/gpio-dwapb.h >> > b/include/linux/platform_data/gpio-dwapb.h >> > index 2dc7f4a..5a52d69 100644 >> > --- a/include/linux/platform_data/gpio-dwapb.h >> > +++ b/include/linux/platform_data/gpio-dwapb.h >> > @@ -19,7 +19,8 @@ struct dwapb_port_property { >> > unsigned int idx; >> > unsigned int ngpio; >> > unsigned int gpio_base; >> > - unsigned int irq; >> > + unsigned int irq[32]; >> > + bool has_irq; >> > bool irq_shared; >> > }; >> > >> > -- >> > 2.7.4 >> >
Hi Hoan On 18 April 2018 08:03 Hoan Tran wrote: > On Fri, Apr 13, 2018 at 9:47 AM, Phil Edworthy wrote: > > On 13 April 2018 17:37 Hoan Tran wrote: > >> On Fri, Apr 13, 2018 at 1:51 AM, Phil Edworthy wrote: > >> > The DesignWare GPIO IP can be configured for either 1 interrupt or > >> > 1 per GPIO in port A, but the driver currently only supports 1 interrupt. > >> > See the DesignWare DW_apb_gpio Databook description of the > >> > 'GPIO_INTR_IO' parameter. > >> > > >> > This change allows the driver to work with up to 32 interrupts, it > >> > will get as many interrupts as specified in the DT 'interrupts' property. > >> > It doesn't do anything clever with the different interrupts, it > >> > just calls the same handler used for single interrupt hardware. > >> > > >> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > >> > --- > >> > One point to mention is that I have made it possible for users to > >> > have unconncted interrupts by specifying holes in the list of interrupts. > >> > This is done by supporting the interrupts-extended DT prop. > >> > However, I have no use for this and had to hack some test case for this. > >> > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts? > >> > > >> > v3: > >> > - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid > >> > bisect problems > >> > v2: > >> > - Replaced interrupt-mask DT prop with support for the interrupts- > >> extended > >> > prop. This means replacing the call to irq_of_parse_and_map() with > calls > >> > to of_irq_parse_one() and irq_create_of_mapping(). > >> > > >> > Note: There are a few *code* lines over 80 chars, but this is just > guidance, > >> > right? Especially as there are already some lines over 80 chars. > >> > --- > > [snip] > > > >> > - if (has_acpi_companion(dev) && pp->idx == 0) > >> > - pp->irq = platform_get_irq(to_platform_device(dev), 0); > >> > + if (has_acpi_companion(dev) && pp->idx == 0) { > >> > + pp->irq[0] = platform_get_irq(to_platform_device(dev), > 0); > >> > + if (pp->irq[0]) > >> > + pp->has_irq = true; > >> > + } > >> > >> It doesn't work for ACPI. Could you do the same logic for ACPI? > > I don’t have access to any device that was baked (i.e. fabbed) with > > multiple output interrupts from the Synopsys GPIO blocks and use ACPI. > > I don't know if any such device exists. > > Below code is tested on X-Gene system which supports 1 interrupt per GPIO > on Port A. You can update it into your patch. > > - if (has_acpi_companion(dev) && pp->idx == 0) > - pp->irq = platform_get_irq(to_platform_device(dev), 0); > + 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; > + } > + } Since I've already got some reviewed-by and acks for v4, I'll leave it to Linus to decide if he wants me to roll your changes into this patch or for you to submit a separate patch. Thanks Phil > >> > pp->irq_shared = false; > >> > pp->gpio_base = -1; > >> > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c > >> > b/drivers/mfd/intel_quark_i2c_gpio.c > >> > index 90e35de..5bddb84 100644 > >> > --- a/drivers/mfd/intel_quark_i2c_gpio.c > >> > +++ b/drivers/mfd/intel_quark_i2c_gpio.c > >> > @@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct > >> > pci_dev > >> *pdev, struct mfd_cell *cell) > >> > pdata->properties->idx = 0; > >> > pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO; > >> > pdata->properties->gpio_base = > INTEL_QUARK_MFD_GPIO_BASE; > >> > - pdata->properties->irq = pdev->irq; > >> > + pdata->properties->irq[0] = pdev->irq; > >> > + pdata->properties->has_irq = true; > >> > pdata->properties->irq_shared = true; > >> > > >> > cell->platform_data = pdata; diff --git > >> > a/include/linux/platform_data/gpio-dwapb.h > >> > b/include/linux/platform_data/gpio-dwapb.h > >> > index 2dc7f4a..5a52d69 100644 > >> > --- a/include/linux/platform_data/gpio-dwapb.h > >> > +++ b/include/linux/platform_data/gpio-dwapb.h > >> > @@ -19,7 +19,8 @@ struct dwapb_port_property { > >> > unsigned int idx; > >> > unsigned int ngpio; > >> > unsigned int gpio_base; > >> > - unsigned int irq; > >> > + unsigned int irq[32]; > >> > + bool has_irq; > >> > bool irq_shared; > >> > }; > >> > > >> > -- > >> > 2.7.4 > >> >
Hi Phil, On Thu, Apr 19, 2018 at 3:03 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote: > Hi Hoan > > On 18 April 2018 08:03 Hoan Tran wrote: >> On Fri, Apr 13, 2018 at 9:47 AM, Phil Edworthy wrote: >> > On 13 April 2018 17:37 Hoan Tran wrote: >> >> On Fri, Apr 13, 2018 at 1:51 AM, Phil Edworthy wrote: >> >> > The DesignWare GPIO IP can be configured for either 1 interrupt or >> >> > 1 per GPIO in port A, but the driver currently only supports 1 interrupt. >> >> > See the DesignWare DW_apb_gpio Databook description of the >> >> > 'GPIO_INTR_IO' parameter. >> >> > >> >> > This change allows the driver to work with up to 32 interrupts, it >> >> > will get as many interrupts as specified in the DT 'interrupts' property. >> >> > It doesn't do anything clever with the different interrupts, it >> >> > just calls the same handler used for single interrupt hardware. >> >> > >> >> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> >> >> > --- >> >> > One point to mention is that I have made it possible for users to >> >> > have unconncted interrupts by specifying holes in the list of interrupts. >> >> > This is done by supporting the interrupts-extended DT prop. >> >> > However, I have no use for this and had to hack some test case for this. >> >> > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts? >> >> > >> >> > v3: >> >> > - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid >> >> > bisect problems >> >> > v2: >> >> > - Replaced interrupt-mask DT prop with support for the interrupts- >> >> extended >> >> > prop. This means replacing the call to irq_of_parse_and_map() with >> calls >> >> > to of_irq_parse_one() and irq_create_of_mapping(). >> >> > >> >> > Note: There are a few *code* lines over 80 chars, but this is just >> guidance, >> >> > right? Especially as there are already some lines over 80 chars. >> >> > --- >> > [snip] >> > >> >> > - if (has_acpi_companion(dev) && pp->idx == 0) >> >> > - pp->irq = platform_get_irq(to_platform_device(dev), 0); >> >> > + if (has_acpi_companion(dev) && pp->idx == 0) { >> >> > + pp->irq[0] = platform_get_irq(to_platform_device(dev), >> 0); >> >> > + if (pp->irq[0]) >> >> > + pp->has_irq = true; >> >> > + } >> >> >> >> It doesn't work for ACPI. Could you do the same logic for ACPI? >> > I don’t have access to any device that was baked (i.e. fabbed) with >> > multiple output interrupts from the Synopsys GPIO blocks and use ACPI. >> > I don't know if any such device exists. >> >> Below code is tested on X-Gene system which supports 1 interrupt per GPIO >> on Port A. You can update it into your patch. >> >> - if (has_acpi_companion(dev) && pp->idx == 0) >> - pp->irq = platform_get_irq(to_platform_device(dev), 0); >> + 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; >> + } >> + } > Since I've already got some reviewed-by and acks for v4, I'll leave it to Linus > to decide if he wants me to roll your changes into this patch or for you to > submit a separate patch. > I prefer this patch works for both DTB and ACPI. Btw let Linus decide. Thanks Hoan > Thanks > Phil > > >> >> > pp->irq_shared = false; >> >> > pp->gpio_base = -1; >> >> > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c >> >> > b/drivers/mfd/intel_quark_i2c_gpio.c >> >> > index 90e35de..5bddb84 100644 >> >> > --- a/drivers/mfd/intel_quark_i2c_gpio.c >> >> > +++ b/drivers/mfd/intel_quark_i2c_gpio.c >> >> > @@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct >> >> > pci_dev >> >> *pdev, struct mfd_cell *cell) >> >> > pdata->properties->idx = 0; >> >> > pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO; >> >> > pdata->properties->gpio_base = >> INTEL_QUARK_MFD_GPIO_BASE; >> >> > - pdata->properties->irq = pdev->irq; >> >> > + pdata->properties->irq[0] = pdev->irq; >> >> > + pdata->properties->has_irq = true; >> >> > pdata->properties->irq_shared = true; >> >> > >> >> > cell->platform_data = pdata; diff --git >> >> > a/include/linux/platform_data/gpio-dwapb.h >> >> > b/include/linux/platform_data/gpio-dwapb.h >> >> > index 2dc7f4a..5a52d69 100644 >> >> > --- a/include/linux/platform_data/gpio-dwapb.h >> >> > +++ b/include/linux/platform_data/gpio-dwapb.h >> >> > @@ -19,7 +19,8 @@ struct dwapb_port_property { >> >> > unsigned int idx; >> >> > unsigned int ngpio; >> >> > unsigned int gpio_base; >> >> > - unsigned int irq; >> >> > + unsigned int irq[32]; >> >> > + bool has_irq; >> >> > bool irq_shared; >> >> > }; >> >> > >> >> > -- >> >> > 2.7.4 >> >> >
On Thu, Apr 19, 2018 at 12:03 PM, Phil Edworthy <phil.edworthy@renesas.com> wrote: > Since I've already got some reviewed-by and acks for v4, I'll leave it to Linus > to decide if he wants me to roll your changes into this patch or for you to > submit a separate patch. Just - Add the reviewed-by and ACKs - Fold in Hoan's fix - Resend We are always a bit fuzzy around the edges with carrying ACKs etc forward for new revisions. Surely that is fine with this minor revision for ACPI. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt index 4a75da7..3c1118b 100644 --- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt @@ -26,8 +26,13 @@ controller. the second encodes the triger flags encoded as described in Documentation/devicetree/bindings/interrupt-controller/interrupts.txt - interrupt-parent : The parent interrupt controller. -- interrupts : The interrupt to the parent controller raised when GPIOs - generate the interrupts. +- interrupts : The interrupts to the parent controller raised when GPIOs + generate the interrupts. If the controller provides one combined interrupt + for all GPIOs, specify a single interrupt. If the controller provides one + interrupt for each GPIO, provide a list of interrupts that correspond to each + of the GPIO pins. When specifying multiple interrupts, if any are unconnected, + use the interrupts-extended property to specify the interrupts and set the + interrupt controller handle for unused interrupts to 0. - snps,nr-gpios : The number of pins in the port, a single cell. - resets : Reset line for the controller. diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 226977f..3273504 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, irq_gc->chip_types[1].handler = handle_edge_irq; if (!pp->irq_shared) { - irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler, - gpio); + 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, + err = devm_request_irq(gpio->dev, pp->irq[0], dwapb_irq_handler_mfd, IRQF_SHARED, "gpio-dwapb-mfd", gpio); if (err) { @@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, if (pp->idx == 0) port->gc.set_config = dwapb_gpio_set_config; - if (pp->irq) + if (pp->has_irq) dwapb_configure_irqs(gpio, port, pp); err = gpiochip_add_data(&port->gc, port); @@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, port->is_registered = true; /* Add GPIO-signaled ACPI event support */ - if (pp->irq) + if (pp->has_irq) acpi_gpiochip_request_interrupts(&port->gc); return err; @@ -601,13 +606,33 @@ dwapb_gpio_get_pdata(struct device *dev) if (dev->of_node && pp->idx == 0 && fwnode_property_read_bool(fwnode, "interrupt-controller")) { - pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); - if (!pp->irq) + struct device_node *np = to_of_node(fwnode); + struct of_phandle_args oirq; + 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++) { + if (of_irq_parse_one(np, j, &oirq)) + continue; + + pp->irq[j] = irq_create_of_mapping(&oirq); + if (pp->irq[j]) + pp->has_irq = true; + } + + if (!pp->has_irq) dev_warn(dev, "no irq for port%d\n", pp->idx); } - if (has_acpi_companion(dev) && pp->idx == 0) - pp->irq = platform_get_irq(to_platform_device(dev), 0); + if (has_acpi_companion(dev) && pp->idx == 0) { + pp->irq[0] = platform_get_irq(to_platform_device(dev), 0); + if (pp->irq[0]) + pp->has_irq = true; + } pp->irq_shared = false; pp->gpio_base = -1; diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c index 90e35de..5bddb84 100644 --- a/drivers/mfd/intel_quark_i2c_gpio.c +++ b/drivers/mfd/intel_quark_i2c_gpio.c @@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell) pdata->properties->idx = 0; pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO; pdata->properties->gpio_base = INTEL_QUARK_MFD_GPIO_BASE; - pdata->properties->irq = pdev->irq; + pdata->properties->irq[0] = pdev->irq; + pdata->properties->has_irq = true; pdata->properties->irq_shared = true; cell->platform_data = pdata; diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h index 2dc7f4a..5a52d69 100644 --- a/include/linux/platform_data/gpio-dwapb.h +++ b/include/linux/platform_data/gpio-dwapb.h @@ -19,7 +19,8 @@ struct dwapb_port_property { unsigned int idx; unsigned int ngpio; unsigned int gpio_base; - unsigned int irq; + unsigned int irq[32]; + bool has_irq; bool irq_shared; };
The DesignWare GPIO IP can be configured for either 1 interrupt or 1 per GPIO in port A, but the driver currently only supports 1 interrupt. See the DesignWare DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter. This change allows the driver to work with up to 32 interrupts, it will get as many interrupts as specified in the DT 'interrupts' property. It doesn't do anything clever with the different interrupts, it just calls the same handler used for single interrupt hardware. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- One point to mention is that I have made it possible for users to have unconncted interrupts by specifying holes in the list of interrupts. This is done by supporting the interrupts-extended DT prop. However, I have no use for this and had to hack some test case for this. Perhaps the driver should support 1 interrupt or all GPIOa as interrupts? v3: - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid bisect problems v2: - Replaced interrupt-mask DT prop with support for the interrupts-extended prop. This means replacing the call to irq_of_parse_and_map() with calls to of_irq_parse_one() and irq_create_of_mapping(). Note: There are a few *code* lines over 80 chars, but this is just guidance, right? Especially as there are already some lines over 80 chars. --- .../devicetree/bindings/gpio/snps-dwapb-gpio.txt | 9 ++++- drivers/gpio/gpio-dwapb.c | 43 +++++++++++++++++----- drivers/mfd/intel_quark_i2c_gpio.c | 3 +- include/linux/platform_data/gpio-dwapb.h | 3 +- 4 files changed, 45 insertions(+), 13 deletions(-)