Re: [PATCH v3 8/8] reset: Add driver for gpio-controlled reset pins

Am Dienstag, den 19.02.2013, 14:57 -0700 schrieb Stephen Warren:

On 02/19/2013 04:35 AM, Philipp Zabel wrote:

quoted

This driver implements a reset controller device that toggles gpios
connected to reset pins of peripheral ICs. The delay between assertion
and de-assertion of the reset signal can be configured.

quoted

diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt

quoted

+Required properties:

quoted

+- reset-delays: List of delays in ms. The corresponding gpio reset line is
+                asserted for this duration to reset.

mS are quite long. Would it make sense for this property to be uS instead?

All GPIO resets that I've seen so far wait for several milliseconds.
But I see no downside here to using microseconds instead.

quoted

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

quoted

+static int gpio_reset_probe(struct platform_device *pdev)

quoted

+	if (of_find_property(np, "reset-delays", NULL)) {
+		delays = devm_kzalloc(&pdev->dev, sizeof(u32) *
+				drvdata->nr_gpios, GFP_KERNEL);
+		if (delays == NULL)
+			return -ENOMEM;

It'd be nice if there were something like of_property_read_u32_index()
so you could read each value one-by-one in the loop later on, rather
than dynamically allocating this temporarily.

Yes. Let's defer that to a separate patch.

quoted

+		ret = of_property_read_u32_array(np, "reset-delays", delays,
+				drvdata->nr_gpios);
+		if (ret < 0)
+			return ret;
+	}
+
+	for (i = 0; i < drvdata->nr_gpios; i++) {
+		drvdata->gpios[i].gpio = of_get_named_gpio_flags(np,
+				"reset-gpios", i, &flags);
+		if (drvdata->gpios[i].gpio < 0) {
+			dev_err(&pdev->dev, "invalid gpio for reset %d\n", i);

It's not an error if the value is -EPROBE_DEFERRED; you might want to
explicitly check for that case and not print anything?

Right, I'll change that.

quoted

+			return drvdata->gpios[i].gpio;
+		}
+
+		/*
+		 * The flags are also used to remember whether a given GPIO
+		 * reset is active-low.
+		 */
+		if (flags & OF_GPIO_ACTIVE_LOW)
+			drvdata->gpios[i].flags = GPIOF_OUT_INIT_HIGH;
+		else
+			drvdata->gpios[i].flags = GPIOF_OUT_INIT_LOW;

That raises the question: What is the initial reset state expected to
be? Some devices might want to stay in reset until their driver
explicitly removes the reset signal.

We could handle that by adding another (optional) property indicating
the initial reset state of each GPIO; default to initially not-in-reset
unless that property exists and specifies initially-in-reset.

As with the time parameter, I wonder if this configuration is something
we want to have in the consumer device tree node, or in the gpio-reset
device node:

	gpio_reset: gpio-reset {
		compatible = "gpio-reset";
		reset-gpios = <&gpio2 15 0>;
		reset-delays = <1000>; /* 1 ms */
		initially-in-reset = <1>;
	}
	some-device {
		resets = <&gpio_reset 0>;
	}
vs.
	gpio_reset: gpio-reset {
		compatible = "gpio-reset";
		reset-gpios = <&gpio2 15 0>;
	}
	some-device {
		resets = <&gpio_reset 0 1000>; /* 1 ms */
		initially-in-reset;
	}

quoted

+		ret = devm_gpio_request_one(&pdev->dev, drvdata->gpios[i].gpio,
+				drvdata->gpios[i].flags, NULL);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to request gpio %d for reset %d\n",
+					drvdata->gpios[i].gpio, i);
+			return ret;
+		}

Perhaps first loop to look up all the GPIOs and initialize data
structures, then loop to request the GPIOs? That'd prevent lots of HW
programming and de-programming for the GPIOs near the start of the list,
in the case where some later GPIO causes -EPROBE_DEFERRED, and so this
probe() function keeps getting executed over and over.

quoted

+		if (delays != NULL)
+			drvdata->gpios[i].delay_ms = delays[i];
+		else
+			drvdata->gpios[i].delay_ms = -1; /* .reset returns -ENOSYS */

(here is where of_property_read_u32_index() might be handy)

quoted

+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);

You might want to add a few things like MODULE_AUTHOR,
MODULE_DESCRIPTION,  MODULE_LICENSE,
MODULE_DEVICE_TABLE(of, gpio_reset_dt_ids), perhaps MODULE_ALIAS for the
platform device too.

Ok.

thanks
Philipp