Re: [PATCH v3 01/20] sh-pfc: Add DT support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../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/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]
> 
> > 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 <linux/ioport.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/of_device.h>
> >  #include <linux/pinctrl/machine.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> > @@ -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.

-- 
Regards,

Laurent Pinchart