diff mbox

[v7] reset: Add driver for gpio-controlled reset pins

Message ID 1369753575-9818-1-git-send-email-p.zabel@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Philipp Zabel May 28, 2013, 3:06 p.m. UTC
This driver implements a reset controller device that toggle a gpio
connected to a reset pin of a peripheral IC. The delay between assertion
and de-assertion of the reset signal can be configured via device tree.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v6:
 - Switched to a single gpio per gpio-reset device, as Olof suggested.
   This should make the device tree binding more straightforward.
 - Changed reset delay property to reset-delay-us.
---
 .../devicetree/bindings/reset/gpio-reset.txt       |  35 +++++
 drivers/reset/Kconfig                              |  11 ++
 drivers/reset/Makefile                             |   1 +
 drivers/reset/gpio-reset.c                         | 171 +++++++++++++++++++++
 4 files changed, 218 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.txt
 create mode 100644 drivers/reset/gpio-reset.c

Comments

Stephen Warren May 29, 2013, 11:19 p.m. UTC | #1
On 05/28/2013 09:06 AM, Philipp Zabel wrote:
> This driver implements a reset controller device that toggle a gpio
> connected to a reset pin of a peripheral IC. The delay between assertion
> and de-assertion of the reset signal can be configured via device tree.

> diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c

> +static void __gpio_reset_set(struct reset_controller_dev *rcdev, int asserted)

Nit: Technically I think __ is a reserved name-space, so you shouldn't
prefix "user-level" (rather than libc/similar) symbols with it, but it's
not a big deal.

> +static int gpio_reset_probe(struct platform_device *pdev)

> +	if (of_gpio_named_count(np, "reset-gpios") != 1)
> +		return -EINVAL;

Should that error-path (and the path for of_property_read_u32() failure)
include a dev_err() like some already do, so it's obvious what the
failure is?

> +	reset_controller_register(&drvdata->rcdev);
> +
> +	platform_set_drvdata(pdev, drvdata);

It might be better to set the drvdata right after it's been allocated.
It perhaps doesn't matter much here, but if the reset controller core
ever starts calling into the registered device inside the call to
reset_controller_register(), then there might be a problem.

Aside from those minor issues,
Reviewed-by: Stephen Warren <swarren@nvidia.com>
Philipp Zabel May 30, 2013, 9:08 a.m. UTC | #2
Hi Stephen,

Am Mittwoch, den 29.05.2013, 17:19 -0600 schrieb Stephen Warren:
> On 05/28/2013 09:06 AM, Philipp Zabel wrote:
> > This driver implements a reset controller device that toggle a gpio
> > connected to a reset pin of a peripheral IC. The delay between assertion
> > and de-assertion of the reset signal can be configured via device tree.
> 
> > diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
> 
> > +static void __gpio_reset_set(struct reset_controller_dev *rcdev, int asserted)
> 
> Nit: Technically I think __ is a reserved name-space, so you shouldn't
> prefix "user-level" (rather than libc/similar) symbols with it, but it's
> not a big deal.

I'll s/__//.

> > +static int gpio_reset_probe(struct platform_device *pdev)
> 
> > +	if (of_gpio_named_count(np, "reset-gpios") != 1)
> > +		return -EINVAL;
> 
> Should that error-path (and the path for of_property_read_u32() failure)
> include a dev_err() like some already do, so it's obvious what the
> failure is?

For reset-gpios, yes. reset-delay-us is marked as optional in the
bindings.

> > +	reset_controller_register(&drvdata->rcdev);
> > +
> > +	platform_set_drvdata(pdev, drvdata);
> 
> It might be better to set the drvdata right after it's been allocated.
> It perhaps doesn't matter much here, but if the reset controller core
> ever starts calling into the registered device inside the call to
> reset_controller_register(), then there might be a problem.

Alright. I'll move ist above the reset_controller_register() call.

> Aside from those minor issues,
> Reviewed-by: Stephen Warren <swarren@nvidia.com>

thanks
Philipp
Pavel Machek May 30, 2013, 10:38 a.m. UTC | #3
On Wed 2013-05-29 17:19:20, Stephen Warren wrote:
> On 05/28/2013 09:06 AM, Philipp Zabel wrote:
> > This driver implements a reset controller device that toggle a gpio
> > connected to a reset pin of a peripheral IC. The delay between assertion
> > and de-assertion of the reset signal can be configured via device tree.
> 
> > diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
> 
> > +static void __gpio_reset_set(struct reset_controller_dev *rcdev, int asserted)
> 
> Nit: Technically I think __ is a reserved name-space, so you shouldn't
> prefix "user-level" (rather than libc/similar) symbols with it, but it's
> not a big deal.

Well.. this is kernel :-). We don't have libc around... and this
standard is normally ignored here. __ is normally used as a prefix for
"simpler" function.

								Pavel
Arnd Bergmann May 30, 2013, 1:16 p.m. UTC | #4
On Thursday 30 May 2013, Pavel Machek wrote:
> On Wed 2013-05-29 17:19:20, Stephen Warren wrote:
> > On 05/28/2013 09:06 AM, Philipp Zabel wrote:
> > > This driver implements a reset controller device that toggle a gpio
> > > connected to a reset pin of a peripheral IC. The delay between assertion
> > > and de-assertion of the reset signal can be configured via device tree.
> > 
> > > diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
> > 
> > > +static void __gpio_reset_set(struct reset_controller_dev *rcdev, int asserted)
> > 
> > Nit: Technically I think __ is a reserved name-space, so you shouldn't
> > prefix "user-level" (rather than libc/similar) symbols with it, but it's
> > not a big deal.
> 
> Well.. this is kernel :-). We don't have libc around... and this
> standard is normally ignored here. __ is normally used as a prefix for
> "simpler" function.

We're not really consistent in the kernel, but I agree the most common
use is where you have a wrapper calling the __foo function and normal
drivers are supposed to call foo instead. My reading is usually that
by calling __foo, the caller says "I know exactly what I'm doing so
I can optimize this".

	Arnd
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt
new file mode 100644
index 0000000..6f7476b
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/gpio-reset.txt
@@ -0,0 +1,35 @@ 
+GPIO reset controller
+=====================
+
+A GPIO reset controller controls a single GPIO that is connected to the reset
+pin of a peripheral IC. Please also refer to reset.txt in this directory for
+common reset controller binding usage.
+
+Required properties:
+- compatible: Should be "gpio-reset"
+- reset-gpios: A gpio used as reset line. The gpio specifier for this property
+               depends on the gpio controller that provides the gpio.
+- #reset-cells: 0, see below
+
+Optional properties:
+- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for
+                  this duration to reset.
+- initially-in-reset: boolean. If not set, the initial state should be a
+                      deasserted reset line. If this property exists, the
+                      reset line should be kept in reset.
+
+example:
+
+sii902x_reset: gpio-reset {
+	compatible = "gpio-reset";
+	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+	reset-delay-us = <10000>;
+	initially-in-reset;
+	#reset-cells = <0>;
+};
+
+/* Device with nRESET pin connected to GPIO5_0 */
+sii902x@39 {
+	/* ... */
+	resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */
+};
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index c9d04f7..1a862df 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -11,3 +11,14 @@  menuconfig RESET_CONTROLLER
 	  via GPIOs or SoC-internal reset controller modules.
 
 	  If unsure, say no.
+
+if RESET_CONTROLLER
+
+config RESET_GPIO
+	tristate "GPIO reset controller support"
+	depends on GPIOLIB && OF
+	help
+	  This driver provides support for reset lines that are controlled
+	  directly by GPIOs.
+
+endif
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 1e2d83f..b854f20 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -1 +1,2 @@ 
 obj-$(CONFIG_RESET_CONTROLLER) += core.o
