| Message ID | 20210601162128.35663-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 5a5bc826fed1d86212eb78114c86808c29b11b0a |
| Headers | show |
| Series | [v1,1/2] gpio: dwapb: Drop redundant check in dwapb_irq_set_type() | expand |
On Tue, Jun 1, 2021 at 6:21 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > For more than 15 years we may not get into ->irq_set_type() > without any meaningful type provided. > > Drop redundant check in dwapb_irq_set_type(). > > See the commit e76de9f8eb67 ("[PATCH] genirq: add SA_TRIGGER support") > out of curiosity. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpio-dwapb.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index d3233cc4b76b..939701c1465e 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -297,9 +297,6 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type) > irq_hw_number_t bit = irqd_to_hwirq(d); > unsigned long level, polarity, flags; > > - if (type & ~IRQ_TYPE_SENSE_MASK) > - return -EINVAL; > - > spin_lock_irqsave(&gc->bgpio_lock, flags); > level = dwapb_read(gpio, GPIO_INTTYPE_LEVEL); > polarity = dwapb_read(gpio, GPIO_INT_POLARITY); > -- > 2.30.2 > Applied, thanks! Bart
On Tue, Jun 1, 2021 at 6:21 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > We have open coded variant of fwnode_irq_get() in dwapb_get_irq(). > Replace it with a simple call. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpio-dwapb.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 939701c1465e..7d61f5821e32 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -528,17 +528,13 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode, > struct dwapb_port_property *pp) > { > - struct device_node *np = NULL; > - int irq = -ENXIO, j; > - > - if (fwnode_property_read_bool(fwnode, "interrupt-controller")) > - np = to_of_node(fwnode); > + int irq, j; > > for (j = 0; j < pp->ngpio; j++) { > - if (np) > - irq = of_irq_get(np, j); > - else if (has_acpi_companion(dev)) > + if (has_acpi_companion(dev)) > irq = platform_get_irq_optional(to_platform_device(dev), j); > + else > + irq = fwnode_irq_get(fwnode, j); > if (irq > 0) > pp->irq[j] = irq; > } > -- > 2.30.2 > Applied, thanks! Bart
Hello Andy On Tue, Jun 01, 2021 at 07:21:28PM +0300, Andy Shevchenko wrote: > We have open coded variant of fwnode_irq_get() in dwapb_get_irq(). > Replace it with a simple call. Sometime ago I was trying to figure out a way to simplify this part of the driver by using the platform_get_irq_optional() method for both ACPI and OF cases. As you must have already found out by yourself it didn't work out because of DW APB GPIO DT-nodes are supposed to have sub-nodes with ports description. The OF-descriptors of these sub-nodes aren't touched by the platform_get_irq_optional() method, it just fails to detect IRQ-controller because it only works with the device OF-node. So I gave up and decided to leave the code as is. I can't remember now why I haven't used fwnode_irq_get() here. Most likely I just preferred a direct of_irq_get() invocation here just for clarity, since the only way we'd be calling fwnode_irq_get() here is to actually get IRQ number from the OF-node anyway, while the acpi_irq_get() method call made from the method fwnode_irq_get() will hardly ever be required here. If you think otherwise or I missing something please tell me. Anyway by applying your patch at least we'll save a few lines of the code and may in future have swnode-base IRQs support in the fwnode_irq_get() method. So it still worths merging in. Thanks for suggesting this change. Acked-by: Serge Semin <fancer.lancer@gmail.com> Tested-by: Serge Semin <fancer.lancer@gmail.com> -Sergey > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpio-dwapb.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 939701c1465e..7d61f5821e32 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -528,17 +528,13 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode, > struct dwapb_port_property *pp) > { > - struct device_node *np = NULL; > - int irq = -ENXIO, j; > - > - if (fwnode_property_read_bool(fwnode, "interrupt-controller")) > - np = to_of_node(fwnode); > + int irq, j; > > for (j = 0; j < pp->ngpio; j++) { > - if (np) > - irq = of_irq_get(np, j); > - else if (has_acpi_companion(dev)) > + if (has_acpi_companion(dev)) > irq = platform_get_irq_optional(to_platform_device(dev), j); > + else > + irq = fwnode_irq_get(fwnode, j); > if (irq > 0) > pp->irq[j] = irq; > } > -- > 2.30.2 >
On Tue, Jun 01, 2021 at 07:21:27PM +0300, Andy Shevchenko wrote: > For more than 15 years we may not get into ->irq_set_type() > without any meaningful type provided. > > Drop redundant check in dwapb_irq_set_type(). > > See the commit e76de9f8eb67 ("[PATCH] genirq: add SA_TRIGGER support") > out of curiosity. Why not. Thanks for the patch. Acked-by: Serge Semin <fancer.lancer@gmail.com> -Sergey > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpio-dwapb.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index d3233cc4b76b..939701c1465e 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -297,9 +297,6 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type) > irq_hw_number_t bit = irqd_to_hwirq(d); > unsigned long level, polarity, flags; > > - if (type & ~IRQ_TYPE_SENSE_MASK) > - return -EINVAL; > - > spin_lock_irqsave(&gc->bgpio_lock, flags); > level = dwapb_read(gpio, GPIO_INTTYPE_LEVEL); > polarity = dwapb_read(gpio, GPIO_INT_POLARITY); > -- > 2.30.2 >
On Wed, Jun 02, 2021 at 05:05:46PM +0300, Serge Semin wrote: > On Tue, Jun 01, 2021 at 07:21:28PM +0300, Andy Shevchenko wrote: > > We have open coded variant of fwnode_irq_get() in dwapb_get_irq(). > > Replace it with a simple call. > > Sometime ago I was trying to figure out a way to simplify this part of > the driver by using the platform_get_irq_optional() method for both > ACPI and OF cases. As you must have already found out by yourself it > didn't work out because of DW APB GPIO DT-nodes are supposed to have > sub-nodes with ports description. The OF-descriptors of these > sub-nodes aren't touched by the platform_get_irq_optional() method, it > just fails to detect IRQ-controller because it only works with the > device OF-node. So I gave up and decided to leave the code as is. I > can't remember now why I haven't used fwnode_irq_get() here. Most > likely I just preferred a direct of_irq_get() invocation here just for > clarity, since the only way we'd be calling fwnode_irq_get() here is > to actually get IRQ number from the OF-node anyway, while the > acpi_irq_get() method call made from the method fwnode_irq_get() will > hardly ever be required here. If you think otherwise or I missing > something please tell me. > > Anyway by applying your patch at least we'll save a few lines of the > code and may in future have swnode-base IRQs support in the > fwnode_irq_get() method. So it still worths merging in. Thanks for > suggesting this change. > > Acked-by: Serge Semin <fancer.lancer@gmail.com> > Tested-by: Serge Semin <fancer.lancer@gmail.com> Thanks for testing! -- With Best Regards, Andy Shevchenko
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index d3233cc4b76b..939701c1465e 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -297,9 +297,6 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type) irq_hw_number_t bit = irqd_to_hwirq(d); unsigned long level, polarity, flags; - if (type & ~IRQ_TYPE_SENSE_MASK) - return -EINVAL; - spin_lock_irqsave(&gc->bgpio_lock, flags); level = dwapb_read(gpio, GPIO_INTTYPE_LEVEL); polarity = dwapb_read(gpio, GPIO_INT_POLARITY);
For more than 15 years we may not get into ->irq_set_type() without any meaningful type provided. Drop redundant check in dwapb_irq_set_type(). See the commit e76de9f8eb67 ("[PATCH] genirq: add SA_TRIGGER support") out of curiosity. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpio-dwapb.c | 3 --- 1 file changed, 3 deletions(-)