Re: [PATCH v2 3/3] gpiolib: Add GPIO initialization
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 wrote:
>
> > This functions adds a way to initialize a GPIO without hogging it.
> >
> > Signed-off-by: Markus Pargmann
>
> (...)
>
> > -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));
> > +
Re: [PATCH v2 3/3] gpiolib: Add GPIO initialization
On Sun, Aug 30, 2015 at 12:44 AM, Markus Pargmann wrote:
> This functions adds a way to initialize a GPIO without hogging it.
>
> Signed-off-by: Markus Pargmann
(...)
> -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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] gpiolib: Add GPIO initialization
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
> ---
> 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@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(s
[PATCH v2 3/3] gpiolib: Add GPIO initialization
This functions adds a way to initialize a GPIO without hogging it.
Signed-off-by: Markus Pargmann
---
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@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 (
