Re: [PATCH v2 13/14] gpio: dwapb: Use positive conditional in dwapb_configure_irqs()

[Serge Semin <fancer.lancer@xxxxxxxxx>]

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@xxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Tested-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> ---
>  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
>