[PATCH/RFC 0/2] gpio: of: Add DT overlay support for GPIO hogs

[PATCH/RFC 0/2] gpio: of: Add DT overlay support for GPIO hogs

[Geert Uytterhoeven]

	Hi all,

As GPIO hogs are configured at GPIO controller initialization time,
adding/removing GPIO hogs in Device Tree overlays currently does not
work.  Hence this patch series adds support for that, by registering an
of_reconfig notifier, as is already done for platform, i2c, and SPI
devices.

Perhaps this would be better served through a pinctrl-gpio driver?
Pinctrl is already working fine with DT overlays, as the pinctrl-*
properties are part of the slave device node, and thus looked up at
slave device node attachment time, not at pin controller initialization
time.

In my particular use case (talking to SPI devices connected to a PMOD
connector on the RSK+RZA1 development board), the GPIO performs board
level muxing of a.o. the SPI MOSI/MISO/SCK signals.  Hence the hog
really needs to be active only while talking to the SPI device, so the
muxing could (in theory) be done upon demand.
But how to describe that in DT, and implement it (using Runtime PM?)?

Thanks for your comments!

Geert Uytterhoeven (2):
  gpio: of: Extract of_gpiochip_add_hog()
  gpio: of: Add DT overlay support for GPIO hogs

 drivers/gpio/gpiolib-of.c | 133 +++++++++++++++++++++++++++++++++-----
 drivers/gpio/gpiolib-of.h |   2 +
 drivers/gpio/gpiolib.c    |  14 +++-
 drivers/gpio/gpiolib.h    |   3 +
 4 files changed, 133 insertions(+), 19 deletions(-)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH/RFC 1/2] gpio: of: Extract of_gpiochip_add_hog()

[Geert Uytterhoeven]

Extract the code to add all GPIO hogs of a gpio-hog node into its own
function, so it can be reused.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/gpio/gpiolib-of.c | 49 ++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index b696e4598a240ea4..dfae797846bb746b 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -625,6 +625,35 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
 	return desc;
 }
 
+/**
+ * of_gpiochip_add_hog - Add all hogs in a hog device node
+ * @chip:	gpio chip to act on
+ * @hog:	device node describing the hogs
+ *
+ * Returns error if it fails otherwise 0 on success.
+ */
+static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
+{
+	enum gpiod_flags dflags;
+	struct gpio_desc *desc;
+	unsigned long lflags;
+	const char *name;
+	unsigned int i;
+	int ret;
+
+	for (i = 0;; i++) {
+		desc = of_parse_own_gpio(hog, chip, i, &name, &lflags, &dflags);
+		if (IS_ERR(desc))
+			break;
+
+		ret = gpiod_hog(desc, name, lflags, dflags);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 /**
  * of_gpiochip_scan_gpios - Scan gpio-controller for gpio definitions
  * @chip:	gpio chip to act on
@@ -635,29 +664,17 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
  */
 static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
 {
-	struct gpio_desc *desc = NULL;
 	struct device_node *np;
-	const char *name;
-	unsigned long lflags;
-	enum gpiod_flags dflags;
-	unsigned int i;
 	int ret;
 
 	for_each_available_child_of_node(chip->of_node, np) {
 		if (!of_property_read_bool(np, "gpio-hog"))
 			continue;
 
-		for (i = 0;; i++) {
-			desc = of_parse_own_gpio(np, chip, i, &name, &lflags,
-						 &dflags);
-			if (IS_ERR(desc))
-				break;
-
-			ret = gpiod_hog(desc, name, lflags, dflags);
-			if (ret < 0) {
-				of_node_put(np);
-				return ret;
-			}
+		ret = of_gpiochip_add_hog(chip, np);
+		if (ret < 0) {
+			of_node_put(np);
+			return ret;
 		}
 	}
 
-- 
2.17.1

[PATCH/RFC 2/2] gpio: of: Add DT overlay support for GPIO hogs

[Geert Uytterhoeven]

As GPIO hogs are configured at GPIO controller initialization time,
adding/removing GPIO hogs in DT overlays does not work.

Add support for GPIO hogs described in DT overlays by registering an OF
reconfiguration notifier, to handle the addition and removal of GPIO hog
subnodes to/from a GPIO controller device node.

Note that when a GPIO hog device node is being removed, its "gpios"
properties is no longer available, so we have to keep track of which
node a hog belongs to, which is done by adding a pointer to the hog's
device node to struct gpio_desc.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/gpio/gpiolib-of.c | 84 +++++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib-of.h |  2 +
 drivers/gpio/gpiolib.c    | 14 +++++--
 drivers/gpio/gpiolib.h    |  3 ++
 4 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index dfae797846bb746b..89a6138ac0a4b506 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -649,6 +649,10 @@ static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
 		ret = gpiod_hog(desc, name, lflags, dflags);
 		if (ret < 0)
 			return ret;
+
+#ifdef CONFIG_OF_DYNAMIC
+		desc->hog = hog;
+#endif
 	}
 
 	return 0;
@@ -676,11 +680,91 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
 			of_node_put(np);
 			return ret;
 		}
+
+		of_node_set_flag(np, OF_POPULATED);
 	}
 
 	return 0;
 }
 
+#ifdef CONFIG_OF_DYNAMIC
+/**
+ * of_gpiochip_remove_hog - Remove all hogs in a hog device node
+ * @chip:	gpio chip to act on
+ * @hog:	device node describing the hogs
+ */
+static void of_gpiochip_remove_hog(struct gpio_chip *chip,
+				   struct device_node *hog)
+{
+	struct gpio_desc *descs = chip->gpiodev->descs;
+	unsigned int i;
+
+	for (i = 0; i < chip->ngpio; i++) {
+		if (test_bit(FLAG_IS_HOGGED, &descs[i].flags) &&
+		    descs[i].hog == hog)
+			gpiochip_free_own_desc(&descs[i]);
+	}
+}
+
+static int of_gpiochip_match_node(struct gpio_chip *chip, void *data)
+{
+	return chip->gpiodev->dev.of_node == data;
+}
+
+static struct gpio_chip *of_find_gpiochip_by_node(struct device_node *np)
+{
+	return gpiochip_find(np, of_gpiochip_match_node);
+}
+
+static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
+			  void *arg)
+{
+	struct of_reconfig_data *rd = arg;
+	struct gpio_chip *chip;
+	int ret;
+
+	switch (of_reconfig_get_state_change(action, arg)) {
+	case OF_RECONFIG_CHANGE_ADD:
+		if (!of_property_read_bool(rd->dn, "gpio-hog"))
+			return NOTIFY_OK;	/* not for us */
+
+		if (of_node_test_and_set_flag(rd->dn, OF_POPULATED))
+			return NOTIFY_OK;
+
+		chip = of_find_gpiochip_by_node(rd->dn->parent);
+		if (chip == NULL)
+			return NOTIFY_OK;	/* not for us */
+
+		ret = of_gpiochip_add_hog(chip, rd->dn);
+		if (ret < 0) {
+			pr_err("%s: failed to add hogs for %pOF\n", __func__,
+			       rd->dn);
+			of_node_clear_flag(rd->dn, OF_POPULATED);
+			return notifier_from_errno(ret);
+		}
+		break;
+
+	case OF_RECONFIG_CHANGE_REMOVE:
+		if (!of_node_check_flag(rd->dn, OF_POPULATED))
+			return NOTIFY_OK;	/* already depopulated */
+
+		chip = of_find_gpiochip_by_node(rd->dn->parent);
+		if (chip == NULL)
+			return NOTIFY_OK;	/* not for us */
+
+		of_gpiochip_remove_hog(chip, rd->dn);
+		of_node_clear_flag(rd->dn, OF_POPULATED);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+struct notifier_block gpio_of_notifier = {
+	.notifier_call = of_gpio_notify,
+};
+#endif /* CONFIG_OF_DYNAMIC */
+
 /**
  * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
  * @gc:		pointer to the gpio_chip structure
diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
index 9768831b1fe2f25b..ed26664f153782fc 100644
--- a/drivers/gpio/gpiolib-of.h
+++ b/drivers/gpio/gpiolib-of.h
@@ -35,4 +35,6 @@ static inline bool of_gpio_need_valid_mask(const struct gpio_chip *gc)
 }
 #endif /* CONFIG_OF_GPIO */
 
+extern struct notifier_block gpio_of_notifier;
+
 #endif /* GPIOLIB_OF_H */
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bff5ac774d870b67..ef12cfcaf0962c1c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2952,6 +2952,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 		clear_bit(FLAG_PULL_DOWN, &desc->flags);
 		clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
 		clear_bit(FLAG_IS_HOGGED, &desc->flags);
+#ifdef CONFIG_OF_DYNAMIC
+		desc->hog = NULL;
+#endif
 		ret = true;
 	}
 
@@ -5145,10 +5148,15 @@ static int __init gpiolib_dev_init(void)
 	if (ret < 0) {
 		pr_err("gpiolib: failed to allocate char dev region\n");
 		bus_unregister(&gpio_bus_type);
-	} else {
-		gpiolib_initialized = true;
-		gpiochip_setup_devs();
+		return ret;
 	}
+
+	gpiolib_initialized = true;
+	gpiochip_setup_devs();
+
+	if (IS_ENABLED(CONFIG_OF_DYNAMIC))
+		WARN_ON(of_reconfig_notifier_register(&gpio_of_notifier));
+
 	return ret;
 }
 core_initcall(gpiolib_dev_init);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index a4a759920faa48ab..7af9931e8572304a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -118,6 +118,9 @@ struct gpio_desc {
 	const char		*label;
 	/* Name of the GPIO */
 	const char		*name;
+#ifdef CONFIG_OF_DYNAMIC
+	struct device_node	*hog;
+#endif
 };
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
-- 
2.17.1

Re: [PATCH/RFC 2/2] gpio: of: Add DT overlay support for GPIO hogs

[Frank Rowand]

On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
> As GPIO hogs are configured at GPIO controller initialization time,
> adding/removing GPIO hogs in DT overlays does not work.
> 
> Add support for GPIO hogs described in DT overlays by registering an OF
> reconfiguration notifier, to handle the addition and removal of GPIO hog
> subnodes to/from a GPIO controller device node.
> 
> Note that when a GPIO hog device node is being removed, its "gpios"
> properties is no longer available, so we have to keep track of which
> node a hog belongs to, which is done by adding a pointer to the hog's
> device node to struct gpio_desc.

If I have read the patches and the existing overlay source correctly,
then some observations:

- A gpio hog node added in an overlay will be properly processed.

- A gpio hog node already existing in the live devicetree, but with a
  non-active status will be properly processed if the status of the
  gpio hog node is changed to "ok" in the overlay.

- If a gpio hog node already exists in the live devicetree with an
  active status, then any updated or added properties in that gpio
  hog node in the overlay will have no effect.

  There is a scenario where the updated property would have an effect:
  apply a second overlay that sets the status to inactive, then apply
  a third overlay that sets the status back to active.  This is a
  rather contrived example and I think it should be documented as
  not supported and the result undefined.

It would be good to document this explicitly.


> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/gpio/gpiolib-of.c | 84 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpio/gpiolib-of.h |  2 +
>  drivers/gpio/gpiolib.c    | 14 +++++--
>  drivers/gpio/gpiolib.h    |  3 ++
>  4 files changed, 100 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index dfae797846bb746b..89a6138ac0a4b506 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -649,6 +649,10 @@ static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
>  		ret = gpiod_hog(desc, name, lflags, dflags);
>  		if (ret < 0)
>  			return ret;
> +
> +#ifdef CONFIG_OF_DYNAMIC
> +		desc->hog = hog;
> +#endif
>  	}
>  
>  	return 0;
> @@ -676,11 +680,91 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>  			of_node_put(np);
>  			return ret;
>  		}
> +
> +		of_node_set_flag(np, OF_POPULATED);
>  	}
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF_DYNAMIC
> +/**
> + * of_gpiochip_remove_hog - Remove all hogs in a hog device node
> + * @chip:	gpio chip to act on
> + * @hog:	device node describing the hogs
> + */
> +static void of_gpiochip_remove_hog(struct gpio_chip *chip,
> +				   struct device_node *hog)
> +{
> +	struct gpio_desc *descs = chip->gpiodev->descs;
> +	unsigned int i;
> +
> +	for (i = 0; i < chip->ngpio; i++) {
> +		if (test_bit(FLAG_IS_HOGGED, &descs[i].flags) &&
> +		    descs[i].hog == hog)
> +			gpiochip_free_own_desc(&descs[i]);
> +	}
> +}
> +
> +static int of_gpiochip_match_node(struct gpio_chip *chip, void *data)
> +{
> +	return chip->gpiodev->dev.of_node == data;
> +}
> +
> +static struct gpio_chip *of_find_gpiochip_by_node(struct device_node *np)
> +{
> +	return gpiochip_find(np, of_gpiochip_match_node);
> +}
> +
> +static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
> +			  void *arg)
> +{
> +	struct of_reconfig_data *rd = arg;
> +	struct gpio_chip *chip;
> +	int ret;
> +
> +	switch (of_reconfig_get_state_change(action, arg)) {
> +	case OF_RECONFIG_CHANGE_ADD:
> +		if (!of_property_read_bool(rd->dn, "gpio-hog"))
> +			return NOTIFY_OK;	/* not for us */
> +
> +		if (of_node_test_and_set_flag(rd->dn, OF_POPULATED))
> +			return NOTIFY_OK;

I don't think OF_POPULATED could be already set.  It seems to be a
bug if it is.


> +
> +		chip = of_find_gpiochip_by_node(rd->dn->parent);
> +		if (chip == NULL)
> +			return NOTIFY_OK;	/* not for us */

If I understand correctly, "not for us" is a misleading comment.
The notification is for the node rd->dn->parent, but the device
does not exist, so we can't do the hogging at the moment.  (If the
device is created later, then the gpio hog child node will exist,
and the init will "do the right thing" with the hog node -- so
not a problem.)

 
> +
> +		ret = of_gpiochip_add_hog(chip, rd->dn);
> +		if (ret < 0) {
> +			pr_err("%s: failed to add hogs for %pOF\n", __func__,
> +			       rd->dn);
> +			of_node_clear_flag(rd->dn, OF_POPULATED);
> +			return notifier_from_errno(ret);
> +		}
> +		break;
> +
> +	case OF_RECONFIG_CHANGE_REMOVE:
> +		if (!of_node_check_flag(rd->dn, OF_POPULATED))
> +			return NOTIFY_OK;	/* already depopulated */

I don't think OF_POPULATED could be already cleared.  It seems to be a
bug if it is.

> +
> +		chip = of_find_gpiochip_by_node(rd->dn->parent);
> +		if (chip == NULL)
> +			return NOTIFY_OK;	/* not for us */

Again, a misleading comment.

> +
> +		of_gpiochip_remove_hog(chip, rd->dn);
> +		of_node_clear_flag(rd->dn, OF_POPULATED);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +struct notifier_block gpio_of_notifier = {
> +	.notifier_call = of_gpio_notify,
> +};
> +#endif /* CONFIG_OF_DYNAMIC */
> +
>  /**
>   * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
>   * @gc:		pointer to the gpio_chip structure
> diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
> index 9768831b1fe2f25b..ed26664f153782fc 100644
> --- a/drivers/gpio/gpiolib-of.h
> +++ b/drivers/gpio/gpiolib-of.h
> @@ -35,4 +35,6 @@ static inline bool of_gpio_need_valid_mask(const struct gpio_chip *gc)
>  }
>  #endif /* CONFIG_OF_GPIO */
>  
> +extern struct notifier_block gpio_of_notifier;
> +
>  #endif /* GPIOLIB_OF_H */
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bff5ac774d870b67..ef12cfcaf0962c1c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2952,6 +2952,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
>  		clear_bit(FLAG_PULL_DOWN, &desc->flags);
>  		clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
>  		clear_bit(FLAG_IS_HOGGED, &desc->flags);
> +#ifdef CONFIG_OF_DYNAMIC
> +		desc->hog = NULL;
> +#endif
>  		ret = true;
>  	}
>  
> @@ -5145,10 +5148,15 @@ static int __init gpiolib_dev_init(void)
>  	if (ret < 0) {
>  		pr_err("gpiolib: failed to allocate char dev region\n");
>  		bus_unregister(&gpio_bus_type);
> -	} else {
> -		gpiolib_initialized = true;
> -		gpiochip_setup_devs();
> +		return ret;
>  	}
> +
> +	gpiolib_initialized = true;
> +	gpiochip_setup_devs();
> +
> +	if (IS_ENABLED(CONFIG_OF_DYNAMIC))
> +		WARN_ON(of_reconfig_notifier_register(&gpio_of_notifier));
> +
>  	return ret;
>  }
>  core_initcall(gpiolib_dev_init);
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index a4a759920faa48ab..7af9931e8572304a 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -118,6 +118,9 @@ struct gpio_desc {
>  	const char		*label;
>  	/* Name of the GPIO */
>  	const char		*name;
> +#ifdef CONFIG_OF_DYNAMIC
> +	struct device_node	*hog;
> +#endif
>  };
>  
>  int gpiod_request(struct gpio_desc *desc, const char *label);
> 

Re: [PATCH/RFC 2/2] gpio: of: Add DT overlay support for GPIO hogs

[Frank Rowand]

On 1/6/20 5:34 PM, Frank Rowand wrote:
> On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
>> As GPIO hogs are configured at GPIO controller initialization time,
>> adding/removing GPIO hogs in DT overlays does not work.
>>
>> Add support for GPIO hogs described in DT overlays by registering an OF
>> reconfiguration notifier, to handle the addition and removal of GPIO hog
>> subnodes to/from a GPIO controller device node.
>>
>> Note that when a GPIO hog device node is being removed, its "gpios"
>> properties is no longer available, so we have to keep track of which
>> node a hog belongs to, which is done by adding a pointer to the hog's
>> device node to struct gpio_desc.
> 
> If I have read the patches and the existing overlay source correctly,
> then some observations:
> 
> - A gpio hog node added in an overlay will be properly processed.
> 
> - A gpio hog node already existing in the live devicetree, but with a
>   non-active status will be properly processed if the status of the
>   gpio hog node is changed to "ok" in the overlay.
> 
> - If a gpio hog node already exists in the live devicetree with an
>   active status, then any updated or added properties in that gpio
>   hog node in the overlay will have no effect.
> 
>   There is a scenario where the updated property would have an effect:
>   apply a second overlay that sets the status to inactive, then apply
>   a third overlay that sets the status back to active.  This is a
>   rather contrived example and I think it should be documented as
>   not supported and the result undefined.

I went back and double checked the related code.  For gpio hog nodes
that are in a non-overlay, the status property is checked because
of_gpiochip_scan_gpios() uses for_each_available_child_of_node()
to search for gpio hog nodes, and for_each_available_child_of_node()
checks the status property.  But in the case of a gpio hog node
added by an overlay, of_gpio_notify() does not check the status
property in the gpio hog node.  The check for the status property
should be added to of_gpio_notify().

-Frank

> 
> It would be good to document this explicitly.
> 
> 
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  drivers/gpio/gpiolib-of.c | 84 +++++++++++++++++++++++++++++++++++++++
>>  drivers/gpio/gpiolib-of.h |  2 +
>>  drivers/gpio/gpiolib.c    | 14 +++++--
>>  drivers/gpio/gpiolib.h    |  3 ++
>>  4 files changed, 100 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> index dfae797846bb746b..89a6138ac0a4b506 100644
>> --- a/drivers/gpio/gpiolib-of.c
>> +++ b/drivers/gpio/gpiolib-of.c
>> @@ -649,6 +649,10 @@ static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
>>  		ret = gpiod_hog(desc, name, lflags, dflags);
>>  		if (ret < 0)
>>  			return ret;
>> +
>> +#ifdef CONFIG_OF_DYNAMIC
>> +		desc->hog = hog;
>> +#endif
>>  	}
>>  
>>  	return 0;
>> @@ -676,11 +680,91 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>>  			of_node_put(np);
>>  			return ret;
>>  		}
>> +
>> +		of_node_set_flag(np, OF_POPULATED);
>>  	}
>>  
>>  	return 0;
>>  }
>>  
>> +#ifdef CONFIG_OF_DYNAMIC
>> +/**
>> + * of_gpiochip_remove_hog - Remove all hogs in a hog device node
>> + * @chip:	gpio chip to act on
>> + * @hog:	device node describing the hogs
>> + */
>> +static void of_gpiochip_remove_hog(struct gpio_chip *chip,
>> +				   struct device_node *hog)
>> +{
>> +	struct gpio_desc *descs = chip->gpiodev->descs;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < chip->ngpio; i++) {
>> +		if (test_bit(FLAG_IS_HOGGED, &descs[i].flags) &&
>> +		    descs[i].hog == hog)
>> +			gpiochip_free_own_desc(&descs[i]);
>> +	}
>> +}
>> +
>> +static int of_gpiochip_match_node(struct gpio_chip *chip, void *data)
>> +{
>> +	return chip->gpiodev->dev.of_node == data;
>> +}
>> +
>> +static struct gpio_chip *of_find_gpiochip_by_node(struct device_node *np)
>> +{
>> +	return gpiochip_find(np, of_gpiochip_match_node);
>> +}
>> +
>> +static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
>> +			  void *arg)
>> +{
>> +	struct of_reconfig_data *rd = arg;
>> +	struct gpio_chip *chip;
>> +	int ret;
>> +
>> +	switch (of_reconfig_get_state_change(action, arg)) {
>> +	case OF_RECONFIG_CHANGE_ADD:
>> +		if (!of_property_read_bool(rd->dn, "gpio-hog"))
>> +			return NOTIFY_OK;	/* not for us */
>> +
>> +		if (of_node_test_and_set_flag(rd->dn, OF_POPULATED))
>> +			return NOTIFY_OK;
> 
> I don't think OF_POPULATED could be already set.  It seems to be a
> bug if it is.
> 
> 
>> +
>> +		chip = of_find_gpiochip_by_node(rd->dn->parent);
>> +		if (chip == NULL)
>> +			return NOTIFY_OK;	/* not for us */
> 
> If I understand correctly, "not for us" is a misleading comment.
> The notification is for the node rd->dn->parent, but the device
> does not exist, so we can't do the hogging at the moment.  (If the
> device is created later, then the gpio hog child node will exist,
> and the init will "do the right thing" with the hog node -- so
> not a problem.)
> 
>  
>> +
>> +		ret = of_gpiochip_add_hog(chip, rd->dn);
>> +		if (ret < 0) {
>> +			pr_err("%s: failed to add hogs for %pOF\n", __func__,
>> +			       rd->dn);
>> +			of_node_clear_flag(rd->dn, OF_POPULATED);
>> +			return notifier_from_errno(ret);
>> +		}
>> +		break;
>> +
>> +	case OF_RECONFIG_CHANGE_REMOVE:
>> +		if (!of_node_check_flag(rd->dn, OF_POPULATED))
>> +			return NOTIFY_OK;	/* already depopulated */
> 
> I don't think OF_POPULATED could be already cleared.  It seems to be a
> bug if it is.
> 
>> +
>> +		chip = of_find_gpiochip_by_node(rd->dn->parent);
>> +		if (chip == NULL)
>> +			return NOTIFY_OK;	/* not for us */
> 
> Again, a misleading comment.
> 
>> +
>> +		of_gpiochip_remove_hog(chip, rd->dn);
>> +		of_node_clear_flag(rd->dn, OF_POPULATED);
>> +		break;
>> +	}
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +struct notifier_block gpio_of_notifier = {
>> +	.notifier_call = of_gpio_notify,
>> +};
>> +#endif /* CONFIG_OF_DYNAMIC */
>> +
>>  /**
>>   * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
>>   * @gc:		pointer to the gpio_chip structure
>> diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
>> index 9768831b1fe2f25b..ed26664f153782fc 100644
>> --- a/drivers/gpio/gpiolib-of.h
>> +++ b/drivers/gpio/gpiolib-of.h
>> @@ -35,4 +35,6 @@ static inline bool of_gpio_need_valid_mask(const struct gpio_chip *gc)
>>  }
>>  #endif /* CONFIG_OF_GPIO */
>>  
>> +extern struct notifier_block gpio_of_notifier;
>> +
>>  #endif /* GPIOLIB_OF_H */
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index bff5ac774d870b67..ef12cfcaf0962c1c 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -2952,6 +2952,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
>>  		clear_bit(FLAG_PULL_DOWN, &desc->flags);
>>  		clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
>>  		clear_bit(FLAG_IS_HOGGED, &desc->flags);
>> +#ifdef CONFIG_OF_DYNAMIC
>> +		desc->hog = NULL;
>> +#endif
>>  		ret = true;
>>  	}
>>  
>> @@ -5145,10 +5148,15 @@ static int __init gpiolib_dev_init(void)
>>  	if (ret < 0) {
>>  		pr_err("gpiolib: failed to allocate char dev region\n");
>>  		bus_unregister(&gpio_bus_type);
>> -	} else {
>> -		gpiolib_initialized = true;
>> -		gpiochip_setup_devs();
>> +		return ret;
>>  	}
>> +
>> +	gpiolib_initialized = true;
>> +	gpiochip_setup_devs();
>> +
>> +	if (IS_ENABLED(CONFIG_OF_DYNAMIC))
>> +		WARN_ON(of_reconfig_notifier_register(&gpio_of_notifier));
>> +
>>  	return ret;
>>  }
>>  core_initcall(gpiolib_dev_init);
>> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
>> index a4a759920faa48ab..7af9931e8572304a 100644
>> --- a/drivers/gpio/gpiolib.h
>> +++ b/drivers/gpio/gpiolib.h
>> @@ -118,6 +118,9 @@ struct gpio_desc {
>>  	const char		*label;
>>  	/* Name of the GPIO */
>>  	const char		*name;
>> +#ifdef CONFIG_OF_DYNAMIC
>> +	struct device_node	*hog;
>> +#endif
>>  };
>>  
>>  int gpiod_request(struct gpio_desc *desc, const char *label);
>>
> 
> 

