* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls [not found] ` <CAMuHMdXiz5hqW=fm-2H1aNKq_gXD2o0508jD2XjxYUvKoEvHdw@mail.gmail.com> @ 2017-03-04 17:48 ` Uwe Kleine-König 2017-03-06 8:49 ` Geert Uytterhoeven 0 siblings, 1 reply; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-04 17:48 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Nicolas Ferre, linux-kernel@vger.kernel.org, linux-gpio, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hello, Cc += linux-gpio@vger.kernel.org On Sat, Mar 04, 2017 at 04:35:46PM +0100, Geert Uytterhoeven wrote: > On Fri, Mar 3, 2017 at 8:44 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Fri, Mar 03, 2017 at 08:21:05PM +0100, Geert Uytterhoeven wrote: > >> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > >> > index 91e7dddbf72c..2f4cdd4e7b4f 100644 > >> > --- a/drivers/tty/serial/sh-sci.c > >> > +++ b/drivers/tty/serial/sh-sci.c > >> > @@ -3022,7 +3022,7 @@ static int sci_probe_single(struct platform_device *dev, > >> > return ret; > >> > > >> > sciport->gpios = mctrl_gpio_init(&sciport->port, 0); > >> > - if (IS_ERR(sciport->gpios) && PTR_ERR(sciport->gpios) != -ENOSYS) > >> > + if (IS_ERR(sciport->gpios)) > >> > return PTR_ERR(sciport->gpios); > >> > >> Now the sh-sci driver fails to probe on legacy platforms where GPIOLIB=n. > >> The check for -ENOSYS made it succeed before. > > > > That's right, intended and the only option that's save (for some > > definition of save; the obvious downside is that there is no > > /dev/tty$whatever for you). > > That's not just a downside, but a plain regression on legacy platforms that > do not use GPIO flow control. The only sane way out is that the driver somehow finds out that no gpios are supposed to be needed. So you can pass in via platform_data that no gpios are supposed to be used if you don't want to enable GPIOLIB (or implement CONFIG_HALFGPIOLIB). But I'd say this is a quirk that should better be fixed globally. So I think we should implement HALFGPIOLIB that includes the lookup stuff and so can make gpiod_get_optional and friends return NULL if there is really no GPIO. > > Ignoring -ENOSYS is only ok if your device doesn't have a cts-gpio. If > > it has, ignoring -ENOSYS hides bugs because the driver sends data while > > it shouldn't or cannot signal the other side that it should stop (or > > start) a transmission. > > Mctrl_gpio supports modern platforms with GPIOLIB and DT or ACPI only. That's wrong. Even for a legacy device you can make use of GPIOs. See arch/arm/mach-tegra/board-paz00.c for a simple example. > On legacy platforms, you cannot use GPIO flow control (except when using a > custom implementation, which is out-of-scope here), so the issue of silently > running without cts-gpio on these platforms is moot. Given that mctrl-gpio can be useful on legacy platforms, a device could silently run without cts-gpio even there. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-04 17:48 ` [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Uwe Kleine-König @ 2017-03-06 8:49 ` Geert Uytterhoeven 2017-03-06 8:58 ` Uwe Kleine-König 0 siblings, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-06 8:49 UTC (permalink / raw) To: Uwe Kleine-König Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Nicolas Ferre, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hi Uwe, On Sat, Mar 4, 2017 at 6:48 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Cc += linux-gpio@vger.kernel.org > > On Sat, Mar 04, 2017 at 04:35:46PM +0100, Geert Uytterhoeven wrote: >> On Fri, Mar 3, 2017 at 8:44 PM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> > On Fri, Mar 03, 2017 at 08:21:05PM +0100, Geert Uytterhoeven wrote: >> >> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c >> >> > index 91e7dddbf72c..2f4cdd4e7b4f 100644 >> >> > --- a/drivers/tty/serial/sh-sci.c >> >> > +++ b/drivers/tty/serial/sh-sci.c >> >> > @@ -3022,7 +3022,7 @@ static int sci_probe_single(struct platform_device *dev, >> >> > return ret; >> >> > >> >> > sciport->gpios = mctrl_gpio_init(&sciport->port, 0); >> >> > - if (IS_ERR(sciport->gpios) && PTR_ERR(sciport->gpios) != -ENOSYS) >> >> > + if (IS_ERR(sciport->gpios)) >> >> > return PTR_ERR(sciport->gpios); >> >> >> >> Now the sh-sci driver fails to probe on legacy platforms where GPIOLIB=n. >> >> The check for -ENOSYS made it succeed before. >> > >> > That's right, intended and the only option that's save (for some >> > definition of save; the obvious downside is that there is no >> > /dev/tty$whatever for you). >> >> That's not just a downside, but a plain regression on legacy platforms that >> do not use GPIO flow control. > > The only sane way out is that the driver somehow finds out that no gpios > are supposed to be needed. So you can pass in via platform_data that no > gpios are supposed to be used if you don't want to enable GPIOLIB (or > implement CONFIG_HALFGPIOLIB). But I'd say this is a quirk that should > better be fixed globally. So I think we should implement HALFGPIOLIB > that includes the lookup stuff and so can make gpiod_get_optional and > friends return NULL if there is really no GPIO. If CONFIG_GPIOLIB=n, no gpios are supposed to be needed. >> > Ignoring -ENOSYS is only ok if your device doesn't have a cts-gpio. If >> > it has, ignoring -ENOSYS hides bugs because the driver sends data while >> > it shouldn't or cannot signal the other side that it should stop (or >> > start) a transmission. >> >> Mctrl_gpio supports modern platforms with GPIOLIB and DT or ACPI only. > > That's wrong. Even for a legacy device you can make use of GPIOs. See > arch/arm/mach-tegra/board-paz00.c for a simple example. Sorry, forgot about gpiod_lookup_table. It's not used by any platform using the sh-sci serial driver. Still, using gpiod_lookup_table depends on CONFIG_GPIOLIB=y. >> On legacy platforms, you cannot use GPIO flow control (except when using a >> custom implementation, which is out-of-scope here), so the issue of silently >> running without cts-gpio on these platforms is moot. > > Given that mctrl-gpio can be useful on legacy platforms, a device could > silently run without cts-gpio even there. On platforms were CONFIG_GPIOLIB=n, this is not true, so the issue is moot. All serial drivers using (optional) mctrl-gpio have this in Kconfig: select SERIAL_MCTRL_GPIO if GPIOLIB So they will use mctrl-gpio when GPIOLIB is enabled. If GPIOPLIB is disabled, no flow control GPIOs are expected, and the driver should not break that case. 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-06 8:49 ` Geert Uytterhoeven @ 2017-03-06 8:58 ` Uwe Kleine-König 2017-03-06 9:09 ` Geert Uytterhoeven 0 siblings, 1 reply; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-06 8:58 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org Hello Geert, On Mon, Mar 06, 2017 at 09:49:39AM +0100, Geert Uytterhoeven wrote: > > Given that mctrl-gpio can be useful on legacy platforms, a device could > > silently run without cts-gpio even there. > > On platforms were CONFIG_GPIOLIB=n, this is not true, so the issue is moot. > > All serial drivers using (optional) mctrl-gpio have this in Kconfig: > > select SERIAL_MCTRL_GPIO if GPIOLIB > > So they will use mctrl-gpio when GPIOLIB is enabled. > If GPIOPLIB is disabled, no flow control GPIOs are expected, and the > driver should not break that case. So it all boils down to the question: Is GPIOLIB=n enough to assume no gpio is needed? I'd say it is not. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-06 8:58 ` Uwe Kleine-König @ 2017-03-06 9:09 ` Geert Uytterhoeven 2017-03-06 9:30 ` Uwe Kleine-König 0 siblings, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-06 9:09 UTC (permalink / raw) To: Uwe Kleine-König Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Nicolas Ferre, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hi Uwe, On Mon, Mar 6, 2017 at 9:58 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, Mar 06, 2017 at 09:49:39AM +0100, Geert Uytterhoeven wrote: >> > Given that mctrl-gpio can be useful on legacy platforms, a device could >> > silently run without cts-gpio even there. >> >> On platforms were CONFIG_GPIOLIB=n, this is not true, so the issue is moot. >> >> All serial drivers using (optional) mctrl-gpio have this in Kconfig: >> >> select SERIAL_MCTRL_GPIO if GPIOLIB >> >> So they will use mctrl-gpio when GPIOLIB is enabled. >> If GPIOPLIB is disabled, no flow control GPIOs are expected, and the >> driver should not break that case. > > So it all boils down to the question: Is GPIOLIB=n enough to assume no > gpio is needed? > > I'd say it is not. How does the platform register these GPIOs when GPIOPLIB is not enabled by the platform, and gpiod_add_lookup_table() is thus not available? Please show me an example. 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-06 9:09 ` Geert Uytterhoeven @ 2017-03-06 9:30 ` Uwe Kleine-König 2017-03-06 9:53 ` Geert Uytterhoeven 0 siblings, 1 reply; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-06 9:30 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Nicolas Ferre, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hello Geert, On Mon, Mar 06, 2017 at 10:09:50AM +0100, Geert Uytterhoeven wrote: > On Mon, Mar 6, 2017 at 9:58 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Mar 06, 2017 at 09:49:39AM +0100, Geert Uytterhoeven wrote: > >> > Given that mctrl-gpio can be useful on legacy platforms, a device could > >> > silently run without cts-gpio even there. > >> > >> On platforms were CONFIG_GPIOLIB=n, this is not true, so the issue is moot. > >> > >> All serial drivers using (optional) mctrl-gpio have this in Kconfig: > >> > >> select SERIAL_MCTRL_GPIO if GPIOLIB > >> > >> So they will use mctrl-gpio when GPIOLIB is enabled. > >> If GPIOPLIB is disabled, no flow control GPIOs are expected, and the > >> driver should not break that case. > > > > So it all boils down to the question: Is GPIOLIB=n enough to assume no > > gpio is needed? > > > > I'd say it is not. > > How does the platform register these GPIOs when GPIOPLIB is not enabled by > the platform, and gpiod_add_lookup_table() is thus not available? Obviously the platformcode cannot. In this case you could argue that platformcode shouldn't register the device if a gpio is necessary. But this reasoning doesn't work for (DT=y || ACPI=y) && GPIOLIB=n. I wouldn't want to code this in each driver (something like: if (IS_ENABLED(GPIOLIB) || device_is_instantiated_by_dt(dev) || device_is_instantiated_by_acpi(dev)) gpios = mctrl_gpio_init(...); else gpios = NULL; ). Putting this into GPIOLIB is the right approach, and so this is another argument for HALFGPIOLIB. This would fix mctrl_gpio_init en passant. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-06 9:30 ` Uwe Kleine-König @ 2017-03-06 9:53 ` Geert Uytterhoeven 2017-03-06 10:02 ` Uwe Kleine-König 0 siblings, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-06 9:53 UTC (permalink / raw) To: Uwe Kleine-König Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Nicolas Ferre, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hi Uwe, On Mon, Mar 6, 2017 at 10:30 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, Mar 06, 2017 at 10:09:50AM +0100, Geert Uytterhoeven wrote: >> On Mon, Mar 6, 2017 at 9:58 AM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> > On Mon, Mar 06, 2017 at 09:49:39AM +0100, Geert Uytterhoeven wrote: >> >> > Given that mctrl-gpio can be useful on legacy platforms, a device could >> >> > silently run without cts-gpio even there. >> >> >> >> On platforms were CONFIG_GPIOLIB=n, this is not true, so the issue is moot. >> >> >> >> All serial drivers using (optional) mctrl-gpio have this in Kconfig: >> >> >> >> select SERIAL_MCTRL_GPIO if GPIOLIB >> >> >> >> So they will use mctrl-gpio when GPIOLIB is enabled. >> >> If GPIOPLIB is disabled, no flow control GPIOs are expected, and the >> >> driver should not break that case. >> > >> > So it all boils down to the question: Is GPIOLIB=n enough to assume no >> > gpio is needed? >> > >> > I'd say it is not. >> >> How does the platform register these GPIOs when GPIOPLIB is not enabled by >> the platform, and gpiod_add_lookup_table() is thus not available? > > Obviously the platformcode cannot. In this case you could argue that > platformcode shouldn't register the device if a gpio is necessary. But > this reasoning doesn't work for (DT=y || ACPI=y) && GPIOLIB=n. > > I wouldn't want to code this in each driver (something like: > > if (IS_ENABLED(GPIOLIB) || device_is_instantiated_by_dt(dev) || device_is_instantiated_by_acpi(dev)) > gpios = mctrl_gpio_init(...); > else > gpios = NULL; > > ). Putting this into GPIOLIB is the right approach, and so this is > another argument for HALFGPIOLIB. This would fix mctrl_gpio_init en > passant. Do we have platforms where DT=y || ACPI=y, but GPIOLIB=n? Ah, x86 ;-) Anyway, for sh-sci.c, platforms either have DT and GPIOLIB, or they do not need mctrl-gpio. 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-06 9:53 ` Geert Uytterhoeven @ 2017-03-06 10:02 ` Uwe Kleine-König 2017-03-14 15:32 ` Linus Walleij 2017-03-16 15:18 ` Linus Walleij 0 siblings, 2 replies; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-06 10:02 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org, Linus Walleij Cc += LinusW On Mon, Mar 06, 2017 at 10:53:27AM +0100, Geert Uytterhoeven wrote: > On Mon, Mar 6, 2017 at 10:30 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Mar 06, 2017 at 10:09:50AM +0100, Geert Uytterhoeven wrote: > >> On Mon, Mar 6, 2017 at 9:58 AM, Uwe Kleine-König > >> <u.kleine-koenig@pengutronix.de> wrote: > >> > On Mon, Mar 06, 2017 at 09:49:39AM +0100, Geert Uytterhoeven wrote: > >> >> > Given that mctrl-gpio can be useful on legacy platforms, a device could > >> >> > silently run without cts-gpio even there. > >> >> > >> >> On platforms were CONFIG_GPIOLIB=n, this is not true, so the issue is moot. > >> >> > >> >> All serial drivers using (optional) mctrl-gpio have this in Kconfig: > >> >> > >> >> select SERIAL_MCTRL_GPIO if GPIOLIB > >> >> > >> >> So they will use mctrl-gpio when GPIOLIB is enabled. > >> >> If GPIOPLIB is disabled, no flow control GPIOs are expected, and the > >> >> driver should not break that case. > >> > > >> > So it all boils down to the question: Is GPIOLIB=n enough to assume no > >> > gpio is needed? > >> > > >> > I'd say it is not. > >> > >> How does the platform register these GPIOs when GPIOPLIB is not enabled by > >> the platform, and gpiod_add_lookup_table() is thus not available? > > > > Obviously the platformcode cannot. In this case you could argue that > > platformcode shouldn't register the device if a gpio is necessary. But > > this reasoning doesn't work for (DT=y || ACPI=y) && GPIOLIB=n. > > > > I wouldn't want to code this in each driver (something like: > > > > if (IS_ENABLED(GPIOLIB) || device_is_instantiated_by_dt(dev) || device_is_instantiated_by_acpi(dev)) > > gpios = mctrl_gpio_init(...); > > else > > gpios = NULL; > > > > ). Putting this into GPIOLIB is the right approach, and so this is > > another argument for HALFGPIOLIB. This would fix mctrl_gpio_init en > > passant. > > Do we have platforms where DT=y || ACPI=y, but GPIOLIB=n? > Ah, x86 ;-) Yeah, and I think rm -r arch/x86 won't be acceptable :-) I assume you can also configure some arm or powerpc systems without GPIOLIB. > Anyway, for sh-sci.c, platforms either have DT and GPIOLIB, or they do not > need mctrl-gpio. So we're in agreement now that HALFGPIOLIB is the way to go? Linus, what do you think? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-06 10:02 ` Uwe Kleine-König @ 2017-03-14 15:32 ` Linus Walleij 2017-03-16 15:18 ` Linus Walleij 1 sibling, 0 replies; 38+ messages in thread From: Linus Walleij @ 2017-03-14 15:32 UTC (permalink / raw) To: Uwe Kleine-König Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, Geert Uytterhoeven, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org On Mon, Mar 6, 2017 at 11:02 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: >> Anyway, for sh-sci.c, platforms either have DT and GPIOLIB, or they do not >> need mctrl-gpio. > > So we're in agreement now that HALFGPIOLIB is the way to go? > Linus, what do you think? I'm too swamped in mail and work to figure this out right now. I guess I will get back to it... I need more active comaintainers I guess, interested in the job? ;) Yours, Linus Walleij _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-06 10:02 ` Uwe Kleine-König 2017-03-14 15:32 ` Linus Walleij @ 2017-03-16 15:18 ` Linus Walleij 2017-03-16 16:37 ` Uwe Kleine-König 2017-03-16 16:38 ` Geert Uytterhoeven 1 sibling, 2 replies; 38+ messages in thread From: Linus Walleij @ 2017-03-16 15:18 UTC (permalink / raw) To: Uwe Kleine-König Cc: Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Mon, Mar 6, 2017 at 11:02 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, Mar 06, 2017 at 10:53:27AM +0100, Geert Uytterhoeven wrote: >> On Mon, Mar 6, 2017 at 10:30 AM, Uwe Kleine-König >> > I wouldn't want to code this in each driver (something like: >> > >> > if (IS_ENABLED(GPIOLIB) || device_is_instantiated_by_dt(dev) || device_is_instantiated_by_acpi(dev)) >> > gpios = mctrl_gpio_init(...); >> > else >> > gpios = NULL; >> > >> > ). Putting this into GPIOLIB is the right approach, and so this is >> > another argument for HALFGPIOLIB. This would fix mctrl_gpio_init en >> > passant. >> >> Do we have platforms where DT=y || ACPI=y, but GPIOLIB=n? >> Ah, x86 ;-) > > Yeah, and I think rm -r arch/x86 won't be acceptable :-) I assume you > can also configure some arm or powerpc systems without GPIOLIB. > >> Anyway, for sh-sci.c, platforms either have DT and GPIOLIB, or they do not >> need mctrl-gpio. > > So we're in agreement now that HALFGPIOLIB is the way to go? > Linus, what do you think? OK modem lines over GPIO. So the problem is that GPIOLIB is needed (obviously) for mctrl_gpio_init() to work properly, and then there are some stubs in drivers/tty/serial/serial_mctrl_gpio.h for !GPIOLIB. And this whole discussion is all about that !GPIOLIB case really, whether DT, ACPI, SFI or board files machine data is used doesn't really matter. We're talking about: > git grep mctrl_gpio_init drivers/tty/serial/atmel_serial.c: atmel_port->gpios = mctrl_gpio_init(&atmel_port->uart, 0); drivers/tty/serial/clps711x.c: s->gpios = mctrl_gpio_init_noauto(&pdev->dev, 0); drivers/tty/serial/etraxfs-uart.c: up->gpios = mctrl_gpio_init_noauto(&pdev->dev, 0); drivers/tty/serial/imx.c: sport->gpios = mctrl_gpio_init(&sport->port, 0); drivers/tty/serial/mxs-auart.c: s->gpios = mctrl_gpio_init_noauto(dev, 0); drivers/tty/serial/sh-sci.c: sciport->gpios = mctrl_gpio_init(&sciport->port, 0); Atmel, ARM, ETRAX, ARM, ARM, Super-H, all have GPIOLIB. Right now no x86, correct? They actually all even do things like this in Kconfig: config SERIAL_ATMEL (...) select SERIAL_MCTRL_GPIO if GPIOLIB What stops us from removing all the stubs in drivers/tty/serial/serial_mctrl_gpio.h and just make SERIAL_MCTRL_GPIO depends on GPIOLIB? >From 2ecc70acc510784d953add707f2a5acfebe484c2 Mon Sep 17 00:00:00 2001 From: Linus Walleij <linus.walleij@linaro.org> Date: Thu, 16 Mar 2017 16:18:21 +0100 Subject: [PATCH] stab at mctrl Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/tty/serial/Kconfig | 1 + drivers/tty/serial/serial_mctrl_gpio.h | 55 ---------------------------------- 2 files changed, 1 insertion(+), 55 deletions(-) diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig index 6117ac8da48f..39833d009c18 100644 --- a/drivers/tty/serial/Kconfig +++ b/drivers/tty/serial/Kconfig @@ -1681,5 +1681,6 @@ endmenu config SERIAL_MCTRL_GPIO tristate + depends on GPIOLIB endif # TTY diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h index fa000bcff217..ba8f8e531d56 100644 --- a/drivers/tty/serial/serial_mctrl_gpio.h +++ b/drivers/tty/serial/serial_mctrl_gpio.h @@ -40,8 +40,6 @@ enum mctrl_gpio_idx { */ struct mctrl_gpios; -#ifdef CONFIG_GPIOLIB - /* * Set state of the modem control output lines via GPIOs. */ @@ -101,57 +99,4 @@ void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios); */ void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios); -#else /* GPIOLIB */ - -static inline -void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl) -{ -} - -static inline -unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl) -{ - return *mctrl; -} - -static inline unsigned int -mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl) -{ - return *mctrl; -} - -static inline -struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios, - enum mctrl_gpio_idx gidx) -{ - return ERR_PTR(-ENOSYS); -} - -static inline -struct mctrl_gpios *mctrl_gpio_init(struct uart_port *port, unsigned int idx) -{ - return ERR_PTR(-ENOSYS); -} - -static inline -struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx) -{ - return ERR_PTR(-ENOSYS); -} - -static inline -void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios) -{ -} - -static inline void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios) -{ -} - -static inline void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios) -{ -} - -#endif /* GPIOLIB */ - #endif -- 2.9.3 Yours, Linus Walleij ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-16 15:18 ` Linus Walleij @ 2017-03-16 16:37 ` Uwe Kleine-König 2017-03-16 16:38 ` Geert Uytterhoeven 1 sibling, 0 replies; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-16 16:37 UTC (permalink / raw) To: Linus Walleij Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, Geert Uytterhoeven, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org On Thu, Mar 16, 2017 at 04:18:52PM +0100, Linus Walleij wrote: > On Mon, Mar 6, 2017 at 11:02 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Mar 06, 2017 at 10:53:27AM +0100, Geert Uytterhoeven wrote: > >> On Mon, Mar 6, 2017 at 10:30 AM, Uwe Kleine-König > > >> > I wouldn't want to code this in each driver (something like: > >> > > >> > if (IS_ENABLED(GPIOLIB) || device_is_instantiated_by_dt(dev) || device_is_instantiated_by_acpi(dev)) > >> > gpios = mctrl_gpio_init(...); > >> > else > >> > gpios = NULL; > >> > > >> > ). Putting this into GPIOLIB is the right approach, and so this is > >> > another argument for HALFGPIOLIB. This would fix mctrl_gpio_init en > >> > passant. > >> > >> Do we have platforms where DT=y || ACPI=y, but GPIOLIB=n? > >> Ah, x86 ;-) > > > > Yeah, and I think rm -r arch/x86 won't be acceptable :-) I assume you > > can also configure some arm or powerpc systems without GPIOLIB. > > > >> Anyway, for sh-sci.c, platforms either have DT and GPIOLIB, or they do not > >> need mctrl-gpio. > > > > So we're in agreement now that HALFGPIOLIB is the way to go? > > Linus, what do you think? > > OK modem lines over GPIO. > > So the problem is that GPIOLIB is needed (obviously) for mctrl_gpio_init() to > work properly, and then there are some stubs in > drivers/tty/serial/serial_mctrl_gpio.h > for !GPIOLIB. > > And this whole discussion is all about that !GPIOLIB case really, > whether DT, ACPI, SFI or board files machine data is used doesn't > really matter. > > We're talking about: > > > git grep mctrl_gpio_init > drivers/tty/serial/atmel_serial.c: atmel_port->gpios = > mctrl_gpio_init(&atmel_port->uart, 0); > drivers/tty/serial/clps711x.c: s->gpios = > mctrl_gpio_init_noauto(&pdev->dev, 0); > drivers/tty/serial/etraxfs-uart.c: up->gpios = > mctrl_gpio_init_noauto(&pdev->dev, 0); > drivers/tty/serial/imx.c: sport->gpios = mctrl_gpio_init(&sport->port, 0); > drivers/tty/serial/mxs-auart.c: s->gpios = mctrl_gpio_init_noauto(dev, 0); > drivers/tty/serial/sh-sci.c: sciport->gpios = > mctrl_gpio_init(&sciport->port, 0); > > Atmel, ARM, ETRAX, ARM, ARM, Super-H, all have GPIOLIB. > Right now no x86, correct? > > They actually all even do things like this in Kconfig: > > config SERIAL_ATMEL > (...) > select SERIAL_MCTRL_GPIO if GPIOLIB > > What stops us from removing all the stubs in > drivers/tty/serial/serial_mctrl_gpio.h > and just make SERIAL_MCTRL_GPIO depends on GPIOLIB? Would be ok for me, too. People seem to object to that though. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-16 15:18 ` Linus Walleij 2017-03-16 16:37 ` Uwe Kleine-König @ 2017-03-16 16:38 ` Geert Uytterhoeven 2017-03-20 9:56 ` Geert Uytterhoeven 1 sibling, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-16 16:38 UTC (permalink / raw) To: Linus Walleij Cc: Uwe Kleine-König, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org Hi Linus, On Thu, Mar 16, 2017 at 4:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Mar 6, 2017 at 11:02 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: >> On Mon, Mar 06, 2017 at 10:53:27AM +0100, Geert Uytterhoeven wrote: >>> On Mon, Mar 6, 2017 at 10:30 AM, Uwe Kleine-König >>> > I wouldn't want to code this in each driver (something like: >>> > >>> > if (IS_ENABLED(GPIOLIB) || device_is_instantiated_by_dt(dev) || device_is_instantiated_by_acpi(dev)) >>> > gpios = mctrl_gpio_init(...); >>> > else >>> > gpios = NULL; >>> > >>> > ). Putting this into GPIOLIB is the right approach, and so this is >>> > another argument for HALFGPIOLIB. This would fix mctrl_gpio_init en >>> > passant. >>> >>> Do we have platforms where DT=y || ACPI=y, but GPIOLIB=n? >>> Ah, x86 ;-) >> >> Yeah, and I think rm -r arch/x86 won't be acceptable :-) I assume you >> can also configure some arm or powerpc systems without GPIOLIB. >> >>> Anyway, for sh-sci.c, platforms either have DT and GPIOLIB, or they do not >>> need mctrl-gpio. >> >> So we're in agreement now that HALFGPIOLIB is the way to go? >> Linus, what do you think? > > OK modem lines over GPIO. > > So the problem is that GPIOLIB is needed (obviously) for mctrl_gpio_init() to > work properly, and then there are some stubs in > drivers/tty/serial/serial_mctrl_gpio.h > for !GPIOLIB. > > And this whole discussion is all about that !GPIOLIB case really, > whether DT, ACPI, SFI or board files machine data is used doesn't > really matter. > > We're talking about: > >> git grep mctrl_gpio_init > drivers/tty/serial/atmel_serial.c: atmel_port->gpios = > mctrl_gpio_init(&atmel_port->uart, 0); > drivers/tty/serial/clps711x.c: s->gpios = > mctrl_gpio_init_noauto(&pdev->dev, 0); > drivers/tty/serial/etraxfs-uart.c: up->gpios = > mctrl_gpio_init_noauto(&pdev->dev, 0); > drivers/tty/serial/imx.c: sport->gpios = mctrl_gpio_init(&sport->port, 0); > drivers/tty/serial/mxs-auart.c: s->gpios = mctrl_gpio_init_noauto(dev, 0); > drivers/tty/serial/sh-sci.c: sciport->gpios = > mctrl_gpio_init(&sciport->port, 0); > > Atmel, ARM, ETRAX, ARM, ARM, Super-H, all have GPIOLIB. > Right now no x86, correct? It's not that black-and-white. Some of SuperH have GPIOLIB, other parts don't. > They actually all even do things like this in Kconfig: > > config SERIAL_ATMEL > (...) > select SERIAL_MCTRL_GPIO if GPIOLIB > > What stops us from removing all the stubs in > drivers/tty/serial/serial_mctrl_gpio.h > and just make SERIAL_MCTRL_GPIO depends on GPIOLIB? Removing the stubs implies adding #ifdefs to the drivers that need to handle the !SERIAL_MCTRL_GPIO case. E.g. I don't want to break the sh-sci serial driver on SuperH platforms that (a) don't select GPIOLIB, and (b) don't use mtrl_gpio. 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-16 16:38 ` Geert Uytterhoeven @ 2017-03-20 9:56 ` Geert Uytterhoeven 2017-03-20 10:03 ` Geert Uytterhoeven 2017-03-20 10:31 ` Uwe Kleine-König 0 siblings, 2 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-20 9:56 UTC (permalink / raw) To: Linus Walleij Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, Uwe Kleine-König, linux-arm-kernel@lists.infradead.org Hi Linus, On Thu, Mar 16, 2017 at 5:38 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> They actually all even do things like this in Kconfig: >> >> config SERIAL_ATMEL >> (...) >> select SERIAL_MCTRL_GPIO if GPIOLIB >> >> What stops us from removing all the stubs in >> drivers/tty/serial/serial_mctrl_gpio.h >> and just make SERIAL_MCTRL_GPIO depends on GPIOLIB? > > Removing the stubs implies adding #ifdefs to the drivers that need > to handle the !SERIAL_MCTRL_GPIO case. > > E.g. I don't want to break the sh-sci serial driver on SuperH platforms that > (a) don't select GPIOLIB, and > (b) don't use mtrl_gpio. Alternatively, after commit 22c403676dbbb7c6 ("gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled") we might just drop the dependency of SERIAL_MCTRL_GPIO on GPIOLIB? Then no special handling is needed in drivers, as all GPIOs will just be considered not present. Documentation/gpio/consumer.txt rightfully sates: | Note that gpio_get*_optional() functions (and their managed variants), unlike | the rest of gpiolib API, also return NULL when gpiolib support is disabled. | This is helpful to driver authors, since they do not need to special case | -ENOSYS return codes. System integrators should however be careful to enable | gpiolib on systems that need it. drivers/tty/serial/serial_mctrl_gpio.o already compiles fine if CONFIG_GPIOLIB=n, which reduces its size by ca. 25%. 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-20 9:56 ` Geert Uytterhoeven @ 2017-03-20 10:03 ` Geert Uytterhoeven 2017-03-20 10:31 ` Uwe Kleine-König 1 sibling, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-20 10:03 UTC (permalink / raw) To: Linus Walleij Cc: Uwe Kleine-König, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Mon, Mar 20, 2017 at 10:56 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > drivers/tty/serial/serial_mctrl_gpio.o already compiles fine if > CONFIG_GPIOLIB=n, which reduces its size by ca. 25%. That is: after removing the #ifdef and dummies in serial_mctrl_gpio.h 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-20 9:56 ` Geert Uytterhoeven 2017-03-20 10:03 ` Geert Uytterhoeven @ 2017-03-20 10:31 ` Uwe Kleine-König 2017-03-20 10:38 ` Geert Uytterhoeven 1 sibling, 1 reply; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-20 10:31 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Linus Walleij, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hello Geert, On Mon, Mar 20, 2017 at 10:56:14AM +0100, Geert Uytterhoeven wrote: > Documentation/gpio/consumer.txt rightfully sates: > | Note that gpio_get*_optional() functions (and their managed variants), unlike > | the rest of gpiolib API, also return NULL when gpiolib support is disabled. > | This is helpful to driver authors, since they do not need to special case > | -ENOSYS return codes. System integrators should however be careful to enable > | gpiolib on systems that need it. I cannot find this paragraph in Documentation/gpio/consumer.txt: $ git grep -e 'ENOSYS' v4.11-rc3 -- Documentation/gpio/ <void> Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-20 10:31 ` Uwe Kleine-König @ 2017-03-20 10:38 ` Geert Uytterhoeven 2017-03-20 11:07 ` Uwe Kleine-König 0 siblings, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-20 10:38 UTC (permalink / raw) To: Uwe Kleine-König Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Linus Walleij, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hi Uwe, On Mon, Mar 20, 2017 at 11:31 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, Mar 20, 2017 at 10:56:14AM +0100, Geert Uytterhoeven wrote: >> Documentation/gpio/consumer.txt rightfully sates: >> | Note that gpio_get*_optional() functions (and their managed variants), unlike >> | the rest of gpiolib API, also return NULL when gpiolib support is disabled. >> | This is helpful to driver authors, since they do not need to special case >> | -ENOSYS return codes. System integrators should however be careful to enable >> | gpiolib on systems that need it. > > I cannot find this paragraph in Documentation/gpio/consumer.txt: > > $ git grep -e 'ENOSYS' v4.11-rc3 -- Documentation/gpio/ > <void> It was added by commit 22c403676dbbb7c6 ("gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled") 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-20 10:38 ` Geert Uytterhoeven @ 2017-03-20 11:07 ` Uwe Kleine-König 2017-03-23 9:32 ` Linus Walleij 0 siblings, 1 reply; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-20 11:07 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Linus Walleij, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hello Geert, On Mon, Mar 20, 2017 at 11:38:52AM +0100, Geert Uytterhoeven wrote: > On Mon, Mar 20, 2017 at 11:31 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Mar 20, 2017 at 10:56:14AM +0100, Geert Uytterhoeven wrote: > >> Documentation/gpio/consumer.txt rightfully sates: > >> | Note that gpio_get*_optional() functions (and their managed variants), unlike > >> | the rest of gpiolib API, also return NULL when gpiolib support is disabled. > >> | This is helpful to driver authors, since they do not need to special case > >> | -ENOSYS return codes. System integrators should however be careful to enable > >> | gpiolib on systems that need it. > > > > I cannot find this paragraph in Documentation/gpio/consumer.txt: > > > > $ git grep -e 'ENOSYS' v4.11-rc3 -- Documentation/gpio/ > > <void> > > It was added by commit 22c403676dbbb7c6 ("gpio: return NULL from > gpiod_get_optional when GPIOLIB is disabled") Ah, that's in next. I still think this is wrong and I'm a bit disappointed that Linus merged this patch (without saying so in the thread even) as I thought Linus and I agreed on this being a bad idea. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-20 11:07 ` Uwe Kleine-König @ 2017-03-23 9:32 ` Linus Walleij 2017-03-23 10:10 ` Uwe Kleine-König 0 siblings, 1 reply; 38+ messages in thread From: Linus Walleij @ 2017-03-23 9:32 UTC (permalink / raw) To: Uwe Kleine-König, Dmitry Torokhov Cc: Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Mon, Mar 20, 2017 at 12:07 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello Geert, > > On Mon, Mar 20, 2017 at 11:38:52AM +0100, Geert Uytterhoeven wrote: >> On Mon, Mar 20, 2017 at 11:31 AM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> > On Mon, Mar 20, 2017 at 10:56:14AM +0100, Geert Uytterhoeven wrote: >> >> Documentation/gpio/consumer.txt rightfully sates: >> >> | Note that gpio_get*_optional() functions (and their managed variants), unlike >> >> | the rest of gpiolib API, also return NULL when gpiolib support is disabled. >> >> | This is helpful to driver authors, since they do not need to special case >> >> | -ENOSYS return codes. System integrators should however be careful to enable >> >> | gpiolib on systems that need it. >> > >> > I cannot find this paragraph in Documentation/gpio/consumer.txt: >> > >> > $ git grep -e 'ENOSYS' v4.11-rc3 -- Documentation/gpio/ >> > <void> >> >> It was added by commit 22c403676dbbb7c6 ("gpio: return NULL from >> gpiod_get_optional when GPIOLIB is disabled") > > Ah, that's in next. > > I still think this is wrong and I'm a bit disappointed that Linus merged > this patch (without saying so in the thread even) as I thought Linus and > I agreed on this being a bad idea. I think it is not good, but what we have before this patch is worse. I.e. it is the lesser evil. Before this patch the API is inconsistent: it gives NULL if GPIOLIB is defined and -ENOSYS if GPIOLIB is not defined. After this patch it gives NULL in both cases, and that is at least consistent. It would be great if someone steps in and fix it so as to return relevant errors (or error pointers rather) in both cases. However I guess it will lead a bunch of fallouts in the consumers, albeit they are not super-many. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 9:32 ` Linus Walleij @ 2017-03-23 10:10 ` Uwe Kleine-König 2017-03-23 10:20 ` Geert Uytterhoeven 2017-03-23 13:37 ` Linus Walleij 0 siblings, 2 replies; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-23 10:10 UTC (permalink / raw) To: Linus Walleij Cc: Dmitry Torokhov, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Thu, Mar 23, 2017 at 10:32:01AM +0100, Linus Walleij wrote: > On Mon, Mar 20, 2017 at 12:07 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > Hello Geert, > > > > On Mon, Mar 20, 2017 at 11:38:52AM +0100, Geert Uytterhoeven wrote: > >> On Mon, Mar 20, 2017 at 11:31 AM, Uwe Kleine-König > >> <u.kleine-koenig@pengutronix.de> wrote: > >> > On Mon, Mar 20, 2017 at 10:56:14AM +0100, Geert Uytterhoeven wrote: > >> >> Documentation/gpio/consumer.txt rightfully sates: > >> >> | Note that gpio_get*_optional() functions (and their managed variants), unlike > >> >> | the rest of gpiolib API, also return NULL when gpiolib support is disabled. > >> >> | This is helpful to driver authors, since they do not need to special case > >> >> | -ENOSYS return codes. System integrators should however be careful to enable > >> >> | gpiolib on systems that need it. > >> > > >> > I cannot find this paragraph in Documentation/gpio/consumer.txt: > >> > > >> > $ git grep -e 'ENOSYS' v4.11-rc3 -- Documentation/gpio/ > >> > <void> > >> > >> It was added by commit 22c403676dbbb7c6 ("gpio: return NULL from > >> gpiod_get_optional when GPIOLIB is disabled") > > > > Ah, that's in next. > > > > I still think this is wrong and I'm a bit disappointed that Linus merged > > this patch (without saying so in the thread even) as I thought Linus and > > I agreed on this being a bad idea. > > I think it is not good, but what we have before this patch is worse. > > I.e. it is the lesser evil. > > Before this patch the API is inconsistent: it gives NULL if GPIOLIB > is defined and -ENOSYS if GPIOLIB is not defined. So old is: it worked as intended with GPIOLIB and produced an error without GPIOLIB even if no GPIO is there and NULL would be right. > After this patch it gives NULL in both cases, and that is at least > consistent. And new is: with GPIOLIB it still works as intended and gives you NULL even if a GPIO is there and so -ENOSYS would be right. I admit that most of the time with GPIOLIB NULL is the right answer because probably there is no GPIO to handle. But if there is a GPIO you run in hard to debug problems instead of being able to see the error code and act accordingly. write(2) and close(2) succeed most of the time, too. Still it's not a good idea to not check the return value. Or let the kernel return success unconditionally. So you exchanged many obvious and easy to fix problems with a few hard ones. I don't agree that's a good idea, but you seem to be willing to try it. Good luck. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 10:10 ` Uwe Kleine-König @ 2017-03-23 10:20 ` Geert Uytterhoeven 2017-03-23 11:11 ` Uwe Kleine-König 2017-03-23 13:37 ` Linus Walleij 1 sibling, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-23 10:20 UTC (permalink / raw) To: Uwe Kleine-König Cc: Linus Walleij, Dmitry Torokhov, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org Hi Uwe, On Thu, Mar 23, 2017 at 11:10 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Thu, Mar 23, 2017 at 10:32:01AM +0100, Linus Walleij wrote: >> On Mon, Mar 20, 2017 at 12:07 PM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> > On Mon, Mar 20, 2017 at 11:38:52AM +0100, Geert Uytterhoeven wrote: >> >> On Mon, Mar 20, 2017 at 11:31 AM, Uwe Kleine-König >> >> <u.kleine-koenig@pengutronix.de> wrote: >> >> > On Mon, Mar 20, 2017 at 10:56:14AM +0100, Geert Uytterhoeven wrote: >> >> >> Documentation/gpio/consumer.txt rightfully sates: >> >> >> | Note that gpio_get*_optional() functions (and their managed variants), unlike >> >> >> | the rest of gpiolib API, also return NULL when gpiolib support is disabled. >> >> >> | This is helpful to driver authors, since they do not need to special case >> >> >> | -ENOSYS return codes. System integrators should however be careful to enable >> >> >> | gpiolib on systems that need it. >> >> > >> >> > I cannot find this paragraph in Documentation/gpio/consumer.txt: >> >> > >> >> > $ git grep -e 'ENOSYS' v4.11-rc3 -- Documentation/gpio/ >> >> > <void> >> >> >> >> It was added by commit 22c403676dbbb7c6 ("gpio: return NULL from >> >> gpiod_get_optional when GPIOLIB is disabled") >> > >> > Ah, that's in next. >> > >> > I still think this is wrong and I'm a bit disappointed that Linus merged >> > this patch (without saying so in the thread even) as I thought Linus and >> > I agreed on this being a bad idea. >> >> I think it is not good, but what we have before this patch is worse. >> >> I.e. it is the lesser evil. >> >> Before this patch the API is inconsistent: it gives NULL if GPIOLIB >> is defined and -ENOSYS if GPIOLIB is not defined. > > So old is: it worked as intended with GPIOLIB and produced an error > without GPIOLIB even if no GPIO is there and NULL would be right. > >> After this patch it gives NULL in both cases, and that is at least >> consistent. > > And new is: with GPIOLIB it still works as intended and gives you NULL > even if a GPIO is there and so -ENOSYS would be right. If I forget to enable a driver, the corresponding class device is also not there. > I admit that most of the time with GPIOLIB NULL is the right answer > because probably there is no GPIO to handle. But if there is a GPIO you > run in hard to debug problems instead of being able to see the error > code and act accordingly. But having the error breaks setups where the GPIO is optional and does not exist. Make sure to enable all drivers and subsystems you need when building your kernel. That's always true. And may indeed be hard to debug (e.g. what kernel options do I need to make systemd work?). > write(2) and close(2) succeed most of the time, too. Still it's not a > good idea to not check the return value. Or let the kernel return > success unconditionally. Writing all bytes passed in the buffer is "optional" in another sense than an "optional" GPIO: you must retry the write, while you can continue if an optional GPIO is not present. > So you exchanged many obvious and easy to fix problems with a few hard > ones. I don't agree that's a good idea, but you seem to be willing to > try it. Good luck. Yeah, before drivers had to explicitly ignore -ENOSYS if they want to support platforms with and without GPIOLIB. Bad... 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 10:20 ` Geert Uytterhoeven @ 2017-03-23 11:11 ` Uwe Kleine-König 2017-03-23 12:03 ` Geert Uytterhoeven 2017-03-23 15:55 ` Dmitry Torokhov 0 siblings, 2 replies; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-23 11:11 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Linus Walleij, Dmitry Torokhov, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org Hello, On Thu, Mar 23, 2017 at 11:20:39AM +0100, Geert Uytterhoeven wrote: > But having the error breaks setups where the GPIO is optional and does > not exist. so the right way forward is to check harder in the situation where -ENOSYS was returned before to determine if there is really no GPIO to be used. "Oh, there are hints that there is no GPIO (GPIOLIB=n), so lets assume there isn't." is wrong. Can we please properly fix the problem instead of papering over it? > Make sure to enable all drivers and subsystems you need when building > your kernel. That's always true. And may indeed be hard to debug (e.g. what > kernel options do I need to make systemd work?). It's worse here. If you forget to enable a driver the device isn't bound and that's obvious to diagnose. When ignoring an optional GPIO there might be a device that claims to work but fails to do so. (e.g. you write to memory, write() returns 0, but the data never landed there.) > > write(2) and close(2) succeed most of the time, too. Still it's not a > > good idea to not check the return value. Or let the kernel return > > success unconditionally. > > Writing all bytes passed in the buffer is "optional" in another sense than > an "optional" GPIO: you must retry the write, while you can continue if > an optional GPIO is not present. And that is the point. You can continue *iff* the optional GPIO is not present. The patch in question removes the ability to determine if that GPIO is present and claims it is not present. > > So you exchanged many obvious and easy to fix problems with a few hard > > ones. I don't agree that's a good idea, but you seem to be willing to > > try it. Good luck. > > Yeah, before drivers had to explicitly ignore -ENOSYS if they want to > support platforms with and without GPIOLIB. Bad... Doing things right is sometimes not maximally easy. But that is no excuse to do it wrong. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 11:11 ` Uwe Kleine-König @ 2017-03-23 12:03 ` Geert Uytterhoeven 2017-03-23 12:34 ` Uwe Kleine-König 2017-03-23 15:55 ` Dmitry Torokhov 1 sibling, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-23 12:03 UTC (permalink / raw) To: Uwe Kleine-König Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Linus Walleij, Dmitry Torokhov, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hi Uwe, On Thu, Mar 23, 2017 at 12:11 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: >> Make sure to enable all drivers and subsystems you need when building >> your kernel. That's always true. And may indeed be hard to debug (e.g. what >> kernel options do I need to make systemd work?). > > It's worse here. If you forget to enable a driver the device isn't bound > and that's obvious to diagnose. When ignoring an optional GPIO there > might be a device that claims to work but fails to do so. (e.g. you > write to memory, write() returns 0, but the data never landed there.) > >> > write(2) and close(2) succeed most of the time, too. Still it's not a >> > good idea to not check the return value. Or let the kernel return >> > success unconditionally. >> >> Writing all bytes passed in the buffer is "optional" in another sense than >> an "optional" GPIO: you must retry the write, while you can continue if >> an optional GPIO is not present. > > And that is the point. You can continue *iff* the optional GPIO is not > present. The patch in question removes the ability to determine if that > GPIO is present and claims it is not present. If you forget to enable a driver/subsystem, you sometimes cannot determine if the device is present or not neither. Hence it boils down to "knowing" if there is a GPIO or not. So, when can there be a GPIO? 1. The GPIO is described in DT. => Not an issue, as DT GPIO implies GPIOLIB, 2. The GPIO is described in legacy platform data. => The platform code should make sure GPIOLIB is selected when needed. Issue solved? 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 12:03 ` Geert Uytterhoeven @ 2017-03-23 12:34 ` Uwe Kleine-König 2017-03-23 12:44 ` Geert Uytterhoeven 2017-03-23 13:41 ` Linus Walleij 0 siblings, 2 replies; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-23 12:34 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Linus Walleij, Dmitry Torokhov, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Thu, Mar 23, 2017 at 01:03:56PM +0100, Geert Uytterhoeven wrote: > Hi Uwe, > > On Thu, Mar 23, 2017 at 12:11 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > >> Make sure to enable all drivers and subsystems you need when building > >> your kernel. That's always true. And may indeed be hard to debug (e.g. what > >> kernel options do I need to make systemd work?). > > > > It's worse here. If you forget to enable a driver the device isn't bound > > and that's obvious to diagnose. When ignoring an optional GPIO there > > might be a device that claims to work but fails to do so. (e.g. you > > write to memory, write() returns 0, but the data never landed there.) > > > >> > write(2) and close(2) succeed most of the time, too. Still it's not a > >> > good idea to not check the return value. Or let the kernel return > >> > success unconditionally. > >> > >> Writing all bytes passed in the buffer is "optional" in another sense than > >> an "optional" GPIO: you must retry the write, while you can continue if > >> an optional GPIO is not present. > > > > And that is the point. You can continue *iff* the optional GPIO is not > > present. The patch in question removes the ability to determine if that > > GPIO is present and claims it is not present. > > If you forget to enable a driver/subsystem, you sometimes cannot determine > if the device is present or not neither. > > Hence it boils down to "knowing" if there is a GPIO or not. > So, when can there be a GPIO? > 1. The GPIO is described in DT. > => Not an issue, as DT GPIO implies GPIOLIB, > 2. The GPIO is described in legacy platform data. > => The platform code should make sure GPIOLIB is selected when needed. > > Issue solved? I like it better to not rely on platform code to do the right thing. Maybe we can make gpiod_get_optional look like this: if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB)) return NULL; else return -ENOSYS; I don't know how isnt_a_acpi_device looks like, probably it involves CONFIG_ACPI and/or dev->acpi_node. This should be safe and still comfortable for legacy platforms, isn't it? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 12:34 ` Uwe Kleine-König @ 2017-03-23 12:44 ` Geert Uytterhoeven 2017-03-23 13:41 ` Linus Walleij 1 sibling, 0 replies; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-23 12:44 UTC (permalink / raw) To: Uwe Kleine-König Cc: Linus Walleij, Dmitry Torokhov, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org Hi Uwe, On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Thu, Mar 23, 2017 at 01:03:56PM +0100, Geert Uytterhoeven wrote: >> On Thu, Mar 23, 2017 at 12:11 PM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> >> Make sure to enable all drivers and subsystems you need when building >> >> your kernel. That's always true. And may indeed be hard to debug (e.g. what >> >> kernel options do I need to make systemd work?). >> > >> > It's worse here. If you forget to enable a driver the device isn't bound >> > and that's obvious to diagnose. When ignoring an optional GPIO there >> > might be a device that claims to work but fails to do so. (e.g. you >> > write to memory, write() returns 0, but the data never landed there.) >> > >> >> > write(2) and close(2) succeed most of the time, too. Still it's not a >> >> > good idea to not check the return value. Or let the kernel return >> >> > success unconditionally. >> >> >> >> Writing all bytes passed in the buffer is "optional" in another sense than >> >> an "optional" GPIO: you must retry the write, while you can continue if >> >> an optional GPIO is not present. >> > >> > And that is the point. You can continue *iff* the optional GPIO is not >> > present. The patch in question removes the ability to determine if that >> > GPIO is present and claims it is not present. >> >> If you forget to enable a driver/subsystem, you sometimes cannot determine >> if the device is present or not neither. >> >> Hence it boils down to "knowing" if there is a GPIO or not. >> So, when can there be a GPIO? >> 1. The GPIO is described in DT. >> => Not an issue, as DT GPIO implies GPIOLIB, >> 2. The GPIO is described in legacy platform data. >> => The platform code should make sure GPIOLIB is selected when needed. >> >> Issue solved? > > I like it better to not rely on platform code to do the right thing. ;-) > Maybe we can make gpiod_get_optional look like this: > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB)) > return NULL; > else > return -ENOSYS; > > I don't know how isnt_a_acpi_device looks like, probably it involves > CONFIG_ACPI and/or dev->acpi_node. > > This should be safe and still comfortable for legacy platforms, isn't it? Yes, that should do the trick. No feedback from me about ACPI. 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 12:34 ` Uwe Kleine-König 2017-03-23 12:44 ` Geert Uytterhoeven @ 2017-03-23 13:41 ` Linus Walleij 2017-03-23 14:43 ` Dmitry Torokhov 1 sibling, 1 reply; 38+ messages in thread From: Linus Walleij @ 2017-03-23 13:41 UTC (permalink / raw) To: Uwe Kleine-König, Dmitry Torokhov Cc: Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Maybe we can make gpiod_get_optional look like this: > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB)) > return NULL; > else > return -ENOSYS; > > I don't know how isnt_a_acpi_device looks like, probably it involves > CONFIG_ACPI and/or dev->acpi_node. > > This should be safe and still comfortable for legacy platforms, isn't it? I like the looks of this. Can we revert Dmitry's patch and apply something like this instead? Dmitry, how do you feel about this? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 13:41 ` Linus Walleij @ 2017-03-23 14:43 ` Dmitry Torokhov 2017-03-23 15:44 ` Dmitry Torokhov 0 siblings, 1 reply; 38+ messages in thread From: Dmitry Torokhov @ 2017-03-23 14:43 UTC (permalink / raw) To: Linus Walleij Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, Geert Uytterhoeven, linux-serial@vger.kernel.org, Uwe Kleine-König, linux-arm-kernel@lists.infradead.org On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote: > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > Maybe we can make gpiod_get_optional look like this: > > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB)) > > return NULL; > > else > > return -ENOSYS; > > > > I don't know how isnt_a_acpi_device looks like, probably it involves > > CONFIG_ACPI and/or dev->acpi_node. > > > > This should be safe and still comfortable for legacy platforms, isn't it? > > I like the looks of this. > > Can we revert Dmitry's patch and apply something like this instead? > > Dmitry, how do you feel about this? I frankly do not see the point. It still makes driver code more complex for no good reason. I also think that not having optional GPIO is not an error, so returning value from error space is not correct. NULL is value from another space altogether. Uwe seems to be concerned about case that I find extremely unlikely. We are talking about a system that does not have GPIO support and behaves just fine, with the exception that it actually has (physically) a *single* GPIO, and that GPIO happens to be optional in a single driver, but in this particular system is actually needed (but that need manifests in a non-obvious way). And we have system integrator that has no idea what they are doing (no schematic, etc). I think that if there is one optional GPIO there will be mandatiry GPIOs in such system as well and selection of GPIOLIB will be forced early on in board bringup. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 14:43 ` Dmitry Torokhov @ 2017-03-23 15:44 ` Dmitry Torokhov 2017-03-23 19:10 ` Uwe Kleine-König 0 siblings, 1 reply; 38+ messages in thread From: Dmitry Torokhov @ 2017-03-23 15:44 UTC (permalink / raw) To: Linus Walleij Cc: Uwe Kleine-König, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote: > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote: > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > Maybe we can make gpiod_get_optional look like this: > > > > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB)) > > > return NULL; > > > else > > > return -ENOSYS; > > > > > > I don't know how isnt_a_acpi_device looks like, probably it involves > > > CONFIG_ACPI and/or dev->acpi_node. > > > > > > This should be safe and still comfortable for legacy platforms, isn't it? > > > > I like the looks of this. > > > > Can we revert Dmitry's patch and apply something like this instead? > > > > Dmitry, how do you feel about this? > > I frankly do not see the point. It still makes driver code more complex > for no good reason. I also think that not having optional GPIO is not an > error, so returning value from error space is not correct. NULL is value > from another space altogether. > > Uwe seems to be concerned about case that I find extremely unlikely. We > are talking about a system that does not have GPIO support and behaves > just fine, with the exception that it actually has (physically) a > *single* GPIO, and that GPIO happens to be optional in a single driver, > but in this particular system is actually needed (but that need > manifests in a non-obvious way). And we have system integrator that has > no idea what they are doing (no schematic, etc). > > I think that if there is one optional GPIO there will be mandatiry GPIOs > in such system as well and selection of GPIOLIB will be forced early on > in board bringup. One more thing: if we keep reporting -ENOSYS in case of !CONFIG_GPIOLIB, then most of the non platform-sepcific drivers will eventually gain code silently coping with this -ENOSYS: data->gpiod = gpiod_getptional(...); if (IS_ERR(data->gpiod)) { error = PTR_ERR(data->gpiod); if (error != -ENOSYS) return error; data->gpiod = NULL; /* This GPIO _is_ optional */ } which will negate Uwe's claim that it will help debugging issues. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 15:44 ` Dmitry Torokhov @ 2017-03-23 19:10 ` Uwe Kleine-König 2017-03-23 19:58 ` Dmitry Torokhov 2017-03-24 8:58 ` Linus Walleij 0 siblings, 2 replies; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-23 19:10 UTC (permalink / raw) To: Dmitry Torokhov Cc: Linus Walleij, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Thu, Mar 23, 2017 at 08:44:41AM -0700, Dmitry Torokhov wrote: > On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote: > > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote: > > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > Maybe we can make gpiod_get_optional look like this: > > > > > > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB)) > > > > return NULL; > > > > else > > > > return -ENOSYS; > > > > > > > > I don't know how isnt_a_acpi_device looks like, probably it involves > > > > CONFIG_ACPI and/or dev->acpi_node. > > > > > > > > This should be safe and still comfortable for legacy platforms, isn't it? > > > > > > I like the looks of this. > > > > > > Can we revert Dmitry's patch and apply something like this instead? > > > > > > Dmitry, how do you feel about this? > > > > I frankly do not see the point. It still makes driver code more complex Note that this code is in the gpio header, and not in driver code. So the driver just does gpiod = gpiod_get_optional(...) if (IS_ERR(gpiod)) return PTR_ERR(gpiod); (as it is supposed to do now). I think that's nice. > > for no good reason. I also think that not having optional GPIO is not an > > error, so returning value from error space is not correct. NULL is value > > from another space altogether. It seems you didn't understand my concern. > > Uwe seems to be concerned about case that I find extremely unlikely. We > > are talking about a system that does not have GPIO support and behaves > > just fine, with the exception that it actually has (physically) a > > *single* GPIO, and that GPIO happens to be optional in a single driver, > > but in this particular system is actually needed (but that need > > manifests in a non-obvious way). And we have system integrator that has > > no idea what they are doing (no schematic, etc). IMHO this is not an excuse to have lax error checking. > > I think that if there is one optional GPIO there will be mandatiry GPIOs > > in such system as well and selection of GPIOLIB will be forced early on > > in board bringup. > > One more thing: if we keep reporting -ENOSYS in case of !CONFIG_GPIOLIB, > then most of the non platform-sepcific drivers will eventually gain code > silently coping with this -ENOSYS: > > data->gpiod = gpiod_getptional(...); > if (IS_ERR(data->gpiod)) { > error = PTR_ERR(data->gpiod); > if (error != -ENOSYS) > return error; > > data->gpiod = NULL; /* This GPIO _is_ optional */ Do you agree that this is wrong? Yes, it behaves right for most cases. But there are cases where it behaves wrong and so it needs fixing. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 19:10 ` Uwe Kleine-König @ 2017-03-23 19:58 ` Dmitry Torokhov 2017-03-24 8:00 ` Uwe Kleine-König 2017-03-24 8:58 ` Linus Walleij 1 sibling, 1 reply; 38+ messages in thread From: Dmitry Torokhov @ 2017-03-23 19:58 UTC (permalink / raw) To: Uwe Kleine-König Cc: Linus Walleij, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Thu, Mar 23, 2017 at 08:10:20PM +0100, Uwe Kleine-König wrote: > On Thu, Mar 23, 2017 at 08:44:41AM -0700, Dmitry Torokhov wrote: > > On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote: > > > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote: > > > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > > Maybe we can make gpiod_get_optional look like this: > > > > > > > > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB)) > > > > > return NULL; > > > > > else > > > > > return -ENOSYS; > > > > > > > > > > I don't know how isnt_a_acpi_device looks like, probably it involves > > > > > CONFIG_ACPI and/or dev->acpi_node. > > > > > > > > > > This should be safe and still comfortable for legacy platforms, isn't it? > > > > > > > > I like the looks of this. > > > > > > > > Can we revert Dmitry's patch and apply something like this instead? > > > > > > > > Dmitry, how do you feel about this? > > > > > > I frankly do not see the point. It still makes driver code more complex > > Note that this code is in the gpio header, and not in driver code. So > the driver just does > > gpiod = gpiod_get_optional(...) > if (IS_ERR(gpiod)) > return PTR_ERR(gpiod); > > (as it is supposed to do now). I think that's nice. Except that it breaks if !CONFIG_GPIOLIB which is legitimate config in many cases. Can I have a DT platform or ACPI platform that does not expose any GPIOs for kernel to control and thus want to disable GPIOLIB? I can't see why not. > > > > for no good reason. I also think that not having optional GPIO is not an > > > error, so returning value from error space is not correct. NULL is value > > > from another space altogether. > > It seems you didn't understand my concern. Likewise. > > > > Uwe seems to be concerned about case that I find extremely unlikely. We > > > are talking about a system that does not have GPIO support and behaves > > > just fine, with the exception that it actually has (physically) a > > > *single* GPIO, and that GPIO happens to be optional in a single driver, > > > but in this particular system is actually needed (but that need > > > manifests in a non-obvious way). And we have system integrator that has > > > no idea what they are doing (no schematic, etc). > > IMHO this is not an excuse to have lax error checking. IMO it is not an error altogether, so there is no lax checking. > > > > I think that if there is one optional GPIO there will be mandatiry GPIOs > > > in such system as well and selection of GPIOLIB will be forced early on > > > in board bringup. > > > > One more thing: if we keep reporting -ENOSYS in case of !CONFIG_GPIOLIB, > > then most of the non platform-sepcific drivers will eventually gain code > > silently coping with this -ENOSYS: > > > > data->gpiod = gpiod_getptional(...); > > if (IS_ERR(data->gpiod)) { > > error = PTR_ERR(data->gpiod); > > if (error != -ENOSYS) > > return error; > > > > data->gpiod = NULL; /* This GPIO _is_ optional */ > > Do you agree that this is wrong? Yes, it behaves right for most cases. > But there are cases where it behaves wrong and so it needs fixing. I think by now it should be obvious that I do not find it wrong. In fact that is what I, as a maintainer of drivers that supposed to work on multitude of platforms, will be forced to do, if the change to stop reporting -ENOSYS gets reverted. A generic driver has no way to know that kernel configuration or firmware configuration is wrong and should not be trusted (except for piling up horrendous DMI hacks in some cases on X86). Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 19:58 ` Dmitry Torokhov @ 2017-03-24 8:00 ` Uwe Kleine-König 2017-03-24 8:29 ` Geert Uytterhoeven 0 siblings, 1 reply; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-24 8:00 UTC (permalink / raw) To: Dmitry Torokhov Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Linus Walleij, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, Geert Uytterhoeven, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hello Dmitry, On Thu, Mar 23, 2017 at 12:58:04PM -0700, Dmitry Torokhov wrote: > On Thu, Mar 23, 2017 at 08:10:20PM +0100, Uwe Kleine-König wrote: > > On Thu, Mar 23, 2017 at 08:44:41AM -0700, Dmitry Torokhov wrote: > > > On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote: > > > > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote: > > > > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König > > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > > > > Maybe we can make gpiod_get_optional look like this: > > > > > > > > > > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB)) > > > > > > return NULL; > > > > > > else > > > > > > return -ENOSYS; > > > > > > > > > > > > I don't know how isnt_a_acpi_device looks like, probably it involves > > > > > > CONFIG_ACPI and/or dev->acpi_node. > > > > > > > > > > > > This should be safe and still comfortable for legacy platforms, isn't it? > > > > > > > > > > I like the looks of this. > > > > > > > > > > Can we revert Dmitry's patch and apply something like this instead? > > > > > > > > > > Dmitry, how do you feel about this? > > > > > > > > I frankly do not see the point. It still makes driver code more complex > > > > Note that this code is in the gpio header, and not in driver code. So > > the driver just does > > > > gpiod = gpiod_get_optional(...) > > if (IS_ERR(gpiod)) > > return PTR_ERR(gpiod); > > > > (as it is supposed to do now). I think that's nice. > > Except that it breaks if !CONFIG_GPIOLIB which is legitimate config in > many cases. Can I have a DT platform or ACPI platform that does not > expose any GPIOs for kernel to control and thus want to disable GPIOLIB? > I can't see why not. > > > > > > > for no good reason. I also think that not having optional GPIO is not an > > > > error, so returning value from error space is not correct. NULL is value > > > > from another space altogether. > > > > It seems you didn't understand my concern. > > Likewise. OK, lets agree that we don't agree then. You think that if someone disabled GPIOLIB it should be safe to assume that there is no GPIO, I think that's wrong. Still I think it should be possible that we agree on my suggestion to return NULL in some cases only. Here is a prototype (not even compile tested without GPIOLIB): ------->8-------- From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled People disagree if gpiod_get_optional should return NULL or ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that the person who decided to disable GPIOLIB is assumed to know that there is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might introduce hard to debug problems if that decision is wrong. So this patch introduces a compromise and let gpiod_get_optional (and its variants) return NULL if the device in question cannot have an associated GPIO because it is neither instantiated by a device tree nor by ACPI. This should handle most cases that are argued about. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 9 deletions(-) diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index fb0fde686cb1..0ca29889290d 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev, return ERR_PTR(-ENOSYS); } -static inline struct gpio_desc *__must_check -gpiod_get_optional(struct device *dev, const char *con_id, - enum gpiod_flags flags) +static inline bool __gpiod_no_optional_possible(struct device *dev) { - return ERR_PTR(-ENOSYS); + /* + * gpiod_get_optional et al can only provide a GPIO if at least one of + * the backends for specifing a GPIO is available. These are device + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if + * GPIOLIB is disabled (which is the case here). + * So if the provided device is unrelated to device tree and ACPI, we + * can be sure that there is no optional GPIO and let gpiod_get_optional + * safely return NULL. + * Otherwise there is still a chance that there is no GPIO but we cannot + * be sure without having to enable a part of GPIOLIB (i.e. the lookup + * part). So lets play safe and return an error. (Though there are also + * arguments that returning NULL then would be beneficial.) + */ + + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) + return false; + + if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_COMPANION(dev)) + return false; + + return true; } static inline struct gpio_desc *__must_check gpiod_get_index_optional(struct device *dev, const char *con_id, unsigned int index, enum gpiod_flags flags) { + if (__gpiod_no_optional_possible(dev)) + return NULL; + return ERR_PTR(-ENOSYS); } +static inline struct gpio_desc *__must_check +gpiod_get_optional(struct device *dev, const char *con_id, + enum gpiod_flags flags) +{ + return gpiod_get_index_optional(dev, con_id, 0, flags); +} + static inline struct gpio_descs *__must_check gpiod_get_array(struct device *dev, const char *con_id, enum gpiod_flags flags) @@ -186,6 +214,9 @@ static inline struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev, const char *con_id, enum gpiod_flags flags) { + if (__gpiod_no_optional_possible(dev)) + return NULL; + return ERR_PTR(-ENOSYS); } @@ -223,17 +254,20 @@ devm_gpiod_get_index(struct device *dev, } static inline struct gpio_desc *__must_check -devm_gpiod_get_optional(struct device *dev, const char *con_id, - enum gpiod_flags flags) +devm_gpiod_get_index_optional(struct device *dev, const char *con_id, + unsigned int index, enum gpiod_flags flags) { + if (__gpiod_no_optional_possible(dev)) + return NULL; + return ERR_PTR(-ENOSYS); } static inline struct gpio_desc *__must_check -devm_gpiod_get_index_optional(struct device *dev, const char *con_id, - unsigned int index, enum gpiod_flags flags) +devm_gpiod_get_optional(struct device *dev, const char *con_id, + enum gpiod_flags flags) { - return ERR_PTR(-ENOSYS); + return devm_gpiod_get_index_optional(dev, con_id, 0, flags); } static inline struct gpio_descs *__must_check @@ -247,6 +281,9 @@ static inline struct gpio_descs *__must_check devm_gpiod_get_array_optional(struct device *dev, const char *con_id, enum gpiod_flags flags) { + if (__gpiod_no_optional_possible(dev)) + return NULL; + return ERR_PTR(-ENOSYS); } Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-24 8:00 ` Uwe Kleine-König @ 2017-03-24 8:29 ` Geert Uytterhoeven 2017-03-24 8:39 ` Uwe Kleine-König 0 siblings, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-24 8:29 UTC (permalink / raw) To: Uwe Kleine-König Cc: Boris Brezillon, Yoshinori Sato, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Linus Walleij, Dmitry Torokhov, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hi Uwe, On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled > > People disagree if gpiod_get_optional should return NULL or > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that > the person who decided to disable GPIOLIB is assumed to know that there > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might > introduce hard to debug problems if that decision is wrong. > > So this patch introduces a compromise and let gpiod_get_optional (and > its variants) return NULL if the device in question cannot have an > associated GPIO because it is neither instantiated by a device tree nor > by ACPI. > > This should handle most cases that are argued about. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++------- > 1 file changed, 46 insertions(+), 9 deletions(-) > > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > index fb0fde686cb1..0ca29889290d 100644 > --- a/include/linux/gpio/consumer.h > +++ b/include/linux/gpio/consumer.h > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev, > return ERR_PTR(-ENOSYS); > } > > -static inline struct gpio_desc *__must_check > -gpiod_get_optional(struct device *dev, const char *con_id, > - enum gpiod_flags flags) > +static inline bool __gpiod_no_optional_possible(struct device *dev) > { > - return ERR_PTR(-ENOSYS); > + /* > + * gpiod_get_optional et al can only provide a GPIO if at least one of > + * the backends for specifing a GPIO is available. These are device > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if > + * GPIOLIB is disabled (which is the case here). > + * So if the provided device is unrelated to device tree and ACPI, we > + * can be sure that there is no optional GPIO and let gpiod_get_optional > + * safely return NULL. > + * Otherwise there is still a chance that there is no GPIO but we cannot > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup > + * part). So lets play safe and return an error. (Though there are also > + * arguments that returning NULL then would be beneficial.) > + */ > + > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) > + return false; At first sight, I though this was OK: 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y. 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y, and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do not use DT (yet), the check for dev->of_node (false) should handle that. 3. However, I managed to do the same for h8300, which does use DT. Hence if mctrl_gpio would start relying on gpiod_get_optional(), this would break the sh-sci driver on h8300 :-( Note that h8300 doesn't have any GPIO drivers (yet?), so CONFIG_GPIPOLIB=n makes perfect sense! So I'm afraid the only option is to always return NULL, and put the responsability on the shoulders of the system integrator... > + if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_COMPANION(dev)) > + return false; No comments about the ACPI case. > static inline struct gpio_desc *__must_check > gpiod_get_index_optional(struct device *dev, const char *con_id, > unsigned int index, enum gpiod_flags flags) > { > + if (__gpiod_no_optional_possible(dev)) > + return NULL; > + > return ERR_PTR(-ENOSYS); Regardless of the above, given you use the exact same construct in four locations, what about letting __gpiod_no_optional_possible() return the NULL or ERR_PTR itself, and renaming it to e.g. __gpiod_no_optional_return_value()? 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-24 8:29 ` Geert Uytterhoeven @ 2017-03-24 8:39 ` Uwe Kleine-König 2017-03-24 8:59 ` Geert Uytterhoeven 0 siblings, 1 reply; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-24 8:39 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Dmitry Torokhov, Linus Walleij, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org, Yoshinori Sato Hello Geert, On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote: > On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled > > > > People disagree if gpiod_get_optional should return NULL or > > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that > > the person who decided to disable GPIOLIB is assumed to know that there > > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might > > introduce hard to debug problems if that decision is wrong. > > > > So this patch introduces a compromise and let gpiod_get_optional (and > > its variants) return NULL if the device in question cannot have an > > associated GPIO because it is neither instantiated by a device tree nor > > by ACPI. > > > > This should handle most cases that are argued about. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 46 insertions(+), 9 deletions(-) > > > > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > > index fb0fde686cb1..0ca29889290d 100644 > > --- a/include/linux/gpio/consumer.h > > +++ b/include/linux/gpio/consumer.h > > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev, > > return ERR_PTR(-ENOSYS); > > } > > > > -static inline struct gpio_desc *__must_check > > -gpiod_get_optional(struct device *dev, const char *con_id, > > - enum gpiod_flags flags) > > +static inline bool __gpiod_no_optional_possible(struct device *dev) > > { > > - return ERR_PTR(-ENOSYS); > > + /* > > + * gpiod_get_optional et al can only provide a GPIO if at least one of > > + * the backends for specifing a GPIO is available. These are device > > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if > > + * GPIOLIB is disabled (which is the case here). > > + * So if the provided device is unrelated to device tree and ACPI, we > > + * can be sure that there is no optional GPIO and let gpiod_get_optional > > + * safely return NULL. > > + * Otherwise there is still a chance that there is no GPIO but we cannot > > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup > > + * part). So lets play safe and return an error. (Though there are also > > + * arguments that returning NULL then would be beneficial.) > > + */ > > + > > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) > > + return false; > > At first sight, I though this was OK: > > 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y. > > 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y, > and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do > not use DT (yet), the check for dev->of_node (false) should handle > that. > > 3. However, I managed to do the same for h8300, which does use DT. Hence > if mctrl_gpio would start relying on gpiod_get_optional(), this would > break the sh-sci driver on h8300 :-( > Note that h8300 doesn't have any GPIO drivers (yet?), so > CONFIG_GPIPOLIB=n makes perfect sense! Thanks for your efforts. > So I'm afraid the only option is to always return NULL, and put the > responsability on the shoulders of the system integrator... The gpio lines could be provided by an i2c gpio adapter, right? So IMHO you don't need platform gpios to justify -ENODEV. So I guess that's a case where we don't come to an agreement. > > + if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_COMPANION(dev)) > > + return false; > > No comments about the ACPI case. > > > static inline struct gpio_desc *__must_check > > gpiod_get_index_optional(struct device *dev, const char *con_id, > > unsigned int index, enum gpiod_flags flags) > > { > > + if (__gpiod_no_optional_possible(dev)) > > + return NULL; > > + > > return ERR_PTR(-ENOSYS); > > Regardless of the above, given you use the exact same construct in four > locations, what about letting __gpiod_no_optional_possible() return the NULL > or ERR_PTR itself, and renaming it to e.g. __gpiod_no_optional_return_value()? I thought about that but didn't find a good name and so considered it more clear this way. Another optimisation would be to unconditionally define get_optional in terms of get_index_optional which would simplify my patch a bit. I'd consider __gpiod_optional_return_value a better name than __gpiod_no_optional_return_value but I'm still not convinced. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-24 8:39 ` Uwe Kleine-König @ 2017-03-24 8:59 ` Geert Uytterhoeven 2017-03-24 9:15 ` Uwe Kleine-König 0 siblings, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-24 8:59 UTC (permalink / raw) To: Uwe Kleine-König Cc: Dmitry Torokhov, Linus Walleij, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org, Yoshinori Sato Hi Uwe, On Fri, Mar 24, 2017 at 9:39 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote: >> On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled >> > >> > People disagree if gpiod_get_optional should return NULL or >> > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that >> > the person who decided to disable GPIOLIB is assumed to know that there >> > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might >> > introduce hard to debug problems if that decision is wrong. >> > >> > So this patch introduces a compromise and let gpiod_get_optional (and >> > its variants) return NULL if the device in question cannot have an >> > associated GPIO because it is neither instantiated by a device tree nor >> > by ACPI. >> > >> > This should handle most cases that are argued about. >> > >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> > --- >> > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++------- >> > 1 file changed, 46 insertions(+), 9 deletions(-) >> > >> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h >> > index fb0fde686cb1..0ca29889290d 100644 >> > --- a/include/linux/gpio/consumer.h >> > +++ b/include/linux/gpio/consumer.h >> > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev, >> > return ERR_PTR(-ENOSYS); >> > } >> > >> > -static inline struct gpio_desc *__must_check >> > -gpiod_get_optional(struct device *dev, const char *con_id, >> > - enum gpiod_flags flags) >> > +static inline bool __gpiod_no_optional_possible(struct device *dev) >> > { >> > - return ERR_PTR(-ENOSYS); >> > + /* >> > + * gpiod_get_optional et al can only provide a GPIO if at least one of >> > + * the backends for specifing a GPIO is available. These are device >> > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if >> > + * GPIOLIB is disabled (which is the case here). >> > + * So if the provided device is unrelated to device tree and ACPI, we >> > + * can be sure that there is no optional GPIO and let gpiod_get_optional >> > + * safely return NULL. >> > + * Otherwise there is still a chance that there is no GPIO but we cannot >> > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup >> > + * part). So lets play safe and return an error. (Though there are also >> > + * arguments that returning NULL then would be beneficial.) >> > + */ >> > + >> > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) >> > + return false; >> >> At first sight, I though this was OK: >> >> 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y. >> >> 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y, >> and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do >> not use DT (yet), the check for dev->of_node (false) should handle >> that. >> >> 3. However, I managed to do the same for h8300, which does use DT. Hence >> if mctrl_gpio would start relying on gpiod_get_optional(), this would >> break the sh-sci driver on h8300 :-( >> Note that h8300 doesn't have any GPIO drivers (yet?), so >> CONFIG_GPIPOLIB=n makes perfect sense! > > Thanks for your efforts. You're welcome. >> So I'm afraid the only option is to always return NULL, and put the >> responsability on the shoulders of the system integrator... > > The gpio lines could be provided by an i2c gpio adapter, right? So IMHO > you don't need platform gpios to justify -ENODEV. So I guess that's a > case where we don't come to an agreement. While you can enable I2C without further dependencies, no I2C GPIO expander will be offered... unless you have enabled CONFIG_GPIOLIB first. >> > static inline struct gpio_desc *__must_check >> > gpiod_get_index_optional(struct device *dev, const char *con_id, >> > unsigned int index, enum gpiod_flags flags) >> > { >> > + if (__gpiod_no_optional_possible(dev)) >> > + return NULL; >> > + >> > return ERR_PTR(-ENOSYS); >> >> Regardless of the above, given you use the exact same construct in four >> locations, what about letting __gpiod_no_optional_possible() return the NULL >> or ERR_PTR itself, and renaming it to e.g. __gpiod_no_optional_return_value()? > > I thought about that but didn't find a good name and so considered it > more clear this way. Another optimisation would be to unconditionally > define get_optional in terms of get_index_optional which would simplify > my patch a bit. > > I'd consider __gpiod_optional_return_value a better name than > __gpiod_no_optional_return_value but I'm still not convinced. No hard feelings about the name from my side. 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-24 8:59 ` Geert Uytterhoeven @ 2017-03-24 9:15 ` Uwe Kleine-König 2017-03-24 9:44 ` Geert Uytterhoeven 0 siblings, 1 reply; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-24 9:15 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Dmitry Torokhov, Linus Walleij, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org, Yoshinori Sato On Fri, Mar 24, 2017 at 09:59:04AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, > > On Fri, Mar 24, 2017 at 9:39 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote: > >> On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König > >> <u.kleine-koenig@pengutronix.de> wrote: > >> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > >> > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled > >> > > >> > People disagree if gpiod_get_optional should return NULL or > >> > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that > >> > the person who decided to disable GPIOLIB is assumed to know that there > >> > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might > >> > introduce hard to debug problems if that decision is wrong. > >> > > >> > So this patch introduces a compromise and let gpiod_get_optional (and > >> > its variants) return NULL if the device in question cannot have an > >> > associated GPIO because it is neither instantiated by a device tree nor > >> > by ACPI. > >> > > >> > This should handle most cases that are argued about. > >> > > >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > >> > --- > >> > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++------- > >> > 1 file changed, 46 insertions(+), 9 deletions(-) > >> > > >> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > >> > index fb0fde686cb1..0ca29889290d 100644 > >> > --- a/include/linux/gpio/consumer.h > >> > +++ b/include/linux/gpio/consumer.h > >> > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev, > >> > return ERR_PTR(-ENOSYS); > >> > } > >> > > >> > -static inline struct gpio_desc *__must_check > >> > -gpiod_get_optional(struct device *dev, const char *con_id, > >> > - enum gpiod_flags flags) > >> > +static inline bool __gpiod_no_optional_possible(struct device *dev) > >> > { > >> > - return ERR_PTR(-ENOSYS); > >> > + /* > >> > + * gpiod_get_optional et al can only provide a GPIO if at least one of > >> > + * the backends for specifing a GPIO is available. These are device > >> > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if > >> > + * GPIOLIB is disabled (which is the case here). > >> > + * So if the provided device is unrelated to device tree and ACPI, we > >> > + * can be sure that there is no optional GPIO and let gpiod_get_optional > >> > + * safely return NULL. > >> > + * Otherwise there is still a chance that there is no GPIO but we cannot > >> > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup > >> > + * part). So lets play safe and return an error. (Though there are also > >> > + * arguments that returning NULL then would be beneficial.) > >> > + */ > >> > + > >> > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) > >> > + return false; > >> > >> At first sight, I though this was OK: > >> > >> 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y. > >> > >> 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y, > >> and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do > >> not use DT (yet), the check for dev->of_node (false) should handle > >> that. > >> > >> 3. However, I managed to do the same for h8300, which does use DT. Hence > >> if mctrl_gpio would start relying on gpiod_get_optional(), this would > >> break the sh-sci driver on h8300 :-( > >> Note that h8300 doesn't have any GPIO drivers (yet?), so > >> CONFIG_GPIPOLIB=n makes perfect sense! > > > > Thanks for your efforts. > > You're welcome. > > >> So I'm afraid the only option is to always return NULL, and put the > >> responsability on the shoulders of the system integrator... > > > > The gpio lines could be provided by an i2c gpio adapter, right? So IMHO > > you don't need platform gpios to justify -ENODEV. So I guess that's a > > case where we don't come to an agreement. > > While you can enable I2C without further dependencies, no I2C GPIO expander > will be offered... unless you have enabled CONFIG_GPIOLIB first. And that is expected, still the device tree could reference such a GPIO and thus create a situation where Dmitry's and my judgement disagree. So I think my suggestion is the best we could do now. It minimizes the number of cases where we disagree. The next best thing would be to implement that half gpiolib stuff (i.e. do the full lookup to be sure there is no gpio) but this comes at a price: We need some time to implement it and it adds a bit to the kernel size. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-24 9:15 ` Uwe Kleine-König @ 2017-03-24 9:44 ` Geert Uytterhoeven 2017-03-24 10:01 ` Uwe Kleine-König 0 siblings, 1 reply; 38+ messages in thread From: Geert Uytterhoeven @ 2017-03-24 9:44 UTC (permalink / raw) To: Uwe Kleine-König Cc: Dmitry Torokhov, Linus Walleij, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org, Yoshinori Sato Hi Uwe, On Fri, Mar 24, 2017 at 10:15 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Fri, Mar 24, 2017 at 09:59:04AM +0100, Geert Uytterhoeven wrote: >> On Fri, Mar 24, 2017 at 9:39 AM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> > On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote: >> >> On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König >> >> <u.kleine-koenig@pengutronix.de> wrote: >> >> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> >> > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled >> >> > >> >> > People disagree if gpiod_get_optional should return NULL or >> >> > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that >> >> > the person who decided to disable GPIOLIB is assumed to know that there >> >> > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might >> >> > introduce hard to debug problems if that decision is wrong. >> >> > >> >> > So this patch introduces a compromise and let gpiod_get_optional (and >> >> > its variants) return NULL if the device in question cannot have an >> >> > associated GPIO because it is neither instantiated by a device tree nor >> >> > by ACPI. >> >> > >> >> > This should handle most cases that are argued about. >> >> > >> >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> >> > --- >> >> > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++------- >> >> > 1 file changed, 46 insertions(+), 9 deletions(-) >> >> > >> >> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h >> >> > index fb0fde686cb1..0ca29889290d 100644 >> >> > --- a/include/linux/gpio/consumer.h >> >> > +++ b/include/linux/gpio/consumer.h >> >> > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev, >> >> > return ERR_PTR(-ENOSYS); >> >> > } >> >> > >> >> > -static inline struct gpio_desc *__must_check >> >> > -gpiod_get_optional(struct device *dev, const char *con_id, >> >> > - enum gpiod_flags flags) >> >> > +static inline bool __gpiod_no_optional_possible(struct device *dev) >> >> > { >> >> > - return ERR_PTR(-ENOSYS); >> >> > + /* >> >> > + * gpiod_get_optional et al can only provide a GPIO if at least one of >> >> > + * the backends for specifing a GPIO is available. These are device >> >> > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if >> >> > + * GPIOLIB is disabled (which is the case here). >> >> > + * So if the provided device is unrelated to device tree and ACPI, we >> >> > + * can be sure that there is no optional GPIO and let gpiod_get_optional >> >> > + * safely return NULL. >> >> > + * Otherwise there is still a chance that there is no GPIO but we cannot >> >> > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup >> >> > + * part). So lets play safe and return an error. (Though there are also >> >> > + * arguments that returning NULL then would be beneficial.) >> >> > + */ >> >> > + >> >> > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) >> >> > + return false; >> >> >> >> At first sight, I though this was OK: >> >> >> >> 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y. >> >> >> >> 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y, >> >> and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do >> >> not use DT (yet), the check for dev->of_node (false) should handle >> >> that. >> >> >> >> 3. However, I managed to do the same for h8300, which does use DT. Hence >> >> if mctrl_gpio would start relying on gpiod_get_optional(), this would >> >> break the sh-sci driver on h8300 :-( >> >> Note that h8300 doesn't have any GPIO drivers (yet?), so >> >> CONFIG_GPIPOLIB=n makes perfect sense! >> > >> > Thanks for your efforts. >> >> You're welcome. >> >> >> So I'm afraid the only option is to always return NULL, and put the >> >> responsability on the shoulders of the system integrator... >> > >> > The gpio lines could be provided by an i2c gpio adapter, right? So IMHO >> > you don't need platform gpios to justify -ENODEV. So I guess that's a >> > case where we don't come to an agreement. >> >> While you can enable I2C without further dependencies, no I2C GPIO expander >> will be offered... unless you have enabled CONFIG_GPIOLIB first. > > And that is expected, still the device tree could reference such a GPIO > and thus create a situation where Dmitry's and my judgement disagree. If the device tree references such a GPIO, and CONFIG_GPIOLIB is not set, the I2C GPIO expander device will not be bound. Frank Rowand's DT scripts (http://elinux.org/Device_Tree_frowand) will come to the rescue, and inform the user which driver(s) to enable. > So I think my suggestion is the best we could do now. It minimizes the > number of cases where we disagree. The next best thing would be to > implement that half gpiolib stuff (i.e. do the full lookup to be sure > there is no gpio) but this comes at a price: We need some time to > implement it and it adds a bit to the kernel size. So I still have to handle -ENOSYS in sh-sci.c, to avoid regressions... 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-24 9:44 ` Geert Uytterhoeven @ 2017-03-24 10:01 ` Uwe Kleine-König 0 siblings, 0 replies; 38+ messages in thread From: Uwe Kleine-König @ 2017-03-24 10:01 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Boris Brezillon, Yoshinori Sato, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Linus Walleij, Dmitry Torokhov, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Janusz Uzycki, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Hello Geert, On Fri, Mar 24, 2017 at 10:44:50AM +0100, Geert Uytterhoeven wrote: > On Fri, Mar 24, 2017 at 10:15 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Fri, Mar 24, 2017 at 09:59:04AM +0100, Geert Uytterhoeven wrote: > >> On Fri, Mar 24, 2017 at 9:39 AM, Uwe Kleine-König > >> <u.kleine-koenig@pengutronix.de> wrote: > >> > On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote: > >> >> On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König > >> >> <u.kleine-koenig@pengutronix.de> wrote: > >> >> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > >> >> > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled > >> >> > > >> >> > People disagree if gpiod_get_optional should return NULL or > >> >> > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that > >> >> > the person who decided to disable GPIOLIB is assumed to know that there > >> >> > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might > >> >> > introduce hard to debug problems if that decision is wrong. > >> >> > > >> >> > So this patch introduces a compromise and let gpiod_get_optional (and > >> >> > its variants) return NULL if the device in question cannot have an > >> >> > associated GPIO because it is neither instantiated by a device tree nor > >> >> > by ACPI. > >> >> > > >> >> > This should handle most cases that are argued about. > >> >> > > >> >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > >> >> > --- > >> >> > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++------- > >> >> > 1 file changed, 46 insertions(+), 9 deletions(-) > >> >> > > >> >> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > >> >> > index fb0fde686cb1..0ca29889290d 100644 > >> >> > --- a/include/linux/gpio/consumer.h > >> >> > +++ b/include/linux/gpio/consumer.h > >> >> > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev, > >> >> > return ERR_PTR(-ENOSYS); > >> >> > } > >> >> > > >> >> > -static inline struct gpio_desc *__must_check > >> >> > -gpiod_get_optional(struct device *dev, const char *con_id, > >> >> > - enum gpiod_flags flags) > >> >> > +static inline bool __gpiod_no_optional_possible(struct device *dev) > >> >> > { > >> >> > - return ERR_PTR(-ENOSYS); > >> >> > + /* > >> >> > + * gpiod_get_optional et al can only provide a GPIO if at least one of > >> >> > + * the backends for specifing a GPIO is available. These are device > >> >> > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if > >> >> > + * GPIOLIB is disabled (which is the case here). > >> >> > + * So if the provided device is unrelated to device tree and ACPI, we > >> >> > + * can be sure that there is no optional GPIO and let gpiod_get_optional > >> >> > + * safely return NULL. > >> >> > + * Otherwise there is still a chance that there is no GPIO but we cannot > >> >> > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup > >> >> > + * part). So lets play safe and return an error. (Though there are also > >> >> > + * arguments that returning NULL then would be beneficial.) > >> >> > + */ > >> >> > + > >> >> > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) > >> >> > + return false; > >> >> > >> >> At first sight, I though this was OK: > >> >> > >> >> 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y. > >> >> > >> >> 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y, > >> >> and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do > >> >> not use DT (yet), the check for dev->of_node (false) should handle > >> >> that. > >> >> > >> >> 3. However, I managed to do the same for h8300, which does use DT. Hence > >> >> if mctrl_gpio would start relying on gpiod_get_optional(), this would > >> >> break the sh-sci driver on h8300 :-( > >> >> Note that h8300 doesn't have any GPIO drivers (yet?), so > >> >> CONFIG_GPIPOLIB=n makes perfect sense! > >> > >> >> So I'm afraid the only option is to always return NULL, and put the > >> >> responsability on the shoulders of the system integrator... > >> > > >> > The gpio lines could be provided by an i2c gpio adapter, right? So IMHO > >> > you don't need platform gpios to justify -ENODEV. So I guess that's a > >> > case where we don't come to an agreement. > >> > >> While you can enable I2C without further dependencies, no I2C GPIO expander > >> will be offered... unless you have enabled CONFIG_GPIOLIB first. > > > > And that is expected, still the device tree could reference such a GPIO > > and thus create a situation where Dmitry's and my judgement disagree. > > If the device tree references such a GPIO, and CONFIG_GPIOLIB is not set, > the I2C GPIO expander device will not be bound. > Frank Rowand's DT scripts (http://elinux.org/Device_Tree_frowand) will come > to the rescue, and inform the user which driver(s) to enable. > > > So I think my suggestion is the best we could do now. It minimizes the > > number of cases where we disagree. The next best thing would be to > > implement that half gpiolib stuff (i.e. do the full lookup to be sure > > there is no gpio) but this comes at a price: We need some time to > > implement it and it adds a bit to the kernel size. > > So I still have to handle -ENOSYS in sh-sci.c, to avoid regressions... How much would it hurt to enable GPIOLIB there now? I assume that the platform has GPIOs and it's only an intermediate step that there is no driver for it at present? At some point you have to bite the bullet. Given that mctrl_gpio is a usage of GPIOs that might be now? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 19:10 ` Uwe Kleine-König 2017-03-23 19:58 ` Dmitry Torokhov @ 2017-03-24 8:58 ` Linus Walleij 1 sibling, 0 replies; 38+ messages in thread From: Linus Walleij @ 2017-03-24 8:58 UTC (permalink / raw) To: Uwe Kleine-König Cc: Dmitry Torokhov, Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Thu, Mar 23, 2017 at 8:10 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Thu, Mar 23, 2017 at 08:44:41AM -0700, Dmitry Torokhov wrote: >> On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote: >> > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote: >> > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König >> > > <u.kleine-koenig@pengutronix.de> wrote: >> > > >> > > > Maybe we can make gpiod_get_optional look like this: >> > > > >> > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB)) >> > > > return NULL; >> > > > else >> > > > return -ENOSYS; >> > > > >> > > > I don't know how isnt_a_acpi_device looks like, probably it involves >> > > > CONFIG_ACPI and/or dev->acpi_node. >> > > > >> > > > This should be safe and still comfortable for legacy platforms, isn't it? >> > > >> > > I like the looks of this. >> > > >> > > Can we revert Dmitry's patch and apply something like this instead? >> > > >> > > Dmitry, how do you feel about this? >> > >> > I frankly do not see the point. It still makes driver code more complex > > Note that this code is in the gpio header, and not in driver code. So > the driver just does > > gpiod = gpiod_get_optional(...) > if (IS_ERR(gpiod)) > return PTR_ERR(gpiod); > > (as it is supposed to do now). I think that's nice. It does look nice. Compare this to what we must do for optional regulators: st->reg = devm_regulator_get_optional(&spi->dev, "vref"); if (IS_ERR(st->reg)) { /* Any other error indicates that the regulator does exist */ if (PTR_ERR(st->reg) != -ENODEV) return PTR_ERR(st->reg); /* Use internal reference */ } So for optional regulators we get -ENODEV if we don't have it, and then proceed on an alternate path, such as using an internal reference voltage. However the fact that regulators and GPIO optionals are already handled differently is creating "cognitive dissonance" or what I should call it. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 11:11 ` Uwe Kleine-König 2017-03-23 12:03 ` Geert Uytterhoeven @ 2017-03-23 15:55 ` Dmitry Torokhov 1 sibling, 0 replies; 38+ messages in thread From: Dmitry Torokhov @ 2017-03-23 15:55 UTC (permalink / raw) To: Uwe Kleine-König Cc: Geert Uytterhoeven, Linus Walleij, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Thu, Mar 23, 2017 at 12:11:06PM +0100, Uwe Kleine-König wrote: > Hello, > > On Thu, Mar 23, 2017 at 11:20:39AM +0100, Geert Uytterhoeven wrote: > > But having the error breaks setups where the GPIO is optional and does > > not exist. > > so the right way forward is to check harder in the situation where > -ENOSYS was returned before to determine if there is really no GPIO to > be used. "Oh, there are hints that there is no GPIO (GPIOLIB=n), so lets > assume there isn't." is wrong. > > Can we please properly fix the problem instead of papering over it? I think I once already said what would need to _attempt_ to fix it "properly". You would need to implement custom parsing of ACPI tables in GPIOLIB (what if they disable ACPI by mistake?), do the same for OF, call board's manufacturer hotline to ensure that they indeed did not forget to describe GPIOs, etc, etc. Or you could trust that person responsible for selecting kernel configuration has a clue, and if GPIOLIB is disabled it was disabled for a reason. -- Dmitry ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls 2017-03-23 10:10 ` Uwe Kleine-König 2017-03-23 10:20 ` Geert Uytterhoeven @ 2017-03-23 13:37 ` Linus Walleij 1 sibling, 0 replies; 38+ messages in thread From: Linus Walleij @ 2017-03-23 13:37 UTC (permalink / raw) To: Uwe Kleine-König, Dmitry Torokhov Cc: Geert Uytterhoeven, Richard Genoud, Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre, Boris Brezillon, linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Janusz Uzycki, linux-gpio@vger.kernel.org On Thu, Mar 23, 2017 at 11:10 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > So you exchanged many obvious and easy to fix problems with a few hard > ones. I don't agree that's a good idea, but you seem to be willing to > try it. Good luck. I think instead of going to sarcastic remarks you can say you NACK the patch and suggest that it be reverted? The problem I have here as maintainer is that both you and Dmitry are very smart people and I have a great deal of trust invested in both of you. When two valued contributors give me very different advice I get a bit confused and maybe the best option is not to change anything at all right now, and just revert Dmitry's patch. git grep -e 'gpio.*optional(' | wc -l gives 154 use sites outside drivers/gpio, so it is not impossible to fix this if we want a good and strict order to it. I'm just a bit overworked to do it myself right now. What do you all say, is it better to revert Dmitry's patch and instead go around and fix the consumers to do it correctly everywhere, after hammering down the exact semantics? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2017-03-24 10:01 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20170303142201.16452-1-richard.genoud@gmail.com>
[not found] ` <CAMuHMdX-K9B20Tu5NtHJRHmGGuT60OFu8TokZxsF8aSaS7HuHQ@mail.gmail.com>
[not found] ` <20170303191248.wlwjri55ip2x5pgg@pengutronix.de>
[not found] ` <CAMuHMdXFrwa2jCVDb+8ecqgARLG0oGrPp9pLs0b6vEiWeKJ+uA@mail.gmail.com>
[not found] ` <20170303194414.n76tr3oa2mlrzjre@pengutronix.de>
[not found] ` <CAMuHMdXiz5hqW=fm-2H1aNKq_gXD2o0508jD2XjxYUvKoEvHdw@mail.gmail.com>
2017-03-04 17:48 ` [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Uwe Kleine-König
2017-03-06 8:49 ` Geert Uytterhoeven
2017-03-06 8:58 ` Uwe Kleine-König
2017-03-06 9:09 ` Geert Uytterhoeven
2017-03-06 9:30 ` Uwe Kleine-König
2017-03-06 9:53 ` Geert Uytterhoeven
2017-03-06 10:02 ` Uwe Kleine-König
2017-03-14 15:32 ` Linus Walleij
2017-03-16 15:18 ` Linus Walleij
2017-03-16 16:37 ` Uwe Kleine-König
2017-03-16 16:38 ` Geert Uytterhoeven
2017-03-20 9:56 ` Geert Uytterhoeven
2017-03-20 10:03 ` Geert Uytterhoeven
2017-03-20 10:31 ` Uwe Kleine-König
2017-03-20 10:38 ` Geert Uytterhoeven
2017-03-20 11:07 ` Uwe Kleine-König
2017-03-23 9:32 ` Linus Walleij
2017-03-23 10:10 ` Uwe Kleine-König
2017-03-23 10:20 ` Geert Uytterhoeven
2017-03-23 11:11 ` Uwe Kleine-König
2017-03-23 12:03 ` Geert Uytterhoeven
2017-03-23 12:34 ` Uwe Kleine-König
2017-03-23 12:44 ` Geert Uytterhoeven
2017-03-23 13:41 ` Linus Walleij
2017-03-23 14:43 ` Dmitry Torokhov
2017-03-23 15:44 ` Dmitry Torokhov
2017-03-23 19:10 ` Uwe Kleine-König
2017-03-23 19:58 ` Dmitry Torokhov
2017-03-24 8:00 ` Uwe Kleine-König
2017-03-24 8:29 ` Geert Uytterhoeven
2017-03-24 8:39 ` Uwe Kleine-König
2017-03-24 8:59 ` Geert Uytterhoeven
2017-03-24 9:15 ` Uwe Kleine-König
2017-03-24 9:44 ` Geert Uytterhoeven
2017-03-24 10:01 ` Uwe Kleine-König
2017-03-24 8:58 ` Linus Walleij
2017-03-23 15:55 ` Dmitry Torokhov
2017-03-23 13:37 ` Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).