[PATCH v2 0/3] gpiolib: Initializing GPIOs using DT property gpio-initval

[PATCH v2 0/3] gpiolib: Initializing GPIOs using DT property gpio-initval

[Markus Pargmann]

Hi,

This is basically a resend with the devicetree mailinglist added to CC.

This series adds a gpio-initval property to the devicetree. It provides a way
to initialize GPIOs to a defined value.

There are 3 patches. The first two simplify the function gpiod_hog. The third
reuses the gpiod_hog()_code for a new gpiod_initialize() and adds the parsing
of the DT.

This series is based on the series
"[PATCH v3 0/9] gpiolib: Add GPIO name support".

Best regards,

Markus


Changes in v2:
 - Fixed Signed-off-by/author


Markus Pargmann (3):
  gpio: Use __gpiod_request directly
  gpiolib: gpiod_hog remove separate name argument
  gpiolib: Add GPIO initialization

 Documentation/devicetree/bindings/gpio/gpio.txt | 29 ++++++----
 drivers/gpio/gpiolib-of.c                       | 11 +++-
 drivers/gpio/gpiolib.c                          | 74 +++++++++++++++++--------
 drivers/gpio/gpiolib.h                          |  6 +-
 4 files changed, 83 insertions(+), 37 deletions(-)

-- 
2.5.0

[PATCH v2 1/3] gpio: Use __gpiod_request directly

[Markus Pargmann]

There is no reason to find out chip and hwnum to use to request a gpio
and get another gpio descriptor. We already have the descriptor we want
to use so we can directly use it.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 79a0b41ce57b..872fdd3617c1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 	      unsigned long lflags, enum gpiod_flags dflags)
 {
-	struct gpio_chip *chip;
-	struct gpio_desc *local_desc;
-	int hwnum;
 	int status;
 
-	chip = gpiod_to_chip(desc);
-	hwnum = gpio_chip_hwgpio(desc);
-
-	local_desc = gpiochip_request_own_desc(chip, hwnum, name);
-	if (IS_ERR(local_desc)) {
+	status = __gpiod_request(desc, name);
+	if (status) {
 		pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
-		       name, chip->label, hwnum);
-		return PTR_ERR(local_desc);
+		       name, gpiod_to_chip(desc)->label,
+		       gpio_chip_hwgpio(desc));
+		return status;
 	}
 
 	status = gpiod_configure_flags(desc, name, lflags, dflags);
 	if (status < 0) {
 		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
-		       name, chip->label, hwnum);
+		       name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
 		gpiochip_free_own_desc(desc);
 		return status;
 	}
-- 
2.5.0

[PATCH v2 1/3] gpio: Use __gpiod_request directly

[Linus Walleij]

On Sun, Aug 30, 2015 at 12:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> There is no reason to find out chip and hwnum to use to request a gpio
> and get another gpio descriptor. We already have the descriptor we want
> to use so we can directly use it.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

Patch applied with Uwe's ACK.

Yours,
Linus Walleij

[PATCH v2 1/3] gpio: Use __gpiod_request directly

[Alexandre Courbot]

On Sun, Aug 30, 2015 at 4:44 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> There is no reason to find out chip and hwnum to use to request a gpio
> and get another gpio descriptor. We already have the descriptor we want
> to use so we can directly use it.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpiolib.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 79a0b41ce57b..872fdd3617c1 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>  int gpiod_hog(struct gpio_desc *desc, const char *name,
>               unsigned long lflags, enum gpiod_flags dflags)
>  {
> -       struct gpio_chip *chip;
> -       struct gpio_desc *local_desc;
> -       int hwnum;
>         int status;
>
> -       chip = gpiod_to_chip(desc);
> -       hwnum = gpio_chip_hwgpio(desc);
> -
> -       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
> -       if (IS_ERR(local_desc)) {
> +       status = __gpiod_request(desc, name);
> +       if (status) {
>                 pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
> -                      name, chip->label, hwnum);
> -               return PTR_ERR(local_desc);
> +                      name, gpiod_to_chip(desc)->label,
> +                      gpio_chip_hwgpio(desc));
> +               return status;
>         }
>
>         status = gpiod_configure_flags(desc, name, lflags, dflags);
>         if (status < 0) {
>                 pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> -                      name, chip->label, hwnum);
> +                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
>                 gpiochip_free_own_desc(desc);

Mmm I should have reviewed this patch earlier, but what bothers me a
bit is that it breaks the symetry that we had by calling
request_own_desc() and free_own_desc() in the failing case (as well as
in gpiochip_free_hogs). And in the end you still need to call
gpiod_to_chip() so I am not sure what the benefit is.

Sure, the code is less verbose, but at the same time it has become
slightly harder to understand. Semantically speaking
"request_own_desc()" is exactly the action we want to convey.
__gpiod_request() is more ambiguous.

Note that this is not a reject, I just wanted to stress that "less
code" is not necessarily the same as "easier to read".

[PATCH v2 1/3] gpio: Use __gpiod_request directly

[Markus Pargmann]

Hi,

On Wed, Sep 23, 2015 at 01:25:28PM +0900, Alexandre Courbot wrote:
> On Sun, Aug 30, 2015 at 4:44 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > There is no reason to find out chip and hwnum to use to request a gpio
> > and get another gpio descriptor. We already have the descriptor we want
> > to use so we can directly use it.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/gpio/gpiolib.c | 17 ++++++-----------
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 79a0b41ce57b..872fdd3617c1 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
> >  int gpiod_hog(struct gpio_desc *desc, const char *name,
> >               unsigned long lflags, enum gpiod_flags dflags)
> >  {
> > -       struct gpio_chip *chip;
> > -       struct gpio_desc *local_desc;
> > -       int hwnum;
> >         int status;
> >
> > -       chip = gpiod_to_chip(desc);
> > -       hwnum = gpio_chip_hwgpio(desc);
> > -
> > -       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
> > -       if (IS_ERR(local_desc)) {
> > +       status = __gpiod_request(desc, name);
> > +       if (status) {
> >                 pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
> > -                      name, chip->label, hwnum);
> > -               return PTR_ERR(local_desc);
> > +                      name, gpiod_to_chip(desc)->label,
> > +                      gpio_chip_hwgpio(desc));
> > +               return status;
> >         }
> >
> >         status = gpiod_configure_flags(desc, name, lflags, dflags);
> >         if (status < 0) {
> >                 pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> > -                      name, chip->label, hwnum);
> > +                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
> >                 gpiochip_free_own_desc(desc);
> 
> Mmm I should have reviewed this patch earlier, but what bothers me a
> bit is that it breaks the symetry that we had by calling
> request_own_desc() and free_own_desc() in the failing case (as well as
> in gpiochip_free_hogs). And in the end you still need to call
> gpiod_to_chip() so I am not sure what the benefit is.

gpiod_to_chip() is only called for errors after this patch. It just
seemed to me as random reader of the code that using
gpiochip_request_own_desc() could be simplified by using __gpiod_request().

> 
> Sure, the code is less verbose, but at the same time it has become
> slightly harder to understand. Semantically speaking
> "request_own_desc()" is exactly the action we want to convey.
> __gpiod_request() is more ambiguous.

At least for me this is slightly better to read as it removes some
unnecessary lines. But that is probably subjective and depends on the way
someone reads code.

Best Regards,

Markus

> 
> Note that this is not a reject, I just wanted to stress that "less
> code" is not necessarily the same as "easier to read".

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150924/df05c08c/attachment.sig>

[PATCH v2 1/3] gpio: Use __gpiod_request directly

[Linus Walleij]

On Tue, Sep 22, 2015 at 9:25 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Sun, Aug 30, 2015 at 4:44 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
>> There is no reason to find out chip and hwnum to use to request a gpio
>> and get another gpio descriptor. We already have the descriptor we want
>> to use so we can directly use it.
>>
>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>> ---
>>  drivers/gpio/gpiolib.c | 17 ++++++-----------
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 79a0b41ce57b..872fdd3617c1 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>>  int gpiod_hog(struct gpio_desc *desc, const char *name,
>>               unsigned long lflags, enum gpiod_flags dflags)
>>  {
>> -       struct gpio_chip *chip;
>> -       struct gpio_desc *local_desc;
>> -       int hwnum;
>>         int status;
>>
>> -       chip = gpiod_to_chip(desc);
>> -       hwnum = gpio_chip_hwgpio(desc);
>> -
>> -       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
>> -       if (IS_ERR(local_desc)) {
>> +       status = __gpiod_request(desc, name);
>> +       if (status) {
>>                 pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
>> -                      name, chip->label, hwnum);
>> -               return PTR_ERR(local_desc);
>> +                      name, gpiod_to_chip(desc)->label,
>> +                      gpio_chip_hwgpio(desc));
>> +               return status;
>>         }
>>
>>         status = gpiod_configure_flags(desc, name, lflags, dflags);
>>         if (status < 0) {
>>                 pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
>> -                      name, chip->label, hwnum);
>> +                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
>>                 gpiochip_free_own_desc(desc);
>
> Mmm I should have reviewed this patch earlier, but what bothers me a
> bit is that it breaks the symetry that we had by calling
> request_own_desc() and free_own_desc() in the failing case (as well as
> in gpiochip_free_hogs). And in the end you still need to call
> gpiod_to_chip() so I am not sure what the benefit is.
>
> Sure, the code is less verbose, but at the same time it has become
> slightly harder to understand. Semantically speaking
> "request_own_desc()" is exactly the action we want to convey.
> __gpiod_request() is more ambiguous.
>
> Note that this is not a reject, I just wanted to stress that "less
> code" is not necessarily the same as "easier to read".

