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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <74CDBE0F657A3D45AFBB94109FB122FF174FDB0312@HQMAIL01.nvidia.com>
Date:	Thu, 1 Dec 2011 13:56:39 -0800
From:	Stephen Warren <swarren@...dia.com>
To:	Linus Walleij <linus.walleij@...ricsson.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:	Grant Likely <grant.likely@...retlab.ca>,
	Barry Song <21cnbao@...il.com>,
	Shawn Guo <shawn.guo@...escale.com>,
	Thomas Abraham <thomas.abraham@...aro.org>,
	Dong Aisheng <dong.aisheng@...aro.org>,
	Rajendra Nayak <rajendra.nayak@...aro.org>,
	Haojian Zhuang <haojian.zhuang@...vell.com>,
	Linus Walleij <linus.walleij@...aro.org>
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/