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

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]

Date:	Fri, 31 Jul 2009 15:25:36 +0200
From:	pHilipp Zabel <philipp.zabel@...il.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	Roger Quadros <quadros.roger@...il.com>, lrg@...mlogic.co.uk,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] regulator: Add GPIO enable control to fixed voltage 
	regulator driver

On Fri, Jul 31, 2009 at 3:10 PM, Mark
Brown<broonie@...nsource.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
--
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/