| Message ID | 20200415141534.31240-15-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:34PM +0300, Andy Shevchenko wrote: > In some cases indentation makes code harder to read. Amend indentation > in those cases despite of lines go a bit over 80 character limit. > > 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 | 24 +++++++++--------------- > 1 file changed, 9 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 84c971e0adf0..a09332d9c7fe 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -437,7 +437,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > } > } > > - for (hwirq = 0 ; hwirq < ngpio ; hwirq++) > + for (hwirq = 0; hwirq < ngpio; hwirq++) > irq_create_mapping(gpio->domain, hwirq); > > port->gc.to_irq = dwapb_gpio_to_irq; > @@ -453,7 +453,7 @@ static void dwapb_irq_teardown(struct dwapb_gpio *gpio) > if (!gpio->domain) > return; > > - for (hwirq = 0 ; hwirq < ngpio ; hwirq++) > + for (hwirq = 0; hwirq < ngpio; hwirq++) > irq_dispose_mapping(irq_find_mapping(gpio->domain, hwirq)); > > irq_domain_remove(gpio->domain); > @@ -478,10 +478,9 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > return -ENOMEM; > #endif > > - dat = gpio->regs + GPIO_EXT_PORTA + (pp->idx * GPIO_EXT_PORT_STRIDE); > - set = gpio->regs + GPIO_SWPORTA_DR + (pp->idx * GPIO_SWPORT_DR_STRIDE); > - dirout = gpio->regs + GPIO_SWPORTA_DDR + > - (pp->idx * GPIO_SWPORT_DDR_STRIDE); > + dat = gpio->regs + GPIO_EXT_PORTA + pp->idx * GPIO_EXT_PORT_STRIDE; > + set = gpio->regs + GPIO_SWPORTA_DR + pp->idx * GPIO_SWPORT_DR_STRIDE; > + dirout = gpio->regs + GPIO_SWPORTA_DDR + pp->idx * GPIO_SWPORT_DDR_STRIDE; > > /* This registers 32 GPIO lines per port */ > err = bgpio_init(&port->gc, gpio->dev, 4, dat, set, NULL, dirout, > @@ -582,17 +581,13 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev) > > if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) || > pp->idx >= DWAPB_MAX_PORTS) { > - dev_err(dev, > - "missing/invalid port index for port%d\n", i); > + dev_err(dev, "missing/invalid port index for port%d\n", i); What about shortening the message text to fit the 80 chars per line rule? I suppose the "missing" word could be omitted. > fwnode_handle_put(fwnode); > return ERR_PTR(-EINVAL); > } > > - if (fwnode_property_read_u32(fwnode, "snps,nr-gpios", > - &pp->ngpio)) { > - dev_info(dev, > - "failed to get number of gpios for port%d\n", > - i); > + if (fwnode_property_read_u32(fwnode, "snps,nr-gpios", &pp->ngpio)) { > + dev_info(dev, "failed to get number of gpios for port%d\n", i); The same here. For instance "no snps,nr-gpios found for port%d" would work here. > pp->ngpio = 32; > } > > @@ -743,8 +738,7 @@ static int dwapb_gpio_suspend(struct device *dev) > ctx->int_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE); > > /* Mask out interrupts */ > - dwapb_write(gpio, GPIO_INTMASK, > - 0xffffffff & ~ctx->wake_en); > + dwapb_write(gpio, GPIO_INTMASK, 0xffffffff & ~ctx->wake_en); Hm, do I need some rest and missing something or the &-operation with 1s here does nothing seeing the operands data types have the same width? (the change introduced by commit 6437c7ba69c3 ("gpio: dwapb: Add wakeup source support")) -Sergey > } > } > spin_unlock_irqrestore(&gc->bgpio_lock, flags); > -- > 2.25.1 >
On Wed, Apr 15, 2020 at 08:15:16PM +0300, Serge Semin wrote: > On Wed, Apr 15, 2020 at 05:15:34PM +0300, Andy Shevchenko wrote: > > In some cases indentation makes code harder to read. Amend indentation > > in those cases despite of lines go a bit over 80 character limit. > > + dev_err(dev, "missing/invalid port index for port%d\n", i); > > What about shortening the message text to fit the 80 chars per line rule? > I suppose the "missing" word could be omitted. More likely port is not needed, but I think this kind of changes are material for another (logically separated) patch. ... > > /* Mask out interrupts */ > > - dwapb_write(gpio, GPIO_INTMASK, > > - 0xffffffff & ~ctx->wake_en); > > > + dwapb_write(gpio, GPIO_INTMASK, 0xffffffff & ~ctx->wake_en); > > Hm, do I need some rest and missing something or the &-operation with 1s here > does nothing seeing the operands data types have the same width? > > (the change introduced by commit 6437c7ba69c3 ("gpio: dwapb: Add wakeup source support")) No, you are right, it seems no-op to me, I have noticed it as well, but I think we may improve this by separate change (as you seems also prefer not to mix logically different changes in one patch).
On Thu, Apr 16, 2020 at 01:56:14PM +0300, Andy Shevchenko wrote: > On Wed, Apr 15, 2020 at 08:15:16PM +0300, Serge Semin wrote: > > On Wed, Apr 15, 2020 at 05:15:34PM +0300, Andy Shevchenko wrote: > > > In some cases indentation makes code harder to read. Amend indentation > > > in those cases despite of lines go a bit over 80 character limit. > > > > + dev_err(dev, "missing/invalid port index for port%d\n", i); > > > > What about shortening the message text to fit the 80 chars per line rule? > > I suppose the "missing" word could be omitted. > > More likely port is not needed, but I think this kind of changes are material > for another (logically separated) patch. > > ... > > > > /* Mask out interrupts */ > > > - dwapb_write(gpio, GPIO_INTMASK, > > > - 0xffffffff & ~ctx->wake_en); > > > > > + dwapb_write(gpio, GPIO_INTMASK, 0xffffffff & ~ctx->wake_en); > > > > Hm, do I need some rest and missing something or the &-operation with 1s here > > does nothing seeing the operands data types have the same width? > > > > (the change introduced by commit 6437c7ba69c3 ("gpio: dwapb: Add wakeup source support")) > > No, you are right, it seems no-op to me, I have noticed it as well, but I think > we may improve this by separate change (as you seems also prefer not to mix > logically different changes in one patch). Ah, Linus already pulled the series in. Next time then.) Regards, -Sergey > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Apr 16, 2020 at 1:06 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> Ah, Linus already pulled the series in. Next time then.)
Yeah sorry, I was a bit stressed by a big mail backlog and also a bit
infatuated with my new b4 tool.
Is it fine to fix any remaining issues with extra patches?
Yours,
Linus Walleij
On Thu, Apr 16, 2020 at 02:14:10PM +0200, Linus Walleij wrote: > On Thu, Apr 16, 2020 at 1:06 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > Ah, Linus already pulled the series in. Next time then.) > > Yeah sorry, I was a bit stressed by a big mail backlog and also a bit > infatuated with my new b4 tool. > > Is it fine to fix any remaining issues with extra patches? I see. No worries. Andy did a good work fixing the indentations. But that caused the 80 chars line rule violation in some cases. The best way would be to avoid the rule violation in the first place, though sometimes it's just impossible without weakening the code readability. I suggested to fix some of the issues by reducing the error messages length and in another case just to remove the no-op &-operation. So If there were following up patches with fixes it would have been great. Though since we have got a violation for several chars in just a few lines of code, we can leave with that for now. So if Andy doesn't have a time to send the followup patches, I'll do this sometime later in the framework of my next patchset. Regards, -Sergey > > Yours, > Linus Walleij
On Thu, Apr 16, 2020 at 3:37 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> caused the 80 chars line rule violation in some cases.
As subsystem maintainer that's a coding style thing I don't
really care much about, I personally violate it all the time.
But you can have it any way you want of course :)
Yours,
Linus Walleij
On Thu, Apr 16, 2020 at 04:37:37PM +0300, Serge Semin wrote: > On Thu, Apr 16, 2020 at 02:14:10PM +0200, Linus Walleij wrote: > > On Thu, Apr 16, 2020 at 1:06 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > Ah, Linus already pulled the series in. Next time then.) > > > > Yeah sorry, I was a bit stressed by a big mail backlog and also a bit > > infatuated with my new b4 tool. > > > > Is it fine to fix any remaining issues with extra patches? > > I see. No worries. Andy did a good work fixing the indentations. But that > caused the 80 chars line rule violation in some cases. The best way would > be to avoid the rule violation in the first place, though sometimes it's > just impossible without weakening the code readability. I suggested to fix > some of the issues by reducing the error messages length and in another > case just to remove the no-op &-operation. So If there were following up > patches with fixes it would have been great. Though since we have got a > violation for several chars in just a few lines of code, we can leave > with that for now. So if Andy doesn't have a time to send the followup > patches, I'll do this sometime later in the framework of my next patchset. I see an immutable branch with this series, so, it means it will be easier to all of us to move on from this point now.
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 84c971e0adf0..a09332d9c7fe 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -437,7 +437,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, } } - for (hwirq = 0 ; hwirq < ngpio ; hwirq++) + for (hwirq = 0; hwirq < ngpio; hwirq++) irq_create_mapping(gpio->domain, hwirq); port->gc.to_irq = dwapb_gpio_to_irq; @@ -453,7 +453,7 @@ static void dwapb_irq_teardown(struct dwapb_gpio *gpio) if (!gpio->domain) return; - for (hwirq = 0 ; hwirq < ngpio ; hwirq++) + for (hwirq = 0; hwirq < ngpio; hwirq++) irq_dispose_mapping(irq_find_mapping(gpio->domain, hwirq)); irq_domain_remove(gpio->domain); @@ -478,10 +478,9 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, return -ENOMEM; #endif - dat = gpio->regs + GPIO_EXT_PORTA + (pp->idx * GPIO_EXT_PORT_STRIDE); - set = gpio->regs + GPIO_SWPORTA_DR + (pp->idx * GPIO_SWPORT_DR_STRIDE); - dirout = gpio->regs + GPIO_SWPORTA_DDR + - (pp->idx * GPIO_SWPORT_DDR_STRIDE); + dat = gpio->regs + GPIO_EXT_PORTA + pp->idx * GPIO_EXT_PORT_STRIDE; + set = gpio->regs + GPIO_SWPORTA_DR + pp->idx * GPIO_SWPORT_DR_STRIDE; + dirout = gpio->regs + GPIO_SWPORTA_DDR + pp->idx * GPIO_SWPORT_DDR_STRIDE; /* This registers 32 GPIO lines per port */ err = bgpio_init(&port->gc, gpio->dev, 4, dat, set, NULL, dirout, @@ -582,17 +581,13 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev) if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) || pp->idx >= DWAPB_MAX_PORTS) { - dev_err(dev, - "missing/invalid port index for port%d\n", i); + dev_err(dev, "missing/invalid port index for port%d\n", i); fwnode_handle_put(fwnode); return ERR_PTR(-EINVAL); } - if (fwnode_property_read_u32(fwnode, "snps,nr-gpios", - &pp->ngpio)) { - dev_info(dev, - "failed to get number of gpios for port%d\n", - i); + if (fwnode_property_read_u32(fwnode, "snps,nr-gpios", &pp->ngpio)) { + dev_info(dev, "failed to get number of gpios for port%d\n", i); pp->ngpio = 32; } @@ -743,8 +738,7 @@ static int dwapb_gpio_suspend(struct device *dev) ctx->int_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE); /* Mask out interrupts */ - dwapb_write(gpio, GPIO_INTMASK, - 0xffffffff & ~ctx->wake_en); + dwapb_write(gpio, GPIO_INTMASK, 0xffffffff & ~ctx->wake_en); } } spin_unlock_irqrestore(&gc->bgpio_lock, flags);