[PATCH 0/7] Pinctrl support for Zynq

[PATCH 0/7] Pinctrl support for Zynq

[Soren Brinkmann]

Hi,

I think we're getting closer. I re-organized the series to illustrate
the dependencies better instead of my development process. The
respective patches have some more details and also changelog.

The first three patches essentially extend pinconf-generic by:
 - properly support and distinguish between the 'groups' and 'pins' DT
   properties
 - option for drivers to use the pinconf-generic infrastructure with the
   generic + driver-specific DT parameters

Doing so, resulted in one change in the API. I fixed up all the
call-sites I found, I hope.

The last four patches are the platform specific stuff. The actual
driver, DT update, documentation, etc.
The DT binding have been revised another time to conform better with how
the generic parameters are defined.

Since the Zynq-specific parts rely on the pinconf-generic changes, I
keep the audience to pinctrl stakeholders. Once there is an agreement on
those patches I will post the Zynq patches on the relevant lists.

	Thanks,
	Soren

Soren Brinkmann (7):
  pinctrl: pinconf-generic: Declare dt_params/conf_items const
  pinctrl: pinconf-generic: Infer map type from DT property
  pinctrl: pinconf-generic: Allow driver to specify DT params
  pinctrl: zynq: Document DT binding
  pinctrl: Add driver for Zynq
  ARM: zynq: Enable pinctrl
  ARM: zynq: DT: Add pinctrl information

 .../bindings/pinctrl/xlnx,zynq-pinctrl.txt         |   90 ++
 arch/arm/boot/dts/zynq-7000.dtsi                   |    8 +-
 arch/arm/boot/dts/zynq-zc702.dts                   |  143 +++
 arch/arm/boot/dts/zynq-zc706.dts                   |  120 ++
 arch/arm/mach-zynq/Kconfig                         |    2 +
 drivers/pinctrl/Kconfig                            |    8 +
 drivers/pinctrl/Makefile                           |    1 +
 drivers/pinctrl/nomadik/pinctrl-abx500.c           |    6 +-
 drivers/pinctrl/pinconf-generic.c                  |  192 ++--
 drivers/pinctrl/pinconf.h                          |    1 +
 drivers/pinctrl/pinctrl-rockchip.c                 |    2 +-
 drivers/pinctrl/pinctrl-tz1090-pdc.c               |    2 +-
 drivers/pinctrl/pinctrl-tz1090.c                   |    2 +-
 drivers/pinctrl/pinctrl-zynq.c                     | 1164 ++++++++++++++++++++
 drivers/pinctrl/sh-pfc/pinctrl.c                   |    2 +-
 include/linux/pinctrl/pinconf-generic.h            |   25 +
 include/linux/pinctrl/pinctrl.h                    |    8 +
 17 files changed, 1683 insertions(+), 93 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
 create mode 100644 drivers/pinctrl/pinctrl-zynq.c

-- 
1.9.1

[PATCH 1/7] pinctrl: pinconf-generic: Declare dt_params/conf_items const

[Soren Brinkmann]

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/pinctrl/pinconf-generic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index e28ef957ca2d..f78b416d7984 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -38,7 +38,7 @@ struct pin_config_item {
 #define PCONFDUMP(a, b, c, d) { .param = a, .display = b, .format = c, \
 				.has_arg = d }
 
-static struct pin_config_item conf_items[] = {
+static const struct pin_config_item conf_items[] = {
 	PCONFDUMP(PIN_CONFIG_BIAS_DISABLE, "input bias disabled", NULL, false),
 	PCONFDUMP(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, "input bias high impedance", NULL, false),
 	PCONFDUMP(PIN_CONFIG_BIAS_BUS_HOLD, "input bias bus hold", NULL, false),
@@ -159,7 +159,7 @@ struct pinconf_generic_dt_params {
 	u32 default_value;
 };
 
-static struct pinconf_generic_dt_params dt_params[] = {
+static const struct pinconf_generic_dt_params dt_params[] = {
 	{ "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
 	{ "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
 	{ "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 },
@@ -209,7 +209,7 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
-		struct pinconf_generic_dt_params *par = &dt_params[i];
+		const struct pinconf_generic_dt_params *par = &dt_params[i];
 		ret = of_property_read_u32(np, par->property, &val);
 
 		/* property not found */
-- 
1.9.1

Re: [PATCH 1/7] pinctrl: pinconf-generic: Declare dt_params/conf_items const

[Linus Walleij]

On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
<soren.brinkmann@xilinx.com> wrote:

> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

Patch applied.

Yours,
Linus Walleij

[PATCH 2/7] pinctrl: pinconf-generic: Infer map type from DT property

[Soren Brinkmann]

With the new 'groups' property, the DT parser can infer the map type
from the fact whether 'pins' or 'groups' is used to specify the pin
group to work on.
To maintain backwards compatibitliy with current usage of the DT
binding, this is only done when an invalid map type is passed to the
parsing function.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
Changes since RFC v2:
 - none
---
 drivers/pinctrl/pinconf-generic.c       | 17 ++++++++++++++---
 include/linux/pinctrl/pinconf-generic.h |  7 +++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index f78b416d7984..1e782a0d6e48 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -264,6 +264,7 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 	unsigned reserve;
 	struct property *prop;
 	const char *group;
+	const char *dt_pin_specifier = "pins";
 
 	ret = of_property_read_string(np, "function", &function);
 	if (ret < 0) {
@@ -284,10 +285,20 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 		reserve++;
 	if (num_configs)
 		reserve++;
+
 	ret = of_property_count_strings(np, "pins");
 	if (ret < 0) {
-		dev_err(dev, "could not parse property pins\n");
-		goto exit;
+		ret = of_property_count_strings(np, "groups");
+		if (ret < 0) {
+			dev_err(dev, "could not parse property pins/groups\n");
+			goto exit;
+		}
+		if (type == PIN_MAP_TYPE_INVALID)
+			type = PIN_MAP_TYPE_CONFIGS_GROUP;
+		dt_pin_specifier = "groups";
+	} else {
+		if (type == PIN_MAP_TYPE_INVALID)
+			type = PIN_MAP_TYPE_CONFIGS_PIN;
 	}
 	reserve *= ret;
 
@@ -296,7 +307,7 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 	if (ret < 0)
 		goto exit;
 
-	of_property_for_each_string(np, "pins", prop, group) {
+	of_property_for_each_string(np, dt_pin_specifier, prop, group) {
 		if (function) {
 			ret = pinctrl_utils_add_map_mux(pctldev, map,
 					reserved_maps, num_maps, group,
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index d578a60eff23..b6dedfbfce69 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -174,6 +174,13 @@ static inline int pinconf_generic_dt_node_to_map_pin(
 			PIN_MAP_TYPE_CONFIGS_PIN);
 }
 
+static inline int pinconf_generic_dt_node_to_map_all(
+		struct pinctrl_dev *pctldev, struct device_node *np_config,
+		struct pinctrl_map **map, unsigned *num_maps)
+{
+	return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
+			PIN_MAP_TYPE_INVALID);
+}
 #endif
 
 #endif /* CONFIG_GENERIC_PINCONF */
-- 
1.9.1

Re: [PATCH 2/7] pinctrl: pinconf-generic: Infer map type from DT property

[Laurent Pinchart]

Hi Soren,

Thank you for the patch.

On Monday 03 November 2014 11:05:26 Soren Brinkmann wrote:
> With the new 'groups' property, the DT parser can infer the map type
> from the fact whether 'pins' or 'groups' is used to specify the pin
> group to work on.
> To maintain backwards compatibitliy with current usage of the DT
> binding, this is only done when an invalid map type is passed to the
> parsing function.

The Renesas PFC implements similar bindings with using the vendor-specific 
properties "renesas,pins" and "renesas,groups" (bindings and implementation 
available at Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt 
and drivers/pinctrl/sh-pfc/pinctrl.c respectively).

The Renesas implementation is a bit more generic in that it allows both pins 
and groups to be specified in a single subnode. Do you think that feature 
would make sense for pinconf-generic as well ?

> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
> Changes since RFC v2:
>  - none
> ---
>  drivers/pinctrl/pinconf-generic.c       | 17 ++++++++++++++---
>  include/linux/pinctrl/pinconf-generic.h |  7 +++++++
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinconf-generic.c
> b/drivers/pinctrl/pinconf-generic.c index f78b416d7984..1e782a0d6e48 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -264,6 +264,7 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev
> *pctldev, unsigned reserve;
>  	struct property *prop;
>  	const char *group;
> +	const char *dt_pin_specifier = "pins";
> 
>  	ret = of_property_read_string(np, "function", &function);
>  	if (ret < 0) {
> @@ -284,10 +285,20 @@ int pinconf_generic_dt_subnode_to_map(struct
> pinctrl_dev *pctldev, reserve++;
>  	if (num_configs)
>  		reserve++;
> +
>  	ret = of_property_count_strings(np, "pins");
>  	if (ret < 0) {
> -		dev_err(dev, "could not parse property pins\n");
> -		goto exit;
> +		ret = of_property_count_strings(np, "groups");
> +		if (ret < 0) {
> +			dev_err(dev, "could not parse property pins/groups\n");
> +			goto exit;
> +		}
> +		if (type == PIN_MAP_TYPE_INVALID)
> +			type = PIN_MAP_TYPE_CONFIGS_GROUP;
> +		dt_pin_specifier = "groups";
> +	} else {
> +		if (type == PIN_MAP_TYPE_INVALID)
> +			type = PIN_MAP_TYPE_CONFIGS_PIN;
>  	}
>  	reserve *= ret;
> 
> @@ -296,7 +307,7 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev
> *pctldev, if (ret < 0)
>  		goto exit;
> 
> -	of_property_for_each_string(np, "pins", prop, group) {
> +	of_property_for_each_string(np, dt_pin_specifier, prop, group) {
>  		if (function) {
>  			ret = pinctrl_utils_add_map_mux(pctldev, map,
>  					reserved_maps, num_maps, group,
> diff --git a/include/linux/pinctrl/pinconf-generic.h
> b/include/linux/pinctrl/pinconf-generic.h index d578a60eff23..b6dedfbfce69
> 100644
> --- a/include/linux/pinctrl/pinconf-generic.h
> +++ b/include/linux/pinctrl/pinconf-generic.h
> @@ -174,6 +174,13 @@ static inline int pinconf_generic_dt_node_to_map_pin(
>  			PIN_MAP_TYPE_CONFIGS_PIN);
>  }
> 
> +static inline int pinconf_generic_dt_node_to_map_all(
> +		struct pinctrl_dev *pctldev, struct device_node *np_config,
> +		struct pinctrl_map **map, unsigned *num_maps)
> +{
> +	return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
> +			PIN_MAP_TYPE_INVALID);
> +}
>  #endif
> 
>  #endif /* CONFIG_GENERIC_PINCONF */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/7] pinctrl: pinconf-generic: Infer map type from DT property

[Sören Brinkmann]

On Wed, 2014-11-05 at 03:56PM +0200, Laurent Pinchart wrote:
> Hi Soren,
> 
> Thank you for the patch.
> 
> On Monday 03 November 2014 11:05:26 Soren Brinkmann wrote:
> > With the new 'groups' property, the DT parser can infer the map type
> > from the fact whether 'pins' or 'groups' is used to specify the pin
> > group to work on.
> > To maintain backwards compatibitliy with current usage of the DT
> > binding, this is only done when an invalid map type is passed to the
> > parsing function.
> 
> The Renesas PFC implements similar bindings with using the vendor-specific 
> properties "renesas,pins" and "renesas,groups" (bindings and implementation 
> available at Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt 
> and drivers/pinctrl/sh-pfc/pinctrl.c respectively).
> 
> The Renesas implementation is a bit more generic in that it allows both pins 
> and groups to be specified in a single subnode. Do you think that feature 
> would make sense for pinconf-generic as well ?

I don't have a use-case for that. I guess if somebody needs this kind of
functionality it could be added later. I would like to avoid blocking
pinctrl-zynq on adding more features here.

	Thanks,
	Sören

Re: [PATCH 2/7] pinctrl: pinconf-generic: Infer map type from DT property

[Laurent Pinchart]

Hi Sören,

On Wednesday 05 November 2014 10:09:35 Sören Brinkmann wrote:
> On Wed, 2014-11-05 at 03:56PM +0200, Laurent Pinchart wrote:
> > On Monday 03 November 2014 11:05:26 Soren Brinkmann wrote:
> > > With the new 'groups' property, the DT parser can infer the map type
> > > from the fact whether 'pins' or 'groups' is used to specify the pin
> > > group to work on.
> > > To maintain backwards compatibitliy with current usage of the DT
> > > binding, this is only done when an invalid map type is passed to the
> > > parsing function.
> > 
> > The Renesas PFC implements similar bindings with using the vendor-specific
> > properties "renesas,pins" and "renesas,groups" (bindings and
> > implementation available at
> > Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt and
> > drivers/pinctrl/sh-pfc/pinctrl.c respectively).
> > 
> > The Renesas implementation is a bit more generic in that it allows both
> > pins and groups to be specified in a single subnode. Do you think that
> > feature would make sense for pinconf-generic as well ?
> 
> I don't have a use-case for that. I guess if somebody needs this kind of
> functionality it could be added later. I would like to avoid blocking
> pinctrl-zynq on adding more features here.

I'm fine with that, as long as the changes won't require breaking the DT ABI. 
I don't think they would but I wanted to raise the issue in case I would have 
missed a potential issue.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/7] pinctrl: pinconf-generic: Infer map type from DT property

[Linus Walleij]

On Wed, Nov 5, 2014 at 2:56 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 03 November 2014 11:05:26 Soren Brinkmann wrote:
>> With the new 'groups' property, the DT parser can infer the map type
>> from the fact whether 'pins' or 'groups' is used to specify the pin
>> group to work on.
>> To maintain backwards compatibitliy with current usage of the DT
>> binding, this is only done when an invalid map type is passed to the
>> parsing function.
>
> The Renesas PFC implements similar bindings with using the vendor-specific
> properties "renesas,pins" and "renesas,groups" (bindings and implementation
> available at Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> and drivers/pinctrl/sh-pfc/pinctrl.c respectively).
>
> The Renesas implementation is a bit more generic in that it allows both pins
> and groups to be specified in a single subnode. Do you think that feature
> would make sense for pinconf-generic as well ?

I think for generic pin controllers either nodes with:

{
    function = "foo";
    pins = "A0", "A1", "A2";
}

or

{
    function = "foo";
    groups = "bar", "baz";
}

In parsing this it's easy to just look for "function" then see if
we're mapping to groups or pins.

It'd be nice if we could then centralize parsing of functions/pins/groups
and add the non-renesas-prefixed configs as alternatives to genericized
the bindings, while keeping backward compatibility.

Yours,
Linus Walleij

Re: [PATCH 2/7] pinctrl: pinconf-generic: Infer map type from DT property

[Sören Brinkmann]

On Tue, 2014-11-11 at 01:29PM +0100, Linus Walleij wrote:
> On Wed, Nov 5, 2014 at 2:56 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Monday 03 November 2014 11:05:26 Soren Brinkmann wrote:
> >> With the new 'groups' property, the DT parser can infer the map type
> >> from the fact whether 'pins' or 'groups' is used to specify the pin
> >> group to work on.
> >> To maintain backwards compatibitliy with current usage of the DT
> >> binding, this is only done when an invalid map type is passed to the
> >> parsing function.
> >
> > The Renesas PFC implements similar bindings with using the vendor-specific
> > properties "renesas,pins" and "renesas,groups" (bindings and implementation
> > available at Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
> > and drivers/pinctrl/sh-pfc/pinctrl.c respectively).
> >
> > The Renesas implementation is a bit more generic in that it allows both pins
> > and groups to be specified in a single subnode. Do you think that feature
> > would make sense for pinconf-generic as well ?
> 
> I think for generic pin controllers either nodes with:
> 
> {
>     function = "foo";
>     pins = "A0", "A1", "A2";
> }
> 
> or
> 
> {
>     function = "foo";
>     groups = "bar", "baz";
> }
> 
> In parsing this it's easy to just look for "function" then see if
> we're mapping to groups or pins.
> 
> It'd be nice if we could then centralize parsing of functions/pins/groups
> and add the non-renesas-prefixed configs as alternatives to genericized
> the bindings, while keeping backward compatibility.

I think the generic parser can't handle that currently. When it finds
'function' it passes the PINs to pinctrl_utils_add_map_mux(). That
function fails when it doesn't receive a group.
I don't know if the non-generic pinctrl drivers solve that somehow, but
pinconf-generic can't handle pins + function in the same node if we
change the meaning of pins to actually just be pins.

That is why I chose the "kludgy" approach. To maintain current behavior.
If nobody needs that though, things could change I guess.

	Sören

Re: [PATCH 2/7] pinctrl: pinconf-generic: Infer map type from DT property

[Linus Walleij]

On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
<soren.brinkmann@xilinx.com> wrote:

> With the new 'groups' property, the DT parser can infer the map type
> from the fact whether 'pins' or 'groups' is used to specify the pin
> group to work on.
> To maintain backwards compatibitliy with current usage of the DT
> binding, this is only done when an invalid map type is passed to the
> parsing function.

So that is this:

> +               if (type == PIN_MAP_TYPE_INVALID)
> +                       type = PIN_MAP_TYPE_CONFIGS_GROUP;
> +               dt_pin_specifier = "groups";

This is just kludgy. There are only two kernel-internal users of this function,
refactor the function signature and change the other callers over instead,
patch the drivers.

Take this opportunity to add some kerneldoc above the
pinconf_generic_dt_subnode_to_map() and give the function a
reasonable signature.

BTW: this is just for config settings for groups right? Not for muxing
I hope?

Yours,
Linus Walleij

Re: [PATCH 2/7] pinctrl: pinconf-generic: Infer map type from DT property

[Sören Brinkmann]

On Tue, 2014-11-11 at 01:47PM +0100, Linus Walleij wrote:
> On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> 
> > With the new 'groups' property, the DT parser can infer the map type
> > from the fact whether 'pins' or 'groups' is used to specify the pin
> > group to work on.
> > To maintain backwards compatibitliy with current usage of the DT
> > binding, this is only done when an invalid map type is passed to the
> > parsing function.
> 
> So that is this:
> 
> > +               if (type == PIN_MAP_TYPE_INVALID)
> > +                       type = PIN_MAP_TYPE_CONFIGS_GROUP;
> > +               dt_pin_specifier = "groups";
> 
> This is just kludgy. There are only two kernel-internal users of this function,
> refactor the function signature and change the other callers over instead,
> patch the drivers.

As I said in my other email. I chose to maintain current behavior to
avoid disturbing current users. If changing this the way you suggest,
works for everybody that is clearly preferred.

> 
> Take this opportunity to add some kerneldoc above the
> pinconf_generic_dt_subnode_to_map() and give the function a
> reasonable signature.

Yeah, didn't want to put too much effort into it without a confirmation
of this being on the right track.

> 
> BTW: this is just for config settings for groups right? Not for muxing
> I hope?

Yep, when 'function' is found, the pins/groups are passed to
pinctrl_utils_add_map_mux() which always uses mux group as type.

	Sören

Re: [PATCH 2/7] pinctrl: pinconf-generic: Infer map type from DT property

[Sören Brinkmann]

On Tue, 2014-11-11 at 01:47PM +0100, Linus Walleij wrote:
> On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> 
> > With the new 'groups' property, the DT parser can infer the map type
> > from the fact whether 'pins' or 'groups' is used to specify the pin
> > group to work on.
> > To maintain backwards compatibitliy with current usage of the DT
> > binding, this is only done when an invalid map type is passed to the
> > parsing function.
> 
> So that is this:
> 
> > +               if (type == PIN_MAP_TYPE_INVALID)
> > +                       type = PIN_MAP_TYPE_CONFIGS_GROUP;
> > +               dt_pin_specifier = "groups";
> 
> This is just kludgy. There are only two kernel-internal users of this function,
> refactor the function signature and change the other callers over instead,
> patch the drivers.

Just looking into this, one user besides those two drivers is
pinconf-generic: pinconf_generic_dt_node_to_map() which is called from
 pinconf_generic_dt_node_to_map_pin() and
 pinconf_generic_dt_node_to_map_group()

And those would be a couple of more users. That's why I chose to not
change the behavior since this adds another 4 drivers to the list of
users that might depend on certain behavior.

	Sören

[PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Soren Brinkmann]

Additionally to the generic DT parameters, allow drivers to provide
driver-specific DT parameters to be used with the generic parser
infrastructure.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/pinctrl/nomadik/pinctrl-abx500.c |   6 +-
 drivers/pinctrl/pinconf-generic.c        | 171 ++++++++++++++++---------------
 drivers/pinctrl/pinconf.h                |   1 +
 drivers/pinctrl/pinctrl-rockchip.c       |   2 +-
 drivers/pinctrl/pinctrl-tz1090-pdc.c     |   2 +-
 drivers/pinctrl/pinctrl-tz1090.c         |   2 +-
 drivers/pinctrl/sh-pfc/pinctrl.c         |   2 +-
 include/linux/pinctrl/pinconf-generic.h  |  18 ++++
 include/linux/pinctrl/pinctrl.h          |   8 ++
 9 files changed, 125 insertions(+), 87 deletions(-)

diff --git a/drivers/pinctrl/nomadik/pinctrl-abx500.c b/drivers/pinctrl/nomadik/pinctrl-abx500.c
index 228972827132..f0fe6555a557 100644
--- a/drivers/pinctrl/nomadik/pinctrl-abx500.c
+++ b/drivers/pinctrl/nomadik/pinctrl-abx500.c
@@ -915,13 +915,13 @@ static int abx500_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 		}
 	}
 
-	ret = pinconf_generic_parse_dt_config(np, &configs, &nconfigs);
+	ret = pinconf_generic_parse_dt_config(np, pctldev, &configs, &nconfigs);
 	if (nconfigs)
 		has_config = 1;
 	np_config = of_parse_phandle(np, "ste,config", 0);
 	if (np_config) {
-		ret = pinconf_generic_parse_dt_config(np_config, &configs,
-				&nconfigs);
+		ret = pinconf_generic_parse_dt_config(np_config, pctldev,
+				&configs, &nconfigs);
 		if (ret)
 			goto exit;
 		has_config |= nconfigs;
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 1e782a0d6e48..23b5359a8c39 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -27,17 +27,6 @@
 #include "pinctrl-utils.h"
 
 #ifdef CONFIG_DEBUG_FS
-
-struct pin_config_item {
-	const enum pin_config_param param;
-	const char * const display;
-	const char * const format;
-	bool has_arg;
-};
-
-#define PCONFDUMP(a, b, c, d) { .param = a, .display = b, .format = c, \
-				.has_arg = d }
-
 static const struct pin_config_item conf_items[] = {
 	PCONFDUMP(PIN_CONFIG_BIAS_DISABLE, "input bias disabled", NULL, false),
 	PCONFDUMP(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, "input bias high impedance", NULL, false),
@@ -60,22 +49,25 @@ static const struct pin_config_item conf_items[] = {
 	PCONFDUMP(PIN_CONFIG_OUTPUT, "pin output", "level", true),
 };
 
-void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
-			      struct seq_file *s, unsigned pin)
+static void _pinconf_generic_dump(struct pinctrl_dev *pctldev,
+				  struct seq_file *s, const char *gname,
+				  unsigned pin,
+				  const struct pin_config_item *items,
+				  int nitems)
 {
-	const struct pinconf_ops *ops = pctldev->desc->confops;
 	int i;
 
-	if (!ops->is_generic)
-		return;
-
-	for (i = 0; i < ARRAY_SIZE(conf_items); i++) {
+	for (i = 0; i < nitems; i++) {
 		unsigned long config;
 		int ret;
 
 		/* We want to check out this parameter */
-		config = pinconf_to_config_packed(conf_items[i].param, 0);
-		ret = pin_config_get_for_pin(pctldev, pin, &config);
+		config = pinconf_to_config_packed(items[i].param, 0);
+		if (gname)
+			ret = pin_config_group_get(dev_name(pctldev->dev),
+						   gname, &config);
+		else
+			ret = pin_config_get_for_pin(pctldev, pin, &config);
 		/* These are legal errors */
 		if (ret == -EINVAL || ret == -ENOTSUPP)
 			continue;
@@ -85,58 +77,50 @@ void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
 		}
 		/* Space between multiple configs */
 		seq_puts(s, " ");
-		seq_puts(s, conf_items[i].display);
+		seq_puts(s, items[i].display);
 		/* Print unit if available */
-		if (conf_items[i].has_arg) {
+		if (items[i].has_arg) {
 			seq_printf(s, " (%u",
 				   pinconf_to_config_argument(config));
-			if (conf_items[i].format)
-				seq_printf(s, " %s)", conf_items[i].format);
+			if (items[i].format)
+				seq_printf(s, " %s)", items[i].format);
 			else
 				seq_puts(s, ")");
 		}
 	}
 }
 
-void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
-			      struct seq_file *s, const char *gname)
+static void pinconf_generic_dump(struct pinctrl_dev *pctldev,
+				 struct seq_file *s, const char *gname,
+				 unsigned pin)
 {
 	const struct pinconf_ops *ops = pctldev->desc->confops;
-	int i;
 
 	if (!ops->is_generic)
 		return;
 
-	for (i = 0; i < ARRAY_SIZE(conf_items); i++) {
-		unsigned long config;
-		int ret;
-
-		/* We want to check out this parameter */
-		config = pinconf_to_config_packed(conf_items[i].param, 0);
-		ret = pin_config_group_get(dev_name(pctldev->dev), gname,
-					   &config);
-		/* These are legal errors */
-		if (ret == -EINVAL || ret == -ENOTSUPP)
-			continue;
-		if (ret) {
-			seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
-			continue;
-		}
-		/* Space between multiple configs */
-		seq_puts(s, " ");
-		seq_puts(s, conf_items[i].display);
-		/* Print unit if available */
-		if (conf_items[i].has_arg) {
-			seq_printf(s, " (%u",
-				   pinconf_to_config_argument(config));
-			if (conf_items[i].format)
-				seq_printf(s, " %s)", conf_items[i].format);
-			else
-				seq_puts(s, ")");
-		}
+	_pinconf_generic_dump(pctldev, s, gname, pin,
+			      conf_items, ARRAY_SIZE(conf_items));
+	if (pctldev->desc->num_dt_params) {
+		BUG_ON(!pctldev->desc->conf_items);
+		_pinconf_generic_dump(pctldev, s, gname, pin,
+				      pctldev->desc->conf_items,
+				      pctldev->desc->num_dt_params);
 	}
 }
 
+void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, unsigned pin)
+{
+	pinconf_generic_dump(pctldev, s, NULL, pin);
+}
+
+void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, const char *gname)
+{
+	pinconf_generic_dump(pctldev, s, gname, 0);
+}
+
 void pinconf_generic_dump_config(struct pinctrl_dev *pctldev,
 				 struct seq_file *s, unsigned long config)
 {
@@ -148,17 +132,22 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev,
 		seq_printf(s, "%s: 0x%x", conf_items[i].display,
 			   pinconf_to_config_argument(config));
 	}
+
+	if (!pctldev->desc->num_dt_params)
+		return;
+
+	BUG_ON(!pctldev->desc->conf_items);
+	for (i = 0; i < pctldev->desc->num_dt_params; i++) {
+		if (pinconf_to_config_param(config) != pctldev->desc->conf_items[i].param)
+			continue;
+		seq_printf(s, "%s: 0x%x", pctldev->desc->conf_items[i].display,
+			   pinconf_to_config_argument(config));
+	}
 }
 EXPORT_SYMBOL_GPL(pinconf_generic_dump_config);
 #endif
 
 #ifdef CONFIG_OF
-struct pinconf_generic_dt_params {
-	const char * const property;
-	enum pin_config_param param;
-	u32 default_value;
-};
-
 static const struct pinconf_generic_dt_params dt_params[] = {
 	{ "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
 	{ "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
@@ -183,6 +172,35 @@ static const struct pinconf_generic_dt_params dt_params[] = {
 	{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0},
 };
 
+static void _parse_dt_cfg(struct device_node *np,
+			  const struct pinconf_generic_dt_params *params,
+			  unsigned int count,
+			  unsigned long *cfg,
+			  unsigned int *ncfg)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		u32 val;
+		int ret;
+		const struct pinconf_generic_dt_params *par = &params[i];
+
+		ret = of_property_read_u32(np, par->property, &val);
+
+		/* property not found */
+		if (ret == -EINVAL)
+			continue;
+
+		/* use default value, when no value is specified */
+		if (ret)
+			val = par->default_value;
+
+		pr_debug("found %s with value %u\n", par->property, val);
+		cfg[*ncfg] = pinconf_to_config_packed(par->param, val);
+		(*ncfg)++;
+	}
+}
+
 /**
  * pinconf_generic_parse_dt_config()
  * parse the config properties into generic pinconfig values.
@@ -191,38 +209,30 @@ static const struct pinconf_generic_dt_params dt_params[] = {
  * @nconfigs: umber of configurations
  */
 int pinconf_generic_parse_dt_config(struct device_node *np,
+				    struct pinctrl_dev *pctldev,
 				    unsigned long **configs,
 				    unsigned int *nconfigs)
 {
 	unsigned long *cfg;
-	unsigned int ncfg = 0;
+	unsigned int max_cfg, ncfg = 0;
 	int ret;
-	int i;
-	u32 val;
 
 	if (!np)
 		return -EINVAL;
 
 	/* allocate a temporary array big enough to hold one of each option */
-	cfg = kzalloc(sizeof(*cfg) * ARRAY_SIZE(dt_params), GFP_KERNEL);
+	max_cfg = ARRAY_SIZE(dt_params);
+	if (pctldev)
+		max_cfg += pctldev->desc->num_dt_params;
+	cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL);
 	if (!cfg)
 		return -ENOMEM;
 
-	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
-		const struct pinconf_generic_dt_params *par = &dt_params[i];
-		ret = of_property_read_u32(np, par->property, &val);
-
-		/* property not found */
-		if (ret == -EINVAL)
-			continue;
-
-		/* use default value, when no value is specified */
-		if (ret)
-			val = par->default_value;
-
-		pr_debug("found %s with value %u\n", par->property, val);
-		cfg[ncfg] = pinconf_to_config_packed(par->param, val);
-		ncfg++;
+	_parse_dt_cfg(np, dt_params, ARRAY_SIZE(dt_params), cfg, &ncfg);
+	if (pctldev && pctldev->desc->num_dt_params) {
+		BUG_ON(!pctldev->desc->params);
+		_parse_dt_cfg(np, pctldev->desc->params,
+			      pctldev->desc->num_dt_params, cfg, &ncfg);
 	}
 
 	ret = 0;
@@ -274,7 +284,8 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 		function = NULL;
 	}
 
-	ret = pinconf_generic_parse_dt_config(np, &configs, &num_configs);
+	ret = pinconf_generic_parse_dt_config(np, pctldev, &configs,
+					      &num_configs);
 	if (ret < 0) {
 		dev_err(dev, "could not parse node property\n");
 		return ret;
diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
index a4a5417e1413..6a6c55864672 100644
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -126,6 +126,7 @@ static inline void pinconf_generic_dump_config(struct pinctrl_dev *pctldev,
 
 #if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_OF)
 int pinconf_generic_parse_dt_config(struct device_node *np,
+				    struct pinctrl_dev *pctldev,
 				    unsigned long **configs,
 				    unsigned int *nconfigs);
 #endif
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 016f4578e494..0e05febe80ba 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -1126,7 +1126,7 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np,
 			return -EINVAL;
 
 		np_config = of_find_node_by_phandle(be32_to_cpup(phandle));
-		ret = pinconf_generic_parse_dt_config(np_config,
+		ret = pinconf_generic_parse_dt_config(np_config, NULL,
 				&grp->data[j].configs, &grp->data[j].nconfigs);
 		if (ret)
 			return ret;
diff --git a/drivers/pinctrl/pinctrl-tz1090-pdc.c b/drivers/pinctrl/pinctrl-tz1090-pdc.c
index 3bb6a3b78864..0e3576ba1a7c 100644
--- a/drivers/pinctrl/pinctrl-tz1090-pdc.c
+++ b/drivers/pinctrl/pinctrl-tz1090-pdc.c
@@ -415,7 +415,7 @@ static int tz1090_pdc_pinctrl_dt_subnode_to_map(struct device *dev,
 		function = NULL;
 	}
 
-	ret = pinconf_generic_parse_dt_config(np, &configs, &num_configs);
+	ret = pinconf_generic_parse_dt_config(np, NULL, &configs, &num_configs);
 	if (ret)
 		return ret;
 
diff --git a/drivers/pinctrl/pinctrl-tz1090.c b/drivers/pinctrl/pinctrl-tz1090.c
index 48d36413b99f..0b36566db451 100644
--- a/drivers/pinctrl/pinctrl-tz1090.c
+++ b/drivers/pinctrl/pinctrl-tz1090.c
@@ -1131,7 +1131,7 @@ static int tz1090_pinctrl_dt_subnode_to_map(struct device *dev,
 		function = NULL;
 	}
 
-	ret = pinconf_generic_parse_dt_config(np, &configs, &num_configs);
+	ret = pinconf_generic_parse_dt_config(np, NULL, &configs, &num_configs);
 	if (ret)
 		return ret;
 
diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c
index 910deaefa0ac..072e7c62cab7 100644
--- a/drivers/pinctrl/sh-pfc/pinctrl.c
+++ b/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -122,7 +122,7 @@ static int sh_pfc_dt_subnode_to_map(struct device *dev, struct device_node *np,
 		return ret;
 	}
 
-	ret = pinconf_generic_parse_dt_config(np, &configs, &num_configs);
+	ret = pinconf_generic_parse_dt_config(np, NULL, &configs, &num_configs);
 	if (ret < 0)
 		return ret;
 
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index b6dedfbfce69..5731703b27c2 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -115,6 +115,18 @@ enum pin_config_param {
 	PIN_CONFIG_END = 0x7FFF,
 };
 
+#ifdef CONFIG_DEBUG_FS
+#define PCONFDUMP(a, b, c, d) { .param = a, .display = b, .format = c, \
+				.has_arg = d }
+
+struct pin_config_item {
+	const enum pin_config_param param;
+	const char * const display;
+	const char * const format;
+	bool has_arg;
+};
+#endif /* CONFIG_DEBUG_FS */
+
 /*
  * Helpful configuration macro to be used in tables etc.
  */
@@ -150,6 +162,12 @@ static inline unsigned long pinconf_to_config_packed(enum pin_config_param param
 struct pinctrl_dev;
 struct pinctrl_map;
 
+struct pinconf_generic_dt_params {
+	const char * const property;
+	enum pin_config_param param;
+	u32 default_value;
+};
+
 int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 		struct device_node *np, struct pinctrl_map **map,
 		unsigned *reserved_maps, unsigned *num_maps,
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index cc8e1aff0e28..36f749775342 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -24,6 +24,7 @@ struct pinctrl_dev;
 struct pinctrl_map;
 struct pinmux_ops;
 struct pinconf_ops;
+struct pin_config_item;
 struct gpio_chip;
 struct device_node;
 
@@ -117,6 +118,8 @@ struct pinctrl_ops {
  * @confops: pin config operations vtable, if you support pin configuration in
  *	your driver
  * @owner: module providing the pin controller, used for refcounting
+ * @num_dt_params: Number of driver-specifid DT parameters
+ * @params: List of DT parameters
  */
 struct pinctrl_desc {
 	const char *name;
@@ -126,6 +129,11 @@ struct pinctrl_desc {
 	const struct pinmux_ops *pmxops;
 	const struct pinconf_ops *confops;
 	struct module *owner;
+#if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_OF)
+	unsigned int num_dt_params;
+	const struct pinconf_generic_dt_params *params;
+	const struct pin_config_item *conf_items;
+#endif
 };
 
 /* External interface to pin controller */
-- 
1.9.1

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Geert Uytterhoeven]

On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> --- a/include/linux/pinctrl/pinctrl.h
> +++ b/include/linux/pinctrl/pinctrl.h

> @@ -117,6 +118,8 @@ struct pinctrl_ops {
>   * @confops: pin config operations vtable, if you support pin configuration in
>   *     your driver
>   * @owner: module providing the pin controller, used for refcounting
> + * @num_dt_params: Number of driver-specifid DT parameters

driver-specific

> + * @params: List of DT parameters

Missing @conf_items documentation.

>   */
>  struct pinctrl_desc {
>         const char *name;
> @@ -126,6 +129,11 @@ struct pinctrl_desc {
>         const struct pinmux_ops *pmxops;
>         const struct pinconf_ops *confops;
>         struct module *owner;
> +#if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_OF)
> +       unsigned int num_dt_params;
> +       const struct pinconf_generic_dt_params *params;
> +       const struct pin_config_item *conf_items;
> +#endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Linus Walleij]

On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
<soren.brinkmann@xilinx.com> wrote:

> Additionally to the generic DT parameters, allow drivers to provide
> driver-specific DT parameters to be used with the generic parser
> infrastructure.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

I like the looks of this, but the patch description is a bit terse.
I'd like it to describe some of the refactorings being done
to the intrinsics, because I have a hard time following the patch.

First please rebase onto the "devel" branch in the pin control
tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
which is merged there is actually doing this already:


        for_each_child_of_node(np_config, np) {
                ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map,
                                                        &reserv, nmaps, type);
                if (ret)
                        break;

                ret = pmic_gpio_dt_subnode_to_map(pctldev, np, map, &reserv,
                                                  nmaps, type);
                if (ret)
                        break;
        }

So it should be patched to illustrate the point of this code.

I'd like feedback from Ivan+Björn on the code too if possible.

> -       ret = pinconf_generic_parse_dt_config(np, &configs, &nconfigs);
> +       ret = pinconf_generic_parse_dt_config(np, pctldev, &configs, &nconfigs);
>         if (nconfigs)
>                 has_config = 1;
>         np_config = of_parse_phandle(np, "ste,config", 0);
>         if (np_config) {
> -               ret = pinconf_generic_parse_dt_config(np_config, &configs,
> -                               &nconfigs);
> +               ret = pinconf_generic_parse_dt_config(np_config, pctldev,
> +                               &configs, &nconfigs);

This code is patched upstream so that ABx500 only uses generic config.
Again rebase on "devel"

> -void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> -                             struct seq_file *s, unsigned pin)
> +static void _pinconf_generic_dump(struct pinctrl_dev *pctldev,
> +                                 struct seq_file *s, const char *gname,
> +                                 unsigned pin,
> +                                 const struct pin_config_item *items,
> +                                 int nitems)

Don't use functions named _foo, actually the underscore is for
preprocessor and compiler things in my book, just give it an intuitive
name instead. Like pinconf_generic_dump_one() if that is suitable
or whatever.

This changes the function signature from something quite intuitively
understood to something pretty hard to understand, so you need to
add kerneldoc to it. (That also enhance my understanding of the
patch.)

> -void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
> -                             struct seq_file *s, const char *gname)
> +static void pinconf_generic_dump(struct pinctrl_dev *pctldev,
> +                                struct seq_file *s, const char *gname,
> +                                unsigned pin)

This looks intuitive and nice.

> +       _pinconf_generic_dump(pctldev, s, gname, pin,
> +                             conf_items, ARRAY_SIZE(conf_items));
> +       if (pctldev->desc->num_dt_params) {
> +               BUG_ON(!pctldev->desc->conf_items);

Don't use BUG_ON() like that, it's nasty. Always try to
recover and bail out instead.

> +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> +                             struct seq_file *s, unsigned pin)
> +{
> +       pinconf_generic_dump(pctldev, s, NULL, pin);
> +}
> +
> +void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
> +                             struct seq_file *s, const char *gname)
> +{
> +       pinconf_generic_dump(pctldev, s, gname, 0);
> +}

Do you really need these helpers? Isn't it simpler just
to call the generic function with the different arguments?

> @@ -148,17 +132,22 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev,
>                 seq_printf(s, "%s: 0x%x", conf_items[i].display,
>                            pinconf_to_config_argument(config));
>         }
> +
> +       if (!pctldev->desc->num_dt_params)
> +               return;
> +
> +       BUG_ON(!pctldev->desc->conf_items);

No BUG_ON() dev_err() and exit.

> +static void _parse_dt_cfg(struct device_node *np,
> +                         const struct pinconf_generic_dt_params *params,
> +                         unsigned int count,
> +                         unsigned long *cfg,
> +                         unsigned int *ncfg)

Should return an error code right? Kerneldoc doesn't hurt either.

> +{
> +       int i;
> +
> +       for (i = 0; i < count; i++) {
> +               u32 val;
> +               int ret;
> +               const struct pinconf_generic_dt_params *par = &params[i];
> +
> +               ret = of_property_read_u32(np, par->property, &val);

Not checking this return value. Alter the function to return an
int value on success.

> +
> +               /* property not found */
> +               if (ret == -EINVAL)
> +                       continue;
> +
> +               /* use default value, when no value is specified */
> +               if (ret)
> +                       val = par->default_value;
> +
> +               pr_debug("found %s with value %u\n", par->property, val);
> +               cfg[*ncfg] = pinconf_to_config_packed(par->param, val);
> +               (*ncfg)++;
> +       }
> +}

There is something very unintuitive about this loop. You pass two
counter indexes (count, ncfg) in basically, that is looking weird,
does it have to look like that? Especially since there is no
bounds check on ncfg!

Just use one index in the loop please. Assign  *ncfg = ... after
the loop has *successfully* iterated.

>  int pinconf_generic_parse_dt_config(struct device_node *np,
> +                                   struct pinctrl_dev *pctldev,
>                                     unsigned long **configs,
>                                     unsigned int *nconfigs)

This is a good refactoring, but no _foo naming!

>  {
>         unsigned long *cfg;
> -       unsigned int ncfg = 0;
> +       unsigned int max_cfg, ncfg = 0;
>         int ret;
> -       int i;
> -       u32 val;
>
>         if (!np)
>                 return -EINVAL;
>
>         /* allocate a temporary array big enough to hold one of each option */
> -       cfg = kzalloc(sizeof(*cfg) * ARRAY_SIZE(dt_params), GFP_KERNEL);
> +       max_cfg = ARRAY_SIZE(dt_params);
> +       if (pctldev)
> +               max_cfg += pctldev->desc->num_dt_params;
> +       cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL);

Aha this looks good...

> +       _parse_dt_cfg(np, dt_params, ARRAY_SIZE(dt_params), cfg, &ncfg);
> +       if (pctldev && pctldev->desc->num_dt_params) {
> +               BUG_ON(!pctldev->desc->params);

No BUG_ON()

> +               _parse_dt_cfg(np, pctldev->desc->params,
> +                             pctldev->desc->num_dt_params, cfg, &ncfg);

This looks similar to what Qualcomm's driver is doing.

Yours,
Linus Walleij

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Ivan T. Ivanov]

On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote:
> On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> brinkmann@xilinx.com> wrote:
> 
> > Additionally to the generic DT parameters, allow drivers to provide
> > driver-specific DT parameters to be used with the generic parser
> > infrastructure.
> > 
> > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com>
> 
> I like the looks of this, but the patch description is a bit terse.
> I'd like it to describe some of the refactorings being done
> to the intrinsics, because I have a hard time following the patch.
> 
> First please rebase onto the "devel" branch in the pin control
> tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> which is merged there is actually doing this already:
> 
> 
>         for_each_child_of_node(np_config, np) {
>                 ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map,
>                                                         &reserv, nmaps, type);
>                 if (ret)
>                         break;
> 
>                 ret = pmic_gpio_dt_subnode_to_map(pctldev, np, map, &reserv,
>                                                   nmaps, type);
>                 if (ret)
>                         break;
>         }
> 
> So it should be patched to illustrate the point of this code.
> 

I like the idea, but have issues with implementations :-). 
 
It is supposed that additional parameters are not generic, 
otherwise they will be part of enum pin_config_param, right?

Probably it will be better if clients could pass array with
driver specific dt bindings to pinconf_generic_dt_node_to_map()?

Regards,
Ivan

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Sören Brinkmann]

On Tue, 2014-11-18 at 10:50AM +0200, Ivan T. Ivanov wrote:
> 
> On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote:
> > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> > brinkmann@xilinx.com> wrote:
> > 
> > > Additionally to the generic DT parameters, allow drivers to provide
> > > driver-specific DT parameters to be used with the generic parser
> > > infrastructure.
> > > 
> > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com>
> > 
> > I like the looks of this, but the patch description is a bit terse.
> > I'd like it to describe some of the refactorings being done
> > to the intrinsics, because I have a hard time following the patch.
> > 
> > First please rebase onto the "devel" branch in the pin control
> > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > which is merged there is actually doing this already:
> > 
> > 
> >         for_each_child_of_node(np_config, np) {
> >                 ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map,
> >                                                         &reserv, nmaps, type);
> >                 if (ret)
> >                         break;
> > 
> >                 ret = pmic_gpio_dt_subnode_to_map(pctldev, np, map, &reserv,
> >                                                   nmaps, type);
> >                 if (ret)
> >                         break;
> >         }
> > 
> > So it should be patched to illustrate the point of this code.
> > 
> 
> I like the idea, but have issues with implementations :-). 
>  
> It is supposed that additional parameters are not generic, 
> otherwise they will be part of enum pin_config_param, right?
> 
> Probably it will be better if clients could pass array with
> driver specific dt bindings to pinconf_generic_dt_node_to_map()?

My idea was to hide that API from the driver. You just pass those
parameters as part of the struct pctldev and the parser - whether this
generic one or anything else - would do the right thing. I don't think
calling the parser from the driver is the right approach.

	Thanks,
	Sören

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Ivan T. Ivanov]

On Tue, 2014-11-18 at 09:25 -0800, Sören Brinkmann wrote:
> On Tue, 2014-11-18 at 10:50AM +0200, Ivan T. Ivanov wrote:
> > 
> > On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote:
> > > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> > > brinkmann@xilinx.com> wrote:
> > > 
> > > > Additionally to the generic DT parameters, allow drivers to 
> > > > provide driver-specific DT parameters to be used with the 
> > > > generic parser infrastructure.
> > > > 
> > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com>
> > > 
> > > I like the looks of this, but the patch description is a bit 
> > > terse. I'd like it to describe some of the refactorings being 
> > > done
> > > to the intrinsics, because I have a hard time following the 
> > > patch.
> > > 
> > > First please rebase onto the "devel" branch in the pin control 
> > > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
> > > which is merged there is actually doing this already:
> > > 
> > > 
> > >         for_each_child_of_node(np_config, np) {
> > >                 ret = pinconf_generic_dt_subnode_to_map(pctldev, 
> > > np, map,
> > >                                                         &reserv, 
> > > nmaps, type);
> > >                 if (ret)
> > >                         break;
> > > 
> > >                 ret = pmic_gpio_dt_subnode_to_map(pctldev, np, 
> > > map, &reserv,
> > >                                                   nmaps, type);
> > >                 if (ret)
> > >                         break;
> > >         }
> > > 
> > > So it should be patched to illustrate the point of this code.
> > > 
> > 
> > I like the idea, but have issues with implementations :-).
> >  
> > It is supposed that additional parameters are not generic,
> > otherwise they will be part of enum pin_config_param, right?
> > 
> > Probably it will be better if clients could pass array with
> > driver specific dt bindings to pinconf_generic_dt_node_to_map()?
> 
> My idea was to hide that API from the driver. You just pass those 
> parameters as part of the struct pctldev and the parser - whether 
> this generic one or anything else - would do the right thing. I 
> don't think calling the parser from the driver is the right approach.

Drivers already know about dt_node_to_map(). My proposal will make
drivers, which register non-standard bindings, little bit simpler.

With your approach probably we can remove dt_node_to_map() and
dt_free_map() callbacks?

Regards,
Ivan

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Sören Brinkmann]

Hi Ivan,

On Wed, 2014-11-19 at 09:49AM +0200, Ivan T. Ivanov wrote:
> 
> On Tue, 2014-11-18 at 09:25 -0800, Sören Brinkmann wrote:
> > On Tue, 2014-11-18 at 10:50AM +0200, Ivan T. Ivanov wrote:
> > > 
> > > On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote:
> > > > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> > > > brinkmann@xilinx.com> wrote:
> > > > 
> > > > > Additionally to the generic DT parameters, allow drivers to 
> > > > > provide driver-specific DT parameters to be used with the 
> > > > > generic parser infrastructure.
> > > > > 
> > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com>
> > > > 
> > > > I like the looks of this, but the patch description is a bit 
> > > > terse. I'd like it to describe some of the refactorings being 
> > > > done
> > > > to the intrinsics, because I have a hard time following the 
> > > > patch.
> > > > 
> > > > First please rebase onto the "devel" branch in the pin control 
> > > > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
> > > > which is merged there is actually doing this already:
> > > > 
> > > > 
> > > >         for_each_child_of_node(np_config, np) {
> > > >                 ret = pinconf_generic_dt_subnode_to_map(pctldev, 
> > > > np, map,
> > > >                                                         &reserv, 
> > > > nmaps, type);
> > > >                 if (ret)
> > > >                         break;
> > > > 
> > > >                 ret = pmic_gpio_dt_subnode_to_map(pctldev, np, 
> > > > map, &reserv,
> > > >                                                   nmaps, type);
> > > >                 if (ret)
> > > >                         break;
> > > >         }
> > > > 
> > > > So it should be patched to illustrate the point of this code.
> > > > 
> > > 
> > > I like the idea, but have issues with implementations :-).
> > >  
> > > It is supposed that additional parameters are not generic,
> > > otherwise they will be part of enum pin_config_param, right?
> > > 
> > > Probably it will be better if clients could pass array with
> > > driver specific dt bindings to pinconf_generic_dt_node_to_map()?
> > 
> > My idea was to hide that API from the driver. You just pass those 
> > parameters as part of the struct pctldev and the parser - whether 
> > this generic one or anything else - would do the right thing. I 
> > don't think calling the parser from the driver is the right approach.
> 
> Drivers already know about dt_node_to_map(). My proposal will make
> drivers, which register non-standard bindings, little bit simpler.

And I think this is not the best solution. Those drivers essentially
still do the DT parsing themselves, just call some common helpers. I
think that should be well separated. The pinctrl driver just assembles
some data structure that has the information regarding custom properties
and the core handles the rest. I find the approach I have in the zynq
driver - which does it that way - more elegant. The only reference to
the core parser there is the function pointer to the generic node to map
function. And even that could probably disappear in the long term when
everything migrates to using the core parser and generic bindings.

Also, why does it make the driver simpler? In my zynq driver I only have
those mentioned data structs and nothing in regards of parsing the DT.
With drivers calling the parser you duplicate exactly that all over the
place in each driver. More code, more duplication. And I don't see where
things become simpler. The core becomes a little more complex, but well,
that's why it gets consolidated there, right?

	Thanks,
	Sören

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Ivan T. Ivanov]

On Wed, 2014-11-19 at 07:35 -0800, Sören Brinkmann wrote:
> Hi Ivan,
> 
> On Wed, 2014-11-19 at 09:49AM +0200, Ivan T. Ivanov wrote:
> > On Tue, 2014-11-18 at 09:25 -0800, Sören Brinkmann wrote:
> > > On Tue, 2014-11-18 at 10:50AM +0200, Ivan T. Ivanov wrote:
> > > > On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote:
> > > > > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> > > > > brinkmann@xilinx.com> wrote:
> > > > > 
> > > > > > Additionally to the generic DT parameters, allow drivers to
> > > > > > provide driver-specific DT parameters to be used with the
> > > > > > generic parser infrastructure.
> > > > > > 
> > > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com>
> > > > > 
> > > > > I like the looks of this, but the patch description is a bit
> > > > > terse. I'd like it to describe some of the refactorings being
> > > > > done
> > > > > to the intrinsics, because I have a hard time following the
> > > > > patch.
> > > > > 
> > > > > First please rebase onto the "devel" branch in the pin control
> > > > > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > > > which is merged there is actually doing this already:
> > > > > 
> > > > > 
> > > > >         for_each_child_of_node(np_config, np) {
> > > > >                 ret = pinconf_generic_dt_subnode_to_map(pctldev,
> > > > > np, map,
> > > > >                                                         &reserv,
> > > > > nmaps, type);
> > > > >                 if (ret)
> > > > >                         break;
> > > > > 
> > > > >                 ret = pmic_gpio_dt_subnode_to_map(pctldev, np,
> > > > > map, &reserv,
> > > > >                                                   nmaps, type);
> > > > >                 if (ret)
> > > > >                         break;
> > > > >         }
> > > > > 
> > > > > So it should be patched to illustrate the point of this code.
> > > > > 
> > > > 
> > > > I like the idea, but have issues with implementations :-).
> > > > 
> > > > It is supposed that additional parameters are not generic,
> > > > otherwise they will be part of enum pin_config_param, right?
> > > > 
> > > > Probably it will be better if clients could pass array with
> > > > driver specific dt bindings to pinconf_generic_dt_node_to_map()?
> > > 
> > > My idea was to hide that API from the driver. You just pass those
> > > parameters as part of the struct pctldev and the parser - whether
> > > this generic one or anything else - would do the right thing. I
> > > don't think calling the parser from the driver is the right approach.
> > 
> > Drivers already know about dt_node_to_map(). My proposal will make
> > drivers, which register non-standard bindings, little bit simpler.
> 
> And I think this is not the best solution. Those drivers essentially
> still do the DT parsing themselves, 

Yes, and this could be avoided if there API which allow them to pass 
non-standard configuration maps.

> just call some common helpers. I
> think that should be well separated. 

Around 27 of 30 drivers are using custom dt_node_to_map(). And this is
because most of them are using custom "x,pins", "x,functions" and 'x,groups"
properties and I think that this is bigger issue, how this is addressed
in this patch?

> The pinctrl driver just assembles
> some data structure that has the information regarding custom properties
> and the core handles the rest. 

Yup, that is nice. What will be really nice if it also handle custom, 
"function", "groups" and "pins" properties. Otherwise most of the drivers
will not be able to benefit from this. 

I just wanted to share my opinion.

Regards,
Ivan

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Sören Brinkmann]

