All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] power: regulator: add driver for ANATOP regulator
@ 2021-03-23 13:48 Ying-Chun Liu
  2021-03-23 13:48 ` [PATCH v2 1/2] " Ying-Chun Liu
  2021-03-23 13:48 ` [PATCH v2 2/2] doc: device-tree-bindings: regulator: anatop regulator Ying-Chun Liu
  0 siblings, 2 replies; 6+ messages in thread
From: Ying-Chun Liu @ 2021-03-23 13:48 UTC (permalink / raw)
  To: u-boot

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop is an integrated regulator inside i.MX6 SoC.
There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
This patch adds the Anatop regulator driver.

Ying-Chun Liu (PaulLiu) (2):
  power: regulator: add driver for ANATOP regulator
  doc: device-tree-bindings: regulator: anatop regulator

v2: add functions for set selector and delay. Define ANATOP_REGULATOR_STEP

 .../regulator/fsl,anatop-regulator.txt        |  45 +++
 drivers/power/regulator/Kconfig               |  10 +
 drivers/power/regulator/Makefile              |   1 +
 drivers/power/regulator/anatop_regulator.c    | 299 ++++++++++++++++++
 4 files changed, 355 insertions(+)
 create mode 100644 doc/device-tree-bindings/regulator/fsl,anatop-regulator.txt
 create mode 100644 drivers/power/regulator/anatop_regulator.c

-- 
2.30.2

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

* [PATCH v2 1/2] power: regulator: add driver for ANATOP regulator
  2021-03-23 13:48 [PATCH v2 0/2] power: regulator: add driver for ANATOP regulator Ying-Chun Liu
