From patchwork Thu May 16 11:53:25 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2577561 Return-Path: X-Original-To: patchwork-linux-sh@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id AC6A03FE1F for ; Thu, 16 May 2013 11:53:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753257Ab3EPLxJ (ORCPT ); Thu, 16 May 2013 07:53:09 -0400 Received: from perceval.ideasonboard.com ([95.142.166.194]:47744 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752871Ab3EPLxH (ORCPT ); Thu, 16 May 2013 07:53:07 -0400 Received: from avalon.localnet (unknown [91.177.152.10]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id F27D135A4C; Thu, 16 May 2013 13:53:04 +0200 (CEST) From: Laurent Pinchart To: Guennadi Liakhovetski Cc: Laurent Pinchart , linux-sh@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org, Linus Walleij , Magnus Damm , Guennadi Liakhovetski Subject: Re: [PATCH v3 01/20] sh-pfc: Add DT support Date: Thu, 16 May 2013 13:53:25 +0200 Message-ID: <1711141.qqk6hopDLW@avalon> User-Agent: KMail/4.10.2 (Linux/3.7.10-gentoo-r1; KDE/4.10.2; x86_64; ; ) In-Reply-To: References: <1368577100-3530-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1368577100-3530-2-git-send-email-laurent.pinchart+renesas@ideasonboard.com> MIME-Version: 1.0 Sender: linux-sh-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-sh@vger.kernel.org Hi Guennadi, On Wednesday 15 May 2013 16:03:10 Guennadi Liakhovetski wrote: > Hi Laurent > > Thanks for your work on this! Sorry for jumping in so late in the game. No worries. > Let's do it this way: I don't think my comments are serious enough to > enforce a v4. If noone else complains, I'm fine with addressing them in an > incremental patch. If you get more comments and have to do a v4, you can > address mine too, otherwise I'm fine with this version going in and > improving it slightly afterwards. I don't mind submitting a v4. > On Wed, 15 May 2013, 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 > > > > --- > > > > .../bindings/pinctrl/renesas,pfc-pinctrl.txt | 143 ++++++++++++ > > drivers/pinctrl/sh-pfc/core.c | 66 +++++- > > drivers/pinctrl/sh-pfc/pinctrl.c | 244 ++++++++++++++++ > > 3 files changed, 451 insertions(+), 2 deletions(-) > > create mode 100644 > > Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt> > > diff --git > > a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt > > b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt new > > file mode 100644 > > index 0000000..e7aae39 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt > > @@ -0,0 +1,143 @@ > > +* Renesas Pin Function Controller (GPIO and Pin Mux/Config) > > + > > +The Pin Function Controller (PFC) is a Pin Mux/Config controller. On > > SH7372, +SH73A0, R8A73A4 and R8A7740 it also acts as a GPIO controller. > > + > > + > > +Pin Control > > +----------- > > + > > +Required Properties: > > + > > + - compatible: should be one of the following. > > + - "renesas,pfc-r8a73a4": for R8A73A4 (R-Mobile APE6) compatible > > pin-controller. > > + - "renesas,pfc-r8a7740": for R8A7740 (R-Mobile A1) compatible pin- > > controller. > > + - "renesas,pfc-r8a7778": for R8A7778 (R-Mobile M1) compatible pin- > > controller. > > + - "renesas,pfc-r8a7779": for R8A7779 (R-Car H1) compatible pin- > > controller. > > + - "renesas,pfc-r8a7790": for R8A7790 (R-Car H2) compatible pin- > > controller. > > + - "renesas,pfc-sh7372": for SH7372 (SH-Mobile AP4) compatible > > pin-controller. > > + - "renesas,pfc-sh73a0": for SH73A0 (SH-Mobile AG5) compatible pin- > > controller. > > + > > + - reg: Base address and length of each memory resource used by the pin > > + controller hardware module. > > + > > +The PFC node also acts as a container for pin configuration nodes. Please > > +refer to pinctrl-bindings.txt in this directory for the definition of the > > +term "pin configuration node" and for the common pinctrl bindings used by > > +client devices. > > + > > +Each pin configuration node represents a desired configuration for a pin, > > +a pin group, or a list of pins or pin groups. The configuration can > > +include the function to select on those pin(s) and pin configuration > > +parameters (such as +pull-up and pull-down). > > + > > +Pin configuration nodes contain pin configuration properties, either > > +directly or grouped in child subnodes. Both pin muxing and configuration > > +parameters can be grouped in that way and referenced as a single pin > > +configuration node by client devices. > > + > > +A configuration node or subnode must reference at least one pin (through > > +the pins or pin groups properties) and contain at least a function or one > > +configuration parameter. When the function is present only pin groups can > > +be used to reference pins. > > + > > +All pin configuration nodes and subnodes names are ignored. All of those > > +nodes are parsed through phandles and processed purely based on their > > +content. > > + > > +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. > > +- 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. > > + > > + > > +GPIO > > +---- > > + > > +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. > > + > > +The syntax of the gpio specifier used by client nodes should be the > > +following with values derived from the SoC user manual. > > + > > + <[phandle of the gpio controller node] > > + [pin number within the gpio controller] > > + [flags and pull up/down]> > > How should pull up / down be specified? Above you say only "active low" is > supported so far. My bad. It should be [flags] only. Pull-up and pull-down configuration should go through the renesas,pull-up and renesas,pull-down properties in pin configuration nodes. > Actually, I'm a bit confused by how pinctrl and GPIO DT nodes should be > implemented: > > > + > > + > > +Examples > > +-------- > > + > > +Example 1: SH73A0 (SH-Mobile AG5) pin controller node > > + > > + pfc: pfc@e6050000 { > > + compatible = "renesas,pfc-sh73a0"; > > + reg = <0xe6050000 0x8000>, > > + <0xe605801c 0x1c>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + }; > > In this example you add one node, that implements both - a pinctrl and a > GPIO controller. However, on some platforms you add two DT nodes: one for > pinctrl and one for GPIO. While doing that your GPIO node has a compatible > string like > > compatible = "renesas,gpio-r8a7778", "renesas,gpio-rcar"; > > Do I understand it right, that a separate DT node is used for a GPIO > controller if the controllers are really well separated on the hardware, > probably, don't share register address ranges? Is there a driver in the > kernel, that actually probes those compatible strings? Or the driver for > those GPIO controllers isn't DT based? On platforms where the GPIO controller hardware is separate from the PFC hardware, GPIOs are handled by the gpio-rcar driver. I've added DT support to gpio-rcar, the patches have been posted to the linux-sh mailing list and are in my tree at git://linuxtv.org/pinchartl/fbdev.git pinmux/3.9/dt. > Maybe at least a short explanation and an example with two DT nodes here, or > a reference to another documentation would help. What about > > diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c > > index 3b2fd43..5fa1c6b 100644 > > --- a/drivers/pinctrl/sh-pfc/core.c > > +++ b/drivers/pinctrl/sh-pfc/core.c > > @@ -18,6 +18,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -348,14 +349,74 @@ int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned > > mark, int pinmux_type)> > > return 0; > > > > } > > > > +#ifdef CONFIG_OF > > Not sure, is it really a good idea to use this #ifdef here? I think there's no point in making the driver larger if CONFIG_OF isn't defined, especially for SH platforms. > I think you could drop all 3 of them in this hunk: I've dropped the other two. > [snip] > > > static int sh_pfc_probe(struct platform_device *pdev) > > { > > > > + const struct platform_device_id *platid = platform_get_device_id(pdev); > > +#ifdef CONFIG_OF > > also here Dropped. > > + struct device_node *np = pdev->dev.of_node; > > +#endif > > > > const struct sh_pfc_soc_info *info; > > struct sh_pfc *pfc; > > int ret; > > > > - info = pdev->id_entry->driver_data > > - ? (void *)pdev->id_entry->driver_data : pdev- >dev.platform_data; > > + if (platid) > > + info = (const void *)platid->driver_data; > > +#ifdef CONFIG_OF > > and here. Dropped. > > + else if (np) > > + info = of_match_device(sh_pfc_of_table, &pdev->dev)->data; > > +#endif > > + else > > + info = pdev->dev.platform_data; I will also add a new patch that removes support for pdev->dev.platform_data, as we don't use it anymore. > > + > > > > if (info == NULL) > > > > return -ENODEV; > > [snip] > > > +static int sh_pfc_dt_node_to_map(struct pinctrl_dev *pctldev, > > + struct device_node *np, > > + struct pinctrl_map **map, unsigned *num_maps) > > +{ > > + struct sh_pfc_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > > + struct device *dev = pmx->pfc->dev; > > + struct device_node *child; > > + unsigned int index; > > + int ret; > > + > > + *map = NULL; > > + *num_maps = 0; > > + index = 0; > > + > > + for_each_child_of_node(np, child) { > > + ret = sh_pfc_dt_subnode_to_map(dev, child, map, num_maps, > > + &index); > > + if (ret < 0) > > + goto done; > > + } > > + > > + /* If no mapping has been found in child nodes try the config node. */ > > + if (*num_maps == 0) { > > + ret = sh_pfc_dt_subnode_to_map(dev, np, map, num_maps, &index); > > + if (ret < 0) > > + goto done; > > + } > > + > > + if (*num_maps == 0) { > > + dev_err(dev, "no mapping found in node %s\n", np->full_name); > > + ret = -EINVAL; > > + goto done; > > + } > > + > > + ret = 0; > > + > > +done: > > + if (ret < 0) > > + sh_pfc_dt_free_map(pctldev, *map, *num_maps); > > + > > + return ret; > > +} > > Maybe simpler > > + if (*num_maps) > + return 0; > + > + dev_err(dev, "no mapping found in node %s\n", np->full_name); > + ret = -EINVAL; > + > +done: > + sh_pfc_dt_free_map(pctldev, *map, *num_maps); > + > + return ret; OK I'll change that. diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt index e7aae39..ce2584c 100644 --- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt @@ -68,6 +68,9 @@ Pin Configuration Node Properties: GPIO ---- +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. @@ -81,7 +84,11 @@ with values derived from the SoC user manual. <[phandle of the gpio controller node] [pin number within the gpio controller] [flags and pull up/down]> + +On other mach-shmobile platforms GPIO is handled by the gpio-rcar driver. +Please refer to Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt +for documentation of the GPIO device tree bindings on those platforms. > [snip] >