Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1591993 > unrolled thread

[PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls

Back to article view | Back to linux.kernel

Contents

  [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Richard Genoud <richard.genoud@gmail.com> - 2017-03-03 15:40 +0100
    Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-03 20:00 +0100
      Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL  calls Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-03-03 20:20 +0100
        Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-03 20:40 +0100
          Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL  calls Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-03-03 20:50 +0100
            Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-04 16:40 +0100
              Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL  calls Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-03-04 19:10 +0100
                Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-06 10:00 +0100
                  Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL  calls Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-03-06 10:00 +0100
                    Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-06 10:20 +0100
                      Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL  calls Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-03-06 10:40 +0100
                        Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL  calls Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-03-06 11:10 +0100
                        Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-06 11:10 +0100

#1591993 — [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls

Since commit 1d267ea6539f ("serial: mctrl-gpio: simplify init routine"),
the mctrl_gpio_to_gpiod() function can't return an error anymore.
So, just testing for a NULL pointer is ok.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/tty/serial/sh-sci.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 9a47cc4f16a2..36fde6f24040 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1934,12 +1934,12 @@ static unsigned int sci_get_mctrl(struct uart_port *port)
 	if (s->autorts) {
 		if (sci_get_cts(port))
 			mctrl |= TIOCM_CTS;
-	} else if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS))) {
+	} else if (!mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS)) {
 		mctrl |= TIOCM_CTS;
 	}
-	if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_DSR)))
+	if (!mctrl_gpio_to_gpiod(gpios, UART_GPIO_DSR))
 		mctrl |= TIOCM_DSR;
-	if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_DCD)))
+	if (!mctrl_gpio_to_gpiod(gpios, UART_GPIO_DCD))
 		mctrl |= TIOCM_CAR;
 
 	return mctrl;
