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

Message ID 1469693621-17851-2-git-send-email-preid@electromag.com.au
State New
Headers show

Commit Message

Phil Reid July 28, 2016, 8:13 a.m. UTC
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(+)

Comments

Linus Walleij July 28, 2016, 8:46 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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 {