All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family
@ 2017-11-22 21:13 Philipp Tomsich
  2017-11-22 21:13 ` [U-Boot] [PATCH v2 2/2] rockchip: defconfig: puma-rk3399: enable FAN53555 regulator driver Philipp Tomsich
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Philipp Tomsich @ 2017-11-22 21:13 UTC (permalink / raw)
  To: u-boot

This adds a driver for the FAN53555 family of regulators.

While these devices support a 'normal' and 'suspend' mode (controlled
via an external pin) to switch between two programmable voltages, this
incarnation of the driver assumes that the device is always operating
in 'normal' mode.

Only setting/reading the programmed voltage is supported at this time
and the following device functionality remains unsupported:
  - switching the selected voltage (via a GPIO)
  - disabling the voltage output via software-control
This matches the functionality of the Linux driver.

Tested on a RK3399-Q7 (with 'option 5' devices): setting voltages from
the U-Boot shell and verifying output voltages on the board.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>

---

Changes in v2:
- adapted documentation on the device-tree binding from Linux

 doc/device-tree-bindings/regulator/fan53555.txt |  23 +++
 drivers/power/regulator/Kconfig                 |  14 ++
 drivers/power/regulator/Makefile                |   1 +
 drivers/power/regulator/fan53555.c              | 255 ++++++++++++++++++++++++
 4 files changed, 293 insertions(+)
 create mode 100644 doc/device-tree-bindings/regulator/fan53555.txt
 create mode 100644 drivers/power/regulator/fan53555.c

diff --git a/doc/device-tree-bindings/regulator/fan53555.txt b/doc/device-tree-bindings/regulator/fan53555.txt
new file mode 100644
index 0000000..b183738
--- /dev/null
+++ b/doc/device-tree-bindings/regulator/fan53555.txt
@@ -0,0 +1,23 @@
+Binding for Fairchild FAN53555 regulators
+
+Required properties:
+  - compatible: "fcs,fan53555"
+  - reg: I2C address
+
+Optional properties:
+  - fcs,suspend-voltage-selector: declare which of the two available
+		voltage selector registers should be used for the suspend
+		voltage. The other one is used for the runtime voltage setting
+		Possible values are either <0> or <1>
+  - vin-supply: regulator supplying the vin pin
+
+Example:
+
+	regulator at 40 {
+		compatible = "fcs,fan53555";
+		regulator-name = "fan53555";
+		regulator-min-microvolt = <1000000>;
+		regulator-max-microvolt = <1800000>;
+		vin-supply = <&parent_reg>;
+		fcs,suspend-voltage-selector = <1>;
+	};
diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
index 8892fa1..c26a765 100644
--- a/drivers/power/regulator/Kconfig
+++ b/drivers/power/regulator/Kconfig
@@ -69,6 +69,20 @@ config DM_REGULATOR_MAX77686
 	features for REGULATOR MAX77686. The driver implements get/set api for:
 	value, enable and mode.
 
+config DM_REGULATOR_FAN53555
+	bool "Enable Driver Model for REGULATOR FAN53555"
+	depends on DM_REGULATOR && DM_I2C
+	---help---
+	This config enables implementation of driver-model regulator uclass
+	features for the FAN53555 regulator. The FAN53555 is a (family of)
+	single-output regulators that supports transitioning between two
+	different output voltages based on an voltage selection pin.
+
+	The driver implements a get/set api for the voltage of the 'normal
+	mode' voltage only. Switching to 'suspend mode' (i.e. the alternate
+	voltage), disabling output via software, or switching the mode is
+	not supported by this driver (at this time).
+
 config DM_REGULATOR_FIXED
 	bool "Enable Driver Model for REGULATOR Fixed value"
 	depends on DM_REGULATOR
diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
index 6c149a9..21040ea 100644
--- a/drivers/power/regulator/Makefile
+++ b/drivers/power/regulator/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_REGULATOR_AS3722)	+= as3722_regulator.o
 obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
 obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o
 obj-$(CONFIG_REGULATOR_PWM) += pwm_regulator.o
+obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.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/fan53555.c b/drivers/power/regulator/fan53555.c
new file mode 100644
index 0000000..3f0b4e9
--- /dev/null
+++ b/drivers/power/regulator/fan53555.c
@@ -0,0 +1,255 @@
+/*
+ * (C) 2017 Theobroma Systems Design und Consulting GmbH
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <bitfield.h>
+#include <errno.h>
+#include <dm.h>
+#include <fdtdec.h>
+#include <i2c.h>
+#include <asm/gpio.h>
+#include <power/pmic.h>
+#include <power/regulator.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/*
+ * The voltage ramp (i.e. minimum voltage and step) is defined by the
+ * combination of 2 nibbles: DIE_ID and DIE_REV.
+ *
+ * See http://www.onsemi.com/pub/Collateral/FAN53555-D.pdf for details.
+ */
+static const struct {
+	u8 die_id;
+	u8 die_rev;
+	u32 vsel_min;
+	u32 vsel_step;
+} ic_types[] = {
+	{ 0x0, 0x3, 600000, 10000 },  /* Option 00 */
+	{ 0x0, 0xf, 800000, 10000 },  /* Option 13 */
+	{ 0x0, 0xc, 600000, 12500 },  /* Option 23 */
+	{ 0x1, 0x3, 600000, 10000 },  /* Option 01 */
+	{ 0x3, 0x3, 600000, 10000 },  /* Option 03 */
+	{ 0x4, 0xf, 603000, 12826 },  /* Option 04 */
+	{ 0x5, 0x3, 600000, 10000 },  /* Option 05 */
+	{ 0x8, 0x1, 600000, 10000 },  /* Option 08 */
+	{ 0x8, 0xf, 600000, 10000 },  /* Option 08 */
+	{ 0xc, 0xf, 603000, 12826 },  /* Option 09 */
+};
+
+/* I2C-accessible byte-sized registers */
+enum {
+	/* Voltage setting */
+	FAN53555_VSEL0 = 0x00,
+	FAN53555_VSEL1,
+	/* Control register */
+	FAN53555_CONTROL,
+	/* IC Type */
+	FAN53555_ID1,
+	/* IC mask version */
+	FAN53555_ID2,
+	/* Monitor register */
+	FAN53555_MONITOR,
+};
+
+struct fan53555_platdata {
+	/* Voltage setting register */
+	unsigned int vol_reg;
+	unsigned int sleep_reg;
+
+};
+
+struct fan53555_priv {
+	/* IC Vendor */
+	unsigned int vendor;
+	/* IC Type and Rev */
+	unsigned int die_id;
+	unsigned int die_rev;
+	/* Voltage range and step(linear) */
+	unsigned int vsel_min;
+	unsigned int vsel_step;
+	/* Voltage slew rate limiting */
+	unsigned int slew_rate;
+	/* Sleep voltage cache */
+	unsigned int sleep_vol_cache;
+};
+
+static int fan53555_write(struct udevice *dev, uint reg, u8 *buff, int len)
+{
+	int ret;
+
+	ret = dm_i2c_write(dev, reg, buff, len);
+	if (ret) {
+		debug("%s: %s() failed to read reg %d\n",
+		      dev->name, __func__, reg);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int fan53555_read(struct udevice *dev, uint reg, u8 *buff, int len)
+{
+	int ret;
+
+	ret = dm_i2c_read(dev, reg, buff, len);
+	if (ret) {
+		debug("%s: %s() failed to read reg %d\n",
+		      dev->name, __func__, reg);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int fan53555_regulator_ofdata_to_platdata(struct udevice *dev)
+{
+	struct fan53555_platdata *dev_pdata = dev_get_platdata(dev);
+	struct dm_regulator_uclass_platdata *uc_pdata;
+	u32 sleep_vsel;
+
+	uc_pdata = dev_get_uclass_platdata(dev);
+	if (!uc_pdata)
+		return -ENXIO;
+
+	/* This is a buck regulator */
+	uc_pdata->type = REGULATOR_TYPE_BUCK;
+
+	sleep_vsel = dev_read_u32_default(dev, "fcs,suspend-voltage-selector",
+					  FAN53555_VSEL1);
+
+	/*
+	 * Depending on the device-tree settings, the 'normal mode'
+	 * voltage is either controlled by VSEL0 or VSEL1.
+	 */
+	switch (sleep_vsel) {
+	case FAN53555_VSEL0:
+		dev_pdata->sleep_reg = FAN53555_VSEL0;
+		dev_pdata->vol_reg = FAN53555_VSEL1;
+		break;
+	case FAN53555_VSEL1:
+		dev_pdata->sleep_reg = FAN53555_VSEL1;
+		dev_pdata->vol_reg = FAN53555_VSEL0;
+		break;
+	default:
+		pr_err("%s: invalid vsel id %d\n", dev->name, sleep_vsel);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int fan53555_regulator_get_value(struct udevice *dev)
+{
+	struct fan53555_platdata *pdata = dev_get_platdata(dev);
+	struct fan53555_priv *priv = dev_get_priv(dev);
+	u8 vol;
+	int voltage;
+
+	/* We only support a single voltage selector (i.e. 'normal' mode). */
+	fan53555_read(dev, pdata->vol_reg, &vol, 1);
+	voltage = priv->vsel_min + (vol & 0x3f) * priv->vsel_step;
+
+	debug("%s: %d uV\n", __func__, voltage);
+	return voltage;
+}
+
+static int fan53555_regulator_set_value(struct udevice *dev, int uV)
+{
+	struct fan53555_platdata *pdata = dev_get_platdata(dev);
+	struct fan53555_priv *priv = dev_get_priv(dev);
+	u8 vol, oldbits, newbits;
+
+	vol = (uV - priv->vsel_min) / priv->vsel_step;
+	fan53555_read(dev, pdata->vol_reg, &oldbits, 1);
+	newbits = bitfield_replace(oldbits, 0, 6, vol);
+	fan53555_write(dev, pdata->vol_reg, &newbits, 1);
+
+	debug("%s: uV=%d; reg %d: %02x -> %02x\n",
+	      __func__, uV, pdata->vol_reg, oldbits, newbits);
+
+	return 0;
+}
+
+static int fan53555_voltages_setup(struct udevice *dev)
+{
+	struct fan53555_priv *priv = dev_get_priv(dev);
+	struct dm_regulator_uclass_platdata *uc_pdata;
+	int i;
+
+	uc_pdata = dev_get_uclass_platdata(dev);
+	if (!uc_pdata)
+		return -ENXIO;
+
+	/* Init voltage range and step */
+	for (i = 0; i < ARRAY_SIZE(ic_types); ++i) {
+		if (ic_types[i].die_id != priv->die_id)
+			continue;
+
+		if (ic_types[i].die_rev != priv->die_rev)
+			continue;
+
+		priv->vsel_min = ic_types[i].vsel_min;
+		priv->vsel_step = ic_types[i].vsel_step;
+
+		return 0;
+	}
+
+	pr_err("%s: %s: die id %d rev %d not supported!\n",
+	       dev->name, __func__, priv->die_id, priv->die_rev);
+	return -EINVAL;
+}
+
+enum {
+	DIE_ID_SHIFT = 0,
+	DIE_ID_WIDTH = 4,
+	DIE_REV_SHIFT = 0,
+	DIE_REV_WIDTH = 4,
+};
+
+
+static int fan53555_probe(struct udevice *dev)
+{
+	struct fan53555_priv *priv = dev_get_priv(dev);
+	u8 id1, id2;
+
+	/* read chip id: vendor, die-id and die-revision */
+	fan53555_read(dev, FAN53555_ID1, &id1, 1);
+	fan53555_read(dev, FAN53555_ID2, &id2, 1);
+
+	priv->vendor = bitfield_extract(id1, 5, 3);
+	priv->die_id = id1 & GENMASK(3, 0);
+	priv->die_rev = id2 & GENMASK(3, 0);
+
+	if (fan53555_voltages_setup(dev) < 0)
+		return -ENODATA;
+
+	debug("%s: FAN53555 option %d rev %d detected\n",
+	      __func__, priv->die_id, priv->die_rev);
+
+	return 0;
+}
+
+static const struct dm_regulator_ops fan53555_regulator_ops = {
+	.get_value	= fan53555_regulator_get_value,
+	.set_value	= fan53555_regulator_set_value,
+};
+
+static const struct udevice_id fan53555_regulator_ids[] = {
+	{ .compatible = "fcs,fan53555" },
+	{ },
+};
+
+U_BOOT_DRIVER(fan53555_regulator) = {
+	.name = "fan53555 regulator",
+	.id = UCLASS_REGULATOR,
+	.ops = &fan53555_regulator_ops,
+	.of_match = fan53555_regulator_ids,
+	.ofdata_to_platdata = fan53555_regulator_ofdata_to_platdata,
+	.platdata_auto_alloc_size = sizeof(struct fan53555_platdata),
+	.priv_auto_alloc_size = sizeof(struct fan53555_priv),
+	.probe = fan53555_probe,
+};
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH v2 2/2] rockchip: defconfig: puma-rk3399: enable FAN53555 regulator driver
  2017-11-22 21:13 [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family Philipp Tomsich
@ 2017-11-22 21:13 ` Philipp Tomsich
  2017-11-23  9:57 ` [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family Lukasz Majewski
  2017-11-26 11:38 ` Simon Glass
  2 siblings, 0 replies; 8+ messages in thread