@@ -3072,10 +3072,8 @@ static int sci_probe_single(struct platform_device *dev,
 		return PTR_ERR(sciport->gpios);
 
 	if (sciport->has_rtscts) {
-		if (!IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(sciport->gpios,
-							UART_GPIO_CTS)) ||
-		    !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(sciport->gpios,
-							UART_GPIO_RTS))) {
+		if (mctrl_gpio_to_gpiod(sciport->gpios, UART_GPIO_CTS) ||
+		    mctrl_gpio_to_gpiod(sciport->gpios, UART_GPIO_RTS)) {
 			dev_err(&dev->dev, "Conflicting RTS/CTS config\n");
 			return -EINVAL;
 		}

[toc] | [next] | [standalone]

#1592223

Hi Richard,

On Fri, Mar 3, 2017 at 3:22 PM, Richard Genoud <richard.genoud@gmail.com> wrote:
> Since commit 1d267ea6539f ("serial: mctrl-gpio: simplify init routine"),
> the mctrl_gpio_to_gpiod() function can't return an error anymore.
> So, just testing for a NULL pointer is ok.

If CONFIG_GPIOLIB=n, mctrl_gpio_to_gpiod() always returns ERR_PTR(-ENOSYS).
That case should be handled correctly, too.

Perhaps mctrl_gpio_to_gpiod() should always return NULL if !CONFIG_GPIOLIB?

This may apply to the other 2 patches, too:
[PATCH 2/4] tty/serial: atmel: remove uneeded IS_ERR_OR_NULL calls
[PATCH 3/4] tty/serial: mxs-auart: remove uneeded IS_ERR_OR_NULL calls

> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  drivers/tty/serial/sh-sci.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 9a47cc4f16a2..36fde6f24040 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1934,12 +1934,12 @@ static unsigned int sci_get_mctrl(struct uart_port *port)
>         if (s->autorts) {
>                 if (sci_get_cts(port))
>                         mctrl |= TIOCM_CTS;
> -       } else if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS))) {
> +       } else if (!mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS)) {
>                 mctrl |= TIOCM_CTS;
>         }
> -       if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_DSR)))
> +       if (!mctrl_gpio_to_gpiod(gpios, UART_GPIO_DSR))
>                 mctrl |= TIOCM_DSR;
> -       if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_DCD)))
> +       if (!mctrl_gpio_to_gpiod(gpios, UART_GPIO_DCD))
>                 mctrl |= TIOCM_CAR;
>
>         return mctrl;
> @@ -3072,10 +3072,8 @@ static int sci_probe_single(struct platform_device *dev,
>                 return PTR_ERR(sciport->gpios);
>
>         if (sciport->has_rtscts) {
> -               if (!IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(sciport->gpios,
> -                                                       UART_GPIO_CTS)) ||
> -                   !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(sciport->gpios,
> -                                                       UART_GPIO_RTS))) {
> +               if (mctrl_gpio_to_gpiod(sciport->gpios, UART_GPIO_CTS) ||
> +                   mctrl_gpio_to_gpiod(sciport->gpios, UART_GPIO_RTS)) {
>                         dev_err(&dev->dev, "Conflicting RTS/CTS config\n");
>                         return -EINVAL;
>                 }

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

[toc] | [prev] | [next] | [standalone]

#1592229 — Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls

On Fri, Mar 03, 2017 at 07:58:36PM +0100, Geert Uytterhoeven wrote:
> Hi Richard,
> 
> On Fri, Mar 3, 2017 at 3:22 PM, Richard Genoud <richard.genoud@gmail.com> wrote:
> > Since commit 1d267ea6539f ("serial: mctrl-gpio: simplify init routine"),
> > the mctrl_gpio_to_gpiod() function can't return an error anymore.
> > So, just testing for a NULL pointer is ok.
> 
> If CONFIG_GPIOLIB=n, mctrl_gpio_to_gpiod() always returns ERR_PTR(-ENOSYS).
> That case should be handled correctly, too.

The correct change to handle this is:

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);
 
 	if (p->capabilities & SCIx_HAVE_RTSCTS) {

Then mctrl_gpio_to_gpiod isn't called. I don't have a machine to test
this, but I think currently this makes the machine barf to continue
here because with sciport->gpios = ERR_PTR(-ENOSYS) calling

	mctrl_gpio_to_gpiod(sciport->gpios, ...)

is a bad idea.
 
> Perhaps mctrl_gpio_to_gpiod() should always return NULL if !CONFIG_GPIOLIB?

No, mctrl_gpio_to_gpiod is right. You are only supposed to call it if
mctrl_gpio_init succeeded.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[toc] | [prev] | [next] | [standalone]

#1592245

Hi Uwe,

On Fri, Mar 3, 2017 at 8:12 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Fri, Mar 03, 2017 at 07:58:36PM +0100, Geert Uytterhoeven wrote:
>> On Fri, Mar 3, 2017 at 3:22 PM, Richard Genoud <richard.genoud@gmail.com> wrote:
>> > Since commit 1d267ea6539f ("serial: mctrl-gpio: simplify init routine"),
>> > the mctrl_gpio_to_gpiod() function can't return an error anymore.
>> > So, just testing for a NULL pointer is ok.
>>
>> If CONFIG_GPIOLIB=n, mctrl_gpio_to_gpiod() always returns ERR_PTR(-ENOSYS).
>> That case should be handled correctly, too.
>
> The correct change to handle this is:
>
> 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.

> Then mctrl_gpio_to_gpiod isn't called. I don't have a machine to test
> this, but I think currently this makes the machine barf to continue
> here because with sciport->gpios = ERR_PTR(-ENOSYS) calling
>
>         mctrl_gpio_to_gpiod(sciport->gpios, ...)
>
> is a bad idea.

If sciport->gpios == ERR_PTR(-ENOSYS), CONFIG_GPIOLIB is not enabled, the
feature is not available, and mctrl_gpio_to_gpiod() will not
dereference the error
pointer.

>> Perhaps mctrl_gpio_to_gpiod() should always return NULL if !CONFIG_GPIOLIB?
>
> No, mctrl_gpio_to_gpiod is right. You are only supposed to call it if
> mctrl_gpio_init succeeded.

Then I have to add checks for sciport->gpios == ERR_PTR(-ENOSYS)...

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

[toc] | [prev] | [next] | [standalone]

#1592256 — Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls

Hello Geert,

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).

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.

So the options are:
 - enable GPIOLIB (maybe enforce that by letting the driver select it)
 - introduce a CONFIG_HALFGPIOLIB that makes gpiod_get_optional and
   mctrl_gpio_init return only then -ENOSYS if there is a gpio specified
   and NULL otherwise.

> > Then mctrl_gpio_to_gpiod isn't called. I don't have a machine to test
> > this, but I think currently this makes the machine barf to continue
> > here because with sciport->gpios = ERR_PTR(-ENOSYS) calling
> >
> >         mctrl_gpio_to_gpiod(sciport->gpios, ...)
> >
> > is a bad idea.
> 
> If sciport->gpios == ERR_PTR(-ENOSYS), CONFIG_GPIOLIB is not enabled, the
> feature is not available, and mctrl_gpio_to_gpiod() will not
> dereference the error
> pointer.

Ah, makes sense.

> >> Perhaps mctrl_gpio_to_gpiod() should always return NULL if !CONFIG_GPIOLIB?
> >
> > No, mctrl_gpio_to_gpiod is right. You are only supposed to call it if
> > mctrl_gpio_init succeeded.
> 
> Then I have to add checks for sciport->gpios == ERR_PTR(-ENOSYS)...

No, ignoring -ENOSYS is wrong.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[toc] | [prev] | [next] | [standalone]

#1592520

Hi Uwe,

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.

> 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.
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.

> So the options are:
>  - enable GPIOLIB (maybe enforce that by letting the driver select it)
>  - introduce a CONFIG_HALFGPIOLIB that makes gpiod_get_optional and
>    mctrl_gpio_init return only then -ENOSYS if there is a gpio specified
>    and NULL otherwise.
>
>> > Then mctrl_gpio_to_gpiod isn't called. I don't have a machine to test
>> > this, but I think currently this makes the machine barf to continue
>> > here because with sciport->gpios = ERR_PTR(-ENOSYS) calling
>> >
>> >         mctrl_gpio_to_gpiod(sciport->gpios, ...)
>> >
>> > is a bad idea.
>>
>> If sciport->gpios == ERR_PTR(-ENOSYS), CONFIG_GPIOLIB is not enabled, the
>> feature is not available, and mctrl_gpio_to_gpiod() will not
>> dereference the error
>> pointer.
>
> Ah, makes sense.
>
>> >> Perhaps mctrl_gpio_to_gpiod() should always return NULL if !CONFIG_GPIOLIB?
>> >
>> > No, mctrl_gpio_to_gpiod is right. You are only supposed to call it if
>> > mctrl_gpio_init succeeded.
>>
>> Then I have to add checks for sciport->gpios == ERR_PTR(-ENOSYS)...
>
> No, ignoring -ENOSYS is wrong.

How else to handle this in a driver that supports both modern (DT && GPIOLIB)
and legacy platforms?

sh-sci is not only used on DT-only architectures like arm, arm64, and h8300,
but also on SuperH, where some platforms use GPIOLIB, others don't, and none
of them use DT yet (jcore doesn't matter, as it doesn't use sh-sci).

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

[toc] | [prev] | [next] | [standalone]

#1592601 — Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls

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/  |

[toc] | [prev] | [next] | [standalone]

#1592997

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

[toc] | [prev] | [next] | [standalone]

#1593001 — Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls

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/  |

[toc] | [prev] | [next] | [standalone]

#1593013

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

[toc] | [prev] | [next] | [standalone]

#1593107 — Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls

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/  |

[toc] | [prev] | [next] | [standalone]

#1593144 — Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls

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/  |

[toc] | [prev] | [next] | [standalone]

#1593147

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web