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    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site