lkml.org 
[lkml]   [2011]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRE: [PATCH 2/2 v5] pinctrl: introduce generic pin config
    Linus Walleij wrote at Thursday, December 01, 2011 3:55 AM:
    > This is a split-off from the earlier patch set which adds generic
    > pin configuration for the pin controllers that want it. Since
    > we may have a system with mixed generic and custom pin controllers,
    > we pass a boolean in the pin controller ops vtable to indicate
    > if it is generic.

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

    > + seq_printf(s, " (%u %s)", to_config_argument(config),
    > + conf_items[i].format);
    > + }
    > +}

    > +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.
    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,
    };

    /* 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,
    * PIN_CONFIG_LOW_POWER_MODE,
    * PIN_CONFIG_WAKEUP,
    * PIN_CONFIG_END,
    */
    enum pin_config_bool_value {
    PIN_CONFIG_OFF,
    PIN_CONFIG_ON,
    };

    /*
    * Value for:
    * PIN_CONFIG_SLEW_RATE_RISING,
    * PIN_CONFIG_SLEW_RATE_FALLING,
    * PIN_CONFIG_POWER_SOURCE,
    * ... is an integer
    */

    --
    nvpublic



    \
     
     \ /
      Last update: 2011-12-01 22:59    [W:0.031 / U:59.976 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site