[U-Boot] fsl_esdhc: GPIO regulator as VQMMC supply?

[U-Boot] fsl_esdhc: GPIO regulator as VQMMC supply?

[Sven Schwermer]

Hi,

I’m trying to implement the VQMMC supply for the an eMMC on my board using a GPIO regulator, i.e.

reg_sd_vsel: regulator-sd-vsel {
  pinctrl-names = "default";
  pinctrl-0 = <&pinctrl_sd_vsel>;
  compatible = "regulator-gpio";
  regulator-name = "sd-vsel";
  regulator-min-microvolt = <1800000>;
  regulator-max-microvolt = <3300000>;
  gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
  states = <1800000 1 3300000 0>;
  gpios-states = <0>; /* default: 3.3V */
};

&usdhc3 {
  vqmmc-supply = <&reg_sd_vsel>;
};

This is using an i.MX7D SoC, so my [eu]sdhc driver is fsl_esdhc. In fsl_esdhc_probe, regulator_set_enable is called for vqmmc-supply. This fails for GPIO regulators because they only have a set_value method and no set_enable method. How is this best resolved? Does it make sense to add a set_enable method to the GPIO regulator or should the esdhc driver handle the -ENOSYS return value gracefully? Or should I write my device tree differently?

Hints are much appreciated. Thanks!

Best regards,
Sven

[U-Boot] fsl_esdhc: GPIO regulator as VQMMC supply?

[Peng Fan]

> Subject: fsl_esdhc: GPIO regulator as VQMMC supply?
> 
> Hi,
> 
> I’m trying to implement the VQMMC supply for the an eMMC on my board
> using a GPIO regulator, i.e.
> 
> reg_sd_vsel: regulator-sd-vsel {
>   pinctrl-names = "default";
>   pinctrl-0 = <&pinctrl_sd_vsel>;
>   compatible = "regulator-gpio";
>   regulator-name = "sd-vsel";
>   regulator-min-microvolt = <1800000>;
>   regulator-max-microvolt = <3300000>;
>   gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
>   states = <1800000 1 3300000 0>;
>   gpios-states = <0>; /* default: 3.3V */ };
> 
> &usdhc3 {
>   vqmmc-supply = <&reg_sd_vsel>;
> };
> 
> This is using an i.MX7D SoC, so my [eu]sdhc driver is fsl_esdhc. In
> fsl_esdhc_probe, regulator_set_enable is called for vqmmc-supply. This fails
> for GPIO regulators because they only have a set_value method and no
> set_enable method. How is this best resolved? Does it make sense to add a
> set_enable method to the GPIO regulator or should the esdhc driver handle
> the -ENOSYS return value gracefully? Or should I write my device tree
> differently?

Add set_enable callback to gpio regulator seems make sense.

Regards,
Peng

> 
> Hints are much appreciated. Thanks!
> 
> Best regards,
> Sven

[U-Boot] [PATCH 1/2] regulator: Factor out common enable code

[Sven Schwermer]

In preparation of being able to enable/disable GPIO regulators, the
code that will be shared among the two kinds to regulators is factored
out into its own source files.

Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---
 drivers/power/regulator/Kconfig            | 10 +++
 drivers/power/regulator/Makefile           |  1 +
 drivers/power/regulator/common-regulator.c | 80 ++++++++++++++++++++++
 drivers/power/regulator/common-regulator.h | 27 ++++++++
 drivers/power/regulator/fixed.c            | 75 ++------------------
 5 files changed, 124 insertions(+), 69 deletions(-)
 create mode 100644 drivers/power/regulator/common-regulator.c
 create mode 100644 drivers/power/regulator/common-regulator.h

diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
index 72dfc48981..e0b8398ef4 100644
--- a/drivers/power/regulator/Kconfig
+++ b/drivers/power/regulator/Kconfig
@@ -92,9 +92,18 @@ config DM_REGULATOR_FAN53555
 	  or switching the mode is not supported by this driver (at
 	  this time).
 
+config DM_REGULATOR_COMMON
+	bool
+	depends on DM_REGULATOR
+
+config SPL_DM_REGULATOR_COMMON
+	bool
+	depends on DM_REGULATOR
+
 config DM_REGULATOR_FIXED
 	bool "Enable Driver Model for REGULATOR Fixed value"
 	depends on DM_REGULATOR
