Hello,
On Thu, Sep 08, 2022 at 01:01:32PM +0300, Andy Shevchenko wrote:
> 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.
My problem with the description is that handling "all possible signed
values" is fairly meaningless: The code arguably did that already, it
did *something* for every possible value. The significant change of
your patch is that the value zero is handled differently.
IOW, what I miss is something along the lines of: "fwnode_irq_get can
return zero to indicate some errors. Handle this case like other errors."
> ...
>
> > > 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@xxxxxxxxxxxxxx/
Ok, fair enough.
>
> > > 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;
Right, hence why I asked for reasoning why zero should be handled
the way you propose to handle it.
> 2) we might need to convert 0 to error if we ever go to report this
Good point.
>
> 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.
However: In wpcm450_gpio_register, there is currently no reporting for
mapping errors in this loop.
I'm fine with either break or continue.
Thanks,
Jonathan
Attachment:
signature.asc
Description: PGP signature
