All of lore.kernel.org
 help / color / mirror / Atom feed
From: Philipp Zabel <p.zabel@pengutronix.de>
To: Stephen Warren <swarren@wwwdotorg.org>
Cc: Mark Rutland <mark.rutland@arm.com>, Marek Vasut <marex@denx.de>,
	Mike Turquette <mturquette@linaro.org>,
	Fabio Estevam <fabio.estevam@freescale.com>,
	Len Brown <len.brown@intel.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	"Rafael J. Wysocki" <rjw@sisk.pl>, Pavel Machek <pavel@ucw.cz>,
	"devicetree-discuss@lists.ozlabs.org"
	<devicetree-discuss@lists.ozlabs.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	Roger Quadros <rogerq@ti.com>
Subject: Re: [PATCH v10] reset: Add driver for gpio-controlled reset pins
Date: Tue, 06 Aug 2013 09:32:25 +0200	[thread overview]
Message-ID: <1375774345.4004.25.camel@pizza.hi.pengutronix.de> (raw)
In-Reply-To: <51FFDFB7.6080806@wwwdotorg.org>

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

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

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.

regards
Philipp

WARNING: multiple messages have this Message-ID (diff)
From: p.zabel@pengutronix.de (Philipp Zabel)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v10] reset: Add driver for gpio-controlled reset pins
Date: Tue, 06 Aug 2013 09:32:25 +0200	[thread overview]
Message-ID: <1375774345.4004.25.camel@pizza.hi.pengutronix.de> (raw)
In-Reply-To: <51FFDFB7.6080806@wwwdotorg.org>

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

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

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.

regards
Philipp

  reply	other threads:[~2013-08-06  7:32 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-18  9:26 [PATCH v10] reset: Add driver for gpio-controlled reset pins Philipp Zabel
2013-07-18  9:26 ` Philipp Zabel
     [not found] ` <1374139586-6344-1-git-send-email-p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2013-07-18 12:06   ` Shawn Guo
2013-07-18 12:06     ` Shawn Guo
2013-08-02  9:28 ` Mark Rutland
2013-08-02  9:28   ` Mark Rutland
2013-08-02 20:09   ` Stephen Warren
2013-08-02 20:09     ` Stephen Warren
2013-08-05 15:13     ` Mark Rutland
2013-08-05 15:13       ` Mark Rutland
2013-08-05 17:22       ` Stephen Warren
2013-08-05 17:22         ` Stephen Warren
2013-08-05  7:32   ` Philipp Zabel
2013-08-05  7:32     ` Philipp Zabel
2013-08-05 15:35     ` Mark Rutland
2013-08-05 15:35       ` Mark Rutland
2013-08-06  7:38       ` Roger Quadros
2013-08-06  7:38         ` Roger Quadros
2013-08-05 17:24     ` Stephen Warren
2013-08-05 17:24       ` Stephen Warren
2013-08-06  7:32       ` Philipp Zabel [this message]
2013-08-06  7:32         ` Philipp Zabel
2013-08-06 16:59         ` Stephen Warren
2013-08-06 16:59           ` Stephen Warren
2013-08-08  9:42           ` Philipp Zabel
2013-08-08 18:43             ` Stephen Warren
2013-08-12 11:04               ` Philipp Zabel
2013-08-12 15:50                 ` Stephen Warren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1375774345.4004.25.camel@pizza.hi.pengutronix.de \
    --to=p.zabel@pengutronix.de \
    --cc=devicetree-discuss@lists.ozlabs.org \
    --cc=fabio.estevam@freescale.com \
    --cc=len.brown@intel.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=marex@denx.de \
    --cc=mark.rutland@arm.com \
    --cc=mturquette@linaro.org \
    --cc=pavel@ucw.cz \
    --cc=rjw@sisk.pl \
    --cc=rogerq@ti.com \
    --cc=s.hauer@pengutronix.de \
    --cc=swarren@wwwdotorg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.