Hi Kever, On 6 September 2016 at 04:03, Kever Yang <kever.y...@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.y...@rock-chips.com> wrote: >>> >>> This driver add support for pwm regulator. >>> >>> Signed-off-by: Elaine Zhang <zhangq...@rock-chips.com> >>> Signed-off-by: Kever Yang <kever.y...@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.jo...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot