Re: [PATCH 1/1] gpio: pca954x: Add optional regulator enable.

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Thu, Jul 28, 2016 at 10:13 AM, Phil Reid <preid@xxxxxxxxxxxxxxxxx> wrote:

> Some i2c gpio devices are connected to a switchale power supply
> which needs to be enabled prior to probing the device. This patch
> allows the drive to enable the devices vcc regulator prior to probing.
>
> Signed-off-by: Phil Reid <preid@xxxxxxxxxxxxxxxxx>

Overall good, I would change the subject to just
"add a regulator" and not use _get_optional.

The regulators should be added to *all* devices
that have VDD/VDDIO etc lines. Whether the platform will
supply them with a real backing regulator or just use a
dummy or even stubs (of CONFIG_REGULATOR) is not
set) is no concern of the individual driver.

Just grab them and handle the errors, like you do.

> +       reg = devm_regulator_get_optional(&client->dev, "vcc");

So use devm_regulator_get()

> +       if (IS_ERR(reg)) {
> +               ret = PTR_ERR(reg);
> +               if (ret != -ENODEV) {
> +                       if (ret != -EPROBE_DEFER)
> +                               dev_err(&client->dev, "reg get err: %d\n", ret);
> +                       return ret;
> +               }

Just bail out if IS_ERR(), one of the possible errors would
be deferral but -ENODEV need no special handling.

> +       } else {
> +               ret = regulator_enable(reg);
> +               if (ret) {
> +                       dev_err(&client->dev, "reg en err: %d\n", ret);
> +                       return ret;
> +               }
> +       }

Then just de-indent the else clause. We should always get
something that we can call regulator_enable() on, even if it
is a dummy or a stub.

There is another problem: you do not call regulator_disable()
anywhere. Call it in the remove() function, and on the error path
in probe following this call, possibly with a goto-try/catch
construction.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html