OK I dropped this patch for now.

Markus can you live without this patch for 2/3 and 3/3?

Yours,
Linus Walleij

[PATCH v2 1/3] gpio: Use __gpiod_request directly

[Markus Pargmann]

Hi,

On Thu, Sep 24, 2015 at 10:49:57AM -0700, Linus Walleij wrote:
> On Tue, Sep 22, 2015 at 9:25 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> > On Sun, Aug 30, 2015 at 4:44 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> >> There is no reason to find out chip and hwnum to use to request a gpio
> >> and get another gpio descriptor. We already have the descriptor we want
> >> to use so we can directly use it.
> >>
> >> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> >> ---
> >>  drivers/gpio/gpiolib.c | 17 ++++++-----------
> >>  1 file changed, 6 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >> index 79a0b41ce57b..872fdd3617c1 100644
> >> --- a/drivers/gpio/gpiolib.c
> >> +++ b/drivers/gpio/gpiolib.c
> >> @@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
> >>  int gpiod_hog(struct gpio_desc *desc, const char *name,
> >>               unsigned long lflags, enum gpiod_flags dflags)
> >>  {
> >> -       struct gpio_chip *chip;
> >> -       struct gpio_desc *local_desc;
> >> -       int hwnum;
> >>         int status;
> >>
> >> -       chip = gpiod_to_chip(desc);
> >> -       hwnum = gpio_chip_hwgpio(desc);
> >> -
> >> -       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
> >> -       if (IS_ERR(local_desc)) {
> >> +       status = __gpiod_request(desc, name);
> >> +       if (status) {
> >>                 pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
> >> -                      name, chip->label, hwnum);
> >> -               return PTR_ERR(local_desc);
> >> +                      name, gpiod_to_chip(desc)->label,
> >> +                      gpio_chip_hwgpio(desc));
> >> +               return status;
> >>         }
> >>
> >>         status = gpiod_configure_flags(desc, name, lflags, dflags);
> >>         if (status < 0) {
> >>                 pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> >> -                      name, chip->label, hwnum);
> >> +                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
> >>                 gpiochip_free_own_desc(desc);
> >
> > Mmm I should have reviewed this patch earlier, but what bothers me a
> > bit is that it breaks the symetry that we had by calling
> > request_own_desc() and free_own_desc() in the failing case (as well as
> > in gpiochip_free_hogs). And in the end you still need to call
> > gpiod_to_chip() so I am not sure what the benefit is.
> >
> > Sure, the code is less verbose, but at the same time it has become
> > slightly harder to understand. Semantically speaking
> > "request_own_desc()" is exactly the action we want to convey.
> > __gpiod_request() is more ambiguous.
> >
> > Note that this is not a reject, I just wanted to stress that "less
> > code" is not necessarily the same as "easier to read".
> 
> OK I dropped this patch for now.
> 
> Markus can you live without this patch for 2/3 and 3/3?

Yes, that's fine. I will remove it and rebase the others.

Best Regards,

Markus

> 
> Yours,
> Linus Walleij
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150927/bbc50b3e/attachment.sig>

[PATCH v2 2/3] gpiolib: gpiod_hog remove separate name argument

[Markus Pargmann]

The gpio name is now stored in the gpio descriptor, so we can simply use
that instead of an argument to the function.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib-of.c | 2 +-
 drivers/gpio/gpiolib.c    | 8 ++++----
 drivers/gpio/gpiolib.h    | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 48a7579dd62d..f1fe5123da28 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -232,7 +232,7 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 				continue;
 			}
 
-			if (gpiod_hog(desc, name, lflags, dflags))
+			if (gpiod_hog(desc, lflags, dflags))
 				continue;
 		}
 	}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 872fdd3617c1..f1559fa72c36 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2181,15 +2181,15 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 /**
  * gpiod_hog - Hog the specified GPIO desc given the provided flags
  * @desc:	gpio whose value will be assigned
- * @name:	gpio line name
  * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
  *		of_get_gpio_hog()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
  */
-int gpiod_hog(struct gpio_desc *desc, const char *name,
-	      unsigned long lflags, enum gpiod_flags dflags)
+int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
+	      enum gpiod_flags dflags)
 {
 	int status;
+	const char *name = desc->name;
 
 	status = __gpiod_request(desc, name);
 	if (status) {
@@ -2211,7 +2211,7 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 	set_bit(FLAG_IS_HOGGED, &desc->flags);
 
 	pr_info("GPIO line %d (%s) hogged as %s%s\n",
-		desc_to_gpio(desc), name,
+		desc_to_gpio(desc), desc->name,
 		(dflags&GPIOD_FLAGS_BIT_DIR_OUT) ? "output" : "input",
 		(dflags&GPIOD_FLAGS_BIT_DIR_OUT) ?
 		  (dflags&GPIOD_FLAGS_BIT_DIR_VAL) ? "/high" : "/low":"");
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 78e634d1c719..6c2aeff59f86 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -97,8 +97,8 @@ struct gpio_desc {
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
-int gpiod_hog(struct gpio_desc *desc, const char *name,
-		unsigned long lflags, enum gpiod_flags dflags);
+int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
+	      enum gpiod_flags dflags);
 
 /*
  * Return the GPIO number of the passed descriptor relative to its chip
-- 
2.5.0

[PATCH v2 2/3] gpiolib: gpiod_hog remove separate name argument

[Linus Walleij]

On Sun, Aug 30, 2015 at 12:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> The gpio name is now stored in the gpio descriptor, so we can simply use
> that instead of an argument to the function.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

Several problems with this patch:

- Does not apply to v4.3-rc1
- Fixed that but noted that it just alters the call to gpiod_hog()
  in of_gpiochip_scan_hogs(), there is a local const char *name that
  should be removed too.
- Looking at of_get_gpio_hog() it is unclear to me that .name
  is set in the gpio_desc as desired. Please check this.

  Crucial code looks like this:

    if (name && of_property_read_string(np, "line-name", name))
        *name = np->name;

  i.e. is the line-name really propagated to gpiod->name?
  Are you sure you have tested this with a DTS using line-name
  and verified that it propagates properly?

Yours,
Linus Walleij

[PATCH v2 2/3] gpiolib: gpiod_hog remove separate name argument

[Markus Pargmann]

Hi,

On Mon, Sep 21, 2015 at 04:28:48PM -0700, Linus Walleij wrote:
> On Sun, Aug 30, 2015 at 12:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > The gpio name is now stored in the gpio descriptor, so we can simply use
> > that instead of an argument to the function.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> 
> Several problems with this patch:
> 
> - Does not apply to v4.3-rc1

Yes, a bit older already, will rebase it.

> - Fixed that but noted that it just alters the call to gpiod_hog()
>   in of_gpiochip_scan_hogs(), there is a local const char *name that
>   should be removed too.

The local const char *name is temporarily used in
of_gpiochip_scan_hogs() to get the name from DT and assign it to the
descriptor:
	desc = of_parse_own_gpio(np, &name, &lflags, &dflags);
	...
	else
		desc->name = name;

> - Looking at of_get_gpio_hog() it is unclear to me that .name
>   is set in the gpio_desc as desired. Please check this.
> 
>   Crucial code looks like this:
> 
>     if (name && of_property_read_string(np, "line-name", name))
>         *name = np->name;

of_get_gpio_hog() only parses the properties of the DT. It does not
assign any properties to the descriptor itself. So I kept it this way
and added the code for assignment to of_gpiochip_scan_gpios.

> 
>   i.e. is the line-name really propagated to gpiod->name?
>   Are you sure you have tested this with a DTS using line-name
>   and verified that it propagates properly?

Yes this is tested and works for me. I am using the two series together
without problems.

Best Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150924/4293446e/attachment.sig>

[PATCH v2 2/3] gpiolib: gpiod_hog remove separate name argument

[Linus Walleij]

On Wed, Sep 23, 2015 at 11:39 PM, Markus Pargmann <mpa@pengutronix.de> wrote:

>> - Fixed that but noted that it just alters the call to gpiod_hog()
>>   in of_gpiochip_scan_hogs(), there is a local const char *name that
>>   should be removed too.
>
> The local const char *name is temporarily used in
> of_gpiochip_scan_hogs() to get the name from DT and assign it to the
> descriptor:
>         desc = of_parse_own_gpio(np, &name, &lflags, &dflags);
>         ...
>         else
>                 desc->name = name;

OK the problem is that this series is dependent on the
named GPIOs series. I want this series to stand alone,
because this series is not as controversial, and I want to
merge these initvals first.

Yours,
Linus Walleij

[PATCH v2 2/3] gpiolib: gpiod_hog remove separate name argument

[Markus Pargmann]

On Thu, Sep 24, 2015 at 10:52:55AM -0700, Linus Walleij wrote:
> On Wed, Sep 23, 2015 at 11:39 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> >> - Fixed that but noted that it just alters the call to gpiod_hog()
> >>   in of_gpiochip_scan_hogs(), there is a local const char *name that
> >>   should be removed too.
> >
> > The local const char *name is temporarily used in
> > of_gpiochip_scan_hogs() to get the name from DT and assign it to the
> > descriptor:
> >         desc = of_parse_own_gpio(np, &name, &lflags, &dflags);
> >         ...
> >         else
> >                 desc->name = name;
> 
> OK the problem is that this series is dependent on the
> named GPIOs series. I want this series to stand alone,
> because this series is not as controversial, and I want to
> merge these initvals first.

OK, I think I won't get this completely independent but it shouldn't be
a problem to get this before the controversial patches. Will do that.

Best Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150927/059e5a81/attachment-0001.sig>

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Markus Pargmann]

This functions adds a way to initialize a GPIO without hogging it.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 Documentation/devicetree/bindings/gpio/gpio.txt | 29 +++++++------
 drivers/gpio/gpiolib-of.c                       |  9 ++++
 drivers/gpio/gpiolib.c                          | 55 ++++++++++++++++++++-----
 drivers/gpio/gpiolib.h                          |  2 +
 4 files changed, 73 insertions(+), 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index 5788d5cf1252..55d58983ba43 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -116,29 +116,34 @@ Every GPIO controller node must contain both an empty "gpio-controller"
 property, and a #gpio-cells integer property, which indicates the number of
 cells in a gpio-specifier.
 
-The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
-providing automatic GPIO request and configuration as part of the
-gpio-controller's driver probe function.
+The GPIO chip may contain GPIO definitions. These define properties for single
+GPIOs of this controller.
 
-Each GPIO hog definition is represented as a child node of the GPIO controller.
+GPIO hogging is a mechanism providing automatic GPIO request and configuration
+as part of the gpio-controller's driver probe function.
+
+GPIO initialization provides an automatic initialization to known save values.
+Instead of GPIO hogging the GPIO's value and direction can be modified by other
+users after it was initialized.
+
+Each GPIO definition is represented as a child node of the GPIO controller.
 Required properties:
-- gpio-hog:   A property specifying that this child node represent a GPIO hog.
 - gpios:      Store the GPIO information (id, flags, ...). Shall contain the
 	      number of cells specified in its parent node (GPIO controller
 	      node).
-Only one of the following properties scanned in the order shown below.
-This means that when multiple properties are present they will be searched
-in the order presented below and the first match is taken as the intended
-configuration.
+
+Optional properties:
+- line-name:  The GPIO label name. If not present the node name is used.
+ Only one of gpio-hog and gpio-initval may be specified.
+- gpio-hog:   A property specifying that this child node represent a GPIO hog.
+- gpio-initval: This GPIO should be initialized to the specified configuration.
+ Only one of input, output-low and output-high may be specified:
 - input:      A property specifying to set the GPIO direction as input.
 - output-low  A property specifying to set the GPIO direction as output with
 	      the value low.
 - output-high A property specifying to set the GPIO direction as output with
 	      the value high.
 
-Optional properties:
-- line-name:  The GPIO label name. If not present the node name is used.
-
 Example of two SOC GPIO banks defined as gpio-controller nodes:
 
 	qe_pio_a: gpio-controller at 1400 {
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index f1fe5123da28..ee00c2c63f57 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -234,6 +234,15 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 
 			if (gpiod_hog(desc, lflags, dflags))
 				continue;
+		} else if (of_property_read_bool(np, "gpio-initval")) {
+			if (!dflags) {
+				dev_warn(chip->dev, "GPIO line %d (%s): no initialization state specified, bailing out\n",
+					 desc_to_gpio(desc), np->name);
+				continue;
+			}
+
+			if (gpiod_initialize(desc, lflags, dflags))
+				continue;
 		}
 	}
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f1559fa72c36..d7aa27a92e82 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2178,15 +2178,8 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 
-/**
- * gpiod_hog - Hog the specified GPIO desc given the provided flags
- * @desc:	gpio whose value will be assigned
- * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
- *		of_get_gpio_hog()
- * @dflags:	gpiod_flags - optional GPIO initialization flags
- */
-int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
-	      enum gpiod_flags dflags)
+static int _gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
+		      enum gpiod_flags dflags)
 {
 	int status;
 	const char *name = desc->name;
@@ -2202,11 +2195,31 @@ int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
 	status = gpiod_configure_flags(desc, name, lflags, dflags);
 	if (status < 0) {
 		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
-		       name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
+		       name, gpiod_to_chip(desc)->label,
+		       gpio_chip_hwgpio(desc));
 		gpiochip_free_own_desc(desc);
 		return status;
 	}
 
+	return 0;
+}
+
+/**
+ * gpiod_hog - Hog the specified GPIO desc given the provided flags
+ * @desc:	gpio whose value will be assigned
+ * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
+ *		of_get_gpio_hog()
+ * @dflags:	gpiod_flags - optional GPIO initialization flags
+ */
+int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
+	      enum gpiod_flags dflags)
+{
+	int ret;
+
+	ret = _gpiod_initialize(desc, lflags, dflags);
+	if (ret)
+		return ret;
+
 	/* Mark GPIO as hogged so it can be identified and removed later */
 	set_bit(FLAG_IS_HOGGED, &desc->flags);
 
@@ -2236,6 +2249,28 @@ static void gpiochip_free_hogs(struct gpio_chip *chip)
 }
 
 /**
+ * gpiod_initialize - Initialize a GPIO with a given value
+ * @desc:	gpio whose value will be assigned
+ * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
+ *		of_get_gpio_hog()
+ * @dflags:	gpiod_flags - optional GPIO initialization flags
+ *
+ * This is used to initialize GPIOs that were defined in DT
+ */
+int gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
+		     enum gpiod_flags dflags)
+{
+	int ret;
+
+	ret = _gpiod_initialize(desc, lflags, dflags);
+	if (ret)
+		return ret;
+
+	__gpiod_free(desc);
+	return ret;
+}
+
+/**
  * gpiod_get_array - obtain multiple GPIOs from a multi-index GPIO function
  * @dev:	GPIO consumer, can be NULL for system-global GPIOs
  * @con_id:	function within the GPIO consumer
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 6c2aeff59f86..b6dfa3526e3a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -99,6 +99,8 @@ int gpiod_request(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
 int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
 	      enum gpiod_flags dflags);
+int gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
+		     enum gpiod_flags dflags);
 
 /*
  * Return the GPIO number of the passed descriptor relative to its chip
-- 
2.5.0

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Markus Pargmann]

On Sun, Aug 30, 2015 at 09:44:46AM +0200, Markus Pargmann wrote:
> This functions adds a way to initialize a GPIO without hogging it.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/gpio/gpio.txt | 29 +++++++------
>  drivers/gpio/gpiolib-of.c                       |  9 ++++
>  drivers/gpio/gpiolib.c                          | 55 ++++++++++++++++++++-----
>  drivers/gpio/gpiolib.h                          |  2 +
>  4 files changed, 73 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> index 5788d5cf1252..55d58983ba43 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -116,29 +116,34 @@ Every GPIO controller node must contain both an empty "gpio-controller"
>  property, and a #gpio-cells integer property, which indicates the number of
>  cells in a gpio-specifier.
>  
> -The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> -providing automatic GPIO request and configuration as part of the
> -gpio-controller's driver probe function.
> +The GPIO chip may contain GPIO definitions. These define properties for single
> +GPIOs of this controller.
>  
> -Each GPIO hog definition is represented as a child node of the GPIO controller.
> +GPIO hogging is a mechanism providing automatic GPIO request and configuration
> +as part of the gpio-controller's driver probe function.
> +
> +GPIO initialization provides an automatic initialization to known save values.
> +Instead of GPIO hogging the GPIO's value and direction can be modified by other
> +users after it was initialized.
> +
> +Each GPIO definition is represented as a child node of the GPIO controller.
>  Required properties:
> -- gpio-hog:   A property specifying that this child node represent a GPIO hog.
>  - gpios:      Store the GPIO information (id, flags, ...). Shall contain the
>  	      number of cells specified in its parent node (GPIO controller
>  	      node).
> -Only one of the following properties scanned in the order shown below.
> -This means that when multiple properties are present they will be searched
> -in the order presented below and the first match is taken as the intended
> -configuration.
> +
> +Optional properties:
> +- line-name:  The GPIO label name. If not present the node name is used.
> + Only one of gpio-hog and gpio-initval may be specified.
> +- gpio-hog:   A property specifying that this child node represent a GPIO hog.
> +- gpio-initval: This GPIO should be initialized to the specified configuration.

Any feedback on this new DT binding?

Best Regards,

Markus

> + Only one of input, output-low and output-high may be specified:
>  - input:      A property specifying to set the GPIO direction as input.
>  - output-low  A property specifying to set the GPIO direction as output with
>  	      the value low.
>  - output-high A property specifying to set the GPIO direction as output with
>  	      the value high.
>  
> -Optional properties:
> -- line-name:  The GPIO label name. If not present the node name is used.
> -
>  Example of two SOC GPIO banks defined as gpio-controller nodes:
>  
>  	qe_pio_a: gpio-controller at 1400 {
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index f1fe5123da28..ee00c2c63f57 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -234,6 +234,15 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
>  
>  			if (gpiod_hog(desc, lflags, dflags))
>  				continue;
> +		} else if (of_property_read_bool(np, "gpio-initval")) {
> +			if (!dflags) {
> +				dev_warn(chip->dev, "GPIO line %d (%s): no initialization state specified, bailing out\n",
> +					 desc_to_gpio(desc), np->name);
> +				continue;
> +			}
> +
> +			if (gpiod_initialize(desc, lflags, dflags))
> +				continue;
>  		}
>  	}
>  }
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index f1559fa72c36..d7aa27a92e82 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2178,15 +2178,8 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>  
> -/**
> - * gpiod_hog - Hog the specified GPIO desc given the provided flags
> - * @desc:	gpio whose value will be assigned
> - * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
> - *		of_get_gpio_hog()
> - * @dflags:	gpiod_flags - optional GPIO initialization flags
> - */
> -int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
> -	      enum gpiod_flags dflags)
> +static int _gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
> +		      enum gpiod_flags dflags)
>  {
>  	int status;
>  	const char *name = desc->name;
> @@ -2202,11 +2195,31 @@ int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
>  	status = gpiod_configure_flags(desc, name, lflags, dflags);
>  	if (status < 0) {
>  		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> -		       name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
> +		       name, gpiod_to_chip(desc)->label,
> +		       gpio_chip_hwgpio(desc));
>  		gpiochip_free_own_desc(desc);
>  		return status;
>  	}
>  
> +	return 0;
> +}
> +
> +/**
> + * gpiod_hog - Hog the specified GPIO desc given the provided flags
> + * @desc:	gpio whose value will be assigned
> + * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
> + *		of_get_gpio_hog()
> + * @dflags:	gpiod_flags - optional GPIO initialization flags
> + */
> +int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
> +	      enum gpiod_flags dflags)
> +{
> +	int ret;
> +
> +	ret = _gpiod_initialize(desc, lflags, dflags);
> +	if (ret)
> +		return ret;
> +
>  	/* Mark GPIO as hogged so it can be identified and removed later */
>  	set_bit(FLAG_IS_HOGGED, &desc->flags);
>  
> @@ -2236,6 +2249,28 @@ static void gpiochip_free_hogs(struct gpio_chip *chip)
>  }
>  
>  /**
> + * gpiod_initialize - Initialize a GPIO with a given value
> + * @desc:	gpio whose value will be assigned
> + * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
> + *		of_get_gpio_hog()
> + * @dflags:	gpiod_flags - optional GPIO initialization flags
> + *
> + * This is used to initialize GPIOs that were defined in DT
> + */
> +int gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
> +		     enum gpiod_flags dflags)
> +{
> +	int ret;
> +
> +	ret = _gpiod_initialize(desc, lflags, dflags);
> +	if (ret)
> +		return ret;
> +
> +	__gpiod_free(desc);
> +	return ret;
> +}
> +
> +/**
>   * gpiod_get_array - obtain multiple GPIOs from a multi-index GPIO function
>   * @dev:	GPIO consumer, can be NULL for system-global GPIOs
>   * @con_id:	function within the GPIO consumer
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 6c2aeff59f86..b6dfa3526e3a 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -99,6 +99,8 @@ int gpiod_request(struct gpio_desc *desc, const char *label);
>  void gpiod_free(struct gpio_desc *desc);
>  int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
>  	      enum gpiod_flags dflags);
> +int gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
> +		     enum gpiod_flags dflags);
>  
>  /*
>   * Return the GPIO number of the passed descriptor relative to its chip
> -- 
> 2.5.0
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150921/7262705f/attachment.sig>

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Linus Walleij]

On Sun, Aug 30, 2015 at 12:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> This functions adds a way to initialize a GPIO without hogging it.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

(...)

> -The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> -providing automatic GPIO request and configuration as part of the
> -gpio-controller's driver probe function.
> +The GPIO chip may contain GPIO definitions. These define properties for single
> +GPIOs of this controller.

Insert text like this:

There are two types of GPIO definitions:

- GPIO hogs are ...

- GPIO initializers are ...

This list form is easier to understand.

> -Each GPIO hog definition is represented as a child node of the GPIO controller.
> +GPIO hogging is a mechanism providing automatic GPIO request and configuration
> +as part of the gpio-controller's driver probe function.
> +
> +GPIO initialization provides an automatic initialization to known save values.
> +Instead of GPIO hogging the GPIO's value and direction can be modified by other
> +users after it was initialized.
> +
> +Each GPIO definition is represented as a child node of the GPIO controller.
>  Required properties:
> -- gpio-hog:   A property specifying that this child node represent a GPIO hog.
>  - gpios:      Store the GPIO information (id, flags, ...). Shall contain the
>               number of cells specified in its parent node (GPIO controller
>               node).
> -Only one of the following properties scanned in the order shown below.
> -This means that when multiple properties are present they will be searched
> -in the order presented below and the first match is taken as the intended
> -configuration.
> +
> +Optional properties:
> +- line-name:  The GPIO label name. If not present the node name is used.
> + Only one of gpio-hog and gpio-initval may be specified.

This is confusing. Instead write: "The two following options are
mutually exclusive. One of them must be specified, but not both."

> +- gpio-hog:   A property specifying that this child node represent a GPIO hog.
> +- gpio-initval: This GPIO should be initialized to the specified configuration.

> + Only one of input, output-low and output-high may be specified:

Insert "Of the following arguments, only one..." (etc)

>  - input:      A property specifying to set the GPIO direction as input.
>  - output-low  A property specifying to set the GPIO direction as output with
>               the value low.
>  - output-high A property specifying to set the GPIO direction as output with
>               the value high.
>
> -Optional properties:
> -- line-name:  The GPIO label name. If not present the node name is used.
> -
>  Example of two SOC GPIO banks defined as gpio-controller nodes:

(...)
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -234,6 +234,15 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
>
>                         if (gpiod_hog(desc, lflags, dflags))
>                                 continue;
> +               } else if (of_property_read_bool(np, "gpio-initval")) {
> +                       if (!dflags) {
> +                               dev_warn(chip->dev, "GPIO line %d (%s): no initialization state specified, bailing out\n",
> +                                        desc_to_gpio(desc), np->name);
> +                               continue;
> +                       }
> +
> +                       if (gpiod_initialize(desc, lflags, dflags))
> +                               continue;

We usually do not mix implementations and bindings but it's OK with me.

>                 }

You need a terminating  else {} - clause to warn if neither of gpio-hog
or gpio-initval is specified.

> -int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
> -             enum gpiod_flags dflags)
> +static int _gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
> +                     enum gpiod_flags dflags)

I don't like _underscore functions. Try to find a name that is descriptive
and does not begin with underscore.

What about just gpiod_init()?

>         if (status < 0) {
>                 pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> -                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
> +                      name, gpiod_to_chip(desc)->label,
> +                      gpio_chip_hwgpio(desc));

Looks like a random, unrelated code reshuffling. Don't do this.

Yours,
Linus Walleij

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Markus Pargmann]

Hi,

On Mon, Sep 21, 2015 at 04:42:09PM -0700, Linus Walleij wrote:
> On Sun, Aug 30, 2015 at 12:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > This functions adds a way to initialize a GPIO without hogging it.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> 
> (...)
> 
> > -The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> > -providing automatic GPIO request and configuration as part of the
> > -gpio-controller's driver probe function.
> > +The GPIO chip may contain GPIO definitions. These define properties for single
> > +GPIOs of this controller.
> 
> Insert text like this:
> 
> There are two types of GPIO definitions:
> 
> - GPIO hogs are ...
> 
> - GPIO initializers are ...
> 
> This list form is easier to understand.
> 
> > -Each GPIO hog definition is represented as a child node of the GPIO controller.
> > +GPIO hogging is a mechanism providing automatic GPIO request and configuration
> > +as part of the gpio-controller's driver probe function.
> > +
> > +GPIO initialization provides an automatic initialization to known save values.
> > +Instead of GPIO hogging the GPIO's value and direction can be modified by other
> > +users after it was initialized.
> > +
> > +Each GPIO definition is represented as a child node of the GPIO controller.
> >  Required properties:
> > -- gpio-hog:   A property specifying that this child node represent a GPIO hog.
> >  - gpios:      Store the GPIO information (id, flags, ...). Shall contain the
> >               number of cells specified in its parent node (GPIO controller
> >               node).
> > -Only one of the following properties scanned in the order shown below.
> > -This means that when multiple properties are present they will be searched
> > -in the order presented below and the first match is taken as the intended
> > -configuration.
> > +
> > +Optional properties:
> > +- line-name:  The GPIO label name. If not present the node name is used.
> > + Only one of gpio-hog and gpio-initval may be specified.
> 
> This is confusing. Instead write: "The two following options are
> mutually exclusive. One of them must be specified, but not both."
> 
> > +- gpio-hog:   A property specifying that this child node represent a GPIO hog.
> > +- gpio-initval: This GPIO should be initialized to the specified configuration.
> 
> > + Only one of input, output-low and output-high may be specified:
> 
> Insert "Of the following arguments, only one..." (etc)

Okay, thanks. Will change these.

> 
> >  - input:      A property specifying to set the GPIO direction as input.
> >  - output-low  A property specifying to set the GPIO direction as output with
> >               the value low.
> >  - output-high A property specifying to set the GPIO direction as output with
> >               the value high.
> >
> > -Optional properties:
> > -- line-name:  The GPIO label name. If not present the node name is used.
> > -
> >  Example of two SOC GPIO banks defined as gpio-controller nodes:
> 
> (...)
> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c
> > @@ -234,6 +234,15 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
> >
> >                         if (gpiod_hog(desc, lflags, dflags))
> >                                 continue;
> > +               } else if (of_property_read_bool(np, "gpio-initval")) {
> > +                       if (!dflags) {
> > +                               dev_warn(chip->dev, "GPIO line %d (%s): no initialization state specified, bailing out\n",
> > +                                        desc_to_gpio(desc), np->name);
> > +                               continue;
> > +                       }
> > +
> > +                       if (gpiod_initialize(desc, lflags, dflags))
> > +                               continue;
> 
> We usually do not mix implementations and bindings but it's OK with me.
> 
> >                 }
> 
> You need a terminating  else {} - clause to warn if neither of gpio-hog
> or gpio-initval is specified.

The idea was to have three cases:

1) Just give the gpio a name (desc->name). No hogging or initialization.
2) gpio-hog to initialize and acquire the GPIO for the whole time the
gpiochip is present.
2) gpio-initval to initialize the GPIO to a given value (as gpio-hog
does) but releasing the GPIO afterwards.

> 
> > -int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
> > -             enum gpiod_flags dflags)
> > +static int _gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
> > +                     enum gpiod_flags dflags)
> 
> I don't like _underscore functions. Try to find a name that is descriptive
> and does not begin with underscore.
> 
> What about just gpiod_init()?

Okay, will change.

> 
> >         if (status < 0) {
> >                 pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> > -                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
> > +                      name, gpiod_to_chip(desc)->label,
> > +                      gpio_chip_hwgpio(desc));
> 
> Looks like a random, unrelated code reshuffling. Don't do this.

Right, will remove that.

> 
> Yours,
> Linus Walleij
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150924/51c525a7/attachment-0001.sig>

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Uwe Kleine-König]

Hello,

(Markus left Pengutronix, so I dropped his non-working address from the
recipients and pick up his discussion.)

as far as I see, this topic isn't closed yet. I want something similar,
maybe even a bit more than this patch achieves:

Some gpios should get a fixed direction. If such a gpio is configured as
output, a value should be specified. This is already working today with
gpio-hogs.

Now additionally I want to initialize some gpios but allow them to be
grabbed later. IMHO there are the following new cases:

It should be possible to:

 a) change the value of a gpio initially configured as output
 b) change the direction of a gpio initially configured as output
 c) change the direction of a gpio initially configured as input

IMHO the dts should describe which case should be applied to a given
gpio.

Is it a valid assumption that a gpio that can change direction is also
allowed to change value when configured as output? I assume this in the
following discussion, some details need to change if this shouldn't be
implied. (I think we need a d) then, not sure how this should look
though.)

I'd suggest the following description for these cases:

 a)
 	/*
	 * initially configured as low output. Consumer can do
	 * gpio_set_value(..., 1); but not gpio_direction_input(...);
	 */
 	nodename {
		gpio-hog;
		gpios = <...>;
		output-low-init;
	};

 b)
 	/*
	 * initially configured as low output. Consumer can do
	 * gpio_set_value(..., 1); and gpio_direction_input(...);
	 */
 	nodename {
		gpio-hog;
		gpios = <...>;
		output-init-low;
	};

 c)
 	/*
	 * initially configured as input. Consumer can do
	 * gpio_direction_output(..., ...) and then set the value
	 * freely.
	 */
	 nodename {
	 	gpio-hog;
		gpios = <...>;
		input-init;
	};