+	select DM_REGULATOR_COMMON
 	---help---
 	This config enables implementation of driver-model regulator uclass
 	features for fixed value regulators. The driver implements get/set api
@@ -103,6 +112,7 @@ config DM_REGULATOR_FIXED
 config SPL_DM_REGULATOR_FIXED
 	bool "Enable Driver Model for REGULATOR Fixed value in SPL"
 	depends on DM_REGULATOR_FIXED
+	select SPL_DM_REGULATOR_COMMON
 	---help---
 	This config enables implementation of driver-model regulator uclass
 	features for fixed value regulators in SPL.
diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
index 8c1506c88e..96bd351c49 100644
--- a/drivers/power/regulator/Makefile
+++ b/drivers/power/regulator/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
 obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o
 obj-$(CONFIG_$(SPL_)REGULATOR_PWM) += pwm_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.o
+obj-$(CONFIG_$(SPL_)DM_REGULATOR_COMMON) += common-regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_GPIO) += gpio-regulator.o
 obj-$(CONFIG_REGULATOR_RK8XX) += rk8xx.o
diff --git a/drivers/power/regulator/common-regulator.c b/drivers/power/regulator/common-regulator.c
new file mode 100644
index 0000000000..7c4dda0f65
--- /dev/null
+++ b/drivers/power/regulator/common-regulator.c
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Disruptive Technologies Research AS
+ * Sven Schwermer <sven.svenschwermer@disruptive-technologies.com>
+ */
+
+#include "common-regulator.h"
+#include <common.h>
+#include <power/regulator.h>
+
+int common_regulator_ofdata_to_platdata(struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata, const char *enable_gpio_name)
+{
+	struct gpio_desc *gpio;
+	int flags = GPIOD_IS_OUT;
+	int ret;
+
+	if (dev_read_bool(dev, "enable-active-high"))
+		flags |= GPIOD_IS_OUT_ACTIVE;
+
+	/* Get optional enable GPIO desc */
+	gpio = &dev_pdata->gpio;
+	ret = gpio_request_by_name(dev, enable_gpio_name, 0, gpio, flags);
+	if (ret) {
+		debug("Regulator '%s' optional enable GPIO - not found! Error: %d\n",
+		      dev->name, ret);
+		if (ret != -ENOENT)
+			return ret;
+	}
+
+	/* Get optional ramp up delay */
+	dev_pdata->startup_delay_us = dev_read_u32_default(dev,
+							"startup-delay-us", 0);
+	dev_pdata->off_on_delay_us =
+			dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0);
+
+	return 0;
+}
+
+int common_regulator_get_enable(const struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata)
+{
+	/* Enable GPIO is optional */
+	if (!dev_pdata->gpio.dev)
+		return true;
+
+	return dm_gpio_get_value(&dev_pdata->gpio);
+}
+
+int common_regulator_set_enable(const struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata, bool enable)
+{
+	int ret;
+
+	debug("%s: dev='%s', enable=%d, delay=%d, has_gpio=%d\n", __func__,
+	      dev->name, enable, dev_pdata->startup_delay_us,
+	      dm_gpio_is_valid(&dev_pdata->gpio));
+	/* Enable GPIO is optional */
+	if (!dm_gpio_is_valid(&dev_pdata->gpio)) {
+		if (!enable)
+			return -ENOSYS;
+		return 0;
+	}
+
+	ret = dm_gpio_set_value(&dev_pdata->gpio, enable);
+	if (ret) {
+		pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
+		      enable);
+		return ret;
+	}
+
+	if (enable && dev_pdata->startup_delay_us)
+		udelay(dev_pdata->startup_delay_us);
+	debug("%s: done\n", __func__);
+
+	if (!enable && dev_pdata->off_on_delay_us)
+		udelay(dev_pdata->off_on_delay_us);
+
+	return 0;
+}
diff --git a/drivers/power/regulator/common-regulator.h b/drivers/power/regulator/common-regulator.h
new file mode 100644
index 0000000000..e9b58c7a90
--- /dev/null
+++ b/drivers/power/regulator/common-regulator.h
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Disruptive Technologies Research AS
+ * Sven Schwermer <sven.svenschwermer@disruptive-technologies.com>
+ */
+
+#ifndef _COMMON_REGULATOR_H
+#define _COMMON_REGULATOR_H
+
+#include <common.h>
+#include <asm/gpio.h>
+#include <dm.h>
+
+struct common_regulator_platdata {
+	struct gpio_desc gpio; /* GPIO for regulator enable control */
+	unsigned int startup_delay_us;
+	unsigned int off_on_delay_us;
+};
+
+int common_regulator_ofdata_to_platdata(struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata, const char *enable_gpio_name);
+int common_regulator_get_enable(const struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata);
+int common_regulator_set_enable(const struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata, bool enable);
+
+#endif /* _COMMON_REGULATOR_H */
diff --git a/drivers/power/regulator/fixed.c b/drivers/power/regulator/fixed.c
index a99aa78310..b58bcecbfe 100644
--- a/drivers/power/regulator/fixed.c
+++ b/drivers/power/regulator/fixed.c
@@ -5,56 +5,26 @@
  *  Przemyslaw Marczak <p.marczak@samsung.com>
  */
 
