devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

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