Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1412897 > unrolled thread

[PATCH 10/14] regulator: pwm: Switch to the atomic PWM API

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 10/14] regulator: pwm: Switch to the atomic PWM API Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 10:30 +0200
    Re: [PATCH 10/14] regulator: pwm: Switch to the atomic PWM API Brian Norris <briannorris@chromium.org> - 2016-06-03 23:00 +0200
      Re: [PATCH 10/14] regulator: pwm: Switch to the atomic PWM API Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-04 08:30 +0200
        Re: [PATCH 10/14] regulator: pwm: Switch to the atomic PWM API Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-06 08:30 +0200

#1412897 — [PATCH 10/14] regulator: pwm: Switch to the atomic PWM API

Use the atomic API wherever appropriate and get rid of pwm_apply_args()
call (the reference period and polarity are now explicitly set when
calling pwm_apply_state()).

We also make use of the pwm_set_relative_duty_cycle() helper to ease
relative to absolute duty_cycle conversion.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/regulator/pwm-regulator.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 524b43f..bf033fd 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -59,16 +59,14 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
 					 unsigned selector)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
-	struct pwm_args pargs;
-	int dutycycle;
+	struct pwm_state pstate;
 	int ret;
 
-	pwm_get_args(drvdata->pwm, &pargs);
+	pwm_prepare_new_state(drvdata->pwm, &pstate);
+	pwm_set_relative_duty_cycle(&pstate,
+			drvdata->duty_cycle_table[selector].dutycycle, 100);
 