+#include "common-regulator.h"
 #include <common.h>
 #include <errno.h>
 #include <dm.h>
-#include <i2c.h>
-#include <asm/gpio.h>
 #include <power/pmic.h>
 #include <power/regulator.h>
 
-struct fixed_regulator_platdata {
-	struct gpio_desc gpio; /* GPIO for regulator enable control */
-	unsigned int startup_delay_us;
-	unsigned int off_on_delay_us;
-};
-
 static int fixed_regulator_ofdata_to_platdata(struct udevice *dev)
 {
 	struct dm_regulator_uclass_platdata *uc_pdata;
-	struct fixed_regulator_platdata *dev_pdata;
-	struct gpio_desc *gpio;
-	int flags = GPIOD_IS_OUT;
-	int ret;
+	struct common_regulator_platdata *dev_pdata;
 
 	dev_pdata = dev_get_platdata(dev);
 	uc_pdata = dev_get_uclass_platdata(dev);
 	if (!uc_pdata)
 		return -ENXIO;
 
-	/* Set type to fixed */
 	uc_pdata->type = REGULATOR_TYPE_FIXED;
 
-	if (dev_read_bool(dev, "enable-active-high"))
-		flags |= GPIOD_IS_OUT_ACTIVE;
-
-	/* Get fixed regulator optional enable GPIO desc */
-	gpio = &dev_pdata->gpio;
-	ret = gpio_request_by_name(dev, "gpio", 0, gpio, flags);
-	if (ret) {
-		debug("Fixed regulator optional enable GPIO - not found! Error: %d\n",
-		      ret);
-		if (ret != -ENOENT)
-			return ret;
-	}
-
-	/* Get optional ramp up delay */
-	dev_pdata->startup_delay_us = dev_read_u32_default(dev,
-							"startup-delay-us", 0);
-	dev_pdata->off_on_delay_us =
-			dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0);
-
-	return 0;
+	return common_regulator_ofdata_to_platdata(dev, dev_pdata, "gpio");
 }
 
 static int fixed_regulator_get_value(struct udevice *dev)
@@ -91,45 +61,12 @@ static int fixed_regulator_get_current(struct udevice *dev)
 
 static int fixed_regulator_get_enable(struct udevice *dev)
 {
-	struct fixed_regulator_platdata *dev_pdata = dev_get_platdata(dev);
-
-	/* Enable GPIO is optional */
-	if (!dev_pdata->gpio.dev)
-		return true;
-
-	return dm_gpio_get_value(&dev_pdata->gpio);
+	return common_regulator_get_enable(dev, dev_get_platdata(dev));
 }
 
 static int fixed_regulator_set_enable(struct udevice *dev, bool enable)
 {
-	struct fixed_regulator_platdata *dev_pdata = dev_get_platdata(dev);
-	int ret;
-
-	debug("%s: dev='%s', enable=%d, delay=%d, has_gpio=%d\n", __func__,
-	      dev->name, enable, dev_pdata->startup_delay_us,
-	      dm_gpio_is_valid(&dev_pdata->gpio));
-	/* Enable GPIO is optional */
-	if (!dm_gpio_is_valid(&dev_pdata->gpio)) {
-		if (!enable)
-			return -ENOSYS;
-		return 0;
-	}
-
-	ret = dm_gpio_set_value(&dev_pdata->gpio, enable);
-	if (ret) {
-		pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
-		      enable);
-		return ret;
-	}
-
-	if (enable && dev_pdata->startup_delay_us)
-		udelay(dev_pdata->startup_delay_us);
-	debug("%s: done\n", __func__);
-
-	if (!enable && dev_pdata->off_on_delay_us)
-		udelay(dev_pdata->off_on_delay_us);
-
-	return 0;
+	return common_regulator_set_enable(dev, dev_get_platdata(dev), enable);
 }
 
 static const struct dm_regulator_ops fixed_regulator_ops = {
@@ -150,5 +87,5 @@ U_BOOT_DRIVER(fixed_regulator) = {
 	.ops = &fixed_regulator_ops,
 	.of_match = fixed_regulator_ids,
 	.ofdata_to_platdata = fixed_regulator_ofdata_to_platdata,
-	.platdata_auto_alloc_size = sizeof(struct fixed_regulator_platdata),
+	.platdata_auto_alloc_size = sizeof(struct common_regulator_platdata),
 };
-- 
2.17.1

[U-Boot] [PATCH 2/2] regulator: Allow enabling GPIO regulator

[Sven Schwermer]

Drivers need to be able to enable regulators that may be implemented as
GPIO regulators. Example: fsl_esdhc enables the vqmmc supply which is
commonly implemented as a GPIO regulator in order to switch between I/O
voltage levels.

Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---
 drivers/power/regulator/Kconfig          |  2 ++
 drivers/power/regulator/gpio-regulator.c | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
index e0b8398ef4..bb10d20545 100644
--- a/drivers/power/regulator/Kconfig
+++ b/drivers/power/regulator/Kconfig
@@ -120,6 +120,7 @@ config SPL_DM_REGULATOR_FIXED
 config DM_REGULATOR_GPIO
 	bool "Enable Driver Model for GPIO REGULATOR"
 	depends on DM_REGULATOR && DM_GPIO
+	select DM_REGULATOR_COMMON
 	---help---
 	This config enables implementation of driver-model regulator uclass
 	features for gpio regulators. The driver implements get/set for
@@ -128,6 +129,7 @@ config DM_REGULATOR_GPIO
 config SPL_DM_REGULATOR_GPIO
 	bool "Enable Driver Model for GPIO REGULATOR in SPL"
 	depends on DM_REGULATOR_GPIO && SPL_GPIO_SUPPORT
+	select SPL_DM_REGULATOR_COMMON
 	---help---
 	This config enables implementation of driver-model regulator uclass
 	features for gpio regulators in SPL.
diff --git a/drivers/power/regulator/gpio-regulator.c b/drivers/power/regulator/gpio-regulator.c
index d18e5d8d2c..617a553a5d 100644
--- a/drivers/power/regulator/gpio-regulator.c
+++ b/drivers/power/regulator/gpio-regulator.c
@@ -4,6 +4,7 @@
  * Keerthy <j-keerthy@ti.com>
  */
 
+#include "common-regulator.h"
 #include <common.h>
 #include <fdtdec.h>
 #include <errno.h>
@@ -18,6 +19,7 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 struct gpio_regulator_platdata {
+	struct common_regulator_platdata common;
 	struct gpio_desc gpio; /* GPIO for regulator voltage control */
 	int states[GPIO_REGULATOR_MAX_STATES];
 	int voltages[GPIO_REGULATOR_MAX_STATES];
@@ -65,7 +67,7 @@ static int gpio_regulator_ofdata_to_platdata(struct udevice *dev)
 		j++;
 	}
 
-	return 0;
+	return common_regulator_ofdata_to_platdata(dev, &dev_pdata->common, "enable-gpios");
 }
 
 static int gpio_regulator_get_value(struct udevice *dev)
@@ -116,9 +118,23 @@ static int gpio_regulator_set_value(struct udevice *dev, int uV)
 	return 0;
 }
 
+static int gpio_regulator_get_enable(struct udevice *dev)
+{
+	struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev);
+	return common_regulator_get_enable(dev, &dev_pdata->common);
+}
+
+static int gpio_regulator_set_enable(struct udevice *dev, bool enable)
+{
+	struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev);
+	return common_regulator_set_enable(dev, &dev_pdata->common, enable);
+}
+
 static const struct dm_regulator_ops gpio_regulator_ops = {
 	.get_value	= gpio_regulator_get_value,
 	.set_value	= gpio_regulator_set_value,
+	.get_enable	= gpio_regulator_get_enable,
+	.set_enable	= gpio_regulator_set_enable,
 };
 
 static const struct udevice_id gpio_regulator_ids[] = {
-- 
2.17.1

[U-Boot] [PATCH 1/2] regulator: Factor out common enable code

[sven at svenschwermer.de]

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

In preparation of being able to enable/disable GPIO regulators, the
code that will be shared among the two kinds to regulators is factored
out into its own source files.

Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---
 drivers/power/regulator/Kconfig            | 10 +++
 drivers/power/regulator/Makefile           |  1 +
 drivers/power/regulator/common-regulator.c | 80 ++++++++++++++++++++++
 drivers/power/regulator/common-regulator.h | 27 ++++++++
 drivers/power/regulator/fixed.c            | 75 ++------------------
 5 files changed, 124 insertions(+), 69 deletions(-)
 create mode 100644 drivers/power/regulator/common-regulator.c
 create mode 100644 drivers/power/regulator/common-regulator.h

diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
index 72dfc48981..e0b8398ef4 100644
--- a/drivers/power/regulator/Kconfig
+++ b/drivers/power/regulator/Kconfig
@@ -92,9 +92,18 @@ config DM_REGULATOR_FAN53555
 	  or switching the mode is not supported by this driver (at
 	  this time).
 
+config DM_REGULATOR_COMMON
+	bool
+	depends on DM_REGULATOR
+
+config SPL_DM_REGULATOR_COMMON
+	bool
+	depends on DM_REGULATOR
+
 config DM_REGULATOR_FIXED
 	bool "Enable Driver Model for REGULATOR Fixed value"
 	depends on DM_REGULATOR
+	select DM_REGULATOR_COMMON
 	---help---
 	This config enables implementation of driver-model regulator uclass
 	features for fixed value regulators. The driver implements get/set api
@@ -103,6 +112,7 @@ config DM_REGULATOR_FIXED
 config SPL_DM_REGULATOR_FIXED
 	bool "Enable Driver Model for REGULATOR Fixed value in SPL"
 	depends on DM_REGULATOR_FIXED
+	select SPL_DM_REGULATOR_COMMON
 	---help---
 	This config enables implementation of driver-model regulator uclass
 	features for fixed value regulators in SPL.
diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
index 8c1506c88e..96bd351c49 100644
--- a/drivers/power/regulator/Makefile
+++ b/drivers/power/regulator/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
 obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o
 obj-$(CONFIG_$(SPL_)REGULATOR_PWM) += pwm_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.o
+obj-$(CONFIG_$(SPL_)DM_REGULATOR_COMMON) += common-regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_GPIO) += gpio-regulator.o
 obj-$(CONFIG_REGULATOR_RK8XX) += rk8xx.o
