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

[pHilipp Zabel <philipp.zabel@gmail.com>]

On Fri, Jul 31, 2009 at 3:10 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> 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).

Negative only, actually. Zero itself is a valid GPIO number (which is
a bit unfortunate because if you forget to initialize .gpio, it will
default to GPIO #0).

If you drop .use_gpio_control, use gpio_is_valid(data->gpio) for this check:

>> +       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.

I think this is not clearly defined in the GPIO API document, so it
could be architecture dependent.

>> +       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.

Maybe following the style in Documentation/kernel-doc-nano-HOWTO.txt
would be worthwhile, then.

regards
Philipp