On Thu, 2014-11-20 at 10:06AM +0200, Ivan T. Ivanov wrote:
> 
> On Wed, 2014-11-19 at 07:35 -0800, Sören Brinkmann wrote:
> > Hi Ivan,
> > 
> > On Wed, 2014-11-19 at 09:49AM +0200, Ivan T. Ivanov wrote:
> > > On Tue, 2014-11-18 at 09:25 -0800, Sören Brinkmann wrote:
> > > > On Tue, 2014-11-18 at 10:50AM +0200, Ivan T. Ivanov wrote:
> > > > > On Tue, 2014-11-11 at 15:53 +0100, Linus Walleij wrote:
> > > > > > On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> > > > > > brinkmann@xilinx.com> wrote:
> > > > > > 
> > > > > > > Additionally to the generic DT parameters, allow drivers to
> > > > > > > provide driver-specific DT parameters to be used with the
> > > > > > > generic parser infrastructure.
> > > > > > > 
> > > > > > > Signed-off-by: Soren Brinkmann brinkmann@xilinx.com>
> > > > > > 
> > > > > > I like the looks of this, but the patch description is a bit
> > > > > > terse. I'd like it to describe some of the refactorings being
> > > > > > done
> > > > > > to the intrinsics, because I have a hard time following the
> > > > > > patch.
> > > > > > 
> > > > > > First please rebase onto the "devel" branch in the pin control
> > > > > > tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> > > > > > which is merged there is actually doing this already:
> > > > > > 
> > > > > > 
> > > > > >         for_each_child_of_node(np_config, np) {
> > > > > >                 ret = pinconf_generic_dt_subnode_to_map(pctldev,
> > > > > > np, map,
> > > > > >                                                         &reserv,
> > > > > > nmaps, type);
> > > > > >                 if (ret)
> > > > > >                         break;
> > > > > > 
> > > > > >                 ret = pmic_gpio_dt_subnode_to_map(pctldev, np,
> > > > > > map, &reserv,
> > > > > >                                                   nmaps, type);
> > > > > >                 if (ret)
> > > > > >                         break;
> > > > > >         }
> > > > > > 
> > > > > > So it should be patched to illustrate the point of this code.
> > > > > > 
> > > > > 
> > > > > I like the idea, but have issues with implementations :-).
> > > > > 
> > > > > It is supposed that additional parameters are not generic,
> > > > > otherwise they will be part of enum pin_config_param, right?
> > > > > 
> > > > > Probably it will be better if clients could pass array with
> > > > > driver specific dt bindings to pinconf_generic_dt_node_to_map()?
> > > > 
> > > > My idea was to hide that API from the driver. You just pass those
> > > > parameters as part of the struct pctldev and the parser - whether
> > > > this generic one or anything else - would do the right thing. I
> > > > don't think calling the parser from the driver is the right approach.
> > > 
> > > Drivers already know about dt_node_to_map(). My proposal will make
> > > drivers, which register non-standard bindings, little bit simpler.
> > 
> > And I think this is not the best solution. Those drivers essentially
> > still do the DT parsing themselves, 
> 
> Yes, and this could be avoided if there API which allow them to pass 
> non-standard configuration maps.
> 
> > just call some common helpers. I
> > think that should be well separated. 
> 
> Around 27 of 30 drivers are using custom dt_node_to_map(). And this is
> because most of them are using custom "x,pins", "x,functions" and 'x,groups"
> properties and I think that this is bigger issue, how this is addressed
> in this patch?

Not really in this patch, but Linus recently added the 'groups'
property. Somewhere in this thread he explained how he'd like to see
things used. With mux nodes that contain 'groups' and 'function' and
conf nodes that can contain 'groups' or 'pins' and the pinconf options.
And pins/groups would actually refer to only pins or groups
respectively.

Also, I hope all my changes here don't break the current behavior. So,
those 27 driver should still be able to do what they currently do. But I
hope they could migrated over to use the generic bindings only in the
longer term, so that these custom properties disappear.

> 
> > The pinctrl driver just assembles
> > some data structure that has the information regarding custom properties
> > and the core handles the rest. 
> 
> Yup, that is nice. What will be really nice if it also handle custom, 
> "function", "groups" and "pins" properties. Otherwise most of the drivers
> will not be able to benefit from this. 

Why would you still need those? I think the idea is to get rid of custom
pins, groups and function properties. Could you explain why those are
needed and why that couldn't be migrated to the amended bindings as outlined by
Linus, please?

	Thanks,
	Sören

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Ivan T. Ivanov]

On Thu, 2014-11-20 at 08:22 -0800, Sören Brinkmann wrote:
> 
> 
> Also, I hope all my changes here don't break the current behavior. So,
> those 27 driver should still be able to do what they currently do. But I
> hope they could migrated over to use the generic bindings only in the
> longer term, so that these custom properties disappear.
> 
> > > The pinctrl driver just assembles
> > > some data structure that has the information regarding custom properties
> > > and the core handles the rest.
> > 
> > Yup, that is nice. What will be really nice if it also handle custom,
> > "function", "groups" and "pins" properties. Otherwise most of the drivers
> > will not be able to benefit from this.
> 
> Why would you still need those? 

I don't need them :-). The point was that still majority of the drivers
will have custom parsing functions. It would be nice if we could fix
that too. I do understand that using custom "pins", "functions"... is
something which is deprecated, but if core parsing functions allow
passing custom strings for above purposes, in a similar way as your
proposal, it will be easier for those drivers to migrate, I believe.

Regards,
Ivan

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Sören Brinkmann]

Hi Ivan,

