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

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

[Phil Reid]

I have a system where the gpio device is attached to a gpio controlled
regulator. If not enabled prior to device probe it fails to load the
driver. Marking the regulator as always on does not always work as the
probe order changes and it's suspectible to race conditions.

I'm not aware of another way around this problem.

Phil Reid (1):
  gpio: pca954x: Add optional regulator enable.

 drivers/gpio/gpio-pca953x.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

-- 
1.8.3.1

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

[Phil Reid]

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@electromag.com.au>
---
 drivers/gpio/gpio-pca953x.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 5e3be32..12d8e91 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -21,6 +21,7 @@
 #include <asm/unaligned.h>
 #include <linux/of_platform.h>
 #include <linux/acpi.h>
+#include <linux/regulator/consumer.h>
 
 #define PCA953X_INPUT		0
 #define PCA953X_OUTPUT		1
@@ -744,6 +745,7 @@ static int pca953x_probe(struct i2c_client *client,
 	int irq_base = 0;
 	int ret;
 	u32 invert = 0;
+	struct regulator *reg;
 
 	chip = devm_kzalloc(&client->dev,
 			sizeof(struct pca953x_chip), GFP_KERNEL);
@@ -763,6 +765,22 @@ static int pca953x_probe(struct i2c_client *client,
 
 	chip->client = client;
 
+	reg = devm_regulator_get_optional(&client->dev, "vcc");
+	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;
+		}
+	} else {
+		ret = regulator_enable(reg);
+		if (ret) {
+			dev_err(&client->dev, "reg en err: %d\n", ret);
+			return ret;
+		}
+	}
+
 	if (id) {
 		chip->driver_data = id->driver_data;
 	} else {
-- 
1.8.3.1

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

[Linus Walleij]

On Thu, Jul 28, 2016 at 10:13 AM, Phil Reid <preid@electromag.com.au> 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@electromag.com.au>

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