Re: [PATCH/RFC 2/2] gpio: of: Add DT overlay support for GPIO hogs

[Frank Rowand]

On 1/7/20 1:10 AM, Frank Rowand wrote:
> On 1/6/20 5:34 PM, Frank Rowand wrote:
>> On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
>>> As GPIO hogs are configured at GPIO controller initialization time,
>>> adding/removing GPIO hogs in DT overlays does not work.
>>>
>>> Add support for GPIO hogs described in DT overlays by registering an OF
>>> reconfiguration notifier, to handle the addition and removal of GPIO hog
>>> subnodes to/from a GPIO controller device node.
>>>
>>> Note that when a GPIO hog device node is being removed, its "gpios"
>>> properties is no longer available, so we have to keep track of which
>>> node a hog belongs to, which is done by adding a pointer to the hog's
>>> device node to struct gpio_desc.
>>
>> If I have read the patches and the existing overlay source correctly,
>> then some observations:
>>
>> - A gpio hog node added in an overlay will be properly processed.
>>
>> - A gpio hog node already existing in the live devicetree, but with a
>>   non-active status will be properly processed if the status of the
>>   gpio hog node is changed to "ok" in the overlay.
>>
>> - If a gpio hog node already exists in the live devicetree with an
>>   active status, then any updated or added properties in that gpio
>>   hog node in the overlay will have no effect.
>>
>>   There is a scenario where the updated property would have an effect:
>>   apply a second overlay that sets the status to inactive, then apply
>>   a third overlay that sets the status back to active.  This is a
>>   rather contrived example and I think it should be documented as
>>   not supported and the result undefined.
> 
> I went back and double checked the related code.  For gpio hog nodes
> that are in a non-overlay, the status property is checked because
> of_gpiochip_scan_gpios() uses for_each_available_child_of_node()
> to search for gpio hog nodes, and for_each_available_child_of_node()
> checks the status property.  But in the case of a gpio hog node
> added by an overlay, of_gpio_notify() does not check the status
> property in the gpio hog node.  The check for the status property
> should be added to of_gpio_notify().

One more thing I have not thought through is the case where the
overlay contains both the gpio node and the gpio hog node.  I'll
walk through that tomorrow.

-Frank

