[U-Boot] [PATCH] power: regulator: pwm: support pwm polarity setting

[Simon Glass]

Hi Elaine,

On 9 April 2017 at 20:09, Elaine Zhang <zhangqing at rock-chips.com> wrote:
>
>
> On 04/10/2017 03:28 AM, Simon Glass wrote:
>>
>> Hi,
>>
>> On 7 April 2017 at 05:02, Kever Yang <kever.yang at rock-chips.com> wrote:
>>>
>>> The latest kernel PWM drivers enable the polarity settings. When system
>>> run from U-Boot to kerenl, if there are differences in polarity set or
>>> duty cycle, the PMW will re-init:
>>>    close -> set polarity and duty cycle -> enable the PWM.
>>> The power supply controled by pwm regulator may have voltage shaking,
>>> which lead to the system not stable.
>>>
>>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>>> ---
>>>
>>>   drivers/power/regulator/pwm_regulator.c | 12 ++++++++++--
>>>   drivers/pwm/pwm-uclass.c                | 10 ++++++++++
>>>   drivers/pwm/rk_pwm.c                    | 17 ++++++++++++++++-
>>>   include/pwm.h                           |  9 +++++++++
>>>   4 files changed, 45 insertions(+), 3 deletions(-)
>>
>>
>> If the generic uclass change and the rk change can be separated, it is
>> good to do it.
>>
>> Also we should have a test for pwm (test/dm/pwm.c). Are you able to do
>> that? If not I could give it a try.
>
> we have no plan to do it.

I've created something simple here. You can base your next series on
u-boot-dm/pwm-working.

http://patchwork.ozlabs.org/patch/751254/

>>
>>
>>>
>>> diff --git a/drivers/power/regulator/pwm_regulator.c
>>> b/drivers/power/regulator/pwm_regulator.c
>>> index 4875238..f8a6712 100644
>>> --- a/drivers/power/regulator/pwm_regulator.c
>>> +++ b/drivers/power/regulator/pwm_regulator.c
>>> @@ -24,6 +24,8 @@ struct pwm_regulator_info {
>>>          int pwm_id;
>>>          /* the period of one PWM cycle */
>>>          int period_ns;
>>> +       /* the polarity of one PWM */
>>> +       int polarity;
>>
>>
>> Can you update the comment to indicate what the values mean? E.g. is 0
>> the normal polarity and 1 inverted?
>
> 0 : normal polarity
> 1 : inverted polarity
> I will update the comment next version.
>>
>>
>>>          struct udevice *pwm;
>>>          /* initialize voltage of regulator */
>>>          unsigned int init_voltage;
>>> @@ -49,7 +51,7 @@ static int pwm_voltage_to_duty_cycle_percentage(struct
>>> udevice *dev, int req_uV)
>>>          int max_uV = priv->max_voltage;
>>>          int diff = max_uV - min_uV;
>>>
>>> -       return 100 - (((req_uV * 100) - (min_uV * 100)) / diff);
>>> +       return ((req_uV * 100) - (min_uV * 100)) / diff;
>>>   }
>>>
>>>   static int pwm_regulator_get_voltage(struct udevice *dev)
>>> @@ -67,6 +69,12 @@ static int pwm_regulator_set_voltage(struct udevice
>>> *dev, int uvolt)
>>>
>>>          duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt);
>>>
>>> +       ret = pwm_set_init(priv->pwm, priv->pwm_id, priv->polarity);
>>
>>
>> I wonder if it would be better to combine the polarity into the
>> pwm_set_config() call? Then we can do everything in one call. If not
>> then I think pwm_set_invert() would be a better name.
>
> The polarity set only once, so we set it in pwm_set_init() call.
> Using pwm_set_invert, of course, is also possible.

OK, so that's why it should a separate call. Seems OK to me.

>
>>
>>> +       if (ret) {
>>> +               dev_err(dev, "Failed to init PWM\n");
>>> +               return ret;
>>> +       }
>>> +
>>>          ret = pwm_set_config(priv->pwm, priv->pwm_id,
>>>                          (priv->period_ns / 100) * duty_cycle,
>>> priv->period_ns);
>>>          if (ret) {
>>> @@ -97,9 +105,9 @@ static int pwm_regulator_ofdata_to_platdata(struct
>>> udevice *dev)
>>>                  debug("%s: Cannot get PWM phandle: ret=%d\n", __func__,
>>> ret);
>>>                  return ret;
>>>          }
>>> -       /* TODO: pwm_id here from device tree if needed */
>>>
>>>          priv->period_ns = args.args[1];
>>> +       priv->polarity = args.args[2];
>>
>>
>> Does this mean that the binding has this argument and we have been
>> ignoring it?
>>
>> Can you bring in the DT binding file from Linux to U-Boot also?

Did you see this one?

Regards,
Simon