-	dutycycle = (pargs.period *
-		    drvdata->duty_cycle_table[selector].dutycycle) / 100;
-
-	ret = pwm_config(drvdata->pwm, dutycycle, pargs.period);
+	ret = pwm_apply_state(drvdata->pwm, &pstate);
 	if (ret) {
 		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
 		return ret;
@@ -126,34 +124,18 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
 	unsigned int ramp_delay = rdev->constraints->ramp_delay;
-	struct pwm_args pargs;
 	unsigned int req_diff = min_uV - rdev->constraints->min_uV;
+	struct pwm_state pstate;
 	unsigned int diff;
-	unsigned int duty_pulse;
-	u64 req_period;
-	u32 rem;
 	int ret;
 
-	pwm_get_args(drvdata->pwm, &pargs);
+	pwm_prepare_new_state(drvdata->pwm, &pstate);
 	diff = rdev->constraints->max_uV - rdev->constraints->min_uV;
 
-	/* First try to find out if we get the iduty cycle time which is
-	 * factor of PWM period time. If (request_diff_to_min * pwm_period)
-	 * is perfect divided by voltage_range_diff then it is possible to
-	 * get duty cycle time which is factor of PWM period. This will help
-	 * to get output voltage nearer to requested value as there is no
-	 * calculation loss.
-	 */
-	req_period = req_diff * pargs.period;
-	div_u64_rem(req_period, diff, &rem);
-	if (!rem) {
-		do_div(req_period, diff);
-		duty_pulse = (unsigned int)req_period;
-	} else {
-		duty_pulse = (pargs.period / 100) * ((req_diff * 100) / diff);
-	}
+	/* We pass diff as the scale to get a uV precision. */
+	pwm_set_relative_duty_cycle(&pstate, req_diff, diff);
 
-	ret = pwm_config(drvdata->pwm, duty_pulse, pargs.period);
+	ret = pwm_apply_state(drvdata->pwm, &pstate);
 	if (ret) {
 		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
 		return ret;
-- 
2.7.4

[toc] | [next] | [standalone]

#1413476

+ Laxman

Hi,

On Fri, Jun 03, 2016 at 10:23:08AM +0200, Boris Brezillon wrote:
> Use the atomic API wherever appropriate and get rid of pwm_apply_args()
> call (the reference period and polarity are now explicitly set when
> calling pwm_apply_state()).
> 
> We also make use of the pwm_set_relative_duty_cycle() helper to ease
> relative to absolute duty_cycle conversion.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/regulator/pwm-regulator.c | 38 ++++++++++----------------------------
>  1 file changed, 10 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index 524b43f..bf033fd 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -59,16 +59,14 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
>  					 unsigned selector)
>  {
>  	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
> -	struct pwm_args pargs;
> -	int dutycycle;
> +	struct pwm_state pstate;
>  	int ret;
>  
> -	pwm_get_args(drvdata->pwm, &pargs);
> +	pwm_prepare_new_state(drvdata->pwm, &pstate);
> +	pwm_set_relative_duty_cycle(&pstate,
> +			drvdata->duty_cycle_table[selector].dutycycle, 100);
>  
> -	dutycycle = (pargs.period *
> -		    drvdata->duty_cycle_table[selector].dutycycle) / 100;
> -
> -	ret = pwm_config(drvdata->pwm, dutycycle, pargs.period);
> +	ret = pwm_apply_state(drvdata->pwm, &pstate);
>  	if (ret) {
>  		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
>  		return ret;
> @@ -126,34 +124,18 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
>  {
>  	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
>  	unsigned int ramp_delay = rdev->constraints->ramp_delay;
> -	struct pwm_args pargs;
>  	unsigned int req_diff = min_uV - rdev->constraints->min_uV;
> +	struct pwm_state pstate;
>  	unsigned int diff;
> -	unsigned int duty_pulse;
> -	u64 req_period;
> -	u32 rem;
>  	int ret;
>  
> -	pwm_get_args(drvdata->pwm, &pargs);
> +	pwm_prepare_new_state(drvdata->pwm, &pstate);
>  	diff = rdev->constraints->max_uV - rdev->constraints->min_uV;
>  
> -	/* First try to find out if we get the iduty cycle time which is
> -	 * factor of PWM period time. If (request_diff_to_min * pwm_period)
> -	 * is perfect divided by voltage_range_diff then it is possible to
> -	 * get duty cycle time which is factor of PWM period. This will help
> -	 * to get output voltage nearer to requested value as there is no
> -	 * calculation loss.
> -	 */
> -	req_period = req_diff * pargs.period;
> -	div_u64_rem(req_period, diff, &rem);
> -	if (!rem) {
> -		do_div(req_period, diff);
> -		duty_pulse = (unsigned int)req_period;
> -	} else {
> -		duty_pulse = (pargs.period / 100) * ((req_diff * 100) / diff);
> -	}
> +	/* We pass diff as the scale to get a uV precision. */
> +	pwm_set_relative_duty_cycle(&pstate, req_diff, diff);

Notably, you're dropping much of Laxman's commit fd786fb0276a ("regulator:
pwm: Try to avoid voltage error in duty cycle calculation"), but I
believe the DIV_ROUND_CLOSEST_ULL() in pwm_set_relative_duty_cycle()
solves his problem better.

>  
> -	ret = pwm_config(drvdata->pwm, duty_pulse, pargs.period);
> +	ret = pwm_apply_state(drvdata->pwm, &pstate);
>  	if (ret) {
>  		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
>  		return ret;

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

[toc] | [prev] | [next] | [standalone]

#1413661

On Fri, 3 Jun 2016 13:50:28 -0700
Brian Norris <briannorris@chromium.org> wrote:

> + Laxman
> 
> Hi,
> 
> On Fri, Jun 03, 2016 at 10:23:08AM +0200, Boris Brezillon wrote:
> > Use the atomic API wherever appropriate and get rid of pwm_apply_args()
> > call (the reference period and polarity are now explicitly set when
> > calling pwm_apply_state()).
> > 
> > We also make use of the pwm_set_relative_duty_cycle() helper to ease
> > relative to absolute duty_cycle conversion.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/regulator/pwm-regulator.c | 38 ++++++++++----------------------------
> >  1 file changed, 10 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> > index 524b43f..bf033fd 100644
> > --- a/drivers/regulator/pwm-regulator.c
> > +++ b/drivers/regulator/pwm-regulator.c
> > @@ -59,16 +59,14 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
> >  					 unsigned selector)
> >  {
> >  	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
> > -	struct pwm_args pargs;
> > -	int dutycycle;
> > +	struct pwm_state pstate;
> >  	int ret;
> >  
> > -	pwm_get_args(drvdata->pwm, &pargs);
> > +	pwm_prepare_new_state(drvdata->pwm, &pstate);
> > +	pwm_set_relative_duty_cycle(&pstate,
> > +			drvdata->duty_cycle_table[selector].dutycycle, 100);
> >  
> > -	dutycycle = (pargs.period *
> > -		    drvdata->duty_cycle_table[selector].dutycycle) / 100;
> > -
> > -	ret = pwm_config(drvdata->pwm, dutycycle, pargs.period);
> > +	ret = pwm_apply_state(drvdata->pwm, &pstate);
> >  	if (ret) {
> >  		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
> >  		return ret;
> > @@ -126,34 +124,18 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
> >  {
> >  	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
> >  	unsigned int ramp_delay = rdev->constraints->ramp_delay;
> > -	struct pwm_args pargs;
> >  	unsigned int req_diff = min_uV - rdev->constraints->min_uV;
> > +	struct pwm_state pstate;
> >  	unsigned int diff;
> > -	unsigned int duty_pulse;
> > -	u64 req_period;
> > -	u32 rem;
> >  	int ret;
> >  
> > -	pwm_get_args(drvdata->pwm, &pargs);
> > +	pwm_prepare_new_state(drvdata->pwm, &pstate);
> >  	diff = rdev->constraints->max_uV - rdev->constraints->min_uV;
> >  
> > -	/* First try to find out if we get the iduty cycle time which is
> > -	 * factor of PWM period time. If (request_diff_to_min * pwm_period)
> > -	 * is perfect divided by voltage_range_diff then it is possible to
> > -	 * get duty cycle time which is factor of PWM period. This will help
> > -	 * to get output voltage nearer to requested value as there is no
> > -	 * calculation loss.
> > -	 */
> > -	req_period = req_diff * pargs.period;
> > -	div_u64_rem(req_period, diff, &rem);
> > -	if (!rem) {
> > -		do_div(req_period, diff);
> > -		duty_pulse = (unsigned int)req_period;
> > -	} else {
> > -		duty_pulse = (pargs.period / 100) * ((req_diff * 100) / diff);
> > -	}
> > +	/* We pass diff as the scale to get a uV precision. */
> > +	pwm_set_relative_duty_cycle(&pstate, req_diff, diff);  
> 
> Notably, you're dropping much of Laxman's commit fd786fb0276a ("regulator:
> pwm: Try to avoid voltage error in duty cycle calculation"), but I
> believe the DIV_ROUND_CLOSEST_ULL() in pwm_set_relative_duty_cycle()
> solves his problem better.

Oops, forgot to comment on that in the commit message. Indeed, the use
of pwm_set_relative_duty_cycle() solves the problem Laxman was seeing.

> 
> >  
> > -	ret = pwm_config(drvdata->pwm, duty_pulse, pargs.period);
> > +	ret = pwm_apply_state(drvdata->pwm, &pstate);
> >  	if (ret) {
> >  		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
> >  		return ret;  
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1414672

On Saturday 04 June 2016 11:58 AM, Boris Brezillon wrote:
> On Fri, 3 Jun 2016 13:50:28 -0700
> Brian Norris <briannorris@chromium.org> wrote:
>
>> + Laxman
>>
>> Hi,
>>
>> On Fri, Jun 03, 2016 at 10:23:08AM +0200, Boris Brezillon wrote:
>>> -	 * calculation loss.
>>> -	 */
>>> -	req_period = req_diff * pargs.period;
>>> -	div_u64_rem(req_period, diff, &rem);
>>> -	if (!rem) {
>>> -		do_div(req_period, diff);
>>> -		duty_pulse = (unsigned int)req_period;
>>> -	} else {
>>> -		duty_pulse = (pargs.period / 100) * ((req_diff * 100) / diff);
>>> -	}
>>> +	/* We pass diff as the scale to get a uV precision. */
>>> +	pwm_set_relative_duty_cycle(&pstate, req_diff, diff);
>> Notably, you're dropping much of Laxman's commit fd786fb0276a ("regulator:
>> pwm: Try to avoid voltage error in duty cycle calculation"), but I
>> believe the DIV_ROUND_CLOSEST_ULL() in pwm_set_relative_duty_cycle()
>> solves his problem better.
> Oops, forgot to comment on that in the commit message. Indeed, the use
> of pwm_set_relative_duty_cycle() solves the problem Laxman was seeing.
>
Yaah, the issue which I was seeing and had fix will be resolved with 
this also.
I wanted to do req_diff * period first before any scaling/division.

Function pwm_set_relative_duty_cycle() does the same,  and hence it is fine.

state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)val * state->period,
+                          scale);



Acked-by: Laxman Dewangan <ldewangan@nvidia.com>



Thanks,
Laxman

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web