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

Groups > linux.kernel > #1586287 > unrolled thread

Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB  is disabled Linus Walleij <linus.walleij@linaro.org> - 2017-02-22 17:10 +0100
    Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when  GPIOLIB is disabled Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-22 19:30 +0100
      Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when  GPIOLIB is disabled Mark Brown <broonie@kernel.org> - 2017-02-22 20:00 +0100
    Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when  GPIOLIB is disabled Mark Brown <broonie@kernel.org> - 2017-02-22 19:50 +0100

#1586287 — Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

On Mon, Feb 13, 2017 at 2:13 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> Given the intent behind gpiod_get_optional() and friends it does not make
> sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to
> work just fine without gpio so let's behave as if gpio was not found.
> Otherwise we have to special-case -ENOSYS in drivers.
>
> Note that there was objection that someone might forget to enable GPIOLIB
> when dealing with a platform that has device that actually specifies
> optional gpio and we'll break it. I find this unconvincing as that would
> have to be the *only GPIO* in the system, which is extremely unlikely.
>
> (...)
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> This is a resend of a similar patch from a couple years ago.

I want to get an indication from Mark Brown on how he see this
semantic compared to how regulators work.

It makes most sense to developers to have the same kind of
semantics in optional GPIOs as in optional regulators.

For optional regulators, if I understand correctly, these are
*electrically optional* as in: a voltage input pin on a chip that
the chip can very well live without. The regulator may be there,
it may not, it may affect the function of the chip but it's still OK
without it.

For this reason regulators will return an error if an optional regulator
is not present, and the code is expected to deal with it.

It is a common misconception that the "optional" part of the
regulator API call means "software optional". It is not the semantic
of this call.

It is also tagged __must_check and return an error when compiled
out. They return -ENODEV, see <linux/regulator/consumer.h>

I would be happy for a patch switching out return value to -ENODEV
for sure :)

Now second: why should the GPIO semantic be different from what
regulators is using?

Consistency is important with regards to Rusty Russell's API
rules, so we should try to have the same semantic:
http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html

Yours,
Linus Walleij

[toc] | [next] | [standalone]

#1586379 — Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

Hi Linus,

On Wed, Feb 22, 2017 at 05:06:35PM +0100, Linus Walleij wrote:
> On Mon, Feb 13, 2017 at 2:13 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > Given the intent behind gpiod_get_optional() and friends it does not make
> > sense to return -ENOSYS when GPIOLIB is disabled: the driver is expected to
> > work just fine without gpio so let's behave as if gpio was not found.
> > Otherwise we have to special-case -ENOSYS in drivers.
> >
> > Note that there was objection that someone might forget to enable GPIOLIB
> > when dealing with a platform that has device that actually specifies
> > optional gpio and we'll break it. I find this unconvincing as that would
> > have to be the *only GPIO* in the system, which is extremely unlikely.
> >
> > (...)
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >
> > This is a resend of a similar patch from a couple years ago.
> 
> I want to get an indication from Mark Brown on how he see this
> semantic compared to how regulators work.

This is completely orthogonal to this patch though. This patch tries to
solve problem that API behaves differently when GPIOLIB is disabled, so
the driver has to check for both NULL and ENOSYS. I would like to make
gpiod_get_optional() return NULL when GPIOLIB is disabled.

> 
> It makes most sense to developers to have the same kind of
> semantics in optional GPIOs as in optional regulators.

I agree, here. Unfortunately optional regulators return -ENOENT and it
will take an effort to change it to NULL if we can persuade Mark that it
makes sense.

However, there are more optional GPIOs than regulators, so switching
gpio to return -ENODEV even bigger task. Besides, I believe that doing:

	gpio = gpiod_get_optional();
	if (IS_ERR(gpio))
		return PTR_ERR(gpio);

is better than:

	gpio = gpiod_get_optional();
	if ((IS_ERR(gpio)) {
		error = PTR_ERR(gpio);
		if (error != -ENODEV)
			return error;
		gpio = NULL;
	}

> 
> For optional regulators, if I understand correctly, these are
> *electrically optional* as in: a voltage input pin on a chip that
> the chip can very well live without. The regulator may be there,
> it may not, it may affect the function of the chip but it's still OK
> without it.
> 
> For this reason regulators will return an error if an optional regulator
> is not present, and the code is expected to deal with it.
> 
> It is a common misconception that the "optional" part of the
> regulator API call means "software optional". It is not the semantic
> of this call.
> 
> It is also tagged __must_check and return an error when compiled
> out. They return -ENODEV, see <linux/regulator/consumer.h>

The point is that they return the same value when regulator is not
present and when regulator support is compiled out. GPIOLIB returns NULL
and -ENOSYS respectively, and I contend that this is wrong and needs to
be resolved first.

> 
> I would be happy for a patch switching out return value to -ENODEV
> for sure :)

I as shown is the example above I think NULL is better for optionals.

> 
> Now second: why should the GPIO semantic be different from what
> regulators is using?
> 
> Consistency is important with regards to Rusty Russell's API
> rules, so we should try to have the same semantic:
> http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html

I agree that if we can converge on the similar behavior it would be
best, but it is bigger (and separate) task. First I think we need to
make API behave sane when support is enabled and when it is disabled.

Thanks.

-- 
Dmitry

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

#1586421 — Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 22, 2017 at 10:27:25AM -0800, Dmitry Torokhov wrote:

> The point is that they return the same value when regulator is not
> present and when regulator support is compiled out. GPIOLIB returns NULL
> and -ENOSYS respectively, and I contend that this is wrong and needs to
> be resolved first.

FWIW this makes sense to me.

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

#1586399 — Re: [PATCH v2] gpio: return NULL from gpiod_get_optional when GPIOLIB is disabled

[Multipart message — attachments visible in raw view] — view raw

On Wed, Feb 22, 2017 at 05:06:35PM +0100, Linus Walleij wrote:

> For optional regulators, if I understand correctly, these are
> *electrically optional* as in: a voltage input pin on a chip that
> the chip can very well live without. The regulator may be there,
> it may not, it may affect the function of the chip but it's still OK
> without it.

> For this reason regulators will return an error if an optional regulator
> is not present, and the code is expected to deal with it.

Yes.

> It is a common misconception that the "optional" part of the
> regulator API call means "software optional". It is not the semantic
> of this call.

> It is also tagged __must_check and return an error when compiled
> out. They return -ENODEV, see <linux/regulator/consumer.h>

Right.  Dmitry actually sent a patch a few weeks ago proposing a change
in this behaviour for the regulator API which I didn't apply.  I'm
mainly worried that this would the already encourage common broken
patterns with people just not writing error handling code properly and
the incorrect software optional assumption you mention above.  The
expectation is that the majority of drivers with optional regulators
will need to do some explicit handling whenever they would interact with
the regulator.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web