diff --git a/drivers/power/regulator/common-regulator.c b/drivers/power/regulator/common-regulator.c
new file mode 100644
index 0000000000..7c4dda0f65
--- /dev/null
+++ b/drivers/power/regulator/common-regulator.c
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Disruptive Technologies Research AS
+ * Sven Schwermer <sven.svenschwermer@disruptive-technologies.com>
+ */
+
+#include "common-regulator.h"
+#include <common.h>
+#include <power/regulator.h>
+
+int common_regulator_ofdata_to_platdata(struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata, const char *enable_gpio_name)
+{
+	struct gpio_desc *gpio;
+	int flags = GPIOD_IS_OUT;
+	int ret;
+
+	if (dev_read_bool(dev, "enable-active-high"))
+		flags |= GPIOD_IS_OUT_ACTIVE;
+
+	/* Get optional enable GPIO desc */
+	gpio = &dev_pdata->gpio;
+	ret = gpio_request_by_name(dev, enable_gpio_name, 0, gpio, flags);
+	if (ret) {
+		debug("Regulator '%s' optional enable GPIO - not found! Error: %d\n",
+		      dev->name, ret);
+		if (ret != -ENOENT)
+			return ret;
+	}
+
+	/* Get optional ramp up delay */
+	dev_pdata->startup_delay_us = dev_read_u32_default(dev,
+							"startup-delay-us", 0);
+	dev_pdata->off_on_delay_us =
+			dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0);
+
+	return 0;
+}
+
+int common_regulator_get_enable(const struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata)
+{
+	/* Enable GPIO is optional */
+	if (!dev_pdata->gpio.dev)
+		return true;
+
+	return dm_gpio_get_value(&dev_pdata->gpio);
+}
+
+int common_regulator_set_enable(const struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata, bool enable)
+{
+	int ret;
+
+	debug("%s: dev='%s', enable=%d, delay=%d, has_gpio=%d\n", __func__,
+	      dev->name, enable, dev_pdata->startup_delay_us,
+	      dm_gpio_is_valid(&dev_pdata->gpio));
+	/* Enable GPIO is optional */
+	if (!dm_gpio_is_valid(&dev_pdata->gpio)) {
+		if (!enable)
+			return -ENOSYS;
+		return 0;
+	}
+
+	ret = dm_gpio_set_value(&dev_pdata->gpio, enable);
+	if (ret) {
+		pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
+		      enable);
+		return ret;
+	}
+
+	if (enable && dev_pdata->startup_delay_us)
+		udelay(dev_pdata->startup_delay_us);
+	debug("%s: done\n", __func__);
+
+	if (!enable && dev_pdata->off_on_delay_us)
+		udelay(dev_pdata->off_on_delay_us);
+
+	return 0;
+}
diff --git a/drivers/power/regulator/common-regulator.h b/drivers/power/regulator/common-regulator.h
new file mode 100644
index 0000000000..e9b58c7a90
--- /dev/null
+++ b/drivers/power/regulator/common-regulator.h
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Disruptive Technologies Research AS
+ * Sven Schwermer <sven.svenschwermer@disruptive-technologies.com>
+ */
+
+#ifndef _COMMON_REGULATOR_H
+#define _COMMON_REGULATOR_H
+
+#include <common.h>
+#include <asm/gpio.h>
+#include <dm.h>
+
+struct common_regulator_platdata {
+	struct gpio_desc gpio; /* GPIO for regulator enable control */
+	unsigned int startup_delay_us;
+	unsigned int off_on_delay_us;
+};
+
+int common_regulator_ofdata_to_platdata(struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata, const char *enable_gpio_name);
+int common_regulator_get_enable(const struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata);
+int common_regulator_set_enable(const struct udevice *dev,
+	struct common_regulator_platdata *dev_pdata, bool enable);
+
+#endif /* _COMMON_REGULATOR_H */
diff --git a/drivers/power/regulator/fixed.c b/drivers/power/regulator/fixed.c
index a99aa78310..b58bcecbfe 100644
--- a/drivers/power/regulator/fixed.c
+++ b/drivers/power/regulator/fixed.c
@@ -5,56 +5,26 @@
  *  Przemyslaw Marczak <p.marczak@samsung.com>
  */
 
+#include "common-regulator.h"
 #include <common.h>
 #include <errno.h>
 #include <dm.h>
-#include <i2c.h>
-#include <asm/gpio.h>
 #include <power/pmic.h>
 #include <power/regulator.h>
 
