Re: [PATCH 2/2] pinctrl: add a generic control interface

[Barry Song]

2011/10/20 Shawn Guo <shawn.guo@xxxxxxxxxxxxx>:
>> Â#ifdef CONFIG_DEBUG_FS
>>
>> Âstatic int pinctrl_pins_show(struct seq_file *s, void *what)
>> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
>> index 4f8d208..189c047 100644
>> --- a/include/linux/pinctrl/pinctrl.h
>> +++ b/include/linux/pinctrl/pinctrl.h
>> @@ -19,6 +19,87 @@
>> Â#include <linux/list.h>
>> Â#include <linux/seq_file.h>
>>
>> +/**
>> + * enum pin_config_param - possible pin configuration parameters
>> + * @PIN_CONFIG_BIAS_UNKNOWN: this bias mode is not known to us
>> + * @PIN_CONFIG_BIAS_FLOAT: no specific bias, the pin will float or state
>> + * Â is not controlled by software
>> + * @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
>> + * @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
>> + * @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
>> + * @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
>> + * @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
>> + */
>> +enum pin_config_param {
>> + Â Â PIN_CONFIG_BIAS_UNKNOWN,
>> + Â Â PIN_CONFIG_BIAS_FLOAT,
>> + Â Â PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
>> + Â Â PIN_CONFIG_BIAS_PULL_UP,
>> + Â Â PIN_CONFIG_BIAS_PULL_DOWN,
>> + Â Â PIN_CONFIG_BIAS_HIGH,
>> + Â Â PIN_CONFIG_BIAS_GROUND,
>> + Â Â PIN_CONFIG_DRIVE_UNKNOWN,
>> + Â Â PIN_CONFIG_DRIVE_PUSH_PULL,
>> + Â Â PIN_CONFIG_DRIVE_OPEN_DRAIN,
>> + Â Â PIN_CONFIG_DRIVE_OPEN_SOURCE,
>> + Â Â PIN_CONFIG_DRIVE_OFF,
>> + Â Â PIN_CONFIG_INPUT_SCHMITT,
>> + Â Â PIN_CONFIG_SLEW_RATE_RISING,
>> + Â Â PIN_CONFIG_SLEW_RATE_FALLING,
>> + Â Â PIN_CONFIG_LOAD_CAPACITANCE,
>> + Â Â PIN_CONFIG_WAKEUP_ENABLE,
>> + Â Â PIN_CONFIG_END,
>> +};
>> +
>
> IMO, trying to enumerate all possible pin_config options is just to
> make everyone's life harder. ÂFirstly, this enumeration is far from
> completion, for imx6 iomuxc example, we have 3 options for pull-up,
> 22, 47 and 100 kOhm, and 7 options for driver-strength, 34, 40, 48,
> 60, 80, 120, and 240 Ohm. ÂIt's just so hard to make this enumeration
> complete. ÂSecondly, defining this param as enum requires the API
> user to call the function multiple times to configure one pin. ÂFor
> example, if I want to configure pin_foo as slow-slew, open-drain,
> 100 kOhm pull-up and 120 Ohm driver-strength, I will have to call
> pin_config(pctldev, pin_foo, ...) 4 times to achieve that.
>
> I like Stephen's idea about having 'u32 param' and let pinctrl drivers
> to encode/decode this u32 for their pinctrl controller. ÂIt makes
> people's life much easier.

A multifunctional API is actually  a bad and hard-to-use API. i agree
we can make param u32 instead of enum and let specific driver
customizes the param by itself.

But if there are some common params, for example, PULL, OC/OD,
WAKEUP_ENABLE, which almost all platforms need,  why don't we make
them have common definitions like:

#define PIN_CONFIG_PULL                             0
#define PIN_CONFIG_OPEN_DRAIN                1
....
#define PIN_CONFIG_USER                            5 (in case)

if one platform has config not in the up list, then:

#define PIN_CONFIG_TERGA_XXX                PIN_CONFIG_USER
#define PIN_CONFIG_TERGA_YYY                (PIN_CONFIG_USER + 1)

without the common definition from  PIN_CONFIG_PULL to
PIN_CONFIG_USER, many platforms will need to definite them repeatedly.
that is what we hate.

>
> --
> Regards,
> Shawn

-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/