diff mbox series

[v3] gpio: dwapb: Add support for 1 interrupt per port A GPIO

Message ID 1523609472-4481-1-git-send-email-phil.edworthy@renesas.com
State New
Headers show
Series [v3] gpio: dwapb: Add support for 1 interrupt per port A GPIO | expand

Commit Message

Phil Edworthy April 13, 2018, 8:51 a.m. UTC
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(-)

Comments

Hoan Tran April 13, 2018, 4:36 p.m. UTC | #1
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Edworthy April 13, 2018, 4:47 p.m. UTC | #2
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
> >
Rob Herring April 16, 2018, 8:03 p.m. UTC | #3
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;
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Edworthy April 17, 2018, 11:43 a.m. UTC | #4
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;
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hoan Tran April 18, 2018, 7:02 a.m. UTC | #5
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
>> >
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Edworthy April 19, 2018, 10:03 a.m. UTC | #6
SGkgSG9hbg0KDQpPbiAxOCBBcHJpbCAyMDE4IDA4OjAzIEhvYW4gVHJhbiB3cm90ZToNCj4gT24g
RnJpLCBBcHIgMTMsIDIwMTggYXQgOTo0NyBBTSwgUGhpbCBFZHdvcnRoeSB3cm90ZToNCj4gPiBP
biAxMyBBcHJpbCAyMDE4IDE3OjM3IEhvYW4gVHJhbiB3cm90ZToNCj4gPj4gT24gRnJpLCBBcHIg
MTMsIDIwMTggYXQgMTo1MSBBTSwgUGhpbCBFZHdvcnRoeSB3cm90ZToNCj4gPj4gPiBUaGUgRGVz
aWduV2FyZSBHUElPIElQIGNhbiBiZSBjb25maWd1cmVkIGZvciBlaXRoZXIgMSBpbnRlcnJ1cHQg
b3INCj4gPj4gPiAxIHBlciBHUElPIGluIHBvcnQgQSwgYnV0IHRoZSBkcml2ZXIgY3VycmVudGx5
IG9ubHkgc3VwcG9ydHMgMSBpbnRlcnJ1cHQuDQo+ID4+ID4gU2VlIHRoZSBEZXNpZ25XYXJlIERX
X2FwYl9ncGlvIERhdGFib29rIGRlc2NyaXB0aW9uIG9mIHRoZQ0KPiA+PiA+ICdHUElPX0lOVFJf
SU8nIHBhcmFtZXRlci4NCj4gPj4gPg0KPiA+PiA+IFRoaXMgY2hhbmdlIGFsbG93cyB0aGUgZHJp
dmVyIHRvIHdvcmsgd2l0aCB1cCB0byAzMiBpbnRlcnJ1cHRzLCBpdA0KPiA+PiA+IHdpbGwgZ2V0
IGFzIG1hbnkgaW50ZXJydXB0cyBhcyBzcGVjaWZpZWQgaW4gdGhlIERUICdpbnRlcnJ1cHRzJyBw
cm9wZXJ0eS4NCj4gPj4gPiBJdCBkb2Vzbid0IGRvIGFueXRoaW5nIGNsZXZlciB3aXRoIHRoZSBk
aWZmZXJlbnQgaW50ZXJydXB0cywgaXQNCj4gPj4gPiBqdXN0IGNhbGxzIHRoZSBzYW1lIGhhbmRs
ZXIgdXNlZCBmb3Igc2luZ2xlIGludGVycnVwdCBoYXJkd2FyZS4NCj4gPj4gPg0KPiA+PiA+IFNp
Z25lZC1vZmYtYnk6IFBoaWwgRWR3b3J0aHkgPHBoaWwuZWR3b3J0aHlAcmVuZXNhcy5jb20+DQo+
ID4+ID4gLS0tDQo+ID4+ID4gT25lIHBvaW50IHRvIG1lbnRpb24gaXMgdGhhdCBJIGhhdmUgbWFk
ZSBpdCBwb3NzaWJsZSBmb3IgdXNlcnMgdG8NCj4gPj4gPiBoYXZlIHVuY29ubmN0ZWQgaW50ZXJy
dXB0cyBieSBzcGVjaWZ5aW5nIGhvbGVzIGluIHRoZSBsaXN0IG9mIGludGVycnVwdHMuDQo+ID4+
ID4gVGhpcyBpcyBkb25lIGJ5IHN1cHBvcnRpbmcgdGhlIGludGVycnVwdHMtZXh0ZW5kZWQgRFQg
cHJvcC4NCj4gPj4gPiBIb3dldmVyLCBJIGhhdmUgbm8gdXNlIGZvciB0aGlzIGFuZCBoYWQgdG8g
aGFjayBzb21lIHRlc3QgY2FzZSBmb3IgdGhpcy4NCj4gPj4gPiBQZXJoYXBzIHRoZSBkcml2ZXIg
c2hvdWxkIHN1cHBvcnQgMSBpbnRlcnJ1cHQgb3IgYWxsIEdQSU9hIGFzIGludGVycnVwdHM/DQo+
ID4+ID4NCj4gPj4gPiB2MzoNCj4gPj4gPiAgLSBSb2xsZWQgbWZkOiBpbnRlbF9xdWFya19pMmNf
Z3BpbyBmaXggaW50byB0aGlzIHBhdGNoIHRvIGF2b2lkDQo+ID4+ID4gYmlzZWN0IHByb2JsZW1z
DQo+ID4+ID4gdjI6DQo+ID4+ID4gIC0gUmVwbGFjZWQgaW50ZXJydXB0LW1hc2sgRFQgcHJvcCB3
aXRoIHN1cHBvcnQgZm9yIHRoZSBpbnRlcnJ1cHRzLQ0KPiA+PiBleHRlbmRlZA0KPiA+PiA+ICAg
IHByb3AuIFRoaXMgbWVhbnMgcmVwbGFjaW5nIHRoZSBjYWxsIHRvIGlycV9vZl9wYXJzZV9hbmRf
bWFwKCkgd2l0aA0KPiBjYWxscw0KPiA+PiA+ICAgIHRvIG9mX2lycV9wYXJzZV9vbmUoKSBhbmQg
aXJxX2NyZWF0ZV9vZl9tYXBwaW5nKCkuDQo+ID4+ID4NCj4gPj4gPiBOb3RlOiBUaGVyZSBhcmUg
YSBmZXcgKmNvZGUqIGxpbmVzIG92ZXIgODAgY2hhcnMsIGJ1dCB0aGlzIGlzIGp1c3QNCj4gZ3Vp
ZGFuY2UsDQo+ID4+ID4gICAgcmlnaHQ/IEVzcGVjaWFsbHkgYXMgdGhlcmUgYXJlIGFscmVhZHkg
c29tZSBsaW5lcyBvdmVyIDgwIGNoYXJzLg0KPiA+PiA+IC0tLQ0KPiA+IFtzbmlwXQ0KPiA+DQo+
ID4+ID4gLSAgICAgICAgICAgICAgIGlmIChoYXNfYWNwaV9jb21wYW5pb24oZGV2KSAmJiBwcC0+
aWR4ID09IDApDQo+ID4+ID4gLSAgICAgICAgICAgICAgICAgICAgICAgcHAtPmlycSA9IHBsYXRm
b3JtX2dldF9pcnEodG9fcGxhdGZvcm1fZGV2aWNlKGRldiksIDApOw0KPiA+PiA+ICsgICAgICAg
ICAgICAgICBpZiAoaGFzX2FjcGlfY29tcGFuaW9uKGRldikgJiYgcHAtPmlkeCA9PSAwKSB7DQo+
ID4+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgcHAtPmlycVswXSA9IHBsYXRmb3JtX2dldF9p
cnEodG9fcGxhdGZvcm1fZGV2aWNlKGRldiksDQo+IDApOw0KPiA+PiA+ICsgICAgICAgICAgICAg
ICAgICAgICAgIGlmIChwcC0+aXJxWzBdKQ0KPiA+PiA+ICsgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgcHAtPmhhc19pcnEgPSB0cnVlOw0KPiA+PiA+ICsgICAgICAgICAgICAgICB9DQo+
ID4+DQo+ID4+IEl0IGRvZXNuJ3Qgd29yayBmb3IgQUNQSS4gQ291bGQgeW91IGRvIHRoZSBzYW1l
IGxvZ2ljIGZvciBBQ1BJPw0KPiA+IEkgZG9u4oCZdCBoYXZlIGFjY2VzcyB0byBhbnkgZGV2aWNl
IHRoYXQgd2FzIGJha2VkIChpLmUuIGZhYmJlZCkgd2l0aA0KPiA+IG11bHRpcGxlIG91dHB1dCBp
bnRlcnJ1cHRzIGZyb20gdGhlIFN5bm9wc3lzIEdQSU8gYmxvY2tzIGFuZCB1c2UgQUNQSS4NCj4g
PiBJIGRvbid0IGtub3cgaWYgYW55IHN1Y2ggZGV2aWNlIGV4aXN0cy4NCj4gDQo+IEJlbG93IGNv
ZGUgaXMgdGVzdGVkIG9uIFgtR2VuZSBzeXN0ZW0gd2hpY2ggc3VwcG9ydHMgMSBpbnRlcnJ1cHQg
cGVyIEdQSU8NCj4gb24gUG9ydCBBLiBZb3UgY2FuIHVwZGF0ZSBpdCBpbnRvIHlvdXIgcGF0Y2gu
DQo+IA0KPiAtICAgICAgICAgICAgICAgaWYgKGhhc19hY3BpX2NvbXBhbmlvbihkZXYpICYmIHBw
LT5pZHggPT0gMCkNCj4gLSAgICAgICAgICAgICAgICAgICAgICAgcHAtPmlycSA9IHBsYXRmb3Jt
X2dldF9pcnEodG9fcGxhdGZvcm1fZGV2aWNlKGRldiksIDApOw0KPiArICAgICAgICAgICAgICAg
aWYgKGhhc19hY3BpX2NvbXBhbmlvbihkZXYpICYmIHBwLT5pZHggPT0gMCkgew0KPiArICAgICAg
ICAgICAgICAgICAgICAgICB1bnNpZ25lZCBpbnQgajsNCj4gKyAgICAgICAgICAgICAgICAgICAg
ICAgZm9yIChqID0gMDsgaiA8IHBwLT5uZ3BpbzsgaisrKSB7DQo+ICsgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgcHAtPmlycVtqXSA9DQo+IHBsYXRmb3JtX2dldF9pcnEodG9fcGxhdGZv
cm1fZGV2aWNlKGRldiksIGopOw0KPiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGlm
IChwcC0+aXJxW2pdKQ0KPiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
cHAtPmhhc19pcnEgPSB0cnVlOw0KPiArICAgICAgICAgICAgICAgICAgICAgICB9DQo+ICsgICAg
ICAgICAgICAgICB9DQpTaW5jZSBJJ3ZlIGFscmVhZHkgZ290IHNvbWUgcmV2aWV3ZWQtYnkgYW5k
IGFja3MgZm9yIHY0LCBJJ2xsIGxlYXZlIGl0IHRvIExpbnVzDQp0byBkZWNpZGUgaWYgaGUgd2Fu
dHMgbWUgdG8gcm9sbCB5b3VyIGNoYW5nZXMgaW50byB0aGlzIHBhdGNoIG9yIGZvciB5b3UgdG8N
CnN1Ym1pdCBhIHNlcGFyYXRlIHBhdGNoLg0KDQpUaGFua3MNClBoaWwNCg0KDQo+ID4+ID4gICAg
ICAgICAgICAgICAgIHBwLT5pcnFfc2hhcmVkICA9IGZhbHNlOw0KPiA+PiA+ICAgICAgICAgICAg
ICAgICBwcC0+Z3Bpb19iYXNlICAgPSAtMTsNCj4gPj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9t
ZmQvaW50ZWxfcXVhcmtfaTJjX2dwaW8uYw0KPiA+PiA+IGIvZHJpdmVycy9tZmQvaW50ZWxfcXVh
cmtfaTJjX2dwaW8uYw0KPiA+PiA+IGluZGV4IDkwZTM1ZGUuLjViZGRiODQgMTAwNjQ0DQo+ID4+
ID4gLS0tIGEvZHJpdmVycy9tZmQvaW50ZWxfcXVhcmtfaTJjX2dwaW8uYw0KPiA+PiA+ICsrKyBi
L2RyaXZlcnMvbWZkL2ludGVsX3F1YXJrX2kyY19ncGlvLmMNCj4gPj4gPiBAQCAtMjMzLDcgKzIz
Myw4IEBAIHN0YXRpYyBpbnQgaW50ZWxfcXVhcmtfZ3Bpb19zZXR1cChzdHJ1Y3QNCj4gPj4gPiBw
Y2lfZGV2DQo+ID4+ICpwZGV2LCBzdHJ1Y3QgbWZkX2NlbGwgKmNlbGwpDQo+ID4+ID4gICAgICAg
ICBwZGF0YS0+cHJvcGVydGllcy0+aWR4ICAgICAgICAgID0gMDsNCj4gPj4gPiAgICAgICAgIHBk
YXRhLT5wcm9wZXJ0aWVzLT5uZ3BpbyAgICAgICAgPSBJTlRFTF9RVUFSS19NRkRfTkdQSU87DQo+
ID4+ID4gICAgICAgICBwZGF0YS0+cHJvcGVydGllcy0+Z3Bpb19iYXNlICAgID0NCj4gSU5URUxf
UVVBUktfTUZEX0dQSU9fQkFTRTsNCj4gPj4gPiAtICAgICAgIHBkYXRhLT5wcm9wZXJ0aWVzLT5p
cnEgICAgICAgICAgPSBwZGV2LT5pcnE7DQo+ID4+ID4gKyAgICAgICBwZGF0YS0+cHJvcGVydGll
cy0+aXJxWzBdICAgICAgID0gcGRldi0+aXJxOw0KPiA+PiA+ICsgICAgICAgcGRhdGEtPnByb3Bl
cnRpZXMtPmhhc19pcnEgICAgICA9IHRydWU7DQo+ID4+ID4gICAgICAgICBwZGF0YS0+cHJvcGVy
dGllcy0+aXJxX3NoYXJlZCAgID0gdHJ1ZTsNCj4gPj4gPg0KPiA+PiA+ICAgICAgICAgY2VsbC0+
cGxhdGZvcm1fZGF0YSA9IHBkYXRhOyBkaWZmIC0tZ2l0DQo+ID4+ID4gYS9pbmNsdWRlL2xpbnV4
L3BsYXRmb3JtX2RhdGEvZ3Bpby1kd2FwYi5oDQo+ID4+ID4gYi9pbmNsdWRlL2xpbnV4L3BsYXRm
b3JtX2RhdGEvZ3Bpby1kd2FwYi5oDQo+ID4+ID4gaW5kZXggMmRjN2Y0YS4uNWE1MmQ2OSAxMDA2
NDQNCj4gPj4gPiAtLS0gYS9pbmNsdWRlL2xpbnV4L3BsYXRmb3JtX2RhdGEvZ3Bpby1kd2FwYi5o
DQo+ID4+ID4gKysrIGIvaW5jbHVkZS9saW51eC9wbGF0Zm9ybV9kYXRhL2dwaW8tZHdhcGIuaA0K
PiA+PiA+IEBAIC0xOSw3ICsxOSw4IEBAIHN0cnVjdCBkd2FwYl9wb3J0X3Byb3BlcnR5IHsNCj4g
Pj4gPiAgICAgICAgIHVuc2lnbmVkIGludCAgICBpZHg7DQo+ID4+ID4gICAgICAgICB1bnNpZ25l
ZCBpbnQgICAgbmdwaW87DQo+ID4+ID4gICAgICAgICB1bnNpZ25lZCBpbnQgICAgZ3Bpb19iYXNl
Ow0KPiA+PiA+IC0gICAgICAgdW5zaWduZWQgaW50ICAgIGlycTsNCj4gPj4gPiArICAgICAgIHVu
c2lnbmVkIGludCAgICBpcnFbMzJdOw0KPiA+PiA+ICsgICAgICAgYm9vbCAgICAgICAgICAgIGhh
c19pcnE7DQo+ID4+ID4gICAgICAgICBib29sICAgICAgICAgICAgaXJxX3NoYXJlZDsNCj4gPj4g
PiAgfTsNCj4gPj4gPg0KPiA+PiA+IC0tDQo+ID4+ID4gMi43LjQNCj4gPj4gPg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hoan Tran April 20, 2018, 6:08 a.m. UTC | #7
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
>> >> >
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij April 26, 2018, 12:58 p.m. UTC | #8
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

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;
 };