pwm: add support for atomic update

Some PWM drivers are testing the PWMF_ENABLED flag. Create an helper function to hide the logic behind enabled test.
This will allow us to smoothly move from the current approach to an atomic PWM update approach.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/pwm/core.c            |  4 ++--
 drivers/pwm/pwm-atmel-tcb.c   |  2 +-
 drivers/pwm/pwm-atmel.c       |  6 +++---
 drivers/pwm/pwm-bcm-kona.c    |  4 ++--
 drivers/pwm/pwm-ep93xx.c      |  4 ++--
 drivers/pwm/pwm-imx.c         |  2 +-
 drivers/pwm/pwm-mxs.c         |  4 ++--
 drivers/pwm/pwm-renesas-tpu.c |  2 +-
 drivers/pwm/pwm-tegra.c       |  6 +++---
 drivers/pwm/pwm-tiecap.c      | 10 +++++-----
 drivers/pwm/pwm-tiehrpwm.c    |  6 +++---
 drivers/pwm/sysfs.c           |  2 +-
 include/linux/pwm.h           |  5 +++++
 13 files changed, 31 insertions(+), 26 deletions(-)

The pwm argument is not modified in pwm_get helpers, make it a const argument so that they can be used from the sysfs functions.

Signed-off-by Boris Brezillon <[email protected]>

---
 include/linux/pwm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Some drivers are directly accessing the ->polarity field in pwm_device. Add an helper to retrieve the current polarity so that we can easily move this field elsewhere (required to support atomic update).

Signed-off-by Boris Brezillon <[email protected]>

---
 include/linux/pwm.h | 5 +++++
 1 file changed, 5 insertions(+)

Use the pwm_get_xxx helpers instead of directly accessing the fields in pwm_device. This will allow us to smoothly move to the atomic update approach.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/pwm/pwm-atmel.c    |  2 +-
 drivers/pwm/pwm-bcm-kona.c |  3 ++-
 drivers/pwm/pwm-imx.c      |  3 ++-
 drivers/pwm/pwm-rockchip.c |  2 +-
 drivers/pwm/sysfs.c        | 11 ++++++-----
 5 files changed, 12 insertions(+), 9 deletions(-)

When requested by a user, the PWM is assigned a default period and polarity extracted from the DT, the platform data or statically set by the driver. Those default values are currently stored in the period and polarity fields of the pwm_device struct, but they will be stored somewhere else once we have introduced the architecture allowing for hardware state retrieval.

The pwm_set_default_polarity and pwm_set_default_period should only be used by PWM drivers or the PWM core infrastructure to specify the default period and polarity values.

Quoted 7 lines · Boris Brezillon

When requested by a user, the PWM is assigned a default period and polarity extracted from the DT, the platform data or statically set by the driver. Those default values are currently stored in the period and polarity fields of the pwm_device struct, but they will be stored somewhere else once we have introduced the architecture allowing for hardware state retrieval.

Quoted 3 lines

The pwm_set_default_polarity and pwm_set_default_period should only be used by PWM drivers or the PWM core infrastructure to specify the default period and polarity values.

Would it make sense to put the prototypes of pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h then?

Quoted 8 lines · On Thu, 2 Jul 2015 08:44:45 +0200 Uwe Kleine-K?nig…

Quoted 7 lines · Boris Brezillon

When requested by a user, the PWM is assigned a default period and polarity extracted from the DT, the platform data or statically set by the driver. Those default values are currently stored in the period and polarity fields of the pwm_device struct, but they will be stored somewhere else once we have introduced the architecture allowing for hardware state retrieval.

Quoted 7 lines

Quoted 3 lines

The pwm_set_default_polarity and pwm_set_default_period should only be used by PWM drivers or the PWM core infrastructure to specify the default period and polarity values.

Would it make sense to put the prototypes of pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h then?

Yes, definitely. I was thinking about moving those functions/prototypes into include/linux/pwm-provider.h, but I'm fine with drivers/pwm/pwm-private.h too.

Thierry, any opinion ?

Quoted 19 lines · Boris Brezillon

Quoted 8 lines · On Thu, 2 Jul 2015 08:44:45 +0200 Uwe Kleine-K?nig…

Quoted 7 lines · Boris Brezillon

When requested by a user, the PWM is assigned a default period and polarity extracted from the DT, the platform data or statically set by the driver. Those default values are currently stored in the period and polarity fields of the pwm_device struct, but they will be stored somewhere else once we have introduced the architecture allowing for hardware state retrieval.

Quoted 7 lines

Quoted 3 lines

The pwm_set_default_polarity and pwm_set_default_period should only be used by PWM drivers or the PWM core infrastructure to specify the default period and polarity values.

Would it make sense to put the prototypes of pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h then?

Quoted 5 lines

Yes, definitely. I was thinking about moving those functions/prototypes into include/linux/pwm-provider.h, but I'm fine with drivers/pwm/pwm-private.h too.

Thierry, any opinion ?

I'm not sure I see the need for this. If they are the default values and drivers have no need to change them, then storing them in the regular period and polarity fields seems just fine (they'll be propagated into new state objects as they get created).

And if the driver has a need to change them, then why would it ever care about the default values?

Hi Thierry,

Quoted 26 lines · On Mon, 20 Jul 2015 10:03:14 +0200 Thierry Reding <[email protected]>…

Quoted 19 lines · Boris Brezillon

Quoted 8 lines · On Thu, 2 Jul 2015 08:44:45 +0200 Uwe Kleine-K?nig…

Quoted 7 lines · Boris Brezillon

When requested by a user, the PWM is assigned a default period and polarity extracted from the DT, the platform data or statically set by the driver. Those default values are currently stored in the period and polarity fields of the pwm_device struct, but they will be stored somewhere else once we have introduced the architecture allowing for hardware state retrieval.

Quoted 7 lines

Quoted 3 lines

The pwm_set_default_polarity and pwm_set_default_period should only be used by PWM drivers or the PWM core infrastructure to specify the default period and polarity values.

Would it make sense to put the prototypes of pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h then?

Quoted 5 lines

Yes, definitely. I was thinking about moving those functions/prototypes into include/linux/pwm-provider.h, but I'm fine with drivers/pwm/pwm-private.h too.

Thierry, any opinion ?

Quoted 7 lines

I'm not sure I see the need for this. If they are the default values and drivers have no need to change them, then storing them in the regular period and polarity fields seems just fine (they'll be propagated into new state objects as they get created).

And if the driver has a need to change them, then why would it ever care about the default values?

Because the period is often directly extracted from the DT, and this extracted period may not match the one configured by the bootloader.

If the driver wants to display the current status without changing the PWM state, then the driver will use the current state. ITOH, if it has to apply a new config, the driver will use the default period value (extracted from the DT) and change the duty-cycle depending on its needs.
This is the case we have with the pwm-regulator driver: we want to display the initial voltage value without changing the PWM config, and when someone decides to change the voltage, we want to use the default period instead of keeping the one configured by the bootloader.

Quoted 42 lines · Boris Brezillon

Hi Thierry,

Quoted 26 lines · On Mon, 20 Jul 2015 10:03:14 +0200 Thierry Reding <[email protected]>…

Quoted 19 lines · Boris Brezillon

Quoted 8 lines · On Thu, 2 Jul 2015 08:44:45 +0200 Uwe Kleine-K?nig…

Quoted 7 lines · Boris Brezillon

When requested by a user, the PWM is assigned a default period and polarity extracted from the DT, the platform data or statically set by the driver. Those default values are currently stored in the period and polarity fields of the pwm_device struct, but they will be stored somewhere else once we have introduced the architecture allowing for hardware state retrieval.

Quoted 7 lines

Quoted 3 lines

The pwm_set_default_polarity and pwm_set_default_period should only be used by PWM drivers or the PWM core infrastructure to specify the default period and polarity values.

Would it make sense to put the prototypes of pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h then?

Quoted 5 lines

Yes, definitely. I was thinking about moving those functions/prototypes into include/linux/pwm-provider.h, but I'm fine with drivers/pwm/pwm-private.h too.

Thierry, any opinion ?

Quoted 7 lines

I'm not sure I see the need for this. If they are the default values and drivers have no need to change them, then storing them in the regular period and polarity fields seems just fine (they'll be propagated into new state objects as they get created).

And if the driver has a need to change them, then why would it ever care about the default values?

Because the period is often directly extracted from the DT, and this extracted period may not match the one configured by the bootloader.

Quoted 9 lines

If the driver wants to display the current status without changing the PWM state, then the driver will use the current state. ITOH, if it has to apply a new config, the driver will use the default period value (extracted from the DT) and change the duty-cycle depending on its needs.
This is the case we have with the pwm-regulator driver: we want to display the initial voltage value without changing the PWM config, and when someone decides to change the voltage, we want to use the default period instead of keeping the one configured by the bootloader.

Wouldn't it make more sense to postpone this until the introduction of the default state, then? That way we'd be getting a more consistent way of dealing with default vs. initial by looking only at state objects.

Ideally initial state should be the same as the default state. Except maybe for the duty-cycle, which won't be encoded in the default state anyway.

include/linux/pwm.h +0 -0

On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
[...]

Quoted 1 line

include/linux/pwm.h +0 -0

[...]

include/linux/pwm.h enum { · line 86

Quoted 5 lines

include/linux/pwm.h +5 -0

+struct pwm_state {
+	unsigned int		period; 	/* in nanoseconds */
+	unsigned int		duty_cycle;	/* in nanoseconds */
+	enum pwm_polarity	polarity;
+};

No need for the extra padding here.

Quoted 11 lines · On Mon, 20 Jul 2015 10:04:59 +0200 Thierry Reding <[email protected]>…

On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
[...]

Quoted 1 line

include/linux/pwm.h +0 -0

[...]

include/linux/pwm.h enum { · line 86

Quoted 5 lines

include/linux/pwm.h +5 -0

+struct pwm_state {
+	unsigned int		period; 	/* in nanoseconds */
+	unsigned int		duty_cycle;	/* in nanoseconds */
+	enum pwm_polarity	polarity;
+};

No need for the extra padding here.

What do you mean by "extra padding" ?
I just reused the indentation used in the pwm_device struct.

Would you prefer something like that ?

struct pwm_state {
unsigned int period; /* in nanoseconds */
unsigned int duty_cycle; /* in nanoseconds */
enum pwm_polarity polarity;
};

Quoted 15 lines · Boris Brezillon

Quoted 11 lines · On Mon, 20 Jul 2015 10:04:59 +0200 Thierry Reding <[email protected]>…

On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
[...]

Quoted 1 line

include/linux/pwm.h +0 -0

[...]

include/linux/pwm.h enum { · line 86

Quoted 5 lines

include/linux/pwm.h +5 -0

+struct pwm_state {
+	unsigned int		period; 	/* in nanoseconds */
+	unsigned int		duty_cycle;	/* in nanoseconds */
+	enum pwm_polarity	polarity;
+};

No need for the extra padding here.

Quoted 2 lines

What do you mean by "extra padding" ?
I just reused the indentation used in the pwm_device struct.

Yeah, I have a local patch to fix that up. I find it useless to pad things like this, and it has the downside that it will become totally inconsistent (or cause a lot of churn by reformatting) if ever you add a field that extends beyond the padding. Single spaces don't have any such drawbacks and, in my opinion, look just as good.

Quoted 8 lines

Would you prefer something like that ?

struct pwm_state {
unsigned int period; /* in nanoseconds */
unsigned int duty_cycle; /* in nanoseconds */
enum pwm_polarity polarity;
};

Yeah. I'd say even the comments would be more suited in a kerneldoc- style comment:

	/**
	 * struct pwm_state - state of a PWM channel
	 * @period: PWM period (in nanoseconds)
	 * @duty_cycle: PWM duty cycle (in nanoseconds)
	 * @polarity: PWM polarity
	 */
	struct pwm_state {
		unsigned int period;
		unsigned int duty_cycle;
		enum pwm_polarity polarity;
	};

This is something that users will need to deal with, so eventually somebody might look at this via some DocBook generated HTML or PDF.

Prepare the transition to PWM atomic update by moving the enabled/disabled state into the pwm_state struct. This way we can easily update the whole PWM state by copying the new state in the ->state field.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/pwm/core.c  | 15 ++++++++++++---
 include/linux/pwm.h |  6 +++---
 2 files changed, 15 insertions(+), 6 deletions(-)

drivers/pwm/core.c +0 -1

index a6bc8e6..3e830ce 100644
@@ -474,8 +474,15 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
  */
 int pwm_enable(struct pwm_device *pwm)
 {
-	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))

The PWM period will be set when calling pwm_config. Remove this useless call to pwm_set_period, which might mess up with the initial PWM state once we have added proper support for PWM init state retrieval.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/video/backlight/pwm_bl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/video/backlight/pwm_bl.c +0 -0

index ae498c1..fe5597c 100644

Quoted 14 lines · Boris Brezillon

The PWM period will be set when calling pwm_config. Remove this useless call to pwm_set_period, which might mess up with the initial PWM state once we have added proper support for PWM init state retrieval.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/video/backlight/pwm_bl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/video/backlight/pwm_bl.c +0 -0

index ae498c1..fe5597c 100644

drivers/video/backlight/pwm_bl.c static int pwm_backlight_probe(struct platform_device *pdev) · line 301

Quoted 10 lines

drivers/video/backlight/pwm_bl.c +1 -3

@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	 * via the PWM lookup table.
 	 */
 	pb->period = pwm_get_default_period(pb->pwm);
-	if (!pb->period && (data->pwm_period_ns > 0)) {
+	if (!pb->period && (data->pwm_period_ns > 0))
 		pb->period = data->pwm_period_ns;
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-	}
 
 	pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);

As far as I remember this line is there in order to pass in a period if the backlight driver is initialized from board setup files. In such a case there won't be an period associated with the PWM channel in the first place.

I think even with the introduction of a default period, we'd be missing out on the board setup case because there is no standard place where it is being set, so it must come from the platform data.

Quoted 31 lines · On Mon, 20 Jul 2015 10:16:00 +0200 Thierry Reding <[email protected]>…

Quoted 14 lines · Boris Brezillon

The PWM period will be set when calling pwm_config. Remove this useless call to pwm_set_period, which might mess up with the initial PWM state once we have added proper support for PWM init state retrieval.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/video/backlight/pwm_bl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/video/backlight/pwm_bl.c +0 -0

index ae498c1..fe5597c 100644

drivers/video/backlight/pwm_bl.c static int pwm_backlight_probe(struct platform_device *pdev) · line 301

Quoted 10 lines

drivers/video/backlight/pwm_bl.c +1 -3

@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	 * via the PWM lookup table.
 	 */
 	pb->period = pwm_get_default_period(pb->pwm);
-	if (!pb->period && (data->pwm_period_ns > 0)) {
+	if (!pb->period && (data->pwm_period_ns > 0))
 		pb->period = data->pwm_period_ns;
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-	}
 
 	pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);

