diff mbox series

[v1,1/4] gpio: dwapb: Unify ACPI enumeration checks in get_irq() and configure_irqs()

Message ID 20210726125436.58685-1-andriy.shevchenko@linux.intel.com
State New
Headers show
Series [v1,1/4] gpio: dwapb: Unify ACPI enumeration checks in get_irq() and configure_irqs() | expand

Commit Message

Andy Shevchenko July 26, 2021, 12:54 p.m. UTC
Shared IRQ is only enabled for ACPI enumeration, there is no need
to have a special flag for that, since we simple can test if device
has been enumerated by ACPI. This unifies the checks in dwapb_get_irq()
and dwapb_configure_irqs().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-dwapb.c                | 13 ++++++-------
 drivers/mfd/intel_quark_i2c_gpio.c       |  1 -
 include/linux/platform_data/gpio-dwapb.h |  1 -
 3 files changed, 6 insertions(+), 9 deletions(-)

Comments

Lee Jones Aug. 2, 2021, 8:48 a.m. UTC | #1
On Mon, 26 Jul 2021, Andy Shevchenko wrote:

> Shared IRQ is only enabled for ACPI enumeration, there is no need

> to have a special flag for that, since we simple can test if device

> has been enumerated by ACPI. This unifies the checks in dwapb_get_irq()

> and dwapb_configure_irqs().

> 

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---

>  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------


>  drivers/mfd/intel_quark_i2c_gpio.c       |  1 -


Acked-by: Lee Jones <lee.jones@linaro.org>


>  include/linux/platform_data/gpio-dwapb.h |  1 -

>  3 files changed, 6 insertions(+), 9 deletions(-)


-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Serge Semin Aug. 2, 2021, 1:40 p.m. UTC | #2
Hello Andy
Thanks for the cleanup series. A tiny note is below.

On Mon, Jul 26, 2021 at 03:54:33PM +0300, Andy Shevchenko wrote:
> Shared IRQ is only enabled for ACPI enumeration, there is no need

> to have a special flag for that, since we simple can test if device

> has been enumerated by ACPI. This unifies the checks in dwapb_get_irq()

> and dwapb_configure_irqs().

> 

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---

>  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------

>  drivers/mfd/intel_quark_i2c_gpio.c       |  1 -

>  include/linux/platform_data/gpio-dwapb.h |  1 -

>  3 files changed, 6 insertions(+), 9 deletions(-)

> 

> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c

> index 3eb13d6d31ef..f6ae69d5d644 100644

> --- a/drivers/gpio/gpio-dwapb.c

> +++ b/drivers/gpio/gpio-dwapb.c

> @@ -436,12 +436,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,

>  	pirq->irqchip.irq_set_wake = dwapb_irq_set_wake;

>  #endif

>  


> -	if (!pp->irq_shared) {

> -		girq->num_parents = pirq->nr_irqs;

> -		girq->parents = pirq->irq;

> -		girq->parent_handler_data = gpio;

> -		girq->parent_handler = dwapb_irq_handler;

> -	} else {

> +	if (has_acpi_companion(gpio->dev)) {


Before this patch the platform flag irq_shared has been as kind of a
hint regarding the shared IRQ case being covered here. But now it
doesn't seem obvious why we've got the ACPI and ACPI-less cases
differently handled. What about adding a small comment about that?
E.g. like this: "Intel ACPI-based platforms mostly have the DW APB
GPIO IRQ lane shared between several devices. In that case the
parental IRQ has to be handled in the shared way so to be properly
delivered to all the connected devices." or something more detailed
for your preference. After that the rest of the comments in the
if-clause could be discarded.

Other than that, feel free to add:
>  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------

Acked-by: Serge Semin <fancer.lancer@gmail.com>

Tested-by: Serge Semin <fancer.lancer@gmail.com>


-Sergey

>  		/* This will let us handle the parent IRQ in the driver */

>  		girq->num_parents = 0;

>  		girq->parents = NULL;

> @@ -458,6 +453,11 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,

>  			dev_err(gpio->dev, "error requesting IRQ\n");

>  			goto err_kfree_pirq;

>  		}

> +	} else {

> +		girq->num_parents = pirq->nr_irqs;

> +		girq->parents = pirq->irq;

> +		girq->parent_handler_data = gpio;

> +		girq->parent_handler = dwapb_irq_handler;

>  	}

>  

>  	girq->chip = &pirq->irqchip;

> @@ -581,7 +581,6 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)

>  			pp->ngpio = DWAPB_MAX_GPIOS;

>  		}

>  

> -		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 01935ae4e9e1..a43993e38b6e 100644

> --- a/drivers/mfd/intel_quark_i2c_gpio.c

> +++ b/drivers/mfd/intel_quark_i2c_gpio.c

> @@ -227,7 +227,6 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev)

>  	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;

>  	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;

>  	pdata->properties->irq[0]	= pci_irq_vector(pdev, 0);