I don't like that "output-low-init" and "output-init-low" are so
similar, so if someone suggests a better naming scheme, that would be
great.

Does the idea (maybe apart from the naming) look good in general and/or
compared to Markus' suggestion?

(For those not having Markus' suggestion in mind any more. There we
could use "gpio-initval" instead of "gpio-hog" and then this had the
semantic of b) or c) using "input", "output-low" and "output-high". a)
wasn't possible to formalize.)

Best regards
Uwe

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

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Lothar Waßmann]

Hi,

On Tue, 7 Feb 2017 12:09:50 +0100 Uwe Kleine-K?nig wrote:
> Hello,
> 
> (Markus left Pengutronix, so I dropped his non-working address from the
> recipients and pick up his discussion.)
> 
> as far as I see, this topic isn't closed yet. I want something similar,
> maybe even a bit more than this patch achieves:
> 
> Some gpios should get a fixed direction. If such a gpio is configured as
> output, a value should be specified. This is already working today with
> gpio-hogs.
> 
> Now additionally I want to initialize some gpios but allow them to be
> grabbed later. IMHO there are the following new cases:
> 
> It should be possible to:
> 
>  a) change the value of a gpio initially configured as output
>  b) change the direction of a gpio initially configured as output
>  c) change the direction of a gpio initially configured as input
> 
> IMHO the dts should describe which case should be applied to a given
> gpio.
> 
> Is it a valid assumption that a gpio that can change direction is also
> allowed to change value when configured as output? I assume this in the
> following discussion, some details need to change if this shouldn't be
> implied. (I think we need a d) then, not sure how this should look
> though.)
> 
> I'd suggest the following description for these cases:
> 
>  a)
>  	/*
> 	 * initially configured as low output. Consumer can do
> 	 * gpio_set_value(..., 1); but not gpio_direction_input(...);
> 	 */
>  	nodename {
> 		gpio-hog;
> 		gpios = <...>;
> 		output-low-init;
> 	};
> 
>  b)
>  	/*
> 	 * initially configured as low output. Consumer can do
> 	 * gpio_set_value(..., 1); and gpio_direction_input(...);
> 	 */
>  	nodename {
> 		gpio-hog;
> 		gpios = <...>;
> 		output-init-low;
> 	};
> 
>  c)
>  	/*
> 	 * initially configured as input. Consumer can do
> 	 * gpio_direction_output(..., ...) and then set the value
> 	 * freely.
> 	 */
> 	 nodename {
> 	 	gpio-hog;
> 		gpios = <...>;
> 		input-init;
> 	};
> 
> I don't like that "output-low-init" and "output-init-low" are so
> similar, so if someone suggests a better naming scheme, that would be
> great.
> 
The position of the 'output' or 'input' within the word could
imply whether the direction can be changed lateron or not. E.g.:
output-init-low => an output whose state is initially low, but can be
                   reconfigured to high lateron.

init-low-output => a gpio that is initially configured as output low,  
                   but can subsequently be reconfigured as input or to
                   a different state

