[PATCH] regulator: core: GPIO #0 is a valid GPIO

[PATCH] regulator: core: GPIO #0 is a valid GPIO

[Dmitry Eremin-Solenikov]

From: Dirk Behme <dirk.behme@de.bosch.com>

With GPIO #0, this if statement will always fail. Remove this, the
check for gpio_is_valid() is sufficent here.

Change-Id: Ia10e8e9c74e11deba852fcd44040cc275b583cfd
Signed-off-by: Gokulkrishnan Nagarajan <Gokulkrishnan.Nagarajan@in.bosch.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 drivers/regulator/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 117f9f8..2f0794f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3524,7 +3524,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	dev_set_drvdata(&rdev->dev, rdev);
 
-	if (config->ena_gpio && gpio_is_valid(config->ena_gpio)) {
+	if (gpio_is_valid(config->ena_gpio)) {
 		ret = regulator_ena_gpio_request(rdev, config);
 		if (ret != 0) {
 			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
-- 
2.1.0.rc1

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

On Fri, Aug 29, 2014 at 10:19:16PM +0400, Dmitry Eremin-Solenikov wrote:
> From: Dirk Behme <dirk.behme@de.bosch.com>
> 
> With GPIO #0, this if statement will always fail. Remove this, the
> check for gpio_is_valid() is sufficent here.

No, read the archives - this will break boards using zero as default.
Any current boards should be using DT and so shouldn't be using fixed
GPIO numbers in the first place which will mean they'll not end up
getting zero as a valid GPIO.  If you are using zero as a GPIO for some
reason provide a way to specify that the GPIO is a real GPIO and not
just the default value for the struct.

> Change-Id: Ia10e8e9c74e11deba852fcd44040cc275b583cfd
> Signed-off-by: Gokulkrishnan Nagarajan <Gokulkrishnan.Nagarajan@in.bosch.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

In any case I can't apply this since you didn't sign it off and please
don't include noise from your internal review system in upstream
postings.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Dirk Behme]

On 29.08.2014 21:01, Mark Brown wrote:
> On Fri, Aug 29, 2014 at 10:19:16PM +0400, Dmitry Eremin-Solenikov wrote:
>> From: Dirk Behme <dirk.behme@de.bosch.com>
>>
>> With GPIO #0, this if statement will always fail. Remove this, the
>> check for gpio_is_valid() is sufficent here.
>
> No, read the archives


Could you kindly give us a pointer to the relevant thread in the archive?


> this will break boards using zero as default.
> Any current boards should be using DT and so shouldn't be using fixed
> GPIO numbers in the first place which will mean they'll not end up
> getting zero as a valid GPIO.


Hmm? What's wrong with a DT entry

<&gpio1 0 0>;

for ena_gpio resulting in zero as a valid GPIO?


> If you are using zero as a GPIO for some
> reason provide a way to specify that the GPIO is a real GPIO and not
> just the default value for the struct.


Do you want to say that GPIO #0 (<&gpio1 0 0>;) isn't a valid GPIO for 
config->ena_gpio?


I wonder how this fits to

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/gpio/gpio-legacy.txt

"GPIOs are identified by unsigned integers in the range 0..MAX_INT"

"If you want to initialize a structure with an invalid GPIO number, use
some negative number (perhaps "-EINVAL");"

then?


Best regards

Dirk

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Geert Uytterhoeven]

On Mon, Sep 1, 2014 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 29.08.2014 21:01, Mark Brown wrote:
>>
>> On Fri, Aug 29, 2014 at 10:19:16PM +0400, Dmitry Eremin-Solenikov wrote:
>>>
>>> From: Dirk Behme <dirk.behme@de.bosch.com>
>>>
>>> With GPIO #0, this if statement will always fail. Remove this, the
>>> check for gpio_is_valid() is sufficent here.
>>
>>
>> No, read the archives
>
> Could you kindly give us a pointer to the relevant thread in the archive?
>
>> this will break boards using zero as default.
>> Any current boards should be using DT and so shouldn't be using fixed
>> GPIO numbers in the first place which will mean they'll not end up
>> getting zero as a valid GPIO.
>
> Hmm? What's wrong with a DT entry
>
> <&gpio1 0 0>;
>
> for ena_gpio resulting in zero as a valid GPIO?
>
>> If you are using zero as a GPIO for some
>> reason provide a way to specify that the GPIO is a real GPIO and not
>> just the default value for the struct.
>
> Do you want to say that GPIO #0 (<&gpio1 0 0>;) isn't a valid GPIO for
> config->ena_gpio?
>
> I wonder how this fits to
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/gpio/gpio-legacy.txt
>
> "GPIOs are identified by unsigned integers in the range 0..MAX_INT"
>
> "If you want to initialize a structure with an invalid GPIO number, use
> some negative number (perhaps "-EINVAL");"
>
> then?

And include/asm-generic/gpio.h:

/* Platforms may implement their GPIO interface with library code,
 * at a small performance cost for non-inlined operations and some
 * extra memory (for code and for per-GPIO table entries).
 *
 * While the GPIO programming interface defines valid GPIO numbers
 * to be in the range 0..MAX_INT, this library restricts them to the
 * smaller range 0..ARCH_NR_GPIOS-1.
 *
 * ARCH_NR_GPIOS is somewhat arbitrary; it usually reflects the sum of
 * builtin/SoC GPIOs plus a number of GPIOs on expanders; the latter is
 * actually an estimate of a board-specific value.
 */

#ifndef ARCH_NR_GPIOS
#define ARCH_NR_GPIOS           256
#endif

/*
 * "valid" GPIO numbers are nonnegative and may be passed to
 * setup routines like gpio_request().  only some valid numbers
 * can successfully be requested and used.
 *
 * Invalid GPIO numbers are useful for indicating no-such-GPIO in
 * platform data and other tables.
 */

static inline bool gpio_is_valid(int number)
{
        return number >= 0 && number < ARCH_NR_GPIOS;
}

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

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

On Mon, Sep 01, 2014 at 09:46:54AM +0200, Dirk Behme wrote:
> On 29.08.2014 21:01, Mark Brown wrote:

> >No, read the archives

> Could you kindly give us a pointer to the relevant thread in the archive?

Not off the top of my head.

> >this will break boards using zero as default.
> >Any current boards should be using DT and so shouldn't be using fixed
> >GPIO numbers in the first place which will mean they'll not end up
> >getting zero as a valid GPIO.

> Hmm? What's wrong with a DT entry

> <&gpio1 0 0>;

> for ena_gpio resulting in zero as a valid GPIO?

If the platform has been converted to DT fully it's only going to happen
if there are exactly as many GPIOs in the system as there are slots in
the GPIO array which is unlikely to happen and trivial to deal with if
it does.  If the platform has been fully converted the GPIO numbers will
be dynamically allocated and the GPIO API starts from the top of the
GPIO range.

> >If you are using zero as a GPIO for some
> >reason provide a way to specify that the GPIO is a real GPIO and not
> >just the default value for the struct.

> Do you want to say that GPIO #0 (<&gpio1 0 0>;) isn't a valid GPIO for
> config->ena_gpio?

No, explicitly specifying GPIO 0 is a problem but that's really only
likely to happen if someone actually asks for it.

> I wonder how this fits to

> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/gpio/gpio-legacy.txt

> "GPIOs are identified by unsigned integers in the range 0..MAX_INT"

> "If you want to initialize a structure with an invalid GPIO number, use
> some negative number (perhaps "-EINVAL");"

> then?

There's no practical way to deploy that without breaking users - as soon
as you treat 0 as a valid GPIO you make all existing users relying on
the natural behaviour of treating 0 as default instantly buggy which is
not practical.  Really the GPIO API is badly specified here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Dmitry Eremin-Solenikov]

