Messages in this thread | | | From | Stephen Warren <> | Date | Mon, 5 Dec 2011 09:36:30 -0800 | Subject | RE: [PATCH 2/2 v5] pinctrl: introduce generic pin config |
| |
Linus Walleij wrote at Monday, December 05, 2011 9:01 AM: > On Thu, Dec 1, 2011 at 10:56 PM, Stephen Warren <swarren@nvidia.com> wrote: > > Linus Walleij wrote: ... > >> +enum pin_config_param { > >> + PIN_CONFIG_BIAS_DISABLE, > >> + PIN_CONFIG_BIAS_HIGH_IMPEDANCE, ... > >> + PIN_CONFIG_WAKEUP, > >> + PIN_CONFIG_END, > >> +}; > > > > This enum conflates both "parameter" and "value" into a single enum space. > > I call these "parameter" and "argument" but I get it. > > > The patch introduces to_config_packed() and friends specifically to pack > > both param and value into a single unsigned long, but then defines the > > "param" to encompass "value" as well. That seems inconsistent. Instead, > > shouldn't you have something more like: > > > > enum pin_config_param { > > PIN_CONFIG_BIAS, > > PIN_CONFIG_DRIVE, > > PIN_CONFIG_INPUT_SCHMITT, > > PIN_CONFIG_INPUT_DEBOUNCE, ... > > PIN_CONFIG_WAKEUP, > > PIN_CONFIG_END, > > }; > > > > /* Value for PIN_CONFIG_BIAS */ > > enum pin_config_bias_value { > > PIN_CONFIG_BIAS_DISABLE, > > PIN_CONFIG_BIAS_HIGH_IMPEDANCE, > > PIN_CONFIG_BIAS_PULL_UP, > > PIN_CONFIG_BIAS_PULL_DOWN, > > PIN_CONFIG_BIAS_HIGH, > > PIN_CONFIG_BIAS_GROUND, > > }; > > But if I can control the resistance of the pull-up resistor > that brings us to a triplet: {parameter, type, argument} > like this to set the generic pull-up to 100 kOhm: > > set_generic_bias(PIN_CONFIG_BIAS, PIN_CONFIG_BIAS_PULL_UP, 100000); > > parameter = BIAS > type = PULL_UP > argument = 100 kOhm
I think that selecting what the value of pull-up is and enabling/disabling pull-up are separate things, so you'd have:
set PIN_CONFIG_PULL_UP_RESISTANCE 100000 set PIN_CONFIG_BIAS PULL_UP
Of course, this probably ties into which of the following options your chip HW has:
a) Pull-up 100K or pull-up 10K or pull-down or tri-state
b) Pull-up or pull-down or tri-state, with a second register field to set pull-up at 100k or 10k.
... which then goes back to the discussion of whether attempting to shoe-horn every SoC into a standardized parameter set makes sense.
> I essentially squash { parameter, type } into a single > enum here, then use the argument to supply the > value. > > > /* Value for PIN_CONFIG_DRIVE */ > > enum pin_config_drive_value { > > PIN_CONFIG_DRIVE_PUSH_PULL, > > PIN_CONFIG_DRIVE_OPEN_DRAIN, > > PIN_CONFIG_DRIVE_OPEN_SOURCE, > > PIN_CONFIG_DRIVE_OFF, > > }; > > > > /* > > * Value for: > > * PIN_CONFIG_INPUT_SCHMITT, > > * PIN_CONFIG_INPUT_DEBOUNCE, > > Don't you mean we would then have > > pin_config_param { > PIN_CONFIG_INPUT_MODE, > ... > } > > enum pin_config_input_mode_value { > PIN_CONFIG_INPUT_MODE_SCHMITT, > PIN_CONFIG_INPUT_MODE_DEBOUNCE, > };
Schmitt and debounce seem like completely orthogonal HW features to me, so I wouldn't make them mutually exclusive.
> > * PIN_CONFIG_LOW_POWER_MODE, > > * PIN_CONFIG_WAKEUP, > > * PIN_CONFIG_END, > > */ > > etc. > > I think it might be sub-dividing it too much, but it certainly > doesn't hurt the implementation much to split it in three, > say 8 bits parameter 8 bits type 16 bits argument if that is > preferable what do others say?
That doesn't seem quite right to me. I guess I can't really describe why though; it just feels wrong to be going that way.
-- nvpublic
-- 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/
| |