[RFC] pinctrl: generic: Add DT bindings

[RFC] pinctrl: generic: Add DT bindings

[Laurent Pinchart]

Document DT properties for the generic pinctrl parameters and add a
parser function.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../bindings/pinctrl/pinctrl-bindings.txt          | 29 +++++++
 drivers/pinctrl/pinconf-generic.c                  | 94 ++++++++++++++++++++++
 drivers/pinctrl/pinconf.h                          | 17 ++++
 3 files changed, 140 insertions(+)

I've successfully tested this patch (or more accurately only the pull-up and
pull-down properties) with the Renesas sh-pfc pinctrl device driver. I will
resent the sh-pfc DT bindings patch series rebased on the generic pinconf
bindings.

Not all generic pinconf properties are currently implemented, but I don't
think that should be a showstopper. We can add them later as needed.

The code is based on both the sh-pfc pinconf DT parser and James Hogan's
tz1090 DT parser ("[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl driver").

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index c95ea82..e499ff0 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -126,3 +126,32 @@ device; they may be grandchildren, for example. Whether this is legal, and
 whether there is any interaction between the child and intermediate parent
 nodes, is again defined entirely by the binding for the individual pin
 controller device.
+
+== Generic pinconf parameters ==
+
+Pin configuration parameters are expressed by DT properties in the pin
+controller device state nodes and child nodes. For devices that use the generic
+pinconf parameters the following properties are defined.
+
+- tristate: A boolean, put the pin into high impedance state when set.
+
+- pull-up: An integer representing the pull-up strength. 0 disables the pull-up,
+  non-zero values enable it.
+
+- pull-down: An integer representing the pull-down strength. 0 disables the
+  pull-down, non-zero values enables it.
+
+- schmitt: An integer, enable or disable Schmitt trigger mode for the pins.
+  Valid values are
+    0: Schmitt trigger disabled (no hysteresis)
+    1: Schmitt trigger enabled
+
+- slew-rate: An integer controlling the pin slew rate. Values are device-
+  dependent.
+
+- drive-strength: An integer representing the drive strength of pins in mA.
+  Valid values are device-dependent.
+
+The pinctrl device DT bindings documentation must list the properties that
+apply to the device, and define the valid range for all device-dependent
+values.
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 2ad5a8d..bd0e41d 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/device.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
@@ -135,3 +136,96 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev,
 }
 EXPORT_SYMBOL_GPL(pinconf_generic_dump_config);
 #endif
