RE: [PATCH 2/2 v5] pinctrl: introduce generic pin config

[Stephen Warren]

Linus Walleij wrote at Monday, December 05, 2011 9:01 AM:
> On Thu, Dec 1, 2011 at 10:56 PM, Stephen Warren <swarren@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/