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

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

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

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

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

[Jaehoon Chung]

Hi Kever,

On 04/07/2017 08:02 PM, Kever Yang 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(-)
> 
> 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;
>  	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);
> 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)
> +{

"channel" is need?

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

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

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

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

[Simon Glass]

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.

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

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

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

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

[Elaine Zhang]

On 04/07/2017 07:28 PM, Jaehoon Chung wrote:
> Hi Kever,
>
> On 04/07/2017 08:02 PM, Kever Yang 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(-)
>>
>> 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;
>>   	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);
>> 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)
>> +{
>
> "channel" is need?
Don't need.
>
>> +	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);
>
> 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.

> Best Regards,
> Jaehoon Chung
>
>> +	/**
>>   	 * set_config() - Set the PWM configuration
>>   	 *
>>   	 * @dev:	PWM device to update
>>
>
>
>
>

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

[Elaine Zhang]

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

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

[Simon Glass]

Hi Elaine,

On 9 April 2017 at 20:09, Elaine Zhang <zhangqing@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@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.

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