[PATCH v2 1/2] Documentation: Add GPIO reset binding to reset binding documentation

[PATCH v2 1/2] Documentation: Add GPIO reset binding to reset binding documentation

[Philipp Zabel]

This patch adds documentation clarifying the reset GPIO bindings most
commonly in use (reset-gpios and <name>-reset-gpios properties).

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
 - Since (<name>-)reset-gpios is already a common pattern, I have dropped the
   additional reset-gpio-names property.
 - A boolean (<name>-)reset-boot-asserted is added to keep GPIOs asserted when
   requesting them.
---
 Documentation/devicetree/bindings/reset/reset.txt | 26 +++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/reset/reset.txt b/Documentation/devicetree/bindings/reset/reset.txt
index 31db6ff..51f9e35 100644
--- a/Documentation/devicetree/bindings/reset/reset.txt
+++ b/Documentation/devicetree/bindings/reset/reset.txt
@@ -2,8 +2,8 @@
 
 This binding is intended to represent the hardware reset signals present
 internally in most IC (SoC, FPGA, ...) designs. Reset signals for whole
-standalone chips are most likely better represented as GPIOs, although there
-are likely to be exceptions to this rule.
+standalone chips are most likely better represented as GPIOs, ideally using a
+common scheme as described below.
 
 Hardware blocks typically receive a reset signal. This signal is generated by
 a reset provider (e.g. power management or clock module) and received by a
@@ -56,6 +56,20 @@ reset-names:	List of reset signal name strings sorted in the same order as
 		the resets property. Consumers drivers will use reset-names to
 		match reset signal names with reset specifiers.
 
+= GPIO Reset consumers =
+
+For the common case of reset lines controlled by GPIOs, the GPIO binding
+documented in devicetree/bindings/gpio/gpio.txt should be used:
+
+Required properties:
+reset-gpios or		Reset GPIO using standard GPIO bindings,
+<name>-reset-gpios:	optionally named to specify the reset line
+
+Optional properties:
+reset-boot-asserted or		Boolean. If set, the corresponding reset is
+<name>-reset-boot-asserted:	initially asserted and should be kept that way
+				until released by the driver.
+
 For example:
 
 	device {
@@ -65,6 +79,14 @@ For example:
 
 This represents a device with a single reset signal named "reset".
 
+	device2 {
+		reset-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
+		reset-boot-asserted;
+	};
+
+This represents a device with a single reset signal, controlled
+by an active-low GPIO, which is initally kept in reset.
+
 	bus {
 		resets = <&rst 10> <&rst 11> <&rst 12> <&rst 11>;
 		reset-names = "i2s1", "i2s2", "dma", "mixer";
-- 
1.8.5.2

[PATCH v2 2/2] reset: Add GPIO support to reset controller framework

[Philipp Zabel]

This adds support for GPIO controlled reset pins on peripheral ICs to the reset
controller framework. Currently there is no support for specifying a delay
between assertion and de-assertion of the reset signal, so this has to be
handled by the drivers.

Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
Changes since v1:
 - Since (<name>-)reset-gpios is already a common pattern, I have dropped the
   additional reset-gpio-names property.
 - A boolean (<name>-)reset-boot-asserted is added to keep GPIOs asserted when
   requesting them.
 - Rebased onto Maxime Ripard's of_reset_control_get patch
---
 drivers/reset/Kconfig      |  17 ++++++
 drivers/reset/Makefile     |   9 ++-
 drivers/reset/core.c       |  29 +++++-----
 drivers/reset/gpio-reset.c | 137 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/reset/reset-core.h |  47 ++++++++++++++++
 5 files changed, 224 insertions(+), 15 deletions(-)
 create mode 100644 drivers/reset/gpio-reset.c
 create mode 100644 drivers/reset/reset-core.h

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index c9d04f7..a6b12a9 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -10,4 +10,21 @@ menuconfig RESET_CONTROLLER
 	  This framework is designed to abstract reset handling of devices
 	  via GPIOs or SoC-internal reset controller modules.
 
+	  If the device tree contains any resets or reset-gpio properties,
+	  this probably should be enabled.
+
 	  If unsure, say no.
+
+if RESET_CONTROLLER
+
+menuconfig RESET_GPIO
+	bool "GPIO Reset Support"
+	depends on GPIOLIB
+	default y if GPIOLIB
+	help
+	  GPIO Reset Controller support.
+
+	  This option lets the reset controller framework handle reset lines
+	  connected to GPIOs.
+
+endif
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index cc29832..764bb74 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -1,2 +1,9 @@
-obj-$(CONFIG_RESET_CONTROLLER) += core.o
+reset-core-objs := core.o
+
+obj-$(CONFIG_RESET_CONTROLLER) += reset-core.o
+
+ifeq ($(CONFIG_RESET_GPIO),y)
+  reset-core-objs += gpio-reset.o
+endif
+
 obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index baeaf82..6bfd2d2 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -18,23 +18,12 @@
 #include <linux/reset-controller.h>
 #include <linux/slab.h>
 
+#include "reset-core.h"
+
 static DEFINE_MUTEX(reset_controller_list_mutex);
 static LIST_HEAD(reset_controller_list);
 
 /**
- * struct reset_control - a reset control
- * @rcdev: a pointer to the reset controller device
- *         this reset control belongs to
- * @id: ID of the reset controller in the reset
- *      controller device
- */
-struct reset_control {
-	struct reset_controller_dev *rcdev;
-	struct device *dev;
-	unsigned int id;
-};
-
-/**
  * of_reset_simple_xlate - translate reset_spec to the reset line number
  * @rcdev: a pointer to the reset controller device
  * @reset_spec: reset line specifier as found in the device tree
@@ -149,6 +138,8 @@ struct reset_control *of_reset_control_get(struct device_node *node,
 						 "reset-names", id);
 	ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
 					 index, &args);
+	if (index == -EINVAL)
+		return ERR_PTR(-ENOENT);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -209,6 +200,13 @@ struct reset_control *reset_control_get(struct device *dev, const char *id)
 	if (!IS_ERR(rstc))
 		rstc->dev = dev;
 
+	/*
+	 * If there is no dedicated reset controller device, check if we have
+	 * a reset line controlled by a GPIO instead.
+	 */
+	if (PTR_ERR(rstc) == -ENOENT)
+		return gpio_reset_control_get(dev, id);
+
 	return rstc;
 }
 EXPORT_SYMBOL_GPL(reset_control_get);
@@ -223,7 +221,10 @@ void reset_control_put(struct reset_control *rstc)
 	if (IS_ERR(rstc))
 		return;
 
-	module_put(rstc->rcdev->owner);
+	if (reset_control_is_gpio(rstc))
+		gpio_reset_control_put(rstc);
+	else
+		module_put(rstc->rcdev->owner);
 	kfree(rstc);
 }
 EXPORT_SYMBOL_GPL(reset_control_put);
diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
new file mode 100644
index 0000000..78cd023
--- /dev/null
+++ b/drivers/reset/gpio-reset.c
@@ -0,0 +1,137 @@
+/*
+ * GPIO Reset Controller
+ *
+ * Copyright 2013 Philipp Zabel, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/reset-controller.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "reset-core.h"
+
+/*
+ * Global GPIO reset controller
+ */
+static struct reset_controller_dev *gpio_rcdev;
+
+static int gpio_reset_set(struct reset_controller_dev *rcdev,
+			  unsigned int gpio, int asserted)
+{
+	struct gpio_desc *gpiod = gpio_to_desc(gpio);
+
+	if (!gpiod)
+		return -EINVAL;
+
+	gpiod_set_value_cansleep(gpiod, asserted);
+
+	return 0;
+}
+
+static int gpio_reset_assert(struct reset_controller_dev *rcdev,
+		unsigned long id)
+{
+	return gpio_reset_set(rcdev, id, 1);
+}
+
+static int gpio_reset_deassert(struct reset_controller_dev *rcdev,
+		unsigned long id)
+{
+	return gpio_reset_set(rcdev, id, 0);
+}
+
+static struct reset_control_ops gpio_reset_ops = {
+	.assert = gpio_reset_assert,
+	.deassert = gpio_reset_deassert,
+};
+
+struct reset_controller_dev *reset_get_gpio_rcdev(void)
+{
+	if (gpio_rcdev)
+		return gpio_rcdev;
+
+	gpio_rcdev = kzalloc(sizeof(*gpio_rcdev), GFP_KERNEL);
+	if (!gpio_rcdev)
+		return NULL;
+
+	gpio_rcdev->owner = THIS_MODULE;
+	gpio_rcdev->ops = &gpio_reset_ops;
+
+	reset_controller_register(gpio_rcdev);
+
+	return gpio_rcdev;
+}
+
+struct reset_control *gpio_reset_control_get(struct device *dev, const char *id)
+{
+	const char *assert_prop = "reset-initially-asserted";
+	const char *gpio_con_id = "reset";
+	struct reset_controller_dev *rcdev;
+	struct reset_control *rstc;
+	struct gpio_desc *gpiod;
+	bool asserted = false;
+	char scratch[48];
+	int ret;
+
+	if (id) {
+		snprintf(scratch, 48, "%s-reset-boot-asserted", id);
+		assert_prop = scratch;
+	}
+
+	asserted = of_property_read_bool(dev->of_node, assert_prop);
+
+	if (id) {
+		snprintf(scratch, 48, "%s-reset", id);
+		gpio_con_id = scratch;
+	}
+
+	gpiod = gpiod_get(dev, gpio_con_id);
+	if (IS_ERR(gpiod)) {
+		dev_err(dev, "Failed to get GPIO reset: %ld\n", PTR_ERR(gpiod));
+		return ERR_CAST(gpiod);
+	}
+
+	ret = gpiod_direction_output(gpiod, asserted);
+	if (ret < 0)
+		goto err_put;
+
+	ret = -ENOMEM;
+	rcdev = reset_get_gpio_rcdev();
+	if (!rcdev)
+		goto err_put;
+
+	rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
+	if (!rstc)
+		goto err_put;
+
+	rstc->dev = dev;
+	rstc->rcdev = rcdev;
+	rstc->id = desc_to_gpio(gpiod);
+
+	return rstc;
+
+err_put:
+	gpiod_put(gpiod);
+	return ERR_PTR(ret);
+}
+
+bool reset_control_is_gpio(struct reset_control *rstc)
+{
+	return rstc->rcdev == gpio_rcdev;
+}
+
+void gpio_reset_control_put(struct reset_control *rstc)
+{
+	struct gpio_desc *gpiod = gpio_to_desc(rstc->id);
+
+	if (gpiod)
+		gpiod_put(gpiod);
+}
diff --git a/drivers/reset/reset-core.h b/drivers/reset/reset-core.h
new file mode 100644
index 0000000..55bd297
--- /dev/null
+++ b/drivers/reset/reset-core.h
@@ -0,0 +1,47 @@
+/*
+ * Reset Controller framework
+ *
+ * Copyright 2013 Philipp Zabel, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __RESET_CORE_H__
+#define __RESET_CORE_H__
+
+/**
+ * struct reset_control - a reset control
+ * @rcdev: a pointer to the reset controller device
+ *         this reset control belongs to
+ * @id: ID of the reset controller in the reset
+ *      controller device
+ */
+struct reset_control {
+	struct reset_controller_dev *rcdev;
+	struct device *dev;
+	unsigned int id;
+};
+
+#if IS_ENABLED(CONFIG_RESET_GPIO)
+struct reset_control *gpio_reset_control_get(struct device *dev, const char *id);
+bool reset_control_is_gpio(struct reset_control *rstc);
+void gpio_reset_control_put(struct reset_control *rstc);
+#else
+static inline struct reset_control *gpio_reset_control_get(struct device *dev,
+						    const char *id)
+{
+	return -ENOSYS;
+}
+
+static inline bool reset_control_is_gpio(struct reset_control *rstc)
+{
+	return false;
+}
+
+static inline void gpio_reset_control_put(struct reset_control *rstc) { }
+#endif
+
+#endif
-- 
1.8.5.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] reset: Add GPIO support to reset controller framework