> 
> -Frank
> 
>>
>> It would be good to document this explicitly.
>>
>>
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>>  drivers/gpio/gpiolib-of.c | 84 +++++++++++++++++++++++++++++++++++++++
>>>  drivers/gpio/gpiolib-of.h |  2 +
>>>  drivers/gpio/gpiolib.c    | 14 +++++--
>>>  drivers/gpio/gpiolib.h    |  3 ++
>>>  4 files changed, 100 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>>> index dfae797846bb746b..89a6138ac0a4b506 100644
>>> --- a/drivers/gpio/gpiolib-of.c
>>> +++ b/drivers/gpio/gpiolib-of.c
>>> @@ -649,6 +649,10 @@ static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
>>>  		ret = gpiod_hog(desc, name, lflags, dflags);
>>>  		if (ret < 0)
>>>  			return ret;
>>> +
>>> +#ifdef CONFIG_OF_DYNAMIC
>>> +		desc->hog = hog;
>>> +#endif
>>>  	}
>>>  
>>>  	return 0;
>>> @@ -676,11 +680,91 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>>>  			of_node_put(np);
>>>  			return ret;
>>>  		}
>>> +
>>> +		of_node_set_flag(np, OF_POPULATED);
>>>  	}
>>>  
>>>  	return 0;
>>>  }
>>>  
>>> +#ifdef CONFIG_OF_DYNAMIC
>>> +/**
>>> + * of_gpiochip_remove_hog - Remove all hogs in a hog device node
>>> + * @chip:	gpio chip to act on
>>> + * @hog:	device node describing the hogs
>>> + */
>>> +static void of_gpiochip_remove_hog(struct gpio_chip *chip,
>>> +				   struct device_node *hog)
>>> +{
>>> +	struct gpio_desc *descs = chip->gpiodev->descs;
>>> +	unsigned int i;
>>> +
>>> +	for (i = 0; i < chip->ngpio; i++) {
>>> +		if (test_bit(FLAG_IS_HOGGED, &descs[i].flags) &&
>>> +		    descs[i].hog == hog)
>>> +			gpiochip_free_own_desc(&descs[i]);
>>> +	}
>>> +}
>>> +
>>> +static int of_gpiochip_match_node(struct gpio_chip *chip, void *data)
>>> +{
>>> +	return chip->gpiodev->dev.of_node == data;
>>> +}
>>> +
>>> +static struct gpio_chip *of_find_gpiochip_by_node(struct device_node *np)
>>> +{
>>> +	return gpiochip_find(np, of_gpiochip_match_node);
>>> +}
>>> +
>>> +static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
>>> +			  void *arg)
>>> +{
>>> +	struct of_reconfig_data *rd = arg;
>>> +	struct gpio_chip *chip;
>>> +	int ret;
>>> +
>>> +	switch (of_reconfig_get_state_change(action, arg)) {
>>> +	case OF_RECONFIG_CHANGE_ADD:
>>> +		if (!of_property_read_bool(rd->dn, "gpio-hog"))
>>> +			return NOTIFY_OK;	/* not for us */
>>> +
>>> +		if (of_node_test_and_set_flag(rd->dn, OF_POPULATED))
>>> +			return NOTIFY_OK;
>>
>> I don't think OF_POPULATED could be already set.  It seems to be a
>> bug if it is.
>>
>>
>>> +
>>> +		chip = of_find_gpiochip_by_node(rd->dn->parent);
>>> +		if (chip == NULL)
>>> +			return NOTIFY_OK;	/* not for us */
>>
>> If I understand correctly, "not for us" is a misleading comment.
>> The notification is for the node rd->dn->parent, but the device
>> does not exist, so we can't do the hogging at the moment.  (If the
>> device is created later, then the gpio hog child node will exist,
>> and the init will "do the right thing" with the hog node -- so
>> not a problem.)
>>
>>  
>>> +
>>> +		ret = of_gpiochip_add_hog(chip, rd->dn);
>>> +		if (ret < 0) {
>>> +			pr_err("%s: failed to add hogs for %pOF\n", __func__,
>>> +			       rd->dn);
>>> +			of_node_clear_flag(rd->dn, OF_POPULATED);
>>> +			return notifier_from_errno(ret);
>>> +		}
>>> +		break;
>>> +
>>> +	case OF_RECONFIG_CHANGE_REMOVE:
>>> +		if (!of_node_check_flag(rd->dn, OF_POPULATED))
>>> +			return NOTIFY_OK;	/* already depopulated */
>>
>> I don't think OF_POPULATED could be already cleared.  It seems to be a
>> bug if it is.
>>
>>> +
>>> +		chip = of_find_gpiochip_by_node(rd->dn->parent);
>>> +		if (chip == NULL)
>>> +			return NOTIFY_OK;	/* not for us */
>>
>> Again, a misleading comment.
>>
>>> +
>>> +		of_gpiochip_remove_hog(chip, rd->dn);
>>> +		of_node_clear_flag(rd->dn, OF_POPULATED);
>>> +		break;
>>> +	}
>>> +
>>> +	return NOTIFY_OK;
>>> +}
>>> +
>>> +struct notifier_block gpio_of_notifier = {
>>> +	.notifier_call = of_gpio_notify,
>>> +};
>>> +#endif /* CONFIG_OF_DYNAMIC */
>>> +
>>>  /**
>>>   * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
>>>   * @gc:		pointer to the gpio_chip structure
>>> diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
>>> index 9768831b1fe2f25b..ed26664f153782fc 100644
>>> --- a/drivers/gpio/gpiolib-of.h
>>> +++ b/drivers/gpio/gpiolib-of.h
>>> @@ -35,4 +35,6 @@ static inline bool of_gpio_need_valid_mask(const struct gpio_chip *gc)
>>>  }
>>>  #endif /* CONFIG_OF_GPIO */
>>>  
>>> +extern struct notifier_block gpio_of_notifier;
>>> +
>>>  #endif /* GPIOLIB_OF_H */
>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> index bff5ac774d870b67..ef12cfcaf0962c1c 100644
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -2952,6 +2952,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
>>>  		clear_bit(FLAG_PULL_DOWN, &desc->flags);
>>>  		clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
>>>  		clear_bit(FLAG_IS_HOGGED, &desc->flags);
>>> +#ifdef CONFIG_OF_DYNAMIC
>>> +		desc->hog = NULL;
>>> +#endif
>>>  		ret = true;
>>>  	}
>>>  
>>> @@ -5145,10 +5148,15 @@ static int __init gpiolib_dev_init(void)
>>>  	if (ret < 0) {
>>>  		pr_err("gpiolib: failed to allocate char dev region\n");
>>>  		bus_unregister(&gpio_bus_type);
>>> -	} else {
>>> -		gpiolib_initialized = true;
>>> -		gpiochip_setup_devs();
>>> +		return ret;
>>>  	}
>>> +
>>> +	gpiolib_initialized = true;
>>> +	gpiochip_setup_devs();
>>> +
>>> +	if (IS_ENABLED(CONFIG_OF_DYNAMIC))
>>> +		WARN_ON(of_reconfig_notifier_register(&gpio_of_notifier));
>>> +
>>>  	return ret;
>>>  }
>>>  core_initcall(gpiolib_dev_init);
>>> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
>>> index a4a759920faa48ab..7af9931e8572304a 100644
>>> --- a/drivers/gpio/gpiolib.h
>>> +++ b/drivers/gpio/gpiolib.h
>>> @@ -118,6 +118,9 @@ struct gpio_desc {
>>>  	const char		*label;
>>>  	/* Name of the GPIO */
>>>  	const char		*name;
>>> +#ifdef CONFIG_OF_DYNAMIC
>>> +	struct device_node	*hog;
>>> +#endif
>>>  };
>>>  
>>>  int gpiod_request(struct gpio_desc *desc, const char *label);
>>>
>>
>>
> 
> 

Re: [PATCH/RFC 2/2] gpio: of: Add DT overlay support for GPIO hogs

[Geert Uytterhoeven]

Hi Frank,

On Tue, Jan 7, 2020 at 8:26 AM Frank Rowand <frowand.list@gmail.com> wrote:
> On 1/7/20 1:10 AM, Frank Rowand wrote:
> > On 1/6/20 5:34 PM, Frank Rowand wrote:
> >> On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
> >>> As GPIO hogs are configured at GPIO controller initialization time,
> >>> adding/removing GPIO hogs in DT overlays does not work.
> >>>
> >>> Add support for GPIO hogs described in DT overlays by registering an OF
> >>> reconfiguration notifier, to handle the addition and removal of GPIO hog
> >>> subnodes to/from a GPIO controller device node.
> >>>
> >>> Note that when a GPIO hog device node is being removed, its "gpios"
> >>> properties is no longer available, so we have to keep track of which
> >>> node a hog belongs to, which is done by adding a pointer to the hog's
> >>> device node to struct gpio_desc.
> >>
> >> If I have read the patches and the existing overlay source correctly,
> >> then some observations:
> >>
> >> - A gpio hog node added in an overlay will be properly processed.
> >>
> >> - A gpio hog node already existing in the live devicetree, but with a
> >>   non-active status will be properly processed if the status of the
> >>   gpio hog node is changed to "ok" in the overlay.
> >>
> >> - If a gpio hog node already exists in the live devicetree with an
> >>   active status, then any updated or added properties in that gpio
> >>   hog node in the overlay will have no effect.
> >>
> >>   There is a scenario where the updated property would have an effect:
> >>   apply a second overlay that sets the status to inactive, then apply
> >>   a third overlay that sets the status back to active.  This is a
> >>   rather contrived example and I think it should be documented as
> >>   not supported and the result undefined.
> >
> > I went back and double checked the related code.  For gpio hog nodes
> > that are in a non-overlay, the status property is checked because
> > of_gpiochip_scan_gpios() uses for_each_available_child_of_node()
> > to search for gpio hog nodes, and for_each_available_child_of_node()
> > checks the status property.  But in the case of a gpio hog node
> > added by an overlay, of_gpio_notify() does not check the status
> > property in the gpio hog node.  The check for the status property
> > should be added to of_gpio_notify().
>
> One more thing I have not thought through is the case where the
> overlay contains both the gpio node and the gpio hog node.  I'll
> walk through that tomorrow.

