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

[Stephen Warren]

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

--
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/