> -	pdata->properties->irq_shared	= true;

>  

>  	cell->platform_data = pdata;

>  	cell->pdata_size = sizeof(*pdata);

> diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h

> index 0aa5c6720259..535e5ed549d9 100644

> --- a/include/linux/platform_data/gpio-dwapb.h

> +++ b/include/linux/platform_data/gpio-dwapb.h

> @@ -14,7 +14,6 @@ struct dwapb_port_property {

>  	unsigned int	ngpio;

>  	unsigned int	gpio_base;

>  	int		irq[DWAPB_MAX_GPIOS];

> -	bool		irq_shared;

>  };

>  

>  struct dwapb_platform_data {

> -- 

> 2.30.2

>
Andy Shevchenko Aug. 2, 2021, 6:37 p.m. UTC | #3
On Mon, Aug 02, 2021 at 04:40:21PM +0300, Serge Semin wrote:
> Hello Andy

> Thanks for the cleanup series. A tiny note is below.


Thanks for review!

> On Mon, Jul 26, 2021 at 03:54:33PM +0300, Andy Shevchenko wrote:

> > Shared IRQ is only enabled for ACPI enumeration, there is no need

> > to have a special flag for that, since we simple can test if device

> > has been enumerated by ACPI. This unifies the checks in dwapb_get_irq()

> > and dwapb_configure_irqs().

> > 

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> > ---

> >  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------

> >  drivers/mfd/intel_quark_i2c_gpio.c       |  1 -

> >  include/linux/platform_data/gpio-dwapb.h |  1 -

> >  3 files changed, 6 insertions(+), 9 deletions(-)

> > 

> > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c

> > index 3eb13d6d31ef..f6ae69d5d644 100644

> > --- a/drivers/gpio/gpio-dwapb.c

> > +++ b/drivers/gpio/gpio-dwapb.c

> > @@ -436,12 +436,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,

> >  	pirq->irqchip.irq_set_wake = dwapb_irq_set_wake;

> >  #endif

> >  

> 

> > -	if (!pp->irq_shared) {

> > -		girq->num_parents = pirq->nr_irqs;

> > -		girq->parents = pirq->irq;

> > -		girq->parent_handler_data = gpio;

> > -		girq->parent_handler = dwapb_irq_handler;

> > -	} else {

> > +	if (has_acpi_companion(gpio->dev)) {

> 

> Before this patch the platform flag irq_shared has been as kind of a

> hint regarding the shared IRQ case being covered here. But now it

> doesn't seem obvious why we've got the ACPI and ACPI-less cases

> differently handled. What about adding a small comment about that?

> E.g. like this: "Intel ACPI-based platforms mostly have the DW APB

> GPIO IRQ lane shared between several devices. In that case the

> parental IRQ has to be handled in the shared way so to be properly

> delivered to all the connected devices." or something more detailed

> for your preference. After that the rest of the comments in the

> if-clause could be discarded.


Sure!


-- 
With Best Regards,
Andy Shevchenko
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 3eb13d6d31ef..f6ae69d5d644 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -436,12 +436,7 @@  static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	pirq->irqchip.irq_set_wake = dwapb_irq_set_wake;
 #endif
 
-	if (!pp->irq_shared) {
-		girq->num_parents = pirq->nr_irqs;
-		girq->parents = pirq->irq;
-		girq->parent_handler_data = gpio;
-		girq->parent_handler = dwapb_irq_handler;
-	} else {
+	if (has_acpi_companion(gpio->dev)) {
 		/* This will let us handle the parent IRQ in the driver */
 		girq->num_parents = 0;
 		girq->parents = NULL;
@@ -458,6 +453,11 @@  static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 			dev_err(gpio->dev, "error requesting IRQ\n");
 			goto err_kfree_pirq;
 		}
+	} else {
+		girq->num_parents = pirq->nr_irqs;
+		girq->parents = pirq->irq;
+		girq->parent_handler_data = gpio;
+		girq->parent_handler = dwapb_irq_handler;
 	}
 
 	girq->chip = &pirq->irqchip;
@@ -581,7 +581,6 @@  static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 			pp->ngpio = DWAPB_MAX_GPIOS;
 		}
 
-		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 01935ae4e9e1..a43993e38b6e 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -227,7 +227,6 @@  static int intel_quark_gpio_setup(struct pci_dev *pdev)
 	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
 	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
 	pdata->properties->irq[0]	= pci_irq_vector(pdev, 0);
-	pdata->properties->irq_shared	= true;
 
 	cell->platform_data = pdata;
 	cell->pdata_size = sizeof(*pdata);
diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
index 0aa5c6720259..535e5ed549d9 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -14,7 +14,6 @@  struct dwapb_port_property {
 	unsigned int	ngpio;
 	unsigned int	gpio_base;
 	int		irq[DWAPB_MAX_GPIOS];
-	bool		irq_shared;
 };
 
 struct dwapb_platform_data {