Messages in this thread | | | From | Grant Likely <> | Subject | Re: [RFC] pinctrl: generic: Add DT bindings | Date | Wed, 12 Jun 2013 13:48:33 +0100 |
| |
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.
| |