public inbox for linux-arm-kernel@lists.infradead.org 
 help / color / mirror / Atom feed
From: tony@atomide•com (Tony Lindgren)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v8 12/12] document: devicetree: bind pinconf with pin single
Date: Mon, 4 Feb 2013 20:07:23 -0800	[thread overview]
Message-ID: <20130205040721.GE25185@atomide.com> (raw)
In-Reply-To: <1359825953-15663-13-git-send-email-haojian.zhuang@linaro.org>

* Haojian Zhuang <haojian.zhuang@linaro•org> [130202 09:32]:
> +- pinctrl-single,drive-strength : array of value that are used to configure
> +  drive strength in the pinmux register. They're value of drive strength
> +  current and drive strength mask.
> +
> +		/* drive strength current, mask */
> +		pinctrl-single,power-source = <0x30 0xf0>;
> +

Looks like a typo, this should probably say pinctrl-single,drive-strength
here instead of power-source?

For the cases where there's a value to be set I'm still wondering how does
a client driver change the value if that needs to be changed dynamically?
Can the value be passed as value + PIN_CONFIG_END?

> +- pinctrl-single,bias-disable : array of value that are used to configure the
> +  input bias disabled in the pinmux register. They're value of bias value,
> +  match bias disabled value and bias disabled mask.
> +
> +		/* bias value, match bias disabled value, mask */
> +		pinctrl-single,bias-disable = <2 0 3>;
> +

I still suggest we use "enable" naming instead of "disable" naming.
So pinctrl-single,bias-enable instead of pinctrl-single,bias-disable.

Then let's change the binding slightly to make it more readable:

		/* enable value, disable value, mask */
		pinctrl-single,bias-enable = <2 0 3>;

For example, I have bit 22 cleared to enable MMC PBIAS, or bit 22
set to disable PBIAS:

		                             /* enable disable  mask */
		pinctrl-single,bias-enable = <0x000000 0x400000 0x400000>;

Then I think pretty much the only change to pinconf_get() would be:

	data = pcs->read(pcs->base + offset) & func->conf[i].mask;

	switch (func->conf[i].param) {
	/* 3 parameters */
	case PIN_CONFIG_BIAS_ENABLE:
	case PIN_CONFIG_BIAS_PULL_DOWN:
	case PIN_CONFIG_BIAS_PULL_UP:
	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
	*config = data;
	if (data == func->conf[i].enable)
		res = 0;
	else
		res = -ENOTSUPP;
	break;
	...

And then the client driver can always assume that if the return value
is 0, the ENABLE value in question is set. And if -ENOTSUPP (or how about
-ENOTSET :) is returned, the ENABLE value is not set?

Then regarding pinconf_set(), don't we also need pinconf_clear() in
addition to pinconf_set()?

Otherwise we need both ENABLE and DISABLE enumeration, which seems
unnecessary to me. So rather than have both PIN_CONFIG_BIAS_ENABLE and
PIN_CONFIG_BIAS_DISABLE, we just need PIN_CONFIG_BIAS_ENABLE.

> +- pinctrl-single,bias-pullup : array of value that are used to configure the
> +  input bias pullup in the pinmux register. They're value of bias value,
> +  match bias pullup value and bias pullup mask.
> +
> +		/* bias value, match bias pullup value, mask */
> +		pinctrl-single,bias-pullup = <0 1 1>;

It seems that you could then use same generic comment in all places
then to make it a bit easier to read:

		/* enable value, disable value, mask */
		pinctrl-single,bias-pullup = <0 1 1>;
...

Regards,

Tony

  reply	other threads:[~2013-02-05  4:07 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-02 17:25 [PATCH v8 00/12] support pinconf in pinctrl single Haojian Zhuang
2013-02-02 17:25 ` [PATCH v8 01/12] gpio: add gpio offset in gpio range cells property Haojian Zhuang
2013-02-05  0:23   ` Tony Lindgren
2013-02-05  1:06     ` Haojian Zhuang
2013-02-10 19:03   ` Linus Walleij
2013-02-11  4:25     ` Viresh Kumar
2013-04-29 16:00   ` [v8,01/12] " James Hogan
2013-04-29 16:49     ` Haojian Zhuang
2013-04-29 20:16       ` James Hogan
2013-02-02 17:25 ` [PATCH v8 02/12] gpio: fix wrong checking condition for gpio range Haojian Zhuang
2013-02-05 17:02   ` Linus Walleij
2013-02-02 17:25 ` [PATCH v8 03/12] gpio: find gpio base by ascend order Haojian Zhuang
2013-02-05 17:14   ` Linus Walleij
2013-02-06  1:59     ` Haojian Zhuang
2013-02-06  4:33       ` Alex Courbot
2013-02-06  5:20         ` Haojian Zhuang
2013-02-06  8:44       ` Linus Walleij
2013-02-06  9:15         ` Haojian Zhuang
2013-02-02 17:25 ` [PATCH v8 04/12] gpio: pl061: allocate irq dynamically Haojian Zhuang
2013-02-02 17:25 ` [PATCH v8 05/12] pinctrl: verify whether gpio chip overlapps range Haojian Zhuang
2013-02-02 17:25 ` [PATCH v8 06/12] gpio: pl061: bind pinctrl by gpio request Haojian Zhuang
2013-02-02 17:25 ` [PATCH v8 07/12] pinctrl: single: create new gpio function range Haojian Zhuang
2013-02-02 17:25 ` [PATCH v8 08/12] pinctrl: generic: dump pin configuration Haojian Zhuang
2013-02-05  0:35   ` Tony Lindgren
2013-02-05  0:57     ` Tony Lindgren
2013-02-05  1:09       ` Haojian Zhuang
2013-02-05  1:08     ` Haojian Zhuang
2013-02-02 17:25 ` [PATCH v8 09/12] pinctrl: single: set function mask as optional Haojian Zhuang
2013-02-02 17:25 ` [PATCH v8 10/12] pinctrl: single: support generic pinconf Haojian Zhuang
2013-02-05  0:46   ` Tony Lindgren
2013-02-05  1:07     ` Haojian Zhuang
2013-02-02 17:25 ` [PATCH v8 11/12] ARM: hs: enable hi4511 with device tree Haojian Zhuang
2013-02-02 17:25 ` [PATCH v8 12/12] document: devicetree: bind pinconf with pin single Haojian Zhuang
2013-02-05  4:07   ` Tony Lindgren [this message]
2013-02-05  8:06     ` Haojian Zhuang
2013-02-05 13:51       ` Haojian Zhuang
2013-02-05 23:30         ` Tony Lindgren
2013-02-06 15:07           ` Haojian Zhuang
2013-02-06 17:16             ` Tony Lindgren
2013-02-05 23:21       ` Tony Lindgren
  -- strict thread matches above, loose matches on Subject: below --
2013-02-11 17:10 [PATCH v8 00/12] bind pinconf with pinctrl single Haojian Zhuang
2013-02-11 17:10 ` [PATCH v8 12/12] document: devicetree: bind pinconf with pin single Haojian Zhuang
2013-02-13 23:51   ` Tony Lindgren

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=20130205040721.GE25185@atomide.com \
    --to=tony@atomide$(echo .)com \
    --cc=linux-arm-kernel@lists$(echo .)infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox