| Message ID | 201306102140.29918.heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 10, 2013 at 9:40 PM, Heiko Stübner <heiko@sntech.de> wrote: > pinconf_generic_parse_dt_config() takes a node as input and generates an > array of generic pinconfig values from the properties of this node. > > As I couldn't find a mechanism to count the number of properties of a node > the function uses internally an array to accept one of parameter and copies > the real present options to a smaller variable at its end. > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> Looks good, patch applied! > +Supported configuration parameters are: > + > +bias-disable - disable any pin bias > +bias-high-impedance - high impedance mode ("third-state", "floating") (...) We should probably document applicable arguments to these, and indicate which ones are boolean. But that can be fixed up later. Yours, Linus Walleij
Hi Heiko, On 10/06/13 20:40, Heiko Stübner wrote: > pinconf_generic_parse_dt_config() takes a node as input and generates an > array of generic pinconfig values from the properties of this node. > > As I couldn't find a mechanism to count the number of properties of a node > the function uses internally an array to accept one of parameter and copies > the real present options to a smaller variable at its end. > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> <snip> > @@ -139,3 +140,83 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev, > } > 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 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 }, > + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 }, > + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 }, > + { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 }, > + { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, > + { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, > + { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, > + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, > + { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, > + { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, > + { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, > + { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, > + { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, > + { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, > + { "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 0 }, > + { "output-low", PIN_CONFIG_OUTPUT, 0, }, > + { "output-high", PIN_CONFIG_OUTPUT, 1, }, shouldn't half of these default to 1 instead of 0? i.e. it's much nicer for the lone flag "bias-pull-up" to enable pull up rather than disable it (you even do this in the DT example in the bindings doc). Otherwise the patch looks good to me (though I haven't tried it yet). Cheers James
Hi James, Am Mittwoch, 12. Juni 2013, 16:55:12 schrieb James Hogan: > Hi Heiko, > > On 10/06/13 20:40, Heiko Stübner wrote: > > pinconf_generic_parse_dt_config() takes a node as input and generates an > > array of generic pinconfig values from the properties of this node. > > > > As I couldn't find a mechanism to count the number of properties of a > > node the function uses internally an array to accept one of parameter > > and copies the real present options to a smaller variable at its end. > > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > <snip> > > > @@ -139,3 +140,83 @@ void pinconf_generic_dump_config(struct pinctrl_dev > > *pctldev, > > > > } > > 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 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 }, > > + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 }, > > + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 }, > > + { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 }, > > + { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, > > + { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, > > + { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, > > + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, > > + { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, > > + { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, > > + { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, > > + { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, > > + { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, > > + { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, > > + { "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 0 }, > > + { "output-low", PIN_CONFIG_OUTPUT, 0, }, > > + { "output-high", PIN_CONFIG_OUTPUT, 1, }, > > shouldn't half of these default to 1 instead of 0? i.e. it's much nicer > for the lone flag "bias-pull-up" to enable pull up rather than disable > it (you even do this in the DT example in the bindings doc). on closer inspection it seems that you may be right. The documentation to the options in the pinconf-generic header even tells that for example the pull options do have a 0 or 1 argument. I guess I got much inspiration from sh-pfc/pinctrl.c when learning about the generic pinconf, which ignores any argument for these options, which I then have mimiced in my rockchip driver and here. But I'm not sure if I understand everything correctly :-) ... isn't the bias- disable the opposite of turning on a pull (like the sh-pfc/pinctrl does) and same with switching from one pull type to another, i.e. activating a pull up would turn off a pull down and on the whole making the argument redundant? The only other candidate I could find was low-power-mode which really could use a "1" as default. All the other pinconf options either use custom arguments or ignore teir argument. > Otherwise the patch looks good to me (though I haven't tried it yet). nice Heiko
Tisdagen den 13:e Juni 2013 klock 12:22 AM, skrev Heiko Stübner <heiko@sntech.de>: > Am Mittwoch, 12. Juni 2013, 16:55:12 schrieb James Hogan: >> > +static 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 }, >> > + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 }, >> > + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 }, >> > + { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 }, >> > + { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, >> > + { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, >> > + { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, >> > + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, >> > + { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, >> > + { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, >> > + { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, >> > + { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, >> > + { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, >> > + { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, >> > + { "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 0 }, >> > + { "output-low", PIN_CONFIG_OUTPUT, 0, }, >> > + { "output-high", PIN_CONFIG_OUTPUT, 1, }, >> >> shouldn't half of these default to 1 instead of 0? i.e. it's much nicer >> for the lone flag "bias-pull-up" to enable pull up rather than disable >> it (you even do this in the DT example in the bindings doc). > > on closer inspection it seems that you may be right. Heiko can you write a patch for this? You can hit both this code and the Rockchip driver at the same time for sure. Please check that the bindings are consistent. > The documentation to the > options in the pinconf-generic header even tells that for example the pull > options do have a 0 or 1 argument. Yeah. Well. Actually there has been plans to have the argument represent the number of Ohms on the pull-up, but we haven't seen any hardware that can actually select that. Maybe we should add that now? It will still be that != 0 implies enablement on platforms that does not support specifying the pull up/down resistance. > But I'm not sure if I understand everything correctly :-) ... isn't the bias- > disable the opposite of turning on a pull (like the sh-pfc/pinctrl does) and > same with switching from one pull type to another, i.e. activating a pull up > would turn off a pull down and on the whole making the argument redundant? This is true, and the plan is surely for the core to not allow or print a big fat warning if someone does something really stupid like activate pull up and pull down at the same time (unless s/he's constructing a heater radiator or something). Currently we don't make any sanity checks like that, BUT your generic parser could actually be extended to do that. Patches welcome ;-) > The only other candidate I could find was low-power-mode which really could > use a "1" as default. All the other pinconf options either use custom > arguments or ignore teir argument. A "1" for what? Not quite following.... Yours, Linus Walleij
Am Donnerstag, 13. Juni 2013, 10:11:28 schrieb Linus Walleij: > Tisdagen den 13:e Juni 2013 klock 12:22 AM, skrev Heiko Stübner > > <heiko@sntech.de>: > > Am Mittwoch, 12. Juni 2013, 16:55:12 schrieb James Hogan: > >> > +static 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 }, > >> > + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 }, > >> > + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 }, > >> > + { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 }, > >> > + { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, > >> > + { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, > >> > + { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, > >> > + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, > >> > + { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, > >> > + { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, > >> > + { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, > >> > + { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, > >> > + { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, > >> > + { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, > >> > + { "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 0 }, > >> > + { "output-low", PIN_CONFIG_OUTPUT, 0, }, > >> > + { "output-high", PIN_CONFIG_OUTPUT, 1, }, > >> > >> shouldn't half of these default to 1 instead of 0? i.e. it's much nicer > >> for the lone flag "bias-pull-up" to enable pull up rather than disable > >> it (you even do this in the DT example in the bindings doc). > > > > on closer inspection it seems that you may be right. > > Heiko can you write a patch for this? You can hit both this code and > the Rockchip driver at the same time for sure. Please check that > the bindings are consistent. > > > The documentation to the > > options in the pinconf-generic header even tells that for example the > > pull options do have a 0 or 1 argument. > > Yeah. Well. > > Actually there has been plans to have the argument represent the > number of Ohms on the pull-up, but we haven't seen any hardware > that can actually select that. > > Maybe we should add that now? It will still be that != 0 implies > enablement on platforms that does not support specifying the > pull up/down resistance. Ok, I'll see that I get this fixed :-) > > > But I'm not sure if I understand everything correctly :-) ... isn't the > > bias- disable the opposite of turning on a pull (like the sh-pfc/pinctrl > > does) and same with switching from one pull type to another, i.e. > > activating a pull up would turn off a pull down and on the whole making > > the argument redundant? > > This is true, and the plan is surely for the core to not allow or print > a big fat warning if someone does something really stupid like > activate pull up and pull down at the same time (unless s/he's > constructing a heater radiator or something). > > Currently we don't make any sanity checks like that, BUT your > generic parser could actually be extended to do that. > > Patches welcome ;-) I don't seem to get of the hook here ;-) But I'll try to fix the issue above first. > > The only other candidate I could find was low-power-mode which really > > could use a "1" as default. All the other pinconf options either use > > custom arguments or ignore teir argument. > > A "1" for what? Not quite following.... According to the pinconf header docs, low-power-mode also expects an argument of 1 or 0. So it's default value should change too ... or we could rename the property, like "low-power-enable" and "low-power-disable", which might make the dt more readable than an arbitary low-power-mode = <0>; Heiko
Am Donnerstag, 13. Juni 2013, 16:35:20 schrieb Heiko Stübner: > Am Donnerstag, 13. Juni 2013, 10:11:28 schrieb Linus Walleij: > > Tisdagen den 13:e Juni 2013 klock 12:22 AM, skrev Heiko Stübner > > > > <heiko@sntech.de>: > > > Am Mittwoch, 12. Juni 2013, 16:55:12 schrieb James Hogan: > > >> > +static 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 }, > > >> > + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 }, > > >> > + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 }, > > >> > + { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 > > >> > }, + { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, > > >> > + { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, > > >> > + { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, > > >> > + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, > > >> > + { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, > > >> > + { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, > > >> > + { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, > > >> > + { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, > > >> > + { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, > > >> > + { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, > > >> > + { "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 0 }, > > >> > + { "output-low", PIN_CONFIG_OUTPUT, 0, }, > > >> > + { "output-high", PIN_CONFIG_OUTPUT, 1, }, > > >> > > >> shouldn't half of these default to 1 instead of 0? i.e. it's much > > >> nicer for the lone flag "bias-pull-up" to enable pull up rather than > > >> disable it (you even do this in the DT example in the bindings doc). > > > > > > on closer inspection it seems that you may be right. > > > > Heiko can you write a patch for this? You can hit both this code and > > the Rockchip driver at the same time for sure. Please check that > > the bindings are consistent. > > > > > The documentation to the > > > options in the pinconf-generic header even tells that for example the > > > pull options do have a 0 or 1 argument. > > > > Yeah. Well. > > > > Actually there has been plans to have the argument represent the > > number of Ohms on the pull-up, but we haven't seen any hardware > > that can actually select that. > > > > Maybe we should add that now? It will still be that != 0 implies > > enablement on platforms that does not support specifying the > > pull up/down resistance. > > Ok, I'll see that I get this fixed :-) Hmm ... what is the meaning of the argument of bias-disable and bias-high- impedance, as the kernel-doc in pinconf-generic.h does not tell? bias-bus-hold ignores its argument and we already clarified that the pull-* do have != 0 or 0 argument. Thanks Heiko
On Thu, Jun 13, 2013 at 4:35 PM, Heiko Stübner <heiko@sntech.de> wrote: > Am Donnerstag, 13. Juni 2013, 10:11:28 schrieb Linus Walleij: >> Tisdagen den 13:e Juni 2013 klock 12:22 AM, skrev Heiko Stübner >> > The only other candidate I could find was low-power-mode which really >> > could use a "1" as default. All the other pinconf options either use >> > custom arguments or ignore teir argument. >> >> A "1" for what? Not quite following.... > > According to the pinconf header docs, low-power-mode also expects an argument > of 1 or 0. So it's default value should change too ... or we could rename the > property, like "low-power-enable" and "low-power-disable", which might make > the dt more readable than an arbitary low-power-mode = <0>; Oh yes, sorry, of course. Go ahead with this. Yours, Linus Walleij
On Thu, Jun 13, 2013 at 5:23 PM, Heiko Stübner <heiko@sntech.de> wrote: >> Ok, I'll see that I get this fixed :-) > > Hmm ... what is the meaning of the argument of bias-disable and bias-high- > impedance, as the kernel-doc in pinconf-generic.h does not tell? I think those arguments are N/A, ignored, doesn't matter. If these options were typed, they would be bool. Please improve documentation if you can... sorry for all the rough edges. > bias-bus-hold ignores its argument and we already clarified that the pull-* do > have != 0 or 0 argument. I think in the DT binding, both these forms: bias-pull-up; bias-pull-up = <150000>; Should be allowed. So when parsing, you first check if it exists, then if there is an argument, if there is no value supplied, just set it to 1, as that is clearly != 0... Yours, Linus Walleij
Hi Linus, On Thursday 13 June 2013 17:36:00 Linus Walleij wrote: > On Thu, Jun 13, 2013 at 5:23 PM, Heiko Stübner <heiko@sntech.de> wrote: > >> Ok, I'll see that I get this fixed :-) > > > > Hmm ... what is the meaning of the argument of bias-disable and bias-high- > > impedance, as the kernel-doc in pinconf-generic.h does not tell? > > I think those arguments are N/A, ignored, doesn't matter. > If these options were typed, they would be bool. > > Please improve documentation if you can... sorry for all the > rough edges. > > > bias-bus-hold ignores its argument and we already clarified that the > > pull-* do have != 0 or 0 argument. > > I think in the DT binding, both these forms: > > bias-pull-up; > bias-pull-up = <150000>; > > Should be allowed. > > So when parsing, you first check if it exists, then if there > is an argument, if there is no value supplied, just set it > to 1, as that is clearly != 0... What's the expected way to disable pull-ups in DT ? Should it be 'bias-pull-up = <0>;' or 'bias-disable;' ?
Hi Heiko, Thank you for the patch. I've tested it on an sh73a0 KZM9G board with the sh- pfc driver and it seems to work fine. Please see the code below for comments. On Monday 10 June 2013 21:40:29 Heiko Stübner wrote: > pinconf_generic_parse_dt_config() takes a node as input and generates an > array of generic pinconfig values from the properties of this node. > > As I couldn't find a mechanism to count the number of properties of a node > the function uses internally an array to accept one of parameter and copies > the real present options to a smaller variable at its end. > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > .../bindings/pinctrl/pinctrl-bindings.txt | 38 +++++++++ > drivers/pinctrl/pinconf-generic.c | 81 +++++++++++++++++ > drivers/pinctrl/pinconf.h | 6 ++ > 3 files changed, 125 insertions(+), 0 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index > c95ea82..ef7cd57 100644 > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > @@ -126,3 +126,41 @@ 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. > + > +== Using generic pinconfig options == > + > +Generic pinconfig parameters can be used by defining a separate node > containing +the applicable parameters (and optional values), like: > + > +pcfg_pull_up: pcfg_pull_up { > + bias-pull-up; > + drive-strength = <20>; > +}; > + > +This node should then be referenced in the appropriate pinctrl node as a > phandle +and parsed in the driver using the pinconf_generic_parse_dt_config > function. + > +Supported configuration parameters are: > + > +bias-disable - disable any pin bias > +bias-high-impedance - high impedance mode ("third-state", "floating") > +bias-bus-hold - latch weakly > +bias-pull-up - pull up the pin > +bias-pull-down - pull down the pin > +bias-pull-pin-default - use pin-default pull state > +drive-push-pull - drive actively high and low > +drive-open-drain - drive with open drain > +drive-open-source - drive with open source > +drive-strength - sink or source at most X mA > +input-schmitt-enable - enable schmitt-trigger mode > +input-schmitt-disable - disable schmitt-trigger mode > +input-schmitt - run in schmitt-trigger mode with hysteresis X > +input-debounce - debounce mode with debound time X > +power-source - select power source X > +slew-rate - use slew-rate X > +low-power-mode - low power mode > +output-low - set the pin to output mode with low level > +output-high - set the pin to output mode with high level > + > +More in-depth documentation on these parameters can be found in > +<include/linux/pinctrl/pinconfig-generic.h> > diff --git a/drivers/pinctrl/pinconf-generic.c > b/drivers/pinctrl/pinconf-generic.c index 9a6812b..3610e7b 100644 > --- a/drivers/pinctrl/pinconf-generic.c > +++ b/drivers/pinctrl/pinconf-generic.c > @@ -21,6 +21,7 @@ > #include <linux/pinctrl/pinctrl.h> > #include <linux/pinctrl/pinconf.h> > #include <linux/pinctrl/pinconf-generic.h> > +#include <linux/of.h> > #include "core.h" > #include "pinconf.h" > > @@ -139,3 +140,83 @@ void pinconf_generic_dump_config(struct pinctrl_dev > *pctldev, } > 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 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 }, > + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 }, > + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 }, > + { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 }, > + { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, > + { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, > + { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, > + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, > + { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, > + { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, > + { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, > + { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, > + { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, > + { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, > + { "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 0 }, > + { "output-low", PIN_CONFIG_OUTPUT, 0, }, > + { "output-high", PIN_CONFIG_OUTPUT, 1, }, > +}; > + > +/** > + * pinconf_generic_parse_dt_config() > + * parse the config properties into generic pinconfig values. > + * @np: node containing the pinconfig properties > + * @configs: array with nconfigs entries containing the generic pinconf > values + * @nconfigs: umber of configurations > + */ > +int pinconf_generic_parse_dt_config(struct device_node *np, > + unsigned long **configs, > + unsigned int *nconfigs) > +{ > + unsigned long cfg[ARRAY_SIZE(dt_params)]; I'm a bit uneasy about allocating large arrays on the stack. Would it be better to dynamically allocate cfg ? I've used kzrealloc in my implementation to grow the config array every time a config was found, but that might not be the most efficient implementation, although I wonder how many configuration options we will see in practice in a single node. > + unsigned int ncfg = 0; > + int ret; > + int i; > + u32 val; > + > + if (!np) > + return -EINVAL; > + > + for (i = 0; i < ARRAY_SIZE(dt_params); i++) { > + 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++; > + } You could add if (ncfg == 0) { *configs = NULL; *nconfigs = 0; return 0; } here. Most of the issues I wanted to raise have already been addressed by comments sent to the list. Do you plan to send a v2 in the near future ? > + > + /* > + * Now limit the number of configs to the real number of > + * found properties. > + */ > + *configs = kzalloc(ncfg * sizeof(unsigned long), GFP_KERNEL); > + if (!*configs) > + return -ENOMEM; > + > + memcpy(*configs, &cfg, ncfg * sizeof(unsigned long)); > + *nconfigs = ncfg; > + return 0; > +} > +#endif > diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h > index 92c7267..a4a5417 100644 > --- a/drivers/pinctrl/pinconf.h > +++ b/drivers/pinctrl/pinconf.h > @@ -123,3 +123,9 @@ static inline void pinconf_generic_dump_config(struct > pinctrl_dev *pctldev, return; > } > #endif > + > +#if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_OF) > +int pinconf_generic_parse_dt_config(struct device_node *np, > + unsigned long **configs, > + unsigned int *nconfigs); > +#endif
Hi Laurent, Am Freitag, 14. Juni 2013, 02:27:01 schrieb Laurent Pinchart: > Hi Heiko, > > Thank you for the patch. I've tested it on an sh73a0 KZM9G board with the > sh- pfc driver and it seems to work fine. Please see the code below for > comments. > > On Monday 10 June 2013 21:40:29 Heiko Stübner wrote: > > pinconf_generic_parse_dt_config() takes a node as input and generates an > > array of generic pinconfig values from the properties of this node. > > > > As I couldn't find a mechanism to count the number of properties of a > > node the function uses internally an array to accept one of parameter > > and copies the real present options to a smaller variable at its end. > > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > --- > > > > .../bindings/pinctrl/pinctrl-bindings.txt | 38 +++++++++ > > drivers/pinctrl/pinconf-generic.c | 81 > > +++++++++++++++++ drivers/pinctrl/pinconf.h | > > 6 ++ > > 3 files changed, 125 insertions(+), 0 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index > > c95ea82..ef7cd57 100644 > > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > @@ -126,3 +126,41 @@ 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. > > + > > +== Using generic pinconfig options == > > + > > +Generic pinconfig parameters can be used by defining a separate node > > containing +the applicable parameters (and optional values), like: > > + > > +pcfg_pull_up: pcfg_pull_up { > > + bias-pull-up; > > + drive-strength = <20>; > > +}; > > + > > +This node should then be referenced in the appropriate pinctrl node as a > > phandle +and parsed in the driver using the > > pinconf_generic_parse_dt_config function. + > > +Supported configuration parameters are: > > + > > +bias-disable - disable any pin bias > > +bias-high-impedance - high impedance mode ("third-state", "floating") > > +bias-bus-hold - latch weakly > > +bias-pull-up - pull up the pin > > +bias-pull-down - pull down the pin > > +bias-pull-pin-default - use pin-default pull state > > +drive-push-pull - drive actively high and low > > +drive-open-drain - drive with open drain > > +drive-open-source - drive with open source > > +drive-strength - sink or source at most X mA > > +input-schmitt-enable - enable schmitt-trigger mode > > +input-schmitt-disable - disable schmitt-trigger mode > > +input-schmitt - run in schmitt-trigger mode with hysteresis X > > +input-debounce - debounce mode with debound time X > > +power-source - select power source X > > +slew-rate - use slew-rate X > > +low-power-mode - low power mode > > +output-low - set the pin to output mode with low level > > +output-high - set the pin to output mode with high level > > + > > +More in-depth documentation on these parameters can be found in > > +<include/linux/pinctrl/pinconfig-generic.h> > > diff --git a/drivers/pinctrl/pinconf-generic.c > > b/drivers/pinctrl/pinconf-generic.c index 9a6812b..3610e7b 100644 > > --- a/drivers/pinctrl/pinconf-generic.c > > +++ b/drivers/pinctrl/pinconf-generic.c > > @@ -21,6 +21,7 @@ > > > > #include <linux/pinctrl/pinctrl.h> > > #include <linux/pinctrl/pinconf.h> > > #include <linux/pinctrl/pinconf-generic.h> > > > > +#include <linux/of.h> > > > > #include "core.h" > > #include "pinconf.h" > > > > @@ -139,3 +140,83 @@ void pinconf_generic_dump_config(struct pinctrl_dev > > *pctldev, } > > > > 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 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 }, > > + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 }, > > + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 }, > > + { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 }, > > + { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, > > + { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, > > + { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, > > + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, > > + { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, > > + { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, > > + { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, > > + { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, > > + { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, > > + { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, > > + { "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 0 }, > > + { "output-low", PIN_CONFIG_OUTPUT, 0, }, > > + { "output-high", PIN_CONFIG_OUTPUT, 1, }, > > +}; > > + > > +/** > > + * pinconf_generic_parse_dt_config() > > + * parse the config properties into generic pinconfig values. > > + * @np: node containing the pinconfig properties > > + * @configs: array with nconfigs entries containing the generic pinconf > > values + * @nconfigs: umber of configurations > > + */ > > +int pinconf_generic_parse_dt_config(struct device_node *np, > > + unsigned long **configs, > > + unsigned int *nconfigs) > > +{ > > + unsigned long cfg[ARRAY_SIZE(dt_params)]; > > I'm a bit uneasy about allocating large arrays on the stack. Would it be > better to dynamically allocate cfg ? I've used kzrealloc in my > implementation to grow the config array every time a config was found, but > that might not be the most efficient implementation, although I wonder how > many configuration options we will see in practice in a single node. Personally I'm not sure ... using kzrealloc once for each found property like I saw it in your patch feels somehow slow, while my big array might cause other problems. If there was a way to count properties in a dt node this would solve the problem, aka alloc an array of the number of properties, parse props and then move to correct sized array when we know the exact number of found ones. But I hadn't found a way to get the number of properties and trying to write my own iterating over the properties did result in strange numbers, probably thru inheritance of properties. > > + unsigned int ncfg = 0; > > + int ret; > > + int i; > > + u32 val; > > + > > + if (!np) > > + return -EINVAL; > > + > > + for (i = 0; i < ARRAY_SIZE(dt_params); i++) { > > + 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++; > > + } > > You could add > > if (ncfg == 0) { > *configs = NULL; > *nconfigs = 0; > return 0; > } > > here. > > Most of the issues I wanted to raise have already been addressed by > comments sent to the list. Do you plan to send a v2 in the near future ? According to Linus this is already in his tree, so I'm currently working on fixup patches for the issues. Should be done hopefully today. Heiko
Am Freitag, 14. Juni 2013, 01:53:49 schrieb Laurent Pinchart: > Hi Linus, > > On Thursday 13 June 2013 17:36:00 Linus Walleij wrote: > > On Thu, Jun 13, 2013 at 5:23 PM, Heiko Stübner <heiko@sntech.de> wrote: > > >> Ok, I'll see that I get this fixed :-) > > > > > > Hmm ... what is the meaning of the argument of bias-disable and > > > bias-high- impedance, as the kernel-doc in pinconf-generic.h does not > > > tell? > > > > I think those arguments are N/A, ignored, doesn't matter. > > If these options were typed, they would be bool. > > > > Please improve documentation if you can... sorry for all the > > rough edges. > > > > > bias-bus-hold ignores its argument and we already clarified that the > > > pull-* do have != 0 or 0 argument. > > > > I think in the DT binding, both these forms: > > > > bias-pull-up; > > bias-pull-up = <150000>; > > > > Should be allowed. > > > > So when parsing, you first check if it exists, then if there > > is an argument, if there is no value supplied, just set it > > to 1, as that is clearly != 0... > > What's the expected way to disable pull-ups in DT ? Should it be > 'bias-pull-up = <0>;' or 'bias-disable;' ? According to the kernedoc I think both are valid and should be handled. Using bias-disable is more descriptive but would also include disabling a "high- impedance" or "bus-hold" bias (if supported by the hardware). Personally, for my rockchip stuff I go with using the "bias-pull-pin-default" <-> "bias-disable".
Hi Heiko, On Friday 14 June 2013 09:34:14 Heiko Stübner wrote: > Am Freitag, 14. Juni 2013, 02:27:01 schrieb Laurent Pinchart: > > Hi Heiko, > > > > Thank you for the patch. I've tested it on an sh73a0 KZM9G board with the > > sh- pfc driver and it seems to work fine. Please see the code below for > > comments. > > > > On Monday 10 June 2013 21:40:29 Heiko Stübner wrote: > > > pinconf_generic_parse_dt_config() takes a node as input and generates an > > > array of generic pinconfig values from the properties of this node. > > > > > > As I couldn't find a mechanism to count the number of properties of a > > > node the function uses internally an array to accept one of parameter > > > and copies the real present options to a smaller variable at its end. > > > > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > > --- > > > > > > .../bindings/pinctrl/pinctrl-bindings.txt | 38 +++++++++ > > > drivers/pinctrl/pinconf-generic.c | 81 +++++++++++++ > > > drivers/pinctrl/pinconf.h | 6 ++ > > > > > > 3 files changed, 125 insertions(+), 0 deletions(-) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > > b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index > > > c95ea82..ef7cd57 100644 > > > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > > > @@ -126,3 +126,41 @@ 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. > > > + > > > +== Using generic pinconfig options == > > > + > > > +Generic pinconfig parameters can be used by defining a separate node > > > containing +the applicable parameters (and optional values), like: > > > + > > > +pcfg_pull_up: pcfg_pull_up { > > > + bias-pull-up; > > > + drive-strength = <20>; > > > +}; > > > + > > > +This node should then be referenced in the appropriate pinctrl node as > > > a > > > phandle +and parsed in the driver using the > > > pinconf_generic_parse_dt_config function. + > > > +Supported configuration parameters are: > > > + > > > +bias-disable - disable any pin bias > > > +bias-high-impedance - high impedance mode ("third-state", "floating") > > > +bias-bus-hold - latch weakly > > > +bias-pull-up - pull up the pin > > > +bias-pull-down - pull down the pin > > > +bias-pull-pin-default - use pin-default pull state > > > +drive-push-pull - drive actively high and low > > > +drive-open-drain - drive with open drain > > > +drive-open-source - drive with open source > > > +drive-strength - sink or source at most X mA > > > +input-schmitt-enable - enable schmitt-trigger mode > > > +input-schmitt-disable - disable schmitt-trigger mode > > > +input-schmitt - run in schmitt-trigger mode with hysteresis X > > > +input-debounce - debounce mode with debound time X > > > +power-source - select power source X > > > +slew-rate - use slew-rate X > > > +low-power-mode - low power mode > > > +output-low - set the pin to output mode with low level > > > +output-high - set the pin to output mode with high level > > > + > > > +More in-depth documentation on these parameters can be found in > > > +<include/linux/pinctrl/pinconfig-generic.h> > > > diff --git a/drivers/pinctrl/pinconf-generic.c > > > b/drivers/pinctrl/pinconf-generic.c index 9a6812b..3610e7b 100644 > > > --- a/drivers/pinctrl/pinconf-generic.c > > > +++ b/drivers/pinctrl/pinconf-generic.c > > > @@ -21,6 +21,7 @@ > > > > > > #include <linux/pinctrl/pinctrl.h> > > > #include <linux/pinctrl/pinconf.h> > > > #include <linux/pinctrl/pinconf-generic.h> > > > > > > +#include <linux/of.h> > > > > > > #include "core.h" > > > #include "pinconf.h" > > > > > > @@ -139,3 +140,83 @@ void pinconf_generic_dump_config(struct pinctrl_dev > > > *pctldev, } > > > > > > 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 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 }, > > > + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 }, > > > + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 }, > > > + { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 }, > > > + { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, > > > + { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, > > > + { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, > > > + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, > > > + { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, > > > + { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, > > > + { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, > > > + { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, > > > + { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, > > > + { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, > > > + { "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 0 }, > > > + { "output-low", PIN_CONFIG_OUTPUT, 0, }, > > > + { "output-high", PIN_CONFIG_OUTPUT, 1, }, > > > +}; > > > + > > > +/** > > > + * pinconf_generic_parse_dt_config() > > > + * parse the config properties into generic pinconfig values. > > > + * @np: node containing the pinconfig properties > > > + * @configs: array with nconfigs entries containing the generic pinconf > > > values + * @nconfigs: umber of configurations > > > + */ > > > +int pinconf_generic_parse_dt_config(struct device_node *np, > > > + unsigned long **configs, > > > + unsigned int *nconfigs) > > > +{ > > > + unsigned long cfg[ARRAY_SIZE(dt_params)]; > > > > I'm a bit uneasy about allocating large arrays on the stack. Would it be > > better to dynamically allocate cfg ? I've used kzrealloc in my > > implementation to grow the config array every time a config was found, but > > that might not be the most efficient implementation, although I wonder how > > many configuration options we will see in practice in a single node. > > Personally I'm not sure ... using kzrealloc once for each found property > like I saw it in your patch feels somehow slow, while my big array might > cause other problems. What about allocating the larger array dynamically instead ? Something like unsigned long *cfg; cfg = kzalloc(sizeof(*cfg) * ARRAY_SIZE(dt_params), GFP_KERNEL); ... (after allocating the returned array and copying data over) kfree(cfg); > If there was a way to count properties in a dt node this would solve the > problem, aka alloc an array of the number of properties, parse props and > then move to correct sized array when we know the exact number of found > ones. > > But I hadn't found a way to get the number of properties and trying to > write my own iterating over the properties did result in strange numbers, > probably thru inheritance of properties. > > > > + unsigned int ncfg = 0; > > > + int ret; > > > + int i; > > > + u32 val; > > > + > > > + if (!np) > > > + return -EINVAL; > > > + > > > + for (i = 0; i < ARRAY_SIZE(dt_params); i++) { > > > + 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++; > > > + } > > > > You could add > > > > if (ncfg == 0) { > > > > *configs = NULL; > > *nconfigs = 0; > > return 0; > > > > } > > > > here. > > > > Most of the issues I wanted to raise have already been addressed by > > comments sent to the list. Do you plan to send a v2 in the near future ? > > According to Linus this is already in his tree, so I'm currently working on > fixup patches for the issues. Should be done hopefully today. Sounds good to me.
Hi Heiko, On Friday 14 June 2013 11:18:22 Heiko Stübner wrote: > Am Freitag, 14. Juni 2013, 01:53:49 schrieb Laurent Pinchart: > > On Thursday 13 June 2013 17:36:00 Linus Walleij wrote: > > > On Thu, Jun 13, 2013 at 5:23 PM, Heiko Stübner <heiko@sntech.de> wrote: > > > >> Ok, I'll see that I get this fixed :-) > > > > > > > > Hmm ... what is the meaning of the argument of bias-disable and > > > > bias-high- impedance, as the kernel-doc in pinconf-generic.h does not > > > > tell? > > > > > > I think those arguments are N/A, ignored, doesn't matter. > > > If these options were typed, they would be bool. > > > > > > Please improve documentation if you can... sorry for all the > > > rough edges. > > > > > > > bias-bus-hold ignores its argument and we already clarified that the > > > > pull-* do have != 0 or 0 argument. > > > > > > I think in the DT binding, both these forms: > > > > > > bias-pull-up; > > > bias-pull-up = <150000>; > > > > > > Should be allowed. > > > > > > So when parsing, you first check if it exists, then if there > > > is an argument, if there is no value supplied, just set it > > > to 1, as that is clearly != 0... > > > > What's the expected way to disable pull-ups in DT ? Should it be > > 'bias-pull-up = <0>;' or 'bias-disable;' ? > > According to the kernedoc I think both are valid and should be handled. > Using bias-disable is more descriptive but would also include disabling a > "high-impedance" or "bus-hold" bias (if supported by the hardware). OK. I still fail to see how the various bias options are supposed to interract together, but I don't think I'll get an answer on that. > Personally, for my rockchip stuff I go with using the > "bias-pull-pin-default" <-> "bias-disable".
On Fri, Jun 14, 2013 at 4:52 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Friday 14 June 2013 11:18:22 Heiko Stübner wrote: >> > What's the expected way to disable pull-ups in DT ? Should it be >> > 'bias-pull-up = <0>;' or 'bias-disable;' ? >> >> According to the kernedoc I think both are valid and should be handled. >> Using bias-disable is more descriptive but would also include disabling a >> "high-impedance" or "bus-hold" bias (if supported by the hardware). > > OK. I still fail to see how the various bias options are supposed to interract > together, but I don't think I'll get an answer on that. > >> Personally, for my rockchip stuff I go with using the >> "bias-pull-pin-default" <-> "bias-disable". I think bias-pull-up = <0>; should actually mean "connect the pin to VDD" and bias-pull-down = <0>; should mean "ground the pin". This makes a lot of sense from the electronics side of things, let's not create a terminology that appeals to programmers, such that "zero always means boolean disable", because we're dealing with electronics here. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index c95ea82..ef7cd57 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt @@ -126,3 +126,41 @@ 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. + +== Using generic pinconfig options == + +Generic pinconfig parameters can be used by defining a separate node containing +the applicable parameters (and optional values), like: + +pcfg_pull_up: pcfg_pull_up { + bias-pull-up; + drive-strength = <20>; +}; + +This node should then be referenced in the appropriate pinctrl node as a phandle +and parsed in the driver using the pinconf_generic_parse_dt_config function. + +Supported configuration parameters are: + +bias-disable - disable any pin bias +bias-high-impedance - high impedance mode ("third-state", "floating") +bias-bus-hold - latch weakly +bias-pull-up - pull up the pin +bias-pull-down - pull down the pin +bias-pull-pin-default - use pin-default pull state +drive-push-pull - drive actively high and low +drive-open-drain - drive with open drain +drive-open-source - drive with open source +drive-strength - sink or source at most X mA +input-schmitt-enable - enable schmitt-trigger mode +input-schmitt-disable - disable schmitt-trigger mode +input-schmitt - run in schmitt-trigger mode with hysteresis X +input-debounce - debounce mode with debound time X +power-source - select power source X +slew-rate - use slew-rate X +low-power-mode - low power mode +output-low - set the pin to output mode with low level +output-high - set the pin to output mode with high level + +More in-depth documentation on these parameters can be found in +<include/linux/pinctrl/pinconfig-generic.h> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c index 9a6812b..3610e7b 100644 --- a/drivers/pinctrl/pinconf-generic.c +++ b/drivers/pinctrl/pinconf-generic.c @@ -21,6 +21,7 @@ #include <linux/pinctrl/pinctrl.h> #include <linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinconf-generic.h> +#include <linux/of.h> #include "core.h" #include "pinconf.h" @@ -139,3 +140,83 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev, } 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 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 }, + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 }, + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 }, + { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 }, + { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, + { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, + { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, + { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, + { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, + { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, + { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, + { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, + { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, + { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, + { "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 0 }, + { "output-low", PIN_CONFIG_OUTPUT, 0, }, + { "output-high", PIN_CONFIG_OUTPUT, 1, }, +}; + +/** + * pinconf_generic_parse_dt_config() + * parse the config properties into generic pinconfig values. + * @np: node containing the pinconfig properties + * @configs: array with nconfigs entries containing the generic pinconf values + * @nconfigs: umber of configurations + */ +int pinconf_generic_parse_dt_config(struct device_node *np, + unsigned long **configs, + unsigned int *nconfigs) +{ + unsigned long cfg[ARRAY_SIZE(dt_params)]; + unsigned int ncfg = 0; + int ret; + int i; + u32 val; + + if (!np) + return -EINVAL; + + for (i = 0; i < ARRAY_SIZE(dt_params); i++) { + 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++; + } + + /* + * Now limit the number of configs to the real number of + * found properties. + */ + *configs = kzalloc(ncfg * sizeof(unsigned long), GFP_KERNEL); + if (!*configs) + return -ENOMEM; + + memcpy(*configs, &cfg, ncfg * sizeof(unsigned long)); + *nconfigs = ncfg; + return 0; +} +#endif diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h index 92c7267..a4a5417 100644 --- a/drivers/pinctrl/pinconf.h +++ b/drivers/pinctrl/pinconf.h @@ -123,3 +123,9 @@ static inline void pinconf_generic_dump_config(struct pinctrl_dev *pctldev, return; } #endif + +#if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_OF) +int pinconf_generic_parse_dt_config(struct device_node *np, + unsigned long **configs, + unsigned int *nconfigs); +#endif
pinconf_generic_parse_dt_config() takes a node as input and generates an array of generic pinconfig values from the properties of this node. As I couldn't find a mechanism to count the number of properties of a node the function uses internally an array to accept one of parameter and copies the real present options to a smaller variable at its end. Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- .../bindings/pinctrl/pinctrl-bindings.txt | 38 +++++++++ drivers/pinctrl/pinconf-generic.c | 81 ++++++++++++++++++++ drivers/pinctrl/pinconf.h | 6 ++ 3 files changed, 125 insertions(+), 0 deletions(-)