[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 at 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 at rock-chips.com> wrote:
>>>
>>> This driver add support for pwm regulator.
>>>
>>> Signed-off-by: Elaine Zhang <zhangqing at rock-chips.com>
>>> Signed-off-by: Kever Yang <kever.yang at 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 at 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