On Thu, Oct 20, 2011 at 05:17:08PM +0800, Barry Song wrote:
+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.
I prefer to completely leave the encoding of this 'u32 param' to pinctrl drivers, so that they can map the bit definition of 'param' to the controller register as much as they can. On imx, we have one register accommodating all pin configuration options for one pin, so we can write 'param' directly into register with exactly same bit definition as register.