[2/4] ARM: S3C24XX: rx1950: make use of atomic PWM API

Message ID 20181026184157.16371-2-u.kleine-koenig@pengutronix.de
State New
Headers show
Series
  • [1/4] pwm: Add new helper to initialize a pwm_state variable with defaults
Related show

Commit Message

Uwe Kleine-König Oct. 26, 2018, 6:41 p.m.
The legacy PWM API should be removed in the long run, so convert a user
to the atomic PWM API.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Krzysztof Kozlowski Oct. 29, 2018, 10:46 a.m. | #1
On Fri, 26 Oct 2018 at 20:54, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> The legacy PWM API should be removed in the long run, so convert a user
> to the atomic PWM API.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
I see this depends on first patch so I assume this will go through pwm tree.

Best regards,
Krzysztof
Uwe Kleine-König Nov. 14, 2018, 9:15 a.m. | #2
On Mon, Oct 29, 2018 at 11:46:15AM +0100, Krzysztof Kozlowski wrote:
> On Fri, 26 Oct 2018 at 20:54, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > The legacy PWM API should be removed in the long run, so convert a user
> > to the atomic PWM API.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> I see this depends on first patch so I assume this will go through pwm tree.

Right, I assume so, too. Unfortunately I didn't hear anything back from
Thierry about this because there seems to be a mail reception problem on
his end.

@Thierry: Could you find the problem? It would be great to get some
feedback from you regarding these patches. There are more pwm users that
could benefit from this change, but I first want some feedback about
this before investing more time here.

Best regards
Uwe
Thierry Reding Nov. 14, 2018, 12:08 p.m. | #3
On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote:
> The legacy PWM API should be removed in the long run, so convert a user
> to the atomic PWM API.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
> index 7f5a18fa305b..5c4459f9a5f7 100644
> --- a/arch/arm/mach-s3c24xx/mach-rx1950.c
> +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
> @@ -375,6 +375,7 @@ static struct pwm_lookup rx1950_pwm_lookup[] = {
>  		   PWM_POLARITY_NORMAL),
>  };
>  
> +static struct pwm_state lcd_pwm_state;

You shouldn't need this. The whole point of the atomic API is that the
PWM carries its state, so the proper way to make a change is to query
the current state, make any required modifications and then apply the
new state.

>  static struct pwm_device *lcd_pwm;
>  
>  static void rx1950_lcd_power(int enable)
> @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
>  
>  		/* GPB1->OUTPUT, GPB1->0 */
>  		gpio_direction_output(S3C2410_GPB(1), 0);
> -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> -		pwm_disable(lcd_pwm);
> +		lcd_pwm_state.duty_cycle = 0;
> +		lcd_pwm_state.enabled = false;
> +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);

The correct way to do this would be:

	struct pwm_state state;

	pwm_get_state(lcd_pwm, &state);
	state.enabled = false;
	state.duty_cycle = 0;
	pwm_apply_state(lcd_pwm, &state);

>  
>  		/* GPC0->0, GPC10->0 */
>  		gpio_direction_output(S3C2410_GPC(0), 0);
>  		gpio_direction_output(S3C2410_GPC(10), 0);
>  	} else {
> -		pwm_config(lcd_pwm, LCD_PWM_DUTY, LCD_PWM_PERIOD);
> -		pwm_enable(lcd_pwm);
> +		lcd_pwm_state.duty_cycle = LCD_PWM_DUTY;
> +		lcd_pwm_state.enabled = true;
> +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);

Similarily here.

>  
>  		gpio_direction_output(S3C2410_GPC(0), 1);
>  		gpio_direction_output(S3C2410_GPC(5), 1);
> @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
>  		return PTR_ERR(lcd_pwm);
>  	}
>  
> -	/*
> -	 * FIXME: pwm_apply_args() should be removed when switching to
> -	 * the atomic PWM API.
> -	 */
> -	pwm_apply_args(lcd_pwm);
> +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> +	lcd_pwm_state.period = LCD_PWM_PERIOD;

This is wrong, though it's probably because the comment is also
confusing. There should be nothing wrong with using pwm_apply_args() in
this case.

While at it, I think the conversion should also be include replacing the
call to pwm_request() by pwm_get(). There's already a PWM lookup table
in the RX1950 board code, so pwm_get() would be able to use that. Note
that for some reason that table contains a period that is different from
LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
the information other than duty cycle should be coming from either DT or
a lookup table.

Thierry
Uwe Kleine-König Nov. 15, 2018, 8:58 a.m. | #4
Hello Thierry,

On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote:
> On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote:
> > +static struct pwm_state lcd_pwm_state;
> 
> You shouldn't need this. The whole point of the atomic API is that the
> PWM carries its state, so the proper way to make a change is to query
> the current state, make any required modifications and then apply the
> new state.

I thought the whole point of the atomic API is to be atomic :-)

> >  static struct pwm_device *lcd_pwm;
> >  
> >  static void rx1950_lcd_power(int enable)
> > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
> >  
> >  		/* GPB1->OUTPUT, GPB1->0 */
> >  		gpio_direction_output(S3C2410_GPB(1), 0);
> > -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> > -		pwm_disable(lcd_pwm);
> > +		lcd_pwm_state.duty_cycle = 0;
> > +		lcd_pwm_state.enabled = false;
> > +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
> 
> The correct way to do this would be:
> 
> 	struct pwm_state state;
> 
> 	pwm_get_state(lcd_pwm, &state);
> 	state.enabled = false;
> 	state.duty_cycle = 0;
> 	pwm_apply_state(lcd_pwm, &state);

The difference here is that with my approach the rx1950 driver caches
the intended pwm_state saving a few cycles for repeatedly copying the
pwm cache to a stack variable at the cost of some memory. I like my
approach a little better but I'm not willing to argue about that one and
can give in.

> >  		gpio_direction_output(S3C2410_GPC(0), 1);
> >  		gpio_direction_output(S3C2410_GPC(5), 1);
> > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
> >  		return PTR_ERR(lcd_pwm);
> >  	}
> >  
> > -	/*
> > -	 * FIXME: pwm_apply_args() should be removed when switching to
> > -	 * the atomic PWM API.
> > -	 */
> > -	pwm_apply_args(lcd_pwm);
> > +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> > +	lcd_pwm_state.period = LCD_PWM_PERIOD;
> 
> This is wrong, though it's probably because the comment is also
> confusing. There should be nothing wrong with using pwm_apply_args() in
> this case.

In my eyes it is wrong because it results in a call to the backend
driver's apply callback to get the default setting just to fix the
configuration and apply that in the code to follow.

> While at it, I think the conversion should also be include replacing the
> call to pwm_request() by pwm_get(). There's already a PWM lookup table
> in the RX1950 board code, so pwm_get() would be able to use that. Note
> that for some reason that table contains a period that is different from
> LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
> the information other than duty cycle should be coming from either DT or
> a lookup table.

Yeah, I noticed that, too. It's not entirely clear to me how to do that
yet. So I thought to care about getting rid of the legacy usage of
pwm_config and pwm_apply_args first.

Best regards
Uwe
Thierry Reding Nov. 15, 2018, 4:15 p.m. | #5
On Thu, Nov 15, 2018 at 09:58:08AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Nov 14, 2018 at 01:08:14PM +0100, Thierry Reding wrote:
> > On Fri, Oct 26, 2018 at 08:41:56PM +0200, Uwe Kleine-König wrote:
> > > +static struct pwm_state lcd_pwm_state;
> > 
> > You shouldn't need this. The whole point of the atomic API is that the
> > PWM carries its state, so the proper way to make a change is to query
> > the current state, make any required modifications and then apply the
> > new state.
> 
> I thought the whole point of the atomic API is to be atomic :-)
> 
> > >  static struct pwm_device *lcd_pwm;
> > >  
> > >  static void rx1950_lcd_power(int enable)
> > > @@ -428,15 +429,17 @@ static void rx1950_lcd_power(int enable)
> > >  
> > >  		/* GPB1->OUTPUT, GPB1->0 */
> > >  		gpio_direction_output(S3C2410_GPB(1), 0);
> > > -		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
> > > -		pwm_disable(lcd_pwm);
> > > +		lcd_pwm_state.duty_cycle = 0;
> > > +		lcd_pwm_state.enabled = false;
> > > +		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
> > 
> > The correct way to do this would be:
> > 
> > 	struct pwm_state state;
> > 
> > 	pwm_get_state(lcd_pwm, &state);
> > 	state.enabled = false;
> > 	state.duty_cycle = 0;
> > 	pwm_apply_state(lcd_pwm, &state);
> 
> The difference here is that with my approach the rx1950 driver caches
> the intended pwm_state saving a few cycles for repeatedly copying the
> pwm cache to a stack variable at the cost of some memory. I like my
> approach a little better but I'm not willing to argue about that one and
> can give in.

It's a redundant copy of the PWM's internal state and not guaranteed to
remain in sync with the PWM hardware state. You also require a global
variable, which is usually a bad idea. And we're not repeatedly copying
data. We occasionally do. These operations are typically only executed
once or twice per boot, so hardly anything that needs to be optimized
for speed.

> > >  		gpio_direction_output(S3C2410_GPC(0), 1);
> > >  		gpio_direction_output(S3C2410_GPC(5), 1);
> > > @@ -491,11 +494,8 @@ static int rx1950_backlight_init(struct device *dev)
> > >  		return PTR_ERR(lcd_pwm);
> > >  	}
> > >  
> > > -	/*
> > > -	 * FIXME: pwm_apply_args() should be removed when switching to
> > > -	 * the atomic PWM API.
> > > -	 */
> > > -	pwm_apply_args(lcd_pwm);
> > > +	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
> > > +	lcd_pwm_state.period = LCD_PWM_PERIOD;
> > 
> > This is wrong, though it's probably because the comment is also
> > confusing. There should be nothing wrong with using pwm_apply_args() in
> > this case.
> 
> In my eyes it is wrong because it results in a call to the backend
> driver's apply callback to get the default setting just to fix the
> configuration and apply that in the code to follow.

