| Message ID | 20200415141534.31240-14-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: dwapb: Clean up the driver and a fix | expand |
On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote: > The negative conditionals are harder to parse by reader. > Switch to positive one in dwapb_configure_irqs(). Sorry as for me this modification is redundant. Yes, I know that if-else statement in some cases better to start with positive expression to make it a bit more clear, but in this case I'd leave it as is. First this rule is applicable if both branches are more or less equal, but here I see the most normal case of using the dt-based generic device, which doesn't declare the IRQs as shared seeing it is selected by far more devices at the moment. Second the non-shared IRQs case also covers a combined and multiple-lined GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have only a single IRQ source supported. Finally If the code was like you suggested from the very beginning I wouldn't say a word, but this patch seems to me at least just moving the code around with gaining less than we have at the moment. Linus, Bartosz and other GPIO-ers may think differently though. Lets see their opinion. Regards, -Sergey > > Cc: Serge Semin <fancer.lancer@gmail.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Tested-by: Serge Semin <fancer.lancer@gmail.com> > --- > drivers/gpio/gpio-dwapb.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 31d29ec6ab5c..84c971e0adf0 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -413,15 +413,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH; > irq_gc->chip_types[1].handler = handle_edge_irq; > > - if (!pp->irq_shared) { > - int i; > - > - for (i = 0; i < pp->ngpio; i++) { > - if (pp->irq[i] >= 0) > - irq_set_chained_handler_and_data(pp->irq[i], > - dwapb_irq_handler, gpio); > - } > - } else { > + if (pp->irq_shared) { > /* > * Request a shared IRQ since where MFD would have devices > * using the same irq pin > @@ -435,6 +427,14 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > gpio->domain = NULL; > return; > } > + } else { > + int i; > + > + for (i = 0; i < pp->ngpio; i++) { > + if (pp->irq[i] >= 0) > + irq_set_chained_handler_and_data(pp->irq[i], > + dwapb_irq_handler, gpio); > + } > } > > for (hwirq = 0 ; hwirq < ngpio ; hwirq++) > -- > 2.25.1 >
On Wed, Apr 15, 2020 at 6:37 PM Serge Semin <fancer.lancer@gmail.com> wrote: > On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote: > > The negative conditionals are harder to parse by reader. > > Switch to positive one in dwapb_configure_irqs(). > > Sorry as for me this modification is redundant. Yes, I know that if-else > statement in some cases better to start with positive expression to make it > a bit more clear, but in this case I'd leave it as is. First this rule is > applicable if both branches are more or less equal, but here I see the most > normal case of using the dt-based generic device, which doesn't declare the > IRQs as shared seeing it is selected by far more devices at the moment. > Second the non-shared IRQs case also covers a combined and multiple-lined > GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have > only a single IRQ source supported. Finally If the code was like you > suggested from the very beginning I wouldn't say a word, but this patch seems > to me at least just moving the code around with gaining less than we have at > the moment. > > Linus, Bartosz and other GPIO-ers may think differently though. Lets see their > opinion. I think I already applied all patches with the batch application tool b4, without properly checking which patches you reviewed and not, sorry :( However if any change is controversial I can revert or pull the patch out. Yours, Linus Walleij
On Thu, Apr 16, 2020 at 01:53:25PM +0200, Linus Walleij wrote: > On Wed, Apr 15, 2020 at 6:37 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote: > > > The negative conditionals are harder to parse by reader. > > > Switch to positive one in dwapb_configure_irqs(). > > > > Sorry as for me this modification is redundant. Yes, I know that if-else > > statement in some cases better to start with positive expression to make it > > a bit more clear, but in this case I'd leave it as is. First this rule is > > applicable if both branches are more or less equal, but here I see the most > > normal case of using the dt-based generic device, which doesn't declare the > > IRQs as shared seeing it is selected by far more devices at the moment. > > Second the non-shared IRQs case also covers a combined and multiple-lined > > GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have > > only a single IRQ source supported. Finally If the code was like you > > suggested from the very beginning I wouldn't say a word, but this patch seems > > to me at least just moving the code around with gaining less than we have at > > the moment. > > > > Linus, Bartosz and other GPIO-ers may think differently though. Lets see their > > opinion. > > I think I already applied all patches with the batch application tool b4, > without properly checking which patches you reviewed and not, sorry :( > > However if any change is controversial I can revert or pull the patch out. In this case it's up to you to decide. I was against this patch in the first place. As for me it seemed redundant (see my justification above). But you or Bartosz may think differently that's why I asked your opinion to decide its destiny. So if you are ok with what the patch provides, then there is no need to revert or reset anything. But if you agree with me, then pulling the patch out with resetting the git history will be the best option. Regards, -Sergey > > Yours, > Linus Walleij
On Thu, Apr 16, 2020 at 3:48 PM Serge Semin <fancer.lancer@gmail.com> wrote: > On Thu, Apr 16, 2020 at 01:53:25PM +0200, Linus Walleij wrote: > > On Wed, Apr 15, 2020 at 6:37 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote: > > > > The negative conditionals are harder to parse by reader. > > > > Switch to positive one in dwapb_configure_irqs(). > > > > > > Sorry as for me this modification is redundant. Yes, I know that if-else > > > statement in some cases better to start with positive expression to make it > > > a bit more clear, but in this case I'd leave it as is. First this rule is > > > applicable if both branches are more or less equal, but here I see the most > > > normal case of using the dt-based generic device, which doesn't declare the > > > IRQs as shared seeing it is selected by far more devices at the moment. > > > Second the non-shared IRQs case also covers a combined and multiple-lined > > > GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have > > > only a single IRQ source supported. Finally If the code was like you > > > suggested from the very beginning I wouldn't say a word, but this patch seems > > > to me at least just moving the code around with gaining less than we have at > > > the moment. > > > > > > Linus, Bartosz and other GPIO-ers may think differently though. Lets see their > > > opinion. > > > > I think I already applied all patches with the batch application tool b4, > > without properly checking which patches you reviewed and not, sorry :( > > > > However if any change is controversial I can revert or pull the patch out. > > In this case it's up to you to decide. I backed out the last two patches now and kept the rest except 13 and 14. Yours, Linus Walleij
On Fri, Apr 17, 2020 at 12:42:33PM +0200, Linus Walleij wrote: > On Thu, Apr 16, 2020 at 3:48 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Thu, Apr 16, 2020 at 01:53:25PM +0200, Linus Walleij wrote: > > > On Wed, Apr 15, 2020 at 6:37 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote: > > > > > The negative conditionals are harder to parse by reader. > > > > > Switch to positive one in dwapb_configure_irqs(). > > > > > > > > Sorry as for me this modification is redundant. Yes, I know that if-else > > > > statement in some cases better to start with positive expression to make it > > > > a bit more clear, but in this case I'd leave it as is. First this rule is > > > > applicable if both branches are more or less equal, but here I see the most > > > > normal case of using the dt-based generic device, which doesn't declare the > > > > IRQs as shared seeing it is selected by far more devices at the moment. > > > > Second the non-shared IRQs case also covers a combined and multiple-lined > > > > GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have > > > > only a single IRQ source supported. Finally If the code was like you > > > > suggested from the very beginning I wouldn't say a word, but this patch seems > > > > to me at least just moving the code around with gaining less than we have at > > > > the moment. > > > > > > > > Linus, Bartosz and other GPIO-ers may think differently though. Lets see their > > > > opinion. > > > > > > I think I already applied all patches with the batch application tool b4, > > > without properly checking which patches you reviewed and not, sorry :( > > > > > > However if any change is controversial I can revert or pull the patch out. > > > > In this case it's up to you to decide. > > I backed out the last two patches now and kept the rest except 13 and 14. Fine with me, thanks!
On Fri, Apr 17, 2020 at 12:42:33PM +0200, Linus Walleij wrote: > On Thu, Apr 16, 2020 at 3:48 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Thu, Apr 16, 2020 at 01:53:25PM +0200, Linus Walleij wrote: > > > On Wed, Apr 15, 2020 at 6:37 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote: > > > > > The negative conditionals are harder to parse by reader. > > > > > Switch to positive one in dwapb_configure_irqs(). > > > > > > > > Sorry as for me this modification is redundant. Yes, I know that if-else > > > > statement in some cases better to start with positive expression to make it > > > > a bit more clear, but in this case I'd leave it as is. First this rule is > > > > applicable if both branches are more or less equal, but here I see the most > > > > normal case of using the dt-based generic device, which doesn't declare the > > > > IRQs as shared seeing it is selected by far more devices at the moment. > > > > Second the non-shared IRQs case also covers a combined and multiple-lined > > > > GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have > > > > only a single IRQ source supported. Finally If the code was like you > > > > suggested from the very beginning I wouldn't say a word, but this patch seems > > > > to me at least just moving the code around with gaining less than we have at > > > > the moment. > > > > > > > > Linus, Bartosz and other GPIO-ers may think differently though. Lets see their > > > > opinion. > > > > > > I think I already applied all patches with the batch application tool b4, > > > without properly checking which patches you reviewed and not, sorry :( > > > > > > However if any change is controversial I can revert or pull the patch out. > > > > In this case it's up to you to decide. > > I backed out the last two patches now and kept the rest except 13 and 14. Ok. Thanks. Regards, -Sergey > > Yours, > Linus Walleij
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 31d29ec6ab5c..84c971e0adf0 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -413,15 +413,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH; irq_gc->chip_types[1].handler = handle_edge_irq; - if (!pp->irq_shared) { - int i; - - for (i = 0; i < pp->ngpio; i++) { - if (pp->irq[i] >= 0) - irq_set_chained_handler_and_data(pp->irq[i], - dwapb_irq_handler, gpio); - } - } else { + if (pp->irq_shared) { /* * Request a shared IRQ since where MFD would have devices * using the same irq pin @@ -435,6 +427,14 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, gpio->domain = NULL; return; } + } else { + int i; + + for (i = 0; i < pp->ngpio; i++) { + if (pp->irq[i] >= 0) + irq_set_chained_handler_and_data(pp->irq[i], + dwapb_irq_handler, gpio); + } } for (hwirq = 0 ; hwirq < ngpio ; hwirq++)