| Message ID | CACRpkdZLSRQfUfc4d=uO0CtH3svN+a17HLbEQYtdy8kMAS_sUA@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Linus, On Mon, May 2, 2016 at 11:06 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, May 2, 2016 at 11:04 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > >> [silly response deleted] >> >> Scrap it. > > :D > >> The only annoying thing is that 0 cannot easily be propagated upstream as >> an error code, so it has to be tested for explicitly. > > Well with the Big Penguin's clear opinion on the matter there is not > much we can do. > > What about this? > > diff --git a/drivers/tty/serial/serial_mctrl_gpio.c > b/drivers/tty/serial/serial_mctrl_gpio.c > index 02147361eaa9..2297ec781681 100644 > --- a/drivers/tty/serial/serial_mctrl_gpio.c > +++ b/drivers/tty/serial/serial_mctrl_gpio.c > @@ -172,6 +172,13 @@ struct mctrl_gpios *mctrl_gpio_init(struct > uart_port *port, unsigned int idx) > dev_err(port->dev, > "failed to find corresponding irq for > %s (idx=%d, err=%d)\n", > mctrl_gpios_desc[i].name, idx, ret); > + /* > + * Satisfy the error code semantics for a missing IRQ, > + * 0 means NO_IRQ, but the framework needs to return > + * a negative to deal with the error. > + */ > + if (!ret) > + ret = -ENOSYS; No, not -ENOSYS, as that triggers my initial problem again (please read the deleted silly response ;-) Checkpatch says: "ENOSYS means 'invalid syscall nr' and nothing else". -EINVAL? > return ERR_PTR(ret); > } > gpios->irq[i] = ret; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, May 2, 2016 at 11:10 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, May 2, 2016 at 11:06 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Mon, May 2, 2016 at 11:04 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >> >>> [silly response deleted] >>> >>> Scrap it. >> >> :D >> >>> The only annoying thing is that 0 cannot easily be propagated upstream as >>> an error code, so it has to be tested for explicitly. >> >> Well with the Big Penguin's clear opinion on the matter there is not >> much we can do. >> >> What about this? >> >> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c >> b/drivers/tty/serial/serial_mctrl_gpio.c >> index 02147361eaa9..2297ec781681 100644 >> --- a/drivers/tty/serial/serial_mctrl_gpio.c >> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >> @@ -172,6 +172,13 @@ struct mctrl_gpios *mctrl_gpio_init(struct >> uart_port *port, unsigned int idx) >> dev_err(port->dev, >> "failed to find corresponding irq for >> %s (idx=%d, err=%d)\n", >> mctrl_gpios_desc[i].name, idx, ret); >> + /* >> + * Satisfy the error code semantics for a missing IRQ, >> + * 0 means NO_IRQ, but the framework needs to return >> + * a negative to deal with the error. >> + */ >> + if (!ret) >> + ret = -ENOSYS; > > No, not -ENOSYS, as that triggers my initial problem again (please read the > deleted silly response ;-) > Checkpatch says: "ENOSYS means 'invalid syscall nr' and nothing else". > > -EINVAL? Sure, whatever works with your semantics. Will you test & send a patch? Yours, Linus Walleij
Hi Linus, On Mon, May 2, 2016 at 11:25 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, May 2, 2016 at 11:10 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Mon, May 2, 2016 at 11:06 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>> On Mon, May 2, 2016 at 11:04 AM, Geert Uytterhoeven >>> <geert@linux-m68k.org> wrote: >>> >>>> [silly response deleted] >>>> >>>> Scrap it. >>> >>> :D >>> >>>> The only annoying thing is that 0 cannot easily be propagated upstream as >>>> an error code, so it has to be tested for explicitly. >>> >>> Well with the Big Penguin's clear opinion on the matter there is not >>> much we can do. >>> >>> What about this? >>> >>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c >>> b/drivers/tty/serial/serial_mctrl_gpio.c >>> index 02147361eaa9..2297ec781681 100644 >>> --- a/drivers/tty/serial/serial_mctrl_gpio.c >>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >>> @@ -172,6 +172,13 @@ struct mctrl_gpios *mctrl_gpio_init(struct >>> uart_port *port, unsigned int idx) >>> dev_err(port->dev, >>> "failed to find corresponding irq for >>> %s (idx=%d, err=%d)\n", >>> mctrl_gpios_desc[i].name, idx, ret); >>> + /* >>> + * Satisfy the error code semantics for a missing IRQ, >>> + * 0 means NO_IRQ, but the framework needs to return >>> + * a negative to deal with the error. >>> + */ >>> + if (!ret) >>> + ret = -ENOSYS; >> >> No, not -ENOSYS, as that triggers my initial problem again (please read the >> deleted silly response ;-) >> Checkpatch says: "ENOSYS means 'invalid syscall nr' and nothing else". >> >> -EINVAL? Upon reconsideration, -ENXIO. > Sure, whatever works with your semantics. > > Will you test & send a patch? This works fine, but I'm a bit reluctant to send out the patches... gpiod_to_irq() is documented to return a negative error code on failure, cfr. Documentation/gpio/consumer.txt and drivers/gpio/gpiolib.c. In fact the check in mctrl_gpio_init() is the only caller that considers zero as an error. Perhaps gpiod_to_irq() should handle .to_irq() returning zero instead? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, May 2, 2016 at 12:05 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > gpiod_to_irq() is documented to return a negative error code on failure, > cfr. Documentation/gpio/consumer.txt and drivers/gpio/gpiolib.c. > In fact the check in mctrl_gpio_init() is the only caller that > considers zero as an > error. > > Perhaps gpiod_to_irq() should handle .to_irq() returning zero instead? OK good point, I sent a patch like so, let's see what people say. I'm a bit torn: part of me feel that since the function has *_to_irq() in it it should use zero for NO_IRQ if there is no translation. But this works too, whatever, rough consensus and running code. Yours, Linus Walleij
diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c index 02147361eaa9..2297ec781681 100644 --- a/drivers/tty/serial/serial_mctrl_gpio.c +++ b/drivers/tty/serial/serial_mctrl_gpio.c @@ -172,6 +172,13 @@ struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx) dev_err(port->dev, "failed to find corresponding irq for %s (idx=%d, err=%d)\n", mctrl_gpios_desc[i].name, idx, ret); + /* + * Satisfy the error code semantics for a missing IRQ, + * 0 means NO_IRQ, but the framework needs to return + * a negative to deal with the error. + */ + if (!ret) + ret = -ENOSYS; return ERR_PTR(ret);