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

[Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Am Dienstag, den 19.02.2013, 14:57 -0700 schrieb Stephen Warren:
> On 02/19/2013 04:35 AM, Philipp Zabel wrote:
> > 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.
> 
> > diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt
> 
> > +Required properties:
> 
> > +- 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.

> > diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
> 
> > +static int gpio_reset_probe(struct platform_device *pdev)
> 
> > +	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.

> > +		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.

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

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