On 09/01/2014 02:15 PM, Mark Brown wrote:
> On Mon, Sep 01, 2014 at 09:46:54AM +0200, Dirk Behme wrote:
>> I wonder how this fits to
>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/gpio/gpio-legacy.txt
>
>> "GPIOs are identified by unsigned integers in the range 0..MAX_INT"
>
>> "If you want to initialize a structure with an invalid GPIO number, use
>> some negative number (perhaps "-EINVAL");"
>
>> then?
>
> There's no practical way to deploy that without breaking users - as soon
> as you treat 0 as a valid GPIO you make all existing users relying on
> the natural behaviour of treating 0 as default instantly buggy which is
> not practical.  Really the GPIO API is badly specified here.
>

Back in the time before DTS conversion started, the 0 was a correct GPIO 
number. If somebody wanted to specify that no gpio is provided, he 
provided -1 as an invalid number. I have the feeling that allowing users 
to use 0 as 'no gpio' is a mistake. Or the API should be changed
to disallow GPIO 0 to exist at all.

-- 
With best wishes
Dmitry

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

On Mon, Sep 01, 2014 at 03:59:34PM +0400, Dmitry Eremin-Solenikov wrote:
> On 09/01/2014 02:15 PM, Mark Brown wrote:

> >There's no practical way to deploy that without breaking users - as soon
> >as you treat 0 as a valid GPIO you make all existing users relying on
> >the natural behaviour of treating 0 as default instantly buggy which is
> >not practical.  Really the GPIO API is badly specified here.

> Back in the time before DTS conversion started, the 0 was a correct GPIO
> number. If somebody wanted to specify that no gpio is provided, he provided
> -1 as an invalid number. I have the feeling that allowing users to use 0 as
> 'no gpio' is a mistake. Or the API should be changed
> to disallow GPIO 0 to exist at all.

Please consider my point about making users instantly buggy - it's not
practical to introduce a new field into existing platform data which
needs initialization.  Disallowing the use of 0 as a GPIO seems more
practical here (in that it's something the platform can control with
more reliable coordination).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Dmitry Eremin-Solenikov]

On 09/01/2014 04:06 PM, Mark Brown wrote:
> On Mon, Sep 01, 2014 at 03:59:34PM +0400, Dmitry Eremin-Solenikov wrote:
>> On 09/01/2014 02:15 PM, Mark Brown wrote:
>
>>> There's no practical way to deploy that without breaking users - as soon
>>> as you treat 0 as a valid GPIO you make all existing users relying on
>>> the natural behaviour of treating 0 as default instantly buggy which is
>>> not practical.  Really the GPIO API is badly specified here.
>
>> Back in the time before DTS conversion started, the 0 was a correct GPIO
>> number. If somebody wanted to specify that no gpio is provided, he provided
>> -1 as an invalid number. I have the feeling that allowing users to use 0 as
>> 'no gpio' is a mistake. Or the API should be changed
>> to disallow GPIO 0 to exist at all.
>
> Please consider my point about making users instantly buggy - it's not
> practical to introduce a new field into existing platform data which
> needs initialization.  Disallowing the use of 0 as a GPIO seems more
> practical here (in that it's something the platform can control with
> more reliable coordination).

I think that this makes other users buggy - those, whose platforms use 
GPIO numbering starting from 0. IMX platform does. legacy platforms do.

Consider NO_IRQ which is still defined to -1 on ARM platforms. If a 
device does not have a connected IRQ, it has to explicitly specify -1 
instead of specifying logical default 0. Is it so? What is so different 
with GPIOs and regulators?

-- 
With best wishes
Dmitry

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

On Mon, Sep 01, 2014 at 04:35:27PM +0400, Dmitry Eremin-Solenikov wrote:
> On 09/01/2014 04:06 PM, Mark Brown wrote:

> >Please consider my point about making users instantly buggy - it's not
> >practical to introduce a new field into existing platform data which
> >needs initialization.  Disallowing the use of 0 as a GPIO seems more
> >practical here (in that it's something the platform can control with
> >more reliable coordination).

> I think that this makes other users buggy - those, whose platforms use GPIO
> numbering starting from 0. IMX platform does. legacy platforms do.

It means that they can't use this feature for these specific GPIOs
unless someone writes some code to make it happen either by renumbering
or by adding generic code that provides a way to specify this, it
doesn't make anything worse than it is already.

> Consider NO_IRQ which is still defined to -1 on ARM platforms. If a device
> does not have a connected IRQ, it has to explicitly specify -1 instead of
> specifying logical default 0. Is it so? What is so different with GPIOs and
> regulators?

As I'm getting fed up of pointing out we can't make existing users buggy
- we couldn't do it when we added support for enable GPIOs and we can't
do it now.  Exactly the same problem would occur if adding platform data
to specify an interrupt where none was previously specified (which is
why there's a move away from NO_IRQ being -1).

Please, you need to care about other users.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Dmitry Eremin-Solenikov]

On 09/01/2014 04:49 PM, Mark Brown wrote:
> On Mon, Sep 01, 2014 at 04:35:27PM +0400, Dmitry Eremin-Solenikov wrote:
>> On 09/01/2014 04:06 PM, Mark Brown wrote:
>
>>> Please consider my point about making users instantly buggy - it's not
>>> practical to introduce a new field into existing platform data which
>>> needs initialization.  Disallowing the use of 0 as a GPIO seems more
>>> practical here (in that it's something the platform can control with
>>> more reliable coordination).
>
>> I think that this makes other users buggy - those, whose platforms use GPIO
>> numbering starting from 0. IMX platform does. legacy platforms do.
>
> It means that they can't use this feature for these specific GPIOs
> unless someone writes some code to make it happen either by renumbering
> or by adding generic code that provides a way to specify this, it
> doesn't make anything worse than it is already.
>
>> Consider NO_IRQ which is still defined to -1 on ARM platforms. If a device
>> does not have a connected IRQ, it has to explicitly specify -1 instead of
>> specifying logical default 0. Is it so? What is so different with GPIOs and
>> regulators?
>
> As I'm getting fed up of pointing out we can't make existing users buggy
> - we couldn't do it when we added support for enable GPIOs and we can't
> do it now.  Exactly the same problem would occur if adding platform data
> to specify an interrupt where none was previously specified (which is
> why there's a move away from NO_IRQ being -1).
>
> Please, you need to care about other users.

OK, if the question is about other users, should we review all current 
users that do not specify that gpio, change them to pass -1 and then 
allow valid GPIO#0 as a valid ena_gpio? What do you think about a patch
that will warn for one or two releases that ena_gpio == 0 will change 
meaning in a forthcoming feature?


-- 
With best wishes
Dmitry

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

On Wed, Sep 03, 2014 at 01:04:13PM +0400, Dmitry Eremin-Solenikov wrote:

> OK, if the question is about other users, should we review all current users
> that do not specify that gpio, change them to pass -1 and then allow valid
> GPIO#0 as a valid ena_gpio? What do you think about a patch
> that will warn for one or two releases that ena_gpio == 0 will change
> meaning in a forthcoming feature?

The warning is sensible (I'm sure we used to have one though I can't see
it in the git logs now) but I'm not sure a two release cycle is enough
for everyone to have upgraded and paid attention.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Linus Walleij]

On Mon, Sep 1, 2014 at 1:59 PM, Dmitry Eremin-Solenikov
<dmitry_eremin@mentor.com> wrote:

> Back in the time before DTS conversion started, the 0 was a correct GPIO
> number. If somebody wanted to specify that no gpio is provided, he provided
> -1 as an invalid number. I have the feeling that allowing users to use 0 as
> 'no gpio' is a mistake. Or the API should be changed
> to disallow GPIO 0 to exist at all.

The API change that is going on is to switch platforms and subsystems
to only deal with GPIO descriptors, which are pointer cookies such as
is regulators and clocks. See Documentation/gpio/*

Fixing the old global GPIO numberspace API is a waste of time IMO,
it is just restricted by design and we want to get rid of it.

Yours,
Linus Walleij

Re: [PATCH] regulator: core: GPIO #0 is a valid GPIO

[Linus Walleij]

On Mon, Sep 1, 2014 at 9:46 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 29.08.2014 21:01, Mark Brown wrote:

>> Any current boards should be using DT and so shouldn't be using fixed
>> GPIO numbers in the first place which will mean they'll not end up
>> getting zero as a valid GPIO.
>
> Hmm? What's wrong with a DT entry
>
> <&gpio1 0 0>;
>
> for ena_gpio resulting in zero as a valid GPIO?

I don't know if it's relevant to the discussion but that is
not GPIO #0 in the global GPIO numberspace, it is
*offset* zero on some gpio_chip named gpio1.

At probe time that chip is given some base offset
and offset 0 becomes the global GPIO number
(base+offset) so if that GPIO chip starts from 128
this indicates global GPIO number 128.

Or something totally different. It's a Linux-specific
implementation detail, all DT GPIO notations are
relative offsets.

Yours,
Linus Walleij