Re: [PATCH 2/2] reset: Add GPIO-based reset controller

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Sean,

On Mon, 2021-10-25 at 14:17 -0400, Sean Anderson wrote:
[...]
> ping?
> 
> Philipp, should I be CCing anyone else? MAINTAINERS only lists you and vger...

That is still correct. Maybe it is time to request a dedicated reset
driver mailing list for better visibility.

I was on vacation for a bit, but thankfully Rob already pointed you in
the right direction. I would like this to live in the core and continue
to use the established reset-gpios bindings.

I'm not sure whether adding a full reset_controller_dev for GPIO resets
is necessary. Maybe there is a good reasond for it, but I found adding a
gpio_desc pointer to struct reset_control and implementing non-shared
GPIO resets in the core would be trivial - I prototyped this at some
point (untested, see below).

To support shared resets, the already requested gpio_desc would have to
be found and used to retrieve the corresponding requested reset_control
from a list. This could probably be achieved by exporting gpiod_find()
from gpiolib.

----------8<----------
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 61e688882643..b82acb38deb5 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -7,6 +7,8 @@
 #include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/kref.h>
@@ -26,6 +28,8 @@ static LIST_HEAD(reset_lookup_list);
  * struct reset_control - a reset control
  * @rcdev: a pointer to the reset controller device
  *         this reset control belongs to
+ * @gpio: a pointer to the gpio_desc controlling the
+ *        reset line, alternative to rcdev and id.
  * @list: list entry for the rcdev's reset controller list
  * @id: ID of the reset controller in the reset
  *      controller device
@@ -41,6 +45,7 @@ static LIST_HEAD(reset_lookup_list);
 struct reset_control {
 	struct reset_controller_dev *rcdev;
 	struct list_head list;
+	struct gpio_desc *gpio;
 	unsigned int id;
 	struct kref refcnt;
 	bool acquired;
@@ -330,7 +335,8 @@ int reset_control_reset(struct reset_control *rstc)
 	if (!rstc)
 		return 0;
 
-	if (WARN_ON(IS_ERR(rstc)))
+	if (WARN_ON(IS_ERR(rstc)) ||
+	    WARN_ON(rstc->gpio))
 		return -EINVAL;
 
 	if (reset_control_is_array(rstc))
@@ -458,7 +464,14 @@ int reset_control_assert(struct reset_control *rstc)
 
 		if (atomic_dec_return(&rstc->deassert_count) != 0)
 			return 0;
+	}
+
+	if (rstc->gpio) {
+		gpiod_set_value(rstc->gpio, 1);
+		return 0;
+	}
 
+	if (rstc->shared) {
 		/*
 		 * Shared reset controls allow the reset line to be in any state
 		 * after this call, so doing nothing is a valid option.
@@ -551,6 +564,11 @@ int reset_control_deassert(struct reset_control *rstc)
 		}
 	}
 
+	if (rstc->gpio) {
+		gpiod_set_value(rstc->gpio, 0);
+		return 0;
+	}
+
 	/*
 	 * If the reset controller does not implement .deassert(), we assume
 	 * that it handles self-deasserting reset lines via .reset(). In that
@@ -559,7 +577,7 @@ int reset_control_deassert(struct reset_control *rstc)
 	 * return -ENOTSUPP.
 	 */
 	if (!rstc->rcdev->ops->deassert)
-		return 0;
+		return -ENOTSUPP;
 
 	return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id);
 }
@@ -609,8 +627,10 @@ int reset_control_status(struct reset_control *rstc)
 	if (WARN_ON(IS_ERR(rstc)) || reset_control_is_array(rstc))
 		return -EINVAL;
 
-	if (rstc->rcdev->ops->status)
+	if (rstc->rcdev && rstc->rcdev->ops->status)
 		return rstc->rcdev->ops->status(rstc->rcdev, rstc->id);
+	if (rstc->gpio)
+		return gpiod_get_value(rstc->gpio);
 
 	return -ENOTSUPP;
 }
@@ -796,9 +816,11 @@ static void __reset_control_release(struct kref *kref)
 
 	lockdep_assert_held(&reset_list_mutex);
 
-	module_put(rstc->rcdev->owner);
+	if (rstc->rcdev) {
+		module_put(rstc->rcdev->owner);
 
-	list_del(&rstc->list);
+		list_del(&rstc->list);
+	}
 	kfree(rstc);
 }
 
@@ -935,19 +957,66 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
 	return rstc;
 }
 
+static struct reset_control *__reset_control_get_gpio(struct device *dev,
+						      const char *id,
+						      bool optional,
+						      bool acquired)
+{
+	struct reset_control *rstc;
+	struct gpio_desc *gpio;
+	char prop[32];
+
+	if (id)
+		snprintf(prop, sizeof(prop), "%s-reset", id);
+	gpio = gpiod_get_optional(dev, id ? prop : "reset", GPIOD_ASIS);
+	if (IS_ERR(gpio))
+		return ERR_CAST(gpio);
+	if (!gpio)
+		return optional ? NULL : ERR_PTR(-ENOENT);
+
+	rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
+	if (!rstc) {
+		gpiod_put(gpio);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	/*
+	 * Set the reset GPIO to output, deasserted, unless the GPIO is already
+	 * set to output, asserted.
+	 */
+	if (!(gpiod_get_direction(gpio) == GPIOF_DIR_OUT &&
+	      gpiod_get_value(gpio) == 1))
+		gpiod_direction_output(gpio, 0);
+
+	rstc->gpio = gpio;
+	rstc->refcnt = 1;
+	rstc->acquired = acquired;
+	rstc->shared = false;
+
+	return rstc;
+}
+
 struct reset_control *__reset_control_get(struct device *dev, const char *id,
 					  int index, bool shared, bool optional,
 					  bool acquired)
 {
+	struct reset_control *rstc = ERR_PTR(-ENOENT);
+
 	if (WARN_ON(shared && acquired))
 		return ERR_PTR(-EINVAL);
 
 	if (dev->of_node)
-		return __of_reset_control_get(dev->of_node, id, index, shared,
+		rstc = __of_reset_control_get(dev->of_node, id, index, shared,
 					      optional, acquired);
+	if ((rstc == NULL || PTR_ERR(rstc) == -ENOENT) && index == 0 &&
+	    shared == 0)
+		rstc = __reset_control_get_gpio(dev, id, optional, acquired);
+
+	if (rstc == NULL || PTR_ERR(rstc) == -ENOENT)
+		rstc = __reset_control_get_from_lookup(dev, id, shared,
+						       optional, acquired);
 
-	return __reset_control_get_from_lookup(dev, id, shared, optional,
-					       acquired);
+	return rstc;
 }
 EXPORT_SYMBOL_GPL(__reset_control_get);
 
---------->8----------

regards
Philipp