@ 2021-03-23 13:48 ` Ying-Chun Liu
  2021-03-23 15:06   ` Sean Anderson
  2021-03-23 13:48 ` [PATCH v2 2/2] doc: device-tree-bindings: regulator: anatop regulator Ying-Chun Liu
  1 sibling, 1 reply; 6+ messages in thread
From: Ying-Chun Liu @ 2021-03-23 13:48 UTC (permalink / raw)
  To: u-boot

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop is an integrated regulator inside i.MX6 SoC.
There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
This patch adds the Anatop regulator driver.

Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
---
v2: add functions for set selector and delay. Define ANATOP_REGULATOR_STEP
---
 drivers/power/regulator/Kconfig            |  10 +
 drivers/power/regulator/Makefile           |   1 +
 drivers/power/regulator/anatop_regulator.c | 299 +++++++++++++++++++++
 3 files changed, 310 insertions(+)
 create mode 100644 drivers/power/regulator/anatop_regulator.c

diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
index fbbea18c7d..1cd1f3f5ed 100644
--- a/drivers/power/regulator/Kconfig
+++ b/drivers/power/regulator/Kconfig
@@ -312,6 +312,16 @@ config DM_REGULATOR_STPMIC1
 	by the PMIC device. This driver is controlled by a device tree node
 	which includes voltage limits.
 
+config DM_REGULATOR_ANATOP
+	bool "Enable driver for ANATOP regulators"
+	depends on DM_REGULATOR
+	select REGMAP
+	select SYSCON
+	help
+	Enable support for the Freescale i.MX on-chip ANATOP LDOs
+	regulators. It is recommended that this option be enabled on
+	i.MX6 platform.
+
 config SPL_DM_REGULATOR_STPMIC1
 	bool "Enable driver for STPMIC1 regulators in SPL"
 	depends on SPL_DM_REGULATOR && PMIC_STPMIC1
diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
index 9d58112dcb..e7198da911 100644
--- a/drivers/power/regulator/Makefile
+++ b/drivers/power/regulator/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_DM_REGULATOR_TPS65910) += tps65910_regulator.o
 obj-$(CONFIG_DM_REGULATOR_TPS62360) += tps62360_regulator.o
 obj-$(CONFIG_$(SPL_)DM_REGULATOR_STPMIC1) += stpmic1.o
 obj-$(CONFIG_DM_REGULATOR_TPS65941) += tps65941_regulator.o
+obj-$(CONFIG_$(SPL_)DM_REGULATOR_ANATOP) += anatop_regulator.o
diff --git a/drivers/power/regulator/anatop_regulator.c b/drivers/power/regulator/anatop_regulator.c
new file mode 100644
index 0000000000..d824f4acc4
--- /dev/null
+++ b/drivers/power/regulator/anatop_regulator.c
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+// Copyright (C) 2021 Linaro
+
+#include <common.h>
+#include <errno.h>
+#include <dm.h>
+#include <log.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <power/pmic.h>
+#include <power/regulator.h>
+#include <regmap.h>
+#include <syscon.h>
+#include <linux/bitops.h>
+#include <linux/ioport.h>
+#include <dm/device-internal.h>
+#include <dm/device_compat.h>
+
+#define LDO_RAMP_UP_UNIT_IN_CYCLES      64 /* 64 cycles per step */
+#define LDO_RAMP_UP_FREQ_IN_MHZ         24 /* cycle based on 24M OSC */
+
+#define LDO_POWER_GATE			0x00
+#define LDO_FET_FULL_ON			0x1f
+
+#define BIT_WIDTH_MAX			32
+
+#define ANATOP_REGULATOR_STEP		25000
+
+struct anatop_regulator {
+	const char *name;
+	u32 control_reg;
+	u32 vol_bit_shift;
+	u32 vol_bit_width;
+	u32 min_bit_val;
+	u32 min_voltage;
+	u32 max_voltage;
+	u32 delay_reg;
+	u32 delay_bit_shift;
+	u32 delay_bit_width;
+};
+
+static u32 anatop_get_bits(struct udevice *dev, u32 addr, int bit_shift,
+			   int bit_width)
+{
+	struct udevice *syscon;
+	struct regmap *regmap;
+	int err;
+	u32 val, mask;
+
+	syscon = dev_get_parent(dev);
+	if (!syscon) {
+		dev_err(dev, "%s: unable to find syscon device\n", __func__);
+		return -ENODEV;
+	}
+
+	regmap = syscon_get_regmap(syscon);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "%s: unable to find regmap (%ld)\n", __func__,
+			PTR_ERR(regmap));
+		return PTR_ERR(regmap);
+	}
+
+	if (bit_width == BIT_WIDTH_MAX)
+		mask = ~0;
+	else
+		mask = (1 << bit_width) - 1;
+
+	err = regmap_read(regmap, addr, &val);
+	if (err)
+		return err;
+
+	val = (val >> bit_shift) & mask;
+
+	return val;
+}
+
+static int anatop_set_bits(struct udevice *dev, u32 addr, int bit_shift,
+			   int bit_width, u32 data)
+{
+	struct udevice *syscon;
+	struct regmap *regmap;
+	int err;
+	u32 val, mask;
+
+	syscon = dev_get_parent(dev);
+	if (!syscon) {
+		dev_err(dev, "%s: unable to find syscon device\n", __func__);
+		return -ENODEV;
+	}
+
+	regmap = syscon_get_regmap(syscon);
+	if (IS_ERR(regmap)) {
+		dev_err(dev,
+			"%s: unable to find regmap (%ld)\n", __func__,
+			PTR_ERR(regmap));
+		return PTR_ERR(regmap);
+	}
+
+	if (bit_width == 32)
+		mask = ~0;
+	else
+		mask = (1 << bit_width) - 1;
+
+	err = regmap_read(regmap, addr, &val);
+	if (err) {
+		dev_err(dev,
+			"%s: cannot read reg (%d)\n", __func__,
+			err);
+		return err;
+	}
+	val = val & ~(mask << bit_shift);
+	err = regmap_write(regmap, addr, (data << bit_shift) | val);
+	if (err) {
+		dev_err(dev,
+			"%s: cannot wrie reg (%d)\n", __func__,
+			err);
+		return err;
+	}
+
+	return 0;
+}
+
+static u32 anatop_get_selector(struct udevice *dev,
+			       const struct anatop_regulator *anatop_reg)
+{
+	u32 val = anatop_get_bits(dev,
+				  anatop_reg->control_reg,
+				  anatop_reg->vol_bit_shift,
+				  anatop_reg->vol_bit_width);
+
+	val = val - anatop_reg->min_bit_val;
+
+	return val;
+}
+
+static int anatop_set_voltage(struct udevice *dev, int uV)
+{
+	const struct anatop_regulator *anatop_reg = dev_get_plat(dev);
+	u32 val;
+	u32 sel;
+	int delayus = 0;
+	int ret = 0;
+	int uv = uV;
+
+	dev_dbg(dev, "%s: uv %d, min %d, max %d\n", __func__,
+		uv, anatop_reg->min_voltage,
+		anatop_reg->max_voltage);
+
+	if (uv < anatop_reg->min_voltage)
+		return -EINVAL;
+
+	if (!anatop_reg->control_reg)
+		return -ENOTSUPP;
+
+	sel = DIV_ROUND_UP(uv - anatop_reg->min_voltage,
+			   ANATOP_REGULATOR_STEP);
+	if (sel * ANATOP_REGULATOR_STEP + anatop_reg->min_voltage >
+	    anatop_reg->max_voltage)
+		return -EINVAL;
+	val = anatop_reg->min_bit_val + sel;
+	dev_dbg(dev, "%s: calculated val %d\n", __func__, val);
+
+	/* check whether need to care about LDO ramp up speed */
+	if (anatop_reg->delay_bit_width) {
+		/*
+		 * the delay for LDO ramp up time is
+		 * based on the register setting, we need
+		 * to calculate how many steps LDO need to
+		 * ramp up, and how much delay needed. (us)
+		 */
+		u32 old_sel;
+		u32 new_sel = sel;
+
+		old_sel = anatop_get_selector(dev, anatop_reg);
+
+		if (new_sel > old_sel) {
+			val = anatop_get_bits(dev,
+					      anatop_reg->delay_reg,
+					      anatop_reg->delay_bit_shift,
+					      anatop_reg->delay_bit_width);
+			delayus = (new_sel - old_sel) *
+				(LDO_RAMP_UP_UNIT_IN_CYCLES << val) /
+				LDO_RAMP_UP_FREQ_IN_MHZ + 1;
+		}
+	}
+
+	anatop_set_bits(dev,
+			anatop_reg->control_reg,
+			anatop_reg->vol_bit_shift,
+			anatop_reg->vol_bit_width,
+			val);
+
+	if (delayus)
+		udelay(delayus);
+
+	return ret;
+}
+
+static int anatop_get_voltage(struct udevice *dev)
+{
+	const struct anatop_regulator *anatop_reg = dev_get_plat(dev);
+	u32 sel;
+
+	sel = anatop_get_selector(dev, anatop_reg);
+
+	return sel * ANATOP_REGULATOR_STEP + anatop_reg->min_voltage;
+}
+
+static const struct dm_regulator_ops anatop_regulator_ops = {
+	.set_value = anatop_set_voltage,
+	.get_value = anatop_get_voltage,
+};
+
+static int anatop_regulator_probe(struct udevice *dev)
+{
+	struct anatop_regulator *sreg;
+	int ret = 0;
+
+	sreg = dev_get_plat(dev);
+	if (!sreg) {
+		dev_err(dev, "failed to get plat data\n");
+		return -ENOMEM;
+	}
+
+	sreg->name = ofnode_read_string(dev_ofnode(dev), "regulator-name");
+	if (!sreg->name) {
+		dev_err(dev, "failed to get a regulator-name\n");
+		return -EINVAL;
+	}
+
+	ret = ofnode_read_u32(dev_ofnode(dev),
+			      "anatop-reg-offset",
+			      &sreg->control_reg);
+	if (ret) {
+		dev_err(dev, "no anatop-reg-offset property set\n");
+		return ret;
+	}
+	ret = ofnode_read_u32(dev_ofnode(dev),
+			      "anatop-vol-bit-width",
+			      &sreg->vol_bit_width);
+	if (ret) {
+		dev_err(dev, "no anatop-vol-bit-width property set\n");
+		return ret;
+	}
+	ret = ofnode_read_u32(dev_ofnode(dev),
+			      "anatop-vol-bit-shift",
+			      &sreg->vol_bit_shift);
+	if (ret) {
+		dev_err(dev, "no anatop-vol-bit-shift property set\n");
+		return ret;
+	}
+	ret = ofnode_read_u32(dev_ofnode(dev),
+			      "anatop-min-bit-val",
+			      &sreg->min_bit_val);
+	if (ret) {
+		dev_err(dev, "no anatop-min-bit-val property set\n");
+		return ret;
+	}
+	ret = ofnode_read_u32(dev_ofnode(dev),
+			      "anatop-min-voltage",
+			      &sreg->min_voltage);
+	if (ret) {
+		dev_err(dev, "no anatop-min-voltage property set\n");
+		return ret;
+	}
+	ret = ofnode_read_u32(dev_ofnode(dev),
+			      "anatop-max-voltage",
+			      &sreg->max_voltage);
+	if (ret) {
+		dev_err(dev, "no anatop-max-voltage property set\n");
+		return ret;
+	}
+
+	/* read LDO ramp up setting, only for core reg */
+	ofnode_read_u32(dev_ofnode(dev), "anatop-delay-reg-offset",
+			&sreg->delay_reg);
+	ofnode_read_u32(dev_ofnode(dev), "anatop-delay-bit-width",
+			&sreg->delay_bit_width);
+	ofnode_read_u32(dev_ofnode(dev), "anatop-delay-bit-shift",
+			&sreg->delay_bit_shift);
+
+	return 0;
+}
+
+static const struct udevice_id of_anatop_regulator_match_tbl[] = {
+	{ .compatible = "fsl,anatop-regulator", },
+	{ /* end */ }
+};
+
+U_BOOT_DRIVER(anatop_regulator) = {
+	.name = "anatop_regulator",
+	.id = UCLASS_REGULATOR,
+	.ops = &anatop_regulator_ops,
+	.of_match = of_anatop_regulator_match_tbl,
+	.plat_auto = sizeof(struct anatop_regulator),
+	.probe = anatop_regulator_probe,
+};
-- 
2.30.2

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

* [PATCH v2 2/2] doc: device-tree-bindings: regulator: anatop regulator
  2021-03-23 13:48 [PATCH v2 0/2] power: regulator: add driver for ANATOP regulator Ying-Chun Liu
  2021-03-23 13:48 ` [PATCH v2 1/2] " Ying-Chun Liu
