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

[Serge Semin <fancer.lancer@xxxxxxxxx>]

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.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Cc: Serge Semin <fancer.lancer@xxxxxxxxx>
> ---
>  drivers/gpio/gpio-dwapb.c                | 12 +++++++-----
>  include/linux/platform_data/gpio-dwapb.h |  1 -
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 2975f2d369874a..87834adccc4534 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -366,6 +366,11 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  	irq_hw_number_t hwirq;
>  	int err, i;
>  

> +	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. 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. For this patch:

Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>

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.

-Sergey

>  	gpio->domain = irq_domain_create_linear(fwnode, ngpio,
>  						 &irq_generic_chip_ops, gpio);
>  	if (!gpio->domain)
> @@ -501,7 +506,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>  	if (pp->idx == 0)
>  		port->gc.set_config = dwapb_gpio_set_config;
>  
> -	if (pp->has_irq)
> +	/* Only port A can provide interrupts in all configurations of the IP */
> +	if (pp->idx == 0)
>  		dwapb_configure_irqs(gpio, port, pp);
>  
>  	err = gpiochip_add_data(&port->gc, port);
> @@ -553,10 +559,6 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
>  
>  		pp->irq[j] = err;
>  	}
> -
> -	pp->has_irq = memchr_inv(pp->irq, 0, sizeof(pp->irq)) != NULL;
> -	if (!pp->has_irq)
> -		dev_warn(dev, "no irq for port%d\n", pp->idx);
>  }
>  
>  static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
> diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
> index 3c606c450d0596..ff1be737bad6aa 100644
> --- a/include/linux/platform_data/gpio-dwapb.h
> +++ b/include/linux/platform_data/gpio-dwapb.h
> @@ -12,7 +12,6 @@ struct dwapb_port_property {
>  	unsigned int	ngpio;
>  	unsigned int	gpio_base;
>  	int		irq[32];
> -	bool		has_irq;
>  	bool		irq_shared;
>  };
>  
> -- 
> 2.26.2
>