Re: [PATCH v1 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sun, May 17, 2020 at 05:26:30PM +0300, Serge Semin wrote:
> On Tue, May 12, 2020 at 09:45:13PM +0300, Andy Shevchenko wrote:
> > has_irq member of struct dwapb_port_property is used only in one place,
> > so, make it local test instead and remove from the structure.

...

> > +	if (memchr_inv(pp->irq, 0, sizeof(pp->irq)) == NULL) {
> > +		dev_warn(gpio->dev, "no IRQ for port%d\n", pp->idx);
> > +		return;
> > +	}
> > +
> 
> Ah, that's why you added the memchr_inv() method in patch 2. So to move it
> to dwapb_configure_irqs() at this point.

Yes.

> Anyway I still think, that it would be
> better to leave the has_irq initialization in the loop there, but here you could
> just remove that assignment.

I think you noticed that I don't like to ping-pong in the series, for what you
propose it would be something like

original --->>>

	if (pp->irq[j] >= 0)
		pp->has_irq = true;

after patch 2 (if your suggestion applied) --->>>

	if (irq > 0) {
		pp->irq[j] = irq;
		pp->has->irq = true;
	}

after this patch --->>>

	if (irq > 0)
		pp->irq[j] = irq;

I prefer not to do this.

OTOH, I guess it might work if we leave original conditional separate to
assignment of IRQ itself (with '>= 0' -> '> 0' being replaced).

I'll look what can I do here.

> For this patch:
> 
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

Thank you!

> I'll test the whole series up when you send v2. Our hardware is equipped with
> two DW APB GPIO IPs with Port A enabled for each. One of them is connected to an
> interrupt controller by a single line.

-- 
With Best Regards,
Andy Shevchenko