[Maxime Ripard]

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

Hi Philipp,

On Mon, Jan 06, 2014 at 06:55:03PM +0100, Philipp Zabel wrote:
> This adds support for GPIO controlled reset pins on peripheral ICs to the reset
> controller framework. Currently there is no support for specifying a delay
> between assertion and de-assertion of the reset signal, so this has to be
> handled by the drivers.
> 
> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> Changes since v1:
>  - Since (<name>-)reset-gpios is already a common pattern, I have dropped the
>    additional reset-gpio-names property.
>  - A boolean (<name>-)reset-boot-asserted is added to keep GPIOs asserted when
>    requesting them.
>  - Rebased onto Maxime Ripard's of_reset_control_get patch
> ---
>  drivers/reset/Kconfig      |  17 ++++++
>  drivers/reset/Makefile     |   9 ++-
>  drivers/reset/core.c       |  29 +++++-----
>  drivers/reset/gpio-reset.c | 137 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/reset/reset-core.h |  47 ++++++++++++++++
>  5 files changed, 224 insertions(+), 15 deletions(-)
>  create mode 100644 drivers/reset/gpio-reset.c
>  create mode 100644 drivers/reset/reset-core.h
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index c9d04f7..a6b12a9 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -10,4 +10,21 @@ menuconfig RESET_CONTROLLER
>  	  This framework is designed to abstract reset handling of devices
>  	  via GPIOs or SoC-internal reset controller modules.
>  
> +	  If the device tree contains any resets or reset-gpio properties,
> +	  this probably should be enabled.
> +
>  	  If unsure, say no.
> +
> +if RESET_CONTROLLER
> +
> +menuconfig RESET_GPIO
> +	bool "GPIO Reset Support"
> +	depends on GPIOLIB
> +	default y if GPIOLIB
> +	help
> +	  GPIO Reset Controller support.
> +
> +	  This option lets the reset controller framework handle reset lines
> +	  connected to GPIOs.
> +
> +endif
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index cc29832..764bb74 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -1,2 +1,9 @@
> -obj-$(CONFIG_RESET_CONTROLLER) += core.o
> +reset-core-objs := core.o
> +
> +obj-$(CONFIG_RESET_CONTROLLER) += reset-core.o
> +
> +ifeq ($(CONFIG_RESET_GPIO),y)
> +  reset-core-objs += gpio-reset.o
> +endif
> +
>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index baeaf82..6bfd2d2 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -18,23 +18,12 @@
>  #include <linux/reset-controller.h>
>  #include <linux/slab.h>
>  
> +#include "reset-core.h"
> +
>  static DEFINE_MUTEX(reset_controller_list_mutex);
>  static LIST_HEAD(reset_controller_list);
>  
>  /**
> - * struct reset_control - a reset control
> - * @rcdev: a pointer to the reset controller device
> - *         this reset control belongs to
> - * @id: ID of the reset controller in the reset
> - *      controller device
> - */
> -struct reset_control {
> -	struct reset_controller_dev *rcdev;
> -	struct device *dev;
> -	unsigned int id;
> -};
> -
> -/**
>   * of_reset_simple_xlate - translate reset_spec to the reset line number
>   * @rcdev: a pointer to the reset controller device
>   * @reset_spec: reset line specifier as found in the device tree
> @@ -149,6 +138,8 @@ struct reset_control *of_reset_control_get(struct device_node *node,
>  						 "reset-names", id);
>  	ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
>  					 index, &args);
> +	if (index == -EINVAL)
> +		return ERR_PTR(-ENOENT);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> @@ -209,6 +200,13 @@ struct reset_control *reset_control_get(struct device *dev, const char *id)
>  	if (!IS_ERR(rstc))
>  		rstc->dev = dev;
>  
> +	/*
> +	 * If there is no dedicated reset controller device, check if we have
> +	 * a reset line controlled by a GPIO instead.
> +	 */
> +	if (PTR_ERR(rstc) == -ENOENT)
> +		return gpio_reset_control_get(dev, id);
> +
>  	return rstc;
>  }
>  EXPORT_SYMBOL_GPL(reset_control_get);
> @@ -223,7 +221,10 @@ void reset_control_put(struct reset_control *rstc)
>  	if (IS_ERR(rstc))
>  		return;
>  
> -	module_put(rstc->rcdev->owner);
> +	if (reset_control_is_gpio(rstc))
> +		gpio_reset_control_put(rstc);
> +	else
> +		module_put(rstc->rcdev->owner);
>  	kfree(rstc);
>  }
>  EXPORT_SYMBOL_GPL(reset_control_put);
> diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
> new file mode 100644
> index 0000000..78cd023
> --- /dev/null
> +++ b/drivers/reset/gpio-reset.c
> @@ -0,0 +1,137 @@
> +/*
> + * GPIO Reset Controller
> + *
> + * Copyright 2013 Philipp Zabel, Pengutronix
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/reset-controller.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "reset-core.h"
> +
> +/*
> + * Global GPIO reset controller
> + */
> +static struct reset_controller_dev *gpio_rcdev;
> +
> +static int gpio_reset_set(struct reset_controller_dev *rcdev,
> +			  unsigned int gpio, int asserted)
> +{
> +	struct gpio_desc *gpiod = gpio_to_desc(gpio);
> +
> +	if (!gpiod)
> +		return -EINVAL;
> +
> +	gpiod_set_value_cansleep(gpiod, asserted);
> +
> +	return 0;
> +}
> +
> +static int gpio_reset_assert(struct reset_controller_dev *rcdev,
> +		unsigned long id)
> +{
> +	return gpio_reset_set(rcdev, id, 1);
> +}
> +
> +static int gpio_reset_deassert(struct reset_controller_dev *rcdev,
> +		unsigned long id)
> +{
> +	return gpio_reset_set(rcdev, id, 0);
> +}
> +
> +static struct reset_control_ops gpio_reset_ops = {
> +	.assert = gpio_reset_assert,
> +	.deassert = gpio_reset_deassert,
> +};
> +
> +struct reset_controller_dev *reset_get_gpio_rcdev(void)
> +{
> +	if (gpio_rcdev)
> +		return gpio_rcdev;
> +
> +	gpio_rcdev = kzalloc(sizeof(*gpio_rcdev), GFP_KERNEL);
> +	if (!gpio_rcdev)
> +		return NULL;
> +
> +	gpio_rcdev->owner = THIS_MODULE;
> +	gpio_rcdev->ops = &gpio_reset_ops;
> +
> +	reset_controller_register(gpio_rcdev);
> +
> +	return gpio_rcdev;
> +}
> +
> +struct reset_control *gpio_reset_control_get(struct device *dev, const char *id)
> +{
> +	const char *assert_prop = "reset-initially-asserted";

I guess you meant reset-boot-asserted here, right?

> +	const char *gpio_con_id = "reset";
> +	struct reset_controller_dev *rcdev;
> +	struct reset_control *rstc;
> +	struct gpio_desc *gpiod;
> +	bool asserted = false;
> +	char scratch[48];
> +	int ret;
> +
> +	if (id) {
> +		snprintf(scratch, 48, "%s-reset-boot-asserted", id);
> +		assert_prop = scratch;
> +	}
> +
> +	asserted = of_property_read_bool(dev->of_node, assert_prop);
> +
> +	if (id) {
> +		snprintf(scratch, 48, "%s-reset", id);
> +		gpio_con_id = scratch;
> +	}
> +
> +	gpiod = gpiod_get(dev, gpio_con_id);
> +	if (IS_ERR(gpiod)) {
> +		dev_err(dev, "Failed to get GPIO reset: %ld\n", PTR_ERR(gpiod));
> +		return ERR_CAST(gpiod);
> +	}
> +
> +	ret = gpiod_direction_output(gpiod, asserted);
> +	if (ret < 0)
> +		goto err_put;

What happens if the GPIO is active low? I see no way in your binding
and driver to give that information, and that would change the
behaviour quite a bit.


Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH v2 2/2] reset: Add GPIO support to reset controller framework

[Philipp Zabel]

Hi Maxime,

Am Montag, den 06.01.2014, 19:14 +0100 schrieb Maxime Ripard:
> > +struct reset_control *gpio_reset_control_get(struct device *dev, const char *id)
> > +{
> > +	const char *assert_prop = "reset-initially-asserted";
> 
> I guess you meant reset-boot-asserted here, right?

Yes, thank you.

> > +	const char *gpio_con_id = "reset";
> > +	struct reset_controller_dev *rcdev;
> > +	struct reset_control *rstc;
> > +	struct gpio_desc *gpiod;
> > +	bool asserted = false;
> > +	char scratch[48];
> > +	int ret;
> > +
> > +	if (id) {
> > +		snprintf(scratch, 48, "%s-reset-boot-asserted", id);
> > +		assert_prop = scratch;
> > +	}
> > +
> > +	asserted = of_property_read_bool(dev->of_node, assert_prop);
> > +
> > +	if (id) {
> > +		snprintf(scratch, 48, "%s-reset", id);
> > +		gpio_con_id = scratch;
> > +	}
> > +
> > +	gpiod = gpiod_get(dev, gpio_con_id);
> > +	if (IS_ERR(gpiod)) {
> > +		dev_err(dev, "Failed to get GPIO reset: %ld\n", PTR_ERR(gpiod));
> > +		return ERR_CAST(gpiod);
> > +	}
> > +
> > +	ret = gpiod_direction_output(gpiod, asserted);
> > +	if (ret < 0)
> > +		goto err_put;
> 
> What happens if the GPIO is active low? I see no way in your binding
> and driver to give that information, and that would change the
> behaviour quite a bit.

I was under the (wrong) impression that gpiod_direction_output takes a
logical value as gpiod_set_value does. Will fix that.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] reset: Add GPIO support to reset controller framework