From: Philipp Tomsich @ 2017-11-22 21:13 UTC (permalink / raw)
  To: u-boot

With a driver for the FAN53555 regulator family available, let's
enable it for the RK3399-Q7 (which has two of these devices
on-module).

We enable this for the full U-Boot stage only, as these regulators
provide a suitable default voltage and supply non-critical (i.e.
for booting up) power rails only.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

Changes in v2: None

 configs/puma-rk3399_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig
index ebbf8a9..233c15a 100644
--- a/configs/puma-rk3399_defconfig
+++ b/configs/puma-rk3399_defconfig
@@ -67,6 +67,7 @@ CONFIG_PINCTRL_ROCKCHIP_RK3399=y
 CONFIG_DM_PMIC=y
 CONFIG_PMIC_RK8XX=y
 CONFIG_SPL_DM_REGULATOR=y
+CONFIG_DM_REGULATOR_FAN53555=y
 CONFIG_DM_REGULATOR_FIXED=y
 CONFIG_SPL_DM_REGULATOR_FIXED=y
 CONFIG_DM_REGULATOR_GPIO=y
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family
  2017-11-22 21:13 [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family Philipp Tomsich
  2017-11-22 21:13 ` [U-Boot] [PATCH v2 2/2] rockchip: defconfig: puma-rk3399: enable FAN53555 regulator driver Philipp Tomsich
@ 2017-11-23  9:57 ` Lukasz Majewski
  2017-11-26 11:38 ` Simon Glass
  2 siblings, 0 replies; 8+ messages in thread
From: Lukasz Majewski @ 2017-11-23  9:57 UTC (permalink / raw)
  To: u-boot

Hi Philipp,

> This adds a driver for the FAN53555 family of regulators.
> 
> While these devices support a 'normal' and 'suspend' mode (controlled
> via an external pin) to switch between two programmable voltages, this
> incarnation of the driver assumes that the device is always operating
> in 'normal' mode.
> 
> Only setting/reading the programmed voltage is supported at this time
> and the following device functionality remains unsupported:
>   - switching the selected voltage (via a GPIO)
>   - disabling the voltage output via software-control
> This matches the functionality of the Linux driver.
> 
> Tested on a RK3399-Q7 (with 'option 5' devices): setting voltages from
> the U-Boot shell and verifying output voltages on the board.
> 
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
> 
> ---
> 
> Changes in v2:
> - adapted documentation on the device-tree binding from Linux
> 
>  doc/device-tree-bindings/regulator/fan53555.txt |  23 +++
>  drivers/power/regulator/Kconfig                 |  14 ++
>  drivers/power/regulator/Makefile                |   1 +
>  drivers/power/regulator/fan53555.c              | 255
> ++++++++++++++++++++++++ 4 files changed, 293 insertions(+)
>  create mode 100644 doc/device-tree-bindings/regulator/fan53555.txt
>  create mode 100644 drivers/power/regulator/fan53555.c
> 
> diff --git a/doc/device-tree-bindings/regulator/fan53555.txt
> b/doc/device-tree-bindings/regulator/fan53555.txt new file mode 100644
> index 0000000..b183738
> --- /dev/null
> +++ b/doc/device-tree-bindings/regulator/fan53555.txt
> @@ -0,0 +1,23 @@
> +Binding for Fairchild FAN53555 regulators
> +
> +Required properties:
> +  - compatible: "fcs,fan53555"
> +  - reg: I2C address
> +
> +Optional properties:
> +  - fcs,suspend-voltage-selector: declare which of the two available
> +		voltage selector registers should be used for the
> suspend
> +		voltage. The other one is used for the runtime
> voltage setting
> +		Possible values are either <0> or <1>
> +  - vin-supply: regulator supplying the vin pin
> +
> +Example:
> +
> +	regulator at 40 {
> +		compatible = "fcs,fan53555";
> +		regulator-name = "fan53555";
> +		regulator-min-microvolt = <1000000>;
> +		regulator-max-microvolt = <1800000>;
> +		vin-supply = <&parent_reg>;
> +		fcs,suspend-voltage-selector = <1>;
> +	};
> diff --git a/drivers/power/regulator/Kconfig
> b/drivers/power/regulator/Kconfig index 8892fa1..c26a765 100644
> --- a/drivers/power/regulator/Kconfig
> +++ b/drivers/power/regulator/Kconfig
> @@ -69,6 +69,20 @@ config DM_REGULATOR_MAX77686
>  	features for REGULATOR MAX77686. The driver implements
> get/set api for: value, enable and mode.
>  
> +config DM_REGULATOR_FAN53555
> +	bool "Enable Driver Model for REGULATOR FAN53555"
> +	depends on DM_REGULATOR && DM_I2C
> +	---help---
> +	This config enables implementation of driver-model regulator
> uclass
> +	features for the FAN53555 regulator. The FAN53555 is a
> (family of)
> +	single-output regulators that supports transitioning between
> two
> +	different output voltages based on an voltage selection pin.
> +
> +	The driver implements a get/set api for the voltage of the
> 'normal
> +	mode' voltage only. Switching to 'suspend mode' (i.e. the
> alternate
> +	voltage), disabling output via software, or switching the
> mode is
> +	not supported by this driver (at this time).
> +
>  config DM_REGULATOR_FIXED
>  	bool "Enable Driver Model for REGULATOR Fixed value"
>  	depends on DM_REGULATOR
> diff --git a/drivers/power/regulator/Makefile
> b/drivers/power/regulator/Makefile index 6c149a9..21040ea 100644
> --- a/drivers/power/regulator/Makefile
> +++ b/drivers/power/regulator/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_REGULATOR_AS3722)	+=
> as3722_regulator.o obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
>  obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o
>  obj-$(CONFIG_REGULATOR_PWM) += pwm_regulator.o
> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.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/fan53555.c
> b/drivers/power/regulator/fan53555.c new file mode 100644
> index 0000000..3f0b4e9
> --- /dev/null
> +++ b/drivers/power/regulator/fan53555.c
> @@ -0,0 +1,255 @@
> +/*
> + * (C) 2017 Theobroma Systems Design und Consulting GmbH
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <bitfield.h>
> +#include <errno.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <i2c.h>
> +#include <asm/gpio.h>
> +#include <power/pmic.h>
> +#include <power/regulator.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * The voltage ramp (i.e. minimum voltage and step) is defined by the
> + * combination of 2 nibbles: DIE_ID and DIE_REV.
> + *
> + * See http://www.onsemi.com/pub/Collateral/FAN53555-D.pdf for
> details.
> + */
> +static const struct {
> +	u8 die_id;
> +	u8 die_rev;
> +	u32 vsel_min;
> +	u32 vsel_step;
> +} ic_types[] = {
> +	{ 0x0, 0x3, 600000, 10000 },  /* Option 00 */
> +	{ 0x0, 0xf, 800000, 10000 },  /* Option 13 */
> +	{ 0x0, 0xc, 600000, 12500 },  /* Option 23 */
> +	{ 0x1, 0x3, 600000, 10000 },  /* Option 01 */
> +	{ 0x3, 0x3, 600000, 10000 },  /* Option 03 */
> +	{ 0x4, 0xf, 603000, 12826 },  /* Option 04 */
> +	{ 0x5, 0x3, 600000, 10000 },  /* Option 05 */
> +	{ 0x8, 0x1, 600000, 10000 },  /* Option 08 */
> +	{ 0x8, 0xf, 600000, 10000 },  /* Option 08 */
> +	{ 0xc, 0xf, 603000, 12826 },  /* Option 09 */
> +};
> +
> +/* I2C-accessible byte-sized registers */
> +enum {
> +	/* Voltage setting */
> +	FAN53555_VSEL0 = 0x00,
> +	FAN53555_VSEL1,
> +	/* Control register */
> +	FAN53555_CONTROL,
> +	/* IC Type */
> +	FAN53555_ID1,
> +	/* IC mask version */
> +	FAN53555_ID2,
> +	/* Monitor register */
> +	FAN53555_MONITOR,
> +};
> +
> +struct fan53555_platdata {
> +	/* Voltage setting register */
> +	unsigned int vol_reg;
> +	unsigned int sleep_reg;
> +
> +};
> +
> +struct fan53555_priv {
> +	/* IC Vendor */
> +	unsigned int vendor;
> +	/* IC Type and Rev */
> +	unsigned int die_id;
> +	unsigned int die_rev;
> +	/* Voltage range and step(linear) */
> +	unsigned int vsel_min;
> +	unsigned int vsel_step;
> +	/* Voltage slew rate limiting */
> +	unsigned int slew_rate;
> +	/* Sleep voltage cache */
> +	unsigned int sleep_vol_cache;
> +};
> +
> +static int fan53555_write(struct udevice *dev, uint reg, u8 *buff,
> int len) +{
> +	int ret;
> +
> +	ret = dm_i2c_write(dev, reg, buff, len);
> +	if (ret) {
> +		debug("%s: %s() failed to read reg %d\n",
> +		      dev->name, __func__, reg);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fan53555_read(struct udevice *dev, uint reg, u8 *buff,
> int len) +{
> +	int ret;
> +
> +	ret = dm_i2c_read(dev, reg, buff, len);
> +	if (ret) {
> +		debug("%s: %s() failed to read reg %d\n",
> +		      dev->name, __func__, reg);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fan53555_regulator_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct fan53555_platdata *dev_pdata = dev_get_platdata(dev);
> +	struct dm_regulator_uclass_platdata *uc_pdata;
> +	u32 sleep_vsel;
> +
> +	uc_pdata = dev_get_uclass_platdata(dev);
> +	if (!uc_pdata)
> +		return -ENXIO;
> +
> +	/* This is a buck regulator */
> +	uc_pdata->type = REGULATOR_TYPE_BUCK;
> +
> +	sleep_vsel = dev_read_u32_default(dev,
> "fcs,suspend-voltage-selector",
> +					  FAN53555_VSEL1);
> +
> +	/*
> +	 * Depending on the device-tree settings, the 'normal mode'
> +	 * voltage is either controlled by VSEL0 or VSEL1.
> +	 */
> +	switch (sleep_vsel) {
> +	case FAN53555_VSEL0:
> +		dev_pdata->sleep_reg = FAN53555_VSEL0;
> +		dev_pdata->vol_reg = FAN53555_VSEL1;
> +		break;
> +	case FAN53555_VSEL1:
> +		dev_pdata->sleep_reg = FAN53555_VSEL1;
> +		dev_pdata->vol_reg = FAN53555_VSEL0;
> +		break;
> +	default:
> +		pr_err("%s: invalid vsel id %d\n", dev->name,
> sleep_vsel);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fan53555_regulator_get_value(struct udevice *dev)
> +{
> +	struct fan53555_platdata *pdata = dev_get_platdata(dev);
> +	struct fan53555_priv *priv = dev_get_priv(dev);
> +	u8 vol;
> +	int voltage;
> +
> +	/* We only support a single voltage selector (i.e. 'normal'
> mode). */
> +	fan53555_read(dev, pdata->vol_reg, &vol, 1);
> +	voltage = priv->vsel_min + (vol & 0x3f) * priv->vsel_step;
> +
> +	debug("%s: %d uV\n", __func__, voltage);
> +	return voltage;
> +}
> +
> +static int fan53555_regulator_set_value(struct udevice *dev, int uV)
> +{
> +	struct fan53555_platdata *pdata = dev_get_platdata(dev);
> +	struct fan53555_priv *priv = dev_get_priv(dev);
> +	u8 vol, oldbits, newbits;
> +
> +	vol = (uV - priv->vsel_min) / priv->vsel_step;
> +	fan53555_read(dev, pdata->vol_reg, &oldbits, 1);
> +	newbits = bitfield_replace(oldbits, 0, 6, vol);
> +	fan53555_write(dev, pdata->vol_reg, &newbits, 1);
> +
> +	debug("%s: uV=%d; reg %d: %02x -> %02x\n",
> +	      __func__, uV, pdata->vol_reg, oldbits, newbits);
> +
> +	return 0;
> +}
> +
> +static int fan53555_voltages_setup(struct udevice *dev)
> +{
> +	struct fan53555_priv *priv = dev_get_priv(dev);
> +	struct dm_regulator_uclass_platdata *uc_pdata;
> +	int i;
> +
> +	uc_pdata = dev_get_uclass_platdata(dev);
> +	if (!uc_pdata)
> +		return -ENXIO;
> +
> +	/* Init voltage range and step */
> +	for (i = 0; i < ARRAY_SIZE(ic_types); ++i) {
> +		if (ic_types[i].die_id != priv->die_id)
> +			continue;
> +
> +		if (ic_types[i].die_rev != priv->die_rev)
> +			continue;
> +
> +		priv->vsel_min = ic_types[i].vsel_min;
> +		priv->vsel_step = ic_types[i].vsel_step;
> +
> +		return 0;
> +	}
> +
> +	pr_err("%s: %s: die id %d rev %d not supported!\n",
> +	       dev->name, __func__, priv->die_id, priv->die_rev);
> +	return -EINVAL;
> +}
> +
> +enum {
> +	DIE_ID_SHIFT = 0,
> +	DIE_ID_WIDTH = 4,
> +	DIE_REV_SHIFT = 0,
> +	DIE_REV_WIDTH = 4,
> +};
> +
> +
> +static int fan53555_probe(struct udevice *dev)
> +{
> +	struct fan53555_priv *priv = dev_get_priv(dev);
> +	u8 id1, id2;
> +
> +	/* read chip id: vendor, die-id and die-revision */
> +	fan53555_read(dev, FAN53555_ID1, &id1, 1);
> +	fan53555_read(dev, FAN53555_ID2, &id2, 1);
> +
> +	priv->vendor = bitfield_extract(id1, 5, 3);
> +	priv->die_id = id1 & GENMASK(3, 0);
> +	priv->die_rev = id2 & GENMASK(3, 0);
> +
> +	if (fan53555_voltages_setup(dev) < 0)
> +		return -ENODATA;
> +
> +	debug("%s: FAN53555 option %d rev %d detected\n",
> +	      __func__, priv->die_id, priv->die_rev);
> +
> +	return 0;
> +}
> +
> +static const struct dm_regulator_ops fan53555_regulator_ops = {
> +	.get_value	= fan53555_regulator_get_value,
> +	.set_value	= fan53555_regulator_set_value,
> +};
> +
> +static const struct udevice_id fan53555_regulator_ids[] = {
> +	{ .compatible = "fcs,fan53555" },
> +	{ },
> +};
> +
> +U_BOOT_DRIVER(fan53555_regulator) = {
> +	.name = "fan53555 regulator",
> +	.id = UCLASS_REGULATOR,
> +	.ops = &fan53555_regulator_ops,
> +	.of_match = fan53555_regulator_ids,
> +	.ofdata_to_platdata = fan53555_regulator_ofdata_to_platdata,
> +	.platdata_auto_alloc_size = sizeof(struct fan53555_platdata),
> +	.priv_auto_alloc_size = sizeof(struct fan53555_priv),
> +	.probe = fan53555_probe,
> +};

Reviewed-by: Lukasz Majewski <lukma@denx.de>

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171123/53cfda74/attachment.sig>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family
  2017-11-22 21:13 [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family Philipp Tomsich
  2017-11-22 21:13 ` [U-Boot] [PATCH v2 2/2] rockchip: defconfig: puma-rk3399: enable FAN53555 regulator driver Philipp Tomsich
  2017-11-23  9:57 ` [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family Lukasz Majewski
@ 2017-11-26 11:38 ` Simon Glass
  2017-11-26 14:10   ` Dr. Philipp Tomsich
  2 siblings, 1 reply; 8+ messages in thread
From: Simon Glass @ 2017-11-26 11:38 UTC (permalink / raw)
  To: u-boot

Hi Philipp,

On 22 November 2017 at 14:13, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> This adds a driver for the FAN53555 family of regulators.
>
> While these devices support a 'normal' and 'suspend' mode (controlled
> via an external pin) to switch between two programmable voltages, this
> incarnation of the driver assumes that the device is always operating
> in 'normal' mode.
>
> Only setting/reading the programmed voltage is supported at this time
> and the following device functionality remains unsupported:
>   - switching the selected voltage (via a GPIO)
>   - disabling the voltage output via software-control
> This matches the functionality of the Linux driver.
>
> Tested on a RK3399-Q7 (with 'option 5' devices): setting voltages from
> the U-Boot shell and verifying output voltages on the board.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>
> ---
>
> Changes in v2:
> - adapted documentation on the device-tree binding from Linux
>
>  doc/device-tree-bindings/regulator/fan53555.txt |  23 +++
>  drivers/power/regulator/Kconfig                 |  14 ++
>  drivers/power/regulator/Makefile                |   1 +
>  drivers/power/regulator/fan53555.c              | 255 ++++++++++++++++++++++++
>  4 files changed, 293 insertions(+)
>  create mode 100644 doc/device-tree-bindings/regulator/fan53555.txt
>  create mode 100644 drivers/power/regulator/fan53555.c

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

Nits below.

>
> diff --git a/doc/device-tree-bindings/regulator/fan53555.txt b/doc/device-tree-bindings/regulator/fan53555.txt
> new file mode 100644
> index 0000000..b183738
> --- /dev/null
> +++ b/doc/device-tree-bindings/regulator/fan53555.txt
> @@ -0,0 +1,23 @@
> +Binding for Fairchild FAN53555 regulators
> +
> +Required properties:
> +  - compatible: "fcs,fan53555"
> +  - reg: I2C address
> +
> +Optional properties:
> +  - fcs,suspend-voltage-selector: declare which of the two available
> +               voltage selector registers should be used for the suspend
> +               voltage. The other one is used for the runtime voltage setting
> +               Possible values are either <0> or <1>
> +  - vin-supply: regulator supplying the vin pin
> +
> +Example:
> +
> +       regulator at 40 {
> +               compatible = "fcs,fan53555";
> +               regulator-name = "fan53555";
> +               regulator-min-microvolt = <1000000>;
> +               regulator-max-microvolt = <1800000>;
> +               vin-supply = <&parent_reg>;
> +               fcs,suspend-voltage-selector = <1>;
> +       };
> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
> index 8892fa1..c26a765 100644
> --- a/drivers/power/regulator/Kconfig
> +++ b/drivers/power/regulator/Kconfig
> @@ -69,6 +69,20 @@ config DM_REGULATOR_MAX77686
>         features for REGULATOR MAX77686. The driver implements get/set api for:
>         value, enable and mode.
>
> +config DM_REGULATOR_FAN53555
> +       bool "Enable Driver Model for REGULATOR FAN53555"
> +       depends on DM_REGULATOR && DM_I2C
> +       ---help---
> +       This config enables implementation of driver-model regulator uclass
> +       features for the FAN53555 regulator. The FAN53555 is a (family of)
> +       single-output regulators that supports transitioning between two
> +       different output voltages based on an voltage selection pin.
> +
> +       The driver implements a get/set api for the voltage of the 'normal
> +       mode' voltage only. Switching to 'suspend mode' (i.e. the alternate
> +       voltage), disabling output via software, or switching the mode is
> +       not supported by this driver (at this time).
> +
>  config DM_REGULATOR_FIXED
>         bool "Enable Driver Model for REGULATOR Fixed value"
>         depends on DM_REGULATOR
> diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
> index 6c149a9..21040ea 100644
> --- a/drivers/power/regulator/Makefile
> +++ b/drivers/power/regulator/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_REGULATOR_AS3722)        += as3722_regulator.o
>  obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
>  obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o
>  obj-$(CONFIG_REGULATOR_PWM) += pwm_regulator.o
> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.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/fan53555.c b/drivers/power/regulator/fan53555.c
> new file mode 100644
> index 0000000..3f0b4e9
> --- /dev/null
> +++ b/drivers/power/regulator/fan53555.c
> @@ -0,0 +1,255 @@
> +/*
> + * (C) 2017 Theobroma Systems Design und Consulting GmbH
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <bitfield.h>
> +#include <errno.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <i2c.h>
> +#include <asm/gpio.h>
> +#include <power/pmic.h>
> +#include <power/regulator.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * The voltage ramp (i.e. minimum voltage and step) is defined by the
> + * combination of 2 nibbles: DIE_ID and DIE_REV.
> + *
> + * See http://www.onsemi.com/pub/Collateral/FAN53555-D.pdf for details.
> + */

struct comment?

> +static const struct {
> +       u8 die_id;
> +       u8 die_rev;
> +       u32 vsel_min;
> +       u32 vsel_step;
> +} ic_types[] = {
> +       { 0x0, 0x3, 600000, 10000 },  /* Option 00 */
> +       { 0x0, 0xf, 800000, 10000 },  /* Option 13 */
> +       { 0x0, 0xc, 600000, 12500 },  /* Option 23 */
> +       { 0x1, 0x3, 600000, 10000 },  /* Option 01 */
> +       { 0x3, 0x3, 600000, 10000 },  /* Option 03 */
> +       { 0x4, 0xf, 603000, 12826 },  /* Option 04 */
> +       { 0x5, 0x3, 600000, 10000 },  /* Option 05 */
> +       { 0x8, 0x1, 600000, 10000 },  /* Option 08 */
> +       { 0x8, 0xf, 600000, 10000 },  /* Option 08 */
> +       { 0xc, 0xf, 603000, 12826 },  /* Option 09 */
> +};
> +
> +/* I2C-accessible byte-sized registers */
> +enum {
> +       /* Voltage setting */
> +       FAN53555_VSEL0 = 0x00,
> +       FAN53555_VSEL1,
> +       /* Control register */
> +       FAN53555_CONTROL,
> +       /* IC Type */
> +       FAN53555_ID1,
> +       /* IC mask version */
> +       FAN53555_ID2,
> +       /* Monitor register */
> +       FAN53555_MONITOR,
> +};
> +
> +struct fan53555_platdata {
> +       /* Voltage setting register */
> +       unsigned int vol_reg;
> +       unsigned int sleep_reg;
> +
> +};
> +
> +struct fan53555_priv {
> +       /* IC Vendor */
> +       unsigned int vendor;
> +       /* IC Type and Rev */
> +       unsigned int die_id;
> +       unsigned int die_rev;
> +       /* Voltage range and step(linear) */
> +       unsigned int vsel_min;
> +       unsigned int vsel_step;
> +       /* Voltage slew rate limiting */
> +       unsigned int slew_rate;
> +       /* Sleep voltage cache */
> +       unsigned int sleep_vol_cache;
> +};
> +
> +static int fan53555_write(struct udevice *dev, uint reg, u8 *buff, int len)

In this file en is only ever 1. How about using pmic_reg_write()?

> +{
> +       int ret;
> +
> +       ret = dm_i2c_write(dev, reg, buff, len);
> +       if (ret) {
> +               debug("%s: %s() failed to read reg %d\n",
> +                     dev->name, __func__, reg);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int fan53555_read(struct udevice *dev, uint reg, u8 *buff, int len)

pmic_reg_read()?

> +{
> +       int ret;
> +
> +       ret = dm_i2c_read(dev, reg, buff, len);
> +       if (ret) {
> +               debug("%s: %s() failed to read reg %d\n",
> +                     dev->name, __func__, reg);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int fan53555_regulator_ofdata_to_platdata(struct udevice *dev)
> +{
> +       struct fan53555_platdata *dev_pdata = dev_get_platdata(dev);
> +       struct dm_regulator_uclass_platdata *uc_pdata;
> +       u32 sleep_vsel;
> +
> +       uc_pdata = dev_get_uclass_platdata(dev);
> +       if (!uc_pdata)
> +               return -ENXIO;

This cannot happen so this check is unnecessary, and I think it is
confusing too.

> +
> +       /* This is a buck regulator */
> +       uc_pdata->type = REGULATOR_TYPE_BUCK;
> +
> +       sleep_vsel = dev_read_u32_default(dev, "fcs,suspend-voltage-selector",
> +                                         FAN53555_VSEL1);
> +
> +       /*
> +        * Depending on the device-tree settings, the 'normal mode'
> +        * voltage is either controlled by VSEL0 or VSEL1.
> +        */
> +       switch (sleep_vsel) {
> +       case FAN53555_VSEL0:
> +               dev_pdata->sleep_reg = FAN53555_VSEL0;
> +               dev_pdata->vol_reg = FAN53555_VSEL1;
> +               break;
> +       case FAN53555_VSEL1:
> +               dev_pdata->sleep_reg = FAN53555_VSEL1;
> +               dev_pdata->vol_reg = FAN53555_VSEL0;
> +               break;
> +       default:
> +               pr_err("%s: invalid vsel id %d\n", dev->name, sleep_vsel);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int fan53555_regulator_get_value(struct udevice *dev)
> +{
> +       struct fan53555_platdata *pdata = dev_get_platdata(dev);
> +       struct fan53555_priv *priv = dev_get_priv(dev);
> +       u8 vol;
> +       int voltage;
> +
> +       /* We only support a single voltage selector (i.e. 'normal' mode). */
> +       fan53555_read(dev, pdata->vol_reg, &vol, 1);

error check

> +       voltage = priv->vsel_min + (vol & 0x3f) * priv->vsel_step;
> +
> +       debug("%s: %d uV\n", __func__, voltage);
> +       return voltage;
> +}
> +
> +static int fan53555_regulator_set_value(struct udevice *dev, int uV)
> +{
> +       struct fan53555_platdata *pdata = dev_get_platdata(dev);
> +       struct fan53555_priv *priv = dev_get_priv(dev);
> +       u8 vol, oldbits, newbits;
> +
> +       vol = (uV - priv->vsel_min) / priv->vsel_step;
> +       fan53555_read(dev, pdata->vol_reg, &oldbits, 1);

more error checks here and below

> +       newbits = bitfield_replace(oldbits, 0, 6, vol);
> +       fan53555_write(dev, pdata->vol_reg, &newbits, 1);
> +
> +       debug("%s: uV=%d; reg %d: %02x -> %02x\n",
> +             __func__, uV, pdata->vol_reg, oldbits, newbits);
> +
> +       return 0;
> +}
> +
> +static int fan53555_voltages_setup(struct udevice *dev)
> +{
> +       struct fan53555_priv *priv = dev_get_priv(dev);
> +       struct dm_regulator_uclass_platdata *uc_pdata;
> +       int i;
> +
> +       uc_pdata = dev_get_uclass_platdata(dev);
> +       if (!uc_pdata)
> +               return -ENXIO;

Again, please drop this check

> +
> +       /* Init voltage range and step */
> +       for (i = 0; i < ARRAY_SIZE(ic_types); ++i) {
> +               if (ic_types[i].die_id != priv->die_id)
> +                       continue;
> +
> +               if (ic_types[i].die_rev != priv->die_rev)
> +                       continue;
> +
> +               priv->vsel_min = ic_types[i].vsel_min;
> +               priv->vsel_step = ic_types[i].vsel_step;
> +
> +               return 0;
> +       }
> +
> +       pr_err("%s: %s: die id %d rev %d not supported!\n",
> +              dev->name, __func__, priv->die_id, priv->die_rev);
> +       return -EINVAL;
> +}
> +
> +enum {
> +       DIE_ID_SHIFT = 0,
> +       DIE_ID_WIDTH = 4,
> +       DIE_REV_SHIFT = 0,
> +       DIE_REV_WIDTH = 4,
> +};
> +
> +
> +static int fan53555_probe(struct udevice *dev)
> +{
> +       struct fan53555_priv *priv = dev_get_priv(dev);
> +       u8 id1, id2;
> +
> +       /* read chip id: vendor, die-id and die-revision */
> +       fan53555_read(dev, FAN53555_ID1, &id1, 1);
> +       fan53555_read(dev, FAN53555_ID2, &id2, 1);
> +
> +       priv->vendor = bitfield_extract(id1, 5, 3);
> +       priv->die_id = id1 & GENMASK(3, 0);
> +       priv->die_rev = id2 & GENMASK(3, 0);
> +
> +       if (fan53555_voltages_setup(dev) < 0)
> +               return -ENODATA;
> +
> +       debug("%s: FAN53555 option %d rev %d detected\n",
> +             __func__, priv->die_id, priv->die_rev);
> +
> +       return 0;
> +}
> +
> +static const struct dm_regulator_ops fan53555_regulator_ops = {
> +       .get_value      = fan53555_regulator_get_value,
> +       .set_value      = fan53555_regulator_set_value,
> +};
> +
> +static const struct udevice_id fan53555_regulator_ids[] = {
> +       { .compatible = "fcs,fan53555" },
> +       { },
> +};
> +
> +U_BOOT_DRIVER(fan53555_regulator) = {
> +       .name = "fan53555 regulator",
> +       .id = UCLASS_REGULATOR,
> +       .ops = &fan53555_regulator_ops,
> +       .of_match = fan53555_regulator_ids,
> +       .ofdata_to_platdata = fan53555_regulator_ofdata_to_platdata,
> +       .platdata_auto_alloc_size = sizeof(struct fan53555_platdata),
> +       .priv_auto_alloc_size = sizeof(struct fan53555_priv),
> +       .probe = fan53555_probe,
> +};
> --
> 2.1.4
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family
  2017-11-26 11:38 ` Simon Glass
@ 2017-11-26 14:10   ` Dr. Philipp Tomsich
  2017-11-27  3:07     ` Simon Glass
  0 siblings, 1 reply; 8+ messages in thread
From: Dr. Philipp Tomsich @ 2017-11-26 14:10 UTC (permalink / raw)
  To: u-boot


> On 26 Nov 2017, at 12:38, Simon Glass <sjg@chromium.org> wrote:
> 
> Hi Philipp,
> 
> On 22 November 2017 at 14:13, Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>> This adds a driver for the FAN53555 family of regulators.
>> 
>> While these devices support a 'normal' and 'suspend' mode (controlled
>> via an external pin) to switch between two programmable voltages, this
>> incarnation of the driver assumes that the device is always operating
>> in 'normal' mode.
>> 
>> Only setting/reading the programmed voltage is supported at this time
>> and the following device functionality remains unsupported:
>>  - switching the selected voltage (via a GPIO)
>>  - disabling the voltage output via software-control
>> This matches the functionality of the Linux driver.
>> 
>> Tested on a RK3399-Q7 (with 'option 5' devices): setting voltages from
>> the U-Boot shell and verifying output voltages on the board.
>> 
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>> 
>> ---
>> 
>> Changes in v2:
>> - adapted documentation on the device-tree binding from Linux
>> 
>> doc/device-tree-bindings/regulator/fan53555.txt |  23 +++
>> drivers/power/regulator/Kconfig                 |  14 ++
>> drivers/power/regulator/Makefile                |   1 +
>> drivers/power/regulator/fan53555.c              | 255 ++++++++++++++++++++++++
>> 4 files changed, 293 insertions(+)
>> create mode 100644 doc/device-tree-bindings/regulator/fan53555.txt
>> create mode 100644 drivers/power/regulator/fan53555.c
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Nits below.
> 
>> 
>> diff --git a/doc/device-tree-bindings/regulator/fan53555.txt b/doc/device-tree-bindings/regulator/fan53555.txt
>> new file mode 100644
>> index 0000000..b183738
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/regulator/fan53555.txt
>> @@ -0,0 +1,23 @@
>> +Binding for Fairchild FAN53555 regulators
>> +
>> +Required properties:
>> +  - compatible: "fcs,fan53555"
>> +  - reg: I2C address
>> +
>> +Optional properties:
>> +  - fcs,suspend-voltage-selector: declare which of the two available
>> +               voltage selector registers should be used for the suspend
>> +               voltage. The other one is used for the runtime voltage setting
>> +               Possible values are either <0> or <1>
>> +  - vin-supply: regulator supplying the vin pin
>> +
>> +Example:
>> +
>> +       regulator at 40 {
>> +               compatible = "fcs,fan53555";
>> +               regulator-name = "fan53555";
>> +               regulator-min-microvolt = <1000000>;
>> +               regulator-max-microvolt = <1800000>;
>> +               vin-supply = <&parent_reg>;
>> +               fcs,suspend-voltage-selector = <1>;
>> +       };
>> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
>> index 8892fa1..c26a765 100644
>> --- a/drivers/power/regulator/Kconfig
>> +++ b/drivers/power/regulator/Kconfig
>> @@ -69,6 +69,20 @@ config DM_REGULATOR_MAX77686
>>        features for REGULATOR MAX77686. The driver implements get/set api for:
>>        value, enable and mode.
>> 
>> +config DM_REGULATOR_FAN53555
>> +       bool "Enable Driver Model for REGULATOR FAN53555"
>> +       depends on DM_REGULATOR && DM_I2C
>> +       ---help---
>> +       This config enables implementation of driver-model regulator uclass
>> +       features for the FAN53555 regulator. The FAN53555 is a (family of)
>> +       single-output regulators that supports transitioning between two
>> +       different output voltages based on an voltage selection pin.
>> +
>> +       The driver implements a get/set api for the voltage of the 'normal
>> +       mode' voltage only. Switching to 'suspend mode' (i.e. the alternate
>> +       voltage), disabling output via software, or switching the mode is
>> +       not supported by this driver (at this time).
>> +
>> config DM_REGULATOR_FIXED
>>        bool "Enable Driver Model for REGULATOR Fixed value"
>>        depends on DM_REGULATOR
>> diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
>> index 6c149a9..21040ea 100644
>> --- a/drivers/power/regulator/Makefile
>> +++ b/drivers/power/regulator/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_REGULATOR_AS3722)        += as3722_regulator.o
>> obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
>> obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o
>> obj-$(CONFIG_REGULATOR_PWM) += pwm_regulator.o
>> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.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/fan53555.c b/drivers/power/regulator/fan53555.c
>> new file mode 100644
>> index 0000000..3f0b4e9
>> --- /dev/null
>> +++ b/drivers/power/regulator/fan53555.c
>> @@ -0,0 +1,255 @@
>> +/*
>> + * (C) 2017 Theobroma Systems Design und Consulting GmbH
>> + *
>> + * SPDX-License-Identifier:     GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <bitfield.h>
>> +#include <errno.h>
>> +#include <dm.h>
>> +#include <fdtdec.h>
>> +#include <i2c.h>
>> +#include <asm/gpio.h>
>> +#include <power/pmic.h>
>> +#include <power/regulator.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +/*
>> + * The voltage ramp (i.e. minimum voltage and step) is defined by the
>> + * combination of 2 nibbles: DIE_ID and DIE_REV.
>> + *
>> + * See http://www.onsemi.com/pub/Collateral/FAN53555-D.pdf for details.
>> + */
> 
> struct comment?
> 
>> +static const struct {
>> +       u8 die_id;
>> +       u8 die_rev;
>> +       u32 vsel_min;
>> +       u32 vsel_step;
>> +} ic_types[] = {
>> +       { 0x0, 0x3, 600000, 10000 },  /* Option 00 */
>> +       { 0x0, 0xf, 800000, 10000 },  /* Option 13 */
>> +       { 0x0, 0xc, 600000, 12500 },  /* Option 23 */
>> +       { 0x1, 0x3, 600000, 10000 },  /* Option 01 */
>> +       { 0x3, 0x3, 600000, 10000 },  /* Option 03 */
>> +       { 0x4, 0xf, 603000, 12826 },  /* Option 04 */
>> +       { 0x5, 0x3, 600000, 10000 },  /* Option 05 */
>> +       { 0x8, 0x1, 600000, 10000 },  /* Option 08 */
>> +       { 0x8, 0xf, 600000, 10000 },  /* Option 08 */
>> +       { 0xc, 0xf, 603000, 12826 },  /* Option 09 */
>> +};
>> +
>> +/* I2C-accessible byte-sized registers */
>> +enum {
>> +       /* Voltage setting */
>> +       FAN53555_VSEL0 = 0x00,
>> +       FAN53555_VSEL1,
>> +       /* Control register */
>> +       FAN53555_CONTROL,
>> +       /* IC Type */
>> +       FAN53555_ID1,
>> +       /* IC mask version */
>> +       FAN53555_ID2,
>> +       /* Monitor register */
>> +       FAN53555_MONITOR,
>> +};
>> +
>> +struct fan53555_platdata {
>> +       /* Voltage setting register */
>> +       unsigned int vol_reg;
>> +       unsigned int sleep_reg;
>> +
>> +};
>> +
>> +struct fan53555_priv {
>> +       /* IC Vendor */
>> +       unsigned int vendor;
>> +       /* IC Type and Rev */
>> +       unsigned int die_id;
>> +       unsigned int die_rev;
>> +       /* Voltage range and step(linear) */
>> +       unsigned int vsel_min;
>> +       unsigned int vsel_step;
>> +       /* Voltage slew rate limiting */
>> +       unsigned int slew_rate;
>> +       /* Sleep voltage cache */
>> +       unsigned int sleep_vol_cache;
>> +};
>> +
>> +static int fan53555_write(struct udevice *dev, uint reg, u8 *buff, int len)
> 
> In this file en is only ever 1. How about using pmic_reg_write()?

