| 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 |
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
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 >
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 --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 {
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(-)