On Fri, 2014-11-21 at 09:35AM +0200, Ivan T. Ivanov wrote:
> 
> On Thu, 2014-11-20 at 08:22 -0800, Sören Brinkmann wrote:
> > 
> > 
> > Also, I hope all my changes here don't break the current behavior. So,
> > those 27 driver should still be able to do what they currently do. But I
> > hope they could migrated over to use the generic bindings only in the
> > longer term, so that these custom properties disappear.
> > 
> > > > The pinctrl driver just assembles
> > > > some data structure that has the information regarding custom properties
> > > > and the core handles the rest.
> > > 
> > > Yup, that is nice. What will be really nice if it also handle custom,
> > > "function", "groups" and "pins" properties. Otherwise most of the drivers
> > > will not be able to benefit from this.
> > 
> > Why would you still need those? 
> 
> I don't need them :-). The point was that still majority of the drivers
> will have custom parsing functions. It would be nice if we could fix
> that too. I do understand that using custom "pins", "functions"... is
> something which is deprecated, but if core parsing functions allow
> passing custom strings for above purposes, in a similar way as your
> proposal, it will be easier for those drivers to migrate, I believe.

This does sound much more like a feature request than a fundamental
problem with the implementation, now. And like Laurent's feature
request, I'd like to turn this down. Otherwise this just gets hold up by
feature requests blocking pinctrl-zynq.
I think the interesting questions are:
1. Does it break any current user?
2. Is there anything fundamentally preventing adding your feature at
some later time as part of such a migration you describe?

	Thanks,
	Sören

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Ivan T. Ivanov]

On Sat, 2014-11-22 at 08:06 -0800, Sören Brinkmann wrote:
> Hi Ivan,
> 
> On Fri, 2014-11-21 at 09:35AM +0200, Ivan T. Ivanov wrote:
> > On Thu, 2014-11-20 at 08:22 -0800, Sören Brinkmann wrote:
> > > 
> > > Also, I hope all my changes here don't break the current behavior. So,
> > > those 27 driver should still be able to do what they currently do. But I
> > > hope they could migrated over to use the generic bindings only in the
> > > longer term, so that these custom properties disappear.
> > > 
> > > > > The pinctrl driver just assembles
> > > > > some data structure that has the information regarding custom properties
> > > > > and the core handles the rest.
> > > > 
> > > > Yup, that is nice. What will be really nice if it also handle custom,
> > > > "function", "groups" and "pins" properties. Otherwise most of the drivers
> > > > will not be able to benefit from this.
> > > 
> > > Why would you still need those?
> > 
> > I don't need them :-). The point was that still majority of the drivers
> > will have custom parsing functions. It would be nice if we could fix
> > that too. I do understand that using custom "pins", "functions"... is
> > something which is deprecated, but if core parsing functions allow
> > passing custom strings for above purposes, in a similar way as your
> > proposal, it will be easier for those drivers to migrate, I believe.
> 
> This does sound much more like a feature request than a fundamental
> problem with the implementation, now. And like Laurent's feature
> request, I'd like to turn this down. Otherwise this just gets hold up by
> feature requests blocking pinctrl-zynq.
> I think the interesting questions are:
> 1. Does it break any current user?
> 2. Is there anything fundamentally preventing adding your feature at
> some later time as part of such a migration you describe?

Well, as I said, this is just my opinion. Is up to Linus to decide.

Regards,
Ivan

Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params

[Sören Brinkmann]

Hi Linus,

On Tue, 2014-11-11 at 03:53PM +0100, Linus Walleij wrote:
> On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> 
> > Additionally to the generic DT parameters, allow drivers to provide
> > driver-specific DT parameters to be used with the generic parser
> > infrastructure.
> >
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> 
> I like the looks of this, but the patch description is a bit terse.
> I'd like it to describe some of the refactorings being done
> to the intrinsics, because I have a hard time following the patch.

I'll be a little more verbose :)

> 
> First please rebase onto the "devel" branch in the pin control
> tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> which is merged there is actually doing this already:
> 
> 
>         for_each_child_of_node(np_config, np) {
>                 ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map,
>                                                         &reserv, nmaps, type);
>                 if (ret)
>                         break;
> 
>                 ret = pmic_gpio_dt_subnode_to_map(pctldev, np, map, &reserv,
>                                                   nmaps, type);
>                 if (ret)
>                         break;
>         }
> 
> So it should be patched to illustrate the point of this code.

I'll look into this.

> 
> I'd like feedback from Ivan+Björn on the code too if possible.
> 
> > -       ret = pinconf_generic_parse_dt_config(np, &configs, &nconfigs);
> > +       ret = pinconf_generic_parse_dt_config(np, pctldev, &configs, &nconfigs);
> >         if (nconfigs)
> >                 has_config = 1;
> >         np_config = of_parse_phandle(np, "ste,config", 0);
> >         if (np_config) {
> > -               ret = pinconf_generic_parse_dt_config(np_config, &configs,
> > -                               &nconfigs);
> > +               ret = pinconf_generic_parse_dt_config(np_config, pctldev,
> > +                               &configs, &nconfigs);
> 
> This code is patched upstream so that ABx500 only uses generic config.
> Again rebase on "devel"

Yeah, causes a conflict, but seems to be pretty much the same.

> 
> > -void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> > -                             struct seq_file *s, unsigned pin)
> > +static void _pinconf_generic_dump(struct pinctrl_dev *pctldev,
> > +                                 struct seq_file *s, const char *gname,
> > +                                 unsigned pin,
> > +                                 const struct pin_config_item *items,
> > +                                 int nitems)
> 
> Don't use functions named _foo, actually the underscore is for
> preprocessor and compiler things in my book, just give it an intuitive
> name instead. Like pinconf_generic_dump_one() if that is suitable
> or whatever.
> 
> This changes the function signature from something quite intuitively
> understood to something pretty hard to understand, so you need to
> add kerneldoc to it. (That also enhance my understanding of the
> patch.)

I'll rename it and add some documentation.

> 
> > -void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
> > -                             struct seq_file *s, const char *gname)
> > +static void pinconf_generic_dump(struct pinctrl_dev *pctldev,
> > +                                struct seq_file *s, const char *gname,
> > +                                unsigned pin)
> 
> This looks intuitive and nice.
> 
> > +       _pinconf_generic_dump(pctldev, s, gname, pin,
> > +                             conf_items, ARRAY_SIZE(conf_items));
> > +       if (pctldev->desc->num_dt_params) {
> > +               BUG_ON(!pctldev->desc->conf_items);
> 
> Don't use BUG_ON() like that, it's nasty. Always try to
> recover and bail out instead.

I merge the condition into the if.

> 
> > +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> > +                             struct seq_file *s, unsigned pin)
> > +{
> > +       pinconf_generic_dump(pctldev, s, NULL, pin);
> > +}
> > +
> > +void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
> > +                             struct seq_file *s, const char *gname)
> > +{
> > +       pinconf_generic_dump(pctldev, s, gname, 0);
> > +}
> 
> Do you really need these helpers? Isn't it simpler just
> to call the generic function with the different arguments?

I'll remove the helpers and patch the users of these functions.

> 
> > @@ -148,17 +132,22 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev,
> >                 seq_printf(s, "%s: 0x%x", conf_items[i].display,
> >                            pinconf_to_config_argument(config));
> >         }
> > +
> > +       if (!pctldev->desc->num_dt_params)
> > +               return;
> > +
> > +       BUG_ON(!pctldev->desc->conf_items);
> 
> No BUG_ON() dev_err() and exit.

As above.

> 
> > +static void _parse_dt_cfg(struct device_node *np,
> > +                         const struct pinconf_generic_dt_params *params,
> > +                         unsigned int count,
> > +                         unsigned long *cfg,
> > +                         unsigned int *ncfg)
> 
> Should return an error code right? Kerneldoc doesn't hurt either.

I don't see a need for an error return. It's currently not needed and
this refactoring doesn't change that, IMHO. I'll add kerneldoc.

> 
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < count; i++) {
> > +               u32 val;
> > +               int ret;
> > +               const struct pinconf_generic_dt_params *par = &params[i];
> > +
> > +               ret = of_property_read_u32(np, par->property, &val);
> 
> Not checking this return value. Alter the function to return an
> int value on success.

It's checked in the very next statement?! And it's all handled in this
function. No need to report anything to the caller.

> 
> > +
> > +               /* property not found */
> > +               if (ret == -EINVAL)
> > +                       continue;
> > +
> > +               /* use default value, when no value is specified */
> > +               if (ret)
> > +                       val = par->default_value;
> > +
> > +               pr_debug("found %s with value %u\n", par->property, val);
> > +               cfg[*ncfg] = pinconf_to_config_packed(par->param, val);
> > +               (*ncfg)++;
> > +       }
> > +}
> 
> There is something very unintuitive about this loop. You pass two
> counter indexes (count, ncfg) in basically, that is looking weird,
> does it have to look like that? Especially since there is no
> bounds check on ncfg!
> 
> Just use one index in the loop please. Assign  *ncfg = ... after
> the loop has *successfully* iterated.

I think this needs to be as is. There are two arrays @cfg and @params.
@params holding the DT params parsed with @count indicating the
boundary. And @cfg, where the parsed options are put with @ncfg being
a write pointer. @nfcg can be passed non-zero into this function. The
caller is responsible to allocate enough memory to hold all possible entries.

> 
> >  int pinconf_generic_parse_dt_config(struct device_node *np,
> > +                                   struct pinctrl_dev *pctldev,
> >                                     unsigned long **configs,
> >                                     unsigned int *nconfigs)
> 
> This is a good refactoring, but no _foo naming!

Will be renamed.