[Maxime Ripard]

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

Hi Philipp,

On Tue, Jan 07, 2014 at 10:52:21AM +0100, Philipp Zabel wrote:
> > > +	const char *gpio_con_id = "reset";
> > > +	struct reset_controller_dev *rcdev;
> > > +	struct reset_control *rstc;
> > > +	struct gpio_desc *gpiod;
> > > +	bool asserted = false;
> > > +	char scratch[48];
> > > +	int ret;
> > > +
> > > +	if (id) {
> > > +		snprintf(scratch, 48, "%s-reset-boot-asserted", id);
> > > +		assert_prop = scratch;
> > > +	}
> > > +
> > > +	asserted = of_property_read_bool(dev->of_node, assert_prop);
> > > +
> > > +	if (id) {
> > > +		snprintf(scratch, 48, "%s-reset", id);
> > > +		gpio_con_id = scratch;
> > > +	}
> > > +
> > > +	gpiod = gpiod_get(dev, gpio_con_id);
> > > +	if (IS_ERR(gpiod)) {
> > > +		dev_err(dev, "Failed to get GPIO reset: %ld\n", PTR_ERR(gpiod));
> > > +		return ERR_CAST(gpiod);
> > > +	}
> > > +
> > > +	ret = gpiod_direction_output(gpiod, asserted);
> > > +	if (ret < 0)
> > > +		goto err_put;
> > 
> > What happens if the GPIO is active low? I see no way in your binding
> > and driver to give that information, and that would change the
> > behaviour quite a bit.
> 
> I was under the (wrong) impression that gpiod_direction_output takes a
> logical value as gpiod_set_value does. Will fix that.

