| Message ID | 1370988237-30593-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Wed, 12 Jun 2013 00:03:57 +0200, Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > Document DT properties for the generic pinctrl parameters and add a > parser function. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > .../bindings/pinctrl/pinctrl-bindings.txt | 29 +++++++ > drivers/pinctrl/pinconf-generic.c | 94 ++++++++++++++++++++++ > drivers/pinctrl/pinconf.h | 17 ++++ > 3 files changed, 140 insertions(+) > > I've successfully tested this patch (or more accurately only the pull-up and > pull-down properties) with the Renesas sh-pfc pinctrl device driver. I will > resent the sh-pfc DT bindings patch series rebased on the generic pinconf > bindings. > > Not all generic pinconf properties are currently implemented, but I don't > think that should be a showstopper. We can add them later as needed. > > The code is based on both the sh-pfc pinconf DT parser and James Hogan's > tz1090 DT parser ("[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl driver"). > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > index c95ea82..e499ff0 100644 > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > @@ -126,3 +126,32 @@ device; they may be grandchildren, for example. Whether this is legal, and > whether there is any interaction between the child and intermediate parent > nodes, is again defined entirely by the binding for the individual pin > controller device. > + > +== Generic pinconf parameters == > + > +Pin configuration parameters are expressed by DT properties in the pin > +controller device state nodes and child nodes. For devices that use the generic > +pinconf parameters the following properties are defined. > + > +- tristate: A boolean, put the pin into high impedance state when set. > + > +- pull-up: An integer representing the pull-up strength. 0 disables the pull-up, > + non-zero values enable it. > + > +- pull-down: An integer representing the pull-down strength. 0 disables the > + pull-down, non-zero values enables it. > + > +- schmitt: An integer, enable or disable Schmitt trigger mode for the pins. > + Valid values are > + 0: Schmitt trigger disabled (no hysteresis) > + 1: Schmitt trigger enabled > + > +- slew-rate: An integer controlling the pin slew rate. Values are device- > + dependent. > + > +- drive-strength: An integer representing the drive strength of pins in mA. > + Valid values are device-dependent. > + > +The pinctrl device DT bindings documentation must list the properties that > +apply to the device, and define the valid range for all device-dependent > +values. I don't see any problem with the above properties, but I would like to see an example. How verbose will a pinctrl node using the generic properties tend to be? > diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c > index 2ad5a8d..bd0e41d 100644 > --- a/drivers/pinctrl/pinconf-generic.c > +++ b/drivers/pinctrl/pinconf-generic.c > @@ -15,6 +15,7 @@ > #include <linux/module.h> > #include <linux/init.h> > #include <linux/device.h> > +#include <linux/of.h> > #include <linux/slab.h> > #include <linux/debugfs.h> > #include <linux/seq_file.h> > @@ -135,3 +136,96 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev, > } > EXPORT_SYMBOL_GPL(pinconf_generic_dump_config); > #endif > + > +struct pinconf_generic_param { > + const char *property; > + enum pin_config_param param; > + bool flag; > +}; > + > +static const struct pinconf_generic_param pinconf_generic_params[] = { > + { "tristate", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, true }, > + { "pull-up", PIN_CONFIG_BIAS_PULL_UP, false }, > + { "pull-down", PIN_CONFIG_BIAS_PULL_DOWN, false }, > + { "schmitt", PIN_CONFIG_INPUT_SCHMITT_ENABLE, true }, > + { "slew-rate", PIN_CONFIG_SLEW_RATE, false }, > + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, false }, > +}; > + > +static int pinconf_generic_add_config(unsigned long **configs, > + unsigned int *num_configs, > + unsigned long config) > +{ > + unsigned int count = *num_configs + 1; > + unsigned long *cfgs; > + > + cfgs = krealloc(*configs, sizeof(*cfgs) * count, GFP_KERNEL); > + if (cfgs == NULL) > + return -ENOMEM; > + > + cfgs[count - 1] = config; > + > + *configs = cfgs; > + *num_configs = count; > + > + return 0; > +} Hmmm. We really need a better method of parsing multiple properties. I've been toying around with a few ideas, but haven't been able to draft something I'm happy with yeat. Regardless, the code in this patch looks fine to me. g. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/06/13 23:03, Laurent Pinchart wrote: > Document DT properties for the generic pinctrl parameters and add a > parser function. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > .../bindings/pinctrl/pinctrl-bindings.txt | 29 +++++++ > drivers/pinctrl/pinconf-generic.c | 94 ++++++++++++++++++++++ > drivers/pinctrl/pinconf.h | 17 ++++ > 3 files changed, 140 insertions(+) > > I've successfully tested this patch (or more accurately only the pull-up and > pull-down properties) with the Renesas sh-pfc pinctrl device driver. I will > resent the sh-pfc DT bindings patch series rebased on the generic pinconf > bindings. > > Not all generic pinconf properties are currently implemented, but I don't > think that should be a showstopper. We can add them later as needed. > > The code is based on both the sh-pfc pinconf DT parser and James Hogan's > tz1090 DT parser ("[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl driver"). Thanks for this patch. I haven't tested it (yet), but have a few comments below. > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > index c95ea82..e499ff0 100644 > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > @@ -126,3 +126,32 @@ device; they may be grandchildren, for example. Whether this is legal, and > whether there is any interaction between the child and intermediate parent > nodes, is again defined entirely by the binding for the individual pin > controller device. > + > +== Generic pinconf parameters == > + > +Pin configuration parameters are expressed by DT properties in the pin > +controller device state nodes and child nodes. For devices that use the generic > +pinconf parameters the following properties are defined. > + > +- tristate: A boolean, put the pin into high impedance state when set. > + > +- pull-up: An integer representing the pull-up strength. 0 disables the pull-up, > + non-zero values enable it. > + > +- pull-down: An integer representing the pull-down strength. 0 disables the > + pull-down, non-zero values enables it. > + > +- schmitt: An integer, enable or disable Schmitt trigger mode for the pins. > + Valid values are > + 0: Schmitt trigger disabled (no hysteresis) > + 1: Schmitt trigger enabled this is set as a flag, so I think it should be described like tristate, "A boolean, ... when set."? Same for pull-up and pull-down (see comment below). <snip> > + { "pull-up", PIN_CONFIG_BIAS_PULL_UP, false }, > + { "pull-down", PIN_CONFIG_BIAS_PULL_DOWN, false }, pinconf-generic.h says "If the argument is != 0 pull-up is enabled, if it is 0, pull-up is disabled", so I think these should be flags unless it's changed there first. Any chance of adding the new "bus-hold" entry too (PIN_CONFIG_BIAS_BUS_HOLD, and flag=true I suppose)? see aa69352252a7a952e6e77734cb87135143a377d2 in LinuxW's pinctrl for-next branch. <snip> > +EXPORT_SYMBOL_GPL(pinconf_generic_parse_params); > diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h > index 92c7267..eb8550b 100644 > --- a/drivers/pinctrl/pinconf.h > +++ b/drivers/pinctrl/pinconf.h > @@ -90,6 +90,23 @@ static inline void pinconf_init_device_debugfs(struct dentry *devroot, > * pin config. > */ > > +#if defined(CONFIG_GENERIC_PINCONF) > + > +int pinconf_generic_parse_params(struct device *dev, struct device_node *np, > + unsigned long **cfgs); > + > +#else > + > +static inline int pinconf_generic_parse_params(struct device *dev, > + struct device_node *np, > + unsigned long **cfgs) > +{ > + *cfgs = NULL; > + return 0; > +} Should this ever be necessary? Sounds like if the driver wanted to use this it should already have selected GENERIC_PINCONF anyway. Cheers James -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant, Thanks for the review. On Wednesday 12 June 2013 13:48:33 Grant Likely wrote: > On Wed, 12 Jun 2013 00:03:57 +0200, Laurent Pinchart wrote: > > Document DT properties for the generic pinctrl parameters and add a > > parser function. > > > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > .../bindings/pinctrl/pinctrl-bindings.txt | 29 +++++++ > > drivers/pinctrl/pinconf-generic.c | 94 +++++++++++++++++ > > drivers/pinctrl/pinconf.h | 17 ++++ > > 3 files changed, 140 insertions(+) > > > > I've successfully tested this patch (or more accurately only the pull-up > > and pull-down properties) with the Renesas sh-pfc pinctrl device driver. > > I will resent the sh-pfc DT bindings patch series rebased on the generic > > pinconf bindings. > > > > Not all generic pinconf properties are currently implemented, but I don't > > think that should be a showstopper. We can add them later as needed. > > > > The code is based on both the sh-pfc pinconf DT parser and James Hogan's > > tz1090 DT parser ("[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl > > driver"). > > > > diff --git > > a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index > > c95ea82..e499ff0 100644 > > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > @@ -126,3 +126,32 @@ device; they may be grandchildren, for example. > > Whether this is legal, and> > > whether there is any interaction between the child and intermediate > > parent nodes, is again defined entirely by the binding for the individual > > pin controller device. > > > > + > > +== Generic pinconf parameters == > > + > > +Pin configuration parameters are expressed by DT properties in the pin > > +controller device state nodes and child nodes. For devices that use the > > generic +pinconf parameters the following properties are defined. > > + > > +- tristate: A boolean, put the pin into high impedance state when set. > > + > > +- pull-up: An integer representing the pull-up strength. 0 disables the > > pull-up, + non-zero values enable it. > > + > > +- pull-down: An integer representing the pull-down strength. 0 disables > > the + pull-down, non-zero values enables it. > > + > > +- schmitt: An integer, enable or disable Schmitt trigger mode for the > > pins. + Valid values are > > + 0: Schmitt trigger disabled (no hysteresis) > > + 1: Schmitt trigger enabled > > + > > +- slew-rate: An integer controlling the pin slew rate. Values are device- > > + dependent. > > + > > +- drive-strength: An integer representing the drive strength of pins in > > mA. + Valid values are device-dependent. > > + > > +The pinctrl device DT bindings documentation must list the properties > > that > > +apply to the device, and define the valid range for all device-dependent > > +values. > > I don't see any problem with the above properties, but I would like to > see an example. How verbose will a pinctrl node using the generic > properties tend to be? Here's a real-life example &pfc { pinctrl-0 = <&scifa4_pins>; pinctrl-names = "default"; mmcif_pins: mmcif { mux { renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0"; renesas,function = "mmc0"; }; cfg { renesas,groups = "mmc0_data8_0"; renesas,pins = "PORT279"; bias-pull-up = <1>; }; }; scifa4_pins: scifa4 { renesas,groups = "scifa4_data", "scifa4_ctrl"; renesas,function = "scifa4"; }; }; The mux node selects function mmc0 on two pin groups, and the cfg node activates pull-ups on one pin group and one particular pin. > > diff --git a/drivers/pinctrl/pinconf-generic.c > > b/drivers/pinctrl/pinconf-generic.c index 2ad5a8d..bd0e41d 100644 > > --- a/drivers/pinctrl/pinconf-generic.c > > +++ b/drivers/pinctrl/pinconf-generic.c > > @@ -15,6 +15,7 @@ > > > > #include <linux/module.h> > > #include <linux/init.h> > > #include <linux/device.h> > > > > +#include <linux/of.h> > > > > #include <linux/slab.h> > > #include <linux/debugfs.h> > > #include <linux/seq_file.h> > > > > @@ -135,3 +136,96 @@ void pinconf_generic_dump_config(struct pinctrl_dev > > *pctldev,> > > } > > EXPORT_SYMBOL_GPL(pinconf_generic_dump_config); > > #endif > > > > + > > +struct pinconf_generic_param { > > + const char *property; > > + enum pin_config_param param; > > + bool flag; > > +}; > > + > > +static const struct pinconf_generic_param pinconf_generic_params[] = { > > + { "tristate", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, true }, > > + { "pull-up", PIN_CONFIG_BIAS_PULL_UP, false }, > > + { "pull-down", PIN_CONFIG_BIAS_PULL_DOWN, false }, > > + { "schmitt", PIN_CONFIG_INPUT_SCHMITT_ENABLE, true }, > > + { "slew-rate", PIN_CONFIG_SLEW_RATE, false }, > > + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, false }, > > +}; > > + > > +static int pinconf_generic_add_config(unsigned long **configs, > > + unsigned int *num_configs, > > + unsigned long config) > > +{ > > + unsigned int count = *num_configs + 1; > > + unsigned long *cfgs; > > + > > + cfgs = krealloc(*configs, sizeof(*cfgs) * count, GFP_KERNEL); > > + if (cfgs == NULL) > > + return -ENOMEM; > > + > > + cfgs[count - 1] = config; > > + > > + *configs = cfgs; > > + *num_configs = count; > > + > > + return 0; > > +} > > Hmmm. We really need a better method of parsing multiple properties. > I've been toying around with a few ideas, but haven't been able to draft > something I'm happy with yeat. > > Regardless, the code in this patch looks fine to me. Thanks. As other generic pinconf DT bindings proposals have been submitted I will resent this patch set with pinconf support stripped out to make sure it gets to v3.11 and will then add pinconf back in follow-up patches for v3.11 or v3.12, depending on when we can agree on generic pinconf bindings.
Hi James, On Wednesday 12 June 2013 15:36:59 James Hogan wrote: > On 11/06/13 23:03, Laurent Pinchart wrote: > > Document DT properties for the generic pinctrl parameters and add a > > parser function. > > > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > .../bindings/pinctrl/pinctrl-bindings.txt | 29 +++++++ > > drivers/pinctrl/pinconf-generic.c | 94 +++++++++++++++++ > > drivers/pinctrl/pinconf.h | 17 ++++ > > 3 files changed, 140 insertions(+) > > > > I've successfully tested this patch (or more accurately only the pull-up > > and pull-down properties) with the Renesas sh-pfc pinctrl device driver. > > I will resent the sh-pfc DT bindings patch series rebased on the generic > > pinconf bindings. > > > > Not all generic pinconf properties are currently implemented, but I don't > > think that should be a showstopper. We can add them later as needed. > > > > The code is based on both the sh-pfc pinconf DT parser and James Hogan's > > tz1090 DT parser ("[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl > > driver"). > > Thanks for this patch. I haven't tested it (yet), but have a few > comments below. > > > diff --git > > a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index > > c95ea82..e499ff0 100644 > > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > @@ -126,3 +126,32 @@ device; they may be grandchildren, for example. > > Whether this is legal, and> > > whether there is any interaction between the child and intermediate > > parent nodes, is again defined entirely by the binding for the individual > > pin controller device. > > > > + > > +== Generic pinconf parameters == > > + > > +Pin configuration parameters are expressed by DT properties in the pin > > +controller device state nodes and child nodes. For devices that use the > > generic +pinconf parameters the following properties are defined. > > + > > +- tristate: A boolean, put the pin into high impedance state when set. > > + > > +- pull-up: An integer representing the pull-up strength. 0 disables the > > pull-up, + non-zero values enable it. > > + > > +- pull-down: An integer representing the pull-down strength. 0 disables > > the + pull-down, non-zero values enables it. > > + > > +- schmitt: An integer, enable or disable Schmitt trigger mode for the > > pins. + Valid values are > > + 0: Schmitt trigger disabled (no hysteresis) > > + 1: Schmitt trigger enabled > > this is set as a flag, so I think it should be described like tristate, > "A boolean, ... when set."? Same for pull-up and pull-down (see comment > below). Can't the value be used to control schmitt trigger parameters (same as for the pull-up and pull-down values, as explained below) ? > <snip> > > > + { "pull-up", PIN_CONFIG_BIAS_PULL_UP, false }, > > + { "pull-down", PIN_CONFIG_BIAS_PULL_DOWN, false }, > > pinconf-generic.h says "If the argument is != 0 pull-up is enabled, if > it is 0, pull-up is disabled", so I think these should be flags unless > it's changed there first. == 0 for disabled and != 0 for enabled doesn't mean that all != 0 values are equivalent. As I read it drivers can use the value to control the pull-up/down strength without violating the documentation. > Any chance of adding the new "bus-hold" entry too > (PIN_CONFIG_BIAS_BUS_HOLD, and flag=true I suppose)? see > aa69352252a7a952e6e77734cb87135143a377d2 in LinuxW's pinctrl for-next > branch. Another generic pinconf DT bindings proposal has been submitted and applied to the devel branch in Linus' pinctrl tree. It includes support for PIN_CONFIG_BIAS_BUS_HOLD. Linus asked me to review the patch, so this one will likely be dropped or at least integrated into the other one. > <snip> > > > +EXPORT_SYMBOL_GPL(pinconf_generic_parse_params); > > diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h > > index 92c7267..eb8550b 100644 > > --- a/drivers/pinctrl/pinconf.h > > +++ b/drivers/pinctrl/pinconf.h > > @@ -90,6 +90,23 @@ static inline void pinconf_init_device_debugfs(struct > > dentry *devroot,> > > * pin config. > > */ > > > > +#if defined(CONFIG_GENERIC_PINCONF) > > + > > +int pinconf_generic_parse_params(struct device *dev, struct device_node > > *np, + unsigned long **cfgs); > > + > > +#else > > + > > +static inline int pinconf_generic_parse_params(struct device *dev, > > + struct device_node *np, > > + unsigned long **cfgs) > > +{ > > + *cfgs = NULL; > > + return 0; > > +} > > Should this ever be necessary? Sounds like if the driver wanted to use > this it should already have selected GENERIC_PINCONF anyway. You're right.
On Fri, Jun 14, 2013 at 12:39 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > mmcif_pins: mmcif { > mux { > renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0"; > renesas,function = "mmc0"; > }; > cfg { > renesas,groups = "mmc0_data8_0"; > renesas,pins = "PORT279"; > bias-pull-up = <1>; If I understood your code correctly that last statement can *optionally* be written like just: bias-pull-up; Without the parameter? I think that Heiko's implementation does this anyway. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Samstag, 15. Juni 2013, 21:56:05 schrieb Linus Walleij: > On Fri, Jun 14, 2013 at 12:39 AM, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > mmcif_pins: mmcif { > > > > mux { > > > > renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0"; > > renesas,function = "mmc0"; > > > > }; > > cfg { > > > > renesas,groups = "mmc0_data8_0"; > > renesas,pins = "PORT279"; > > bias-pull-up = <1>; > > If I understood your code correctly that last statement can *optionally* > be written like just: > > bias-pull-up; > > Without the parameter? > > I think that Heiko's implementation does this anyway. Yep, with the fixes-series from yesterday the bias-pull-* now have a better default value of <1>. so you can do bias-pull-up; which is then identical to the bias-pull-up = <1>; above (both are valid of course). Disable would the be either bias-disable; or bias-pull-up = <0>; A driver should probably handle both, as both are valid pinconf options or this. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Heiko, On Saturday 15 June 2013 22:16:13 Heiko Stübner wrote: > Am Samstag, 15. Juni 2013, 21:56:05 schrieb Linus Walleij: > > On Fri, Jun 14, 2013 at 12:39 AM, Laurent Pinchart wrote: > > > mmcif_pins: mmcif { > > > mux { > > > renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0"; > > > renesas,function = "mmc0"; > > > }; > > > cfg { > > > renesas,groups = "mmc0_data8_0"; > > > renesas,pins = "PORT279"; > > > bias-pull-up = <1>; > > > > If I understood your code correctly that last statement can *optionally* > > > > be written like just: > > bias-pull-up; > > > > Without the parameter? > > > > I think that Heiko's implementation does this anyway. > > Yep, with the fixes-series from yesterday the bias-pull-* now have a better > default value of <1>. > > so you can do > bias-pull-up; > which is then identical to the > bias-pull-up = <1>; > above (both are valid of course). My patch used bias-pull-up = <1>; as the current version of Heiko's DT parser didn't support bias-pull-up; correctly at that time. It's fixed now, and my prefered is then bias-pull-up;. > Disable would the be either > bias-disable; > or > bias-pull-up = <0>; > > A driver should probably handle both, as both are valid pinconf options or > this. I feel a bit uneasy about that. Do we really need to support two different ways to achieve the same result ?
On Sun, Jun 16, 2013 at 1:35 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Saturday 15 June 2013 22:16:13 Heiko Stübner wrote: >> Am Samstag, 15. Juni 2013, 21:56:05 schrieb Linus Walleij: >> Disable would the be either >> bias-disable; >> or >> bias-pull-up = <0>; >> >> A driver should probably handle both, as both are valid pinconf options or >> this. > > I feel a bit uneasy about that. Do we really need to support two different > ways to achieve the same result ? In this specific case I think yes, but not on all options. As dicussed earlier this was designed for systems where you could set the pull-up resistance, like bias-pull-up = <600000>; would give 600kOhm pull up. In most existing systems that is silly, as they can't specify it, so they should be able to do just: bias-pull-up; as that is all they can do. If we have to cut one way, we should cut the former until such a system appears. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Sunday 16 June 2013 01:51:32 Linus Walleij wrote: > On Sun, Jun 16, 2013 at 1:35 AM, Laurent Pinchart wrote: > > On Saturday 15 June 2013 22:16:13 Heiko Stübner wrote: > >> Am Samstag, 15. Juni 2013, 21:56:05 schrieb Linus Walleij: > >> > >> Disable would the be either > >> > >> bias-disable; > >> > >> or > >> > >> bias-pull-up = <0>; > >> > >> A driver should probably handle both, as both are valid pinconf options > >> or this. > > > > I feel a bit uneasy about that. Do we really need to support two different > > ways to achieve the same result ? > > In this specific case I think yes, but not on all options. > > As dicussed earlier this was designed for systems where > you could set the pull-up resistance, like > > bias-pull-up = <600000>; > > would give 600kOhm pull up. > > In most existing systems that is silly, as they can't specify it, so they > should be able to do just: > > bias-pull-up; > > as that is all they can do. If we have to cut one way, we should cut the > former until such a system appears. I'm fine with bias-pull-up = <1>; vs bias-pull-up;. What bothers me a bit is bias-pull-up = <0>; vs bias-disable;.
On Sun, Jun 16, 2013 at 1:52 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Linus, > On Sunday 16 June 2013 01:51:32 Linus Walleij wrote: >> As dicussed earlier this was designed for systems where >> you could set the pull-up resistance, like >> >> bias-pull-up = <600000>; >> >> would give 600kOhm pull up. >> >> In most existing systems that is silly, as they can't specify it, so they >> should be able to do just: >> >> bias-pull-up; >> >> as that is all they can do. If we have to cut one way, we should cut the >> former until such a system appears. > > I'm fine with bias-pull-up = <1>; vs bias-pull-up;. What bothers me a bit is > bias-pull-up = <0>; vs bias-disable;. Oh yeah OK you got a point there for sure. Setting bias-pull-up = <0>; would be equal to short-circuit so it does not make any kind of sense. Let's keep an eye on this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/15/2013 02:16 PM, Heiko Stübner wrote: > Am Samstag, 15. Juni 2013, 21:56:05 schrieb Linus Walleij: >> On Fri, Jun 14, 2013 at 12:39 AM, Laurent Pinchart >> >> <laurent.pinchart@ideasonboard.com> wrote: >>> mmcif_pins: mmcif { >>> >>> mux { >>> >>> renesas,groups = "mmc0_data8_0", "mmc0_ctrl_0"; >>> renesas,function = "mmc0"; >>> >>> }; >>> cfg { >>> >>> renesas,groups = "mmc0_data8_0"; >>> renesas,pins = "PORT279"; >>> bias-pull-up = <1>; >> >> If I understood your code correctly that last statement can *optionally* >> be written like just: >> >> bias-pull-up; >> >> Without the parameter? >> >> I think that Heiko's implementation does this anyway. > > Yep, with the fixes-series from yesterday the bias-pull-* now have a better > default value of <1>. > > so you can do > bias-pull-up; > which is then identical to the > bias-pull-up = <1>; > above (both are valid of course). A property with a value is an integer. One without is a Boolean. The same property shouldn't be both. Regarding the value representing resistance: That feels pretty odd. Enabling a pullup and selecting the pullup resistance feel like orthogonal options which shouldn't be bound to each-other. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/11/2013 04:03 PM, Laurent Pinchart wrote: > Document DT properties for the generic pinctrl parameters and add a > parser function. > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > +== Generic pinconf parameters == > + > +Pin configuration parameters are expressed by DT properties in the pin > +controller device state nodes and child nodes. For devices that use the generic > +pinconf parameters the following properties are defined. > + > +- tristate: A boolean, put the pin into high impedance state when set. I don't think that a Boolean is appropriate here; it's really a tri-state: * Nothing is specified about the tristate value; don't touch that aspect of HW. * Turn tristate on. * Turn tristate off. This can be a meaningful distinction when relying on e.g. the bootloader having already set up the appropriate tristate value, or when dynamically switching between different pinctrl states and not wanting to affect tristate, or when piecing together multiple DT nodes that describe part of a state, where one node covers just muxing and the other just pin config (e.g. where the mux and pin config configuration registers in HW affect different overlapping groups). > +- schmitt: An integer, enable or disable Schmitt trigger mode for the pins. > + Valid values are > + 0: Schmitt trigger disabled (no hysteresis) > + 1: Schmitt trigger enabled A similar comment applies here. > +- slew-rate: An integer controlling the pin slew rate. Values are device- > + dependent. > + > +- drive-strength: An integer representing the drive strength of pins in mA. > + Valid values are device-dependent. I'm still not convinced that requiring this to be in mA is a good idea. Different HW will use different units for documentation, or even specify no units at all, so it might not always be possible to represent this in terms of mA. Asking for the documentation to be re-written simply to support the DT binding just isn't going to happen. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 19, 2013 at 11:58 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 06/11/2013 04:03 PM, Laurent Pinchart wrote: >> +- tristate: A boolean, put the pin into high impedance state when set. The other patch defines bias-high-impedance which is more likely to be the string used. Anyway: > I don't think that a Boolean is appropriate here; it's really a tri-state: > > * Nothing is specified about the tristate value; don't touch that aspect > of HW. > * Turn tristate on. > * Turn tristate off. I was thinking more about using bias-disable; to explicitly turn that off. But maybe a specific disable option is needed, like for schmitt-trigger (see below). > This can be a meaningful distinction when relying on e.g. the bootloader > having already set up the appropriate tristate value, or when > dynamically switching between different pinctrl states and not wanting > to affect tristate, or when piecing together multiple DT nodes that > describe part of a state, where one node covers just muxing and the > other just pin config (e.g. where the mux and pin config configuration > registers in HW affect different overlapping groups). OK point taken... although when we're dealing with generic pin config the idea is not to cover everything but the majority of not-so-complicated cases. >> +- schmitt: An integer, enable or disable Schmitt trigger mode for the pins. >> + Valid values are >> + 0: Schmitt trigger disabled (no hysteresis) >> + 1: Schmitt trigger enabled > > A similar comment applies here. The other patch adds: input-schmitt-enable; input-schmitt-disable; So this is covered. >> +- slew-rate: An integer controlling the pin slew rate. Values are device- >> + dependent. >> + >> +- drive-strength: An integer representing the drive strength of pins in mA. >> + Valid values are device-dependent. > > I'm still not convinced that requiring this to be in mA is a good idea. > Different HW will use different units for documentation, or even specify > no units at all, so it might not always be possible to represent this in > terms of mA. Asking for the documentation to be re-written simply to > support the DT binding just isn't going to happen. We can add custom DT bindings for these. This is to cover those where we know enough about it to actually use this generic binding. If we don't really know what is happening we may as well call it vendor,foo = <?>; Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index c95ea82..e499ff0 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt @@ -126,3 +126,32 @@ device; they may be grandchildren, for example. Whether this is legal, and whether there is any interaction between the child and intermediate parent nodes, is again defined entirely by the binding for the individual pin controller device. + +== Generic pinconf parameters == + +Pin configuration parameters are expressed by DT properties in the pin +controller device state nodes and child nodes. For devices that use the generic +pinconf parameters the following properties are defined. + +- tristate: A boolean, put the pin into high impedance state when set. + +- pull-up: An integer representing the pull-up strength. 0 disables the pull-up, + non-zero values enable it. + +- pull-down: An integer representing the pull-down strength. 0 disables the + pull-down, non-zero values enables it. + +- schmitt: An integer, enable or disable Schmitt trigger mode for the pins. + Valid values are + 0: Schmitt trigger disabled (no hysteresis) + 1: Schmitt trigger enabled + +- slew-rate: An integer controlling the pin slew rate. Values are device- + dependent. + +- drive-strength: An integer representing the drive strength of pins in mA. + Valid values are device-dependent. + +The pinctrl device DT bindings documentation must list the properties that +apply to the device, and define the valid range for all device-dependent +values. diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c index 2ad5a8d..bd0e41d 100644 --- a/drivers/pinctrl/pinconf-generic.c +++ b/drivers/pinctrl/pinconf-generic.c @@ -15,6 +15,7 @@ #include <linux/module.h> #include <linux/init.h> #include <linux/device.h> +#include <linux/of.h> #include <linux/slab.h> #include <linux/debugfs.h> #include <linux/seq_file.h> @@ -135,3 +136,96 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev, } EXPORT_SYMBOL_GPL(pinconf_generic_dump_config); #endif + +struct pinconf_generic_param { + const char *property; + enum pin_config_param param; + bool flag; +}; + +static const struct pinconf_generic_param pinconf_generic_params[] = { + { "tristate", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, true }, + { "pull-up", PIN_CONFIG_BIAS_PULL_UP, false }, + { "pull-down", PIN_CONFIG_BIAS_PULL_DOWN, false }, + { "schmitt", PIN_CONFIG_INPUT_SCHMITT_ENABLE, true }, + { "slew-rate", PIN_CONFIG_SLEW_RATE, false }, + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, false }, +}; + +static int pinconf_generic_add_config(unsigned long **configs, + unsigned int *num_configs, + unsigned long config) +{ + unsigned int count = *num_configs + 1; + unsigned long *cfgs; + + cfgs = krealloc(*configs, sizeof(*cfgs) * count, GFP_KERNEL); + if (cfgs == NULL) + return -ENOMEM; + + cfgs[count - 1] = config; + + *configs = cfgs; + *num_configs = count; + + return 0; +} + +/** + * pinconf_generic_parse_params - Parse generic pinconf parameters from DT + * @dev: the device, used to print error messages + * @np: the DT node that contains generic pinconf parameters + * @cfgs: the returned array of pinconf parameters + * + * The parameters array is allocated dynamically and returned through the cfgs + * argument. The caller is responsible for freeing the array with kfree(). If no + * configuration parameter is found, or if an error occurs, no parameters array + * will be allocated and the cfgs argument will be set to NULL. + * + * Return the number of configuration parameters successfully parsed, or a + * negative value if an error occurred. Used error codes are + * + * -ENOMEM if memory can't be allocated for the parameters array + */ +int pinconf_generic_parse_params(struct device *dev, struct device_node *np, + unsigned long **cfgs) +{ + unsigned long *configs = NULL; + unsigned int num_configs = 0; + unsigned int i; + int ret; + + for (i = 0; i < ARRAY_SIZE(pinconf_generic_params); ++i) { + const struct pinconf_generic_param *param = + &pinconf_generic_params[i]; + unsigned long config; + u32 val; + + if (param->flag) { + ret = of_property_read_bool(np, param->property) + ? 0 : -EINVAL; + val = 1; + } else { + ret = of_property_read_u32(np, param->property, &val); + } + + if (ret) { + if (ret != -EINVAL) + dev_err(dev, "failed to parse property %s\n", + param->property); + continue; + } + + config = pinconf_to_config_packed(param->param, val); + ret = pinconf_generic_add_config(&configs, &num_configs, config); + if (ret < 0) { + kfree(configs); + *cfgs = NULL; + return ret; + } + } + + *cfgs = configs; + return num_configs; +} +EXPORT_SYMBOL_GPL(pinconf_generic_parse_params); diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h index 92c7267..eb8550b 100644 --- a/drivers/pinctrl/pinconf.h +++ b/drivers/pinctrl/pinconf.h @@ -90,6 +90,23 @@ static inline void pinconf_init_device_debugfs(struct dentry *devroot, * pin config. */ +#if defined(CONFIG_GENERIC_PINCONF) + +int pinconf_generic_parse_params(struct device *dev, struct device_node *np, + unsigned long **cfgs); + +#else + +static inline int pinconf_generic_parse_params(struct device *dev, + struct device_node *np, + unsigned long **cfgs) +{ + *cfgs = NULL; + return 0; +} + +#endif + #if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_DEBUG_FS) void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
Document DT properties for the generic pinctrl parameters and add a parser function. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- .../bindings/pinctrl/pinctrl-bindings.txt | 29 +++++++ drivers/pinctrl/pinconf-generic.c | 94 ++++++++++++++++++++++ drivers/pinctrl/pinconf.h | 17 ++++ 3 files changed, 140 insertions(+) I've successfully tested this patch (or more accurately only the pull-up and pull-down properties) with the Renesas sh-pfc pinctrl device driver. I will resent the sh-pfc DT bindings patch series rebased on the generic pinconf bindings. Not all generic pinconf properties are currently implemented, but I don't think that should be a showstopper. We can add them later as needed. The code is based on both the sh-pfc pinconf DT parser and James Hogan's tz1090 DT parser ("[PATCH v2 6/9] pinctrl-tz1090: add TZ1090 pinctrl driver").