+obj-$(CONFIG_RESET_GPIO) += gpio-reset.o
diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
new file mode 100644
index 0000000..7faea09
--- /dev/null
+++ b/drivers/reset/gpio-reset.c
@@ -0,0 +1,169 @@ 
+/*
+ * GPIO Reset Controller driver
+ *
+ * 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/delay.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+
+struct gpio_reset_data {
+	struct reset_controller_dev rcdev;
+	unsigned int gpio;
+	bool active_low;
+	u32 delay_us;
+};
+
+static void __gpio_reset_set(struct reset_controller_dev *rcdev, int asserted)
+{
+	struct gpio_reset_data *drvdata = container_of(rcdev,
+			struct gpio_reset_data, rcdev);
+	int value = asserted;
+
+	if (drvdata->active_low)
+		value = !value;
+
+	gpio_set_value(drvdata->gpio, value);
+}
+
+static int gpio_reset(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct gpio_reset_data *drvdata = container_of(rcdev,
+			struct gpio_reset_data, rcdev);
+
+	if (drvdata->delay_us < 0)
+		return -ENOSYS;
+
+	__gpio_reset_set(rcdev, 1);
+	udelay(drvdata->delay_us);
+	__gpio_reset_set(rcdev, 0);
+
+	return 0;
+}
+
+static int gpio_reset_assert(struct reset_controller_dev *rcdev,
+		unsigned long id)
+{
+	__gpio_reset_set(rcdev, 1);
+
+	return 0;
+}
+
+static int gpio_reset_deassert(struct reset_controller_dev *rcdev,
+		unsigned long id)
+{
+	__gpio_reset_set(rcdev, 0);
+
+	return 0;
+}
+
+static struct reset_control_ops gpio_reset_ops = {
+	.reset = gpio_reset,
+	.assert = gpio_reset_assert,
+	.deassert = gpio_reset_deassert,
+};
+
+static int of_gpio_reset_xlate(struct reset_controller_dev *rcdev,
+			       const struct of_phandle_args *reset_spec)
+{
+	if (WARN_ON(reset_spec->args_count != 0))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int gpio_reset_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct gpio_reset_data *drvdata;
+	enum of_gpio_flags flags;
+	unsigned long gpio_flags;
+	bool initially_in_reset;
+	int ret;
+
+	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
+	if (drvdata == NULL)
+		return -ENOMEM;
+
+	if (of_gpio_named_count(np, "reset-gpios") != 1)
+		return -EINVAL;
+
+	drvdata->gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags);
+	if (drvdata->gpio == -EPROBE_DEFER) {
+		return drvdata->gpio;
+	} else if (!gpio_is_valid(drvdata->gpio)) {
+		dev_err(&pdev->dev, "invalid reset gpio: %d\n", drvdata->gpio);
+		return drvdata->gpio;
+	}
+
+	drvdata->active_low = flags & OF_GPIO_ACTIVE_LOW;
+
+	ret = of_property_read_u32(np, "reset-delay-us", &drvdata->delay_us);
+	if (ret < 0)
+		return ret;
+
+	initially_in_reset = of_property_read_bool(np, "initially-in-reset");
+	if (drvdata->active_low ^ initially_in_reset)
+		gpio_flags = GPIOF_OUT_INIT_HIGH;
+	else
+		gpio_flags = GPIOF_OUT_INIT_LOW;
+
+	ret = devm_gpio_request_one(&pdev->dev, drvdata->gpio, gpio_flags, NULL);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request gpio %d: %d\n",
+			drvdata->gpio, ret);
+		return ret;
+	}
+
+	drvdata->rcdev.of_node = np;
+	drvdata->rcdev.owner = THIS_MODULE;
+	drvdata->rcdev.nr_resets = 1;
+	drvdata->rcdev.ops = &gpio_reset_ops;
+	drvdata->rcdev.of_xlate = of_gpio_reset_xlate;
+	reset_controller_register(&drvdata->rcdev);
+
+	platform_set_drvdata(pdev, drvdata);
+
+	return 0;
+}
+
+static int gpio_reset_remove(struct platform_device *pdev)
+{
+	struct gpio_reset_data *drvdata = platform_get_drvdata(pdev);
+
+	reset_controller_unregister(&drvdata->rcdev);
+
+	return 0;
+}
+
+static struct of_device_id gpio_reset_dt_ids[] = {
+	{ .compatible = "gpio-reset" },
+	{ }
+};
+
+static struct platform_driver gpio_reset_driver = {
+	.probe = gpio_reset_probe,
+	.remove = gpio_reset_remove,
+	.driver = {
+		.name = "gpio-reset",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(gpio_reset_dt_ids),
+	},
+};
+
+module_platform_driver(gpio_reset_driver);
+
+MODULE_AUTHOR("Philipp Zabel <p.zabel@pengutronix.de>");
+MODULE_DESCRIPTION("gpio reset controller");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpio-reset");
+MODULE_DEVICE_TABLE(of, gpio_reset_dt_ids);