@ 2021-03-23 13:48 ` Ying-Chun Liu
  1 sibling, 0 replies; 6+ messages in thread
From: Ying-Chun Liu @ 2021-03-23 13:48 UTC (permalink / raw)
  To: u-boot

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Document the bindings for fsl,anatop-regulator

Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
---
 .../regulator/fsl,anatop-regulator.txt        | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 doc/device-tree-bindings/regulator/fsl,anatop-regulator.txt

diff --git a/doc/device-tree-bindings/regulator/fsl,anatop-regulator.txt b/doc/device-tree-bindings/regulator/fsl,anatop-regulator.txt
new file mode 100644
index 0000000000..2a60e4941b
--- /dev/null
+++ b/doc/device-tree-bindings/regulator/fsl,anatop-regulator.txt
@@ -0,0 +1,45 @@
+ANATOP REGULATOR
+
+Anatop is an integrated regulator inside i.MX6 SoC.
+
+Required properties:
+- compatible:		Must be "fsl,anatop-regulator".
+- regulator-name:	Name of the regulator
+- anatop-reg-offset:	u32 value representing the anatop MFD register offset.
+- anatop-vol-bit-shift:	u32 value representing the bit shift for the register.
+- anatop-vol-bit-width:	u32 value representing the number of bits used in the
+  			register.
+- anatop-min-bit-val:	u32 value representing the minimum value of this
+  			register.
+- anatop-min-voltage:	u32 value representing the minimum voltage of this
+  			regulator.
+- anatop-max-voltage:	u32 value representing the maximum voltage of this
+  			regulator.
+
+Optional properties:
+- anatop-delay-reg-offset: u32 value representing the anatop MFD step time
+  			   register offset.
+- anatop-delay-bit-shift: u32 value representing the bit shift for the step
+  			  time register.
+- anatop-delay-bit-width: u32 value representing the number of bits used in
+  			  the step time register.
+- anatop-enable-bit:	  u32 value representing regulator enable bit offset.
+- vin-supply:		  input supply phandle.
+
+Example:
+	regulator-vddpu {
+		compatible = "fsl,anatop-regulator";
+		regulator-name = "vddpu";
+		regulator-min-microvolt = <725000>;
+		regulator-max-microvolt = <1300000>;
+		regulator-always-on;
+		anatop-reg-offset = <0x140>;
+		anatop-vol-bit-shift = <9>;
+		anatop-vol-bit-width = <5>;
+		anatop-delay-reg-offset = <0x170>;
+		anatop-delay-bit-shift = <24>;
+		anatop-delay-bit-width = <2>;
+		anatop-min-bit-val = <1>;
+		anatop-min-voltage = <725000>;
+		anatop-max-voltage = <1300000>;
+	};
-- 
2.30.2

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

* [PATCH v2 1/2] power: regulator: add driver for ANATOP regulator
  2021-03-23 13:48 ` [PATCH v2 1/2] " Ying-Chun Liu
