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

Message ID 1415041531-15520-3-git-send-email-soren.brinkmann@xilinx.com
State New, archived
Headers show

Commit Message

Soren Brinkmann Nov. 3, 2014, 7:05 p.m. UTC
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(-)

Comments

Laurent Pinchart Nov. 5, 2014, 1:56 p.m. UTC | #1
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 */
Soren Brinkmann Nov. 5, 2014, 6:09 p.m. UTC | #2
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
Laurent Pinchart Nov. 5, 2014, 6:17 p.m. UTC | #3
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.
Linus Walleij Nov. 11, 2014, 12:29 p.m. UTC | #4
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
Linus Walleij Nov. 11, 2014, 12:47 p.m. UTC | #5
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
Soren Brinkmann Nov. 12, 2014, 6:43 p.m. UTC | #6
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
Soren Brinkmann Nov. 12, 2014, 6:46 p.m. UTC | #7
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
Soren Brinkmann Nov. 12, 2014, 7:38 p.m. UTC | #8
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
diff mbox

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 */