+
+struct pinconf_generic_param {
+	const char *property;
+	enum pin_config_param param;
+	bool flag;
+};
+
+static const struct pinconf_generic_param pinconf_generic_params[] = {
+	{ "tristate",		PIN_CONFIG_BIAS_HIGH_IMPEDANCE,		true },
+	{ "pull-up",		PIN_CONFIG_BIAS_PULL_UP,		false },
+	{ "pull-down",		PIN_CONFIG_BIAS_PULL_DOWN,		false },
+	{ "schmitt",		PIN_CONFIG_INPUT_SCHMITT_ENABLE,	true },
+	{ "slew-rate",		PIN_CONFIG_SLEW_RATE,			false },
+	{ "drive-strength",	PIN_CONFIG_DRIVE_STRENGTH,		false },
+};
+
+static int pinconf_generic_add_config(unsigned long **configs,
+				      unsigned int *num_configs,
+				      unsigned long config)
+{
+	unsigned int count = *num_configs + 1;
+	unsigned long *cfgs;
+
+	cfgs = krealloc(*configs, sizeof(*cfgs) * count, GFP_KERNEL);
+	if (cfgs == NULL)
+		return -ENOMEM;
+
+	cfgs[count - 1] = config;
+
+	*configs = cfgs;
+	*num_configs = count;
+
+	return 0;
+}
+
+/**
+ * pinconf_generic_parse_params - Parse generic pinconf parameters from DT
+ * @dev: the device, used to print error messages
+ * @np: the DT node that contains generic pinconf parameters
+ * @cfgs: the returned array of pinconf parameters
+ *
+ * The parameters array is allocated dynamically and returned through the cfgs
+ * argument. The caller is responsible for freeing the array with kfree(). If no
+ * configuration parameter is found, or if an error occurs, no parameters array
+ * will be allocated and the cfgs argument will be set to NULL.
+ *
+ * Return the number of configuration parameters successfully parsed, or a
+ * negative value if an error occurred. Used error codes are
+ *
+ * -ENOMEM if memory can't be allocated for the parameters array
+ */
+int pinconf_generic_parse_params(struct device *dev, struct device_node *np,
+				 unsigned long **cfgs)
+{
+	unsigned long *configs = NULL;
+	unsigned int num_configs = 0;
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(pinconf_generic_params); ++i) {
+		const struct pinconf_generic_param *param =
+			&pinconf_generic_params[i];
+		unsigned long config;
+		u32 val;
+
+		if (param->flag) {
+			ret = of_property_read_bool(np, param->property)
+			    ? 0 : -EINVAL;
+			val = 1;
+		} else {
+			ret = of_property_read_u32(np, param->property, &val);
+		}
+
+		if (ret) {
+			if (ret != -EINVAL)
+				dev_err(dev, "failed to parse property %s\n",
+					param->property);
+			continue;
+		}
+
+		config = pinconf_to_config_packed(param->param, val);
+		ret = pinconf_generic_add_config(&configs, &num_configs, config);
+		if (ret < 0) {
+			kfree(configs);
+			*cfgs = NULL;
+			return ret;
+		}
+	}
+
+	*cfgs = configs;
+	return num_configs;
+}
+EXPORT_SYMBOL_GPL(pinconf_generic_parse_params);
diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
index 92c7267..eb8550b 100644
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -90,6 +90,23 @@ static inline void pinconf_init_device_debugfs(struct dentry *devroot,
  * pin config.
  */
 
+#if defined(CONFIG_GENERIC_PINCONF)
+
+int pinconf_generic_parse_params(struct device *dev, struct device_node *np,
+				 unsigned long **cfgs);
+
+#else
+
+static inline int pinconf_generic_parse_params(struct device *dev,
+					       struct device_node *np,
+					       unsigned long **cfgs)
+{
+	*cfgs = NULL;
+	return 0;
+}
+
+#endif
+
 #if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_DEBUG_FS)
 
 void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
-- 
Regards,

Laurent Pinchart

Re: [RFC] pinctrl: generic: Add DT bindings

[Grant Likely]

On Wed, 12 Jun 2013 00:03:57 +0200, Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> Document DT properties for the generic pinctrl parameters and add a
> parser function.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../bindings/pinctrl/pinctrl-bindings.txt          | 29 +++++++
>  drivers/pinctrl/pinconf-generic.c                  | 94 ++++++++++++++++++++++
>  drivers/pinctrl/pinconf.h                          | 17 ++++
>  3 files changed, 140 insertions(+)
> 
> I've successfully tested this patch (or more accurately only the pull-up and
> pull-down properties) with the Renesas sh-pfc pinctrl device driver. I will
> resent the sh-pfc DT bindings patch series rebased on the generic pinconf
> bindings.
> 
> Not all generic pinconf properties are currently implemented, but I don't
> think that should be a showstopper. We can add them later as needed.
> 
> The code is based on both the sh-pfc pinconf DT parser and James Hogan's
> tz1090 DT parser ("[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl driver").
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> index c95ea82..e499ff0 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> @@ -126,3 +126,32 @@ device; they may be grandchildren, for example. Whether this is legal, and
>  whether there is any interaction between the child and intermediate parent
>  nodes, is again defined entirely by the binding for the individual pin
>  controller device.
> +
> +== Generic pinconf parameters ==
> +
> +Pin configuration parameters are expressed by DT properties in the pin
> +controller device state nodes and child nodes. For devices that use the generic
> +pinconf parameters the following properties are defined.
> +
> +- tristate: A boolean, put the pin into high impedance state when set.
> +
> +- pull-up: An integer representing the pull-up strength. 0 disables the pull-up,
> +  non-zero values enable it.
> +
> +- pull-down: An integer representing the pull-down strength. 0 disables the
> +  pull-down, non-zero values enables it.
> +
> +- schmitt: An integer, enable or disable Schmitt trigger mode for the pins.
> +  Valid values are
> +    0: Schmitt trigger disabled (no hysteresis)
> +    1: Schmitt trigger enabled
> +
> +- slew-rate: An integer controlling the pin slew rate. Values are device-
> +  dependent.
> +
> +- drive-strength: An integer representing the drive strength of pins in mA.
> +  Valid values are device-dependent.
> +
> +The pinctrl device DT bindings documentation must list the properties that
> +apply to the device, and define the valid range for all device-dependent
> +values.

I don't see any problem with the above properties, but I would like to
see an example. How verbose will a pinctrl node using the generic
properties tend to be?

> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
> index 2ad5a8d..bd0e41d 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/device.h>
> +#include <linux/of.h>
>  #include <linux/slab.h>
>  #include <linux/debugfs.h>
>  #include <linux/seq_file.h>
> @@ -135,3 +136,96 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev,
>  }
>  EXPORT_SYMBOL_GPL(pinconf_generic_dump_config);
>  #endif
> +
> +struct pinconf_generic_param {
> +	const char *property;
> +	enum pin_config_param param;
> +	bool flag;
> +};
> +
> +static const struct pinconf_generic_param pinconf_generic_params[] = {
> +	{ "tristate",		PIN_CONFIG_BIAS_HIGH_IMPEDANCE,		true },
> +	{ "pull-up",		PIN_CONFIG_BIAS_PULL_UP,		false },
> +	{ "pull-down",		PIN_CONFIG_BIAS_PULL_DOWN,		false },
> +	{ "schmitt",		PIN_CONFIG_INPUT_SCHMITT_ENABLE,	true },
> +	{ "slew-rate",		PIN_CONFIG_SLEW_RATE,			false },
> +	{ "drive-strength",	PIN_CONFIG_DRIVE_STRENGTH,		false },
> +};
> +
> +static int pinconf_generic_add_config(unsigned long **configs,
> +				      unsigned int *num_configs,
> +				      unsigned long config)
> +{
> +	unsigned int count = *num_configs + 1;
> +	unsigned long *cfgs;
> +
> +	cfgs = krealloc(*configs, sizeof(*cfgs) * count, GFP_KERNEL);
> +	if (cfgs == NULL)
> +		return -ENOMEM;
> +
> +	cfgs[count - 1] = config;
> +
> +	*configs = cfgs;
> +	*num_configs = count;
> +
> +	return 0;
> +}

Hmmm. We really need a better method of parsing multiple properties.
I've been toying around with a few ideas, but haven't been able to draft
something I'm happy with yeat.

Regardless, the code in this patch looks fine to me.

g.

Re: [RFC] pinctrl: generic: Add DT bindings

[James Hogan]

On 11/06/13 23:03, Laurent Pinchart wrote:
> Document DT properties for the generic pinctrl parameters and add a
> parser function.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../bindings/pinctrl/pinctrl-bindings.txt          | 29 +++++++
>  drivers/pinctrl/pinconf-generic.c                  | 94 ++++++++++++++++++++++
>  drivers/pinctrl/pinconf.h                          | 17 ++++
>  3 files changed, 140 insertions(+)
> 
> I've successfully tested this patch (or more accurately only the pull-up and
> pull-down properties) with the Renesas sh-pfc pinctrl device driver. I will
> resent the sh-pfc DT bindings patch series rebased on the generic pinconf
> bindings.
> 
> Not all generic pinconf properties are currently implemented, but I don't
> think that should be a showstopper. We can add them later as needed.
> 
> The code is based on both the sh-pfc pinconf DT parser and James Hogan's
> tz1090 DT parser ("[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl driver").

Thanks for this patch. I haven't tested it (yet), but have a few
comments below.

> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> index c95ea82..e499ff0 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> @@ -126,3 +126,32 @@ device; they may be grandchildren, for example. Whether this is legal, and
>  whether there is any interaction between the child and intermediate parent
>  nodes, is again defined entirely by the binding for the individual pin
>  controller device.
> +
> +== Generic pinconf parameters ==
> +
> +Pin configuration parameters are expressed by DT properties in the pin
> +controller device state nodes and child nodes. For devices that use the generic
> +pinconf parameters the following properties are defined.
> +
> +- tristate: A boolean, put the pin into high impedance state when set.
> +
> +- pull-up: An integer representing the pull-up strength. 0 disables the pull-up,
> +  non-zero values enable it.
> +
> +- pull-down: An integer representing the pull-down strength. 0 disables the
> +  pull-down, non-zero values enables it.
> +
> +- schmitt: An integer, enable or disable Schmitt trigger mode for the pins.
> +  Valid values are
> +    0: Schmitt trigger disabled (no hysteresis)
> +    1: Schmitt trigger enabled

this is set as a flag, so I think it should be described like tristate,
"A boolean, ... when set."? Same for pull-up and pull-down (see comment
below).

<snip>

> +	{ "pull-up",		PIN_CONFIG_BIAS_PULL_UP,		false },
> +	{ "pull-down",		PIN_CONFIG_BIAS_PULL_DOWN,		false },

pinconf-generic.h says "If the argument is != 0 pull-up is enabled, if
it is 0, pull-up is disabled", so I think these should be flags unless
it's changed there first.

Any chance of adding the new "bus-hold" entry too
(PIN_CONFIG_BIAS_BUS_HOLD, and flag=true I suppose)? see
aa69352252a7a952e6e77734cb87135143a377d2 in LinuxW's pinctrl for-next
branch.

<snip>