init-input      => your case c)
input           => a GPIO input that cannot be reconfigured

IOW: if the pin direction is mentioned in front of the 'init', it
cannot be changed lateron, if it is mentioned after the 'init' it can
be changed. If nothing follows 'init', the 'init' can be omitted.
This would also allow an 'output-low' as a GPIO that is permanently
grounded...

Lothar Wa?mann

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Uwe Kleine-König]

Hello Lothar,

On Tue, Feb 07, 2017 at 02:30:31PM +0100, Lothar Wa?mann wrote:
> On Tue, 7 Feb 2017 12:09:50 +0100 Uwe Kleine-K?nig wrote:
> >  a)
> >  	/*
> > 	 * initially configured as low output. Consumer can do
> > 	 * gpio_set_value(..., 1); but not gpio_direction_input(...);
> > 	 */
> >  	nodename {
> > 		gpio-hog;
> > 		gpios = <...>;
> > 		output-low-init;
> > 	};
> > 
> >  b)
> >  	/*
> > 	 * initially configured as low output. Consumer can do
> > 	 * gpio_set_value(..., 1); and gpio_direction_input(...);
> > 	 */
> >  	nodename {
> > 		gpio-hog;
> > 		gpios = <...>;
> > 		output-init-low;
> > 	};
> > 
> >  c)
> >  	/*
> > 	 * initially configured as input. Consumer can do
> > 	 * gpio_direction_output(..., ...) and then set the value
> > 	 * freely.
> > 	 */
> > 	 nodename {
> > 	 	gpio-hog;
> > 		gpios = <...>;
> > 		input-init;
> > 	};
> > 
> > I don't like that "output-low-init" and "output-init-low" are so
> > similar, so if someone suggests a better naming scheme, that would be
> > great.
> > 
> The position of the 'output' or 'input' within the word could
> imply whether the direction can be changed lateron or not. E.g.:
> output-init-low => an output whose state is initially low, but can be
>                    reconfigured to high lateron.

That's your a) which I called output-low-init

> init-low-output => a gpio that is initially configured as output low,  
>                    but can subsequently be reconfigured as input or to
>                    a different state

That's your b) which I called output-init-low.
 
> init-input      => your case c)
... which I called input-init.

> input           => a GPIO input that cannot be reconfigured

This is just a permutation of words, and so doesn't fix the problem that
a) and b) use similar words. For you it's "init" before the stuff
that can be changed. For me it's "init" is after the stuff that can be
changed. 
I slightly prefer my naming here because we already have "output-low"
and "output-high" and my names also use direction-before-value.

But I interpret your reply as interest and general confirmation that the
idea is fine.

> This would also allow an 'output-low' as a GPIO that is permanently
> grounded...

That works already today.

Best regards
Uwe

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

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Uwe Kleine-König]

Hello,

On Tue, Feb 07, 2017 at 12:09:50PM +0100, Uwe Kleine-K?nig wrote:
> as far as I see, this topic isn't closed yet. I want something similar,
> maybe even a bit more than this patch achieves:
> 
> Some gpios should get a fixed direction. If such a gpio is configured as
> output, a value should be specified. This is already working today with
> gpio-hogs.
> 
> Now additionally I want to initialize some gpios but allow them to be
> grabbed later. IMHO there are the following new cases:
> 
> It should be possible to:
> 
>  a) change the value of a gpio initially configured as output
>  b) change the direction of a gpio initially configured as output
>  c) change the direction of a gpio initially configured as input
> 
> IMHO the dts should describe which case should be applied to a given
> gpio.
> 
> Is it a valid assumption that a gpio that can change direction is also
> allowed to change value when configured as output? I assume this in the
> following discussion, some details need to change if this shouldn't be
> implied. (I think we need a d) then, not sure how this should look
> though.)
> 
> I'd suggest the following description for these cases:
> 
>  a)
>  	/*
> 	 * initially configured as low output. Consumer can do
> 	 * gpio_set_value(..., 1); but not gpio_direction_input(...);
> 	 */
>  	nodename {
> 		gpio-hog;
> 		gpios = <...>;
> 		output-low-init;
> 	};
> 
>  b)
>  	/*
> 	 * initially configured as low output. Consumer can do
> 	 * gpio_set_value(..., 1); and gpio_direction_input(...);
> 	 */
>  	nodename {
> 		gpio-hog;
> 		gpios = <...>;
> 		output-init-low;
> 	};
> 
>  c)
>  	/*
> 	 * initially configured as input. Consumer can do
> 	 * gpio_direction_output(..., ...) and then set the value
> 	 * freely.
> 	 */
> 	 nodename {
> 	 	gpio-hog;
> 		gpios = <...>;
> 		input-init;
> 	};

I thought a bit more about this and the changes that would be necessary
to achieve this:

 - introduce new flags IS_FIXED_DIR and IS_FIXED_VALUE for struct
   gpio_desc::flags and error out accordingly in gpiod_set_direction_*
   and gpiod_set_value respectively. Then we have:

   	- already existing hogs: IS_FIXED_DIR | IS_FIXED_VALUE
	- a) IS_FIXED_DIR
	- b)+c) 0
	- d) (if desired) IS_FIXED_VALUE
 
   d) might need additional bookkeeping to remember the configured value
   when the direction is changed to input.
   These flags must be passed to gpiod_hog, via lflags should work.

 - don't auto-request hogged pins
   Currently hogged pins are not available for consumers to be
   requested. This needs to be changed at least for the three (or four)
   new cases.
   IMHO it is sensible to allow requesting a gpio that is completely
   hogged and just prevent changing direction and (if it's an output)
   value.
   TODO: review places that use IS_REQUESTED for need of adaption
   TODO: race between hog and consumer?

 - teach of_parse_own_gpio about IS_FIXED_DIR and IS_FIXED_VALUE

 - expand gpiolib_dbg_show to show IS_FIXED_DIR and IS_FIXED_VALUE

@Linus: I'd like to have an ok from your side that you like the approach
before working on implementing this. So it would be great if you shared
your thoughts about my ideas.

Best regards
Uwe

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

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Linus Walleij]

On Sat, May 6, 2017 at 10:32 PM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:

>> I'd suggest the following description for these cases:
>>
>>  a)
>>       /*
>>        * initially configured as low output. Consumer can do
>>        * gpio_set_value(..., 1); but not gpio_direction_input(...);
>>        */
>>       nodename {
>>               gpio-hog;
>>               gpios = <...>;
>>               output-low-init;
>>       };
>>
>>  b)
>>       /*
>>        * initially configured as low output. Consumer can do
>>        * gpio_set_value(..., 1); and gpio_direction_input(...);
>>        */
>>       nodename {
>>               gpio-hog;
>>               gpios = <...>;
>>               output-init-low;
>>       };
>>
>>  c)
>>       /*
>>        * initially configured as input. Consumer can do
>>        * gpio_direction_output(..., ...) and then set the value
>>        * freely.
>>        */
>>        nodename {
>>               gpio-hog;
>>               gpios = <...>;
>>               input-init;
>>       };
>
> I thought a bit more about this and the changes that would be necessary
> to achieve this:
>
>  - introduce new flags IS_FIXED_DIR and IS_FIXED_VALUE for struct
>    gpio_desc::flags and error out accordingly in gpiod_set_direction_*
>    and gpiod_set_value respectively. Then we have:
>
>         - already existing hogs: IS_FIXED_DIR | IS_FIXED_VALUE
>         - a) IS_FIXED_DIR
>         - b)+c) 0
>         - d) (if desired) IS_FIXED_VALUE

I don't really understand IS_FIXED_DIR, if that is a property of the
hardware then that is something the driver should be handling,
i.e. the driver must know if a line is fixed direction (only input or only
output). Same for IS_FIXED_VALUE.

Or do you mean it is something else, like these are only software
flags for hogs? Then they should be HOG_IS_FIXED_DIR etc.

>  - don't auto-request hogged pins
>    Currently hogged pins are not available for consumers to be
>    requested. This needs to be changed at least for the three (or four)
>    new cases.

But look at the dictionary definition of "hog":
https://en.wiktionary.org/wiki/hog

The usage here is as in "to hog resources" so:

3. A greedy person; one who refuses to share.

And check the verb form:

1. hog (third-person singular simple present hogs, present participle
  hogging, simple past and past participle hogged)
(transitive) To greedily take more than one's share, to take precedence
 at the expense of another or others.
[quotations]Hey! Quit hogging all the blankets.

So of course hogs request and hold the lines. That is what hogging
is all about.