@ 2021-03-23 15:06   ` Sean Anderson
  2021-03-24  2:01     ` Ying-Chun Liu
  0 siblings, 1 reply; 6+ messages in thread
From: Sean Anderson @ 2021-03-23 15:06 UTC (permalink / raw)
  To: u-boot



On 3/23/21 9:48 AM, Ying-Chun Liu wrote:
 > From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
 >
 > Anatop is an integrated regulator inside i.MX6 SoC.
 > There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
 > And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
 > This patch adds the Anatop regulator driver.

+CC Peng Fan, Fabio Estevam

I don't see support for vin-supply. I have made comments below showing
the (relatively-minimal IMO) changes necessary to support it, along
with some additional comments.

 >
 > Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
 > ---
 > v2: add functions for set selector and delay. Define ANATOP_REGULATOR_STEP
 > ---
 >   drivers/power/regulator/Kconfig            |  10 +
 >   drivers/power/regulator/Makefile           |   1 +
 >   drivers/power/regulator/anatop_regulator.c | 299 +++++++++++++++++++++
 >   3 files changed, 310 insertions(+)
 >   create mode 100644 drivers/power/regulator/anatop_regulator.c
 >
 > diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
 > index fbbea18c7d..1cd1f3f5ed 100644
 > --- a/drivers/power/regulator/Kconfig
 > +++ b/drivers/power/regulator/Kconfig
 > @@ -312,6 +312,16 @@ config DM_REGULATOR_STPMIC1
 >   	by the PMIC device. This driver is controlled by a device tree node
 >   	which includes voltage limits.
 >
 > +config DM_REGULATOR_ANATOP
 > +	bool "Enable driver for ANATOP regulators"
 > +	depends on DM_REGULATOR
 > +	select REGMAP
 > +	select SYSCON
 > +	help
 > +	Enable support for the Freescale i.MX on-chip ANATOP LDOs
 > +	regulators. It is recommended that this option be enabled on
 > +	i.MX6 platform.
 > +
 >   config SPL_DM_REGULATOR_STPMIC1
 >   	bool "Enable driver for STPMIC1 regulators in SPL"
 >   	depends on SPL_DM_REGULATOR && PMIC_STPMIC1
 > diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
 > index 9d58112dcb..e7198da911 100644
 > --- a/drivers/power/regulator/Makefile
 > +++ b/drivers/power/regulator/Makefile
 > @@ -30,3 +30,4 @@ obj-$(CONFIG_DM_REGULATOR_TPS65910) += tps65910_regulator.o
 >   obj-$(CONFIG_DM_REGULATOR_TPS62360) += tps62360_regulator.o
 >   obj-$(CONFIG_$(SPL_)DM_REGULATOR_STPMIC1) += stpmic1.o
 >   obj-$(CONFIG_DM_REGULATOR_TPS65941) += tps65941_regulator.o
 > +obj-$(CONFIG_$(SPL_)DM_REGULATOR_ANATOP) += anatop_regulator.o
 > diff --git a/drivers/power/regulator/anatop_regulator.c b/drivers/power/regulator/anatop_regulator.c
 > new file mode 100644
 > index 0000000000..d824f4acc4
 > --- /dev/null
 > +++ b/drivers/power/regulator/anatop_regulator.c
 > @@ -0,0 +1,299 @@
 > +// SPDX-License-Identifier: GPL-2.0+
 > +//
 > +// Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
 > +// Copyright (C) 2021 Linaro

