[U-Boot] [PATCH 0/7] add pwm regulator driver

[U-Boot] [PATCH 0/7] add pwm regulator driver

[Kever Yang]

This patch set add pwm regulator driver and enable it
on rk3399, also do some update and fix to make the regulator
driver work properly.



Kever Yang (7):
  rockchip: rk3399: update PPLL and pmu_pclk frequency
  rockchip: rkpwm: fix the register sequence
  power: regulator: add pwm regulator
  rockchip: evb_rk3399: init vdd_center regulator
  Kconfig: rockchip: enable DM_PWM and DM_REGULATOR
  dts: evb-rk3399: add init voltage node for vdd-center
  config: evb-rk3399: enable pwm regulator

 arch/arm/Kconfig                                |   2 +
 arch/arm/dts/rk3399-evb.dts                     |   1 +
 arch/arm/include/asm/arch-rockchip/cru_rk3399.h |   4 +-
 arch/arm/include/asm/arch-rockchip/pwm.h        |   2 +-
 board/rockchip/evb_rk3399/evb-rk3399.c          |   6 +
 configs/evb-rk3399_defconfig                    |   1 +
 drivers/power/regulator/Kconfig                 |   9 ++
 drivers/power/regulator/Makefile                |   1 +
 drivers/power/regulator/pwm_regulator.c         | 157 ++++++++++++++++++++++++
 9 files changed, 180 insertions(+), 3 deletions(-)
 create mode 100644 drivers/power/regulator/pwm_regulator.c

-- 
1.9.1

[U-Boot] [PATCH 1/7] rockchip: rk3399: update PPLL and pmu_pclk frequency

[Kever Yang]

This patch update PPLL to 676MHz and PMU_PCLK to 48MHz.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 arch/arm/include/asm/arch-rockchip/cru_rk3399.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3399.h b/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
index c919f47..6776e48 100644
--- a/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
+++ b/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
@@ -64,9 +64,9 @@ check_member(rk3399_cru, sdio1_con[1], 0x594);
 #define APLL_HZ		(600*MHz)
 #define GPLL_HZ		(594*MHz)
 #define CPLL_HZ		(384*MHz)
-#define PPLL_HZ		(594*MHz)
+#define PPLL_HZ		(676*MHz)
 
-#define PMU_PCLK_HZ	(99*MHz)
+#define PMU_PCLK_HZ	(48*MHz)
 
 #define ACLKM_CORE_HZ	(300*MHz)
 #define ATCLK_CORE_HZ	(300*MHz)
-- 
1.9.1

[U-Boot] [PATCH 2/7] rockchip: rkpwm: fix the register sequence

[Kever Yang]

Reference to kernel source code, rockchip pwm has three
type, we are using v2 for rk3288 and rk3399, so let's
update the register to sync with pwm_data_v2 in kernel.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 arch/arm/include/asm/arch-rockchip/pwm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/arch-rockchip/pwm.h b/arch/arm/include/asm/arch-rockchip/pwm.h
index 08ff945..5d9a178 100644
--- a/arch/arm/include/asm/arch-rockchip/pwm.h
+++ b/arch/arm/include/asm/arch-rockchip/pwm.h
@@ -10,8 +10,8 @@
 
 struct rk3288_pwm {
 	u32 cnt;
-	u32 period_hpr;
 	u32 duty_lpr;
+	u32 period_hpr;
 	u32 ctrl;
 };
 check_member(rk3288_pwm, ctrl, 0xc);
-- 
1.9.1

[U-Boot] [PATCH 3/7] power: regulator: add pwm regulator

[Kever Yang]

This driver add support for pwm regulator.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 drivers/power/regulator/Kconfig         |   9 ++
 drivers/power/regulator/Makefile        |   1 +
 drivers/power/regulator/pwm_regulator.c | 157 ++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/power/regulator/pwm_regulator.c

diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
index 17f22dd..c2eaa84 100644
--- a/drivers/power/regulator/Kconfig
+++ b/drivers/power/regulator/Kconfig
@@ -42,6 +42,15 @@ config DM_REGULATOR_PFUZE100
 	features for REGULATOR PFUZE100. The driver implements get/set api for:
 	value, enable and mode.
 
+config REGULATOR_PWM
+	bool "Enable driver for PWM regulators"
+	depends on DM_REGULATOR
+	---help---
+	Enable support for the regulator functions of the PWM. The
+	driver implements get/set api for the various BUCKS.
+	This driver is controlled by a device tree node
+	which includes voltage limits.
+
 config DM_REGULATOR_MAX77686
 	bool "Enable Driver Model for REGULATOR MAX77686"
 	depends on DM_REGULATOR && DM_PMIC_MAX77686
diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
index 1590d85..ab461ec 100644
--- a/drivers/power/regulator/Makefile
+++ b/drivers/power/regulator/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_$(SPL_)DM_REGULATOR) += regulator-uclass.o
 obj-$(CONFIG_REGULATOR_ACT8846) += act8846.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_FIXED) += fixed.o
 obj-$(CONFIG_REGULATOR_RK808) += rk808.o
 obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
diff --git a/drivers/power/regulator/pwm_regulator.c b/drivers/power/regulator/pwm_regulator.c
new file mode 100644
index 0000000..f75731b
--- /dev/null
+++ b/drivers/power/regulator/pwm_regulator.c
@@ -0,0 +1,157 @@
+/*
+ * Copyright (C) 2016 Rockchip Electronics Co., Ltd
+ *
+ * Based on kernel drivers/regulator/pwm-regulator.c
+ * Copyright (C) 2014 - STMicroelectronics Inc.
+ * Author: Lee Jones <lee.jones@linaro.org>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <pwm.h>
+#include <power/regulator.h>
+#include <libfdt.h>
+#include <fdt_support.h>
+#include <fdtdec.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct pwm_regulator_info {
+	int pwm_id;
+	int period;
+	struct udevice *pwm;
+	unsigned int init_voltage;
+	unsigned int max_voltage;
+	unsigned int min_voltage;
+	unsigned int boot_on;
+	unsigned int volt_uV;
+};
+
+static int pwm_regulator_enable(struct udevice *dev, bool enable)
+{
+	struct pwm_regulator_info *priv = dev_get_priv(dev);
+
+	return pwm_set_enable(priv->pwm, priv->pwm_id, enable);
+}
+
+static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int req_uV)
+{
+	struct pwm_regulator_info *priv = dev_get_priv(dev);
+	int min_uV = priv->min_voltage;
+	int max_uV = priv->max_voltage;
+	int diff = max_uV - min_uV;
+
+	return 100 - (((req_uV * 100) - (min_uV * 100)) / diff);
+}
+
+static int pwm_regulator_get_voltage(struct udevice *dev)
+{
+	struct pwm_regulator_info *priv = dev_get_priv(dev);
+
+	return priv->volt_uV;
+}
+
+static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt)
+{
+	struct pwm_regulator_info *priv = dev_get_priv(dev);
+	int duty_cycle;
+	int ret = 0;
+
+	duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
+
+	ret = pwm_set_config(priv->pwm, priv->pwm_id,
+			(priv->period / 100) * duty_cycle, priv->period);
+	if (ret) {
+		dev_err(dev, "Failed to configure PWM\n");
+		return ret;
+	}
+
+	ret = pwm_set_enable(priv->pwm, priv->pwm_id, true);
+	if (ret) {
+		dev_err(dev, "Failed to enable PWM\n");
+		return ret;
+	}
+	priv->volt_uV = uvolt;
+	return ret;
+}
+
+static int pwm_regulator_ofdata_to_platdata(struct udevice *dev)
+{
+	struct pwm_regulator_info *priv = dev_get_priv(dev);
+	struct fdtdec_phandle_args args;
+	const void *blob = gd->fdt_blob;
+	int node = dev->of_offset;
+	int ret;
+
+	ret = fdtdec_parse_phandle_with_args(blob, node, "pwms", "#pwm-cells",
+					     0, 0, &args);
+	if (ret) {
+		debug("%s: Cannot get PWM phandle: ret=%d\n", __func__, ret);
+		return ret;
+	}
+
+	priv->period = args.args[1];
+
+	/* rkpwm do not use the pwm_id, set it to 0 */
+	priv->pwm_id = 0;
+
+	priv->init_voltage = fdtdec_get_int(blob, node,
+			"regulator-init-microvolt", 0);
+	if (priv->init_voltage < 0)
+		printf("Cannot find regulator pwm init_voltage\n");
+
+	ret = uclass_get_device_by_of_offset(UCLASS_PWM, args.node, &priv->pwm);
+	if (ret) {
+		debug("%s: Cannot get PWM: ret=%d\n", __func__, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pwm_regulator_probe(struct udevice *dev)
+{
+	struct pwm_regulator_info *priv = dev_get_priv(dev);
+	struct dm_regulator_uclass_platdata *uc_pdata;
+
+	uc_pdata = dev_get_uclass_platdata(dev);
+
+	uc_pdata->type = REGULATOR_TYPE_BUCK;
+	uc_pdata->mode_count = 0;
+	priv->max_voltage = uc_pdata->max_uV;
+	priv->min_voltage = uc_pdata->min_uV;
+
+	if (priv->init_voltage)
+		pwm_regulator_set_voltage(dev, priv->init_voltage);
+
+	if (priv->boot_on)
+		pwm_regulator_enable(dev, 1);
+
+	return 0;
+}
+
+static const struct dm_regulator_ops pwm_regulator_ops = {
+	.get_value  = pwm_regulator_get_voltage,
+	.set_value  = pwm_regulator_set_voltage,
+	.set_enable = pwm_regulator_enable,
+};
+
+static const struct udevice_id pwm_regulator_ids[] = {
+	{ .compatible = "pwm-regulator" },
+	{ .compatible = "rockchip_pwm_regulator" },
+	{ }
+};
+
+U_BOOT_DRIVER(pwm_regulator) = {
+	.name = "pwm_regulator",
+	.id = UCLASS_REGULATOR,
+	.ops = &pwm_regulator_ops,
+	.probe = pwm_regulator_probe,
+	.of_match = pwm_regulator_ids,
+	.ofdata_to_platdata	= pwm_regulator_ofdata_to_platdata,
+	.priv_auto_alloc_size	= sizeof(struct pwm_regulator_info),
+};
+
-- 
1.9.1

[U-Boot] [PATCH 4/7] rockchip: evb_rk3399: init vdd_center regulator

[Kever Yang]

This patch add vdd_center pwm regulator get_device to
enable this regulator.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 board/rockchip/evb_rk3399/evb-rk3399.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c
index 5b245e4..863e746 100644
--- a/board/rockchip/evb_rk3399/evb-rk3399.c
+++ b/board/rockchip/evb_rk3399/evb-rk3399.c
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <dm.h>
 #include <dm/pinctrl.h>
+#include <dm/uclass-internal.h>
 #include <asm/arch/periph.h>
 #include <usb.h>
 #include <dwc3-uboot.h>
@@ -41,6 +42,11 @@ int board_init(void)
 		goto out;
 	}
 
+	/* rk3399 need init vdd_center to get correct output voltage */
+	ret = regulator_get_by_platname("vdd_center", &regulator);
+	if (ret)
+		debug("%s: Cannot get vdd_center regulator\n", __func__);
+
 	ret = regulator_get_by_platname("vcc5v0_host", &regulator);
 	if (ret) {
 		debug("%s vcc5v0_host init fail! ret %d\n", __func__, ret);
-- 
1.9.1

[U-Boot] [PATCH 5/7] Kconfig: rockchip: enable DM_PWM and DM_REGULATOR

[Kever Yang]

This patch enable DM_PWM and DM_REGULATOR on rockchip SoCs.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 arch/arm/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 4928206..c877f5d 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -863,6 +863,8 @@ config ARCH_ROCKCHIP
 	select DM_SPI
 	select DM_SPI_FLASH
 	select DM_USB if USB
+	select DM_PWM
+	select DM_REGULATOR
 
 config TARGET_THUNDERX_88XX
 	bool "Support ThunderX 88xx"
-- 
1.9.1

[U-Boot] [PATCH 6/7] dts: evb-rk3399: add init voltage node for vdd-center

[Kever Yang]

This patch add regulator-init-microvolt for pwm regulator
to get a init value when driver do probe init.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 arch/arm/dts/rk3399-evb.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/rk3399-evb.dts b/arch/arm/dts/rk3399-evb.dts
index bd7801b..fa60e19 100644
--- a/arch/arm/dts/rk3399-evb.dts
+++ b/arch/arm/dts/rk3399-evb.dts
@@ -23,6 +23,7 @@
 		regulator-name = "vdd_center";
 		regulator-min-microvolt = <800000>;
 		regulator-max-microvolt = <1400000>;
+		regulator-init-microvolt = <950000>;
 		regulator-always-on;
 		regulator-boot-on;
 		status = "okay";
-- 
1.9.1

[U-Boot] [PATCH 7/7] config: evb-rk3399: enable pwm regulator

[Kever Yang]

This patch enable the pwm regulator for evb-rk3399.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

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

diff --git a/configs/evb-rk3399_defconfig b/configs/evb-rk3399_defconfig
index 9a6d422..62081a4 100644
--- a/configs/evb-rk3399_defconfig
+++ b/configs/evb-rk3399_defconfig
@@ -29,6 +29,7 @@ CONFIG_PINCTRL=y
 CONFIG_ROCKCHIP_RK3399_PINCTRL=y
 CONFIG_DM_PWM=y
 CONFIG_PWM_ROCKCHIP=y
+CONFIG_REGULATOR_PWM=y
 CONFIG_RAM=y
 CONFIG_SYS_NS16550=y
 CONFIG_DEBUG_UART=y
-- 
1.9.1

[U-Boot] [PATCH 1/7] rockchip: rk3399: update PPLL and pmu_pclk frequency

[Simon Glass]

Hi Kever,

On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
> This patch update PPLL to 676MHz and PMU_PCLK to 48MHz.

Why?

>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
>  arch/arm/include/asm/arch-rockchip/cru_rk3399.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3399.h b/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
> index c919f47..6776e48 100644
> --- a/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
> +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
> @@ -64,9 +64,9 @@ check_member(rk3399_cru, sdio1_con[1], 0x594);
>  #define APLL_HZ                (600*MHz)
>  #define GPLL_HZ                (594*MHz)
>  #define CPLL_HZ                (384*MHz)
> -#define PPLL_HZ                (594*MHz)
> +#define PPLL_HZ                (676*MHz)
>
> -#define PMU_PCLK_HZ    (99*MHz)
> +#define PMU_PCLK_HZ    (48*MHz)
>
>  #define ACLKM_CORE_HZ  (300*MHz)
>  #define ATCLK_CORE_HZ  (300*MHz)
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH 2/7] rockchip: rkpwm: fix the register sequence

[Simon Glass]

On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
> Reference to kernel source code, rockchip pwm has three
> type, we are using v2 for rk3288 and rk3399, so let's
> update the register to sync with pwm_data_v2 in kernel.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
>  arch/arm/include/asm/arch-rockchip/pwm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[U-Boot] [PATCH 3/7] power: regulator: add pwm regulator

[Simon Glass]

Hi Kever,

On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
> This driver add support for pwm regulator.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
>  drivers/power/regulator/Kconfig         |   9 ++
>  drivers/power/regulator/Makefile        |   1 +
>  drivers/power/regulator/pwm_regulator.c | 157 ++++++++++++++++++++++++++++++++
>  3 files changed, 167 insertions(+)
>  create mode 100644 drivers/power/regulator/pwm_regulator.c
>
> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
> index 17f22dd..c2eaa84 100644
> --- a/drivers/power/regulator/Kconfig
> +++ b/drivers/power/regulator/Kconfig
> @@ -42,6 +42,15 @@ config DM_REGULATOR_PFUZE100
>         features for REGULATOR PFUZE100. The driver implements get/set api for:
>         value, enable and mode.
>
> +config REGULATOR_PWM
> +       bool "Enable driver for PWM regulators"
> +       depends on DM_REGULATOR
> +       ---help---
> +       Enable support for the regulator functions of the PWM. The

Does a PWM have regulator functions? Do you mean a board that uses a
PWM to control a regulator?

> +       driver implements get/set api for the various BUCKS.
> +       This driver is controlled by a device tree node
> +       which includes voltage limits.
> +
>  config DM_REGULATOR_MAX77686
>         bool "Enable Driver Model for REGULATOR MAX77686"
>         depends on DM_REGULATOR && DM_PMIC_MAX77686
> diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
> index 1590d85..ab461ec 100644
> --- a/drivers/power/regulator/Makefile
> +++ b/drivers/power/regulator/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_$(SPL_)DM_REGULATOR) += regulator-uclass.o
>  obj-$(CONFIG_REGULATOR_ACT8846) += act8846.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_FIXED) += fixed.o
>  obj-$(CONFIG_REGULATOR_RK808) += rk808.o
>  obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
> diff --git a/drivers/power/regulator/pwm_regulator.c b/drivers/power/regulator/pwm_regulator.c
> new file mode 100644
> index 0000000..f75731b
> --- /dev/null
> +++ b/drivers/power/regulator/pwm_regulator.c
> @@ -0,0 +1,157 @@
> +/*
> + * Copyright (C) 2016 Rockchip Electronics Co., Ltd
> + *
> + * Based on kernel drivers/regulator/pwm-regulator.c
> + * Copyright (C) 2014 - STMicroelectronics Inc.
> + * Author: Lee Jones <lee.jones@linaro.org>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <pwm.h>
> +#include <power/regulator.h>
> +#include <libfdt.h>
> +#include <fdt_support.h>
> +#include <fdtdec.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct pwm_regulator_info {
> +       int pwm_id;

Please add a comment for members

> +       int period;

period_ms is better, if this is milliseconds. Or period_us?

> +       struct udevice *pwm;
> +       unsigned int init_voltage;
> +       unsigned int max_voltage;
> +       unsigned int min_voltage;
> +       unsigned int boot_on;

bool?

> +       unsigned int volt_uV;
> +};
> +
> +static int pwm_regulator_enable(struct udevice *dev, bool enable)
> +{
> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
> +
> +       return pwm_set_enable(priv->pwm, priv->pwm_id, enable);
> +}
> +
> +static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int req_uV)
> +{
> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
> +       int min_uV = priv->min_voltage;
> +       int max_uV = priv->max_voltage;
> +       int diff = max_uV - min_uV;
> +
> +       return 100 - (((req_uV * 100) - (min_uV * 100)) / diff);
> +}
> +
> +static int pwm_regulator_get_voltage(struct udevice *dev)
> +{
> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
> +
> +       return priv->volt_uV;
> +}
> +
> +static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt)
> +{
> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
> +       int duty_cycle;
> +       int ret = 0;
> +
> +       duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
> +
> +       ret = pwm_set_config(priv->pwm, priv->pwm_id,
> +                       (priv->period / 100) * duty_cycle, priv->period);
> +       if (ret) {
> +               dev_err(dev, "Failed to configure PWM\n");
> +               return ret;
> +       }
> +
> +       ret = pwm_set_enable(priv->pwm, priv->pwm_id, true);
> +       if (ret) {
> +               dev_err(dev, "Failed to enable PWM\n");
> +               return ret;
> +       }
> +       priv->volt_uV = uvolt;
> +       return ret;
> +}
> +
> +static int pwm_regulator_ofdata_to_platdata(struct udevice *dev)
> +{
> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
> +       struct fdtdec_phandle_args args;
> +       const void *blob = gd->fdt_blob;
> +       int node = dev->of_offset;
> +       int ret;
> +
> +       ret = fdtdec_parse_phandle_with_args(blob, node, "pwms", "#pwm-cells",
> +                                            0, 0, &args);
> +       if (ret) {
> +               debug("%s: Cannot get PWM phandle: ret=%d\n", __func__, ret);
> +               return ret;
> +       }
> +
> +       priv->period = args.args[1];
> +
> +       /* rkpwm do not use the pwm_id, set it to 0 */
> +       priv->pwm_id = 0;

But this is not a rockchip driver. Also private data starts at 0 so
you can skip this.

> +
> +       priv->init_voltage = fdtdec_get_int(blob, node,
> +                       "regulator-init-microvolt", 0);
> +       if (priv->init_voltage < 0)
> +               printf("Cannot find regulator pwm init_voltage\n");

debug()

If that is an error, please return -EINVAL. The error seems wrong
also. If the property is missing then fdtdec_get_int() will return
your default value (0). So perhaps the error should be 'invalid
voltage < 0'. But actually I cannot see why such a check is useful.
People should not make such mistakes in device-tree data.

> +
> +       ret = uclass_get_device_by_of_offset(UCLASS_PWM, args.node, &priv->pwm);
> +       if (ret) {
> +               debug("%s: Cannot get PWM: ret=%d\n", __func__, ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int pwm_regulator_probe(struct udevice *dev)
> +{
> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
> +       struct dm_regulator_uclass_platdata *uc_pdata;
> +
> +       uc_pdata = dev_get_uclass_platdata(dev);
> +
> +       uc_pdata->type = REGULATOR_TYPE_BUCK;
> +       uc_pdata->mode_count = 0;
> +       priv->max_voltage = uc_pdata->max_uV;
> +       priv->min_voltage = uc_pdata->min_uV;
> +
> +       if (priv->init_voltage)
> +               pwm_regulator_set_voltage(dev, priv->init_voltage);
> +
> +       if (priv->boot_on)
> +               pwm_regulator_enable(dev, 1);
> +
> +       return 0;
> +}
> +
> +static const struct dm_regulator_ops pwm_regulator_ops = {
> +       .get_value  = pwm_regulator_get_voltage,
> +       .set_value  = pwm_regulator_set_voltage,
> +       .set_enable = pwm_regulator_enable,
> +};
> +
> +static const struct udevice_id pwm_regulator_ids[] = {
> +       { .compatible = "pwm-regulator" },
> +       { .compatible = "rockchip_pwm_regulator" },

Should that be rockchip,pwm-regulator ?

> +       { }
> +};
> +
> +U_BOOT_DRIVER(pwm_regulator) = {
> +       .name = "pwm_regulator",
> +       .id = UCLASS_REGULATOR,
> +       .ops = &pwm_regulator_ops,
> +       .probe = pwm_regulator_probe,
> +       .of_match = pwm_regulator_ids,
> +       .ofdata_to_platdata     = pwm_regulator_ofdata_to_platdata,
> +       .priv_auto_alloc_size   = sizeof(struct pwm_regulator_info),
> +};
> +
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH 4/7] rockchip: evb_rk3399: init vdd_center regulator

[Simon Glass]

On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
> This patch add vdd_center pwm regulator get_device to

You don't need to say 'This patch', just:

"Add vdd...

> enable this regulator.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
>  board/rockchip/evb_rk3399/evb-rk3399.c | 6 ++++++
>  1 file changed, 6 insertions(+)

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

[U-Boot] [PATCH 5/7] Kconfig: rockchip: enable DM_PWM and DM_REGULATOR

[Simon Glass]

On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
> This patch enable DM_PWM and DM_REGULATOR on rockchip SoCs.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
>  arch/arm/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)

