lkml.org 
[lkml]   [2011]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 2/2 v5] pinctrl: introduce generic pin config
    From
    On Thu, Dec 1, 2011 at 10:56 PM, Stephen Warren <swarren@nvidia.com> wrote:

    >> +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
    >> +                           struct seq_file *s, unsigned pin)
    > ...
    >> +             config = to_config_packed(conf_items[i].param, 0);
    > ...
    >> +             /* Print unit if available */
    >> +             if (conf_items[i].format && config != 0)
    >
    > Why the check for "config != 0"; isn't the "param" always left in config
    > by pin_config_get, such that it's never 0?

    Should be to_config_argumen(config) != 0 so that if you
    have say an "unspecified pull-up", that means BIAS_PULL_UP
    and argument 0, so we do not print this as (0 Ohm).

    Fixed it.

    >> +enum pin_config_param {
    >> +     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,
    >> +     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_INPUT_DEBOUNCE,
    >> +     PIN_CONFIG_SLEW_RATE_RISING,
    >> +     PIN_CONFIG_SLEW_RATE_FALLING,
    >> +     PIN_CONFIG_POWER_SOURCE,
    >> +     PIN_CONFIG_LOW_POWER_MODE,
    >> +     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_SLEW_RATE_RISING,
    >        PIN_CONFIG_SLEW_RATE_FALLING,
    >        PIN_CONFIG_POWER_SOURCE,
    >        PIN_CONFIG_LOW_POWER_MODE,
    >        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 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,
    };

    >  * 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?

    Yours,
    Linus Wallej
    --
    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/

    \
     
     \ /
      Last update: 2011-12-05 17:03    [W:0.041 / U:62.656 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site