> +EXPORT_SYMBOL_GPL(pinconf_generic_parse_params);
> diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
> index 92c7267..eb8550b 100644
> --- a/drivers/pinctrl/pinconf.h
> +++ b/drivers/pinctrl/pinconf.h
> @@ -90,6 +90,23 @@ static inline void pinconf_init_device_debugfs(struct dentry *devroot,
>   * pin config.
>   */
>  
> +#if defined(CONFIG_GENERIC_PINCONF)
> +
> +int pinconf_generic_parse_params(struct device *dev, struct device_node *np,
> +				 unsigned long **cfgs);
> +
> +#else
> +
> +static inline int pinconf_generic_parse_params(struct device *dev,
> +					       struct device_node *np,
> +					       unsigned long **cfgs)
> +{
> +	*cfgs = NULL;
> +	return 0;
> +}

Should this ever be necessary? Sounds like if the driver wanted to use
this it should already have selected GENERIC_PINCONF anyway.

Cheers
James

Re: [RFC] pinctrl: generic: Add DT bindings

[Laurent Pinchart]

Hi Grant,

Thanks for the review.

On Wednesday 12 June 2013 13:48:33 Grant Likely wrote:
> On Wed, 12 Jun 2013 00:03:57 +0200, Laurent Pinchart wrote:
> > Document DT properties for the generic pinctrl parameters and add a
> > parser function.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../bindings/pinctrl/pinctrl-bindings.txt          | 29 +++++++
> >  drivers/pinctrl/pinconf-generic.c                  | 94 +++++++++++++++++
> >  drivers/pinctrl/pinconf.h                          | 17 ++++
> >  3 files changed, 140 insertions(+)
> > 
> > I've successfully tested this patch (or more accurately only the pull-up
> > and pull-down properties) with the Renesas sh-pfc pinctrl device driver.
> > I will resent the sh-pfc DT bindings patch series rebased on the generic
> > pinconf bindings.
> > 
> > Not all generic pinconf properties are currently implemented, but I don't
> > think that should be a showstopper. We can add them later as needed.
> > 
> > The code is based on both the sh-pfc pinconf DT parser and James Hogan's
> > tz1090 DT parser ("[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl
> > driver").
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index
> > c95ea82..e499ff0 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > @@ -126,3 +126,32 @@ device; they may be grandchildren, for example.
> > Whether this is legal, and> 
> >  whether there is any interaction between the child and intermediate
> >  parent nodes, is again defined entirely by the binding for the individual
> >  pin controller device.
> > 
> > +
> > +== Generic pinconf parameters ==
> > +
> > +Pin configuration parameters are expressed by DT properties in the pin
> > +controller device state nodes and child nodes. For devices that use the
> > generic +pinconf parameters the following properties are defined.
> > +
> > +- tristate: A boolean, put the pin into high impedance state when set.
> > +
> > +- pull-up: An integer representing the pull-up strength. 0 disables the
> > pull-up, +  non-zero values enable it.
> > +
> > +- pull-down: An integer representing the pull-down strength. 0 disables
> > the +  pull-down, non-zero values enables it.
> > +
> > +- schmitt: An integer, enable or disable Schmitt trigger mode for the
> > pins. +  Valid values are
> > +    0: Schmitt trigger disabled (no hysteresis)
> > +    1: Schmitt trigger enabled
> > +
> > +- slew-rate: An integer controlling the pin slew rate. Values are device-
> > +  dependent.
> > +
> > +- drive-strength: An integer representing the drive strength of pins in
> > mA. +  Valid values are device-dependent.
> > +
> > +The pinctrl device DT bindings documentation must list the properties
> > that
> > +apply to the device, and define the valid range for all device-dependent
> > +values.
> 
> I don't see any problem with the above properties, but I would like to
> see an example. How verbose will a pinctrl node using the generic
> properties tend to be?

Here's a real-life example

&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";
                        bias-pull-up = <1>;
                };
        };

        scifa4_pins: scifa4 {
                renesas,groups = "scifa4_data", "scifa4_ctrl";
                renesas,function = "scifa4";
        };
};

The mux node selects function mmc0 on two pin groups, and the cfg node 
activates pull-ups on one pin group and one particular pin.