>    IMHO it is sensible to allow requesting a gpio that is completely
>    hogged and just prevent changing direction and (if it's an output)
>    value.

No it doesn't make sense at all to take something that is hogged
because it is completely unintuitive to the very word "hog".

As I have said before: a new property, something like
"initial-directions" and "initial-values" need to be created and
handled separately for this, without even involving the hogging
mechanism.

You need to introduce something in parallel from the hogs that
does not request the lines, but just sets up initial values and
directions on them and then leave them to be acquired later.

We already keep track of the direction inside the gpio_desc
and we have callbacks to read that up from the hardware if
supported, and I guess the  same goes for initial values.

I just don't see what you need here and why you want to expand
the hog concept beyond its intuitive use?

I say define something new in device tree with an intuitive name,
that does what you want it to do: just sets up initial direction and
optional value (on outputs) using the existing callbacks and then just
gets out of the way. No hogging.

Yours,
Linus Walleij

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Uwe Kleine-König]

Hello Linus,

On Sun, May 07, 2017 at 09:30:26AM +0200, Linus Walleij wrote:
> On Sat, May 6, 2017 at 10:32 PM, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
> 
> >> I'd suggest the following description for these cases:
> >>
> >>  a)
> >>       /*
> >>        * initially configured as low output. Consumer can do
> >>        * gpio_set_value(..., 1); but not gpio_direction_input(...);
> >>        */
> >>       nodename {
> >>               gpio-hog;
> >>               gpios = <...>;
> >>               output-low-init;
> >>       };
> >>
> >>  b)
> >>       /*
> >>        * initially configured as low output. Consumer can do
> >>        * gpio_set_value(..., 1); and gpio_direction_input(...);
> >>        */
> >>       nodename {
> >>               gpio-hog;
> >>               gpios = <...>;
> >>               output-init-low;
> >>       };
> >>
> >>  c)
> >>       /*
> >>        * initially configured as input. Consumer can do
> >>        * gpio_direction_output(..., ...) and then set the value
> >>        * freely.
> >>        */
> >>        nodename {
> >>               gpio-hog;
> >>               gpios = <...>;
> >>               input-init;
> >>       };
> >
> > I thought a bit more about this and the changes that would be necessary
> > to achieve this:
> >
> >  - introduce new flags IS_FIXED_DIR and IS_FIXED_VALUE for struct
> >    gpio_desc::flags and error out accordingly in gpiod_set_direction_*
> >    and gpiod_set_value respectively. Then we have:
> >
> >         - already existing hogs: IS_FIXED_DIR | IS_FIXED_VALUE
> >         - a) IS_FIXED_DIR
> >         - b)+c) 0
> >         - d) (if desired) IS_FIXED_VALUE
> 
> I don't really understand IS_FIXED_DIR, if that is a property of the
> hardware then that is something the driver should be handling,
> i.e. the driver must know if a line is fixed direction (only input or only
> output). Same for IS_FIXED_VALUE.

It's not about the hardware haven an input-only GPIO, but on the board
it might be bad to drive the GPIO because another device already drives
it.

> Or do you mean it is something else, like these are only software
> flags for hogs? Then they should be HOG_IS_FIXED_DIR etc.
> 
> >  - don't auto-request hogged pins
> >    Currently hogged pins are not available for consumers to be
> >    requested. This needs to be changed at least for the three (or four)
> >    new cases.
> 
> But look at the dictionary definition of "hog":
> https://en.wiktionary.org/wiki/hog
> 
> The usage here is as in "to hog resources" so:
> 
> 3. A greedy person; one who refuses to share.
> 
> And check the verb form:
> 
> 1. hog (third-person singular simple present hogs, present participle
>   hogging, simple past and past participle hogged)
> (transitive) To greedily take more than one's share, to take precedence
>  at the expense of another or others.
> [quotations]Hey! Quit hogging all the blankets.
> 
> So of course hogs request and hold the lines. That is what hogging
> is all about.
> 
> >    IMHO it is sensible to allow requesting a gpio that is completely
> >    hogged and just prevent changing direction and (if it's an output)
> >    value.
> 
> No it doesn't make sense at all to take something that is hogged
> because it is completely unintuitive to the very word "hog".

What I want is that it is possible to restrict the usage of a GPIO more
fine-grained. For example only fix the direction to input but allow a
driver to still read out it's value. Or fix the direction to output but
allow changing the level. So the semantic of a hogged gpio as
implemented today is a special case of my new use cases. So it seems
naturally to extend the already existing concept.

And concerning the meaning of hogging: It still takes some freedom from
others, just not everything because you can still request the line. So
not being a native English speaker the word still fits for me.

And technically it is sensible to implement the new use cases and
today's hogging together and so it is also sensible IMHO to use the same
mechanism in the device tree.

> As I have said before: a new property, something like
> "initial-directions" and "initial-values" need to be created and
> handled separately for this, without even involving the hogging
> mechanism.
> 
> You need to introduce something in parallel from the hogs that
> does not request the lines, but just sets up initial values and
> directions on them and then leave them to be acquired later.
> 
> We already keep track of the direction inside the gpio_desc
> and we have callbacks to read that up from the hardware if
> supported, and I guess the  same goes for initial values.
> 
> I just don't see what you need here and why you want to expand
> the hog concept beyond its intuitive use?
> 
> I say define something new in device tree with an intuitive name,
> that does what you want it to do: just sets up initial direction and
> optional value (on outputs) using the existing callbacks and then just
> gets out of the way. No hogging.

So to stick to your suggestion I would have in the end for two lines
io12 that must be 0 for ESD reasons and io13 that drives the RST-line of
a companion DSP that should be kept low until userspace is ready and
then allow being driven high via gpioctl:

	io12 {
		gpio-hog;
		gpio = <4 0>;
		output-low;
	};

	io13 {
		gpio-init;
		gpio = <5 0>;
		output-low;
		fixed-direction;
	};

For me it looks artificial to not use "gpio-hog" for the 2nd
specification but if that is the compromise that we can agree on, that's
ok for me.

Orthogonal to gpio-hog being used for this or not, I like

	output-low;
	fixed-direction;

better than

	output-low-init;

as the former is more intuitive.

Best regards
Uwe

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

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Linus Walleij]

On Sun, May 7, 2017 at 11:45 AM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
> [Me]

>> >    IMHO it is sensible to allow requesting a gpio that is completely
>> >    hogged and just prevent changing direction and (if it's an output)
>> >    value.
>>
>> No it doesn't make sense at all to take something that is hogged
>> because it is completely unintuitive to the very word "hog".
>
> What I want is that it is possible to restrict the usage of a GPIO more
> fine-grained. For example only fix the direction to input but allow a
> driver to still read out it's value. Or fix the direction to output but
> allow changing the level.

So let me understand this right.

For something input-only, return -EINVAL from .direction_output()
and vice versa mutatis mutandis for something output-only.

This semantic is set at runtime when using these functions.

Why can't you do this in the specific driver? Why does that have
to be handled by the core? I mean of course we can add a
flag to the gpio_desc if you think it's gonna be a common case,
but then I want an indication that there is a bunch of drivers that
need this done in the core.

> So the semantic of a hogged gpio as
> implemented today is a special case of my new use cases. So it seems
> naturally to extend the already existing concept.

I disagree and the DT maintainer Rob Herring already said he kind
of dislikes the hogs already, so let's not expand their use beyond
what is already being done, which is explicit hogging.

> And concerning the meaning of hogging: It still takes some freedom from
> others, just not everything because you can still request the line. So
> not being a native English speaker the word still fits for me.

IMO this breaks Rusty Russell's API Design Manifesto,
levels 7 and 6:

7. The obvious use is (probably) the correct one.
6. The name tells you how to use it.

In my case, just no. It is not obvuous and does not tell me how
to use it.

Something like gpio-line-initial-directions, gpio-line-initial-values
etc does.

> And technically it is sensible to implement the new use cases and
> today's hogging together and so it is also sensible IMHO to use the same
> mechanism in the device tree.

I disagree with that, sorry.

But let's ask for more opinions.

> So to stick to your suggestion I would have in the end for two lines
> io12 that must be 0 for ESD reasons and io13 that drives the RST-line of
> a companion DSP that should be kept low until userspace is ready and
> then allow being driven high via gpioctl:
>
>         io12 {
>                 gpio-hog;
>                 gpio = <4 0>;
>                 output-low;
>         };
>
>         io13 {
>                 gpio-init;
>                 gpio = <5 0>;
>                 output-low;
>                 fixed-direction;
>         };
>
> For me it looks artificial to not use "gpio-hog" for the 2nd
> specification but if that is the compromise that we can agree on, that's
> ok for me.

This looks OK to me.

> Orthogonal to gpio-hog being used for this or not, I like
>
>         output-low;
>         fixed-direction;
>
> better than
>
>         output-low-init;
>
> as the former is more intuitive.

Those are OK with me but I guess with

fixed-direction-output;
fixed-direction-input;

I still don't really understand the fixed-direction thing.

If that is a property of the *hardware* it should not be in the
device tree IMO. It should be in the driver and derived from
the compatible string.

If that is a property of the *use case* in *this system*
then something like this makes sense: we may need to
restrict it further for electrical reasons, that is nice.

I think you will have a problem with Rob on this though, he's not
a big fan of these hogging nodes and I'm not sure he's gonna like
the gpio-init nodes much.

If you send some patch, make sure to get it ACKed by Rob.

Yours,
Linus Walleij

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Uwe Kleine-König]

Hello Linus,

On Thu, May 11, 2017 at 04:29:30PM +0200, Linus Walleij wrote:
> On Sun, May 7, 2017 at 11:45 AM, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
> > [Me]
> 
> >> >    IMHO it is sensible to allow requesting a gpio that is completely
> >> >    hogged and just prevent changing direction and (if it's an output)
> >> >    value.
> >>
> >> No it doesn't make sense at all to take something that is hogged
> >> because it is completely unintuitive to the very word "hog".
> >
> > What I want is that it is possible to restrict the usage of a GPIO more
> > fine-grained. For example only fix the direction to input but allow a
> > driver to still read out it's value. Or fix the direction to output but
> > allow changing the level.
> 
> So let me understand this right.
> 
> For something input-only, return -EINVAL from .direction_output()
> and vice versa mutatis mutandis for something output-only.

Not sure if it's -EINVAL, but some error for sure, yes.
 
> This semantic is set at runtime when using these functions.
> 
> Why can't you do this in the specific driver? Why does that have

It's not known to the driver. On an i.MX28 based machine a certain pin
of the SoC might be NC and so should be set as output-low. That's
specific to that machine while the i.MX28 (and so the driver) could well
operate that pin as input. It might also make sense to limit a line to
input only if it is connected to another device that drives the line.

> to be handled by the core? I mean of course we can add a
> flag to the gpio_desc if you think it's gonna be a common case,
> but then I want an indication that there is a bunch of drivers that
> need this done in the core.

So this is entirely independent of the driver in question.

> > So the semantic of a hogged gpio as
> > implemented today is a special case of my new use cases. So it seems
> > naturally to extend the already existing concept.
> 
> I disagree

I guess you disagree to the 2nd sentence only because the first is
obviously true.

>            and the DT maintainer Rob Herring already said he kind
> of dislikes the hogs already, so let's not expand their use beyond
> what is already being done, which is explicit hogging.
> 
> > And concerning the meaning of hogging: It still takes some freedom from
> > others, just not everything because you can still request the line. So
> > not being a native English speaker the word still fits for me.
> 
> IMO this breaks Rusty Russell's API Design Manifesto,
> levels 7 and 6:
> 
> 7. The obvious use is (probably) the correct one.
> 6. The name tells you how to use it.
> 
> In my case, just no. It is not obvuous and does not tell me how
> to use it.
> 
> Something like gpio-line-initial-directions, gpio-line-initial-values
> etc does.
> 
> > And technically it is sensible to implement the new use cases and
> > today's hogging together and so it is also sensible IMHO to use the same
> > mechanism in the device tree.
> 
> I disagree with that, sorry.

gpio-hogs are a special case of my suggestion. So even if the binding
looks different in the end, it doesn't make sense to have two
implementations for the same thing.

> > So to stick to your suggestion I would have in the end for two lines
> > io12 that must be 0 for ESD reasons and io13 that drives the RST-line of
> > a companion DSP that should be kept low until userspace is ready and
> > then allow being driven high via gpioctl:
> >
> >         io12 {
> >                 gpio-hog;
> >                 gpio = <4 0>;
> >                 output-low;
> >         };
> >
> >         io13 {
> >                 gpio-init;
> >                 gpio = <5 0>;
> >                 output-low;
> >                 fixed-direction;
> >         };
> >
> > For me it looks artificial to not use "gpio-hog" for the 2nd
> > specification but if that is the compromise that we can agree on, that's
> > ok for me.
> 
> This looks OK to me.

This is my favourite binding among the stuff discussed given you don't
want to "reuse" gpio-hog, but I'm open for better ones :-)

The following looks nice:

	io12 {
		gpio;
		gpio = <4 0>;
		init-output-low;
		fixed-direction;
		fixed-value;
	};

but won't work because there is "gpio" twice.

If you ask me, having

	io12 {
		gpio-init;
		gpio = <4 0>;
		output-low;
		fixed-direction;
		fixed-value;
	};

somehow makes it superfluous to be able to say

	io12 {
		gpio-hog;
		gpio = <4 0>;
		output-low;
	};

as the only difference would be that with the former the gpio could be
requested while this isn't possible with the latter. (Which is also a
difference that shouldn't be there in a hardware description.)

> I think you will have a problem with Rob on this though, he's not
> a big fan of these hogging nodes and I'm not sure he's gonna like
> the gpio-init nodes much.

Rob: IMHO it makes sense to describe limitations like "This pin should
not be driven (as there is another driver)" or "This is an output pin
with the safe initial level being low" (for a line that drives the reset
pin of a device) or maybe even "This pin might be used as input or
output, the safe value is input" (for a pin that is available on a
pinheader with a pullup, so the usage is unknown) in the device tree. I
don't really care about how they should be specified, so if you have a
different idea, I'm all ears.

Best regards
Uwe

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

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Russell King - ARM Linux]

On Tue, Feb 07, 2017 at 12:09:50PM +0100, Uwe Kleine-K?nig wrote:
> Now additionally I want to initialize some gpios but allow them to be
> grabbed later. IMHO there are the following new cases:
> 
> It should be possible to:
> 
>  a) change the value of a gpio initially configured as output
>  b) change the direction of a gpio initially configured as output
>  c) change the direction of a gpio initially configured as input
> 
> IMHO the dts should describe which case should be applied to a given
> gpio.

Isn't it the job of the board firmware to ensure that the hardware is
setup to a reasonably sane state for the board?

You give an example of holding GPIOs low in Linux for "ESD reasons"
later in this thread, but if you're only doing that in Linux, what if
Linux isn't running yet, and the GPIO has been left floating by the
board firmware?  Fixing this in Linux is really too late.

Floating GPIOs are also a source of higher current drain - a GPIO
sitting mid-rail turns both transistors on for a MOS input, which gives
a direct path across the power supply.  The quicker that the GPIOs can
be initialised, the less wasted power there is.  So, that's another
argument for board firmware doing the basic GPIO initialisation and not
stuffing this into DT and having the kernel do it.

We could then talk about floating GPIOs that activate higher power
devices, and the arguments for doing GPIO initialisation as early as
possible in board firmware continue to stack up.

IMHO, initial configuration of GPIOs is the job of board firmware, not
the kernel.  I see no sane reason to push that into the kernel.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 3/3] gpiolib: Add GPIO initialization

[Uwe Kleine-König]

Hello Russell,

On Sun, May 07, 2017 at 11:22:01AM +0100, Russell King - ARM Linux wrote:
> On Tue, Feb 07, 2017 at 12:09:50PM +0100, Uwe Kleine-K?nig wrote:
> > Now additionally I want to initialize some gpios but allow them to be
> > grabbed later. IMHO there are the following new cases:
> > 
> > It should be possible to:
> > 
> >  a) change the value of a gpio initially configured as output
> >  b) change the direction of a gpio initially configured as output
> >  c) change the direction of a gpio initially configured as input
> > 
> > IMHO the dts should describe which case should be applied to a given
> > gpio.
> 
> Isn't it the job of the board firmware to ensure that the hardware is
> setup to a reasonably sane state for the board?
> 
> You give an example of holding GPIOs low in Linux for "ESD reasons"
> later in this thread, but if you're only doing that in Linux, what if
> Linux isn't running yet, and the GPIO has been left floating by the
> board firmware?  Fixing this in Linux is really too late.
> 
> Floating GPIOs are also a source of higher current drain - a GPIO
> sitting mid-rail turns both transistors on for a MOS input, which gives
> a direct path across the power supply.  The quicker that the GPIOs can
> be initialised, the less wasted power there is.  So, that's another
> argument for board firmware doing the basic GPIO initialisation and not
> stuffing this into DT and having the kernel do it.
> 
> We could then talk about floating GPIOs that activate higher power
> devices, and the arguments for doing GPIO initialisation as early as
> possible in board firmware continue to stack up.
> 
> IMHO, initial configuration of GPIOs is the job of board firmware, not
> the kernel.  I see no sane reason to push that into the kernel.

still it is hardware description, isn't it? And Linux isn't the only
consumer of the dtb, barebox understands it, too, and should be teached
to handle the gpio-init stuff once we agreed on a binding.

Best regards
Uwe

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