From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
Haojian Zhuang <haojian.zhuang@linaro.org>,
"linux-sh@vger.kernel.org" <linux-sh@vger.kernel.org>,
"devicetree-discuss@lists.ozlabs.org"
<devicetree-discuss@lists.ozlabs.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
Magnus Damm <damm@opensource.se>,
Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Subject: Re: [PATCH v4 02/21] sh-pfc: Add DT support
Date: Wed, 29 May 2013 10:01:05 +0200 [thread overview]
Message-ID: <1544149.k900vg9Sxe@avalon> (raw)
In-Reply-To: <CACRpkdZNv5AO48Bmquyr8mBQvgjBMCk3Ob8ngutSq8RV1HFT0w@mail.gmail.com>
Hi Linus,
Thanks for the review.
On Friday 24 May 2013 10:52:42 Linus Walleij wrote:
> On Tue, May 21, 2013 at 2:14 PM, Laurent Pinchart wrote:
> > Support device instantiation through the device tree. The compatible
> > property is used to select the SoC pinmux information.
> >
> > Set the gpio_chip device field to the PFC device to enable automatic
> > GPIO OF support.
> >
> > Cc: devicetree-discuss@lists.ozlabs.org
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
>
> (...)
>
> (Again I'm rather nervous about all the device tree binding reviewing being
> dropped in the lap of Linux subsystem maintainers and then expecting them
> to be good and OS-neutral... Not your fault though.)
I agree with you. I'm open to review from other OS vendors :-)
> > +Pin Configuration Node Properties:
> > +
> > +- renesas,pins : An array of strings, each string containing the name of
> > + a pin.
> > +- renesas,groups : An array of strings, each string containing the name
> > + of a pin group.
> > +
> > +- renesas,function: A string containing the name of the function to mux
> > + to the pin group(s) specified by the renesas,groups property
> > +
> > + Valid values for pin, group and function names can be found in the
> > + group and function arrays of the PFC data file corresponding to the
> > + SoC (drivers/pinctrl/sh-pfc/pfc-*.c)
> > +
> > +- renesas,pull-up: An integer representing the pull-up strength to be
> > + applied to all pins specified by the renesas,pins and renesas-groups
> > + properties. 0 disables the pull-up, 1 enables it. Other values should
> > + not be used.
>
> Then just use a boolean.
I meant that other values should not be used for now. Future revisions of the
GPIO core might allow controlling the pull-up strength, hence the integer
property. Same for the pull-down property. This is especially true if we
device to implement generic pinconf bindings.
> > +- renesas,pull-down: An integer representing the pull-down strength to be
> > + applied to all pins specified by the renesas,pins and renesas-groups
> > + properties. 0 disables the pull-down, 1 enables it. Other values should
> > + not be used.
>
> Just use a boolean oneliner then.
>
> But I prefer that you define something really generic in
> Documentation/devicetree/bindings/pinconf.txt for anyone using
> generic pin config, then reference that.
>
> This way we can have a more general config binding for any system
> using generic pin config, mapping to the configs we have in
> <linux/pinctrl/pinconf-generic.h>
>
> The upside is that we could move the pinconf generic parsing code
> to drivers/pinctrl/pinconf-generic.c and thus avoid code duplication.
>
> We have so far not tried much to standardize pinctrl bindings, but
> this would be a good opportunity.
I agree. I'll reply to the "[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl
driver" mail thread to further discuss the pinconf generic bindings.
> > +On SH7372, SH73A0, R8A73A4 and R8A7740 the PFC node is also a GPIO
> > +controller node.
> > +
> > +Required Properties:
> > +
> > + - gpio-controller: Marks the device node as a gpio controller.
> > +
> > + - #gpio-cells: Should be 2. The first cell is the pin number and the
> > + second cell is used to specify optional parameters as bit flags.
> > + Only the GPIO active low flag (bit 0) is currently supported.
>
> I suggest these properties shall be defined in an include file using that
> new include hierarchy, since you're using pinconf generic.
>
> include/dt-bindings/gpio/gpio.h
> Referenced by <dt-bindings/gpio/gpio.h> in the .dts[i] files.
Agreed, I'll fix that.
> In Linux-next you find how tegra machines and the imx6sl DTs have
> started to use this facility for symbolic names.
>
> And for that you should reference
> <dt-bindings/gpio/gpio.h>
>
> Use the symbolic names GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
> here.
>
> (...)
>
> > +Example 2: A GPIO LED node that references a GPIO
> > +
> > + leds {
> > + compatible = "gpio-leds";
> > + led1 {
> > + gpios = <&pfc 20 1>; /* Active low */
>
> #include <dt-bindings/gpio/gpio.h>
> gpios = <&pfc 20 GPIO_ACTIVE_LOW>;
>
> (...)
>
> > +Example 3: KZM-A9-GT (SH-Mobile AG5) default pin state hog and pin
> > control maps + for the MMCIF and SCIFA4 devices
> > +
> > + &pfc {
> > + pinctrl-0 = <&scifa4_pins>;
> > + pinctrl-names = "default";
> > +
> > + mmcif_pins: mmcif {
> > + mux {
> > + renesas,groups = "mmc0_data8_0",
> > "mmc0_ctrl_0"; + renesas,function = "mmc0";
> > + };
> > + cfg {
> > + renesas,groups = "mmc0_data8_0";
> > + renesas,pins = "PORT279";
> > + renesas,pull-up = <1>;
>
> Just
> renesas,pull-up;
>
> Or go for the generic pinconf binding I'm suggesting.
> Then I guess it'd be something like:
>
> pinconf-bias-pull-up;
>
> (Quite readable.)
>
> (...)
>
> > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c
> > +static const struct sh_pfc_config_param sh_pfc_config_params[] = {
> > + { "renesas,pull-up", PIN_CONFIG_BIAS_PULL_UP },
> > + { "renesas,pull-down", PIN_CONFIG_BIAS_PULL_DOWN },
> > +};
>
> So these should be checked as booleans instead:
>
> > + for (i = 0; i < ARRAY_SIZE(sh_pfc_config_params); ++i) {
> > + const struct sh_pfc_config_param *param =
> > + &sh_pfc_config_params[i];
> > + unsigned long config;
> > + u32 val;
> > +
> > + ret = of_property_read_u32(np, param->name, &val);
>
> of_property_read_bool()
>
> Though I much rather like this code added as helper lib in pinconf-
> generic.c. Use drivers/pinctrl/pinconf.h for API.
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2013-05-29 8:01 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-21 12:14 [PATCH v4 00/21] SH pinctrl DT support Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 01/21] sh-pfc: Remove support for platform data Laurent Pinchart
2013-05-24 8:23 ` Linus Walleij
2013-05-21 12:14 ` [PATCH v4 02/21] sh-pfc: Add DT support Laurent Pinchart
2013-05-24 8:52 ` Linus Walleij
2013-05-29 8:01 ` Laurent Pinchart [this message]
2013-05-21 12:14 ` [PATCH v4 03/21] ARM: shmobile: r8a73a4: Add pin control device to device tree Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 04/21] ARM: shmobile: r8a7740: " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 05/21] ARM: shmobile: r8a7778: " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 06/21] ARM: shmobile: r8a7778: Add GPIO controller devices " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 07/21] ARM: shmobile: r8a7779: Add pin control device " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 08/21] ARM: shmobile: r8a7779: Add GPIO controller devices " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 09/21] ARM: shmobile: r8a7790: Add pin control device " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 10/21] ARM: shmobile: r8a7790: Add GPIO controller devices " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 11/21] ARM: shmobile: sh7372: Add pin control device " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 12/21] ARM: shmobile: sh73a0: " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 13/21] ARM: shmobile: armadillo-reference: Move pinctrl mappings " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 14/21] ARM: shmobile: armadillo-reference: Add st1232 pin mappings Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 15/21] ARM: shmobile: armadillo-reference: Move st1232 reset GPIO to DT Laurent Pinchart
2013-05-24 8:59 ` Linus Walleij
2013-05-24 12:24 ` Magnus Damm
2013-05-29 8:01 ` Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 16/21] ARM: shmobile: armadillo-reference: Add LED1-LED4 to the device tree Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 17/21] ARM: shmobile: kzm9g-reference: Move pinctrl mappings to " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 18/21] ARM: shmobile: kzm9g-reference: Move SDHI regulators to DT Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 19/21] ARM: shmobile: kzm9g-reference: Add LED1-LED4 to the device tree Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 20/21] ARM: shmobile: marzen-reference: Move pinctrl mappings to " Laurent Pinchart
2013-05-21 12:14 ` [PATCH v4 21/21] ARM: shmobile: marzen-reference: Add LED2-LED4 to the " Laurent Pinchart
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=1544149.k900vg9Sxe@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=damm@opensource.se \
--cc=devicetree-discuss@lists.ozlabs.org \
--cc=g.liakhovetski@gmx.de \
--cc=haojian.zhuang@linaro.org \
--cc=laurent.pinchart+renesas@ideasonboard.com \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-sh@vger.kernel.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).