pwm: Add new helper to initialize a pwm_state variable with defaults

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

---
 arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Quoted 4 lines · Uwe Kleine-K?nig

The legacy PWM API should be removed in the long run, so convert a user to the atomic PWM API.

Quoted 4 lines

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by Krzysztof Kozlowski <[email protected]>

I see this depends on first patch so I assume this will go through pwm tree.

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

---
 arch/arm/mach-s3c24xx/mach-rx1950.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

arch/arm/mach-s3c24xx/mach-rx1950.c +0 -0

index 7f5a18fa305b..5c4459f9a5f7 100644

arch/arm/mach-s3c24xx/mach-rx1950.c +0 -0

 		gpio_direction_output(S3C2410_GPC(0), 1);
 		gpio_direction_output(S3C2410_GPC(5), 1);

Quoted 1 line · Uwe Kleine-K?nig

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

arch/arm/mach-s3c24xx/mach-rx1950.c static void rx1950_lcd_power(int enable) · line 434

Quoted 12 lines

static struct pwm_device *lcd_pwm;

static void rx1950_lcd_power(int enable)

arch/arm/mach-s3c24xx/mach-rx1950.c +3 -2

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

Quoted 6 lines

arch/arm/mach-s3c24xx/mach-rx1950.c +0 -0

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

arch/arm/mach-s3c24xx/mach-rx1950.c static int rx1950_backlight_init(struct device *dev) · line 498

Quoted 7 lines

arch/arm/mach-s3c24xx/mach-rx1950.c +2 -5

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

Hello Thierry,

Quoted 7 lines · Thierry Reding

Quoted 1 line · Uwe Kleine-K?nig

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

Quoted 15 lines

arch/arm/mach-s3c24xx/mach-rx1950.c static void rx1950_lcd_power(int enable) · line 434

Quoted 12 lines

static struct pwm_device *lcd_pwm;

static void rx1950_lcd_power(int enable)

arch/arm/mach-s3c24xx/mach-rx1950.c +3 -2

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

Quoted 6 lines

	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.

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-s3c24xx/mach-rx1950.c +0 -0

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

Quoted 11 lines

arch/arm/mach-s3c24xx/mach-rx1950.c static int rx1950_backlight_init(struct device *dev) · line 498

Quoted 7 lines

arch/arm/mach-s3c24xx/mach-rx1950.c +2 -5

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

Quoted 7 lines

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.

Quoted 25 lines · Uwe Kleine-K?nig

Quoted 23 lines · Thierry Reding

arch/arm/mach-s3c24xx/mach-rx1950.c static void rx1950_lcd_power(int enable) · line 434

Quoted 13 lines · Uwe Kleine-K?nig

arch/arm/mach-s3c24xx/mach-rx1950.c +4 -2

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

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

Quoted 5 lines

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.

Quoted 18 lines

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-s3c24xx/mach-rx1950.c +0 -0

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

Quoted 11 lines

arch/arm/mach-s3c24xx/mach-rx1950.c static int rx1950_backlight_init(struct device *dev) · line 498

Quoted 7 lines

arch/arm/mach-s3c24xx/mach-rx1950.c +2 -5

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

Quoted 3 lines

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

Quoted 11 lines

Quoted 7 lines

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.

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

---
 drivers/bus/ts-nbus.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

drivers/bus/ts-nbus.c +7 -9

index 073fd9011154..51f5ed870ad1 100644
@@ -282,7 +282,7 @@ EXPORT_SYMBOL_GPL(ts_nbus_write);
 static int ts_nbus_probe(struct platform_device *pdev)
 {
 	struct pwm_device *pwm;
-	struct pwm_args pargs;
+	struct pwm_state pstate;
 	struct device *dev = &pdev->dev;
 	struct ts_nbus *ts_nbus;
 	int ret;

@@ -305,25 +305,19 @@ static int ts_nbus_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pwm_get_args(pwm, &pargs);
-	if (!pargs.period) {
+	pwm_get_state_default(pwm, &pstate);
+	if (!pstate.period) {
 		dev_err(&pdev->dev, "invalid PWM period\n");
 		return -EINVAL;
 	}
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to
-	 * the atomic PWM API.
-	 */
-	pwm_apply_args(pwm);
-	ret = pwm_config(pwm, pargs.period, pargs.period);
+	/* We can start the FPGA and populate the peripherals now. */
+	pstate.enabled = true;
+	pstate.duty_cycle = pstate.period;
+	ret = pwm_apply_state(pwm, &pstate);

There is nothing specific for the i.MX28 SoC in the ts-nbus driver and it compiles just fine with an ARM/s3c24xx configuration.

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 drivers/bus/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This helps to convert drivers from the legacy API to pwm_apply_state without having to make the aware of the configured polarity (and in some cases even period).

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Quoted 4 lines · Uwe Kleine-K?nig

This helps to convert drivers from the legacy API to pwm_apply_state without having to make the aware of the configured polarity (and in some cases even period).

Quoted 4 lines

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

You're going to have to at least Cc me on patches 2 & 3 so that I can see how you intend this to be used.

Quoted 5 lines · Thierry Reding

Quoted 4 lines · Uwe Kleine-K?nig

This helps to convert drivers from the legacy API to pwm_apply_state without having to make the aware of the configured polarity (and in some cases even period).

Quoted 7 lines

Quoted 4 lines

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

You're going to have to at least Cc me on patches 2 & 3 so that I can see how you intend this to be used.

you were in To: for all four patches:

patch 1: https://www.spinics.net/lists/linux-pwm/msg08178.html
patch 2: https://www.spinics.net/lists/linux-pwm/msg08179.html
patch 3: https://www.spinics.net/lists/linux-pwm/msg08180.html
patch 4: https://www.spinics.net/lists/linux-pwm/msg08181.html

This helps to convert drivers from the legacy API to pwm_apply_state without having to make the aware of the configured polarity (and in some cases even period).

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Quoted 8 lines · Uwe Kleine-K?nig

This helps to convert drivers from the legacy API to pwm_apply_state without having to make the aware of the configured polarity (and in some cases even period).

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

So if I understand your intention correctly here you want consumers to be able to get at the default state, which would be the state as it was "configured" in DT or the PWM lookup table so that you can modify that state (well, really just setting the duty-cycle) before you actually send the state to the hardware.

So the difference to pwm_apply_args() is that you don't want the driver to specifically program a duty cycle of 0 before setting the actual duty cycle that you want to set.

Is that about right?

Isn't that exactly what pwm_init_state() already does? pwm-backlight uses it for exactly the same purpose that you seem to be using it in RX1950 backlight control.

Hello Thierry,

Quoted 16 lines · Thierry Reding

Quoted 8 lines · Uwe Kleine-K?nig

This helps to convert drivers from the legacy API to pwm_apply_state without having to make the aware of the configured polarity (and in some cases even period).

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

So if I understand your intention correctly here you want consumers to be able to get at the default state, which would be the state as it was "configured" in DT or the PWM lookup table so that you can modify that state (well, really just setting the duty-cycle) before you actually send the state to the hardware.

Quoted 5 lines

So the difference to pwm_apply_args() is that you don't want the driver to specifically program a duty cycle of 0 before setting the actual duty cycle that you want to set.

Is that about right?

Yes, and a resulting advantage is that the apply callback is only called once.

Quoted 3 lines

Isn't that exactly what pwm_init_state() already does? pwm-backlight uses it for exactly the same purpose that you seem to be using it in RX1950 backlight control.

I didn't use pwm_init_state because that doesn't give a helpful value for state->enabled. (I think it is: if the driver provides a get_state callback it is whatever that one set, otherwise it's off.)

Also the name pwm_init_state isn't that good IMHO. Just from the name I'd expect it to be something like

	memset(state, 0, sizeof(&state));

instead of something more informed that depends on hardware state and/or data provided by a device tree.

Quoted 30 lines · Uwe Kleine-K?nig

Quoted 16 lines · Thierry Reding

Quoted 8 lines · Uwe Kleine-K?nig

This helps to convert drivers from the legacy API to pwm_apply_state without having to make the aware of the configured polarity (and in some cases even period).

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

So if I understand your intention correctly here you want consumers to be able to get at the default state, which would be the state as it was "configured" in DT or the PWM lookup table so that you can modify that state (well, really just setting the duty-cycle) before you actually send the state to the hardware.

Quoted 5 lines

So the difference to pwm_apply_args() is that you don't want the driver to specifically program a duty cycle of 0 before setting the actual duty cycle that you want to set.

Is that about right?

Yes, and a resulting advantage is that the apply callback is only called once.

Quoted 3 lines

Isn't that exactly what pwm_init_state() already does? pwm-backlight uses it for exactly the same purpose that you seem to be using it in RX1950 backlight control.

Quoted 3 lines

I didn't use pwm_init_state because that doesn't give a helpful value for state->enabled. (I think it is: if the driver provides a get_state callback it is whatever that one set, otherwise it's off.)

That's correct. It's pretty much the same thing that you have, except you explicitly set enable = false. But that's not really important because consumers are supposed to explicitly override that anyway.

Quoted 4 lines

Also the name pwm_init_state isn't that good IMHO. Just from the name I'd expect it to be something like

Quoted 4 lines

	memset(state, 0, sizeof(&state));

instead of something more informed that depends on hardware state and/or data provided by a device tree.

Be that as it may, the kerneldoc is pretty explicit about what it does and so is the code. Not liking the name shouldn't be an excuse for duplicating functionality.

Hello Thierry,

Quoted 31 lines · Thierry Reding

Quoted 30 lines · Uwe Kleine-K?nig

Quoted 16 lines · Thierry Reding

Quoted 8 lines · Uwe Kleine-K?nig

This helps to convert drivers from the legacy API to pwm_apply_state without having to make the aware of the configured polarity (and in some cases even period).

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

So if I understand your intention correctly here you want consumers to be able to get at the default state, which would be the state as it was "configured" in DT or the PWM lookup table so that you can modify that state (well, really just setting the duty-cycle) before you actually send the state to the hardware.

Quoted 5 lines

So the difference to pwm_apply_args() is that you don't want the driver to specifically program a duty cycle of 0 before setting the actual duty cycle that you want to set.

Is that about right?

Yes, and a resulting advantage is that the apply callback is only called once.

Quoted 3 lines

Isn't that exactly what pwm_init_state() already does? pwm-backlight uses it for exactly the same purpose that you seem to be using it in RX1950 backlight control.

Quoted 7 lines

Quoted 3 lines

I didn't use pwm_init_state because that doesn't give a helpful value for state->enabled. (I think it is: if the driver provides a get_state callback it is whatever that one set, otherwise it's off.)

That's correct. It's pretty much the same thing that you have, except you explicitly set enable = false. But that's not really important because consumers are supposed to explicitly override that anyway.

If we hardcode to enable=false the call to pwm_get_state could be dropped, too. Then the two functions are really semantically identical.

Quoted 9 lines

Quoted 8 lines

Also the name pwm_init_state isn't that good IMHO. Just from the name I'd expect it to be something like

	memset(state, 0, sizeof(&state));

instead of something more informed that depends on hardware state and/or data provided by a device tree.

Quoted 3 lines

Be that as it may, the kerneldoc is pretty explicit about what it does and so is the code. Not liking the name shouldn't be an excuse for duplicating functionality.

With your suggestion to move the initialisation of ->state into the core, maybe this function can go away from the public API such that the naming doesn't really matter (and can be adapted easily).