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

On 04/10/2017 03:28 AM, Simon Glass wrote:

Hi,

On 7 April 2017 at 05:02, Kever Yang kever.yang@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@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.

...

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.

...

•   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?

...

     priv->init_voltage = fdtdec_get_int(blob, node,
                     "regulator-init-microvolt", -1);

diff --git a/drivers/pwm/pwm-uclass.c b/drivers/pwm/pwm-uclass.c index c2200af..287a7f3 100644 --- a/drivers/pwm/pwm-uclass.c +++ b/drivers/pwm/pwm-uclass.c @@ -9,6 +9,16 @@ #include <dm.h> #include <pwm.h>

+int pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{

•   struct pwm_ops *ops = pwm_get_ops(dev);
  

•   if (!ops->set_init)
  

•           return -ENOSYS;
  

•   return ops->set_init(dev, channel, polarity);
  

+}

• int pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) {

diff --git a/drivers/pwm/rk_pwm.c b/drivers/pwm/rk_pwm.c index 9254f5b..5ff1e00 100644 --- a/drivers/pwm/rk_pwm.c +++ b/drivers/pwm/rk_pwm.c @@ -21,8 +21,22 @@ DECLARE_GLOBAL_DATA_PTR; struct rk_pwm_priv { struct rk3288_pwm *regs; ulong freq;

•   uint enable_conf;
  

  };

+static int rk_pwm_set_init(struct udevice *dev, uint channel, uint polarity) +{

•   struct rk_pwm_priv *priv = dev_get_priv(dev);
  

•   debug("%s: polarity=%u\n", __func__, polarity);
  

•   if (polarity)
  

•           priv->enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSTIVE;
  

•   else
  

•           priv->enable_conf |= PWM_DUTY_POSTIVE | PWM_INACTIVE_NEGATIVE;
  

•   return 0;
  

+}

• static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns, uint duty_ns) {

@@ -32,7 +46,7 @@ static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,

     debug("%s: period_ns=%u, duty_ns=%u\n", __func__, period_ns, duty_ns);
     writel(PWM_SEL_SRC_CLK | PWM_OUTPUT_LEFT | PWM_LP_DISABLE |

•           PWM_CONTINUOUS | PWM_DUTY_POSTIVE | PWM_INACTIVE_POSTIVE |
  

•           PWM_CONTINUOUS | priv->enable_conf |
             RK_PWM_DISABLE,
             &regs->ctrl);
  
  

@@ -83,6 +97,7 @@ static int rk_pwm_probe(struct udevice *dev) }

static const struct pwm_ops rk_pwm_ops = {

•   .set_init       = rk_pwm_set_init,
     .set_config     = rk_pwm_set_config,
     .set_enable     = rk_pwm_set_enable,
  

  };

diff --git a/include/pwm.h b/include/pwm.h index 851915e..a66ee1f 100644 --- a/include/pwm.h +++ b/include/pwm.h @@ -14,6 +14,15 @@ /* struct pwm_ops: Operations for the PWM uclass */ struct pwm_ops { /**

•    * set_init() - Set the PWM invert
  

•    *
  

•    * @dev:        PWM device to update
  

•    * @channel:    PWM channel to update
  

•    * @polarity:  PWM invert polarity
  

•    * @return 0 if OK, -ve on error
  

•    */
  

•   int (*set_init)(struct udevice *dev, uint channel, uint polarity);
  

•   /**
      * set_config() - Set the PWM configuration
      *
      * @dev:        PWM device to update
  

-- 1.9.1

Regards, Simon