As far as I remember this line is there in order to pass in a period if the backlight driver is initialized from board setup files. In such a case there won't be an period associated with the PWM channel in the first place.

Quoted 3 lines

I think even with the introduction of a default period, we'd be missing out on the board setup case because there is no standard place where it is being set, so it must come from the platform data.

AFAICT, we don't need to explicitly set the period when probing the backlight device, because it will be set next time we call pwm_config(), and since we're passing pb->period when calling pwm_config() everything should be fine.

Quoted 34 lines · Boris Brezillon

Quoted 31 lines · On Mon, 20 Jul 2015 10:16:00 +0200 Thierry Reding <[email protected]>…

Quoted 14 lines · Boris Brezillon

The PWM period will be set when calling pwm_config. Remove this useless call to pwm_set_period, which might mess up with the initial PWM state once we have added proper support for PWM init state retrieval.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/video/backlight/pwm_bl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/video/backlight/pwm_bl.c +0 -0

index ae498c1..fe5597c 100644

drivers/video/backlight/pwm_bl.c static int pwm_backlight_probe(struct platform_device *pdev) · line 301

Quoted 10 lines

drivers/video/backlight/pwm_bl.c +1 -3

@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	 * via the PWM lookup table.
 	 */
 	pb->period = pwm_get_default_period(pb->pwm);
-	if (!pb->period && (data->pwm_period_ns > 0)) {
+	if (!pb->period && (data->pwm_period_ns > 0))
 		pb->period = data->pwm_period_ns;
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-	}
 
 	pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);

As far as I remember this line is there in order to pass in a period if the backlight driver is initialized from board setup files. In such a case there won't be an period associated with the PWM channel in the first place.

Quoted 8 lines

Quoted 3 lines

I think even with the introduction of a default period, we'd be missing out on the board setup case because there is no standard place where it is being set, so it must come from the platform data.

AFAICT, we don't need to explicitly set the period when probing the backlight device, because it will be set next time we call pwm_config(), and since we're passing pb->period when calling pwm_config() everything should be fine.

Calling pwm_set_period() is still good for consistency. Consider for example what happens if after the driver were to call pwm_get_period(). It would return some more or less random value (likely 0 or whatever it had been set to by an earlier user).

Technically I think the most proper equivalent here would be to set the default state's period to data->pwm_period_ns, but I don't think that's proper to do. Perhaps since this is only relevant to boards where the backlight device is created from board setup code we don't have to care so much about messing up the initial state because either the board setup code has been carefully written to match what the bootloader set up, or because they don't match at all, in which case we don't have to worry anyway.

Quoted 44 lines · On Mon, 20 Jul 2015 10:36:50 +0200 Thierry Reding <[email protected]>…

Quoted 42 lines · Boris Brezillon

Quoted 31 lines · On Mon, 20 Jul 2015 10:16:00 +0200 Thierry Reding <[email protected]>…

Quoted 14 lines · Boris Brezillon

The PWM period will be set when calling pwm_config. Remove this useless call to pwm_set_period, which might mess up with the initial PWM state once we have added proper support for PWM init state retrieval.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/video/backlight/pwm_bl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/video/backlight/pwm_bl.c +0 -0

index ae498c1..fe5597c 100644

drivers/video/backlight/pwm_bl.c static int pwm_backlight_probe(struct platform_device *pdev) · line 301

Quoted 10 lines

drivers/video/backlight/pwm_bl.c +1 -3

@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	 * via the PWM lookup table.
 	 */
 	pb->period = pwm_get_default_period(pb->pwm);
-	if (!pb->period && (data->pwm_period_ns > 0)) {
+	if (!pb->period && (data->pwm_period_ns > 0))
 		pb->period = data->pwm_period_ns;
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-	}
 
 	pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);

As far as I remember this line is there in order to pass in a period if the backlight driver is initialized from board setup files. In such a case there won't be an period associated with the PWM channel in the first place.

Quoted 3 lines

I think even with the introduction of a default period, we'd be missing out on the board setup case because there is no standard place where it is being set, so it must come from the platform data.

AFAICT, we don't need to explicitly set the period when probing the backlight device, because it will be set next time we call pwm_config(), and since we're passing pb->period when calling pwm_config() everything should be fine.

Quoted 4 lines

Calling pwm_set_period() is still good for consistency. Consider for example what happens if after the driver were to call pwm_get_period(). It would return some more or less random value (likely 0 or whatever it had been set to by an earlier user).

Yes, that's true in general, but in this specific driver pwm_get_period() is never called, and the driver only relies on the pb->period value.

Quoted 8 lines

Technically I think the most proper equivalent here would be to set the default state's period to data->pwm_period_ns, but I don't think that's proper to do. Perhaps since this is only relevant to boards where the backlight device is created from board setup code we don't have to care so much about messing up the initial state because either the board setup code has been carefully written to match what the bootloader set up, or because they don't match at all, in which case we don't have to worry anyway.

IMHO, if we had to support default period values for non DT boards, the proper way would be to pass something in the PWM platform data and let the PWM driver (or PWM core) initialize the default PWM state. This way the PWM user could rely on the pwm_get_default_period() helper to extract the default period value.

Quoted 52 lines · Boris Brezillon

Quoted 48 lines · On Mon, 20 Jul 2015 10:36:50 +0200 Thierry Reding <[email protected]>…

Quoted 34 lines · Boris Brezillon

Quoted 31 lines · On Mon, 20 Jul 2015 10:16:00 +0200 Thierry Reding <[email protected]>…

Quoted 14 lines · Boris Brezillon

The PWM period will be set when calling pwm_config. Remove this useless call to pwm_set_period, which might mess up with the initial PWM state once we have added proper support for PWM init state retrieval.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/video/backlight/pwm_bl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/video/backlight/pwm_bl.c +0 -0

index ae498c1..fe5597c 100644

drivers/video/backlight/pwm_bl.c static int pwm_backlight_probe(struct platform_device *pdev) · line 301

Quoted 10 lines

drivers/video/backlight/pwm_bl.c +1 -3

@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	 * via the PWM lookup table.
 	 */
 	pb->period = pwm_get_default_period(pb->pwm);
-	if (!pb->period && (data->pwm_period_ns > 0)) {
+	if (!pb->period && (data->pwm_period_ns > 0))
 		pb->period = data->pwm_period_ns;
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-	}
 
 	pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);

As far as I remember this line is there in order to pass in a period if the backlight driver is initialized from board setup files. In such a case there won't be an period associated with the PWM channel in the first place.

Quoted 8 lines

Quoted 3 lines

I think even with the introduction of a default period, we'd be missing out on the board setup case because there is no standard place where it is being set, so it must come from the platform data.

AFAICT, we don't need to explicitly set the period when probing the backlight device, because it will be set next time we call pwm_config(), and since we're passing pb->period when calling pwm_config() everything should be fine.

Calling pwm_set_period() is still good for consistency. Consider for example what happens if after the driver were to call pwm_get_period(). It would return some more or less random value (likely 0 or whatever it had been set to by an earlier user).

Quoted 3 lines

Yes, that's true in general, but in this specific driver pwm_get_period() is never called, and the driver only relies on the pb->period value.

Perhaps that's something that should change. If the PWM core has all this infrastructure there should be no need for the backlight driver to keep it's own copy of that variable.

Quoted 53 lines · On Mon, 20 Jul 2015 11:10:04 +0200 Thierry Reding <[email protected]>…

Quoted 52 lines · Boris Brezillon

Quoted 48 lines · On Mon, 20 Jul 2015 10:36:50 +0200 Thierry Reding <[email protected]>…

Quoted 34 lines · Boris Brezillon

Quoted 31 lines · On Mon, 20 Jul 2015 10:16:00 +0200 Thierry Reding <[email protected]>…

Quoted 14 lines · Boris Brezillon

The PWM period will be set when calling pwm_config. Remove this useless call to pwm_set_period, which might mess up with the initial PWM state once we have added proper support for PWM init state retrieval.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/video/backlight/pwm_bl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/video/backlight/pwm_bl.c +0 -0

index ae498c1..fe5597c 100644

drivers/video/backlight/pwm_bl.c static int pwm_backlight_probe(struct platform_device *pdev) · line 301

Quoted 10 lines

drivers/video/backlight/pwm_bl.c +1 -3

@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	 * via the PWM lookup table.
 	 */
 	pb->period = pwm_get_default_period(pb->pwm);
-	if (!pb->period && (data->pwm_period_ns > 0)) {
+	if (!pb->period && (data->pwm_period_ns > 0))
 		pb->period = data->pwm_period_ns;
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-	}
 
 	pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);

As far as I remember this line is there in order to pass in a period if the backlight driver is initialized from board setup files. In such a case there won't be an period associated with the PWM channel in the first place.

Quoted 8 lines

Quoted 3 lines

I think even with the introduction of a default period, we'd be missing out on the board setup case because there is no standard place where it is being set, so it must come from the platform data.

AFAICT, we don't need to explicitly set the period when probing the backlight device, because it will be set next time we call pwm_config(), and since we're passing pb->period when calling pwm_config() everything should be fine.

Calling pwm_set_period() is still good for consistency. Consider for example what happens if after the driver were to call pwm_get_period(). It would return some more or less random value (likely 0 or whatever it had been set to by an earlier user).

Quoted 7 lines

Quoted 3 lines

Yes, that's true in general, but in this specific driver pwm_get_period() is never called, and the driver only relies on the pb->period value.

Perhaps that's something that should change. If the PWM core has all this infrastructure there should be no need for the backlight driver to keep it's own copy of that variable.

Yes, probably. In any case, I don't think we want PWM users to be able to mess up with the current or default PWM state, that's why I was planning on making the pwm_set_default_xxx helpers private to PWM drivers and core infrastructure.

Also note that if we keep this assignment it should at least be changed to a pwm_set_default_period() so that it does not override the current PWM state.

Add a ->init_state() function to the pwm_ops struct to let PWM drivers initialize the PWM state attached to a PWM device.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/pwm/core.c  | 3 +++
 include/linux/pwm.h | 2 ++
 2 files changed, 5 insertions(+)

drivers/pwm/core.c +3 -0

index 3e830ce..30631f5 100644
@@ -264,6 +264,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->hwpwm = i;
 		pwm_set_default_polarity(pwm, polarity);
 
+		if (chip->ops->init_state)
+			chip->ops->init_state(chip, pwm);
+
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
 

include/linux/pwm.h +0 -0

index 0f36a06..b47244a 100644

Quoted 23 lines · Boris Brezillon

Add a ->init_state() function to the pwm_ops struct to let PWM drivers initialize the PWM state attached to a PWM device.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/pwm/core.c  | 3 +++
 include/linux/pwm.h | 2 ++
 2 files changed, 5 insertions(+)

drivers/pwm/core.c +3 -0

index 3e830ce..30631f5 100644
@@ -264,6 +264,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->hwpwm = i;
 		pwm_set_default_polarity(pwm, polarity);
 
+		if (chip->ops->init_state)
+			chip->ops->init_state(chip, pwm);
+
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
 

include/linux/pwm.h struct pwm_ops { · line 181

Quoted 10 lines

include/linux/pwm.h +2 -0

index 0f36a06..b47244a 100644
@@ -177,6 +177,8 @@ struct pwm_ops {
 					  struct pwm_device *pwm);
 	void			(*disable)(struct pwm_chip *chip,
 					   struct pwm_device *pwm);
+	void			(*init_state)(struct pwm_chip *chip,
+					      struct pwm_device *pwm);

I think I'd call this reset_state. init has this connotation of setting a set of default values. For reset it's clearer in my opinion that it's resetting to the hardware state.

Add an ->apply() method to the pwm_ops struct to allow PWM drivers to implement atomic update.
This method will be prefered over the ->enable(), ->disable() and ->config() methods if available.

Add the pwm_get_state(), pwm_get_default_state() and pwm_apply_state() functions for PWM users to be able to use the atomic update feature.

Note that the pwm_apply_state() does not guarantee the atomicity of the update operation, it all depends on the availability and implementation of the ->apply() method.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/pwm/core.c  | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
 include/linux/pwm.h |  26 +++++++++++++
 2 files changed, 124 insertions(+), 12 deletions(-)

drivers/pwm/core.c +0 -0

index 30631f5..6dafd8e 100644

drivers/pwm/core.c +9 -0

+
+		state.period = period_ns;
+		state.duty_cycle = duty_ns;
+
+		err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
+	} else {
+		err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
+	}
+
 	if (err)
 		return err;
 

drivers/pwm/core.c +10 -4

+
+		state.polarity = polarity;
+		err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
+		if (!err)
+			pwm->state.polarity = polarity;
+
+		return err;
+	}
+
 	if (!pwm->chip->ops->set_polarity)
 		return -ENOSYS;
 

@@ -477,17 +499,27 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
  */
 int pwm_enable(struct pwm_device *pwm)
 {
-	if (pwm && !pwm_is_enabled(pwm)) {
-		int err;
+	int err;
 
-		err = pwm->chip->ops->enable(pwm->chip, pwm);
-		if (!err)

drivers/pwm/core.c +12 -2

+	} else {
+		err = pwm->chip->ops->enable(pwm->chip, pwm);
 	}
 
-	return pwm ? 0 : -EINVAL;
+	if (!err)
+		pwm->state.enabled = true;
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(pwm_enable);
 

@@ -497,13 +529,67 @@ EXPORT_SYMBOL_GPL(pwm_enable);
  */
 void pwm_disable(struct pwm_device *pwm)
 {
-	if (pwm && pwm_is_enabled(pwm)) {
+	if (!pwm || !pwm_is_enabled(pwm))
+		return;
+
+	if (pwm->chip->ops->apply) {
+		struct pwm_state state = pwm->state;
+

drivers/pwm/core.c +4 -0

+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
+{
+	int err = 0;
+

drivers/pwm/core.c +31 -0

+	} else {
+		/*
+		 * FIXME: restore the initial state in case of error.
+		 */
+		if (state->polarity != pwm->state.polarity) {
+			pwm_disable(pwm);
+			err = pwm_set_polarity(pwm, state->polarity);
+			if (err)
+				goto out;
+		}
+
+		if (state->period != pwm->state.period ||
+		    state->duty_cycle != pwm->state.duty_cycle) {
+			err = pwm_config(pwm, state->period, state->duty_cycle);
+			if (err)
+				goto out;
+		}
+
+		if (state->enabled != pwm->state.enabled) {
+			if (state->enabled)
+				err = pwm_enable(pwm);
+			else
+				pwm_disable(pwm);
+		}
+	}
+
+out:
+	return err;
+}
+EXPORT_SYMBOL_GPL(pwm_apply_state);
+
 static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
 {
 	struct pwm_chip *chip;

include/linux/pwm.h +0 -0

index b47244a..7e99679 100644

Quoted 23 lines · Boris Brezillon

Add an ->apply() method to the pwm_ops struct to allow PWM drivers to implement atomic update.
This method will be prefered over the ->enable(), ->disable() and ->config() methods if available.

Add the pwm_get_state(), pwm_get_default_state() and pwm_apply_state() functions for PWM users to be able to use the atomic update feature.

Note that the pwm_apply_state() does not guarantee the atomicity of the update operation, it all depends on the availability and implementation of the ->apply() method.

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/pwm/core.c  | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
 include/linux/pwm.h |  26 +++++++++++++
 2 files changed, 124 insertions(+), 12 deletions(-)

drivers/pwm/core.c +0 -0

index 30631f5..6dafd8e 100644

drivers/pwm/core.c int pwmchip_add_with_polarity(struct pwm_chip *chip, · line 244

Quoted 9 lines

drivers/pwm/core.c +3 -2

@@ -238,8 +238,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 	unsigned int i;
 	int ret;
 
-	if (!chip || !chip->dev || !chip->ops || !chip->ops->config ||
-	    !chip->ops->enable || !chip->ops->disable || !chip->npwm)
+	if (!chip || !chip->dev || !chip->ops || (!chip->ops->apply &&
+	    (!chip->ops->config || !chip->ops->enable ||
+	     !chip->ops->disable)) || !chip->npwm)
 		return -EINVAL;

This is becoming really unreadable, perhaps split it into two checks, or even split out the sanity check on the ops into a separate function to make the negations easier to read:

	static bool pwm_ops_check(const struct pwm_ops *ops)
	{
		/* driver supports legacy, non-atomic operation */
		if (ops->config && ops->enable && ops->disable)
			return true;

		/* driver supports atomic operation */
		if (ops->apply)
			return true;

		return false;
	}

and then use this:

	if (!chip || !chip->dev || !chip->ops || !chip->npwm)
		return -EINVAL;

	if (!pwm_ops_check(chip->ops))
		return -EINVAL;

drivers/pwm/core.c int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns) · line 435

Quoted 8 lines

drivers/pwm/core.c +2 -1

 	mutex_lock(&pwm_lock);

@@ -430,7 +431,17 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
 	if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
 		return -EINVAL;
 
-	err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
+	if (pwm->chip->ops->apply) {
+		struct pwm_state state = pwm->state;

Shouldn't this use pwm_get_state()?

drivers/pwm/core.c EXPORT_SYMBOL_GPL(pwm_set_polarity); · line 514

Quoted 4 lines

drivers/pwm/core.c +3 -0

+
+		state.enabled = true;
+		err = pwm->chip->ops->apply(pwm->chip, pwm, &state);

There should be a space between the above two lines.

Quoted 6 lines

drivers/pwm/core.c +4 -0

+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
+{
+	int err = 0;
+

drivers/pwm/core.c EXPORT_SYMBOL_GPL(pwm_enable); · line 561

Quoted 10 lines

drivers/pwm/core.c +10 -0

+	if (!pwm)
+		return -EINVAL;
+
+	if (!memcmp(state, &pwm->state, sizeof(*state)))
+		return 0;
+
+	if (pwm->chip->ops->apply) {
+		err = pwm->chip->ops->apply(pwm->chip, pwm, state);
+		if (!err)
+			pwm->state = *state;

Maybe we want pwm_set_state() for this?

Quoted 43 lines

drivers/pwm/core.c +31 -0

+	} else {
+		/*
+		 * FIXME: restore the initial state in case of error.
+		 */
+		if (state->polarity != pwm->state.polarity) {
+			pwm_disable(pwm);
+			err = pwm_set_polarity(pwm, state->polarity);
+			if (err)
+				goto out;
+		}
+
+		if (state->period != pwm->state.period ||
+		    state->duty_cycle != pwm->state.duty_cycle) {
+			err = pwm_config(pwm, state->period, state->duty_cycle);
+			if (err)
+				goto out;
+		}
+
+		if (state->enabled != pwm->state.enabled) {
+			if (state->enabled)
+				err = pwm_enable(pwm);
+			else
+				pwm_disable(pwm);
+		}
+	}
+
+out:
+	return err;
+}
+EXPORT_SYMBOL_GPL(pwm_apply_state);
+
 static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
 {
 	struct pwm_chip *chip;

include/linux/pwm.h +0 -0

index b47244a..7e99679 100644
@@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 	return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
 }
 

include/linux/pwm.h static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm) · line 157

Quoted 4 lines

include/linux/pwm.h +4 -0

+/*
+ * pwm_apply_state - apply a new state to the PWM device
+ */
+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);

If you add kerneldoc, please add it properly. It should start with /** and you need to list at least the parameters and return value.

Quoted 1 line · On Mon, 20 Jul 2015 10:59:40 +0200 Thierry Reding <[email protected]>…

On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:

Quoted 8 lines

Quoted 8 lines

drivers/pwm/core.c +8 -0

+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
+{
+	int err = 0;
+
+	if (!pwm)
+		return -EINVAL;
+
+	if (!memcmp(state, &pwm->state, sizeof(*state)))

Quoted 8 lines

drivers/pwm/core.c EXPORT_SYMBOL_GPL(pwm_enable); · line 561

Quoted 6 lines

drivers/pwm/core.c +6 -0

+		return 0;
+
+	if (pwm->chip->ops->apply) {
+		err = pwm->chip->ops->apply(pwm->chip, pwm, state);
+		if (!err)
+			pwm->state = *state;

Maybe we want pwm_set_state() for this?

I'm not opposed to the addition of the pwm_set_state() function as long as it's a private one: I don't want to let PMW drivers or users mess up with the current PWM state.

Quoted 8 lines

Quoted 8 lines

include/linux/pwm.h +0 -0

index b47244a..7e99679 100644
@@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 	return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
 }
 

Quoted 7 lines

include/linux/pwm.h static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm) · line 157

Quoted 4 lines

include/linux/pwm.h +4 -0

+/*
+ * pwm_apply_state - apply a new state to the PWM device
+ */
+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);

If you add kerneldoc, please add it properly. It should start with /** and you need to list at least the parameters and return value.

Yes, I'll fix that.
BTW, I remember that you were expecting another name for this function (pwm_update IIRC).

Implement the ->init_state() function to expose initial state.

Signed-off-by Boris Brezillon <[email protected]>

---

drivers/pwm/pwm-rockchip.c +7 -0

@@ -98,6 +110,36 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
 *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_init_state(struct pwm_chip *chip,
+				    struct pwm_device *pwm)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	unsigned long clk_rate;
+	u64 tmp;
+	int ret;

drivers/pwm/pwm-rockchip.c +11 -0

+	pwm->state.duty_cycle = tmp;
+
+	pc->data->init(chip, chip->pwms);
+
+	if (!pwm_is_enabled(pwm))
+		clk_disable(pc->clk);
+}
+
 static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device
 *pwm, int duty_ns, int period_ns)
 {

@@ -171,6 +213,7 @@ static void rockchip_pwm_disable(struct pwm_chip *chip,
 struct pwm_device *pwm) }
 
 static const struct pwm_ops rockchip_pwm_ops_v1 = {
+	.init_state = rockchip_pwm_init_state,
 	.config = rockchip_pwm_config,
 	.enable = rockchip_pwm_enable,
 	.disable = rockchip_pwm_disable,

@@ -178,6 +221,7 @@ static const struct pwm_ops rockchip_pwm_ops_v1 = {
 };
 
 static const struct pwm_ops rockchip_pwm_ops_v2 = {
+	.init_state = rockchip_pwm_init_state,
 	.config = rockchip_pwm_config,
 	.set_polarity = rockchip_pwm_set_polarity,
 	.enable = rockchip_pwm_enable,

@@ -195,6 +239,7 @@ static const struct rockchip_pwm_data pwm_data_v1 = {
 	.prescaler = 2,
 	.ops = &rockchip_pwm_ops_v1,
 	.set_enable = rockchip_pwm_set_enable_v1,
+	.init = rockchip_pwm_init_v1,
 };
 
 static const struct rockchip_pwm_data pwm_data_v2 = {

Hi Boris,

Am Mittwoch, 1. Juli 2015, 10:21:58 schrieb Boris Brezillon:

Quoted 4 lines

Implement the ->init_state() function to expose initial state.

Signed-off-by Boris Brezillon <[email protected]>

---

[...]

Quoted 11 lines

drivers/pwm/pwm-rockchip.c +7 -0

@@ -98,6 +110,36 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
 *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_init_state(struct pwm_chip *chip,
+				    struct pwm_device *pwm)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	unsigned long clk_rate;
+	u64 tmp;
+	int ret;

drivers/pwm/pwm-rockchip.c static void rockchip_pwm_set_enable_v2(struct pwm_chip · line 129

Quoted 10 lines

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

+
+	ret = clk_enable(pc->clk);
+	if (ret)
+		return;
+
+	clk_rate = clk_get_rate(pc->clk);
+
+	tmp = readl(pc->base + pc->data->regs.period);
+	tmp *= pc->data->prescaler * NSEC_PER_SEC;
+	tmp = do_div(tmp, clk_rate);

I guess you want to have the division result here and not the remainder, so

drivers/pwm/pwm-rockchip.c static const struct rockchip_pwm_data pwm_data_v2 = { · line 255

Quoted 6 lines

drivers/pwm/pwm-rockchip.c +1 -0

 static const struct rockchip_pwm_data pwm_data_v2 = {

@@ -207,6 +252,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
 	.prescaler = 1,
 	.ops = &rockchip_pwm_ops_v2,
 	.set_enable = rockchip_pwm_set_enable_v2,
+	.init = rockchip_pwm_init_v2,

you're referencing the v2 init here, but only add it in the next patch? [pwm: rockchip: add support for atomic update]

Implement the ->apply() function to add support for atomic update.

Signed-off-by Boris Brezillon <[email protected]>

---

drivers/pwm/pwm-rockchip.c +7 -0

@@ -110,6 +113,26 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
 *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_init_v2(struct pwm_chip *chip, struct pwm_device
 *pwm) +{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
+			  PWM_CONTINUOUS;
+	u32 val;
+
+	val = readl(pc->base + pc->data->regs.ctrl);

Hi Boris,

Am Mittwoch, 1. Juli 2015, 10:21:59 schrieb Boris Brezillon:

Quoted 23 lines

Implement the ->apply() function to add support for atomic update.

Signed-off-by Boris Brezillon <[email protected]>

---

drivers/pwm/pwm-rockchip.c +14 -0

@@ -110,6 +113,26 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
 *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 }
 
+static void rockchip_pwm_init_v2(struct pwm_chip *chip, struct pwm_device
 *pwm) +{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
+			  PWM_CONTINUOUS;
+	u32 val;
+
+	val = readl(pc->base + pc->data->regs.ctrl);
+
+	if ((val & enable_conf) != enable_conf)
+		return;
+
+	pwm->state.enabled = true;
+
+	enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;

drivers/pwm/pwm-rockchip.c static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, · line 133

Quoted 3 lines

drivers/pwm/pwm-rockchip.c +3 -0

+
+	if ((val & enable_conf) == enable_conf)
+		pwm->state.polarity = PWM_POLARITY_INVERSED;

the inactive setting does not affect the polarity of the running pwm, only what to do when it gets turned off. Also PWM_DUTY_NEGATIVE is the "0" value for the bit so also is bad to compare against (and results in wrong readings). So I would suggest changing this like

Implement the ->enable(), ->disable() and ->is_enabled methods and remove the PWM call in ->set_voltage_sel().
This is particularly important for critical regulators tagged as always-on, because not claiming the PWM (and its dependencies) might lead to unpredictable behavior (like a system hang because the PWM clk is only claimed when the PWM device is enabled).

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644

drivers/regulator/pwm-regulator.c +12 -6

@@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct
 regulator_dev *rdev,
 
 	drvdata->state = selector;
 
-	ret = pwm_enable(drvdata->pwm);
-	if (ret) {
-		dev_err(&rdev->dev, "Failed to enable PWM\n");
-		return ret;
-	}
-
 	return 0;
 }

@@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct
 regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV;
 }
 
+static int pwm_regulator_enable(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	return pwm_enable(drvdata->pwm);
+}
+
+static int pwm_regulator_disable(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	pwm_disable(drvdata->pwm);

Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:

Quoted 50 lines

Implement the ->enable(), ->disable() and ->is_enabled methods and remove the PWM call in ->set_voltage_sel().
This is particularly important for critical regulators tagged as always-on, because not claiming the PWM (and its dependencies) might lead to unpredictable behavior (like a system hang because the PWM clk is only claimed when the PWM device is enabled).

Signed-off-by Boris Brezillon <[email protected]>

---
 drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644

drivers/regulator/pwm-regulator.c +16 -6

@@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct
 regulator_dev *rdev,
 
 	drvdata->state = selector;
 
-	ret = pwm_enable(drvdata->pwm);
-	if (ret) {
-		dev_err(&rdev->dev, "Failed to enable PWM\n");
-		return ret;
-	}
-
 	return 0;
 }

@@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct
 regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV;
 }
 
+static int pwm_regulator_enable(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	return pwm_enable(drvdata->pwm);
+}
+
+static int pwm_regulator_disable(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	pwm_disable(drvdata->pwm);
+
+	return 0;
+}
+

drivers/regulator/pwm-regulator.c static int pwm_regulator_list_voltage(struct regulator_dev *rdev, · line 97

Quoted 6 lines

drivers/regulator/pwm-regulator.c +6 -0

+static int pwm_regulator_is_enabled(struct regulator_dev *dev)
+{
+       struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+       return pwm_is_enabled(drvdata->pwm);
+}

nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of tabs)

Hi Heiko,

On Wed, 01 Jul 2015 13:58:09 +0200

Quoted 58 lines · Heiko St?bner

Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:

Quoted 57 lines

Implement the ->enable(), ->disable() and ->is_enabled methods and remove
the PWM call in ->set_voltage_sel().
This is particularly important for critical regulators tagged as always-on,
because not claiming the PWM (and its dependencies) might lead to unpredictable behavior (like a system hang because the PWM clk is only claimed when the PWM device is enabled).

Signed-off-by Boris Brezillon <[email protected]>

---

 drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644

drivers/regulator/pwm-regulator.c +0 -6

@@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct
 regulator_dev *rdev,
 
 	drvdata->state = selector;
 
-	ret = pwm_enable(drvdata->pwm);
-	if (ret) {
-		dev_err(&rdev->dev, "Failed to enable PWM\n");
-		return ret;
-	}
-
 
 	return 0;

}

drivers/regulator/pwm-regulator.c +17 -0

@@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct
 regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV;
 
 }
 
+static int pwm_regulator_enable(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	return pwm_enable(drvdata->pwm);
+}
+
+static int pwm_regulator_disable(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	pwm_disable(drvdata->pwm);
+
+	return 0;
+}
+
+static int pwm_regulator_is_enabled(struct regulator_dev *dev)

Quoted 8 lines

drivers/regulator/pwm-regulator.c static int pwm_regulator_list_voltage(struct regulator_dev *rdev, · line 97

Quoted 5 lines

drivers/regulator/pwm-regulator.c +5 -0

+{
+       struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+       return pwm_is_enabled(drvdata->pwm);
+}

nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of tabs)

Yep, I noticed checkpatch warnings/errors before sending the patch, but since this is just an RFC I decided to fix them for the next version ;-)

Am Mittwoch, 1. Juli 2015, 14:05:31 schrieb Boris Brezillon:

Quoted 71 lines

Hi Heiko,

On Wed, 01 Jul 2015 13:58:09 +0200

Quoted 66 lines · Heiko St?bner

Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:

Quoted 52 lines

Implement the ->enable(), ->disable() and ->is_enabled methods and remove
the PWM call in ->set_voltage_sel().
This is particularly important for critical regulators tagged as always-on,
because not claiming the PWM (and its dependencies) might lead to unpredictable behavior (like a system hang because the PWM clk is only claimed when the PWM device is enabled).

Signed-off-by Boris Brezillon <[email protected]>

---

 drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644

drivers/regulator/pwm-regulator.c +0 -6

@@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct
 regulator_dev *rdev,
 
 	drvdata->state = selector;
 
-	ret = pwm_enable(drvdata->pwm);
-	if (ret) {
-		dev_err(&rdev->dev, "Failed to enable PWM\n");
-		return ret;
-	}
-
 
 	return 0;

}

drivers/regulator/pwm-regulator.c +12 -0

@@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct
 regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV;
 
 }
 
+static int pwm_regulator_enable(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	return pwm_enable(drvdata->pwm);
+}
+
+static int pwm_regulator_disable(struct regulator_dev *dev)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+	pwm_disable(drvdata->pwm);

drivers/regulator/pwm-regulator.c static int pwm_regulator_list_voltage(struct · line 98

Quoted 10 lines

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

+
+	return 0;
+}
+
+static int pwm_regulator_is_enabled(struct regulator_dev *dev)
+{
+       struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+
+       return pwm_is_enabled(drvdata->pwm);
+}

nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of tabs)

Quoted 2 lines

Yep, I noticed checkpatch warnings/errors before sending the patch, but since this is just an RFC I decided to fix them for the next version ;-)

ok, so I'll just skip over any more style issues for now. Making my way through your series and trying it on my veyron right now :-)

Implement the ->enable(), ->disable() and ->is_enabled methods and remove the PWM call in ->set_voltage_sel().

Quoted 2 lines · Boris Brezillon

Implement the ->enable(), ->disable() and ->is_enabled methods and remove the PWM call in ->set_voltage_sel().

This doesn't apply, please check and resend.

Quoted 3 lines · Mark Brown

Quoted 2 lines · Boris Brezillon

Implement the ->enable(), ->disable() and ->is_enabled methods and remove the PWM call in ->set_voltage_sel().

Quoted 1 line

This doesn't apply, please check and resend.

This series was made on top of Linus' tree (4.2-rc1 IIRC) and patch 14 and 15 were not meant to be applied without Thierry's approval (they depend on other changes in the PWM framework).
I can rebase them on top of linux-next (or just on top of Linus' 4.2-rc2) if Thierry is okay with that, but I don't think rebasing them on your regulator's for-next branch is a good idea.

Quoted 7 lines · Boris Brezillon

Quoted 5 lines · Mark Brown

Quoted 2 lines · Boris Brezillon

Implement the ->enable(), ->disable() and ->is_enabled methods and remove the PWM call in ->set_voltage_sel().

This doesn't apply, please check and resend.

Quoted 6 lines

This series was made on top of Linus' tree (4.2-rc1 IIRC) and patch 14 and 15 were not meant to be applied without Thierry's approval (they depend on other changes in the PWM framework).
I can rebase them on top of linux-next (or just on top of Linus' 4.2-rc2) if Thierry is okay with that, but I don't think rebasing them on your regulator's for-next branch is a good idea.

What is the dependency for the enable patch?