I think this is safe.

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

[U-Boot] [PATCH 6/7] dts: evb-rk3399: add init voltage node for vdd-center

[Simon Glass]

Hi Kever,

On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
> This patch add regulator-init-microvolt for pwm regulator
> to get a init value when driver do probe init.

How about:

Add a regulator-init-microvolt value for the vd_center regulator so that  ....

(please complete the sentence - commits should explain why they are
needed if it isn't obvious)

>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
>  arch/arm/dts/rk3399-evb.dts | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/dts/rk3399-evb.dts b/arch/arm/dts/rk3399-evb.dts
> index bd7801b..fa60e19 100644
> --- a/arch/arm/dts/rk3399-evb.dts
> +++ b/arch/arm/dts/rk3399-evb.dts
> @@ -23,6 +23,7 @@
>                 regulator-name = "vdd_center";
>                 regulator-min-microvolt = <800000>;
>                 regulator-max-microvolt = <1400000>;
> +               regulator-init-microvolt = <950000>;
>                 regulator-always-on;
>                 regulator-boot-on;
>                 status = "okay";
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH 7/7] config: evb-rk3399: enable pwm regulator

[Simon Glass]

On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
> This patch enable the pwm regulator for evb-rk3399.
>

Please remove 'This patch'

> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
>  configs/evb-rk3399_defconfig | 1 +
>  1 file changed, 1 insertion(+)

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

[U-Boot] [PATCH 1/7] rockchip: rk3399: update PPLL and pmu_pclk frequency

[Kever Yang]

Hi Simon,

On 09/06/2016 09:03 AM, Simon Glass wrote:
> Hi Kever,
>
> On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
>> This patch update PPLL to 676MHz and PMU_PCLK to 48MHz.
> Why?
>

1. 48MHz can make sure the pwm can get exact 50% duty ratio, but 99MHz 
can not,
2. We think 48MHz is fast enough for pmu pclk and it is lower power cost 
than 99MHz,
3. PPLL 676 MHz and PMU_PCLK 48MHz are the clock rate we are using 
internally for kernel,
it suppose not to change the bus clock like pmu_pclk in kernel, so we 
want to change it in uboot.

Thanks,
- Kever
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>>   arch/arm/include/asm/arch-rockchip/cru_rk3399.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3399.h b/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
>> index c919f47..6776e48 100644
>> --- a/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
>> +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
>> @@ -64,9 +64,9 @@ check_member(rk3399_cru, sdio1_con[1], 0x594);
>>   #define APLL_HZ                (600*MHz)
>>   #define GPLL_HZ                (594*MHz)
>>   #define CPLL_HZ                (384*MHz)
>> -#define PPLL_HZ                (594*MHz)
>> +#define PPLL_HZ                (676*MHz)
>>
>> -#define PMU_PCLK_HZ    (99*MHz)
>> +#define PMU_PCLK_HZ    (48*MHz)
>>
>>   #define ACLKM_CORE_HZ  (300*MHz)
>>   #define ATCLK_CORE_HZ  (300*MHz)
>> --
>> 1.9.1
>>
> Regards,
> Simon
>
>
>

[U-Boot] [PATCH 3/7] power: regulator: add pwm regulator

[Kever Yang]

Hi Simon,

On 09/06/2016 09:03 AM, Simon Glass wrote:
> Hi Kever,
>
> On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
>> This driver add support for pwm regulator.
>>
>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>>   drivers/power/regulator/Kconfig         |   9 ++
>>   drivers/power/regulator/Makefile        |   1 +
>>   drivers/power/regulator/pwm_regulator.c | 157 ++++++++++++++++++++++++++++++++
>>   3 files changed, 167 insertions(+)
>>   create mode 100644 drivers/power/regulator/pwm_regulator.c
>>
>> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
>> index 17f22dd..c2eaa84 100644
>> --- a/drivers/power/regulator/Kconfig
>> +++ b/drivers/power/regulator/Kconfig
>> @@ -42,6 +42,15 @@ config DM_REGULATOR_PFUZE100
>>          features for REGULATOR PFUZE100. The driver implements get/set api for:
>>          value, enable and mode.
>>
>> +config REGULATOR_PWM
>> +       bool "Enable driver for PWM regulators"
>> +       depends on DM_REGULATOR
>> +       ---help---
>> +       Enable support for the regulator functions of the PWM. The
> Does a PWM have regulator functions? Do you mean a board that uses a
> PWM to control a regulator?

Yes, the PWM control the regulator, the voltage is depend on the PWM 
duty ratio.
The PWM regulator is used in some of rockchip board.

>
>> +       driver implements get/set api for the various BUCKS.
>> +       This driver is controlled by a device tree node
>> +       which includes voltage limits.
>> +
>>   config DM_REGULATOR_MAX77686
>>          bool "Enable Driver Model for REGULATOR MAX77686"
>>          depends on DM_REGULATOR && DM_PMIC_MAX77686
>> diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
>> index 1590d85..ab461ec 100644
>> --- a/drivers/power/regulator/Makefile
>> +++ b/drivers/power/regulator/Makefile
>> @@ -9,6 +9,7 @@ obj-$(CONFIG_$(SPL_)DM_REGULATOR) += regulator-uclass.o
>>   obj-$(CONFIG_REGULATOR_ACT8846) += act8846.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_FIXED) += fixed.o
>>   obj-$(CONFIG_REGULATOR_RK808) += rk808.o
>>   obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
>> diff --git a/drivers/power/regulator/pwm_regulator.c b/drivers/power/regulator/pwm_regulator.c
>> new file mode 100644
>> index 0000000..f75731b
>> --- /dev/null
>> +++ b/drivers/power/regulator/pwm_regulator.c
>> @@ -0,0 +1,157 @@
>> +/*
>> + * Copyright (C) 2016 Rockchip Electronics Co., Ltd
>> + *
>> + * Based on kernel drivers/regulator/pwm-regulator.c
>> + * Copyright (C) 2014 - STMicroelectronics Inc.
>> + * Author: Lee Jones <lee.jones@linaro.org>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <pwm.h>
>> +#include <power/regulator.h>
>> +#include <libfdt.h>
>> +#include <fdt_support.h>
>> +#include <fdtdec.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +struct pwm_regulator_info {
>> +       int pwm_id;
> Please add a comment for members

OK,
>
>> +       int period;
> period_ms is better, if this is milliseconds. Or period_us?

Yes, period_ns will be fine, which is used in pwm driver.
>
>> +       struct udevice *pwm;
>> +       unsigned int init_voltage;
>> +       unsigned int max_voltage;
>> +       unsigned int min_voltage;
>> +       unsigned int boot_on;
> bool?

I think this could be removed in next version for it is not used right now.
>
>> +       unsigned int volt_uV;
>> +};
>> +
>> +static int pwm_regulator_enable(struct udevice *dev, bool enable)
>> +{
>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>> +
>> +       return pwm_set_enable(priv->pwm, priv->pwm_id, enable);
>> +}
>> +
>> +static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int req_uV)
>> +{
>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>> +       int min_uV = priv->min_voltage;
>> +       int max_uV = priv->max_voltage;
>> +       int diff = max_uV - min_uV;
>> +
>> +       return 100 - (((req_uV * 100) - (min_uV * 100)) / diff);
>> +}
>> +
>> +static int pwm_regulator_get_voltage(struct udevice *dev)
>> +{
>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>> +
>> +       return priv->volt_uV;
>> +}
>> +
>> +static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt)
>> +{
>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>> +       int duty_cycle;
>> +       int ret = 0;
>> +
>> +       duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
>> +
>> +       ret = pwm_set_config(priv->pwm, priv->pwm_id,
>> +                       (priv->period / 100) * duty_cycle, priv->period);
>> +       if (ret) {
>> +               dev_err(dev, "Failed to configure PWM\n");
>> +               return ret;
>> +       }
>> +
>> +       ret = pwm_set_enable(priv->pwm, priv->pwm_id, true);
>> +       if (ret) {
>> +               dev_err(dev, "Failed to enable PWM\n");
>> +               return ret;
>> +       }
>> +       priv->volt_uV = uvolt;
>> +       return ret;
>> +}
>> +
>> +static int pwm_regulator_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>> +       struct fdtdec_phandle_args args;
>> +       const void *blob = gd->fdt_blob;
>> +       int node = dev->of_offset;
>> +       int ret;
>> +
>> +       ret = fdtdec_parse_phandle_with_args(blob, node, "pwms", "#pwm-cells",
>> +                                            0, 0, &args);
>> +       if (ret) {
>> +               debug("%s: Cannot get PWM phandle: ret=%d\n", __func__, ret);
>> +               return ret;
>> +       }
>> +
>> +       priv->period = args.args[1];
>> +
>> +       /* rkpwm do not use the pwm_id, set it to 0 */
>> +       priv->pwm_id = 0;
> But this is not a rockchip driver. Also private data starts at 0 so
> you can skip this.

