[PATCH v10] reset: Add driver for gpio-controlled reset pins

[Philipp Zabel]

Hi Stephen

Am Dienstag, den 06.08.2013, 10:59 -0600 schrieb Stephen Warren:
> On 08/06/2013 01:32 AM, Philipp Zabel wrote:
> > Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:
> >> On 08/05/2013 01:32 AM, Philipp Zabel wrote:
> >>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
> >>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
> >> ...
> >>>> I think this should look more like the below:
> >>>>
> >>>> /* Device with nRESET pin connected to GPIO5_0 */
> >>>> sii902x at 39 {
> >>>> 	/* named for the actual input line */
> >>>> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> >>>> 	/* 
> >>>> 	 * If there's some configurable property relating to the reset
> >>>> 	 * line, we can describe it
> >>>> 	 */
> >>>> 	vendor,some-optional-reset-gpio-property;
> >>>> 	...
> >>>> };
> >>>
> >>> I don't like the arbitrary name, as that makes it difficult to handle
> >>> this in an automated way. In this case I'd prefer to use 'reset-gpios'
> >>> and optionally 'reset-gpio-names' analogous to how clocks and interrupts
> >>> (and resets) are handled.
> >>
> >> Hmm. Just be aware that you can't force existing bindings to be
> >> retro-actively modified, or you'll break the DT ABI. So, at the very
> >> least we'd have to allow the existing custom-property-based approach for
> >> bindings where it's already been used.
> > 
> > Of course we have to continue supporting existing bindings. We could
> > continue using the GPIO API directly for those cases, or we could add a
> > function similar to of_get_named_gpio to wrap the GPIO:
> > 
> > 	rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0);
> > 	reset_control_assert(rstc);
> > 	usleep(1000);
> > 	reset_control_deassert(rstc);
> 
> Well, you'd need to pass two names into that function; one would be the
> name of the legacy property and the other the entry in reset-names or
> reset-gpio-names. It's quite unlikely that the same string would be used
> in both places.

there is no reset-names here. The legacy properties are only one GPIO
per property or addressed by index, currently. I don't want to change
that.

> > My point is that for new bindings I'd prefer a well known property name
> > such as reset-gpios and a -names string list (as described
> > Documentation/devicetree/bindings/resource-names.txt) over ad-hoc
> > property names such as nreset-gpios, <vendor>-<submodule>-(n)reset,
> > nrst-gpios etc., both for consistency with existing resource properties
> > and because it is easier to grep for a single property name than for a
> > combination of all possible datasheet spellings of "reset".

^ This is my main concern.

> > I'd like
> > 	rstc = reset_control_get(dev, "nreset");
> > to go look for
> > 	resets = <&src 3>;
> > 	reset-names = "nreset";
> > or for
> > 	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> > 	reset-gpio-names = "nreset";
> > by default.
> 
> That's rather complex for little benefit though.

In the majority of cases there will only be one reset per device. In
this case, the supplemental names property is not needed. The
resets/reset-names binding is using this scheme already, so using the
same for reset-gpios improves consistency.
If it weren't for the customary *-gpios property names, I'd suggest to
allow using GPIOs in the resets property directly:
	resets = <&gpio5 0 GPIO_ACTIVE_LOW>;

> Take a look at existing
> plain GPIO bindings, regulators, etc. They all simply encode the name
> you're looking for directly into the property name, which is a lot less
> overhead; simpler for humans to write and read without having to match n
> properties together, and simpler to parse in code.

It is not simpler to parse for humans if there is not a clear naming
scheme. For regulators, I can at least grep for the common "-supply"
suffix to recognize regulator supplies. But the "nrst-gpios" won't be
easily found. And if the datasheet calls the pin "RESET_N", or even
"CE", how will I call the reset property? I'd strongly prefer to
standardize on a recognizable name.

regards
Philipp