power: regulator: pwm: support pwm polarity setting

PATCH Superseded Kever Yang 4 participants 6 messages Apr 7, 2017 – Apr 17, 2017 PWM

Share & tools

Kever Yang

author

2017-04-07 11:02 UTC #0 lore raw

PATCH 1/1 power: regulator: pwm: support pwm polarity setting

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 <[email protected]>

---

 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(-)

drivers/power/regulator/pwm_regulator.c +10 -2

index 4875238..f8a6712 100644
@@ -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;
 	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);
+	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];
 
 	priv->init_voltage = fdtdec_get_int(blob, node,
 			"regulator-init-microvolt", -1);

drivers/pwm/pwm-uclass.c +10 -0

index c2200af..287a7f3 100644
@@ -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)
 {

drivers/pwm/rk_pwm.c +16 -1

index 9254f5b..5ff1e00 100644
@@ -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,
 };

include/pwm.h +9 -0

index 851915e..a66ee1f 100644
@@ -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

Signature · 1 line

1.9.1

Jaehoon Chung

2017-04-07 11:28 UTC #1 lore raw

PATCH 1/1 power: regulator: pwm: support pwm polarity setting

Hi Kever,

Quoted 88 lines · Kever Yang

Signed-off-by Kever Yang <[email protected]>

---

 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(-)

drivers/power/regulator/pwm_regulator.c +10 -2

index 4875238..f8a6712 100644
@@ -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;
 	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);
+	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];
 
 	priv->init_voltage = fdtdec_get_int(blob, node,
 			"regulator-init-microvolt", -1);

drivers/pwm/pwm-uclass.c +10 -0

index c2200af..287a7f3 100644
@@ -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)
 {

drivers/pwm/rk_pwm.c +0 -0

index 9254f5b..5ff1e00 100644

drivers/pwm/rk_pwm.c DECLARE_GLOBAL_DATA_PTR; · line 28

Quoted 8 lines

drivers/pwm/rk_pwm.c +3 -0

@@ -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)
+{

"channel" is need?

Quoted 37 lines

drivers/pwm/rk_pwm.c +13 -1

+	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,
 };

include/pwm.h +0 -0

index 851915e..a66ee1f 100644
@@ -14,6 +14,15 @@
 /* struct pwm_ops: Operations for the PWM uclass */

include/pwm.h line 24

Quoted 10 lines

include/pwm.h +8 -0

 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);

Is there a reason about using int type? It's always return 0... Otherwise do you have a plan to use "int" type?

Best Regards,
Jaehoon Chung

Quoted 5 lines

include/pwm.h +1 -0

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

Elaine Zhang

2017-04-10 01:50 UTC #2 lore raw

PATCH 1/1 power: regulator: pwm: support pwm polarity setting

Quoted 97 lines · Jaehoon Chung

Hi Kever,

Quoted 94 lines · Kever Yang

Signed-off-by Kever Yang <[email protected]>

---

  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(-)

drivers/power/regulator/pwm_regulator.c +10 -2

index 4875238..f8a6712 100644
@@ -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;
  	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);
+	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];
 
  	priv->init_voltage = fdtdec_get_int(blob, node,
  			"regulator-init-microvolt", -1);

drivers/pwm/pwm-uclass.c +10 -0

index c2200af..287a7f3 100644
@@ -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)
  {

drivers/pwm/rk_pwm.c +1 -0

index 9254f5b..5ff1e00 100644
@@ -21,8 +21,22 @@ DECLARE_GLOBAL_DATA_PTR;
  struct rk_pwm_priv {
  	struct rk3288_pwm *regs;
  	ulong freq;
+	uint enable_conf;
  };

Quoted 4 lines

drivers/pwm/rk_pwm.c DECLARE_GLOBAL_DATA_PTR; · line 28
Quoted 2 lines
drivers/pwm/rk_pwm.c +2 -0
+static int rk_pwm_set_init(struct udevice *dev, uint channel, uint polarity)
+{
"channel" is need?

Don't need.

Quoted 43 lines

drivers/pwm/rk_pwm.c +13 -1

+	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,
  };

include/pwm.h +3 -0

index 851915e..a66ee1f 100644
@@ -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

Quoted 9 lines

include/pwm.h line 24
Quoted 5 lines
include/pwm.h +5 -0
+	 * @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);
Is there a reason about using int type? It's always return 0... Otherwise do you have a plan to use "int" type?

No particular reason.
just follow as set_config and set_enable.

Quoted 12 lines

Best Regards,
Jaehoon Chung
Quoted 5 lines
include/pwm.h +1 -0
+	/**
  	 * set_config() - Set the PWM configuration
  	 *
  	 * @dev:	PWM device to update

Simon Glass

2017-04-09 19:28 UTC #3 lore raw

PATCH 1/1 power: regulator: pwm: support pwm polarity setting

In reply to Kever Yang

Hi,

Quoted 7 lines · Kever Yang

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.

Quoted 8 lines

Signed-off-by Kever Yang <[email protected]>

---

 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.

drivers/power/regulator/pwm_regulator.c struct pwm_regulator_info { · line 28

Quoted 11 lines

drivers/power/regulator/pwm_regulator.c +2 -0

index 4875238..f8a6712 100644
@@ -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?

Quoted 13 lines

drivers/power/regulator/pwm_regulator.c +1 -1

        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)

drivers/power/regulator/pwm_regulator.c static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt) · line 72

Quoted 4 lines

drivers/power/regulator/pwm_regulator.c +1 -0

@@ -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.

Quoted 9 lines

drivers/power/regulator/pwm_regulator.c +5 -0

+       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) {

drivers/power/regulator/pwm_regulator.c static int pwm_regulator_ofdata_to_platdata(struct udevice *dev) · line 110

Quoted 7 lines

drivers/power/regulator/pwm_regulator.c +1 -1

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

Quoted 89 lines

drivers/power/regulator/pwm_regulator.c +0 -0

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

drivers/pwm/pwm-uclass.c +10 -0

index c2200af..287a7f3 100644
@@ -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)
 {

drivers/pwm/rk_pwm.c +16 -1

index 9254f5b..5ff1e00 100644
@@ -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,
 };

include/pwm.h +9 -1

index 851915e..a66ee1f 100644
@@ -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
--

Quoted 2 lines

1.9.1

Regards,
Simon

Elaine Zhang

2017-04-10 02:09 UTC #4 lore raw

PATCH 1/1 power: regulator: pwm: support pwm polarity setting

Quoted 19 lines · Simon Glass

Hi,
Quoted 7 lines · Kever Yang
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.
Quoted 8 lines
Signed-off-by Kever Yang <[email protected]>
---
  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(-)

Quoted 5 lines

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.

Quoted 15 lines

drivers/power/regulator/pwm_regulator.c struct pwm_regulator_info { · line 28
Quoted 12 lines
drivers/power/regulator/pwm_regulator.c +2 -0
index 4875238..f8a6712 100644
@@ -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.

Quoted 15 lines

drivers/power/regulator/pwm_regulator.c +1 -1

         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)

Quoted 7 lines

drivers/power/regulator/pwm_regulator.c static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt) · line 72
Quoted 3 lines
drivers/power/regulator/pwm_regulator.c +1 -0
         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.

Quoted 119 lines

Quoted 10 lines

drivers/power/regulator/pwm_regulator.c +5 -0

+       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) {

drivers/power/regulator/pwm_regulator.c static int pwm_regulator_ofdata_to_platdata(struct udevice *dev) · line 110

Quoted 7 lines

drivers/power/regulator/pwm_regulator.c +1 -1

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

Quoted 89 lines

drivers/power/regulator/pwm_regulator.c +0 -0

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

drivers/pwm/pwm-uclass.c +10 -0

index c2200af..287a7f3 100644
@@ -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)
  {

drivers/pwm/rk_pwm.c +16 -1

index 9254f5b..5ff1e00 100644
@@ -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,
  };

include/pwm.h +9 -1

index 851915e..a66ee1f 100644
@@ -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
--

Quoted 2 lines

1.9.1

Regards,
Simon

Simon Glass

2017-04-17 03:03 UTC #5 lore raw

PATCH 1/1 power: regulator: pwm: support pwm polarity setting

Hi Elaine,

Quoted 25 lines · Elaine Zhang

Quoted 22 lines · Simon Glass
Hi,
Quoted 16 lines · Kever Yang
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 <[email protected]>
---
  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(-)

Quoted 7 lines

Quoted 5 lines
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/

Quoted 49 lines

Quoted 18 lines
Quoted 9 lines
drivers/power/regulator/pwm_regulator.c +0 -0
index 4875238..f8a6712 100644
drivers/power/regulator/pwm_regulator.c struct pwm_regulator_info { · line 28
Quoted 5 lines
drivers/power/regulator/pwm_regulator.c +2 -0
@@ -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.
Quoted 18 lines
Quoted 18 lines
drivers/power/regulator/pwm_regulator.c +1 -1
         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)
drivers/power/regulator/pwm_regulator.c +0 -0
@@ -67,6 +69,12 @@ static int pwm_regulator_set_voltage(struct udevice
 *dev, int uvolt)
 
Quoted 8 lines
drivers/power/regulator/pwm_regulator.c static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt) · line 72
Quoted 3 lines
drivers/power/regulator/pwm_regulator.c +1 -0
         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.

Quoted 2 lines

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.

Quoted 19 lines

drivers/power/regulator/pwm_regulator.c +5 -0

+       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) {

drivers/power/regulator/pwm_regulator.c +0 -1

@@ -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 */

Quoted 8 lines

Quoted 8 lines
drivers/power/regulator/pwm_regulator.c static int pwm_regulator_ofdata_to_platdata(struct udevice *dev) · line 110
Quoted 2 lines
drivers/power/regulator/pwm_regulator.c +1 -0
         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