Okay, I see. But there's already pwm_init_state() which pretty much does
what you do here, right?

Thinking about it some more, I wonder if we shouldn't be more consistent
about state handling here. So the reason why pwm_apply_args() exists is
to program the PWM with a known state if the PWM doesn't support
hardware readout. Since most drivers don't support hardware readout, we
need this to at some specific point synchronize the hardware and
internal states.

In retrospect I'm not sure that's necessary, because there's more and
more evidence that we don't want to touch a PWM configuration until a
consumer explicitly says so. There's also the slight problem that the
pwm_apply_args() is really only necessary if the PWM driver doesn't
support hardware readout, but that's not something that consumers are
aware of or should worry about.

In light of that I think perhaps a better solution would be to basically
apply the PWM arguments to the internal state at request time. We can't
do it earlier because we don't know the arguments before the PWM is
requested. So I think at request time we could do something like this:

	if (chip->ops->get_state)
		chip->ops->get_state(chip, pwm, &pwm->state);
	else
		pwm_init_state(pwm, &pwm->state);

That way drivers always get either the current state or the "default"
state that was configured via platform-specific means (DT or lookup).

> > While at it, I think the conversion should also be include replacing the
> > call to pwm_request() by pwm_get(). There's already a PWM lookup table
> > in the RX1950 board code, so pwm_get() would be able to use that. Note
> > that for some reason that table contains a period that is different from
> > LCD_PWM_PERIOD, so I think that should also be addressed. Basically all
> > the information other than duty cycle should be coming from either DT or
> > a lookup table.
> 
> Yeah, I noticed that, too. It's not entirely clear to me how to do that
> yet. So I thought to care about getting rid of the legacy usage of
> pwm_config and pwm_apply_args first.

So I think I got the backlight PWM confused with the LCD PWM. The former
is what we have in the PWM lookup table and which controls the backlight
brightness. The latter is what we don't have in the table and which is
requested using the legacy pwm_request() function. That also explains
why there's a difference between the period in the PWM lookup table for
the backlight PWM and the LCD_PWM_PERIOD macro.

Adding support for pwm_get() should be as simple ad adding an entry to
rx1950_pwm_lookup[], such as this:

	PWM_LOOKUP("samsung-pwm", 1, "pwm-backlight.0", "lcd", 192960,
		   PWM_POLARITY_NORMAL),

Then in rx1950_backlight_init(), which is passed the device structure
that represents the backlight (i.e. "pwm-backlight.0"), pwm_get() can
be called like this:

	lcd_pwm = pwm_get(dev, "lcd");

The matching code should then be able to find the second entry as the
best match and return the correct one.

Thierry

Patch

diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
index 7f5a18fa305b..5c4459f9a5f7 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -375,6 +375,7 @@  static struct pwm_lookup rx1950_pwm_lookup[] = {
 		   PWM_POLARITY_NORMAL),
 };
 
+static struct pwm_state lcd_pwm_state;
 static struct pwm_device *lcd_pwm;
 
 static void rx1950_lcd_power(int enable)
@@ -428,15 +429,17 @@  static void rx1950_lcd_power(int enable)
 
 		/* GPB1->OUTPUT, GPB1->0 */
 		gpio_direction_output(S3C2410_GPB(1), 0);
-		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
-		pwm_disable(lcd_pwm);
+		lcd_pwm_state.duty_cycle = 0;
+		lcd_pwm_state.enabled = false;
+		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
 
 		/* GPC0->0, GPC10->0 */
 		gpio_direction_output(S3C2410_GPC(0), 0);
 		gpio_direction_output(S3C2410_GPC(10), 0);
 	} else {
-		pwm_config(lcd_pwm, LCD_PWM_DUTY, LCD_PWM_PERIOD);
-		pwm_enable(lcd_pwm);
+		lcd_pwm_state.duty_cycle = LCD_PWM_DUTY;
+		lcd_pwm_state.enabled = true;
+		pwm_apply_state(lcd_pwm, &lcd_pwm_state);
 
 		gpio_direction_output(S3C2410_GPC(0), 1);
 		gpio_direction_output(S3C2410_GPC(5), 1);
@@ -491,11 +494,8 @@  static int rx1950_backlight_init(struct device *dev)
 		return PTR_ERR(lcd_pwm);
 	}
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to
-	 * the atomic PWM API.
-	 */
-	pwm_apply_args(lcd_pwm);
+	pwm_get_state_default(lcd_pwm, &lcd_pwm_state);
+	lcd_pwm_state.period = LCD_PWM_PERIOD;
 
 	rx1950_lcd_power(1);
 	rx1950_bl_power(1);