Messages in this thread | | | From | Stephen Warren <> | Date | Thu, 1 Dec 2011 13:56:39 -0800 | Subject | RE: [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
| |