I don't think gpiod_set_value does either unfortunately.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH v2 2/2] reset: Add GPIO support to reset controller framework

[Philipp Zabel]

Am Dienstag, den 07.01.2014, 12:19 +0100 schrieb Maxime Ripard:
> Hi Philipp,
> 
> On Tue, Jan 07, 2014 at 10:52:21AM +0100, Philipp Zabel wrote:
> > > > +	const char *gpio_con_id = "reset";
> > > > +	struct reset_controller_dev *rcdev;
> > > > +	struct reset_control *rstc;
> > > > +	struct gpio_desc *gpiod;
> > > > +	bool asserted = false;
> > > > +	char scratch[48];
> > > > +	int ret;
> > > > +
> > > > +	if (id) {
> > > > +		snprintf(scratch, 48, "%s-reset-boot-asserted", id);
> > > > +		assert_prop = scratch;
> > > > +	}
> > > > +
> > > > +	asserted = of_property_read_bool(dev->of_node, assert_prop);
> > > > +
> > > > +	if (id) {
> > > > +		snprintf(scratch, 48, "%s-reset", id);
> > > > +		gpio_con_id = scratch;
> > > > +	}
> > > > +
> > > > +	gpiod = gpiod_get(dev, gpio_con_id);
> > > > +	if (IS_ERR(gpiod)) {
> > > > +		dev_err(dev, "Failed to get GPIO reset: %ld\n", PTR_ERR(gpiod));
> > > > +		return ERR_CAST(gpiod);
> > > > +	}
> > > > +
> > > > +	ret = gpiod_direction_output(gpiod, asserted);
> > > > +	if (ret < 0)
> > > > +		goto err_put;
> > > 
> > > What happens if the GPIO is active low? I see no way in your binding
> > > and driver to give that information, and that would change the
> > > behaviour quite a bit.
> > 
> > I was under the (wrong) impression that gpiod_direction_output takes a
> > logical value as gpiod_set_value does. Will fix that.
> 
> I don't think gpiod_set_value does either unfortunately.

It does, and Alexandre (added to Cc:) suggested that
gpiod_direction_output probably should, too.
I'll send a patch.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] reset: Add GPIO support to reset controller framework

[Maxime Ripard]

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

On Tue, Jan 07, 2014 at 12:28:09PM +0100, Philipp Zabel wrote:
> Am Dienstag, den 07.01.2014, 12:19 +0100 schrieb Maxime Ripard:
> > > I was under the (wrong) impression that gpiod_direction_output takes a
> > > logical value as gpiod_set_value does. Will fix that.
> > 
> > I don't think gpiod_set_value does either unfortunately.
> 
> It does, and Alexandre (added to Cc:) suggested that
> gpiod_direction_output probably should, too.  I'll send a patch.

Ah, yes, I was looking at 3.12, and this has been changed in 3.13.

Nevermind then.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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