I haven't veried that, but I assume that works already without my patch,
as GPIO hogs are configured at GPIO controller initialization time.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 2/2] gpio: of: Add DT overlay support for GPIO hogs

[Geert Uytterhoeven]

Hi Frank,

On Tue, Jan 7, 2020 at 8:10 AM Frank Rowand <frowand.list@gmail.com> wrote:
> On 1/6/20 5:34 PM, Frank Rowand wrote:
> > On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
> >> As GPIO hogs are configured at GPIO controller initialization time,
> >> adding/removing GPIO hogs in DT overlays does not work.
> >>
> >> Add support for GPIO hogs described in DT overlays by registering an OF
> >> reconfiguration notifier, to handle the addition and removal of GPIO hog
> >> subnodes to/from a GPIO controller device node.
> >>
> >> Note that when a GPIO hog device node is being removed, its "gpios"
> >> properties is no longer available, so we have to keep track of which
> >> node a hog belongs to, which is done by adding a pointer to the hog's
> >> device node to struct gpio_desc.
> >
> > If I have read the patches and the existing overlay source correctly,
> > then some observations:
> >
> > - A gpio hog node added in an overlay will be properly processed.
> >
> > - A gpio hog node already existing in the live devicetree, but with a
> >   non-active status will be properly processed if the status of the
> >   gpio hog node is changed to "ok" in the overlay.
> >
> > - If a gpio hog node already exists in the live devicetree with an
> >   active status, then any updated or added properties in that gpio
> >   hog node in the overlay will have no effect.
> >
> >   There is a scenario where the updated property would have an effect:
> >   apply a second overlay that sets the status to inactive, then apply
> >   a third overlay that sets the status back to active.  This is a
> >   rather contrived example and I think it should be documented as
> >   not supported and the result undefined.
>
> I went back and double checked the related code.  For gpio hog nodes
> that are in a non-overlay, the status property is checked because
> of_gpiochip_scan_gpios() uses for_each_available_child_of_node()
> to search for gpio hog nodes, and for_each_available_child_of_node()
> checks the status property.  But in the case of a gpio hog node
> added by an overlay, of_gpio_notify() does not check the status
> property in the gpio hog node.  The check for the status property
> should be added to of_gpio_notify().

Right.  of_device_is_available() should be called to check this.
Note that of_i2c_notify() and of_spi_notify() also lack such a check.
of_platform_notify() calls of_platform_device_create_pdata(), which does
have the check.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 2/2] gpio: of: Add DT overlay support for GPIO hogs

[Frank Rowand]

On 1/7/20 2:11 AM, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Tue, Jan 7, 2020 at 8:10 AM Frank Rowand <frowand.list@gmail.com> wrote:
>> On 1/6/20 5:34 PM, Frank Rowand wrote:
>>> On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
>>>> As GPIO hogs are configured at GPIO controller initialization time,
>>>> adding/removing GPIO hogs in DT overlays does not work.
>>>>
>>>> Add support for GPIO hogs described in DT overlays by registering an OF
>>>> reconfiguration notifier, to handle the addition and removal of GPIO hog
>>>> subnodes to/from a GPIO controller device node.
>>>>
>>>> Note that when a GPIO hog device node is being removed, its "gpios"
>>>> properties is no longer available, so we have to keep track of which
>>>> node a hog belongs to, which is done by adding a pointer to the hog's
>>>> device node to struct gpio_desc.
>>>
>>> If I have read the patches and the existing overlay source correctly,
>>> then some observations:
>>>
>>> - A gpio hog node added in an overlay will be properly processed.
>>>
>>> - A gpio hog node already existing in the live devicetree, but with a
>>>   non-active status will be properly processed if the status of the
>>>   gpio hog node is changed to "ok" in the overlay.

Verified by testing.


>>> - If a gpio hog node already exists in the live devicetree with an
>>>   active status, then any updated or added properties in that gpio
>>>   hog node in the overlay will have no effect.

Should be documented.


>>>   There is a scenario where the updated property would have an effect:
>>>   apply a second overlay that sets the status to inactive, then apply
>>>   a third overlay that sets the status back to active.  This is a
>>>   rather contrived example and I think it should be documented as
>>>   not supported and the result undefined.

I was wrong in this case.

of_reconfig_get_state_change() does not simply report whether a node
is added or removed, which confused me because it returns
OF_RECONFIG_CHANGE_ADD and OF_RECONFIG_CHANGE_REMOVE (as well as
no change), which I was incorrectly translating to node added or
node removed.   OF_RECONFIG_CHANGE_ADD and OF_RECONFIG_CHANGE_REMOVE
properly report a node becoming available or available due to changes
in the "status" property, as well as accounting for a node being
added or removed.

So the case that I was worried about is handled correctly.


>> I went back and double checked the related code.  For gpio hog nodes
>> that are in a non-overlay, the status property is checked because
>> of_gpiochip_scan_gpios() uses for_each_available_child_of_node()
>> to search for gpio hog nodes, and for_each_available_child_of_node()
>> checks the status property.  But in the case of a gpio hog node
>> added by an overlay, of_gpio_notify() does not check the status
>> property in the gpio hog node.  The check for the status property
>> should be added to of_gpio_notify().
> 
> Right.  of_device_is_available() should be called to check this.
> Note that of_i2c_notify() and of_spi_notify() also lack such a check.
> of_platform_notify() calls of_platform_device_create_pdata(), which does
> have the check.

And thus I was wrong about this also, so of_gpio_notify() does not need to
check the status property, since of_reconfig_get_state_change() already
implicitly incorporates this check.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH/RFC 2/2] gpio: of: Add DT overlay support for GPIO hogs

[Frank Rowand]

On 1/24/20 3:57 PM, Frank Rowand wrote:
> On 1/7/20 2:11 AM, Geert Uytterhoeven wrote:
>> Hi Frank,
>>
>> On Tue, Jan 7, 2020 at 8:10 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>> On 1/6/20 5:34 PM, Frank Rowand wrote:
>>>> On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
>>>>> As GPIO hogs are configured at GPIO controller initialization time,
>>>>> adding/removing GPIO hogs in DT overlays does not work.
>>>>>
>>>>> Add support for GPIO hogs described in DT overlays by registering an OF
>>>>> reconfiguration notifier, to handle the addition and removal of GPIO hog
>>>>> subnodes to/from a GPIO controller device node.
>>>>>
>>>>> Note that when a GPIO hog device node is being removed, its "gpios"
>>>>> properties is no longer available, so we have to keep track of which
>>>>> node a hog belongs to, which is done by adding a pointer to the hog's
>>>>> device node to struct gpio_desc.
>>>>
>>>> If I have read the patches and the existing overlay source correctly,
>>>> then some observations:
>>>>
>>>> - A gpio hog node added in an overlay will be properly processed.
>>>>
>>>> - A gpio hog node already existing in the live devicetree, but with a
>>>>   non-active status will be properly processed if the status of the
>>>>   gpio hog node is changed to "ok" in the overlay.
> 
> Verified by testing.
> 
> 
>>>> - If a gpio hog node already exists in the live devicetree with an
>>>>   active status, then any updated or added properties in that gpio
>>>>   hog node in the overlay will have no effect.
> 
> Should be documented.
> 
> 
>>>>   There is a scenario where the updated property would have an effect:
>>>>   apply a second overlay that sets the status to inactive, then apply
>>>>   a third overlay that sets the status back to active.  This is a
>>>>   rather contrived example and I think it should be documented as
>>>>   not supported and the result undefined.
> 
> I was wrong in this case.
> 
> of_reconfig_get_state_change() does not simply report whether a node
> is added or removed, which confused me because it returns
> OF_RECONFIG_CHANGE_ADD and OF_RECONFIG_CHANGE_REMOVE (as well as
> no change), which I was incorrectly translating to node added or
> node removed.   OF_RECONFIG_CHANGE_ADD and OF_RECONFIG_CHANGE_REMOVE
> properly report a node becoming available or available due to changes
                                            ^^^^^^^^^^^^
                                            or unavailable

> in the "status" property, as well as accounting for a node being
> added or removed.
> 
> So the case that I was worried about is handled correctly.
> 
> 
>>> I went back and double checked the related code.  For gpio hog nodes
>>> that are in a non-overlay, the status property is checked because
>>> of_gpiochip_scan_gpios() uses for_each_available_child_of_node()
>>> to search for gpio hog nodes, and for_each_available_child_of_node()
>>> checks the status property.  But in the case of a gpio hog node
>>> added by an overlay, of_gpio_notify() does not check the status
>>> property in the gpio hog node.  The check for the status property
>>> should be added to of_gpio_notify().
>>
>> Right.  of_device_is_available() should be called to check this.
>> Note that of_i2c_notify() and of_spi_notify() also lack such a check.
>> of_platform_notify() calls of_platform_device_create_pdata(), which does
>> have the check.
> 
> And thus I was wrong about this also, so of_gpio_notify() does not need to
> check the status property, since of_reconfig_get_state_change() already
> implicitly incorporates this check.
> 
>>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>
> 
> 

Re: [PATCH/RFC 2/2] gpio: of: Add DT overlay support for GPIO hogs

[Geert Uytterhoeven]

Hi Frank,

On Tue, Jan 7, 2020 at 12:34 AM Frank Rowand <frowand.list@gmail.com> wrote:
> On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
> > As GPIO hogs are configured at GPIO controller initialization time,
> > adding/removing GPIO hogs in DT overlays does not work.
> >
> > Add support for GPIO hogs described in DT overlays by registering an OF
> > reconfiguration notifier, to handle the addition and removal of GPIO hog
> > subnodes to/from a GPIO controller device node.
> >
> > Note that when a GPIO hog device node is being removed, its "gpios"
> > properties is no longer available, so we have to keep track of which
> > node a hog belongs to, which is done by adding a pointer to the hog's
> > device node to struct gpio_desc.
>
> If I have read the patches and the existing overlay source correctly,
> then some observations:
>
> - A gpio hog node added in an overlay will be properly processed.
>
> - A gpio hog node already existing in the live devicetree, but with a
>   non-active status will be properly processed if the status of the
>   gpio hog node is changed to "ok" in the overlay.
>
> - If a gpio hog node already exists in the live devicetree with an
>   active status, then any updated or added properties in that gpio
>   hog node in the overlay will have no effect.
>
>   There is a scenario where the updated property would have an effect:
>   apply a second overlay that sets the status to inactive, then apply
>   a third overlay that sets the status back to active.  This is a
>   rather contrived example and I think it should be documented as
>   not supported and the result undefined.
>
> It would be good to document this explicitly.

I didn't verify this in detail, but I believe the existing overlay
support for platform, i2c, and SPI devices behaves the same.

> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c

> > +static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
> > +                       void *arg)
> > +{
> > +     struct of_reconfig_data *rd = arg;
> > +     struct gpio_chip *chip;
> > +     int ret;
> > +
> > +     switch (of_reconfig_get_state_change(action, arg)) {
> > +     case OF_RECONFIG_CHANGE_ADD:
> > +             if (!of_property_read_bool(rd->dn, "gpio-hog"))
> > +                     return NOTIFY_OK;       /* not for us */
> > +
> > +             if (of_node_test_and_set_flag(rd->dn, OF_POPULATED))
> > +                     return NOTIFY_OK;
>
> I don't think OF_POPULATED could be already set.  It seems to be a
> bug if it is.

For a real gpio-hog it indeed is not.  But this function is called for
every change made to the device tree (add a printk() and look at the
output during boot).  So this serves as a (cheap) line of defense.
The of_find_gpiochip_by_node() call below is more expensive to call.

> > +
> > +             chip = of_find_gpiochip_by_node(rd->dn->parent);
> > +             if (chip == NULL)
> > +                     return NOTIFY_OK;       /* not for us */
>
> If I understand correctly, "not for us" is a misleading comment.
> The notification is for the node rd->dn->parent, but the device
> does not exist, so we can't do the hogging at the moment.  (If the
> device is created later, then the gpio hog child node will exist,
> and the init will "do the right thing" with the hog node -- so
> not a problem.)

This function is called for all additions to the device tree.
So rd->dn->parent may not even be a gpio controller node.
Hence unless this is a gpio controller node for this hog, this
notification is "not for us".

> > +
> > +             ret = of_gpiochip_add_hog(chip, rd->dn);
> > +             if (ret < 0) {
> > +                     pr_err("%s: failed to add hogs for %pOF\n", __func__,
> > +                            rd->dn);
> > +                     of_node_clear_flag(rd->dn, OF_POPULATED);
> > +                     return notifier_from_errno(ret);
> > +             }
> > +             break;
> > +
> > +     case OF_RECONFIG_CHANGE_REMOVE:
> > +             if (!of_node_check_flag(rd->dn, OF_POPULATED))
> > +                     return NOTIFY_OK;       /* already depopulated */
>
> I don't think OF_POPULATED could be already cleared.  It seems to be a
> bug if it is.

Same here. First line of defense.

> > +
> > +             chip = of_find_gpiochip_by_node(rd->dn->parent);
> > +             if (chip == NULL)
> > +                     return NOTIFY_OK;       /* not for us */
>
> Again, a misleading comment.

Same here. rd->dn->parent may be something else.

> > +
> > +             of_gpiochip_remove_hog(chip, rd->dn);
> > +             of_node_clear_flag(rd->dn, OF_POPULATED);
> > +             break;
> > +     }
> > +
> > +     return NOTIFY_OK;
> > +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 0/2] gpio: of: Add DT overlay support for GPIO hogs

[Bartosz Golaszewski]

pon., 30 gru 2019 o 14:38 Geert Uytterhoeven <geert+renesas@glider.be>
napisał(a):
>
>         Hi all,
>
> As GPIO hogs are configured at GPIO controller initialization time,
> adding/removing GPIO hogs in Device Tree overlays currently does not
> work.  Hence this patch series adds support for that, by registering an
> of_reconfig notifier, as is already done for platform, i2c, and SPI
> devices.
>
> Perhaps this would be better served through a pinctrl-gpio driver?
> Pinctrl is already working fine with DT overlays, as the pinctrl-*
> properties are part of the slave device node, and thus looked up at
> slave device node attachment time, not at pin controller initialization
> time.
>
> In my particular use case (talking to SPI devices connected to a PMOD
> connector on the RSK+RZA1 development board), the GPIO performs board
> level muxing of a.o. the SPI MOSI/MISO/SCK signals.  Hence the hog
> really needs to be active only while talking to the SPI device, so the
> muxing could (in theory) be done upon demand.
> But how to describe that in DT, and implement it (using Runtime PM?)?
>