> 
> >  {
> >         unsigned long *cfg;
> > -       unsigned int ncfg = 0;
> > +       unsigned int max_cfg, ncfg = 0;
> >         int ret;
> > -       int i;
> > -       u32 val;
> >
> >         if (!np)
> >                 return -EINVAL;
> >
> >         /* allocate a temporary array big enough to hold one of each option */
> > -       cfg = kzalloc(sizeof(*cfg) * ARRAY_SIZE(dt_params), GFP_KERNEL);
> > +       max_cfg = ARRAY_SIZE(dt_params);
> > +       if (pctldev)
> > +               max_cfg += pctldev->desc->num_dt_params;
> > +       cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL);
> 
> Aha this looks good...
> 
> > +       _parse_dt_cfg(np, dt_params, ARRAY_SIZE(dt_params), cfg, &ncfg);
> > +       if (pctldev && pctldev->desc->num_dt_params) {
> > +               BUG_ON(!pctldev->desc->params);
> 
> No BUG_ON()

as above.

	Sören

[PATCH 4/7] pinctrl: zynq: Document DT binding

[Soren Brinkmann]

Add documentation for the devicetree binding for the Zynq pincontroller.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 .../bindings/pinctrl/xlnx,zynq-pinctrl.txt         | 90 ++++++++++++++++++++++
 1 file changed, 90 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
new file mode 100644
index 000000000000..86a86b644e6c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
@@ -0,0 +1,90 @@
+	Binding for Xilinx Zynq Pinctrl
+
+Required properties:
+- compatible: "xlnx,zynq-pinctrl"
+- syscon: phandle to SLCR
+- reg: Offset and length of pinctrl space in SLCR
+
+Please refer to pinctrl-bindings.txt in this directory for details of the
+common pinctrl bindings used by client devices, including the meaning of the
+phrase "pin configuration node".
+
+Zynq's pin configuration nodes act as a container for an abitrary number of
+subnodes. Each of these subnodes represents some desired configuration for a
+pin, a group, or a list of pins or groups. This configuration can include the
+mux function to select on those pin(s)/group(s), and various pin configuration
+parameters, such as pull-up, slew rate, etc.
+
+The name of each subnode is not important; all subnodes should be enumerated
+and processed purely based on their content.
+
+Each subnode only affects those parameters that are explicitly listed. In
+other words, a subnode that lists a mux function but no pin configuration
+parameters implies no information about any pin configuration parameters.
+Similarly, a pin subnode that describes a pullup parameter implies no
+information about e.g. the mux function.
+
+
+The following generic properties as defined in pinctrl-bindings.txt are valid
+to specify in a pin configuration subnode:
+ groups, pins, function, bias-disable, bias-high-impedance, bias-pull-up,
+ slew-rate, low-power-disable, low-power-enable
+
+ Valid arguments for 'slew-rate' are '0' and '1' to select between slow and fast
+ respectively.
+
+ Valid values for groups are:
+   ethernet0_0_grp, ethernet1_0_grp, mdio0_0_grp, mdio1_0_grp,
+   qspi0_0_grp, qspi1_0_grp, qspi_fbclk, qspi_cs1_grp, spi0_0_grp,
+   spi0_1_grp - spi0_2_grp, spi1_0_grp - spi1_3_grp, sdio0_0_grp - sdio0_2_grp,
+   sdio1_0_grp - sdio1_3_grp, sdio0_emio_wp, sdio0_emio_cd, sdio1_emio_wp,
+   sdio1_emio_cd, smc0_nor, smc0_nor_cs1_grp, smc0_nor_addr25_grp, smc0_nand,
+   can0_0_grp - can0_10_grp, can1_0_grp - can1_11_grp, uart0_0_grp - uart0_10_grp,
+   uart1_0_grp - uart1_11_grp, i2c0_0_grp - i2c0_10_grp, i2c1_0_grp - i2c1_10_grp,
+   ttc0_0_grp - ttc0_2_grp, ttc1_0_grp - ttc1_2_grp, swdt0_0_grp - swdt0_4_grp,
+   gpio0_0_grp - gpio0_53_grp
+
+ Valid values for pins are:
+   MIO0 - MIO53
+
+ Valid values for function are:
+   ethernet0, ethernet1, mdio0, mdio1, qspi0, qspi1, qspi_fbclk, qspi_cs1,
+   spi0, spi1, sdio0, sdio0_pc, sdio0_cd, sdio0_wp,
+   sdio1, sdio1_pc, sdio1_cd, sdio1_wp,
+   smc0_nor, smc0_nor_cs1, smc0_nor_addr25, smc0_nand, can0, can1, uart0, uart1,
+   i2c0, i2c1, ttc0, ttc1, swdt0, gpio0
+
+The following driver-specific properties as defined here are valid to specify in
+a pin configuration subnode:
+ - io-standard: Configure the pin to use the selected IO standard according to
+   this mapping:
+    1: LVCMOS18
+    2: LVCMOS25
+    3: LVCMOS33
+    4: HSTL
+
+Example:
+	pinctrl0: pinctrl@700 {
+		compatible = "xlnx,pinctrl-zynq";
+		reg = <0x700 0x200>;
+		syscon = <&slcr>;
+
+		pinctrl_uart1_default: pinctrl-uart1-default {
+			common {
+				groups = "uart1_10_grp";
+				function = "uart1";
+				slew-rate = <0>;
+				io-standard = <1>;
+			};
+
+			rx {
+				pins = "MIO49";
+				bias-high-impedance;
+			};
+
+			tx {
+				pins = "MIO48";
+				bias-disable;
+			};
+		};
+	};
-- 
1.9.1

Re: [PATCH 4/7] pinctrl: zynq: Document DT binding

[Andreas Färber]

Am 03.11.2014 um 20:05 schrieb Soren Brinkmann:
> Add documentation for the devicetree binding for the Zynq pincontroller.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
>  .../bindings/pinctrl/xlnx,zynq-pinctrl.txt         | 90 ++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> new file mode 100644
> index 000000000000..86a86b644e6c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> @@ -0,0 +1,90 @@
> +	Binding for Xilinx Zynq Pinctrl
> +
> +Required properties:
> +- compatible: "xlnx,zynq-pinctrl"
> +- syscon: phandle to SLCR
> +- reg: Offset and length of pinctrl space in SLCR
> +
> +Please refer to pinctrl-bindings.txt in this directory for details of the
> +common pinctrl bindings used by client devices, including the meaning of the
> +phrase "pin configuration node".
> +
> +Zynq's pin configuration nodes act as a container for an abitrary number of

"arbitrary"

> +subnodes. Each of these subnodes represents some desired configuration for a
> +pin, a group, or a list of pins or groups. This configuration can include the
> +mux function to select on those pin(s)/group(s), and various pin configuration
> +parameters, such as pull-up, slew rate, etc.
> +
> +The name of each subnode is not important; all subnodes should be enumerated
> +and processed purely based on their content.
> +
> +Each subnode only affects those parameters that are explicitly listed. In
> +other words, a subnode that lists a mux function but no pin configuration
> +parameters implies no information about any pin configuration parameters.
> +Similarly, a pin subnode that describes a pullup parameter implies no
> +information about e.g. the mux function.
> +
> +
> +The following generic properties as defined in pinctrl-bindings.txt are valid
> +to specify in a pin configuration subnode:
> + groups, pins, function, bias-disable, bias-high-impedance, bias-pull-up,
> + slew-rate, low-power-disable, low-power-enable
> +
> + Valid arguments for 'slew-rate' are '0' and '1' to select between slow and fast
> + respectively.
> +
> + Valid values for groups are:
> +   ethernet0_0_grp, ethernet1_0_grp, mdio0_0_grp, mdio1_0_grp,
> +   qspi0_0_grp, qspi1_0_grp, qspi_fbclk, qspi_cs1_grp, spi0_0_grp,
> +   spi0_1_grp - spi0_2_grp, spi1_0_grp - spi1_3_grp, sdio0_0_grp - sdio0_2_grp,
> +   sdio1_0_grp - sdio1_3_grp, sdio0_emio_wp, sdio0_emio_cd, sdio1_emio_wp,
> +   sdio1_emio_cd, smc0_nor, smc0_nor_cs1_grp, smc0_nor_addr25_grp, smc0_nand,
> +   can0_0_grp - can0_10_grp, can1_0_grp - can1_11_grp, uart0_0_grp - uart0_10_grp,
> +   uart1_0_grp - uart1_11_grp, i2c0_0_grp - i2c0_10_grp, i2c1_0_grp - i2c1_10_grp,
> +   ttc0_0_grp - ttc0_2_grp, ttc1_0_grp - ttc1_2_grp, swdt0_0_grp - swdt0_4_grp,
> +   gpio0_0_grp - gpio0_53_grp
> +
> + Valid values for pins are:
> +   MIO0 - MIO53

What about the four EMIO_SD* pins?

> +
> + Valid values for function are:
> +   ethernet0, ethernet1, mdio0, mdio1, qspi0, qspi1, qspi_fbclk, qspi_cs1,
> +   spi0, spi1, sdio0, sdio0_pc, sdio0_cd, sdio0_wp,
> +   sdio1, sdio1_pc, sdio1_cd, sdio1_wp,
> +   smc0_nor, smc0_nor_cs1, smc0_nor_addr25, smc0_nand, can0, can1, uart0, uart1,
> +   i2c0, i2c1, ttc0, ttc1, swdt0, gpio0
> +
> +The following driver-specific properties as defined here are valid to specify in
> +a pin configuration subnode:
> + - io-standard: Configure the pin to use the selected IO standard according to
> +   this mapping:
> +    1: LVCMOS18
> +    2: LVCMOS25
> +    3: LVCMOS33
> +    4: HSTL
> +
> +Example:
> +	pinctrl0: pinctrl@700 {
> +		compatible = "xlnx,pinctrl-zynq";
> +		reg = <0x700 0x200>;
> +		syscon = <&slcr>;
> +
> +		pinctrl_uart1_default: pinctrl-uart1-default {

Nit: Is it really necessary to prefix subnodes of "pinctrl@700" with
"pinctrl-", here and in .dts?

> +			common {
> +				groups = "uart1_10_grp";
> +				function = "uart1";
> +				slew-rate = <0>;
> +				io-standard = <1>;
> +			};
> +
> +			rx {
> +				pins = "MIO49";
> +				bias-high-impedance;
> +			};
> +
> +			tx {
> +				pins = "MIO48";
> +				bias-disable;
> +			};
> +		};
> +	};

Otherwise looks good.

Cheers,
Andreas

-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg

Re: [PATCH 4/7] pinctrl: zynq: Document DT binding

[Sören Brinkmann]

On Wed, 2014-11-05 at 04:35AM +0100, Andreas Färber wrote:
> Am 03.11.2014 um 20:05 schrieb Soren Brinkmann:
> > Add documentation for the devicetree binding for the Zynq pincontroller.
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > ---
> >  .../bindings/pinctrl/xlnx,zynq-pinctrl.txt         | 90 ++++++++++++++++++++++
> >  1 file changed, 90 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> > new file mode 100644
> > index 000000000000..86a86b644e6c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> > @@ -0,0 +1,90 @@
> > +	Binding for Xilinx Zynq Pinctrl
> > +
> > +Required properties:
> > +- compatible: "xlnx,zynq-pinctrl"
> > +- syscon: phandle to SLCR
> > +- reg: Offset and length of pinctrl space in SLCR
> > +
> > +Please refer to pinctrl-bindings.txt in this directory for details of the
> > +common pinctrl bindings used by client devices, including the meaning of the
> > +phrase "pin configuration node".
> > +
> > +Zynq's pin configuration nodes act as a container for an abitrary number of
> 
> "arbitrary"

I'll fix that.

> 
> > +subnodes. Each of these subnodes represents some desired configuration for a
> > +pin, a group, or a list of pins or groups. This configuration can include the
> > +mux function to select on those pin(s)/group(s), and various pin configuration
> > +parameters, such as pull-up, slew rate, etc.
> > +
> > +The name of each subnode is not important; all subnodes should be enumerated
> > +and processed purely based on their content.
> > +
> > +Each subnode only affects those parameters that are explicitly listed. In
> > +other words, a subnode that lists a mux function but no pin configuration
> > +parameters implies no information about any pin configuration parameters.
> > +Similarly, a pin subnode that describes a pullup parameter implies no
> > +information about e.g. the mux function.
> > +
> > +
> > +The following generic properties as defined in pinctrl-bindings.txt are valid
> > +to specify in a pin configuration subnode:
> > + groups, pins, function, bias-disable, bias-high-impedance, bias-pull-up,
> > + slew-rate, low-power-disable, low-power-enable
> > +
> > + Valid arguments for 'slew-rate' are '0' and '1' to select between slow and fast
> > + respectively.
> > +
> > + Valid values for groups are:
> > +   ethernet0_0_grp, ethernet1_0_grp, mdio0_0_grp, mdio1_0_grp,
> > +   qspi0_0_grp, qspi1_0_grp, qspi_fbclk, qspi_cs1_grp, spi0_0_grp,
> > +   spi0_1_grp - spi0_2_grp, spi1_0_grp - spi1_3_grp, sdio0_0_grp - sdio0_2_grp,
> > +   sdio1_0_grp - sdio1_3_grp, sdio0_emio_wp, sdio0_emio_cd, sdio1_emio_wp,
> > +   sdio1_emio_cd, smc0_nor, smc0_nor_cs1_grp, smc0_nor_addr25_grp, smc0_nand,
> > +   can0_0_grp - can0_10_grp, can1_0_grp - can1_11_grp, uart0_0_grp - uart0_10_grp,
> > +   uart1_0_grp - uart1_11_grp, i2c0_0_grp - i2c0_10_grp, i2c1_0_grp - i2c1_10_grp,
> > +   ttc0_0_grp - ttc0_2_grp, ttc1_0_grp - ttc1_2_grp, swdt0_0_grp - swdt0_4_grp,
> > +   gpio0_0_grp - gpio0_53_grp
> > +
> > + Valid values for pins are:
> > +   MIO0 - MIO53
> 
> What about the four EMIO_SD* pins?

You can't do any pinconf on those and the config_(set|get) return some
error code when you pass anything but the 54 MIO pins to those
functions. So, they are actually not valid for PINs. They are only used
as group for pinmuxing where the respective group needs to be used.

> 
> > +
> > + Valid values for function are:
> > +   ethernet0, ethernet1, mdio0, mdio1, qspi0, qspi1, qspi_fbclk, qspi_cs1,
> > +   spi0, spi1, sdio0, sdio0_pc, sdio0_cd, sdio0_wp,
> > +   sdio1, sdio1_pc, sdio1_cd, sdio1_wp,
> > +   smc0_nor, smc0_nor_cs1, smc0_nor_addr25, smc0_nand, can0, can1, uart0, uart1,
> > +   i2c0, i2c1, ttc0, ttc1, swdt0, gpio0
> > +
> > +The following driver-specific properties as defined here are valid to specify in
> > +a pin configuration subnode:
> > + - io-standard: Configure the pin to use the selected IO standard according to
> > +   this mapping:
> > +    1: LVCMOS18
> > +    2: LVCMOS25
> > +    3: LVCMOS33
> > +    4: HSTL
> > +
> > +Example:
> > +	pinctrl0: pinctrl@700 {
> > +		compatible = "xlnx,pinctrl-zynq";
> > +		reg = <0x700 0x200>;
> > +		syscon = <&slcr>;
> > +
> > +		pinctrl_uart1_default: pinctrl-uart1-default {
> 
> Nit: Is it really necessary to prefix subnodes of "pinctrl@700" with
> "pinctrl-", here and in .dts?

Guess not. Is there any best practice for naming pinctrl-state nodes?
I might shorten it.

> 
> > +			common {
> > +				groups = "uart1_10_grp";
> > +				function = "uart1";
> > +				slew-rate = <0>;
> > +				io-standard = <1>;
> > +			};
> > +
> > +			rx {
> > +				pins = "MIO49";
> > +				bias-high-impedance;
> > +			};
> > +
> > +			tx {
> > +				pins = "MIO48";
> > +				bias-disable;
> > +			};
> > +		};
> > +	};
> 
> Otherwise looks good.

Thanks for reviewing.

	Sören

Re: [PATCH 4/7] pinctrl: zynq: Document DT binding

[Linus Walleij]

On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
<soren.brinkmann@xilinx.com> wrote:

> Add documentation for the devicetree binding for the Zynq pincontroller.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
(...)
> +Example:
> +       pinctrl0: pinctrl@700 {
> +               compatible = "xlnx,pinctrl-zynq";
> +               reg = <0x700 0x200>;
> +               syscon = <&slcr>;
> +
> +               pinctrl_uart1_default: pinctrl-uart1-default {
> +                       common {
> +                               groups = "uart1_10_grp";
> +                               function = "uart1";
> +                               slew-rate = <0>;
> +                               io-standard = <1>;
> +                       };

I don't really like that you mix multiplexing and config in the
same node. I would prefer if the generic bindings say we have
muxing nodes and config nodes, and those are disparate.

Can't you just split this:

common-mux {
    groups = "uart1_10_grp";
    function = "uart1";
};

common-config {
    groups = "uart1_10_grp";
    slew-rate = <0>;
    io-standard = <1>;
};

That way we can identify nodes as mux nodes (have "function")
or config nodes (have "groups" or "pins" but not "function") which
I think makes things easier to read.

Yours,
Linus Walleij

Re: [PATCH 4/7] pinctrl: zynq: Document DT binding

[Sören Brinkmann]

On Tue, 2014-11-11 at 04:00PM +0100, Linus Walleij wrote:
> On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> 
> > Add documentation for the devicetree binding for the Zynq pincontroller.
> >
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> (...)
> > +Example:
> > +       pinctrl0: pinctrl@700 {
> > +               compatible = "xlnx,pinctrl-zynq";
> > +               reg = <0x700 0x200>;
> > +               syscon = <&slcr>;
> > +
> > +               pinctrl_uart1_default: pinctrl-uart1-default {
> > +                       common {
> > +                               groups = "uart1_10_grp";
> > +                               function = "uart1";
> > +                               slew-rate = <0>;
> > +                               io-standard = <1>;
> > +                       };
> 
> I don't really like that you mix multiplexing and config in the
> same node. I would prefer if the generic bindings say we have
> muxing nodes and config nodes, and those are disparate.
> 
> Can't you just split this:
> 
> common-mux {
>     groups = "uart1_10_grp";
>     function = "uart1";
> };
> 
> common-config {
>     groups = "uart1_10_grp";
>     slew-rate = <0>;
>     io-standard = <1>;
> };
> 
> That way we can identify nodes as mux nodes (have "function")
> or config nodes (have "groups" or "pins" but not "function") which
> I think makes things easier to read.

I think such separation is not required by the bindings currently and
the parser assumes everything can be present in any node.
Can we add that requirement to the generic bindings without breaking
current users? I think it would make the implementation a little easier.
	
	Thanks,
	Sören

Re: [PATCH 4/7] pinctrl: zynq: Document DT binding

[Linus Walleij]

On Wed, Nov 12, 2014 at 7:53 PM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Tue, 2014-11-11 at 04:00PM +0100, Linus Walleij wrote:

>> I don't really like that you mix multiplexing and config in the
>> same node. I would prefer if the generic bindings say we have
>> muxing nodes and config nodes, and those are disparate.
>>
>> Can't you just split this:
>>
>> common-mux {
>>     groups = "uart1_10_grp";
>>     function = "uart1";
>> };
>>
>> common-config {
>>     groups = "uart1_10_grp";
>>     slew-rate = <0>;
>>     io-standard = <1>;
>> };
>>
>> That way we can identify nodes as mux nodes (have "function")
>> or config nodes (have "groups" or "pins" but not "function") which
>> I think makes things easier to read.
>
> I think such separation is not required by the bindings currently and
> the parser assumes everything can be present in any node.

The bindings say:

== Generic pin multiplexing node content ==

pin multiplexing nodes:

function                - the mux function to select
groups                  - the list of groups to select with this function

Example:

state_0_node_a {
        function = "uart0";
        groups = "u0rxtx", "u0rtscts";
};
state_1_node_a {
        function = "spi0";
        groups = "spi0pins";
};


== Generic pin configuration node content ==

(...)
Supported generic properties are:

pins                    - the list of pins that properties in the node
                          apply to (either this or "group" has to be
                          specified)
group                   - the group to apply the properties to, if the driver
                          supports configuration of whole groups rather than
                          individual pins (either this or "pins" has to be
                          specified)

It is not explicit that they have to be separate nodes but if needed
we can state that more clearly.

> Can we add that requirement to the generic bindings without breaking
> current users? I think it would make the implementation a little easier.

I think so.

Yours,
Linus Walleij

[PATCH 5/7] pinctrl: Add driver for Zynq

[Soren Brinkmann]

This adds a pin-control driver for Zynq.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
changes since RFCv2:
 - let Zynq select PINCTRL_ZYNQ. Boot hangs when pinctrl information is
   present in DT but no driver available.
 - add #defines to get rid of magical constants
 - add commas at end of initializers
 - separate changes in mach-zynq in separate patch
 - add driver specific io-standard DT property
 - refactored pinconf set function to not require arguments for
   argument-less properties
 - squash other patches in
   - support for IO-standard property
   - support for low-power mode property
   - migration to pinconf_generic_dt_node_to_map_all()
 - use newly created infrastructure to add pass driver-specific DT
   params to pinconf-generic

changes since RFC:
 - use syscon/regmap to access registers in SLCR space
 - rebase to 3.18: rename enable -> set_mux
 - add kernel-doc
 - support pinconf
   - supported attributes
     - pin-bias: pull up, tristate, disable
     - slew-rate: 0 == slow, 1 == fast; generic pinconf does not display
       argument
---
 drivers/pinctrl/Kconfig        |    8 +
 drivers/pinctrl/Makefile       |    1 +
 drivers/pinctrl/pinctrl-zynq.c | 1164 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1173 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-zynq.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index c6a66de6ed72..33cbae0a7f6f 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -203,6 +203,14 @@ config PINCTRL_PALMAS
 	  open drain configuration for the Palmas series devices like
 	  TPS65913, TPS80036 etc.
 
+config PINCTRL_ZYNQ
+	bool "Pinctrl driver for Xilinx Zynq"
+	depends on ARCH_ZYNQ
+	select PINMUX
+	select GENERIC_PINCONF
+	help
+	  This selectes the pinctrl driver for Xilinx Zynq.
+
 source "drivers/pinctrl/berlin/Kconfig"
 source "drivers/pinctrl/freescale/Kconfig"
 source "drivers/pinctrl/mvebu/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 51f52d32859e..aa999cf57f04 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_PINCTRL_XWAY)	+= pinctrl-xway.o
 obj-$(CONFIG_PINCTRL_LANTIQ)	+= pinctrl-lantiq.o
 obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
 obj-$(CONFIG_PINCTRL_ST) 	+= pinctrl-st.o
+obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 
 obj-$(CONFIG_ARCH_BERLIN)	+= berlin/
 obj-y				+= freescale/
diff --git a/drivers/pinctrl/pinctrl-zynq.c b/drivers/pinctrl/pinctrl-zynq.c
new file mode 100644
index 000000000000..9359e9ff157d
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-zynq.c
@@ -0,0 +1,1164 @@
+/*
+ * Zynq pin controller
+ *
+ *  Copyright (C) 2014 Xilinx
+ *
+ *  Sören Brinkmann <soren.brinkmann@xilinx.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/regmap.h>
+#include "pinctrl-utils.h"
+#include "core.h"
+
+#define ZYNQ_NUM_MIOS	54
+
+#define ZYNQ_PCTRL_MIO_MST_TRI0	0x10c
+#define ZYNQ_PCTRL_MIO_MST_TRI1	0x110
+
+#define ZYNQ_PINMUX_MUX_SHIFT	1
+#define ZYNQ_PINMUX_MUX_MASK	(0x7f << ZYNQ_PINMUX_MUX_SHIFT)
+
+/**
+ * struct zynq_pinctrl - driver data
+ * @pctrl:		Pinctrl device
+ * @syscon:		Syscon regmap
+ * @pctrl_offset:	Offset for pinctrl into the @syscon space
+ * @groups:		Pingroups
+ * @ngroupos:		Number of @groups
+ * @funcs:		Pinmux functions
+ * @nfuncs:		Number of @funcs
+ */
+struct zynq_pinctrl {
+	struct pinctrl_dev *pctrl;
+	struct regmap *syscon;
+	u32 pctrl_offset;
+	const struct zynq_pctrl_group *groups;
+	unsigned int ngroups;
+	const struct zynq_pinmux_function *funcs;
+	unsigned int nfuncs;
+};
+
+struct zynq_pctrl_group {
+	const char *name;
+	const unsigned int *pins;
+	const unsigned npins;
+};
+
+/**
+ * struct zynq_pinmux_function - a pinmux function
+ * @name:	Name of the pinmux function.
+ * @groups:	List of pingroups for this function.
+ * @ngroups:	Number of entries in @groups.
+ * @mux_val:	Selector for this function
+ * @mux:	Offset of function specific mux
+ * @mux_mask:	Mask for function specific selector
+ * @mux_shift:	Shift for function specific selector
+ */
+struct zynq_pinmux_function {
+	const char *name;
+	const char * const *groups;
+	unsigned int ngroups;
+	unsigned int mux_val;
+	u32 mux;
+	u32 mux_mask;
+	u8 mux_shift;
+};
+
+enum zynq_pinmux_functions {
+	ZYNQ_PMUX_ethernet0,
+	ZYNQ_PMUX_ethernet1,
+	ZYNQ_PMUX_mdio0,
+	ZYNQ_PMUX_mdio1,
+	ZYNQ_PMUX_qspi0,
+	ZYNQ_PMUX_qspi1,
+	ZYNQ_PMUX_qspi_fbclk,
+	ZYNQ_PMUX_qspi_cs1,
+	ZYNQ_PMUX_spi0,
+	ZYNQ_PMUX_spi1,
+	ZYNQ_PMUX_sdio0,
+	ZYNQ_PMUX_sdio0_pc,
+	ZYNQ_PMUX_sdio0_cd,
+	ZYNQ_PMUX_sdio0_wp,
+	ZYNQ_PMUX_sdio1,
+	ZYNQ_PMUX_sdio1_pc,
+	ZYNQ_PMUX_sdio1_cd,
+	ZYNQ_PMUX_sdio1_wp,
+	ZYNQ_PMUX_smc0_nor,
+	ZYNQ_PMUX_smc0_nor_cs1,
+	ZYNQ_PMUX_smc0_nor_addr25,
+	ZYNQ_PMUX_smc0_nand,
+	ZYNQ_PMUX_can0,
+	ZYNQ_PMUX_can1,
+	ZYNQ_PMUX_uart0,
+	ZYNQ_PMUX_uart1,
+	ZYNQ_PMUX_i2c0,
+	ZYNQ_PMUX_i2c1,
+	ZYNQ_PMUX_ttc0,
+	ZYNQ_PMUX_ttc1,
+	ZYNQ_PMUX_swdt0,
+	ZYNQ_PMUX_gpio0,
+	ZYNQ_PMUX_MAX_FUNC
+};
+
+const struct pinctrl_pin_desc zynq_pins[] = {
+	PINCTRL_PIN(0,  "MIO0"),
+	PINCTRL_PIN(1,  "MIO1"),
+	PINCTRL_PIN(2,  "MIO2"),
+	PINCTRL_PIN(3,  "MIO3"),
+	PINCTRL_PIN(4,  "MIO4"),
+	PINCTRL_PIN(5,  "MIO5"),
+	PINCTRL_PIN(6,  "MIO6"),
+	PINCTRL_PIN(7,  "MIO7"),
+	PINCTRL_PIN(8,  "MIO8"),
+	PINCTRL_PIN(9,  "MIO9"),
+	PINCTRL_PIN(10, "MIO10"),
+	PINCTRL_PIN(11, "MIO11"),
+	PINCTRL_PIN(12, "MIO12"),
+	PINCTRL_PIN(13, "MIO13"),
+	PINCTRL_PIN(14, "MIO14"),
+	PINCTRL_PIN(15, "MIO15"),
+	PINCTRL_PIN(16, "MIO16"),
+	PINCTRL_PIN(17, "MIO17"),
+	PINCTRL_PIN(18, "MIO18"),
+	PINCTRL_PIN(19, "MIO19"),
+	PINCTRL_PIN(20, "MIO20"),
+	PINCTRL_PIN(21, "MIO21"),
+	PINCTRL_PIN(22, "MIO22"),
+	PINCTRL_PIN(23, "MIO23"),
+	PINCTRL_PIN(24, "MIO24"),
+	PINCTRL_PIN(25, "MIO25"),
+	PINCTRL_PIN(26, "MIO26"),
+	PINCTRL_PIN(27, "MIO27"),
+	PINCTRL_PIN(28, "MIO28"),
+	PINCTRL_PIN(29, "MIO29"),
+	PINCTRL_PIN(30, "MIO30"),
+	PINCTRL_PIN(31, "MIO31"),
+	PINCTRL_PIN(32, "MIO32"),
+	PINCTRL_PIN(33, "MIO33"),
+	PINCTRL_PIN(34, "MIO34"),
+	PINCTRL_PIN(35, "MIO35"),
+	PINCTRL_PIN(36, "MIO36"),
+	PINCTRL_PIN(37, "MIO37"),
+	PINCTRL_PIN(38, "MIO38"),
+	PINCTRL_PIN(39, "MIO39"),
+	PINCTRL_PIN(40, "MIO40"),
+	PINCTRL_PIN(41, "MIO41"),
+	PINCTRL_PIN(42, "MIO42"),
+	PINCTRL_PIN(43, "MIO43"),
+	PINCTRL_PIN(44, "MIO44"),
+	PINCTRL_PIN(45, "MIO45"),
+	PINCTRL_PIN(46, "MIO46"),
+	PINCTRL_PIN(47, "MIO47"),
+	PINCTRL_PIN(48, "MIO48"),
+	PINCTRL_PIN(49, "MIO49"),
+	PINCTRL_PIN(50, "MIO50"),
+	PINCTRL_PIN(51, "MIO51"),
+	PINCTRL_PIN(52, "MIO52"),
+	PINCTRL_PIN(53, "MIO53"),
+	PINCTRL_PIN(54, "EMIO_SD0_WP"),
+	PINCTRL_PIN(55, "EMIO_SD0_CD"),
+	PINCTRL_PIN(56, "EMIO_SD1_WP"),
+	PINCTRL_PIN(57, "EMIO_SD0_CD"),
+};
+
+/* pin groups */
+static const unsigned int ethernet0_0_pins[] = {16, 17, 18, 19, 20, 21, 22, 23,
+						24, 25, 26, 27};
+static const unsigned int ethernet1_0_pins[] = {28, 29, 30, 31, 32, 33, 34, 35,
+						36, 37, 38, 39};
+static const unsigned int mdio0_0_pins[] = {52, 53};
+static const unsigned int mdio1_0_pins[] = {52, 53};
+static const unsigned int qspi0_0_pins[] = {1, 2, 3, 4, 5, 6};
+
+static const unsigned int qspi1_0_pins[] = {9, 10, 11, 12, 13};
+static const unsigned int qspi_cs1_pins[] = {0};
+static const unsigned int qspi_fbclk_pins[] = {8};
+static const unsigned int spi0_0_pins[] = {16, 17, 18, 19, 20, 21};
+static const unsigned int spi0_1_pins[] = {28, 29, 30, 31, 32, 33};
+static const unsigned int spi0_2_pins[] = {40, 41, 42, 43, 44, 45};
+static const unsigned int spi1_0_pins[] = {10, 11, 12, 13, 14, 15};
+static const unsigned int spi1_1_pins[] = {22, 23, 24, 25, 26, 27};
+static const unsigned int spi1_2_pins[] = {34, 35, 36, 37, 38, 39};
+static const unsigned int spi1_3_pins[] = {46, 47, 48, 49, 40, 51};
+static const unsigned int sdio0_0_pins[] = {16, 17, 18, 19, 20, 21};
+static const unsigned int sdio0_1_pins[] = {28, 29, 30, 31, 32, 33};
+static const unsigned int sdio0_2_pins[] = {40, 41, 42, 43, 44, 45};
+static const unsigned int sdio1_0_pins[] = {10, 11, 12, 13, 14, 15};
+static const unsigned int sdio1_1_pins[] = {22, 23, 24, 25, 26, 27};
+static const unsigned int sdio1_2_pins[] = {34, 35, 36, 37, 38, 39};
+static const unsigned int sdio1_3_pins[] = {46, 47, 48, 49, 40, 51};
+static const unsigned int sdio0_emio_wp_pins[] = {54};
+static const unsigned int sdio0_emio_cd_pins[] = {55};
+static const unsigned int sdio1_emio_wp_pins[] = {56};
+static const unsigned int sdio1_emio_cd_pins[] = {57};
+static const unsigned int smc0_nor_pins[] = {0, 3, 4, 5, 6, 7, 8, 9, 10, 11, 13,
+					     15, 16, 17, 18, 19, 20, 21, 22, 23,
+					     24, 25, 26, 27, 28, 29, 30, 31, 32,
+					     33, 34, 35, 36, 37, 38, 39};
+static const unsigned int smc0_nor_cs1_pins[] = {1};
+static const unsigned int smc0_nor_addr25_pins[] = {1};
+static const unsigned int smc0_nand_pins[] = {0, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11,
+					      12, 13, 14, 16, 17, 18, 19, 20,
+					      21, 22, 23};
+/* Note: CAN MIO clock inputs are modeled in the clock framework */
+static const unsigned int can0_0_pins[] = {10, 11};
+static const unsigned int can0_1_pins[] = {14, 15};
+static const unsigned int can0_2_pins[] = {18, 19};
+static const unsigned int can0_3_pins[] = {22, 23};
+static const unsigned int can0_4_pins[] = {26, 27};
+static const unsigned int can0_5_pins[] = {30, 31};
+static const unsigned int can0_6_pins[] = {34, 35};
+static const unsigned int can0_7_pins[] = {38, 39};
+static const unsigned int can0_8_pins[] = {42, 43};
+static const unsigned int can0_9_pins[] = {46, 47};
+static const unsigned int can0_10_pins[] = {50, 51};
+static const unsigned int can1_0_pins[] = {8, 9};
+static const unsigned int can1_1_pins[] = {12, 13};
+static const unsigned int can1_2_pins[] = {16, 17};
+static const unsigned int can1_3_pins[] = {20, 21};
+static const unsigned int can1_4_pins[] = {24, 25};
+static const unsigned int can1_5_pins[] = {28, 29};
+static const unsigned int can1_6_pins[] = {32, 33};
+static const unsigned int can1_7_pins[] = {36, 37};
+static const unsigned int can1_8_pins[] = {40, 41};
+static const unsigned int can1_9_pins[] = {44, 45};
+static const unsigned int can1_10_pins[] = {48, 49};
+static const unsigned int can1_11_pins[] = {52, 53};
+static const unsigned int uart0_0_pins[] = {10, 11};
+static const unsigned int uart0_1_pins[] = {14, 15};
+static const unsigned int uart0_2_pins[] = {18, 19};
+static const unsigned int uart0_3_pins[] = {22, 23};
+static const unsigned int uart0_4_pins[] = {26, 27};
+static const unsigned int uart0_5_pins[] = {30, 31};
+static const unsigned int uart0_6_pins[] = {34, 35};
+static const unsigned int uart0_7_pins[] = {38, 39};
+static const unsigned int uart0_8_pins[] = {42, 43};
+static const unsigned int uart0_9_pins[] = {46, 47};
+static const unsigned int uart0_10_pins[] = {50, 51};
+static const unsigned int uart1_0_pins[] = {8, 9};
+static const unsigned int uart1_1_pins[] = {12, 13};
+static const unsigned int uart1_2_pins[] = {16, 17};
+static const unsigned int uart1_3_pins[] = {20, 21};
+static const unsigned int uart1_4_pins[] = {24, 25};
+static const unsigned int uart1_5_pins[] = {28, 29};
+static const unsigned int uart1_6_pins[] = {32, 33};
+static const unsigned int uart1_7_pins[] = {36, 37};
+static const unsigned int uart1_8_pins[] = {40, 41};
+static const unsigned int uart1_9_pins[] = {44, 45};
+static const unsigned int uart1_10_pins[] = {48, 49};
+static const unsigned int uart1_11_pins[] = {52, 53};
+static const unsigned int i2c0_0_pins[] = {10, 11};
+static const unsigned int i2c0_1_pins[] = {14, 15};
+static const unsigned int i2c0_2_pins[] = {18, 19};
+static const unsigned int i2c0_3_pins[] = {22, 23};
+static const unsigned int i2c0_4_pins[] = {26, 27};
+static const unsigned int i2c0_5_pins[] = {30, 31};
+static const unsigned int i2c0_6_pins[] = {34, 35};
+static const unsigned int i2c0_7_pins[] = {38, 39};
+static const unsigned int i2c0_8_pins[] = {42, 43};
+static const unsigned int i2c0_9_pins[] = {46, 47};
+static const unsigned int i2c0_10_pins[] = {50, 51};
+static const unsigned int i2c1_0_pins[] = {12, 13};
+static const unsigned int i2c1_1_pins[] = {16, 17};
+static const unsigned int i2c1_2_pins[] = {20, 21};
+static const unsigned int i2c1_3_pins[] = {24, 25};
+static const unsigned int i2c1_4_pins[] = {28, 29};
+static const unsigned int i2c1_5_pins[] = {32, 33};
+static const unsigned int i2c1_6_pins[] = {36, 37};
+static const unsigned int i2c1_7_pins[] = {40, 41};
+static const unsigned int i2c1_8_pins[] = {44, 45};
+static const unsigned int i2c1_9_pins[] = {48, 49};
+static const unsigned int i2c1_10_pins[] = {52, 53};
+static const unsigned int ttc0_0_pins[] = {18, 19};
+static const unsigned int ttc0_1_pins[] = {30, 31};
+static const unsigned int ttc0_2_pins[] = {42, 43};
+static const unsigned int ttc1_0_pins[] = {16, 17};
+static const unsigned int ttc1_1_pins[] = {28, 29};
+static const unsigned int ttc1_2_pins[] = {40, 41};
+static const unsigned int swdt0_0_pins[] = {14, 15};
+static const unsigned int swdt0_1_pins[] = {26, 27};
+static const unsigned int swdt0_2_pins[] = {38, 39};
+static const unsigned int swdt0_3_pins[] = {50, 51};
+static const unsigned int swdt0_4_pins[] = {52, 53};
+static const unsigned int gpio0_0_pins[] = {0};
+static const unsigned int gpio0_1_pins[] = {1};
+static const unsigned int gpio0_2_pins[] = {2};
+static const unsigned int gpio0_3_pins[] = {3};
+static const unsigned int gpio0_4_pins[] = {4};
+static const unsigned int gpio0_5_pins[] = {5};
+static const unsigned int gpio0_6_pins[] = {6};
+static const unsigned int gpio0_7_pins[] = {7};
+static const unsigned int gpio0_8_pins[] = {8};
+static const unsigned int gpio0_9_pins[] = {9};
+static const unsigned int gpio0_10_pins[] = {10};
+static const unsigned int gpio0_11_pins[] = {11};
+static const unsigned int gpio0_12_pins[] = {12};
+static const unsigned int gpio0_13_pins[] = {13};
+static const unsigned int gpio0_14_pins[] = {14};
+static const unsigned int gpio0_15_pins[] = {15};
+static const unsigned int gpio0_16_pins[] = {16};
+static const unsigned int gpio0_17_pins[] = {17};
+static const unsigned int gpio0_18_pins[] = {18};
+static const unsigned int gpio0_19_pins[] = {19};
+static const unsigned int gpio0_20_pins[] = {20};
+static const unsigned int gpio0_21_pins[] = {21};
+static const unsigned int gpio0_22_pins[] = {22};
+static const unsigned int gpio0_23_pins[] = {23};
+static const unsigned int gpio0_24_pins[] = {24};
+static const unsigned int gpio0_25_pins[] = {25};
+static const unsigned int gpio0_26_pins[] = {26};
+static const unsigned int gpio0_27_pins[] = {27};
+static const unsigned int gpio0_28_pins[] = {28};
+static const unsigned int gpio0_29_pins[] = {29};
+static const unsigned int gpio0_30_pins[] = {30};
+static const unsigned int gpio0_31_pins[] = {31};
+static const unsigned int gpio0_32_pins[] = {32};
+static const unsigned int gpio0_33_pins[] = {33};
+static const unsigned int gpio0_34_pins[] = {34};
+static const unsigned int gpio0_35_pins[] = {35};
+static const unsigned int gpio0_36_pins[] = {36};
+static const unsigned int gpio0_37_pins[] = {37};
+static const unsigned int gpio0_38_pins[] = {38};
+static const unsigned int gpio0_39_pins[] = {39};
+static const unsigned int gpio0_40_pins[] = {40};
+static const unsigned int gpio0_41_pins[] = {41};
+static const unsigned int gpio0_42_pins[] = {42};
+static const unsigned int gpio0_43_pins[] = {43};
+static const unsigned int gpio0_44_pins[] = {44};
+static const unsigned int gpio0_45_pins[] = {45};
+static const unsigned int gpio0_46_pins[] = {46};
+static const unsigned int gpio0_47_pins[] = {47};
+static const unsigned int gpio0_48_pins[] = {48};
+static const unsigned int gpio0_49_pins[] = {49};
+static const unsigned int gpio0_50_pins[] = {50};
+static const unsigned int gpio0_51_pins[] = {51};
+static const unsigned int gpio0_52_pins[] = {52};
+static const unsigned int gpio0_53_pins[] = {53};
+
+#define DEFINE_ZYNQ_PINCTRL_GRP(nm) \
+	{ \
+		.name = #nm "_grp", \
+		.pins = nm ## _pins, \
+		.npins = ARRAY_SIZE(nm ## _pins), \
+	}
+
+struct zynq_pctrl_group zynq_pctrl_groups[] = {
+	DEFINE_ZYNQ_PINCTRL_GRP(ethernet0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(ethernet1_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(mdio0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(mdio1_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(qspi0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(qspi1_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(qspi_fbclk),
+	DEFINE_ZYNQ_PINCTRL_GRP(qspi_cs1),
+	DEFINE_ZYNQ_PINCTRL_GRP(spi0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(spi0_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(spi0_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(spi1_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(spi1_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(spi1_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(spi1_3),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio0_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio0_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio1_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio1_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio1_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio1_3),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio0_emio_wp),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio0_emio_cd),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio1_emio_wp),
+	DEFINE_ZYNQ_PINCTRL_GRP(sdio1_emio_cd),
+	DEFINE_ZYNQ_PINCTRL_GRP(smc0_nor),
+	DEFINE_ZYNQ_PINCTRL_GRP(smc0_nor_cs1),
+	DEFINE_ZYNQ_PINCTRL_GRP(smc0_nor_addr25),
+	DEFINE_ZYNQ_PINCTRL_GRP(smc0_nand),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_3),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_4),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_5),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_6),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_7),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_8),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_9),
+	DEFINE_ZYNQ_PINCTRL_GRP(can0_10),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_3),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_4),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_5),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_6),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_7),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_8),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_9),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_10),
+	DEFINE_ZYNQ_PINCTRL_GRP(can1_11),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_3),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_4),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_5),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_6),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_7),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_8),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_9),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart0_10),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_3),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_4),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_5),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_6),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_7),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_8),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_9),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_10),
+	DEFINE_ZYNQ_PINCTRL_GRP(uart1_11),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_3),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_4),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_5),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_6),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_7),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_8),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_9),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c0_10),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_3),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_4),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_5),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_6),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_7),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_8),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_9),
+	DEFINE_ZYNQ_PINCTRL_GRP(i2c1_10),
+	DEFINE_ZYNQ_PINCTRL_GRP(ttc0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(ttc0_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(ttc0_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(ttc1_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(ttc1_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(ttc1_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(swdt0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(swdt0_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(swdt0_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(swdt0_3),
+	DEFINE_ZYNQ_PINCTRL_GRP(swdt0_4),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_0),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_1),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_2),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_3),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_4),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_5),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_6),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_7),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_8),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_9),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_10),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_11),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_12),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_13),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_14),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_15),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_16),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_17),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_18),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_19),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_20),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_21),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_22),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_23),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_24),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_25),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_26),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_27),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_28),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_29),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_30),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_31),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_32),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_33),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_34),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_35),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_36),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_37),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_38),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_39),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_40),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_41),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_42),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_43),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_44),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_45),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_46),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_47),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_48),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_49),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_50),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_51),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_52),
+	DEFINE_ZYNQ_PINCTRL_GRP(gpio0_53),
+};
+
+/* function groups */
+static const char * const ethernet0_groups[] = {"ethernet0_0_grp"};
+static const char * const ethernet1_groups[] = {"ethernet1_0_grp"};
+static const char * const mdio0_groups[] = {"mdio0_0_grp"};
+static const char * const mdio1_groups[] = {"mdio1_0_grp"};
+static const char * const qspi0_groups[] = {"qspi0_0_grp"};
+static const char * const qspi1_groups[] = {"qspi0_1_grp"};
+static const char * const qspi_fbclk_groups[] = {"qspi_fbclk_grp"};
+static const char * const qspi_cs1_groups[] = {"qspi_cs1_grp"};
+static const char * const spi0_groups[] = {"spi0_0_grp", "spi0_1_grp",
+					   "spi0_2_grp"};
+static const char * const spi1_groups[] = {"spi1_0_grp", "spi1_1_grp",
+					   "spi1_2_grp", "spi1_3_grp"};
+static const char * const sdio0_groups[] = {"sdio0_0_grp", "sdio0_1_grp",
+					    "sdio0_2_grp"};
+static const char * const sdio1_groups[] = {"sdio1_0_grp", "sdio1_1_grp",
+					    "sdio1_2_grp", "sdio1_3_grp"};
+static const char * const sdio0_pc_groups[] = {"gpio0_0_grp",
+		"gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
+		"gpio0_8_grp", "gpio0_10_grp", "gpio0_12_grp",
+		"gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
+		"gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
+		"gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
+		"gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
+		"gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
+		"gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
+		"gpio0_50_grp", "gpio0_52_grp"};
+static const char * const sdio1_pc_groups[] = {"gpio0_1_grp",
+		"gpio0_3_grp", "gpio0_5_grp", "gpio0_7_grp",
+		"gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
+		"gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
+		"gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
+		"gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
+		"gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
+		"gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
+		"gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
+		"gpio0_51_grp", "gpio0_53_grp"};
+static const char * const sdio0_cd_groups[] = {"gpio0_0_grp",
+		"gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
+		"gpio0_10_grp", "gpio0_12_grp",
+		"gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
+		"gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
+		"gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
+		"gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
+		"gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
+		"gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
+		"gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
+		"gpio0_3_grp", "gpio0_5_grp",
+		"gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
+		"gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
+		"gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
+		"gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
+		"gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
+		"gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
+		"gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
+		"gpio0_51_grp", "gpio0_53_grp", "sdio0_emio_cd_grp"};
+static const char * const sdio0_wp_groups[] = {"gpio0_0_grp",
+		"gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
+		"gpio0_10_grp", "gpio0_12_grp",
+		"gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
+		"gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
+		"gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
+		"gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
+		"gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
+		"gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
+		"gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
+		"gpio0_3_grp", "gpio0_5_grp",
+		"gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
+		"gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
+		"gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
+		"gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
+		"gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
+		"gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
+		"gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
+		"gpio0_51_grp", "gpio0_53_grp", "sdio0_emio_wp_grp"};
+static const char * const sdio1_cd_groups[] = {"gpio0_0_grp",
+		"gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
+		"gpio0_10_grp", "gpio0_12_grp",
+		"gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
+		"gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
+		"gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
+		"gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
+		"gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
+		"gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
+		"gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
+		"gpio0_3_grp", "gpio0_5_grp",
+		"gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
+		"gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
+		"gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
+		"gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
+		"gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
+		"gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
+		"gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
+		"gpio0_51_grp", "gpio0_53_grp", "sdio1_emio_cd_grp"};
+static const char * const sdio1_wp_groups[] = {"gpio0_0_grp",
+		"gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
+		"gpio0_10_grp", "gpio0_12_grp",
+		"gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
+		"gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
+		"gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
+		"gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
+		"gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
+		"gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
+		"gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
+		"gpio0_3_grp", "gpio0_5_grp",
+		"gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
+		"gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
+		"gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
+		"gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
+		"gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
+		"gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
+		"gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
+		"gpio0_51_grp", "gpio0_53_grp", "sdio1_emio_wp_grp"};
+static const char * const smc0_nor_groups[] = {"smc0_nor"};
+static const char * const smc0_nor_cs1_groups[] = {"smc0_nor_cs1_grp"};
+static const char * const smc0_nor_addr25_groups[] = {"smc0_nor_addr25_grp"};
+static const char * const smc0_nand_groups[] = {"smc0_nand"};
+static const char * const can0_groups[] = {"can0_0_grp", "can0_1_grp",
+		"can0_2_grp", "can0_3_grp", "can0_4_grp", "can0_5_grp",
+		"can0_6_grp", "can0_7_grp", "can0_8_grp", "can0_9_grp",
+		"can0_10_grp"};
+static const char * const can1_groups[] = {"can1_0_grp", "can1_1_grp",
+		"can1_2_grp", "can1_3_grp", "can1_4_grp", "can1_5_grp",
+		"can1_6_grp", "can1_7_grp", "can1_8_grp", "can1_9_grp",
+		"can1_10_grp", "can1_11_grp"};
+static const char * const uart0_groups[] = {"uart0_0_grp", "uart0_1_grp",
+		"uart0_2_grp", "uart0_3_grp", "uart0_4_grp", "uart0_5_grp",
+		"uart0_6_grp", "uart0_7_grp", "uart0_8_grp", "uart0_9_grp",
+		"uart0_10_grp"};
+static const char * const uart1_groups[] = {"uart1_0_grp", "uart1_1_grp",
+		"uart1_2_grp", "uart1_3_grp", "uart1_4_grp", "uart1_5_grp",
+		"uart1_6_grp", "uart1_7_grp", "uart1_8_grp", "uart1_9_grp",
+		"uart1_10_grp", "uart1_11_grp"};
+static const char * const i2c0_groups[] = {"i2c0_0_grp", "i2c0_1_grp",
+		"i2c0_2_grp", "i2c0_3_grp", "i2c0_4_grp", "i2c0_5_grp",
+		"i2c0_6_grp", "i2c0_7_grp", "i2c0_8_grp", "i2c0_9_grp",
+		"i2c0_10_grp"};
+static const char * const i2c1_groups[] = {"i2c1_0_grp", "i2c1_1_grp",
+		"i2c1_2_grp", "i2c1_3_grp", "i2c1_4_grp", "i2c1_5_grp",
+		"i2c1_6_grp", "i2c1_7_grp", "i2c1_8_grp", "i2c1_9_grp",
+		"i2c1_10_grp"};
+static const char * const ttc0_groups[] = {"ttc0_0_grp", "ttc0_1_grp",
+					   "ttc0_2_grp"};
+static const char * const ttc1_groups[] = {"ttc1_0_grp", "ttc1_1_grp",
+					   "ttc1_2_grp"};
+static const char * const swdt0_groups[] = {"swdt0_0_grp", "swdt0_1_grp",
+		"swdt0_2_grp", "swdt0_3_grp", "swdt0_4_grp"};
+static const char * const gpio0_groups[] = {"gpio0_0_grp",
+		"gpio0_2_grp", "gpio0_4_grp", "gpio0_6_grp",
+		"gpio0_8_grp", "gpio0_10_grp", "gpio0_12_grp",
+		"gpio0_14_grp", "gpio0_16_grp", "gpio0_18_grp",
+		"gpio0_20_grp", "gpio0_22_grp", "gpio0_24_grp",
+		"gpio0_26_grp", "gpio0_28_grp", "gpio0_30_grp",
+		"gpio0_32_grp", "gpio0_34_grp", "gpio0_36_grp",
+		"gpio0_38_grp", "gpio0_40_grp", "gpio0_42_grp",
+		"gpio0_44_grp", "gpio0_46_grp", "gpio0_48_grp",
+		"gpio0_50_grp", "gpio0_52_grp", "gpio0_1_grp",
+		"gpio0_3_grp", "gpio0_5_grp", "gpio0_7_grp",
+		"gpio0_9_grp", "gpio0_11_grp", "gpio0_13_grp",
+		"gpio0_15_grp", "gpio0_17_grp", "gpio0_19_grp",
+		"gpio0_21_grp", "gpio0_23_grp", "gpio0_25_grp",
+		"gpio0_27_grp", "gpio0_29_grp", "gpio0_31_grp",
+		"gpio0_33_grp", "gpio0_35_grp", "gpio0_37_grp",
+		"gpio0_39_grp", "gpio0_41_grp", "gpio0_43_grp",
+		"gpio0_45_grp", "gpio0_47_grp", "gpio0_49_grp",
+		"gpio0_51_grp", "gpio0_53_grp"};
+
+#define DEFINE_ZYNQ_PINMUX_FUNCTION(fname, mval)	\
+	[ZYNQ_PMUX_##fname] = {				\
+		.name = #fname,				\
+		.groups = fname##_groups,		\
+		.ngroups = ARRAY_SIZE(fname##_groups),	\
+		.mux_val = mval,			\
+	}
+
+#define DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(fname, mval, mux, mask, shift)	\
+	[ZYNQ_PMUX_##fname] = {				\
+		.name = #fname,				\
+		.groups = fname##_groups,		\
+		.ngroups = ARRAY_SIZE(fname##_groups),	\
+		.mux_val = mval,			\
+		.mux_mask = mask,			\
+		.mux_shift = shift,			\
+	}
+
+#define ZYNQ_SDIO_WP_SHIFT	0
+#define ZYNQ_SDIO_WP_MASK	(0x3f << ZYNQ_SDIO_WP_SHIFT)
+#define ZYNQ_SDIO_CD_SHIFT	16
+#define ZYNQ_SDIO_CD_MASK	(0x3f << ZYNQ_SDIO_CD_SHIFT)
+
+static const struct zynq_pinmux_function zynq_pmux_functions[] = {
+	DEFINE_ZYNQ_PINMUX_FUNCTION(ethernet0, 1),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(ethernet1, 1),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(mdio0, 0x40),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(mdio1, 0x50),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(qspi0, 1),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(qspi1, 1),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(qspi_fbclk, 1),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(qspi_cs1, 1),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(spi0, 0x50),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(spi1, 0x50),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(sdio0, 0x40),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(sdio0_pc, 0xc),
+	DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio0_wp, 0, 130, ZYNQ_SDIO_WP_MASK,
+					ZYNQ_SDIO_WP_SHIFT),
+	DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio0_cd, 0, 130, ZYNQ_SDIO_CD_MASK,
+					ZYNQ_SDIO_CD_SHIFT),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(sdio1, 0x40),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(sdio1_pc, 0xc),
+	DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio1_wp, 0, 134, ZYNQ_SDIO_WP_MASK,
+					ZYNQ_SDIO_WP_SHIFT),
+	DEFINE_ZYNQ_PINMUX_FUNCTION_MUX(sdio1_cd, 0, 134, ZYNQ_SDIO_CD_MASK,
+					ZYNQ_SDIO_CD_SHIFT),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nor, 4),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nor_cs1, 8),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nor_addr25, 4),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(smc0_nand, 8),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(can0, 0x10),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(can1, 0x10),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(uart0, 0x70),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(uart1, 0x70),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(i2c0, 0x20),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(i2c1, 0x20),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(ttc0, 0x60),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(ttc1, 0x60),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(swdt0, 0x30),
+	DEFINE_ZYNQ_PINMUX_FUNCTION(gpio0, 0),
+};
+
+
+/* pinctrl */
+static int zynq_pctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct zynq_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctrl->ngroups;
+}
+
+static const char *zynq_pctrl_get_group_name(struct pinctrl_dev *pctldev,
+					     unsigned selector)
+{
+	struct zynq_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctrl->groups[selector].name;
+}
+
+static int zynq_pctrl_get_group_pins(struct pinctrl_dev *pctldev,
+				     unsigned selector,
+				     const unsigned **pins,
+				     unsigned *num_pins)
+{
+	struct zynq_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	*pins = pctrl->groups[selector].pins;
+	*num_pins = pctrl->groups[selector].npins;
+
+	return 0;
+}
+
+static const struct pinctrl_ops zynq_pctrl_ops = {
+	.get_groups_count = zynq_pctrl_get_groups_count,
+	.get_group_name = zynq_pctrl_get_group_name,
+	.get_group_pins = zynq_pctrl_get_group_pins,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
+	.dt_free_map = pinctrl_utils_dt_free_map,
+};
+
+/* pinmux */
+static int zynq_pmux_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	struct zynq_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctrl->nfuncs;
+}
+
+static const char *zynq_pmux_get_function_name(struct pinctrl_dev *pctldev,
+					       unsigned selector)
+{
+	struct zynq_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctrl->funcs[selector].name;
+}
+
+static int zynq_pmux_get_function_groups(struct pinctrl_dev *pctldev,
+					 unsigned selector,
+					 const char * const **groups,
+					 unsigned * const num_groups)
+{
+	struct zynq_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = pctrl->funcs[selector].groups;
+	*num_groups = pctrl->funcs[selector].ngroups;
+	return 0;
+}
+
+static int zynq_pinmux_set_mux(struct pinctrl_dev *pctldev,
+			       unsigned function,
+			       unsigned group)
+{
+	int i, ret;
+	struct zynq_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	const struct zynq_pctrl_group *pgrp = &pctrl->groups[group];
+	const struct zynq_pinmux_function *func = &pctrl->funcs[function];
+
+	/*
+	 * SD WP & CD are special. They have dedicated registers
+	 * to mux them in
+	 */
+	if (function == ZYNQ_PMUX_sdio0_cd || function == ZYNQ_PMUX_sdio0_wp ||
+			function == ZYNQ_PMUX_sdio1_cd ||
+			function == ZYNQ_PMUX_sdio1_wp) {
+		u32 reg;
+
+		ret = regmap_read(pctrl->syscon,
+				  pctrl->pctrl_offset + func->mux, &reg);
+		if (ret)
+			return ret;
+
+		reg &= ~func->mux_mask;
+		reg |= pgrp->pins[0] << func->mux_shift;
+		ret = regmap_write(pctrl->syscon,
+				   pctrl->pctrl_offset + func->mux, reg);
+		if (ret)
+			return ret;
+	} else {
+		for (i = 0; i < pgrp->npins; i++) {
+			unsigned int pin = pgrp->pins[i];
+			u32 reg, addr = pctrl->pctrl_offset + (4 * pin);
+
+			ret = regmap_read(pctrl->syscon, addr, &reg);
+			if (ret)
+				return ret;
+
+			reg &= ~ZYNQ_PINMUX_MUX_MASK;
+			reg |= func->mux_val << ZYNQ_PINMUX_MUX_SHIFT;
+			ret = regmap_write(pctrl->syscon, addr, reg);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static const struct pinmux_ops zynq_pinmux_ops = {
+	.get_functions_count = zynq_pmux_get_functions_count,
+	.get_function_name = zynq_pmux_get_function_name,
+	.get_function_groups = zynq_pmux_get_function_groups,
+	.set_mux = zynq_pinmux_set_mux,
+};
+
+/* pinconfig */
+#define ZYNQ_PINCONF_TRISTATE		BIT(0)
+#define ZYNQ_PINCONF_SPEED		BIT(8)
+#define ZYNQ_PINCONF_PULLUP		BIT(12)
+#define ZYNQ_PINCONF_DISABLE_RECVR	BIT(13)
+
+#define ZYNQ_PINCONF_IOTYPE_SHIFT	9
+#define ZYNQ_PINCONF_IOTYPE_MASK	(7 << ZYNQ_PINCONF_IOTYPE_SHIFT)
+
+enum zynq_io_standards {
+	zynq_iostd_min,
+	zynq_iostd_lvcmos18,
+	zynq_iostd_lvcmos25,
+	zynq_iostd_lvcmos33,
+	zynq_iostd_hstl,
+	zynq_iostd_max
+};
+
+/**
+ * enum zynq_pin_config_param - possible pin configuration parameters
+ * @PIN_CONFIG_IOSTANDARD: if the pin can select an IO standard, the argument to
+ *	this parameter (on a custom format) tells the driver which alternative
+ *	IO standard to use.
+ */
+enum zynq_pin_config_param {
+	PIN_CONFIG_IOSTANDARD = PIN_CONFIG_END + 1,
+};
+
+static const struct pinconf_generic_dt_params zynq_dt_params[] = {
+	{ "io-standard", PIN_CONFIG_IOSTANDARD, zynq_iostd_lvcmos18},
+};
+
+static const struct pin_config_item zynq_conf_items[] = {
+	PCONFDUMP(PIN_CONFIG_IOSTANDARD, "IO-standard", NULL, true),
+};
+
+static unsigned int zynq_pinconf_iostd_get(u32 reg)
+{
+	return (reg & ZYNQ_PINCONF_IOTYPE_MASK) >> ZYNQ_PINCONF_IOTYPE_SHIFT;
+}
+
+static int zynq_pinconf_cfg_get(struct pinctrl_dev *pctldev,
+				unsigned pin,
+				unsigned long *config)
+{
+	u32 reg;
+	int ret;
+	unsigned int arg = 0;
+	unsigned int param = pinconf_to_config_param(*config);
+	struct zynq_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	if (pin >= ZYNQ_NUM_MIOS)
+		return -ENOTSUPP;
+
+	ret = regmap_read(pctrl->syscon, pctrl->pctrl_offset + (4 * pin), &reg);
+	if (ret)
+		return -EIO;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (!(reg & ZYNQ_PINCONF_PULLUP))
+			return -EINVAL;
+		arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		if (!(reg & ZYNQ_PINCONF_TRISTATE))
+			return -EINVAL;
+		arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (reg & ZYNQ_PINCONF_PULLUP || reg & ZYNQ_PINCONF_TRISTATE)
+			return -EINVAL;
+		break;
+	case PIN_CONFIG_SLEW_RATE:
+		arg = !!(reg & ZYNQ_PINCONF_SPEED);
+		break;
+	case PIN_CONFIG_LOW_POWER_MODE:
+	{
+		enum zynq_io_standards iostd = zynq_pinconf_iostd_get(reg);
+
+		if (iostd != zynq_iostd_hstl)
+			return -EINVAL;
+		if (!(reg & ZYNQ_PINCONF_DISABLE_RECVR))
+			return -EINVAL;
+		arg = !!(reg & ZYNQ_PINCONF_DISABLE_RECVR);
+		break;
+	}
+	case PIN_CONFIG_IOSTANDARD:
+		arg = zynq_pinconf_iostd_get(reg);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+	return 0;
+}
+
+static int zynq_pinconf_cfg_set(struct pinctrl_dev *pctldev,
+				unsigned pin,
+				unsigned long *configs,
+				unsigned num_configs)
+{
+	int i, ret;
+	u32 reg;
+	u32 pullup = 0;
+	u32 tristate = 0;
+	struct zynq_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	if (pin >= ZYNQ_NUM_MIOS)
+		return -ENOTSUPP;
+
+	ret = regmap_read(pctrl->syscon, pctrl->pctrl_offset + (4 * pin), &reg);
+	if (ret)
+		return -EIO;
+
+	for (i = 0; i < num_configs; i++) {
+		unsigned int param = pinconf_to_config_param(configs[i]);
+		unsigned int arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_PULL_UP:
+			pullup = ZYNQ_PINCONF_PULLUP;
+			break;
+		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+			tristate = ZYNQ_PINCONF_TRISTATE;
+			break;
+		case PIN_CONFIG_BIAS_DISABLE:
+			reg &= ~(ZYNQ_PINCONF_PULLUP | ZYNQ_PINCONF_TRISTATE);
+			break;
+		case PIN_CONFIG_SLEW_RATE:
+			if (arg)
+				reg |= ZYNQ_PINCONF_SPEED;
+			else
+				reg &= ~ZYNQ_PINCONF_SPEED;
+
+			break;
+		case PIN_CONFIG_IOSTANDARD:
+			if (arg <= zynq_iostd_min || arg >= zynq_iostd_max) {
+				dev_warn(pctldev->dev,
+					 "unsupported IO standard '%u'\n",
+					 param);
+				break;
+			}
+			reg &= ~ZYNQ_PINCONF_IOTYPE_MASK;
+			reg |= arg << ZYNQ_PINCONF_IOTYPE_SHIFT;
+			break;
+		case PIN_CONFIG_LOW_POWER_MODE:
+			if (arg)
+				reg |= ZYNQ_PINCONF_DISABLE_RECVR;
+			else
+				reg &= ~ZYNQ_PINCONF_DISABLE_RECVR;
+
+			break;
+		default:
+			dev_warn(pctldev->dev,
+				 "unsupported configuration parameter '%u'\n",
+				 param);
+			continue;
+		}
+	}
+
+	if (tristate || pullup) {
+		reg &= ~(ZYNQ_PINCONF_PULLUP | ZYNQ_PINCONF_TRISTATE);
+		reg |= tristate | pullup;
+	}
+
+	ret = regmap_write(pctrl->syscon, pctrl->pctrl_offset + (4 * pin), reg);
+	if (ret)
+		return -EIO;
+
+	return 0;
+}
+
+static int zynq_pinconf_group_set(struct pinctrl_dev *pctldev,
+				  unsigned selector,
+				  unsigned long *configs,
+				  unsigned num_configs)
+{
+	int i, ret;
+	struct zynq_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	const struct zynq_pctrl_group *pgrp = &pctrl->groups[selector];
+
+	for (i = 0; i < pgrp->npins; i++) {
+		ret = zynq_pinconf_cfg_set(pctldev, pgrp->pins[i], configs,
+					   num_configs);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct pinconf_ops zynq_pinconf_ops = {
+	.is_generic = true,
+	.pin_config_get = zynq_pinconf_cfg_get,
+	.pin_config_set = zynq_pinconf_cfg_set,
+	.pin_config_group_set = zynq_pinconf_group_set,
+};
+
+static struct pinctrl_desc zynq_desc = {
+	.name = "zynq_pinctrl",
+	.pins = zynq_pins,
+	.npins = ARRAY_SIZE(zynq_pins),
+	.pctlops = &zynq_pctrl_ops,
+	.pmxops = &zynq_pinmux_ops,
+	.confops = &zynq_pinconf_ops,
+	.num_dt_params = ARRAY_SIZE(zynq_dt_params),
+	.params = zynq_dt_params,
+	.conf_items = zynq_conf_items,
+	.owner = THIS_MODULE,
+};
+
+static int zynq_pinctrl_probe(struct platform_device *pdev)
+
+{
+	struct resource *res;
+	struct zynq_pinctrl *pctrl;
+
+	pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	pctrl->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							"syscon");
+	if (IS_ERR(pctrl->syscon)) {
+		dev_err(&pdev->dev, "unable to get syscon\n");
+		return PTR_ERR(pctrl->syscon);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "missing IO resource\n");
+		return -ENODEV;
+	}
+	pctrl->pctrl_offset = res->start;
+
+	pctrl->groups = zynq_pctrl_groups;
+	pctrl->ngroups = ARRAY_SIZE(zynq_pctrl_groups);
+	pctrl->funcs = zynq_pmux_functions;
+	pctrl->nfuncs = ARRAY_SIZE(zynq_pmux_functions);
+
+	pctrl->pctrl = pinctrl_register(&zynq_desc, &pdev->dev, pctrl);
+	if (!pctrl->pctrl)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pctrl);
+
+	dev_info(&pdev->dev, "zynq pinctrl initialized\n");
+
+	return 0;
+}
+
+int zynq_pinctrl_remove(struct platform_device *pdev)
+{
+	struct zynq_pinctrl *pctrl = platform_get_drvdata(pdev);
+
+	pinctrl_unregister(pctrl->pctrl);
+
+	return 0;
+}
+
+static const struct of_device_id zynq_pinctrl_of_match[] = {
+	{ .compatible = "xlnx,pinctrl-zynq" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, zynq_pinctrl_of_match);
+
+static struct platform_driver zynq_pinctrl_driver = {
+	.driver = {
+		.name = "zynq-pinctrl",
+		.of_match_table = zynq_pinctrl_of_match,
+	},
+	.probe = zynq_pinctrl_probe,
+	.remove = zynq_pinctrl_remove,
+};
+
+module_platform_driver(zynq_pinctrl_driver);
+
+MODULE_AUTHOR("Sören Brinkmann <soren.brinkmann@xilinx.com>");
+MODULE_DESCRIPTION("Xilinx Zynq pinctrl driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [PATCH 5/7] pinctrl: Add driver for Zynq

[Andreas Färber]

Hi,

Am 03.11.2014 um 20:05 schrieb Soren Brinkmann:
> +	PINCTRL_PIN(54, "EMIO_SD0_WP"),
> +	PINCTRL_PIN(55, "EMIO_SD0_CD"),
> +	PINCTRL_PIN(56, "EMIO_SD1_WP"),
> +	PINCTRL_PIN(57, "EMIO_SD0_CD"),

Should this be "EMIO_SD1_CD"?

Regards,
Andreas

-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg

Re: [PATCH 5/7] pinctrl: Add driver for Zynq

[Sören Brinkmann]

On Wed, 2014-11-05 at 04:24AM +0100, Andreas Färber wrote:
> Hi,
> 
> Am 03.11.2014 um 20:05 schrieb Soren Brinkmann:
> > +	PINCTRL_PIN(54, "EMIO_SD0_WP"),
> > +	PINCTRL_PIN(55, "EMIO_SD0_CD"),
> > +	PINCTRL_PIN(56, "EMIO_SD1_WP"),
> > +	PINCTRL_PIN(57, "EMIO_SD0_CD"),
> 
> Should this be "EMIO_SD1_CD"?

Absolutely, good catch.

	Thanks,
	Sören

Re: [PATCH 5/7] pinctrl: Add driver for Zynq

[Andreas Färber]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

Am 03.11.2014 um 20:05 schrieb Soren Brinkmann:
> +enum zynq_pinmux_functions {
> +	ZYNQ_PMUX_ethernet0,
> +	ZYNQ_PMUX_ethernet1,
> +	ZYNQ_PMUX_mdio0,
> +	ZYNQ_PMUX_mdio1,
> +	ZYNQ_PMUX_qspi0,
> +	ZYNQ_PMUX_qspi1,
> +	ZYNQ_PMUX_qspi_fbclk,
> +	ZYNQ_PMUX_qspi_cs1,

Are usb0 and usb1 intentionally missing in this patch for lack of
upstream USB driver? If so, then please mention what you left out in the
commit message.

> +	ZYNQ_PMUX_spi0,
> +	ZYNQ_PMUX_spi1,
> +	ZYNQ_PMUX_sdio0,
> +	ZYNQ_PMUX_sdio0_pc,
> +	ZYNQ_PMUX_sdio0_cd,
> +	ZYNQ_PMUX_sdio0_wp,
> +	ZYNQ_PMUX_sdio1,
> +	ZYNQ_PMUX_sdio1_pc,
> +	ZYNQ_PMUX_sdio1_cd,
> +	ZYNQ_PMUX_sdio1_wp,
> +	ZYNQ_PMUX_smc0_nor,
> +	ZYNQ_PMUX_smc0_nor_cs1,
> +	ZYNQ_PMUX_smc0_nor_addr25,
> +	ZYNQ_PMUX_smc0_nand,
> +	ZYNQ_PMUX_can0,
> +	ZYNQ_PMUX_can1,
> +	ZYNQ_PMUX_uart0,
> +	ZYNQ_PMUX_uart1,
> +	ZYNQ_PMUX_i2c0,
> +	ZYNQ_PMUX_i2c1,
> +	ZYNQ_PMUX_ttc0,
> +	ZYNQ_PMUX_ttc1,
> +	ZYNQ_PMUX_swdt0,
> +	ZYNQ_PMUX_gpio0,
> +	ZYNQ_PMUX_MAX_FUNC
> +};

Regards,
Andreas

-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/7] pinctrl: Add driver for Zynq

[Sören Brinkmann]

On Wed, 2014-11-05 at 06:12AM +0100, Andreas Färber wrote:
> Am 03.11.2014 um 20:05 schrieb Soren Brinkmann:
> > +enum zynq_pinmux_functions {
> > +	ZYNQ_PMUX_ethernet0,
> > +	ZYNQ_PMUX_ethernet1,
> > +	ZYNQ_PMUX_mdio0,
> > +	ZYNQ_PMUX_mdio1,
> > +	ZYNQ_PMUX_qspi0,
> > +	ZYNQ_PMUX_qspi1,
> > +	ZYNQ_PMUX_qspi_fbclk,
> > +	ZYNQ_PMUX_qspi_cs1,
> 
> Are usb0 and usb1 intentionally missing in this patch for lack of
> upstream USB driver? If so, then please mention what you left out in the
> commit message.

I don't know how that could happen, but no, it's not intentionally
missing, let's call it accidentally. It just slipped through. Don't
ask me how I could miss USB.

	Thanks,
	Sören

[PATCH 6/7] ARM: zynq: Enable pinctrl

[Soren Brinkmann]

Select pinctrl and the Zynq pinctrl driver.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
Changes since RFC v2:
 - separate mach-zynq changes in their own patch
---
 arch/arm/mach-zynq/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
index aaa5162c1509..78e5e007f52d 100644
--- a/arch/arm/mach-zynq/Kconfig
+++ b/arch/arm/mach-zynq/Kconfig
@@ -9,6 +9,8 @@ config ARCH_ZYNQ
 	select HAVE_ARM_TWD if SMP
 	select ICST
 	select MFD_SYSCON
+	select PINCTRL
+	select PINCTRL_ZYNQ
 	select SOC_BUS
 	help
 	  Support for Xilinx Zynq ARM Cortex A9 Platform
-- 
1.9.1

[PATCH 7/7] ARM: zynq: DT: Add pinctrl information

[Soren Brinkmann]

Add pinctrl descriptions to the zc702 and zc706 device trees.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
Changes since RFC v2:
 - add pinconf properties to zc702 mdio node
 - remove arguments from bias-related props

Changes since RFC v1:
 - separate DT changes into their own patch
---
 arch/arm/boot/dts/zynq-7000.dtsi |   8 ++-
 arch/arm/boot/dts/zynq-zc702.dts | 143 +++++++++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/zynq-zc706.dts | 120 ++++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index ce2ef5bec4f2..fc5246a4f69f 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -237,7 +237,7 @@
 		slcr: slcr@f8000000 {
 			#address-cells = <1>;
 			#size-cells = <1>;
-			compatible = "xlnx,zynq-slcr", "syscon";
+			compatible = "xlnx,zynq-slcr", "syscon", "simple-bus";
 			reg = <0xF8000000 0x1000>;
 			ranges;
 			clkc: clkc@100 {
@@ -258,6 +258,12 @@
 						"dbg_trc", "dbg_apb";
 				reg = <0x100 0x100>;
 			};
+
+			pinctrl0: pinctrl@700 {
+				compatible = "xlnx,pinctrl-zynq";
+				reg = <0x700 0x200>;
+				syscon = <&slcr>;
+			};
 		};
 
 		dmac_s: dmac@f8003000 {
diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts
index 94e2cda6f9b6..f64e7ad53c65 100644
--- a/arch/arm/boot/dts/zynq-zc702.dts
+++ b/arch/arm/boot/dts/zynq-zc702.dts
@@ -40,21 +40,32 @@
 
 &can0 {
 	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_can0_default>;
 };
 
 &gem0 {
 	status = "okay";
 	phy-mode = "rgmii-id";
 	phy-handle = <&ethernet_phy>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_gem0_default>;
 
 	ethernet_phy: ethernet-phy@7 {
 		reg = <7>;
 	};
 };
 
+&gpio0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_gpio0_default>;
+};
+
 &i2c0 {
 	status = "okay";
 	clock-frequency = <400000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c0_default>;
 
 	i2cswitch@74 {
 		compatible = "nxp,pca9548";
@@ -128,10 +139,142 @@
 	};
 };
 
+&pinctrl0 {
+	pinctrl_can0_default: pinctrl-can0-default {
+		common {
+			function = "can0";
+			groups = "can0_9_grp";
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+
+		rx {
+			pins = "MIO46";
+			bias-high-impedance;
+		};
+
+		tx {
+			pins = "MIO47";
+			bias-disable;
+		};
+	};
+
+	pinctrl_gem0_default: pinctrl-gem0-default {
+		common {
+			function = "ethernet0";
+			groups = "ethernet0_0_grp";
+			slew-rate = <0>;
+			io-standard = <4>;
+		};
+
+		rx {
+			pins = "MIO22", "MIO23", "MIO24", "MIO25", "MIO26", "MIO27";
+			bias-high-impedance;
+			low-power-disable;
+		};
+
+		tx {
+			pins = "MIO16", "MIO17", "MIO18", "MIO19", "MIO20", "MIO21";
+			bias-disable;
+			low-power-enable;
+		};
+
+		mdio {
+			function = "mdio0";
+			groups = "mdio0_0_grp";
+			slew-rate = <0>;
+			io-standard = <1>;
+			bias-disable;
+		};
+	};
+
+	pinctrl_gpio0_default: pinctrl-gpio0-default {
+		common {
+			function = "gpio0";
+			groups = "gpio0_7_grp", "gpio0_8_grp", "gpio0_9_grp",
+				 "gpio0_10_grp", "gpio0_11_grp", "gpio0_12_grp",
+				 "gpio0_13_grp", "gpio0_14_grp";
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+
+		pull-up {
+			pins = "MIO9", "MIO10", "MIO11", "MIO12", "MIO13", "MIO14";
+			bias-pull-up;
+		};
+
+		pull-none {
+			pins = "MIO7", "MIO8";
+			bias-disable;
+		};
+	};
+
+	pinctrl_i2c0_default: pinctrl-i2c0-default {
+		common {
+			groups = "i2c0_10_grp";
+			function = "i2c0";
+			bias-pull-up;
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+	};
+
+	pinctrl_sdhci0_default: pinctrl-sdhci0-default {
+		common {
+			groups = "sdio0_2_grp";
+			function = "sdio0";
+			slew-rate = <0>;
+			io-standard = <1>;
+			bias-disable;
+		};
+
+		cd {
+			groups = "gpio0_0_grp";
+			function = "sdio0_cd";
+			bias-high-impedance;
+			bias-pull-up;
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+
+		wp {
+			groups = "gpio0_15_grp";
+			function = "sdio0_wp";
+			bias-high-impedance;
+			bias-pull-up;
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+	};
+
+	pinctrl_uart1_default: pinctrl-uart1-default {
+		common {
+			groups = "uart1_10_grp";
+			function = "uart1";
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+
+		rx {
+			pins = "MIO49";
+			bias-high-impedance;
+		};
+
+		tx {
+			pins = "MIO48";
+			bias-disable = <0>;
+		};
+	};
+};
+
 &sdhci0 {
 	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_sdhci0_default>;
 };
 
 &uart1 {
 	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart1_default>;
 };
diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts
index a8bbdfbc7093..5575a175f867 100644
--- a/arch/arm/boot/dts/zynq-zc706.dts
+++ b/arch/arm/boot/dts/zynq-zc706.dts
@@ -33,15 +33,24 @@
 	status = "okay";
 	phy-mode = "rgmii-id";
 	phy-handle = <&ethernet_phy>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_gem0_default>;
 
 	ethernet_phy: ethernet-phy@7 {
 		reg = <7>;
 	};
 };
 
+&gpio0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_gpio0_default>;
+};
+
 &i2c0 {
 	status = "okay";
 	clock-frequency = <400000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c0_default>;
 
 	i2cswitch@74 {
 		compatible = "nxp,pca9548";
@@ -107,10 +116,121 @@
 	};
 };
 
+&pinctrl0 {
+	pinctrl_gem0_default: pinctrl-gem0-default {
+		common {
+			function = "ethernet0";
+			groups = "ethernet0_0_grp";
+			slew-rate = <0>;
+			io-standard = <4>;
+		};
+
+		rx {
+			pins = "MIO22", "MIO23", "MIO24", "MIO25", "MIO26", "MIO27";
+			bias-high-impedance;
+			low-power-disable;
+		};
+
+		tx {
+			pins = "MIO16", "MIO17", "MIO18", "MIO19", "MIO20", "MIO21";
+			low-power-enable;
+			bias-disable;
+		};
+
+		mdio {
+			function = "mdio0";
+			groups = "mdio0_0_grp";
+			slew-rate = <0>;
+			io-standard = <1>;
+			bias-disable;
+		};
+	};
+
+	pinctrl_gpio0_default: pinctrl-gpio0-default {
+		common {
+			function = "gpio0";
+			groups = "gpio0_7_grp", "gpio0_46_grp", "gpio0_47_grp";
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+
+		pull-up {
+			pins = "MIO46", "MIO47";
+			bias-pull-up;
+		};
+
+		pull-none {
+			pins = "MIO7";
+			bias-disable;
+		};
+	};
+
+	pinctrl_i2c0_default: pinctrl-i2c0-default {
+		common {
+			groups = "i2c0_10_grp";
+			function = "i2c0";
+			bias-pull-up;
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+	};
+
+	pinctrl_sdhci0_default: pinctrl-sdhci0-default {
+		common {
+			groups = "sdio0_2_grp";
+			function = "sdio0";
+			slew-rate = <0>;
+			io-standard = <1>;
+			bias-disable;
+		};
+
+		cd {
+			groups = "gpio0_14_grp";
+			function = "sdio0_cd";
+			bias-high-impedance;
+			bias-pull-up;
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+
+		wp {
+			groups = "gpio0_15_grp";
+			function = "sdio0_wp";
+			bias-high-impedance;
+			bias-pull-up;
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+	};
+
+	pinctrl_uart1_default: pinctrl-uart1-default {
+		common {
+			groups = "uart1_10_grp";
+			function = "uart1";
+			slew-rate = <0>;
+			io-standard = <1>;
+		};
+
+		rx {
+			pins = "MIO49";
+			bias-high-impedance;
+		};
+
+		tx {
+			pins = "MIO48";
+			bias-disable;
+		};
+	};
+};
+
 &sdhci0 {
 	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_sdhci0_default>;
 };
 
 &uart1 {
 	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart1_default>;
 };
-- 
1.9.1

Re: [PATCH 0/7] Pinctrl support for Zynq

[Andreas Färber]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

Hi Sören,

Am 03.11.2014 um 20:05 schrieb Soren Brinkmann:
> Soren Brinkmann (7):
>   pinctrl: pinconf-generic: Declare dt_params/conf_items const
>   pinctrl: pinconf-generic: Infer map type from DT property
>   pinctrl: pinconf-generic: Allow driver to specify DT params
>   pinctrl: zynq: Document DT binding
>   pinctrl: Add driver for Zynq
>   ARM: zynq: Enable pinctrl
>   ARM: zynq: DT: Add pinctrl information

Thanks for your work on this,

Tested-by: Andreas Färber <afaerber@suse.de>

I've tracked down all 54 MIO pins of the Parallella and cooked up the
equivalent DT patch. QSPI and USB still seem to be missing drivers
upstream; I reused the SPI driver for the QSPI with pinctrl and
Punnaiah's chipidea driver (not fully working) without pinctrl for lack
of group/function definitions. For testing purposes I've configured a
heartbeat trigger for the USER_LED (CR10).

To my disappointment these pinctrl additions did not fix one issue:
Whenever a write access to be handled by the bitstream (0x808f0f04) is
performed, the board hangs and the heartbeat stops. Would a bug in the
bitstream allow this to happen, or are more drivers missing to actually
make use of the PL in general? With a downstream ADI/Xilinx 3.12 kernel
that problem does not surface.

Regards,
Andreas

https://github.com/afaerber/linux/commits/parallella-next

-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/7] Pinctrl support for Zynq

[Sören Brinkmann]

On Wed, 2014-11-05 at 06:56AM +0100, Andreas Färber wrote:
> Hi Sören,
> 
> Am 03.11.2014 um 20:05 schrieb Soren Brinkmann:
> > Soren Brinkmann (7):
> >   pinctrl: pinconf-generic: Declare dt_params/conf_items const
> >   pinctrl: pinconf-generic: Infer map type from DT property
> >   pinctrl: pinconf-generic: Allow driver to specify DT params
> >   pinctrl: zynq: Document DT binding
> >   pinctrl: Add driver for Zynq
> >   ARM: zynq: Enable pinctrl
> >   ARM: zynq: DT: Add pinctrl information
> 
> Thanks for your work on this,
> 
> Tested-by: Andreas Färber <afaerber@suse.de>

Thanks for testing.

> 
> I've tracked down all 54 MIO pins of the Parallella and cooked up the
> equivalent DT patch. QSPI and USB still seem to be missing drivers
> upstream; I reused the SPI driver for the QSPI with pinctrl and
> Punnaiah's chipidea driver (not fully working) without pinctrl for lack
> of group/function definitions. For testing purposes I've configured a
> heartbeat trigger for the USER_LED (CR10).
> 
> To my disappointment these pinctrl additions did not fix one issue:
> Whenever a write access to be handled by the bitstream (0x808f0f04) is
> performed, the board hangs and the heartbeat stops. Would a bug in the
> bitstream allow this to happen, or are more drivers missing to actually
> make use of the PL in general? With a downstream ADI/Xilinx 3.12 kernel
> that problem does not surface.

This doesn't sound like being related to pinctrl at all.
Devices in the PL are just memory mapped on the AXI bus. There is
nothing needed to access those. Hangs do in most cases indicate that the
IP does not respond (properly). In my experience this is mostly caused
by 
 - level shifters not enabled
 - IP kept in reset
 - IP is clock gated
With the clock gating being the culprit in most cases. Did you check
those things?

	Sören

Re: [PATCH 0/7] Pinctrl support for Zynq

[Andreas Färber]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

Am 05.11.2014 um 18:03 schrieb Sören Brinkmann:
> On Wed, 2014-11-05 at 06:56AM +0100, Andreas Färber wrote:
>> I've tracked down all 54 MIO pins of the Parallella and cooked up the
>> equivalent DT patch. [...] For testing purposes I've configured a
>> heartbeat trigger for the USER_LED (CR10).
>>
>> To my disappointment these pinctrl additions did not fix one issue:
>> Whenever a write access to be handled by the bitstream (0x808f0f04) is
>> performed, the board hangs and the heartbeat stops. Would a bug in the
>> bitstream allow this to happen, or are more drivers missing to actually
>> make use of the PL in general? With a downstream ADI/Xilinx 3.12 kernel
>> that problem does not surface.
> 
> This doesn't sound like being related to pinctrl at all.
> Devices in the PL are just memory mapped on the AXI bus. There is
> nothing needed to access those. Hangs do in most cases indicate that the
> IP does not respond (properly). In my experience this is mostly caused
> by 
>  - level shifters not enabled
>  - IP kept in reset
>  - IP is clock gated
> With the clock gating being the culprit in most cases. Did you check
> those things?

Figured it out: zynq-7000.dtsi sets fclk-enable = <0>, i.e., all PL
clocks are disabled by default. When overriding that tiny property with
0xf it suddenly works as expected! I'll send a patch later in the day.

Are boards expected to use clocks = <&clkc 15>, ...; on individual nodes
relying on the PL? Or does enabling those clocks require actually
loading a bitstream so that it is not being done by default?

It seems ranger dangerous to me that a single MMIO write can freeze the
system - as a software developer I would've expected this to be caught
and handled as a SIGBUS.

Regards,
Andreas

-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/7] Pinctrl support for Zynq

[Sören Brinkmann]

On Thu, 2014-11-06 at 04:51AM +0100, Andreas Färber wrote:
> Am 05.11.2014 um 18:03 schrieb Sören Brinkmann:
> > On Wed, 2014-11-05 at 06:56AM +0100, Andreas Färber wrote:
> >> I've tracked down all 54 MIO pins of the Parallella and cooked up the
> >> equivalent DT patch. [...] For testing purposes I've configured a
> >> heartbeat trigger for the USER_LED (CR10).
> >>
> >> To my disappointment these pinctrl additions did not fix one issue:
> >> Whenever a write access to be handled by the bitstream (0x808f0f04) is
> >> performed, the board hangs and the heartbeat stops. Would a bug in the
> >> bitstream allow this to happen, or are more drivers missing to actually
> >> make use of the PL in general? With a downstream ADI/Xilinx 3.12 kernel
> >> that problem does not surface.
> > 
> > This doesn't sound like being related to pinctrl at all.
> > Devices in the PL are just memory mapped on the AXI bus. There is
> > nothing needed to access those. Hangs do in most cases indicate that the
> > IP does not respond (properly). In my experience this is mostly caused
> > by 
> >  - level shifters not enabled
> >  - IP kept in reset
> >  - IP is clock gated
> > With the clock gating being the culprit in most cases. Did you check
> > those things?
> 
> Figured it out: zynq-7000.dtsi sets fclk-enable = <0>, i.e., all PL
> clocks are disabled by default. When overriding that tiny property with
> 0xf it suddenly works as expected! I'll send a patch later in the day.
> 
> Are boards expected to use clocks = <&clkc 15>, ...; on individual nodes
> relying on the PL? Or does enabling those clocks require actually
> loading a bitstream so that it is not being done by default?

Clocks are under control of drivers. Drivers are supposed to
use the clock framework to enable the clocks they use. The fclk-enable
is just a work around to enable driver-less IP and similar.

So, if your FPGA part has proper drivers, you should add the appropriate
description in DT and the drivers would need to use the CCF to enable
the clocks. Otherwise you can work around this by specifying
fclk-enable.

	Sören