From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1754613AbcHARKS (ORCPT <rfc822;w@1wt.eu>);
	Mon, 1 Aug 2016 13:10:18 -0400
Received: from Galois.linutronix.de ([146.0.238.70]:56513 "EHLO
	Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754575AbcHARJ6 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 1 Aug 2016 13:09:58 -0400
Date: Mon, 1 Aug 2016 19:07:37 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Sebastian Frias <sf84@laposte.net>
cc: Grant Likely <grant.likely@secretlab.ca>,
        Marc Zyngier <marc.zyngier@arm.com>,
        Jason Cooper <jason@lakedaemon.net>,
        LKML <linux-kernel@vger.kernel.org>, Mason <slash.tmp@free.fr>
Subject: Re: [PATCH v2] irqdomain: factorise irq_domain_xlate_onetwocell()
In-Reply-To: <579F635E.6090505@laposte.net>
Message-ID: <alpine.DEB.2.11.1608011903220.19845@nanos>
References: <579F5C3B.5030805@laposte.net> <579F635E.6090505@laposte.net>
User-Agent: Alpine 2.11 (DEB 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, 1 Aug 2016, Sebastian Frias wrote:
> Commit 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
> drivers can use") introduced three similar functions:
> 
> irq_domain_xlate_onecell()
> irq_domain_xlate_twocell()
> irq_domain_xlate_onetwocell()
> 
> yet the last one, irq_domain_xlate_onetwocell(), can be factored to use the
> two previous ones to avoid code duplication.
> 
> Fixes: 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
> drivers can use")

That does not fix anything. It optimizes code. We use the "Fixes" tag only
when the existing code is buggy.

> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> ---
> 
> NOTE: the factored code is not strictly the same in the sense that
> 16b2e6e2f31d returns "intspec[1]" as 'out_type', while this patch would
> make it return "intspec[1] & IRQ_TYPE_SENSE_MASK".

So the proper way to do that is to split this into two patches:

 #1 Change the existing code to do the masking and explain why it is correct.

 #2 Refactor the code and get rid of the duplicated implementation.

 
> Feel free to comment on that matter.
> 
> ---
>  kernel/irq/irqdomain.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index bee8b02..125a28c 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -839,9 +839,12 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
>  {
>  	if (WARN_ON(intsize < 1))
>  		return -EINVAL;
> -	*out_hwirq = intspec[0];
> -	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
> -	return 0;
> +	if (intsize == 1)
> +		return irq_domain_xlate_onecell(d, ctrlr, intspec, intsize,
> +						out_hwirq, out_type);
> +	else
> +		return irq_domain_xlate_twocell(d, ctrlr, intspec, intsize,
> +						out_hwirq, out_type);

So I really wonder how much of a saving that change is. I wouldn't be
surprised if it would create worse code on some architectures.

Thanks,

	tglx