All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Geert Uytterhoeven" <geert@linux-m68k.org>,
	"Richard Genoud" <richard.genoud@gmail.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Geert Uytterhoeven" <geert+renesas@glider.be>,
	"Nicolas Ferre" <nicolas.ferre@microchip.com>,
	"Boris Brezillon" <boris.brezillon@free-electrons.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Janusz Uzycki" <j.uzycki@elproma.com.pl>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>
Subject: Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
Date: Thu, 23 Mar 2017 08:44:41 -0700	[thread overview]
Message-ID: <20170323154441.GA9460@dtor-ws> (raw)
In-Reply-To: <20170323144325.GA952@dtor-ws>

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

WARNING: multiple messages have this Message-ID (diff)
From: dmitry.torokhov@gmail.com (Dmitry Torokhov)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
Date: Thu, 23 Mar 2017 08:44:41 -0700	[thread overview]
Message-ID: <20170323154441.GA9460@dtor-ws> (raw)
In-Reply-To: <20170323144325.GA952@dtor-ws>

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

  reply	other threads:[~2017-03-23 15:44 UTC|newest]

Thread overview: 106+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-03 14:22 [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Richard Genoud
2017-03-03 14:22 ` Richard Genoud
2017-03-03 14:22 ` Richard Genoud
2017-03-03 18:58 ` Geert Uytterhoeven
2017-03-03 18:58   ` Geert Uytterhoeven
2017-03-03 19:12   ` Uwe Kleine-König
2017-03-03 19:12     ` Uwe Kleine-König
2017-03-03 19:12     ` Uwe Kleine-König
2017-03-03 19:21     ` Geert Uytterhoeven
2017-03-03 19:21       ` Geert Uytterhoeven
2017-03-03 19:21       ` Geert Uytterhoeven
2017-03-03 19:44       ` Uwe Kleine-König
2017-03-03 19:44         ` Uwe Kleine-König
2017-03-04 15:35         ` Geert Uytterhoeven
2017-03-04 15:35           ` Geert Uytterhoeven
2017-03-04 17:48           ` Uwe Kleine-König
2017-03-04 17:48             ` Uwe Kleine-König
2017-03-04 17:48             ` Uwe Kleine-König
2017-03-06  8:49             ` Geert Uytterhoeven
2017-03-06  8:49               ` Geert Uytterhoeven
2017-03-06  8:49               ` Geert Uytterhoeven
2017-03-06  8:58               ` Uwe Kleine-König
2017-03-06  8:58                 ` Uwe Kleine-König
2017-03-06  9:09                 ` Geert Uytterhoeven
2017-03-06  9:09                   ` Geert Uytterhoeven
2017-03-06  9:09                   ` Geert Uytterhoeven
2017-03-06  9:30                   ` Uwe Kleine-König
2017-03-06  9:30                     ` Uwe Kleine-König
2017-03-06  9:30                     ` Uwe Kleine-König
2017-03-06  9:53                     ` Geert Uytterhoeven
2017-03-06  9:53                       ` Geert Uytterhoeven
2017-03-06  9:53                       ` Geert Uytterhoeven
2017-03-06 10:02                       ` Uwe Kleine-König
2017-03-06 10:02                         ` Uwe Kleine-König
2017-03-14 15:32                         ` Linus Walleij
2017-03-14 15:32                           ` Linus Walleij
2017-03-14 15:32                           ` Linus Walleij
2017-03-16 15:18                         ` Linus Walleij
2017-03-16 15:18                           ` Linus Walleij
2017-03-16 16:37                           ` Uwe Kleine-König
2017-03-16 16:37                             ` Uwe Kleine-König
2017-03-16 16:37                             ` Uwe Kleine-König
2017-03-16 16:38                           ` Geert Uytterhoeven
2017-03-16 16:38                             ` Geert Uytterhoeven
2017-03-20  9:56                             ` Geert Uytterhoeven
2017-03-20  9:56                               ` Geert Uytterhoeven
2017-03-20  9:56                               ` Geert Uytterhoeven
2017-03-20 10:03                               ` Geert Uytterhoeven
2017-03-20 10:03                                 ` Geert Uytterhoeven
2017-03-20 10:31                               ` Uwe Kleine-König
2017-03-20 10:31                                 ` Uwe Kleine-König
2017-03-20 10:31                                 ` Uwe Kleine-König
2017-03-20 10:38                                 ` Geert Uytterhoeven
2017-03-20 10:38                                   ` Geert Uytterhoeven
2017-03-20 10:38                                   ` Geert Uytterhoeven
2017-03-20 11:07                                   ` Uwe Kleine-König
2017-03-20 11:07                                     ` Uwe Kleine-König
2017-03-20 11:07                                     ` Uwe Kleine-König
2017-03-23  9:32                                     ` Linus Walleij
2017-03-23  9:32                                       ` Linus Walleij
2017-03-23 10:10                                       ` Uwe Kleine-König
2017-03-23 10:10                                         ` Uwe Kleine-König
2017-03-23 10:20                                         ` Geert Uytterhoeven
2017-03-23 10:20                                           ` Geert Uytterhoeven
2017-03-23 11:11                                           ` Uwe Kleine-König
2017-03-23 11:11                                             ` Uwe Kleine-König
2017-03-23 12:03                                             ` Geert Uytterhoeven
2017-03-23 12:03                                               ` Geert Uytterhoeven
2017-03-23 12:03                                               ` Geert Uytterhoeven
2017-03-23 12:34                                               ` Uwe Kleine-König
2017-03-23 12:34                                                 ` Uwe Kleine-König
2017-03-23 12:44                                                 ` Geert Uytterhoeven
2017-03-23 12:44                                                   ` Geert Uytterhoeven
2017-03-23 13:41                                                 ` Linus Walleij
2017-03-23 13:41                                                   ` Linus Walleij
2017-03-23 14:43                                                   ` Dmitry Torokhov
2017-03-23 14:43                                                     ` Dmitry Torokhov
2017-03-23 14:43                                                     ` Dmitry Torokhov
2017-03-23 15:44                                                     ` Dmitry Torokhov [this message]
2017-03-23 15:44                                                       ` Dmitry Torokhov
2017-03-23 19:10                                                       ` Uwe Kleine-König
2017-03-23 19:10                                                         ` Uwe Kleine-König
2017-03-23 19:58                                                         ` Dmitry Torokhov
2017-03-23 19:58                                                           ` Dmitry Torokhov
2017-03-24  8:00                                                           ` Uwe Kleine-König
2017-03-24  8:00                                                             ` Uwe Kleine-König
2017-03-24  8:00                                                             ` Uwe Kleine-König
2017-03-24  8:29                                                             ` Geert Uytterhoeven
2017-03-24  8:29                                                               ` Geert Uytterhoeven
2017-03-24  8:29                                                               ` Geert Uytterhoeven
2017-03-24  8:39                                                               ` Uwe Kleine-König
2017-03-24  8:39                                                                 ` Uwe Kleine-König
2017-03-24  8:59                                                                 ` Geert Uytterhoeven
2017-03-24  8:59                                                                   ` Geert Uytterhoeven
2017-03-24  9:15                                                                   ` Uwe Kleine-König
2017-03-24  9:15                                                                     ` Uwe Kleine-König
2017-03-24  9:44                                                                     ` Geert Uytterhoeven
2017-03-24  9:44                                                                       ` Geert Uytterhoeven
2017-03-24 10:01                                                                       ` Uwe Kleine-König
2017-03-24 10:01                                                                         ` Uwe Kleine-König
2017-03-24  8:58                                                         ` Linus Walleij
2017-03-24  8:58                                                           ` Linus Walleij
2017-03-23 15:55                                             ` Dmitry Torokhov
2017-03-23 15:55                                               ` Dmitry Torokhov
2017-03-23 13:37                                         ` Linus Walleij
2017-03-23 13:37                                           ` Linus Walleij

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170323154441.GA9460@dtor-ws \
    --to=dmitry.torokhov@gmail.com \
    --cc=boris.brezillon@free-electrons.com \
    --cc=geert+renesas@glider.be \
    --cc=geert@linux-m68k.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=j.uzycki@elproma.com.pl \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=nicolas.ferre@microchip.com \
    --cc=richard.genoud@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.