Use C-style comments (/* */) for everything other than the initial SPDX
line.

 > +
 > +#include <common.h>
 > +#include <errno.h>
 > +#include <dm.h>
 > +#include <log.h>
 > +#include <linux/delay.h>
 > +#include <linux/err.h>

These should be below with the other linux headers.

 > +#include <power/pmic.h>
 > +#include <power/regulator.h>
 > +#include <regmap.h>
 > +#include <syscon.h>
 > +#include <linux/bitops.h>
 > +#include <linux/ioport.h>
 > +#include <dm/device-internal.h>
 > +#include <dm/device_compat.h>

These should be above the linux headers.

 > +
 > +#define LDO_RAMP_UP_UNIT_IN_CYCLES      64 /* 64 cycles per step */
 > +#define LDO_RAMP_UP_FREQ_IN_MHZ         24 /* cycle based on 24M OSC */
 > +
 > +#define LDO_POWER_GATE			0x00
 > +#define LDO_FET_FULL_ON			0x1f
 > +
 > +#define BIT_WIDTH_MAX			32
 > +
 > +#define ANATOP_REGULATOR_STEP		25000
 > +
 > +struct anatop_regulator {
 > +	const char *name;

	struct udevice *supply;

 > +	u32 control_reg;
 > +	u32 vol_bit_shift;
 > +	u32 vol_bit_width;
 > +	u32 min_bit_val;
 > +	u32 min_voltage;
 > +	u32 max_voltage;
 > +	u32 delay_reg;
 > +	u32 delay_bit_shift;
 > +	u32 delay_bit_width;
 > +};
 > +
 > +static u32 anatop_get_bits(struct udevice *dev, u32 addr, int bit_shift,
 > +			   int bit_width)
 > +{
 > +	struct udevice *syscon;
 > +	struct regmap *regmap;
 > +	int err;
 > +	u32 val, mask;
 > +
 > +	syscon = dev_get_parent(dev);
 > +	if (!syscon) {
 > +		dev_err(dev, "%s: unable to find syscon device\n", __func__);

Use dev_dbg, per [1]. This applies to most other logs in this file as
well.

 > +		return -ENODEV;

Use ENOENT, per [1].

[1] https://lists.denx.de/pipermail/u-boot/2021-March/445207.html

 > +	}
 > +
 > +	regmap = syscon_get_regmap(syscon);
 > +	if (IS_ERR(regmap)) {
 > +		dev_err(dev, "%s: unable to find regmap (%ld)\n", __func__,
 > +			PTR_ERR(regmap));
 > +		return PTR_ERR(regmap);
 > +	}

This should be done in probe().

 > +
 > +	if (bit_width == BIT_WIDTH_MAX)
 > +		mask = ~0;
 > +	else
 > +		mask = (1 << bit_width) - 1;
 > +
 > +	err = regmap_read(regmap, addr, &val);
 > +	if (err)
 > +		return err;
 > +
 > +	val = (val >> bit_shift) & mask;
 > +
 > +	return val;
 > +}
 > +
 > +static int anatop_set_bits(struct udevice *dev, u32 addr, int bit_shift,
 > +			   int bit_width, u32 data)
 > +{
 > +	struct udevice *syscon;
 > +	struct regmap *regmap;
 > +	int err;
 > +	u32 val, mask;
 > +
 > +	syscon = dev_get_parent(dev);
 > +	if (!syscon) {
 > +		dev_err(dev, "%s: unable to find syscon device\n", __func__);
 > +		return -ENODEV;
 > +	}
 > +
 > +	regmap = syscon_get_regmap(syscon);
 > +	if (IS_ERR(regmap)) {
 > +		dev_err(dev,
 > +			"%s: unable to find regmap (%ld)\n", __func__,
 > +			PTR_ERR(regmap));
 > +		return PTR_ERR(regmap);
 > +	}

Again, should be done in probe.

 > +
 > +	if (bit_width == 32)
 > +		mask = ~0;
 > +	else
 > +		mask = (1 << bit_width) - 1;
 > +
 > +	err = regmap_read(regmap, addr, &val);
 > +	if (err) {
 > +		dev_err(dev,
 > +			"%s: cannot read reg (%d)\n", __func__,
 > +			err);
 > +		return err;
 > +	}
 > +	val = val & ~(mask << bit_shift);
 > +	err = regmap_write(regmap, addr, (data << bit_shift) | val);
 > +	if (err) {
 > +		dev_err(dev,
 > +			"%s: cannot wrie reg (%d)\n", __func__,
 > +			err);
 > +		return err;
 > +	}
 > +
 > +	return 0;
 > +}
 > +
 > +static u32 anatop_get_selector(struct udevice *dev,
 > +			       const struct anatop_regulator *anatop_reg)
 > +{
 > +	u32 val = anatop_get_bits(dev,
 > +				  anatop_reg->control_reg,
 > +				  anatop_reg->vol_bit_shift,
 > +				  anatop_reg->vol_bit_width);
 > +
 > +	val = val - anatop_reg->min_bit_val;
 > +
 > +	return val;
 > +}
 > +
 > +static int anatop_set_voltage(struct udevice *dev, int uV)
 > +{
 > +	const struct anatop_regulator *anatop_reg = dev_get_plat(dev);
 > +	u32 val;
 > +	u32 sel;
 > +	int delayus = 0;
 > +	int ret = 0;

This does not need to be set to 0.

 > +	int uv = uV;
 > +
 > +	dev_dbg(dev, "%s: uv %d, min %d, max %d\n", __func__,
 > +		uv, anatop_reg->min_voltage,
 > +		anatop_reg->max_voltage);
 > +
 > +	if (uv < anatop_reg->min_voltage)
 > +		return -EINVAL;
 > +
 > +	if (!anatop_reg->control_reg)
 > +		return -ENOTSUPP;
 > +
 > +	sel = DIV_ROUND_UP(uv - anatop_reg->min_voltage,
 > +			   ANATOP_REGULATOR_STEP);
 > +	if (sel * ANATOP_REGULATOR_STEP + anatop_reg->min_voltage >
 > +	    anatop_reg->max_voltage)
 > +		return -EINVAL;
 > +	val = anatop_reg->min_bit_val + sel;
 > +	dev_dbg(dev, "%s: calculated val %d\n", __func__, val);

	if (anatop_reg->supply) {
		ret = regulator_set_value(anatop_reg->supply, uV + 150000);
		if (ret)
			return ret;
	}

 > +
 > +	/* check whether need to care about LDO ramp up speed */

Can this be handled by setting dm_regulator_uclass_platdata.ramp_delay?

 > +	if (anatop_reg->delay_bit_width) {
 > +		/*
 > +		 * the delay for LDO ramp up time is
 > +		 * based on the register setting, we need
 > +		 * to calculate how many steps LDO need to
 > +		 * ramp up, and how much delay needed. (us)
 > +		 */
 > +		u32 old_sel;
 > +		u32 new_sel = sel;
 > +
 > +		old_sel = anatop_get_selector(dev, anatop_reg);
 > +
 > +		if (new_sel > old_sel) {
 > +			val = anatop_get_bits(dev,
 > +					      anatop_reg->delay_reg,
 > +					      anatop_reg->delay_bit_shift,
 > +					      anatop_reg->delay_bit_width);
 > +			delayus = (new_sel - old_sel) *
 > +				(LDO_RAMP_UP_UNIT_IN_CYCLES << val) /
 > +				LDO_RAMP_UP_FREQ_IN_MHZ + 1;
 > +		}
 > +	}
 > +
 > +	anatop_set_bits(dev,
 > +			anatop_reg->control_reg,
 > +			anatop_reg->vol_bit_shift,
 > +			anatop_reg->vol_bit_width,
 > +			val);
 > +
 > +	if (delayus)
 > +		udelay(delayus);
 > +
 > +	return ret;
 > +}
 > +
 > +static int anatop_get_voltage(struct udevice *dev)
 > +{
 > +	const struct anatop_regulator *anatop_reg = dev_get_plat(dev);
 > +	u32 sel;
 > +
 > +	sel = anatop_get_selector(dev, anatop_reg);
 > +
 > +	return sel * ANATOP_REGULATOR_STEP + anatop_reg->min_voltage;
 > +}
 > +
 > +static const struct dm_regulator_ops anatop_regulator_ops = {
 > +	.set_value = anatop_set_voltage,
 > +	.get_value = anatop_get_voltage,
 > +};
 > +
 > +static int anatop_regulator_probe(struct udevice *dev)
 > +{
 > +	struct anatop_regulator *sreg;

Please use consistent names for this structure.

 > +	int ret = 0;
 > +
 > +	sreg = dev_get_plat(dev);
 > +	if (!sreg) {
 > +		dev_err(dev, "failed to get plat data\n");

The driver model checks for this. Do not check it again.

 > +		return -ENOMEM;
 > +	}
 > +
 > +	sreg->name = ofnode_read_string(dev_ofnode(dev), "regulator-name");
 > +	if (!sreg->name) {
 > +		dev_err(dev, "failed to get a regulator-name\n");
 > +		return -EINVAL;
 > +	}

	ret = device_get_supply_regulator(dev, "vin-supply", &sreg->supply)
	if (ret)
		return ret;
	
	ret = regulator_set_enable(sreg->supply, true);
	if (ret)
		return ret;

 > +
 > +	ret = ofnode_read_u32(dev_ofnode(dev),

use dev_read_u32.

 > +			      "anatop-reg-offset",
 > +			      &sreg->control_reg);
 > +	if (ret) {
 > +		dev_err(dev, "no anatop-reg-offset property set\n");
 > +		return ret;
 > +	}

Perhaps consider something like

	ret = dev_read_u32(dev, "some-prop", &sreg->some_prop);
	if (ret)
		return log_msg_ret("some-prop", ret);

 > +	ret = ofnode_read_u32(dev_ofnode(dev),
 > +			      "anatop-vol-bit-width",
 > +			      &sreg->vol_bit_width);
 > +	if (ret) {
 > +		dev_err(dev, "no anatop-vol-bit-width property set\n");
 > +		return ret;
 > +	}
 > +	ret = ofnode_read_u32(dev_ofnode(dev),
 > +			      "anatop-vol-bit-shift",
 > +			      &sreg->vol_bit_shift);
 > +	if (ret) {
 > +		dev_err(dev, "no anatop-vol-bit-shift property set\n");
 > +		return ret;
 > +	}
 > +	ret = ofnode_read_u32(dev_ofnode(dev),
 > +			      "anatop-min-bit-val",
 > +			      &sreg->min_bit_val);
 > +	if (ret) {
 > +		dev_err(dev, "no anatop-min-bit-val property set\n");
 > +		return ret;
 > +	}
 > +	ret = ofnode_read_u32(dev_ofnode(dev),
 > +			      "anatop-min-voltage",
 > +			      &sreg->min_voltage);
 > +	if (ret) {
 > +		dev_err(dev, "no anatop-min-voltage property set\n");
 > +		return ret;
 > +	}
 > +	ret = ofnode_read_u32(dev_ofnode(dev),
 > +			      "anatop-max-voltage",
 > +			      &sreg->max_voltage);
 > +	if (ret) {
 > +		dev_err(dev, "no anatop-max-voltage property set\n");
 > +		return ret;
 > +	}
 > +
 > +	/* read LDO ramp up setting, only for core reg */
 > +	ofnode_read_u32(dev_ofnode(dev), "anatop-delay-reg-offset",
 > +			&sreg->delay_reg);
 > +	ofnode_read_u32(dev_ofnode(dev), "anatop-delay-bit-width",
 > +			&sreg->delay_bit_width);
 > +	ofnode_read_u32(dev_ofnode(dev), "anatop-delay-bit-shift",
 > +			&sreg->delay_bit_shift);
 > +
 > +	return 0;
 > +}
 > +
 > +static const struct udevice_id of_anatop_regulator_match_tbl[] = {
 > +	{ .compatible = "fsl,anatop-regulator", },
 > +	{ /* end */ }
 > +};
 > +
 > +U_BOOT_DRIVER(anatop_regulator) = {
 > +	.name = "anatop_regulator",
 > +	.id = UCLASS_REGULATOR,
 > +	.ops = &anatop_regulator_ops,
 > +	.of_match = of_anatop_regulator_match_tbl,
 > +	.plat_auto = sizeof(struct anatop_regulator),
 > +	.probe = anatop_regulator_probe,
 > +};
 >

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