pmic_reg_write would require the regulator to be part of a PMIC
device (i.e. have the pmic as a parent).  This is a pure regulator
that is not part of a PMIC.

If the intent is to not have such devices, I can model this as a
PMIC with a single regulator...

> 
>> +{
>> +       int ret;
>> +
>> +       ret = dm_i2c_write(dev, reg, buff, len);
>> +       if (ret) {
>> +               debug("%s: %s() failed to read reg %d\n",
>> +                     dev->name, __func__, reg);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int fan53555_read(struct udevice *dev, uint reg, u8 *buff, int len)
> 
> pmic_reg_read()?
> 
>> +{
>> +       int ret;
>> +
>> +       ret = dm_i2c_read(dev, reg, buff, len);
>> +       if (ret) {
>> +               debug("%s: %s() failed to read reg %d\n",
>> +                     dev->name, __func__, reg);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int fan53555_regulator_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +       struct fan53555_platdata *dev_pdata = dev_get_platdata(dev);
>> +       struct dm_regulator_uclass_platdata *uc_pdata;
>> +       u32 sleep_vsel;
>> +
>> +       uc_pdata = dev_get_uclass_platdata(dev);
>> +       if (!uc_pdata)
>> +               return -ENXIO;
> 
> This cannot happen so this check is unnecessary, and I think it is
> confusing too.
> 
>> +
>> +       /* This is a buck regulator */
>> +       uc_pdata->type = REGULATOR_TYPE_BUCK;
>> +
>> +       sleep_vsel = dev_read_u32_default(dev, "fcs,suspend-voltage-selector",
>> +                                         FAN53555_VSEL1);
>> +
>> +       /*
>> +        * Depending on the device-tree settings, the 'normal mode'
>> +        * voltage is either controlled by VSEL0 or VSEL1.
>> +        */
>> +       switch (sleep_vsel) {
>> +       case FAN53555_VSEL0:
>> +               dev_pdata->sleep_reg = FAN53555_VSEL0;
>> +               dev_pdata->vol_reg = FAN53555_VSEL1;
>> +               break;
>> +       case FAN53555_VSEL1:
>> +               dev_pdata->sleep_reg = FAN53555_VSEL1;
>> +               dev_pdata->vol_reg = FAN53555_VSEL0;
>> +               break;
>> +       default:
>> +               pr_err("%s: invalid vsel id %d\n", dev->name, sleep_vsel);
>> +               return -EINVAL;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int fan53555_regulator_get_value(struct udevice *dev)
>> +{
>> +       struct fan53555_platdata *pdata = dev_get_platdata(dev);
>> +       struct fan53555_priv *priv = dev_get_priv(dev);
>> +       u8 vol;
>> +       int voltage;
>> +
>> +       /* We only support a single voltage selector (i.e. 'normal' mode). */
>> +       fan53555_read(dev, pdata->vol_reg, &vol, 1);
> 
> error check
> 
>> +       voltage = priv->vsel_min + (vol & 0x3f) * priv->vsel_step;
>> +
>> +       debug("%s: %d uV\n", __func__, voltage);
>> +       return voltage;
>> +}
>> +
>> +static int fan53555_regulator_set_value(struct udevice *dev, int uV)
>> +{
>> +       struct fan53555_platdata *pdata = dev_get_platdata(dev);
>> +       struct fan53555_priv *priv = dev_get_priv(dev);
>> +       u8 vol, oldbits, newbits;
>> +
>> +       vol = (uV - priv->vsel_min) / priv->vsel_step;
>> +       fan53555_read(dev, pdata->vol_reg, &oldbits, 1);
> 
> more error checks here and below
> 
>> +       newbits = bitfield_replace(oldbits, 0, 6, vol);
>> +       fan53555_write(dev, pdata->vol_reg, &newbits, 1);
>> +
>> +       debug("%s: uV=%d; reg %d: %02x -> %02x\n",
>> +             __func__, uV, pdata->vol_reg, oldbits, newbits);
>> +
>> +       return 0;
>> +}
>> +
>> +static int fan53555_voltages_setup(struct udevice *dev)
>> +{
>> +       struct fan53555_priv *priv = dev_get_priv(dev);
>> +       struct dm_regulator_uclass_platdata *uc_pdata;
>> +       int i;
>> +
>> +       uc_pdata = dev_get_uclass_platdata(dev);
>> +       if (!uc_pdata)
>> +               return -ENXIO;
> 
> Again, please drop this check
> 
>> +
>> +       /* Init voltage range and step */
>> +       for (i = 0; i < ARRAY_SIZE(ic_types); ++i) {
>> +               if (ic_types[i].die_id != priv->die_id)
>> +                       continue;
>> +
>> +               if (ic_types[i].die_rev != priv->die_rev)
>> +                       continue;
>> +
>> +               priv->vsel_min = ic_types[i].vsel_min;
>> +               priv->vsel_step = ic_types[i].vsel_step;
>> +
>> +               return 0;
>> +       }
>> +
>> +       pr_err("%s: %s: die id %d rev %d not supported!\n",
>> +              dev->name, __func__, priv->die_id, priv->die_rev);
>> +       return -EINVAL;
>> +}
>> +
>> +enum {
>> +       DIE_ID_SHIFT = 0,
>> +       DIE_ID_WIDTH = 4,
>> +       DIE_REV_SHIFT = 0,
>> +       DIE_REV_WIDTH = 4,
>> +};
>> +
>> +
>> +static int fan53555_probe(struct udevice *dev)
>> +{
>> +       struct fan53555_priv *priv = dev_get_priv(dev);
>> +       u8 id1, id2;
>> +
>> +       /* read chip id: vendor, die-id and die-revision */
>> +       fan53555_read(dev, FAN53555_ID1, &id1, 1);
>> +       fan53555_read(dev, FAN53555_ID2, &id2, 1);
>> +
>> +       priv->vendor = bitfield_extract(id1, 5, 3);
>> +       priv->die_id = id1 & GENMASK(3, 0);
>> +       priv->die_rev = id2 & GENMASK(3, 0);
>> +
>> +       if (fan53555_voltages_setup(dev) < 0)
>> +               return -ENODATA;
>> +
>> +       debug("%s: FAN53555 option %d rev %d detected\n",
>> +             __func__, priv->die_id, priv->die_rev);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct dm_regulator_ops fan53555_regulator_ops = {
>> +       .get_value      = fan53555_regulator_get_value,
>> +       .set_value      = fan53555_regulator_set_value,
>> +};
>> +
>> +static const struct udevice_id fan53555_regulator_ids[] = {
>> +       { .compatible = "fcs,fan53555" },
>> +       { },
>> +};
>> +
>> +U_BOOT_DRIVER(fan53555_regulator) = {
>> +       .name = "fan53555 regulator",
>> +       .id = UCLASS_REGULATOR,
>> +       .ops = &fan53555_regulator_ops,
>> +       .of_match = fan53555_regulator_ids,
>> +       .ofdata_to_platdata = fan53555_regulator_ofdata_to_platdata,
>> +       .platdata_auto_alloc_size = sizeof(struct fan53555_platdata),
>> +       .priv_auto_alloc_size = sizeof(struct fan53555_priv),
>> +       .probe = fan53555_probe,
>> +};
>> --
>> 2.1.4
>> 
> 
> Regards,
> Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family
  2017-11-26 14:10   ` Dr. Philipp Tomsich
@ 2017-11-27  3:07     ` Simon Glass
  2017-11-27 20:49       ` Dr. Philipp Tomsich
  0 siblings, 1 reply; 8+ messages in thread
From: Simon Glass @ 2017-11-27  3:07 UTC (permalink / raw)
  To: u-boot

Hi Philipp,

On 26 November 2017 at 07:10, Dr. Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
>
>> On 26 Nov 2017, at 12:38, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Philipp,
>>
>> On 22 November 2017 at 14:13, Philipp Tomsich
>> <philipp.tomsich@theobroma-systems.com> wrote:
>>> This adds a driver for the FAN53555 family of regulators.
>>>
>>> While these devices support a 'normal' and 'suspend' mode (controlled
>>> via an external pin) to switch between two programmable voltages, this
>>> incarnation of the driver assumes that the device is always operating
>>> in 'normal' mode.
>>>
>>> Only setting/reading the programmed voltage is supported at this time
>>> and the following device functionality remains unsupported:
>>>  - switching the selected voltage (via a GPIO)
>>>  - disabling the voltage output via software-control
>>> This matches the functionality of the Linux driver.
>>>
>>> Tested on a RK3399-Q7 (with 'option 5' devices): setting voltages from
>>> the U-Boot shell and verifying output voltages on the board.
>>>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - adapted documentation on the device-tree binding from Linux
>>>
>>> doc/device-tree-bindings/regulator/fan53555.txt |  23 +++
>>> drivers/power/regulator/Kconfig                 |  14 ++
>>> drivers/power/regulator/Makefile                |   1 +
>>> drivers/power/regulator/fan53555.c              | 255 ++++++++++++++++++++++++
>>> 4 files changed, 293 insertions(+)
>>> create mode 100644 doc/device-tree-bindings/regulator/fan53555.txt
>>> create mode 100644 drivers/power/regulator/fan53555.c
[...]

>>> +static int fan53555_write(struct udevice *dev, uint reg, u8 *buff, int len)
>>
>> In this file en is only ever 1. How about using pmic_reg_write()?
>
> pmic_reg_write would require the regulator to be part of a PMIC
> device (i.e. have the pmic as a parent).  This is a pure regulator
> that is not part of a PMIC.
>
> If the intent is to not have such devices, I can model this as a
> PMIC with a single regulator...

Yes I *think* all regulators should have a PMIC as a parent. A PMIC is
a type of multi-function device.

Regards,
Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family
  2017-11-27  3:07     ` Simon Glass
@ 2017-11-27 20:49       ` Dr. Philipp Tomsich
  2018-11-16  2:05         ` Simon Glass
  0 siblings, 1 reply; 8+ messages in thread
From: Dr. Philipp Tomsich @ 2017-11-27 20:49 UTC (permalink / raw)
  To: u-boot


> On 27 Nov 2017, at 04:07, Simon Glass <sjg@chromium.org> wrote:
> 
> Hi Philipp,
> 
> On 26 November 2017 at 07:10, Dr. Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>> 
>>> On 26 Nov 2017, at 12:38, Simon Glass <sjg@chromium.org> wrote:
>>> 
>>> Hi Philipp,
>>> 
>>> On 22 November 2017 at 14:13, Philipp Tomsich
>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>> This adds a driver for the FAN53555 family of regulators.
>>>> 
>>>> While these devices support a 'normal' and 'suspend' mode (controlled
>>>> via an external pin) to switch between two programmable voltages, this
>>>> incarnation of the driver assumes that the device is always operating
>>>> in 'normal' mode.
>>>> 
>>>> Only setting/reading the programmed voltage is supported at this time
>>>> and the following device functionality remains unsupported:
>>>> - switching the selected voltage (via a GPIO)
>>>> - disabling the voltage output via software-control
>>>> This matches the functionality of the Linux driver.
>>>> 
>>>> Tested on a RK3399-Q7 (with 'option 5' devices): setting voltages from
>>>> the U-Boot shell and verifying output voltages on the board.
>>>> 
>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>>>> 
>>>> ---
>>>> 
>>>> Changes in v2:
>>>> - adapted documentation on the device-tree binding from Linux
>>>> 
>>>> doc/device-tree-bindings/regulator/fan53555.txt |  23 +++
>>>> drivers/power/regulator/Kconfig                 |  14 ++
>>>> drivers/power/regulator/Makefile                |   1 +
>>>> drivers/power/regulator/fan53555.c              | 255 ++++++++++++++++++++++++
>>>> 4 files changed, 293 insertions(+)
>>>> create mode 100644 doc/device-tree-bindings/regulator/fan53555.txt
>>>> create mode 100644 drivers/power/regulator/fan53555.c
> [...]
> 
>>>> +static int fan53555_write(struct udevice *dev, uint reg, u8 *buff, int len)
>>> 
>>> In this file en is only ever 1. How about using pmic_reg_write()?
>> 
>> pmic_reg_write would require the regulator to be part of a PMIC
>> device (i.e. have the pmic as a parent).  This is a pure regulator
>> that is not part of a PMIC.
>> 
>> If the intent is to not have such devices, I can model this as a
>> PMIC with a single regulator...
> 
> Yes I *think* all regulators should have a PMIC as a parent. A PMIC is
> a type of multi-function device.

I disagree, although the fan53555 (if we ever fully implement support for
it beyond what Linux supports) might fall into a grey area.

Let’s say, I add the intermediate PMIC (which is a quick change and I was
tempted to do it instead of keeping this discussion alive, but I think this is
worthwhile to be discussed a bit further):
1.	The DM tree would not correspond to the DTS, as the regulator is
	modelled below the I2C-device for the FAN53555 and (if we assume
	this node becomes the PMIC) there’s no regulators below that.
2.	Consequently, I’d need to implement a custom bind() method for this
	PMIC just to bind a single regulator below it.

This sounds very wasteful, as the only thing I’d gain would be the use of
the pmic_reg_*() family of functions.  Admittedly, I’d like to to have use of
this IO-abstraction, but it still seems a bit wasteful to do this.

Now, let’s assume we ever implement the advanced functions of the
regulator, which adds switching between two set voltages (routed to a
single output); this would give two options of modelling it (if there was
a PMIC device in the hierarchy):
A.	create two children for VSEL1 and VSEL2 and provide a set-voltage
	method for each… and not associate the GPIO for switching between
	these with any of them
B.	use the regulator framework (and manage the GPIO as part of the
	set-mode verb/action) … and have a single child below the PMIC
	again.

Now, in that case, I’d go with option B.

So in summary: we’ll always end up with a single regulator below a
PMIC that will need to provide a custom bind-op… but I’d be able to
reuse the pmic_reg_* functions.

I am not convinced yet, but I’ll go with whatever we decide here.

Cheers,
Philipp.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family
  2017-11-27 20:49       ` Dr. Philipp Tomsich
@ 2018-11-16  2:05         ` Simon Glass
  0 siblings, 0 replies; 8+ messages in thread
From: Simon Glass @ 2018-11-16  2:05 UTC (permalink / raw)
  To: u-boot

Hi Philipp,

On 27 November 2017 at 12:49, Dr. Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
>
>> On 27 Nov 2017, at 04:07, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Philipp,
>>
>> On 26 November 2017 at 07:10, Dr. Philipp Tomsich
>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>
>>>> On 26 Nov 2017, at 12:38, Simon Glass <sjg@chromium.org> wrote:
>>>>
>>>> Hi Philipp,
>>>>
>>>> On 22 November 2017 at 14:13, Philipp Tomsich
>>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>>> This adds a driver for the FAN53555 family of regulators.
>>>>>
>>>>> While these devices support a 'normal' and 'suspend' mode (controlled
>>>>> via an external pin) to switch between two programmable voltages, this
>>>>> incarnation of the driver assumes that the device is always operating
>>>>> in 'normal' mode.
>>>>>
>>>>> Only setting/reading the programmed voltage is supported at this time
>>>>> and the following device functionality remains unsupported:
>>>>> - switching the selected voltage (via a GPIO)
>>>>> - disabling the voltage output via software-control
>>>>> This matches the functionality of the Linux driver.
>>>>>
>>>>> Tested on a RK3399-Q7 (with 'option 5' devices): setting voltages from
>>>>> the U-Boot shell and verifying output voltages on the board.
>>>>>
>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - adapted documentation on the device-tree binding from Linux
>>>>>
>>>>> doc/device-tree-bindings/regulator/fan53555.txt |  23 +++
>>>>> drivers/power/regulator/Kconfig                 |  14 ++
>>>>> drivers/power/regulator/Makefile                |   1 +
>>>>> drivers/power/regulator/fan53555.c              | 255 ++++++++++++++++++++++++
>>>>> 4 files changed, 293 insertions(+)
>>>>> create mode 100644 doc/device-tree-bindings/regulator/fan53555.txt
>>>>> create mode 100644 drivers/power/regulator/fan53555.c
>> [...]
>>
>>>>> +static int fan53555_write(struct udevice *dev, uint reg, u8 *buff, int len)
>>>>
>>>> In this file en is only ever 1. How about using pmic_reg_write()?
>>>
>>> pmic_reg_write would require the regulator to be part of a PMIC
>>> device (i.e. have the pmic as a parent).  This is a pure regulator
>>> that is not part of a PMIC.
>>>
>>> If the intent is to not have such devices, I can model this as a
>>> PMIC with a single regulator...
>>
>> Yes I *think* all regulators should have a PMIC as a parent. A PMIC is
>> a type of multi-function device.
>
> I disagree, although the fan53555 (if we ever fully implement support for
> it beyond what Linux supports) might fall into a grey area.
>
> Let’s say, I add the intermediate PMIC (which is a quick change and I was
> tempted to do it instead of keeping this discussion alive, but I think this is
> worthwhile to be discussed a bit further):
> 1.      The DM tree would not correspond to the DTS, as the regulator is
>         modelled below the I2C-device for the FAN53555 and (if we assume
>         this node becomes the PMIC) there’s no regulators below that.
> 2.      Consequently, I’d need to implement a custom bind() method for this
>         PMIC just to bind a single regulator below it.
>
> This sounds very wasteful, as the only thing I’d gain would be the use of
> the pmic_reg_*() family of functions.  Admittedly, I’d like to to have use of
> this IO-abstraction, but it still seems a bit wasteful to do this.
>
> Now, let’s assume we ever implement the advanced functions of the
> regulator, which adds switching between two set voltages (routed to a
> single output); this would give two options of modelling it (if there was
> a PMIC device in the hierarchy):
> A.      create two children for VSEL1 and VSEL2 and provide a set-voltage
>         method for each… and not associate the GPIO for switching between
>         these with any of them
> B.      use the regulator framework (and manage the GPIO as part of the
>         set-mode verb/action) … and have a single child below the PMIC
>         again.
>
> Now, in that case, I’d go with option B.
>
> So in summary: we’ll always end up with a single regulator below a
> PMIC that will need to provide a custom bind-op… but I’d be able to
> reuse the pmic_reg_* functions.
>
> I am not convinced yet, but I’ll go with whatever we decide here.

Long delay, but I found this email again.

At present most regulators have a PMIC as a parent, but I don't see
anywhere where it is required. Perhaps we should document what should
be done here?

Regards,
Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-11-16  2:05 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-22 21:13 [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family Philipp Tomsich
2017-11-22 21:13 ` [U-Boot] [PATCH v2 2/2] rockchip: defconfig: puma-rk3399: enable FAN53555 regulator driver Philipp Tomsich
2017-11-23  9:57 ` [U-Boot] [PATCH v2 1/2] power: regulator: add driver for the FAN53555 family Lukasz Majewski
2017-11-26 11:38 ` Simon Glass
2017-11-26 14:10   ` Dr. Philipp Tomsich
2017-11-27  3:07     ` Simon Glass
2017-11-27 20:49       ` Dr. Philipp Tomsich
2018-11-16  2:05         ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.