linux-kernel - Re: [PATCH v2 1/4] regulator-core: support shared enable GPIO concept

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] [day] [month] [year] [list]

Date:	Sun, 27 Jan 2013 14:00:15 +0800
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	"Kim, Milo" <Milo.Kim@...com>
Cc:	Axel Lin <axel.lin@...ics.com>, "Girdwood, Liam" <lrg@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/4] regulator-core: support shared enable GPIO concept

On Tue, Jan 15, 2013 at 04:35:41AM +0000, Kim, Milo wrote:
>  A Regulator can be enabled by external GPIO pin.
>  This is configurable in the regulator_config.

Please use subject lines matching the subsystem - not doing this makes
it more likely that patches will be missed or responses delayed.  For
example, when looking at my patch queue for regulator patches I search
for "regulator:" in my review pending queue, patches that don't have
that won't turn up.  This should be "regulator: core: ...".

Anyway, this series looks pretty close now...

> +/* Manage enable GPIO list. Same GPIO pin can be shared among regulators */
> +static int regulator_ena_gpio_request(struct regulator_dev *rdev,
> +				const struct regulator_config *config)
> +{
> +	struct regulator_enable_gpio *pin;
> +	int ret;
> +
> +	list_for_each_entry(pin, &regulator_ena_gpio_list, list) {
> +		if (pin->gpio == config->ena_gpio) {
> +			rdev_info(rdev, "GPIO %d is already used\n",
> +				config->ena_gpio);
> +			return 0;

This log is going to get noisy once the GPIOs are shared.  A _dbg()
would be OK though.

> +	ret = gpio_request_one(config->ena_gpio,
> +				GPIOF_DIR_OUT | config->ena_gpio_flags,
> +				rdev_get_name(rdev));
> +	if (ret)
> +		return ret;
> +
> +	pin = kzalloc(sizeof(struct regulator_enable_gpio), GFP_KERNEL);
> +	if (pin == NULL)
> +		return -ENOMEM;

Should free the GPIO if there's an error here.

> +	pin->regulator = rdev;

Do we really want to keep track of the regulator here, again once we
start sharing pins...

We also need some matching code in the release path to free the GPIO and
struct when the regulator is removed.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)