* [PATCH v2 1/2] power: regulator: add driver for ANATOP regulator
  2021-03-23 15:06   ` Sean Anderson
@ 2021-03-24  2:01     ` Ying-Chun Liu
  2021-03-25 14:20       ` Sean Anderson
  0 siblings, 1 reply; 6+ messages in thread
From: Ying-Chun Liu @ 2021-03-24  2:01 UTC (permalink / raw)
  To: u-boot

Hi Sean,

Thanks for the review. I fix almost of the issues. Will upload the v3 soon.

Still have some questions.


Sean Anderson ? 2021/3/23 ??11:06 ??:
>
>
>
> ????if (anatop_reg->supply) {
> ??????? ret = regulator_set_value(anatop_reg->supply, uV + 150000);
> ??????? if (ret)
> ??????????? return ret;
> ????}
>

What is 150000? Is it the min_dropout_uV?

Should I set it to 125000 instead?



>
>
> ????
> ????ret = regulator_set_enable(sreg->supply, true);
> ????if (ret)
> ??????? return ret;
>

Since vin-supply is optional, I change it to

??????? ret = device_get_supply_regulator(dev, "vin-supply",
????????????????????????????????????????? &anatop_reg->supply);
??????? if (!ret) {
??????????????? ret = regulator_set_enable(anatop_reg->supply, true);
??????????????? if (ret)
??????????????????????? return ret;
??????? }

Is this ok?

> > +
> > +??? ret = ofnode_read_u32(dev_ofnode(dev),
>
> > +U_BOOT_DRIVER(anatop_regulator) = {
> > +??? .name = "anatop_regulator",
> > +??? .id = UCLASS_REGULATOR,
> > +??? .ops = &anatop_regulator_ops,
> > +??? .of_match = of_anatop_regulator_match_tbl,
> > +??? .plat_auto = sizeof(struct anatop_regulator),
> > +??? .probe = anatop_regulator_probe,
> > +};
> >


Yours,
Paul

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

* [PATCH v2 1/2] power: regulator: add driver for ANATOP regulator
  2021-03-24  2:01     ` Ying-Chun Liu