I may be missing the whole picture, but from your description this
sounds like a job for the mux framework. Maybe we could make runtime
PM aware of muxing for this type of use-cases?

Bart

Re: [PATCH/RFC 0/2] gpio: of: Add DT overlay support for GPIO hogs

[Geert Uytterhoeven]

Hi Bartosz,

On Fri, Jan 3, 2020 at 10:51 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> pon., 30 gru 2019 o 14:38 Geert Uytterhoeven <geert+renesas@glider.be>
> napisał(a):
> > As GPIO hogs are configured at GPIO controller initialization time,
> > adding/removing GPIO hogs in Device Tree overlays currently does not
> > work.  Hence this patch series adds support for that, by registering an
> > of_reconfig notifier, as is already done for platform, i2c, and SPI
> > devices.
> >
> > Perhaps this would be better served through a pinctrl-gpio driver?
> > Pinctrl is already working fine with DT overlays, as the pinctrl-*
> > properties are part of the slave device node, and thus looked up at
> > slave device node attachment time, not at pin controller initialization
> > time.
> >
> > In my particular use case (talking to SPI devices connected to a PMOD
> > connector on the RSK+RZA1 development board), the GPIO performs board
> > level muxing of a.o. the SPI MOSI/MISO/SCK signals.  Hence the hog
> > really needs to be active only while talking to the SPI device, so the
> > muxing could (in theory) be done upon demand.
> > But how to describe that in DT, and implement it (using Runtime PM?)?
>
> I may be missing the whole picture, but from your description this
> sounds like a job for the mux framework. Maybe we could make runtime
> PM aware of muxing for this type of use-cases?

I'm happy with a (static) GPIO hog.

BTW, what exactly do you mean with "mux framework"? Pinctrl/pinmux?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 0/2] gpio: of: Add DT overlay support for GPIO hogs

[Bartosz Golaszewski]

wt., 7 sty 2020 o 08:46 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a):
>
> I'm happy with a (static) GPIO hog.
>
> BTW, what exactly do you mean with "mux framework"? Pinctrl/pinmux?
>

No, I meant the multiplexer subsystem under drivers/mux. I thought we
could call mux_control_select() from pm_runtime_get_*() or something
similar. This is just an idea though, and I see Frank already did an
in-depth analysis so never mind my comment.

Bart

Re: [PATCH/RFC 0/2] gpio: of: Add DT overlay support for GPIO hogs

[Geert Uytterhoeven]

Hi Bartosz,

On Tue, Jan 7, 2020 at 10:03 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> wt., 7 sty 2020 o 08:46 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a):
> > I'm happy with a (static) GPIO hog.
> >
> > BTW, what exactly do you mean with "mux framework"? Pinctrl/pinmux?
>
> No, I meant the multiplexer subsystem under drivers/mux. I thought we
> could call mux_control_select() from pm_runtime_get_*() or something
> similar. This is just an idea though, and I see Frank already did an
> in-depth analysis so never mind my comment.

Thanks, I wasn't aware of drivers/mux/...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 0/2] gpio: of: Add DT overlay support for GPIO hogs

[Frank Rowand]

On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
> 	Hi all,
> 
> As GPIO hogs are configured at GPIO controller initialization time,
> adding/removing GPIO hogs in Device Tree overlays currently does not
> work.  Hence this patch series adds support for that, by registering an
> of_reconfig notifier, as is already done for platform, i2c, and SPI
> devices.
> 
> Perhaps this would be better served through a pinctrl-gpio driver?
> Pinctrl is already working fine with DT overlays, as the pinctrl-*
> properties are part of the slave device node, and thus looked up at
> slave device node attachment time, not at pin controller initialization
> time.
> 
> In my particular use case (talking to SPI devices connected to a PMOD
> connector on the RSK+RZA1 development board), the GPIO performs board
> level muxing of a.o. the SPI MOSI/MISO/SCK signals.  Hence the hog
> really needs to be active only while talking to the SPI device, so the
> muxing could (in theory) be done upon demand.
> But how to describe that in DT, and implement it (using Runtime PM?)?

I'm trying to understand the use case.  I can easily imagine two cases:

  (1) want to configure the GPIO to be able to use the SPI bus sometimes,
      but configure the GPIO differently when not using the SPI bus

  (2) want to describe a device on the SPI bus in an overlay, thus
      also needing to describe the associate gpio hog node in the
      same overlay

For use case (2), the proposed patch seems to be a good solution.

For use case (1), this is a case of trying to use devicetree as a
way to control configuration instead of describing the hardware.
In this case, Bartosz' reply may indicate the way forward.

I'll assume use case (2) for patch comments.

> 
> Thanks for your comments!
> 
> Geert Uytterhoeven (2):
>   gpio: of: Extract of_gpiochip_add_hog()
>   gpio: of: Add DT overlay support for GPIO hogs
> 
>  drivers/gpio/gpiolib-of.c | 133 +++++++++++++++++++++++++++++++++-----
>  drivers/gpio/gpiolib-of.h |   2 +
>  drivers/gpio/gpiolib.c    |  14 +++-
>  drivers/gpio/gpiolib.h    |   3 +
>  4 files changed, 133 insertions(+), 19 deletions(-)
> 

Re: [PATCH/RFC 0/2] gpio: of: Add DT overlay support for GPIO hogs

[Geert Uytterhoeven]

Hi Frank,

On Tue, Jan 7, 2020 at 12:34 AM Frank Rowand <frowand.list@gmail.com> wrote:
> On 12/30/19 7:38 AM, Geert Uytterhoeven wrote:
> > As GPIO hogs are configured at GPIO controller initialization time,
> > adding/removing GPIO hogs in Device Tree overlays currently does not
> > work.  Hence this patch series adds support for that, by registering an
> > of_reconfig notifier, as is already done for platform, i2c, and SPI
> > devices.
> >
> > Perhaps this would be better served through a pinctrl-gpio driver?
> > Pinctrl is already working fine with DT overlays, as the pinctrl-*
> > properties are part of the slave device node, and thus looked up at
> > slave device node attachment time, not at pin controller initialization
> > time.
> >
> > In my particular use case (talking to SPI devices connected to a PMOD
> > connector on the RSK+RZA1 development board), the GPIO performs board
> > level muxing of a.o. the SPI MOSI/MISO/SCK signals.  Hence the hog
> > really needs to be active only while talking to the SPI device, so the
> > muxing could (in theory) be done upon demand.
> > But how to describe that in DT, and implement it (using Runtime PM?)?
>
> I'm trying to understand the use case.  I can easily imagine two cases:
>
>   (1) want to configure the GPIO to be able to use the SPI bus sometimes,
>       but configure the GPIO differently when not using the SPI bus
>
>   (2) want to describe a device on the SPI bus in an overlay, thus
>       also needing to describe the associate gpio hog node in the
>       same overlay
>
> For use case (2), the proposed patch seems to be a good solution.
>
> For use case (1), this is a case of trying to use devicetree as a
> way to control configuration instead of describing the hardware.
> In this case, Bartosz' reply may indicate the way forward.
>
> I'll assume use case (2) for patch comments.

Yes, my main interest is use case (2).
I have no plans to pursue use case (1).

However, I have some more comments and questions for use case (1).
Before you can control configuration, you have to describe the hardware.
Hence isn't that a job for DT?
Furthermore, I'd like you to step back and answer the following question:
what is the difference between a GPIO serving as a chip select for an
SPI slave, and a GPIO controlling board level muxing?  In both cases the
GPIO controls to which hardware other signals are routed, and both may
be changed at runtime.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds