Messages in this thread Patch in this message | | | Date | Thu, 20 Oct 2011 12:24:56 +0200 | Subject | Re: [PATCH 2/2] pinctrl: add a generic control interface | From | Linus Walleij <> |
| |
On Thu, Oct 20, 2011 at 1:04 AM, Stephen Warren <swarren@nvidia.com> wrote:
>> +/** >> + * enum pin_config_param - possible pin configuration parameters >> + * @PIN_CONFIG_BIAS_UNKNOWN: this bias mode is not known to us > > I'm not sure what use that would be; shouldn't we remove that, and add > new PIN_CONFIG_BIAS_* values as/when they're needed?
Came from some idea that you could also pin_congif_get() to get the setting of a pin, but that's overdesigned, so killed it now.
>> + * @PIN_CONFIG_BIAS_FLOAT: no specific bias, the pin will float or state >> + * is not controlled by software > > "float" and "not controlled by SW" are very different things. How is float > different to high impedance?
True. And the comment for BIAS_HIGH_IMPEDANCE even says so. Deleted this.
>> + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance >> + * mode, also know as "third-state" (tristate) or "high-Z" or "floating". >> + * On output pins this effectively disconnects the pin, which is useful >> + * if for example some other pin is going to drive the signal connected >> + * to it for a while. Pins used for input are usually always high >> + * impedance. >> + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high >> + * impedance to VDD), if the controller supports specifying a certain >> + * pull-up resistance, this is given as an argument (in Ohms) when >> + * setting this parameter > > What value should be used to disable a pull-up; 0?
A semantic question would also be if pull up is implicitly disabled if you issue PIN_CONFIG_BIAS_PULL_DOWN when you are in PULL_UP state.
I added PIN_CONFIG_BIAS_DISABLED to set_pin_config(pin, PIN_CONFIG_BIAS_DISABLED);
So we can transition to a state of totally disabled pin bias.
(How to handled them is up to the driver...)
> What value should be > used when requesting pull-up where the SoC doesn't have a configurable > pull-up resistor; anything non-zero?
I think the argument should be ignored if it's simply on/off.
Then PIN_CONFIG_BIAS_DISABLE to turn it off.
> Tegra has separate configurations for > pull-up/down strength (0..31) and pull-up/down enable (up/down/none), though > I suppose we could map 0 to off, 1..32 to on with strength 0..31, although > that'd prevent a driver from toggling the enable bit on/off without knowing > what the strength code should be programmed to...
No a DISABLE enum is cleaner I think.
> Tegra's pull-up/down strengths aren't documented in terms of ohms, but > rather a "drive strength code" on scale 0..31. I'm not sure what that > truly maps to in hardware.
Hmmmmmmm can you check it with some hardware engineer? I have a strong feeling that this is or should be very strictly specified somewhere in an electrical datasheet. How else can users of the circuit design the electronics around it? Trial-and-error? :-)
>> + * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high >> + * impedance to GROUND), if the controller supports specifying a certain >> + * pull-down resistance, this is given as an argument (in Ohms) when >> + * setting this parameter >> + * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD >> + * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND > > At least some of these options appear mutually exclusive; I wonder if we > shouldn't have a PIN_CONFIG_BIAS parameter, and have the legal values be > HIGH/GROUND/NONE. Pull up/down are probably separate. HIGH_IMPEDANCE seems > more like a PIN_CONFIG_DRIVE value than a PIN_CONFIG_BIAS value.
Yes some of them are. I think it should be up to the driver to handle how the mutual exclusiveness of its supported configs are done, returning the occasional -EINVAL.
Later we may consolidate this, say by letting the core keep track of config states (it currently doesn't) and reject things that are mutually exclusive. But I think that's overkill right now?
>> + * @PIN_CONFIG_DRIVE_UNKNOWN: we don't know the drive mode of this pin, for >> + * example since it is controlled by hardware or the information is not >> + * accessible but we need a meaningful enumerator in e.g. initialization >> + * code > > Again, I'm not sure this is useful; if this is not a configurable parameter, > surely initialization code would simply skip attempting to set this param?
Deleted.
>> + * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and >> + * low, this is the most typical case and is typically achieved with two >> + * active transistors on the output. If the pin can support different >> + * drive strengths for push/pull, the strength is given on a custom format >> + * as argument when setting pins to this mode >> + * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open >> + * collector) which means it is usually wired with other output ports >> + * which are then pulled up with an external resistor. If the pin can >> + * support different drive strengths for the open drain pin, the strength >> + * is given on a custom format as argument when setting pins to this mode >> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain >> + * (open emitter) which is the same as open drain mutatis mutandis but >> + * pulled to ground. If the pin can support different drive strengths for >> + * the open drain pin, the strength is given on a custom format as >> + * argument when setting pins to this mode >> + * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off > > These seem like mutually exclusive values for a PIN_CONFIG_DRIVE param.
They are. Driver needs to handle that, and say disable OPEN_DRAIN if it is on when enabling OPEN_SOURCE.
>> + * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in >> + * schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis, >> + * the threshold value is given on a custom format as argument when >> + * setting pins to this mode >> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising >> + * signals on the pin. The argument gives the rise time in nanoseconds >> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling >> + * signals on the pin. The argument gives the fall time in nanoseconds >> + * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and >> + * will deform waveforms when signals are transmitted on them, by >> + * applying a load capacitance, the waveform can be rectified. The >> + * argument gives the load capacitance in picofarad (pF) >> + * @PIN_CONFIG_WAKEUP_ENABLE: this will configure an input pin such that if a >> + * signal transition arrives at the pin when the pin controller/system >> + * is sleeping, it will wake up the system >> + * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if >> + * you need to pass in custom configurations to the pin controller, use >> + * PIN_CONFIG_END+1 as the base offset > > Tegra needs at least some more: > > PIN_CONFIG_HIGH_SPEED_MODE (0..1) [tegra_drive_pingroup_config.hsm]
Is it only for output so it's actually PIN_CONFIG_DRIVE_HIGH_SPEED or only for input so it's PIN_CONFIG_INPUT_HIGHSPEED?
So this is like the inverse of the slew rate or so?
> PIN_CONFIG_LOW_POWER_MODE (0..3) [tegra_drive_pingroup_config.drive]
OK added PIN_CONFIG_LOW_POWER_MODE, it could actually map to say ground setting after a state transition in some cases.
>> @@ -113,6 +204,10 @@ extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc, > ... >> +extern int pin_config(struct pinctrl_dev *pctldev, int pin, >> + enum pin_config_param param, unsigned long data); >> +extern int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group, >> + enum pin_config_param param, unsigned long data); > > Hmmm. Do we really want to expose these as public APIs? I suppose it > will allow us to start configuring all these parameters ASAP, but all > previous discussion has been aimed at having the pinctrl core set up an > initial set of values at boot-time using a board-supplied table (so no > external API), and then we were still talking about how to manipulate > the values at run-time. Do we really want to encode all this information > into drivers calling these APIs?
I already have such drivers: in the ABx500 there is a SIM card interface, these set pull up/down and load capacitance (also voltage!) depending on information obtained after actively communicating with the card and asking about its electrical characteristics.
Same applies to some eMMC I think.
So we probably cannot avoid exposing this...
However, yes, the simple cases should be for default-config and then state transitions to things like sleeping or different "C-states".
Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |