| Message ID | 201306141742.49923.heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 14, 2013 at 5:42 PM, Heiko Stübner <heiko@sntech.de> wrote: > The bias-pull-* options use values > 0 to indicate that the pull should > be activated and optionally also indicate the strength of the pull. > Therefore use an default value of 1 for these options. > > Split the low-power-mode option into low-power-enable and -disable. > > Update the documentation to describe the param arguments better. > > Wrong default options > Reported-by: James Hogan <james.hogan@imgtec.com> > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> Patch applied. Yours, Linus Walleij
On 06/14/2013 09:42 AM, Heiko Stübner wrote: > The bias-pull-* options use values > 0 to indicate that the pull should > be activated and optionally also indicate the strength of the pull. > Therefore use an default value of 1 for these options. > > Split the low-power-mode option into low-power-enable and -disable. > > Update the documentation to describe the param arguments better. > > Wrong default options > Reported-by: James Hogan <james.hogan@imgtec.com> > That blank line should be before the Reported-by not after it. > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > -low-power-mode - low power mode > +low-power-enable - enable low power mode > +low-power-disable - disable low power mode Hmmm. That's changing the binding definition. What if somebody already wrote their device tree according previous definition? It seems to be that tri-states are preferable for pinctrl DT: no entry: do nothing = 0: disable = 1: enable > +Arguments for parameters: > + > +- bias-pull-up, -down and -pin-default take as optional argument 0 to disable > + the pull, on hardware supporting it the pull strength in Ohm. bias-disable > + will also disable any active pull. Does this agree with the latest definition of the kernel-internal meaning of 0 for pull-up/down? > +- input-schmitt takes as argument the adjustable hysteresis in a > + driver-specific format > + > +- input-debounce takes the debounce time as argument or 0 to disable debouncing > + > +- power-source argument is the custom value describing the source to select > + > +- slew-rate takes as argument the target rate in a driver-specific format If those things have driver-specific (note: should be DT-binding-specific, not driver-specific) values, then I'm not convinced that having a generic parameter name for them is a good idea; it makes things look the same when they aren't. By forcing each binding to include the vendor prefix on those properties and hence define a custom property name, you're making it clear that the semantics may be different.
On Thu, Jun 20, 2013 at 12:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 06/14/2013 09:42 AM, Heiko Stübner wrote: >> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > >> -low-power-mode - low power mode >> +low-power-enable - enable low power mode >> +low-power-disable - disable low power mode > > Hmmm. That's changing the binding definition. What if somebody already > wrote their device tree according previous definition? It's not merged so see it as alterations to a WIP in the turners workshop or something. > It seems to be that tri-states are preferable for pinctrl DT: > > no entry: do nothing > = 0: disable > = 1: enable Better with explict enable/disable strings instead of <0> or <1> I think, but the semantic effect would be the same I guess, the upside with *enable/*disable strings is that we do not have to handle cases like tristate = <2>; ... >> +Arguments for parameters: >> + >> +- bias-pull-up, -down and -pin-default take as optional argument 0 to disable >> + the pull, on hardware supporting it the pull strength in Ohm. bias-disable >> + will also disable any active pull. > > Does this agree with the latest definition of the kernel-internal > meaning of 0 for pull-up/down? No that is wrong. Heiko, care to fix this binding doc? >> +- input-schmitt takes as argument the adjustable hysteresis in a >> + driver-specific format >> + >> +- input-debounce takes the debounce time as argument or 0 to disable debouncing >> + >> +- power-source argument is the custom value describing the source to select >> + >> +- slew-rate takes as argument the target rate in a driver-specific format > > If those things have driver-specific (note: should be > DT-binding-specific, not driver-specific) values, then I'm not convinced > that having a generic parameter name for them is a good idea; it makes > things look the same when they aren't. By forcing each binding to > include the vendor prefix on those properties and hence define a custom > property name, you're making it clear that the semantics may be different. Hmmm I don't think they're used right now, let's deal with them when we have something to showcase them with. Patches to delete the unclear bindings will be considered... Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index ef7cd57..2d730e3 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt @@ -158,9 +158,29 @@ 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 +low-power-enable - enable low power mode +low-power-disable - disable 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 +Arguments for parameters: + +- bias-pull-up, -down and -pin-default take as optional argument 0 to disable + the pull, on hardware supporting it the pull strength in Ohm. bias-disable + will also disable any active pull. + +- drive-strength takes as argument the target strength in mA. + +- input-schmitt takes as argument the adjustable hysteresis in a + driver-specific format + +- input-debounce takes the debounce time as argument or 0 to disable debouncing + +- power-source argument is the custom value describing the source to select + +- slew-rate takes as argument the target rate in a driver-specific format + +All parameters not listed here, do not take an argument. + 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 d3c693c..dcf0371 100644 --- a/drivers/pinctrl/pinconf-generic.c +++ b/drivers/pinctrl/pinconf-generic.c @@ -152,9 +152,9 @@ 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 }, + { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 }, + { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 }, + { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 }, { "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 }, @@ -165,7 +165,8 @@ static struct pinconf_generic_dt_params dt_params[] = { { "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 }, + { "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 }, + { "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 }, { "output-low", PIN_CONFIG_OUTPUT, 0, }, { "output-high", PIN_CONFIG_OUTPUT, 1, }, };
The bias-pull-* options use values > 0 to indicate that the pull should be activated and optionally also indicate the strength of the pull. Therefore use an default value of 1 for these options. Split the low-power-mode option into low-power-enable and -disable. Update the documentation to describe the param arguments better. Wrong default options Reported-by: James Hogan <james.hogan@imgtec.com> Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- .../bindings/pinctrl/pinctrl-bindings.txt | 22 +++++++++++++++++++- drivers/pinctrl/pinconf-generic.c | 9 ++++---- 2 files changed, 26 insertions(+), 5 deletions(-)