> > diff --git a/drivers/pinctrl/pinconf-generic.c
> > b/drivers/pinctrl/pinconf-generic.c index 2ad5a8d..bd0e41d 100644
> > --- a/drivers/pinctrl/pinconf-generic.c
> > +++ b/drivers/pinctrl/pinconf-generic.c
> > @@ -15,6 +15,7 @@
> > 
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> >  #include <linux/device.h>
> > 
> > +#include <linux/of.h>
> > 
> >  #include <linux/slab.h>
> >  #include <linux/debugfs.h>
> >  #include <linux/seq_file.h>
> > 
> > @@ -135,3 +136,96 @@ void pinconf_generic_dump_config(struct pinctrl_dev
> > *pctldev,> 
> >  }
> >  EXPORT_SYMBOL_GPL(pinconf_generic_dump_config);
> >  #endif
> > 
> > +
> > +struct pinconf_generic_param {
> > +	const char *property;
> > +	enum pin_config_param param;
> > +	bool flag;
> > +};
> > +
> > +static const struct pinconf_generic_param pinconf_generic_params[] = {
> > +	{ "tristate",		PIN_CONFIG_BIAS_HIGH_IMPEDANCE,		true },
> > +	{ "pull-up",		PIN_CONFIG_BIAS_PULL_UP,		false },
> > +	{ "pull-down",		PIN_CONFIG_BIAS_PULL_DOWN,		false },
> > +	{ "schmitt",		PIN_CONFIG_INPUT_SCHMITT_ENABLE,	true },
> > +	{ "slew-rate",		PIN_CONFIG_SLEW_RATE,			false },
> > +	{ "drive-strength",	PIN_CONFIG_DRIVE_STRENGTH,		false },
> > +};
> > +
> > +static int pinconf_generic_add_config(unsigned long **configs,
> > +				      unsigned int *num_configs,
> > +				      unsigned long config)
> > +{
> > +	unsigned int count = *num_configs + 1;
> > +	unsigned long *cfgs;
> > +
> > +	cfgs = krealloc(*configs, sizeof(*cfgs) * count, GFP_KERNEL);
> > +	if (cfgs == NULL)
> > +		return -ENOMEM;
> > +
> > +	cfgs[count - 1] = config;
> > +
> > +	*configs = cfgs;
> > +	*num_configs = count;
> > +
> > +	return 0;
> > +}
> 
> Hmmm. We really need a better method of parsing multiple properties.
> I've been toying around with a few ideas, but haven't been able to draft
> something I'm happy with yeat.
> 
> Regardless, the code in this patch looks fine to me.

Thanks. As other generic pinconf DT bindings proposals have been submitted I 
will resent this patch set with pinconf support stripped out to make sure it 
gets to v3.11 and will then add pinconf back in follow-up patches for v3.11 or 
v3.12, depending on when we can agree on generic pinconf bindings.

-- 
Regards,

Laurent Pinchart

Re: [RFC] pinctrl: generic: Add DT bindings

[Laurent Pinchart]

Hi James,

On Wednesday 12 June 2013 15:36:59 James Hogan wrote:
> On 11/06/13 23:03, Laurent Pinchart wrote:
> > Document DT properties for the generic pinctrl parameters and add a
> > parser function.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../bindings/pinctrl/pinctrl-bindings.txt          | 29 +++++++
> >  drivers/pinctrl/pinconf-generic.c                  | 94 +++++++++++++++++
> >  drivers/pinctrl/pinconf.h                          | 17 ++++
> >  3 files changed, 140 insertions(+)
> > 
> > I've successfully tested this patch (or more accurately only the pull-up
> > and pull-down properties) with the Renesas sh-pfc pinctrl device driver.
> > I will resent the sh-pfc DT bindings patch series rebased on the generic
> > pinconf bindings.
> > 
> > Not all generic pinconf properties are currently implemented, but I don't
> > think that should be a showstopper. We can add them later as needed.
> > 
> > The code is based on both the sh-pfc pinconf DT parser and James Hogan's
> > tz1090 DT parser ("[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl
> > driver").
>
> Thanks for this patch. I haven't tested it (yet), but have a few
> comments below.
> 
> > diff --git
> > a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index
> > c95ea82..e499ff0 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > @@ -126,3 +126,32 @@ device; they may be grandchildren, for example.
> > Whether this is legal, and> 
> >  whether there is any interaction between the child and intermediate
> >  parent nodes, is again defined entirely by the binding for the individual
> >  pin controller device.
> > 
> > +
> > +== Generic pinconf parameters ==
> > +
> > +Pin configuration parameters are expressed by DT properties in the pin
> > +controller device state nodes and child nodes. For devices that use the
> > generic +pinconf parameters the following properties are defined.
> > +
> > +- tristate: A boolean, put the pin into high impedance state when set.
> > +
> > +- pull-up: An integer representing the pull-up strength. 0 disables the
> > pull-up, +  non-zero values enable it.
> > +
> > +- pull-down: An integer representing the pull-down strength. 0 disables
> > the +  pull-down, non-zero values enables it.
> > +
> > +- schmitt: An integer, enable or disable Schmitt trigger mode for the
> > pins. +  Valid values are
> > +    0: Schmitt trigger disabled (no hysteresis)
> > +    1: Schmitt trigger enabled
> 
> this is set as a flag, so I think it should be described like tristate,
> "A boolean, ... when set."? Same for pull-up and pull-down (see comment
> below).

Can't the value be used to control schmitt trigger parameters (same as for the 
pull-up and pull-down values, as explained below) ?

> <snip>
> 
> > +	{ "pull-up",		PIN_CONFIG_BIAS_PULL_UP,		false },
> > +	{ "pull-down",		PIN_CONFIG_BIAS_PULL_DOWN,		false },
> 
> pinconf-generic.h says "If the argument is != 0 pull-up is enabled, if
> it is 0, pull-up is disabled", so I think these should be flags unless
> it's changed there first.

== 0 for disabled and != 0 for enabled doesn't mean that all != 0 values are 
equivalent. As I read it drivers can use the value to control the pull-up/down 
strength without violating the documentation.

> Any chance of adding the new "bus-hold" entry too
> (PIN_CONFIG_BIAS_BUS_HOLD, and flag=true I suppose)? see
> aa69352252a7a952e6e77734cb87135143a377d2 in LinuxW's pinctrl for-next
> branch.

Another generic pinconf DT bindings proposal has been submitted and applied to 
the devel branch in Linus' pinctrl tree. It includes support for 
PIN_CONFIG_BIAS_BUS_HOLD. Linus asked me to review the patch, so this one will 
likely be dropped or at least integrated into the other one.

> <snip>
> 
> > +EXPORT_SYMBOL_GPL(pinconf_generic_parse_params);
> > diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
> > index 92c7267..eb8550b 100644
> > --- a/drivers/pinctrl/pinconf.h
> > +++ b/drivers/pinctrl/pinconf.h
> > @@ -90,6 +90,23 @@ static inline void pinconf_init_device_debugfs(struct
> > dentry *devroot,> 
> >   * pin config.
> >   */
> > 
> > +#if defined(CONFIG_GENERIC_PINCONF)
> > +
> > +int pinconf_generic_parse_params(struct device *dev, struct device_node
> > *np, +				 unsigned long **cfgs);
> > +
> > +#else
> > +
> > +static inline int pinconf_generic_parse_params(struct device *dev,
> > +					       struct device_node *np,
> > +					       unsigned long **cfgs)
> > +{
> > +	*cfgs = NULL;
> > +	return 0;
> > +}
> 
> Should this ever be necessary? Sounds like if the driver wanted to use
> this it should already have selected GENERIC_PINCONF anyway.

You're right.

-- 
Regards,

Laurent Pinchart

Re: [RFC] pinctrl: generic: Add DT bindings

[Linus Walleij]

On Fri, Jun 14, 2013 at 12:39 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

>         mmcif_pins: mmcif {
>                 mux {
>                         renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0";
>                         renesas,function = "mmc0";
>                 };
>                 cfg {
>                         renesas,groups = "mmc0_data8_0";
>                         renesas,pins = "PORT279";
>                         bias-pull-up = <1>;

If I understood your code correctly that last statement can *optionally*
be written like just:

 bias-pull-up;

Without the parameter?

I think that Heiko's implementation does this anyway.

Yours,
Linus Walleij

Re: [RFC] pinctrl: generic: Add DT bindings

[Heiko Stübner]

Am Samstag, 15. Juni 2013, 21:56:05 schrieb Linus Walleij:
> On Fri, Jun 14, 2013 at 12:39 AM, Laurent Pinchart
> 
> <laurent.pinchart@ideasonboard.com> wrote:
> >         mmcif_pins: mmcif {
> >         
> >                 mux {
> >                 
> >                         renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0";
> >                         renesas,function = "mmc0";
> >                 
> >                 };
> >                 cfg {
> >                 
> >                         renesas,groups = "mmc0_data8_0";
> >                         renesas,pins = "PORT279";
> >                         bias-pull-up = <1>;
> 
> If I understood your code correctly that last statement can *optionally*
> be written like just:
> 
>  bias-pull-up;
> 
> Without the parameter?
> 
> I think that Heiko's implementation does this anyway.

Yep, with the fixes-series from yesterday the bias-pull-* now have a better 
default value of <1>.

so you can do
	bias-pull-up;
which is then identical to the
	bias-pull-up = <1>;
above (both are valid of course).


Disable would the be either
	bias-disable;
or
	bias-pull-up = <0>;

A driver should probably handle both, as both are valid pinconf options or 
this.

Re: [RFC] pinctrl: generic: Add DT bindings

[Laurent Pinchart]

Hi Heiko,

On Saturday 15 June 2013 22:16:13 Heiko Stübner wrote:
> Am Samstag, 15. Juni 2013, 21:56:05 schrieb Linus Walleij:
> > On Fri, Jun 14, 2013 at 12:39 AM, Laurent Pinchart wrote:
> > >         mmcif_pins: mmcif {
> > >                 mux {
> > >                         renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0";
> > >                         renesas,function = "mmc0";
> > >                 };
> > >                 cfg {
> > >                         renesas,groups = "mmc0_data8_0";
> > >                         renesas,pins = "PORT279";
> > >                         bias-pull-up = <1>;
> > 
> > If I understood your code correctly that last statement can *optionally*
> > 
> > be written like just:
> >  bias-pull-up;
> > 
> > Without the parameter?
> > 
> > I think that Heiko's implementation does this anyway.
> 
> Yep, with the fixes-series from yesterday the bias-pull-* now have a better
> default value of <1>.
> 
> so you can do
> 	bias-pull-up;
> which is then identical to the
> 	bias-pull-up = <1>;
> above (both are valid of course).

My patch used bias-pull-up = <1>; as the current version of Heiko's DT parser 
didn't support bias-pull-up; correctly at that time. It's fixed now, and my 
prefered is then bias-pull-up;.

> Disable would the be either
> 	bias-disable;
> or
> 	bias-pull-up = <0>;
> 
> A driver should probably handle both, as both are valid pinconf options or
> this.

I feel a bit uneasy about that. Do we really need to support two different 
ways to achieve the same result ?

-- 
Regards,

Laurent Pinchart

Re: [RFC] pinctrl: generic: Add DT bindings

[Linus Walleij]

On Sun, Jun 16, 2013 at 1:35 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Saturday 15 June 2013 22:16:13 Heiko Stübner wrote:
>> Am Samstag, 15. Juni 2013, 21:56:05 schrieb Linus Walleij:

>> Disable would the be either
>>       bias-disable;
>> or
>>       bias-pull-up = <0>;
>>
>> A driver should probably handle both, as both are valid pinconf options or
>> this.
>
> I feel a bit uneasy about that. Do we really need to support two different
> ways to achieve the same result ?

In this specific case I think yes, but not on all options.

As dicussed earlier this was designed for systems where
you could set the pull-up resistance, like

bias-pull-up = <600000>;

would give 600kOhm pull up.

In most existing systems that is silly, as they can't specify
it, so they should be able to do just:

bias-pull-up;

as that is all they can do. If we have to cut one way, we
should cut the former until such a system appears.

Yours,
Linus Walleij

Re: [RFC] pinctrl: generic: Add DT bindings

[Laurent Pinchart]

Hi Linus,

On Sunday 16 June 2013 01:51:32 Linus Walleij wrote:
> On Sun, Jun 16, 2013 at 1:35 AM, Laurent Pinchart wrote:
> > On Saturday 15 June 2013 22:16:13 Heiko Stübner wrote:
> >> Am Samstag, 15. Juni 2013, 21:56:05 schrieb Linus Walleij:
> >> 
> >> Disable would the be either
> >> 
> >>       bias-disable;
> >> 
> >> or
> >> 
> >>       bias-pull-up = <0>;
> >> 
> >> A driver should probably handle both, as both are valid pinconf options
> >> or this.
> > 
> > I feel a bit uneasy about that. Do we really need to support two different
> > ways to achieve the same result ?
> 
> In this specific case I think yes, but not on all options.
> 
> As dicussed earlier this was designed for systems where
> you could set the pull-up resistance, like
> 
> bias-pull-up = <600000>;
> 
> would give 600kOhm pull up.
> 
> In most existing systems that is silly, as they can't specify it, so they
> should be able to do just:
> 
> bias-pull-up;
> 
> as that is all they can do. If we have to cut one way, we should cut the
> former until such a system appears.

I'm fine with bias-pull-up = <1>; vs bias-pull-up;. What bothers me a bit is 
bias-pull-up = <0>; vs bias-disable;.

-- 
Regards,

Laurent Pinchart

Re: [RFC] pinctrl: generic: Add DT bindings

[Linus Walleij]

On Sun, Jun 16, 2013 at 1:52 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Linus,
> On Sunday 16 June 2013 01:51:32 Linus Walleij wrote:

>> As dicussed earlier this was designed for systems where
>> you could set the pull-up resistance, like
>>
>> bias-pull-up = <600000>;
>>
>> would give 600kOhm pull up.
>>
>> In most existing systems that is silly, as they can't specify it, so they
>> should be able to do just:
>>
>> bias-pull-up;
>>
>> as that is all they can do. If we have to cut one way, we should cut the
>> former until such a system appears.
>
> I'm fine with bias-pull-up = <1>; vs bias-pull-up;. What bothers me a bit is
> bias-pull-up = <0>; vs bias-disable;.

Oh yeah OK you got a point there for sure.

Setting bias-pull-up = <0>; would be equal to short-circuit
so it does not make any kind of sense.

Let's keep an eye on this.

Yours,
Linus Walleij

Re: [RFC] pinctrl: generic: Add DT bindings

[Stephen Warren]

On 06/15/2013 02:16 PM, Heiko Stübner wrote:
> Am Samstag, 15. Juni 2013, 21:56:05 schrieb Linus Walleij:
>> On Fri, Jun 14, 2013 at 12:39 AM, Laurent Pinchart
>>
>> <laurent.pinchart@ideasonboard.com> wrote:
>>>         mmcif_pins: mmcif {
>>>         
>>>                 mux {
>>>                 
>>>                         renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0";
>>>                         renesas,function = "mmc0";
>>>                 
>>>                 };
>>>                 cfg {
>>>                 
>>>                         renesas,groups = "mmc0_data8_0";
>>>                         renesas,pins = "PORT279";
>>>                         bias-pull-up = <1>;
>>
>> If I understood your code correctly that last statement can *optionally*
>> be written like just:
>>
>>  bias-pull-up;
>>
>> Without the parameter?
>>
>> I think that Heiko's implementation does this anyway.
> 
> Yep, with the fixes-series from yesterday the bias-pull-* now have a better 
> default value of <1>.
> 
> so you can do
> 	bias-pull-up;
> which is then identical to the
> 	bias-pull-up = <1>;
> above (both are valid of course).

A property with a value is an integer. One without is a Boolean. The
same property shouldn't be both.

Regarding the value representing resistance: That feels pretty odd.
Enabling a pullup and selecting the pullup resistance feel like
orthogonal options which shouldn't be bound to each-other.

Re: [RFC] pinctrl: generic: Add DT bindings

[Stephen Warren]

On 06/11/2013 04:03 PM, Laurent Pinchart wrote:
> Document DT properties for the generic pinctrl parameters and add a
> parser function.

> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

> +== Generic pinconf parameters ==
> +
> +Pin configuration parameters are expressed by DT properties in the pin
> +controller device state nodes and child nodes. For devices that use the generic
> +pinconf parameters the following properties are defined.
> +
> +- tristate: A boolean, put the pin into high impedance state when set.

I don't think that a Boolean is appropriate here; it's really a tri-state:

* Nothing is specified about the tristate value; don't touch that aspect
of HW.
* Turn tristate on.
* Turn tristate off.

This can be a meaningful distinction when relying on e.g. the bootloader
having already set up the appropriate tristate value, or when
dynamically switching between different pinctrl states and not wanting
to affect tristate, or when piecing together multiple DT nodes that
describe part of a state, where one node covers just muxing and the
other just pin config (e.g. where the mux and pin config configuration
registers in HW affect different overlapping groups).

> +- schmitt: An integer, enable or disable Schmitt trigger mode for the pins.
> +  Valid values are
> +    0: Schmitt trigger disabled (no hysteresis)
> +    1: Schmitt trigger enabled

A similar comment applies here.

> +- slew-rate: An integer controlling the pin slew rate. Values are device-
> +  dependent.
> +
> +- drive-strength: An integer representing the drive strength of pins in mA.
> +  Valid values are device-dependent.

I'm still not convinced that requiring this to be in mA is a good idea.
Different HW will use different units for documentation, or even specify
no units at all, so it might not always be possible to represent this in
terms of mA. Asking for the documentation to be re-written simply to
support the DT binding just isn't going to happen.

Re: [RFC] pinctrl: generic: Add DT bindings

[Linus Walleij]

On Wed, Jun 19, 2013 at 11:58 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/11/2013 04:03 PM, Laurent Pinchart wrote:

>> +- tristate: A boolean, put the pin into high impedance state when set.

The other patch defines bias-high-impedance which is more likely
to be the string used.

Anyway:

> I don't think that a Boolean is appropriate here; it's really a tri-state:
>
> * Nothing is specified about the tristate value; don't touch that aspect
> of HW.
> * Turn tristate on.
> * Turn tristate off.

I was thinking more about using bias-disable; to explicitly turn that
off.

But maybe a specific disable option is needed, like for schmitt-trigger
(see below).

> This can be a meaningful distinction when relying on e.g. the bootloader
> having already set up the appropriate tristate value, or when
> dynamically switching between different pinctrl states and not wanting
> to affect tristate, or when piecing together multiple DT nodes that
> describe part of a state, where one node covers just muxing and the
> other just pin config (e.g. where the mux and pin config configuration
> registers in HW affect different overlapping groups).

OK point taken... although when we're dealing with generic pin config
the idea is not to cover everything but the majority of not-so-complicated
cases.

>> +- schmitt: An integer, enable or disable Schmitt trigger mode for the pins.
>> +  Valid values are
>> +    0: Schmitt trigger disabled (no hysteresis)
>> +    1: Schmitt trigger enabled
>
> A similar comment applies here.

The other patch adds:
input-schmitt-enable;
input-schmitt-disable;

So this is covered.

>> +- slew-rate: An integer controlling the pin slew rate. Values are device-
>> +  dependent.
>> +
>> +- drive-strength: An integer representing the drive strength of pins in mA.
>> +  Valid values are device-dependent.
>
> I'm still not convinced that requiring this to be in mA is a good idea.
> Different HW will use different units for documentation, or even specify
> no units at all, so it might not always be possible to represent this in
> terms of mA. Asking for the documentation to be re-written simply to
> support the DT binding just isn't going to happen.

We can add custom DT bindings for these. This is to cover those
where we know enough about it to actually use this generic binding.
If we don't really know what is happening we may as well call it
vendor,foo = <?>;

Yours,
Linus Walleij