Re: [PATCH] regulator: Add GPIO enable control to fixed voltage regulator driver

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

On Fri, Jul 31, 2009 at 03:55:18PM +0300, Roger Quadros wrote:

Looks good, some relatively nitpicky issues:

> +	int use_gpio_control;

This isn't needed, just use an invalid GPIO value (zero or less).

> +       if (data->use_gpio_control) {
> +               gpio_set_value(data->gpio,
> +                               data->enable_high ? 1 : 0);

Nicer to use the _cansleep() variants in case the GPIO is one on an
I2C/SPI device of some kind.  The regulator API doesn't mind if drivers
sleep so long as they don't do so excessively and it's not normally
sufficiently performance critical to make the inlining worth it.

> +		if (ret) {
> +			dev_err(&pdev->dev, "Could not obtain regulator " \
> +					"enable GPIO %d\n", config->gpio);

Please do something like:

	dev_err(&pdev->dev,
		"Could not obtain enable GPIO %d: %d\n",
		config->gpio, ret);

so that the error message is all in one in the source (so it's easier to
find when grepping the kernel log.  You also don't need the \.

> +			goto err_name;
> +		} else {

No need for the else clause; you've got the goto above.

> +			ret = gpio_direction_output(config->gpio,
> +				config->enable_high ? 0 : 1);
> +			if (ret) {
> +				dev_err(&pdev->dev, "Could not configure " \
> +					"enable GPIO %d direction\n",
> +					config->gpio);
> +				gpio_free(config->gpio);
> +				goto err_name;
> +			}

Same comment as above with regard to the error message.  It would be
nice to have the default state passed in as platform data if you can't
read it back to help avoid bouncing supplies at startup.  IIRC
gpio_get_value() will generally take a good stab at giving the current
state no matter if the GPIO is input our output but I'd need to check.

> +       int use_gpio_control;   /* Use GPIO enable control */
> +       int gpio;               /* GPIO to use for enable control */
> +
> +       int enable_high;        /* Polarity of enable GPIO
> +                                * 1 = Active High, 0 = Active Low

If you mark the comments with /** they'll get picked up by kerneldoc.