This can be removed for rkpwm does not use it, some other SoC need to
fill it with DT parse if they need it.
>
>> +
>> +       priv->init_voltage = fdtdec_get_int(blob, node,
>> +                       "regulator-init-microvolt", 0);
>> +       if (priv->init_voltage < 0)
>> +               printf("Cannot find regulator pwm init_voltage\n");
> debug()
>
> If that is an error, please return -EINVAL. The error seems wrong
> also. If the property is missing then fdtdec_get_int() will return
> your default value (0). So perhaps the error should be 'invalid
> voltage < 0'. But actually I cannot see why such a check is useful.
> People should not make such mistakes in device-tree data.

How about using -1 us default value to get an error and then we can return
error code.
>
>> +
>> +       ret = uclass_get_device_by_of_offset(UCLASS_PWM, args.node, &priv->pwm);
>> +       if (ret) {
>> +               debug("%s: Cannot get PWM: ret=%d\n", __func__, ret);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int pwm_regulator_probe(struct udevice *dev)
>> +{
>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>> +       struct dm_regulator_uclass_platdata *uc_pdata;
>> +
>> +       uc_pdata = dev_get_uclass_platdata(dev);
>> +
>> +       uc_pdata->type = REGULATOR_TYPE_BUCK;
>> +       uc_pdata->mode_count = 0;
>> +       priv->max_voltage = uc_pdata->max_uV;
>> +       priv->min_voltage = uc_pdata->min_uV;
>> +
>> +       if (priv->init_voltage)
>> +               pwm_regulator_set_voltage(dev, priv->init_voltage);
>> +
>> +       if (priv->boot_on)
>> +               pwm_regulator_enable(dev, 1);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct dm_regulator_ops pwm_regulator_ops = {
>> +       .get_value  = pwm_regulator_get_voltage,
>> +       .set_value  = pwm_regulator_set_voltage,
>> +       .set_enable = pwm_regulator_enable,
>> +};
>> +
>> +static const struct udevice_id pwm_regulator_ids[] = {
>> +       { .compatible = "pwm-regulator" },
>> +       { .compatible = "rockchip_pwm_regulator" },
> Should that be rockchip,pwm-regulator ?

Yep, I will remove this in next version because the only compatible is 
enough now.

Thanks,
- Kever
>
>> +       { }
>> +};
>> +
>> +U_BOOT_DRIVER(pwm_regulator) = {
>> +       .name = "pwm_regulator",
>> +       .id = UCLASS_REGULATOR,
>> +       .ops = &pwm_regulator_ops,
>> +       .probe = pwm_regulator_probe,
>> +       .of_match = pwm_regulator_ids,
>> +       .ofdata_to_platdata     = pwm_regulator_ofdata_to_platdata,
>> +       .priv_auto_alloc_size   = sizeof(struct pwm_regulator_info),
>> +};
>> +
>> --
>> 1.9.1
>>
> Regards,
> Simon
>
>
>

[U-Boot] [PATCH 6/7] dts: evb-rk3399: add init voltage node for vdd-center

[Kever Yang]

Hi Simon,

On 09/06/2016 09:04 AM, Simon Glass wrote:
> Hi Kever,
>
> On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
>> This patch add regulator-init-microvolt for pwm regulator
>> to get a init value when driver do probe init.
> How about:
>
> Add a regulator-init-microvolt value for the vd_center regulator so that  ....
>
> (please complete the sentence - commits should explain why they are
> needed if it isn't obvious)
>

OK, will update in next version patch.

Thanks,
- Kever
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>>   arch/arm/dts/rk3399-evb.dts | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/dts/rk3399-evb.dts b/arch/arm/dts/rk3399-evb.dts
>> index bd7801b..fa60e19 100644
>> --- a/arch/arm/dts/rk3399-evb.dts
>> +++ b/arch/arm/dts/rk3399-evb.dts
>> @@ -23,6 +23,7 @@
>>                  regulator-name = "vdd_center";
>>                  regulator-min-microvolt = <800000>;
>>                  regulator-max-microvolt = <1400000>;
>> +               regulator-init-microvolt = <950000>;
>>                  regulator-always-on;
>>                  regulator-boot-on;
>>                  status = "okay";
>> --
>> 1.9.1
>>
> Regards,
> Simon
>
>
>

[U-Boot] [PATCH 1/7] rockchip: rk3399: update PPLL and pmu_pclk frequency

[Simon Glass]

Hi Kever,

On 6 September 2016 at 03:52, Kever Yang <kever.yang@rock-chips.com> wrote:
> Hi Simon,
>
> On 09/06/2016 09:03 AM, Simon Glass wrote:
>>
>> Hi Kever,
>>
>> On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>
>>> This patch update PPLL to 676MHz and PMU_PCLK to 48MHz.
>>
>> Why?
>>
>
> 1. 48MHz can make sure the pwm can get exact 50% duty ratio, but 99MHz can
> not,
> 2. We think 48MHz is fast enough for pmu pclk and it is lower power cost
> than 99MHz,
> 3. PPLL 676 MHz and PMU_PCLK 48MHz are the clock rate we are using
> internally for kernel,
> it suppose not to change the bus clock like pmu_pclk in kernel, so we want
> to change it in uboot.

OK, thank you. Please can you add this info to the commit message in
v2? My point is that commits should explain why they are needed, if
not obvious.

>
> Thanks,
> - Kever
>
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>> ---
>>>
>>>   arch/arm/include/asm/arch-rockchip/cru_rk3399.h | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
>>> b/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
>>> index c919f47..6776e48 100644
>>> --- a/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
>>> +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3399.h
>>> @@ -64,9 +64,9 @@ check_member(rk3399_cru, sdio1_con[1], 0x594);
>>>   #define APLL_HZ                (600*MHz)
>>>   #define GPLL_HZ                (594*MHz)
>>>   #define CPLL_HZ                (384*MHz)
>>> -#define PPLL_HZ                (594*MHz)
>>> +#define PPLL_HZ                (676*MHz)
>>>
>>> -#define PMU_PCLK_HZ    (99*MHz)
>>> +#define PMU_PCLK_HZ    (48*MHz)
>>>
>>>   #define ACLKM_CORE_HZ  (300*MHz)
>>>   #define ATCLK_CORE_HZ  (300*MHz)
>>> --
>>> 1.9.1
>>>
>> Regards,
>> Simon
>>
>>
>>
>
>

Regards,
Simon

[U-Boot] [PATCH 3/7] power: regulator: add pwm regulator

[Simon Glass]

Hi Kever,

On 6 September 2016 at 04:03, Kever Yang <kever.yang@rock-chips.com> wrote:
> Hi Simon,
>
>
> On 09/06/2016 09:03 AM, Simon Glass wrote:
>>
>> Hi Kever,
>>
>> On 29 August 2016 at 21:02, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>
>>> This driver add support for pwm regulator.
>>>
>>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>> ---
>>>
>>>   drivers/power/regulator/Kconfig         |   9 ++
>>>   drivers/power/regulator/Makefile        |   1 +
>>>   drivers/power/regulator/pwm_regulator.c | 157
>>> ++++++++++++++++++++++++++++++++
>>>   3 files changed, 167 insertions(+)
>>>   create mode 100644 drivers/power/regulator/pwm_regulator.c
>>>
>>> diff --git a/drivers/power/regulator/Kconfig
>>> b/drivers/power/regulator/Kconfig
>>> index 17f22dd..c2eaa84 100644
>>> --- a/drivers/power/regulator/Kconfig
>>> +++ b/drivers/power/regulator/Kconfig
>>> @@ -42,6 +42,15 @@ config DM_REGULATOR_PFUZE100
>>>          features for REGULATOR PFUZE100. The driver implements get/set
>>> api for:
>>>          value, enable and mode.
>>>
>>> +config REGULATOR_PWM
>>> +       bool "Enable driver for PWM regulators"
>>> +       depends on DM_REGULATOR
>>> +       ---help---
>>> +       Enable support for the regulator functions of the PWM. The
>>
>> Does a PWM have regulator functions? Do you mean a board that uses a
>> PWM to control a regulator?
>
>
> Yes, the PWM control the regulator, the voltage is depend on the PWM duty
> ratio.
> The PWM regulator is used in some of rockchip board.

OK please can you update the help a little?

>
>
>>
>>> +       driver implements get/set api for the various BUCKS.
>>> +       This driver is controlled by a device tree node
>>> +       which includes voltage limits.
>>> +
>>>   config DM_REGULATOR_MAX77686
>>>          bool "Enable Driver Model for REGULATOR MAX77686"
>>>          depends on DM_REGULATOR && DM_PMIC_MAX77686
>>> diff --git a/drivers/power/regulator/Makefile
>>> b/drivers/power/regulator/Makefile
>>> index 1590d85..ab461ec 100644
>>> --- a/drivers/power/regulator/Makefile
>>> +++ b/drivers/power/regulator/Makefile
>>> @@ -9,6 +9,7 @@ obj-$(CONFIG_$(SPL_)DM_REGULATOR) += regulator-uclass.o
>>>   obj-$(CONFIG_REGULATOR_ACT8846) += act8846.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_FIXED) += fixed.o
>>>   obj-$(CONFIG_REGULATOR_RK808) += rk808.o
>>>   obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
>>> diff --git a/drivers/power/regulator/pwm_regulator.c
>>> b/drivers/power/regulator/pwm_regulator.c
>>> new file mode 100644
>>> index 0000000..f75731b
>>> --- /dev/null
>>> +++ b/drivers/power/regulator/pwm_regulator.c
>>> @@ -0,0 +1,157 @@
>>> +/*
>>> + * Copyright (C) 2016 Rockchip Electronics Co., Ltd
>>> + *
>>> + * Based on kernel drivers/regulator/pwm-regulator.c
>>> + * Copyright (C) 2014 - STMicroelectronics Inc.
>>> + * Author: Lee Jones <lee.jones@linaro.org>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <dm.h>
>>> +#include <errno.h>
>>> +#include <pwm.h>
>>> +#include <power/regulator.h>
>>> +#include <libfdt.h>
>>> +#include <fdt_support.h>
>>> +#include <fdtdec.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +struct pwm_regulator_info {
>>> +       int pwm_id;
>>
>> Please add a comment for members
>
>
> OK,
>>
>>
>>> +       int period;
>>
>> period_ms is better, if this is milliseconds. Or period_us?
>
>
> Yes, period_ns will be fine, which is used in pwm driver.
>>
>>
>>> +       struct udevice *pwm;
>>> +       unsigned int init_voltage;
>>> +       unsigned int max_voltage;
>>> +       unsigned int min_voltage;
>>> +       unsigned int boot_on;
>>
>> bool?
>
>
> I think this could be removed in next version for it is not used right now.
>
>>
>>> +       unsigned int volt_uV;
>>> +};
>>> +
>>> +static int pwm_regulator_enable(struct udevice *dev, bool enable)
>>> +{
>>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>>> +
>>> +       return pwm_set_enable(priv->pwm, priv->pwm_id, enable);
>>> +}
>>> +
>>> +static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int
>>> req_uV)
>>> +{
>>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>>> +       int min_uV = priv->min_voltage;
>>> +       int max_uV = priv->max_voltage;
>>> +       int diff = max_uV - min_uV;
>>> +
>>> +       return 100 - (((req_uV * 100) - (min_uV * 100)) / diff);
>>> +}
>>> +
>>> +static int pwm_regulator_get_voltage(struct udevice *dev)
>>> +{
>>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>>> +
>>> +       return priv->volt_uV;
>>> +}
>>> +
>>> +static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt)
>>> +{
>>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>>> +       int duty_cycle;
>>> +       int ret = 0;
>>> +
>>> +       duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
>>> +
>>> +       ret = pwm_set_config(priv->pwm, priv->pwm_id,
>>> +                       (priv->period / 100) * duty_cycle, priv->period);
>>> +       if (ret) {
>>> +               dev_err(dev, "Failed to configure PWM\n");
>>> +               return ret;
>>> +       }
>>> +
>>> +       ret = pwm_set_enable(priv->pwm, priv->pwm_id, true);
>>> +       if (ret) {
>>> +               dev_err(dev, "Failed to enable PWM\n");
>>> +               return ret;
>>> +       }
>>> +       priv->volt_uV = uvolt;
>>> +       return ret;
>>> +}
>>> +
>>> +static int pwm_regulator_ofdata_to_platdata(struct udevice *dev)
>>> +{
>>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>>> +       struct fdtdec_phandle_args args;
>>> +       const void *blob = gd->fdt_blob;
>>> +       int node = dev->of_offset;
>>> +       int ret;
>>> +
>>> +       ret = fdtdec_parse_phandle_with_args(blob, node, "pwms",
>>> "#pwm-cells",
>>> +                                            0, 0, &args);
>>> +       if (ret) {
>>> +               debug("%s: Cannot get PWM phandle: ret=%d\n", __func__,
>>> ret);
>>> +               return ret;
>>> +       }
>>> +
>>> +       priv->period = args.args[1];
>>> +
>>> +       /* rkpwm do not use the pwm_id, set it to 0 */
>>> +       priv->pwm_id = 0;
>>
>> But this is not a rockchip driver. Also private data starts at 0 so
>> you can skip this.
>
>
> This can be removed for rkpwm does not use it, some other SoC need to
> fill it with DT parse if they need it.

OK, then can you just remove rkpwm and the assignment from the
comment? It will be confusing in a generic driver. How about something
like:

/*set pwm_id here from device tree if needed */

>>
>>
>>> +
>>> +       priv->init_voltage = fdtdec_get_int(blob, node,
>>> +                       "regulator-init-microvolt", 0);
>>> +       if (priv->init_voltage < 0)
>>> +               printf("Cannot find regulator pwm init_voltage\n");
>>
>> debug()
>>
>> If that is an error, please return -EINVAL. The error seems wrong
>> also. If the property is missing then fdtdec_get_int() will return
>> your default value (0). So perhaps the error should be 'invalid
>> voltage < 0'. But actually I cannot see why such a check is useful.
>> People should not make such mistakes in device-tree data.
>
>
> How about using -1 us default value to get an error and then we can return
> error code.

Yes you can do that.

>
>>
>>> +
>>> +       ret = uclass_get_device_by_of_offset(UCLASS_PWM, args.node,
>>> &priv->pwm);
>>> +       if (ret) {
>>> +               debug("%s: Cannot get PWM: ret=%d\n", __func__, ret);
>>> +               return ret;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int pwm_regulator_probe(struct udevice *dev)
>>> +{
>>> +       struct pwm_regulator_info *priv = dev_get_priv(dev);
>>> +       struct dm_regulator_uclass_platdata *uc_pdata;
>>> +
>>> +       uc_pdata = dev_get_uclass_platdata(dev);
>>> +
>>> +       uc_pdata->type = REGULATOR_TYPE_BUCK;
>>> +       uc_pdata->mode_count = 0;
>>> +       priv->max_voltage = uc_pdata->max_uV;
>>> +       priv->min_voltage = uc_pdata->min_uV;
>>> +
>>> +       if (priv->init_voltage)
>>> +               pwm_regulator_set_voltage(dev, priv->init_voltage);
>>> +
>>> +       if (priv->boot_on)
>>> +               pwm_regulator_enable(dev, 1);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static const struct dm_regulator_ops pwm_regulator_ops = {
>>> +       .get_value  = pwm_regulator_get_voltage,
>>> +       .set_value  = pwm_regulator_set_voltage,
>>> +       .set_enable = pwm_regulator_enable,
>>> +};
>>> +
>>> +static const struct udevice_id pwm_regulator_ids[] = {
>>> +       { .compatible = "pwm-regulator" },
>>> +       { .compatible = "rockchip_pwm_regulator" },
>>
>> Should that be rockchip,pwm-regulator ?
>
>
> Yep, I will remove this in next version because the only compatible is
> enough now.
>
> Thanks,
> - Kever
>
>>
>>> +       { }
>>> +};
>>> +
>>> +U_BOOT_DRIVER(pwm_regulator) = {
>>> +       .name = "pwm_regulator",
>>> +       .id = UCLASS_REGULATOR,
>>> +       .ops = &pwm_regulator_ops,
>>> +       .probe = pwm_regulator_probe,
>>> +       .of_match = pwm_regulator_ids,
>>> +       .ofdata_to_platdata     = pwm_regulator_ofdata_to_platdata,
>>> +       .priv_auto_alloc_size   = sizeof(struct pwm_regulator_info),
>>> +};
>>> +
>>> --
>>> 1.9.1
>>>
>> Regards,
>> Simon
>>
>>
>>
>
>

Regards,
Simon