@ 2021-03-25 14:20       ` Sean Anderson
  0 siblings, 0 replies; 6+ messages in thread
From: Sean Anderson @ 2021-03-25 14:20 UTC (permalink / raw)
  To: u-boot



On 3/23/21 10:01 PM, Ying-Chun Liu (PaulLiu) wrote:
> Hi Sean,
> 
> Thanks for the review. I fix almost of the issues. Will upload the v3 soon.
> 
> Still have some questions.
> 
> 
> Sean Anderson ? 2021/3/23 ??11:06 ??:
>>
>>
>>
>>  ????if (anatop_reg->supply) {
>>  ??????? ret = regulator_set_value(anatop_reg->supply, uV + 150000);
>>  ??????? if (ret)
>>  ??????????? return ret;
>>  ????}
>>
> 
> What is 150000? Is it the min_dropout_uV?

Yes.

> 
> Should I set it to 125000 instead?

Yes.

> 
> 
> 
>>
>>
>>      
>>  ????ret = regulator_set_enable(sreg->supply, true);
>>  ????if (ret)
>>  ??????? return ret;
>>
> 
> Since vin-supply is optional, I change it to
> 
>  ??????? ret = device_get_supply_regulator(dev, "vin-supply",
>  ????????????????????????????????????????? &anatop_reg->supply);
>  ??????? if (!ret) {
>  ??????????????? ret = regulator_set_enable(anatop_reg->supply, true);
>  ??????????????? if (ret)
>  ??????????????????????? return ret;
>  ??????? }
> 
> Is this ok?

Yes.

--Sean

> 
>>> +
>>> +??? ret = ofnode_read_u32(dev_ofnode(dev),
>>
>>> +U_BOOT_DRIVER(anatop_regulator) = {
>>> +??? .name = "anatop_regulator",
>>> +??? .id = UCLASS_REGULATOR,
>>> +??? .ops = &anatop_regulator_ops,
>>> +??? .of_match = of_anatop_regulator_match_tbl,
>>> +??? .plat_auto = sizeof(struct anatop_regulator),
>>> +??? .probe = anatop_regulator_probe,
>>> +};
>>>
> 
> 
> Yours,
> Paul
> 

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

end of thread, other threads:[~2021-03-25 14:20 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 13:48 [PATCH v2 0/2] power: regulator: add driver for ANATOP regulator Ying-Chun Liu
2021-03-23 13:48 ` [PATCH v2 1/2] " Ying-Chun Liu
2021-03-23 15:06   ` Sean Anderson
2021-03-24  2:01     ` Ying-Chun Liu
2021-03-25 14:20       ` Sean Anderson
2021-03-23 13:48 ` [PATCH v2 2/2] doc: device-tree-bindings: regulator: anatop regulator Ying-Chun Liu

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.