| Message ID | 1415041531-15520-3-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 */
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
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.
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
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
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
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
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
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 */
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(-)