Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Sep 2022 13:01:32 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Jonathan =?iso-8859-1?Q?Neusch=E4fer?= <j.neuschaefer@gmx.net>
Cc:     Linus Walleij <linus.walleij@linaro.org>, openbmc@lists.ozlabs.org,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 1/1] pinctrl: wpcm450: Correct the fwnode_irq_get()
 return value check
Message-ID: <Yxm9fB/5IJS3MXGu@smile.fi.intel.com>
References: <20220905191408.73794-1-andriy.shevchenko@linux.intel.com>
 <YxkHaBKtiO9zVuKM@probook>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <YxkHaBKtiO9zVuKM@probook>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Wed, Sep 07, 2022 at 11:04:40PM +0200, Jonathan Neusch?fer wrote:
> On Mon, Sep 05, 2022 at 10:14:08PM +0300, Andy Shevchenko wrote:
> > fwnode_irq_get() may return all possible signed values, such as Linux
> > error code. Fix the code to handle this properly.
> 
> It would be good to note explicitly here what a return value of zero
> means, i.e., as the documentation of of_irq_get says, "IRQ mapping
> failure", and why it should result in skipping this IRQ.

Not that I'm fun of duplicating documentation in the commit message,
but it won't take much in this case.

...

> >  		for (i = 0; i < WPCM450_NUM_GPIO_IRQS; i++) {
> > -			int irq = fwnode_irq_get(child, i);
> > +			int irq;
> >  
> > +			irq = fwnode_irq_get(child, i);

> (Unneccesary churn, but I'll allow it)

The point here is to see that we actually check something that we just got
from somewhere else. It's slightly better for reading and maintaining the
code as I explained in [1].

And there is a difference to the cases like

static int foo(struct platform_device *pdev, ...)
{
	struct device *dev = &pdev->dev;
	...
}

when we know ahead that if pdev is NULL, something is _so_ wrong that
it's not even our issue.

[1]: https://lore.kernel.org/lkml/CAHp75Vda5KX5pVrNeueQEODoEy405eTb9SYJtts-Lm9jMNocHQ@mail.gmail.com/

> >  			if (irq < 0)
> >  				break;
> > +			if (!irq)
> > +				continue;
> 
> Since irq == 0 seems to be an error condition, the following seems fine
> to me, and simpler:
> 
> -			if (irq < 0)
> +			if (irq <= 0)
>  				break;

Not sure it's the same by two reasons:
 1) break != continue;
 2) we might need to convert 0 to error if we ever go to report this

So, to me mapping error shouldn't be fatal to continue, but I would
like to hear your interpretation since you know this case much better
than me.

Thanks for the review!

-- 
With Best Regards,
Andy Shevchenko