-struct fixed_regulator_platdata {
-	struct gpio_desc gpio; /* GPIO for regulator enable control */
-	unsigned int startup_delay_us;
-	unsigned int off_on_delay_us;
-};
-
 static int fixed_regulator_ofdata_to_platdata(struct udevice *dev)
 {
 	struct dm_regulator_uclass_platdata *uc_pdata;
-	struct fixed_regulator_platdata *dev_pdata;
-	struct gpio_desc *gpio;
-	int flags = GPIOD_IS_OUT;
-	int ret;
+	struct common_regulator_platdata *dev_pdata;
 
 	dev_pdata = dev_get_platdata(dev);
 	uc_pdata = dev_get_uclass_platdata(dev);
 	if (!uc_pdata)
 		return -ENXIO;
 
-	/* Set type to fixed */
 	uc_pdata->type = REGULATOR_TYPE_FIXED;
 
-	if (dev_read_bool(dev, "enable-active-high"))
-		flags |= GPIOD_IS_OUT_ACTIVE;
-
-	/* Get fixed regulator optional enable GPIO desc */
-	gpio = &dev_pdata->gpio;
-	ret = gpio_request_by_name(dev, "gpio", 0, gpio, flags);
-	if (ret) {
-		debug("Fixed regulator optional enable GPIO - not found! Error: %d\n",
-		      ret);
-		if (ret != -ENOENT)
-			return ret;
-	}
-
-	/* Get optional ramp up delay */
-	dev_pdata->startup_delay_us = dev_read_u32_default(dev,
-							"startup-delay-us", 0);
-	dev_pdata->off_on_delay_us =
-			dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0);
-
-	return 0;
+	return common_regulator_ofdata_to_platdata(dev, dev_pdata, "gpio");
 }
 
 static int fixed_regulator_get_value(struct udevice *dev)
@@ -91,45 +61,12 @@ static int fixed_regulator_get_current(struct udevice *dev)
 
 static int fixed_regulator_get_enable(struct udevice *dev)
 {
-	struct fixed_regulator_platdata *dev_pdata = dev_get_platdata(dev);
-
-	/* Enable GPIO is optional */
-	if (!dev_pdata->gpio.dev)
-		return true;
-
-	return dm_gpio_get_value(&dev_pdata->gpio);
+	return common_regulator_get_enable(dev, dev_get_platdata(dev));
 }
 
 static int fixed_regulator_set_enable(struct udevice *dev, bool enable)
 {
-	struct fixed_regulator_platdata *dev_pdata = dev_get_platdata(dev);
-	int ret;
-
-	debug("%s: dev='%s', enable=%d, delay=%d, has_gpio=%d\n", __func__,
-	      dev->name, enable, dev_pdata->startup_delay_us,
-	      dm_gpio_is_valid(&dev_pdata->gpio));
-	/* Enable GPIO is optional */
-	if (!dm_gpio_is_valid(&dev_pdata->gpio)) {
-		if (!enable)
-			return -ENOSYS;
-		return 0;
-	}
-
-	ret = dm_gpio_set_value(&dev_pdata->gpio, enable);
-	if (ret) {
-		pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
-		      enable);
-		return ret;
-	}
-
-	if (enable && dev_pdata->startup_delay_us)
-		udelay(dev_pdata->startup_delay_us);
-	debug("%s: done\n", __func__);
-
-	if (!enable && dev_pdata->off_on_delay_us)
-		udelay(dev_pdata->off_on_delay_us);
-
-	return 0;
+	return common_regulator_set_enable(dev, dev_get_platdata(dev), enable);
 }
 
 static const struct dm_regulator_ops fixed_regulator_ops = {
@@ -150,5 +87,5 @@ U_BOOT_DRIVER(fixed_regulator) = {
 	.ops = &fixed_regulator_ops,
 	.of_match = fixed_regulator_ids,
 	.ofdata_to_platdata = fixed_regulator_ofdata_to_platdata,
-	.platdata_auto_alloc_size = sizeof(struct fixed_regulator_platdata),
+	.platdata_auto_alloc_size = sizeof(struct common_regulator_platdata),
 };
-- 
2.17.1

[U-Boot] [PATCH 2/2] regulator: Allow enabling GPIO regulator

[sven at svenschwermer.de]

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

Drivers need to be able to enable regulators that may be implemented as
GPIO regulators. Example: fsl_esdhc enables the vqmmc supply which is
commonly implemented as a GPIO regulator in order to switch between I/O
voltage levels.

Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---
 drivers/power/regulator/Kconfig          |  2 ++
 drivers/power/regulator/gpio-regulator.c | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
index e0b8398ef4..bb10d20545 100644
--- a/drivers/power/regulator/Kconfig
+++ b/drivers/power/regulator/Kconfig
@@ -120,6 +120,7 @@ config SPL_DM_REGULATOR_FIXED
 config DM_REGULATOR_GPIO
 	bool "Enable Driver Model for GPIO REGULATOR"
 	depends on DM_REGULATOR && DM_GPIO
+	select DM_REGULATOR_COMMON
 	---help---
 	This config enables implementation of driver-model regulator uclass
 	features for gpio regulators. The driver implements get/set for
@@ -128,6 +129,7 @@ config DM_REGULATOR_GPIO
 config SPL_DM_REGULATOR_GPIO
 	bool "Enable Driver Model for GPIO REGULATOR in SPL"
 	depends on DM_REGULATOR_GPIO && SPL_GPIO_SUPPORT
+	select SPL_DM_REGULATOR_COMMON
 	---help---
 	This config enables implementation of driver-model regulator uclass
 	features for gpio regulators in SPL.
diff --git a/drivers/power/regulator/gpio-regulator.c b/drivers/power/regulator/gpio-regulator.c
index d18e5d8d2c..617a553a5d 100644
--- a/drivers/power/regulator/gpio-regulator.c
+++ b/drivers/power/regulator/gpio-regulator.c
@@ -4,6 +4,7 @@
  * Keerthy <j-keerthy@ti.com>
  */
 
+#include "common-regulator.h"
 #include <common.h>
 #include <fdtdec.h>
 #include <errno.h>
@@ -18,6 +19,7 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 struct gpio_regulator_platdata {
+	struct common_regulator_platdata common;
 	struct gpio_desc gpio; /* GPIO for regulator voltage control */
 	int states[GPIO_REGULATOR_MAX_STATES];
 	int voltages[GPIO_REGULATOR_MAX_STATES];
@@ -65,7 +67,7 @@ static int gpio_regulator_ofdata_to_platdata(struct udevice *dev)
 		j++;
 	}
 
-	return 0;
+	return common_regulator_ofdata_to_platdata(dev, &dev_pdata->common, "enable-gpios");
 }
 
 static int gpio_regulator_get_value(struct udevice *dev)
