All of lore.kernel.org
 help / color / mirror / Atom feed
From: swarren@wwwdotorg.org (Stephen Warren)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] pinctrl/nomadik: Add "ste,config" property
Date: Fri, 04 Jan 2013 13:02:33 -0700	[thread overview]
Message-ID: <50E73559.60508@wwwdotorg.org> (raw)
In-Reply-To: <1357316010-677-1-git-send-email-linus.walleij@stericsson.com>

On 01/04/2013 09:13 AM, Linus Walleij wrote:
> From: Gabriel Fernandez <gabriel.fernandez@stericsson.com>
> 
> The "ste,config" property will contain the pin config node.
> It will be easier to define a pin configuration and use it by
> reference without duplicating lines tedious.
> 
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@stericsson.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> diff --git a/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt b/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt

> -- ste,input : <0/1/2/3>
> +- ste,config: Handle of pin configuration node (ste,config = <&in_pu>)
> +
> +- ste,input : <0/1/2>

The changes to ste,input and ste,sleep-output look like some unrelated
change.

I guess the idea of ste,input is quite neat, but ...

>  Example board file extract:
>  
> +	in_pu: input_pull_up {
> +		ste,input = <1>;
> +	};

... these nodes shouldn't be placed at the top-level of the device tree;
housing them inside the pin controller node itself makes much more sense
since the pin controller binding is able/allowed to define what goes
inside the pin controller node, but shouldn't influence the top-level of
the device tree.

> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c

> +	np_config = of_parse_phandle(np, "ste,config", 0);
> +	if (np_config) {
> +		for (i = 0; i < ARRAY_SIZE(nmk_cfg_params); i++) {
> +			unsigned long cfg = 0;
> +			int val;

Is it worth making ste,config optional, so that config properties can be
placed either into the node directly, or into a node referenced by
ste,config? That might make doing one-off unusual configurations easier
- no need to create a separate node that's only used once. Still, if
that's unlikely on your HW, it's probably no big deal.

WARNING: multiple messages have this Message-ID (diff)
From: Stephen Warren <swarren@wwwdotorg.org>
To: Linus Walleij <linus.walleij@stericsson.com>
Cc: linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Stephen Warren <swarren@nvidia.com>,
	Anmar Oueja <anmar.oueja@linaro.org>,
	Gabriel Fernandez <gabriel.fernandez@stericsson.com>,
	Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH] pinctrl/nomadik: Add "ste,config" property
Date: Fri, 04 Jan 2013 13:02:33 -0700	[thread overview]
Message-ID: <50E73559.60508@wwwdotorg.org> (raw)
In-Reply-To: <1357316010-677-1-git-send-email-linus.walleij@stericsson.com>

On 01/04/2013 09:13 AM, Linus Walleij wrote:
> From: Gabriel Fernandez <gabriel.fernandez@stericsson.com>
> 
> The "ste,config" property will contain the pin config node.
> It will be easier to define a pin configuration and use it by
> reference without duplicating lines tedious.
> 
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@stericsson.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> diff --git a/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt b/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt

> -- ste,input : <0/1/2/3>
> +- ste,config: Handle of pin configuration node (ste,config = <&in_pu>)
> +
> +- ste,input : <0/1/2>

The changes to ste,input and ste,sleep-output look like some unrelated
change.

I guess the idea of ste,input is quite neat, but ...

>  Example board file extract:
>  
> +	in_pu: input_pull_up {
> +		ste,input = <1>;
> +	};

... these nodes shouldn't be placed at the top-level of the device tree;
housing them inside the pin controller node itself makes much more sense
since the pin controller binding is able/allowed to define what goes
inside the pin controller node, but shouldn't influence the top-level of
the device tree.

> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c

> +	np_config = of_parse_phandle(np, "ste,config", 0);
> +	if (np_config) {
> +		for (i = 0; i < ARRAY_SIZE(nmk_cfg_params); i++) {
> +			unsigned long cfg = 0;
> +			int val;

Is it worth making ste,config optional, so that config properties can be
placed either into the node directly, or into a node referenced by
ste,config? That might make doing one-off unusual configurations easier
- no need to create a separate node that's only used once. Still, if
that's unlikely on your HW, it's probably no big deal.

  reply	other threads:[~2013-01-04 20:02 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-01-04 16:13 [PATCH] pinctrl/nomadik: Add "ste,config" property Linus Walleij
2013-01-04 16:13 ` Linus Walleij
2013-01-04 20:02 ` Stephen Warren [this message]
2013-01-04 20:02   ` Stephen Warren
2013-01-08 11:02   ` Linus Walleij
2013-01-08 11:02     ` Linus Walleij

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=50E73559.60508@wwwdotorg.org \
    --to=swarren@wwwdotorg.org \
    --cc=linux-arm-kernel@lists.infradead.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.