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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]