@@ -116,9 +118,23 @@ static int gpio_regulator_set_value(struct udevice *dev, int uV)
 	return 0;
 }
 
+static int gpio_regulator_get_enable(struct udevice *dev)
+{
+	struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev);
+	return common_regulator_get_enable(dev, &dev_pdata->common);
+}
+
+static int gpio_regulator_set_enable(struct udevice *dev, bool enable)
+{
+	struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev);
+	return common_regulator_set_enable(dev, &dev_pdata->common, enable);
+}
+
 static const struct dm_regulator_ops gpio_regulator_ops = {
 	.get_value	= gpio_regulator_get_value,
 	.set_value	= gpio_regulator_set_value,
+	.get_enable	= gpio_regulator_get_enable,
+	.set_enable	= gpio_regulator_set_enable,
 };
 
 static const struct udevice_id gpio_regulator_ids[] = {
-- 
2.17.1

[U-Boot] [PATCH 1/2] regulator: Factor out common enable code

[Simon Glass]

Hi,

On Wed, 12 Jun 2019 at 09:26, Sven Schwermer
<sven.schwermer@disruptive-technologies.com> wrote:
>
> In preparation of being able to enable/disable GPIO regulators, the
> code that will be shared among the two kinds to regulators is factored
> out into its own source files.
>
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
>  drivers/power/regulator/Kconfig            | 10 +++
>  drivers/power/regulator/Makefile           |  1 +
>  drivers/power/regulator/common-regulator.c | 80 ++++++++++++++++++++++
>  drivers/power/regulator/common-regulator.h | 27 ++++++++
>  drivers/power/regulator/fixed.c            | 75 ++------------------
>  5 files changed, 124 insertions(+), 69 deletions(-)
>  create mode 100644 drivers/power/regulator/common-regulator.c
>  create mode 100644 drivers/power/regulator/common-regulator.h

This looks fine but I think regulator_common.c (and the regulator
functions too) would be better.

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/2] regulator: Allow enabling GPIO regulator

[Simon Glass]

On Wed, 12 Jun 2019 at 09:26, Sven Schwermer
<sven.schwermer@disruptive-technologies.com> wrote:
>
> Drivers need to be able to enable regulators that may be implemented as
> GPIO regulators. Example: fsl_esdhc enables the vqmmc supply which is
> commonly implemented as a GPIO regulator in order to switch between I/O
> voltage levels.
>
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
>  drivers/power/regulator/Kconfig          |  2 ++
>  drivers/power/regulator/gpio-regulator.c | 18 +++++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Same comment about naming.