The "atomic" API allows us to configure PWM period and duty_cycle and enable it in one call.
The patch also moves the pwm_init_state just before any use of the pwm_state struct, this fixes a potential bug where pwm_get_state can be called before pwm_init_state.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com ---
Changes in v2: - Do not force the PWM be off in the first call to pwm_apply_state. - Delayed applying the state until we know what the period is. - Removed pb->period as after the conversion is not needed.
drivers/video/backlight/pwm_bl.c | 71 ++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 30 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index bdfcc0a71db1..dd1cb29b5332 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -28,7 +28,6 @@ struct pwm_bl_data { struct pwm_device *pwm; struct device *dev; - unsigned int period; unsigned int lth_brightness; unsigned int *levels; bool enabled; @@ -46,7 +45,8 @@ struct pwm_bl_data { void (*exit)(struct device *); };
-static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness) +static void pwm_backlight_power_on(struct pwm_bl_data *pb, + struct pwm_state *state) { int err;
@@ -57,7 +57,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness) if (err < 0) dev_err(pb->dev, "failed to enable power supply\n");
- pwm_enable(pb->pwm); + state->enabled = true; + pwm_apply_state(pb->pwm, state);
if (pb->post_pwm_on_delay) msleep(pb->post_pwm_on_delay); @@ -70,6 +71,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
static void pwm_backlight_power_off(struct pwm_bl_data *pb) { + struct pwm_state state; + if (!pb->enabled) return;
@@ -79,8 +82,10 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) if (pb->pwm_off_delay) msleep(pb->pwm_off_delay);
- pwm_config(pb->pwm, 0, pb->period); - pwm_disable(pb->pwm); + pwm_get_state(pb->pwm, &state); + state.enabled = false; + state.duty_cycle = 0; + pwm_apply_state(pb->pwm, &state);
regulator_disable(pb->power_supply); pb->enabled = false; @@ -89,14 +94,17 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) { unsigned int lth = pb->lth_brightness; + struct pwm_state state; u64 duty_cycle;
+ pwm_get_state(pb->pwm, &state); + if (pb->levels) duty_cycle = pb->levels[brightness]; else duty_cycle = brightness;
- duty_cycle *= pb->period - lth; + duty_cycle *= state.period - lth; do_div(duty_cycle, pb->scale);
return duty_cycle + lth; @@ -106,6 +114,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) { struct pwm_bl_data *pb = bl_get_data(bl); int brightness = bl->props.brightness; + struct pwm_state state; int duty_cycle;
if (bl->props.power != FB_BLANK_UNBLANK || @@ -118,8 +127,12 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
if (brightness > 0) { duty_cycle = compute_duty_cycle(pb, brightness); - pwm_config(pb->pwm, duty_cycle, pb->period); - pwm_backlight_power_on(pb, brightness); + pwm_get_state(pb->pwm, &state); + state.duty_cycle = duty_cycle; + if (!state.enabled) + pwm_backlight_power_on(pb, &state); + else + pwm_apply_state(pb->pwm, &state); } else pwm_backlight_power_off(pb);
@@ -447,7 +460,6 @@ static int pwm_backlight_probe(struct platform_device *pdev) struct device_node *node = pdev->dev.of_node; struct pwm_bl_data *pb; struct pwm_state state; - struct pwm_args pargs; unsigned int i; int ret;
@@ -539,10 +551,26 @@ static int pwm_backlight_probe(struct platform_device *pdev)
dev_dbg(&pdev->dev, "got pwm for backlight\n");
- if (!data->levels) { - /* Get the PWM period (in nanoseconds) */ - pwm_get_state(pb->pwm, &state); + /* Sync up PWM state. */ + pwm_init_state(pb->pwm, &state); + + /* + * The DT case will set the pwm_period_ns field to 0 and store the + * period, parsed from the DT, in the PWM device. For the non-DT case, + * set the period from platform data if it has not already been set + * via the PWM lookup table. + */ + if (!state.period && (data->pwm_period_ns > 0)) + state.period = data->pwm_period_ns;
+ ret = pwm_apply_state(pb->pwm, &state); + if (ret) { + dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n", + ret); + goto err_alloc; + } + + if (!data->levels) { ret = pwm_backlight_brightness_default(&pdev->dev, data, state.period); if (ret < 0) { @@ -559,24 +587,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) pb->levels = data->levels; }
- /* - * FIXME: pwm_apply_args() should be removed when switching to - * the atomic PWM API. - */ - pwm_apply_args(pb->pwm); - - /* - * The DT case will set the pwm_period_ns field to 0 and store the - * period, parsed from the DT, in the PWM device. For the non-DT case, - * set the period from platform data if it has not already been set - * via the PWM lookup table. - */ - pwm_get_args(pb->pwm, &pargs); - pb->period = pargs.period; - if (!pb->period && (data->pwm_period_ns > 0)) - pb->period = data->pwm_period_ns; - - pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale); + pb->lth_brightness = data->lth_brightness * (state.period / pb->scale);
memset(&props, 0, sizeof(struct backlight_properties)); props.type = BACKLIGHT_RAW;
On Fri, Jul 27, 2018 at 05:11:21PM +0200, Enric Balletbo i Serra wrote:
The "atomic" API allows us to configure PWM period and duty_cycle and enable it in one call.
The patch also moves the pwm_init_state just before any use of the pwm_state struct, this fixes a potential bug where pwm_get_state can be called before pwm_init_state.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com
Changes in v2:
- Do not force the PWM be off in the first call to pwm_apply_state.
- Delayed applying the state until we know what the period is.
- Removed pb->period as after the conversion is not needed.
Re-reading this I have spotted a couple of things I probably could have mentioned against v1... sorry.
I think it's looking good though, I expect to be able to ack v3.
drivers/video/backlight/pwm_bl.c | 71 ++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 30 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index bdfcc0a71db1..dd1cb29b5332 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -28,7 +28,6 @@ struct pwm_bl_data { struct pwm_device *pwm; struct device *dev;
- unsigned int period; unsigned int lth_brightness; unsigned int *levels; bool enabled;
@@ -46,7 +45,8 @@ struct pwm_bl_data { void (*exit)(struct device *); };
-static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness) +static void pwm_backlight_power_on(struct pwm_bl_data *pb,
struct pwm_state *state){ int err;
@@ -57,7 +57,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness) if (err < 0) dev_err(pb->dev, "failed to enable power supply\n");
- pwm_enable(pb->pwm);
state->enabled = true;
pwm_apply_state(pb->pwm, state);
if (pb->post_pwm_on_delay) msleep(pb->post_pwm_on_delay);
@@ -70,6 +71,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
static void pwm_backlight_power_off(struct pwm_bl_data *pb) {
- struct pwm_state state;
- if (!pb->enabled) return;
@@ -79,8 +82,10 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) if (pb->pwm_off_delay) msleep(pb->pwm_off_delay);
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
- pwm_get_state(pb->pwm, &state);
- state.enabled = false;
- state.duty_cycle = 0;
- pwm_apply_state(pb->pwm, &state);
This is an in exact conversion because this code ignores a failure to set the duty cycle to zero whilst pwm_apply_state() does not.
This would only matter if pwm_config() returns an error and given that a PWM which does not support a duty cycle of zero is permitted to adjust zero to the smallest supported value there is no *need* for a driver to return an error here. In other words... this is a subtle change of behaviour and perhaps (even probably) irrelevant.
However I'm still interested whether you did any work to confirm or deny whether drivers that reports error on zero duty cycle actually exist.
regulator_disable(pb->power_supply); pb->enabled = false; @@ -89,14 +94,17 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) { unsigned int lth = pb->lth_brightness;
struct pwm_state state; u64 duty_cycle;
pwm_get_state(pb->pwm, &state);
if (pb->levels) duty_cycle = pb->levels[brightness]; else duty_cycle = brightness;
- duty_cycle *= pb->period - lth;
duty_cycle *= state.period - lth; do_div(duty_cycle, pb->scale);
return duty_cycle + lth;
@@ -106,6 +114,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) { struct pwm_bl_data *pb = bl_get_data(bl); int brightness = bl->props.brightness;
struct pwm_state state; int duty_cycle;
if (bl->props.power != FB_BLANK_UNBLANK ||
@@ -118,8 +127,12 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
if (brightness > 0) { duty_cycle = compute_duty_cycle(pb, brightness);
pwm_config(pb->pwm, duty_cycle, pb->period);
In principle the same subtle change applies here... but if pwm_config() reported an error here then the backlight probably didn't work before your change either so less need to worry about it!
pwm_backlight_power_on(pb, brightness);
pwm_get_state(pb->pwm, &state);state.duty_cycle = duty_cycle;if (!state.enabled)pwm_backlight_power_on(pb, &state);
It verges towards nit picking but I don't really like the way a half updated state is shared between ...update_status and ...power_on.
I'd rather it looked something like:
pwm_get_state(pb->pwm, &state); if (!state.enabled) { pwm_backlight_power_on(pb); <-- no sharing here, make on match off } else { pwm_backlight_update_duty_cycle(pb, &state, brightness); pwm_apply_state(pb->pwm, &state); } (and have pwm_backlight_power_on() also call ...update_duty_cycle too)
Thoughts?
Daniel.
Hi Daniel,
On 30/07/18 13:12, Daniel Thompson wrote:
On Fri, Jul 27, 2018 at 05:11:21PM +0200, Enric Balletbo i Serra wrote:
The "atomic" API allows us to configure PWM period and duty_cycle and enable it in one call.
The patch also moves the pwm_init_state just before any use of the pwm_state struct, this fixes a potential bug where pwm_get_state can be called before pwm_init_state.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com
Changes in v2:
- Do not force the PWM be off in the first call to pwm_apply_state.
- Delayed applying the state until we know what the period is.
- Removed pb->period as after the conversion is not needed.
Re-reading this I have spotted a couple of things I probably could have mentioned against v1... sorry.
I think it's looking good though, I expect to be able to ack v3.
drivers/video/backlight/pwm_bl.c | 71 ++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 30 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index bdfcc0a71db1..dd1cb29b5332 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -28,7 +28,6 @@ struct pwm_bl_data { struct pwm_device *pwm; struct device *dev;
- unsigned int period; unsigned int lth_brightness; unsigned int *levels; bool enabled;
@@ -46,7 +45,8 @@ struct pwm_bl_data { void (*exit)(struct device *); };
-static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness) +static void pwm_backlight_power_on(struct pwm_bl_data *pb,
struct pwm_state *state){ int err;
@@ -57,7 +57,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness) if (err < 0) dev_err(pb->dev, "failed to enable power supply\n");
- pwm_enable(pb->pwm);
state->enabled = true;
pwm_apply_state(pb->pwm, state);
if (pb->post_pwm_on_delay) msleep(pb->post_pwm_on_delay);
@@ -70,6 +71,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
static void pwm_backlight_power_off(struct pwm_bl_data *pb) {
- struct pwm_state state;
- if (!pb->enabled) return;
@@ -79,8 +82,10 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) if (pb->pwm_off_delay) msleep(pb->pwm_off_delay);
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
- pwm_get_state(pb->pwm, &state);
- state.enabled = false;
- state.duty_cycle = 0;
- pwm_apply_state(pb->pwm, &state);
This is an in exact conversion because this code ignores a failure to set the duty cycle to zero whilst pwm_apply_state() does not.
This would only matter if pwm_config() returns an error and given that a PWM which does not support a duty cycle of zero is permitted to adjust zero to the smallest supported value there is no *need* for a driver to return an error here. In other words... this is a subtle change of behaviour and perhaps (even probably) irrelevant.
However I'm still interested whether you did any work to confirm or deny whether drivers that reports error on zero duty cycle actually exist.
Interesting, actually I don't have a use case for this, and I think that there is nothing in the kernel. I know that some devices (like chromebook minnie and jaq) the pwm must be >= 1% or 3% for the first non-zero value but I don't know any where 0 is a problem.
regulator_disable(pb->power_supply); pb->enabled = false; @@ -89,14 +94,17 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) { unsigned int lth = pb->lth_brightness;
struct pwm_state state; u64 duty_cycle;
pwm_get_state(pb->pwm, &state);
if (pb->levels) duty_cycle = pb->levels[brightness]; else duty_cycle = brightness;
- duty_cycle *= pb->period - lth;
duty_cycle *= state.period - lth; do_div(duty_cycle, pb->scale);
return duty_cycle + lth;
@@ -106,6 +114,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) { struct pwm_bl_data *pb = bl_get_data(bl); int brightness = bl->props.brightness;
struct pwm_state state; int duty_cycle;
if (bl->props.power != FB_BLANK_UNBLANK ||
@@ -118,8 +127,12 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
if (brightness > 0) { duty_cycle = compute_duty_cycle(pb, brightness);
pwm_config(pb->pwm, duty_cycle, pb->period);In principle the same subtle change applies here... but if pwm_config() reported an error here then the backlight probably didn't work before your change either so less need to worry about it!
pwm_backlight_power_on(pb, brightness);
pwm_get_state(pb->pwm, &state);state.duty_cycle = duty_cycle;if (!state.enabled)pwm_backlight_power_on(pb, &state);It verges towards nit picking but I don't really like the way a half updated state is shared between ...update_status and ...power_on.
I'd rather it looked something like:
pwm_get_state(pb->pwm, &state); if (!state.enabled) { pwm_backlight_power_on(pb); <-- no sharing here, make on match off } else { pwm_backlight_update_duty_cycle(pb, &state, brightness); pwm_apply_state(pb->pwm, &state); }(and have pwm_backlight_power_on() also call ...update_duty_cycle too)
Thoughts?
What about something like this:
static int pwm_backlight_update_status(struct backlight_device *bl) { ...
if (brightness > 0) { pwm_get_state(pb->pwm, &state); /* we can get rid of duty_cycle temporal variable */ state.duty_cycle = compute_duty_cycle(pb, brightness); pwm_apply_state(pb->pwm, &state); pwm_backlight_power_on(pb); } else pwm_backlight_power_off(pb); ... }
static void pwm_backlight_power_on(struct pwm_bl_data *pb) { struct pwm_state state;
pwm_get_state(pb->pwm, &state);
if (state.enabled) return;
...
state.enabled = true; pwm_apply_state(pb->pwm, &state);
... }
static void pwm_backlight_power_off(struct pwm_bl_data *pb) { struct pwm_state state;
...
pwm_get_state(pb->pwm, &state); state.enabled = false; state.duty_cycle = 0; pwm_apply_state(pb->pwm, &state);
... }
And I think that we can get rid of pb->enabled variable.
Best regards, Enric
Daniel.
On Tue, Aug 07, 2018 at 11:38:04AM +0200, Enric Balletbo i Serra wrote:
Hi Daniel,
On 30/07/18 13:12, Daniel Thompson wrote:
On Fri, Jul 27, 2018 at 05:11:21PM +0200, Enric Balletbo i Serra wrote:
The "atomic" API allows us to configure PWM period and duty_cycle and enable it in one call.
The patch also moves the pwm_init_state just before any use of the pwm_state struct, this fixes a potential bug where pwm_get_state can be called before pwm_init_state.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com
Changes in v2:
- Do not force the PWM be off in the first call to pwm_apply_state.
- Delayed applying the state until we know what the period is.
- Removed pb->period as after the conversion is not needed.
Re-reading this I have spotted a couple of things I probably could have mentioned against v1... sorry.
I think it's looking good though, I expect to be able to ack v3.
drivers/video/backlight/pwm_bl.c | 71 ++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 30 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index bdfcc0a71db1..dd1cb29b5332 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -28,7 +28,6 @@ struct pwm_bl_data { struct pwm_device *pwm; struct device *dev;
- unsigned int period; unsigned int lth_brightness; unsigned int *levels; bool enabled;
@@ -46,7 +45,8 @@ struct pwm_bl_data { void (*exit)(struct device *); };
-static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness) +static void pwm_backlight_power_on(struct pwm_bl_data *pb,
struct pwm_state *state){ int err;
@@ -57,7 +57,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness) if (err < 0) dev_err(pb->dev, "failed to enable power supply\n");
- pwm_enable(pb->pwm);
state->enabled = true;
pwm_apply_state(pb->pwm, state);
if (pb->post_pwm_on_delay) msleep(pb->post_pwm_on_delay);
@@ -70,6 +71,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
static void pwm_backlight_power_off(struct pwm_bl_data *pb) {
- struct pwm_state state;
- if (!pb->enabled) return;
@@ -79,8 +82,10 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) if (pb->pwm_off_delay) msleep(pb->pwm_off_delay);
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
- pwm_get_state(pb->pwm, &state);
- state.enabled = false;
- state.duty_cycle = 0;
- pwm_apply_state(pb->pwm, &state);
This is an in exact conversion because this code ignores a failure to set the duty cycle to zero whilst pwm_apply_state() does not.
This would only matter if pwm_config() returns an error and given that a PWM which does not support a duty cycle of zero is permitted to adjust zero to the smallest supported value there is no *need* for a driver to return an error here. In other words... this is a subtle change of behaviour and perhaps (even probably) irrelevant.
However I'm still interested whether you did any work to confirm or deny whether drivers that reports error on zero duty cycle actually exist.
Interesting, actually I don't have a use case for this, and I think that there is nothing in the kernel. I know that some devices (like chromebook minnie and jaq) the pwm must be >= 1% or 3% for the first non-zero value but I don't know any where 0 is a problem.
regulator_disable(pb->power_supply); pb->enabled = false; @@ -89,14 +94,17 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) { unsigned int lth = pb->lth_brightness;
struct pwm_state state; u64 duty_cycle;
pwm_get_state(pb->pwm, &state);
if (pb->levels) duty_cycle = pb->levels[brightness]; else duty_cycle = brightness;
- duty_cycle *= pb->period - lth;
duty_cycle *= state.period - lth; do_div(duty_cycle, pb->scale);
return duty_cycle + lth;
@@ -106,6 +114,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl) { struct pwm_bl_data *pb = bl_get_data(bl); int brightness = bl->props.brightness;
struct pwm_state state; int duty_cycle;
if (bl->props.power != FB_BLANK_UNBLANK ||
@@ -118,8 +127,12 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
if (brightness > 0) { duty_cycle = compute_duty_cycle(pb, brightness);
pwm_config(pb->pwm, duty_cycle, pb->period);In principle the same subtle change applies here... but if pwm_config() reported an error here then the backlight probably didn't work before your change either so less need to worry about it!
pwm_backlight_power_on(pb, brightness);
pwm_get_state(pb->pwm, &state);state.duty_cycle = duty_cycle;if (!state.enabled)pwm_backlight_power_on(pb, &state);It verges towards nit picking but I don't really like the way a half updated state is shared between ...update_status and ...power_on.
I'd rather it looked something like:
pwm_get_state(pb->pwm, &state); if (!state.enabled) { pwm_backlight_power_on(pb); <-- no sharing here, make on match off } else { pwm_backlight_update_duty_cycle(pb, &state, brightness); pwm_apply_state(pb->pwm, &state); }(and have pwm_backlight_power_on() also call ...update_duty_cycle too)
Thoughts?
What about something like this:
static int pwm_backlight_update_status(struct backlight_device *bl) { ...
if (brightness > 0) { pwm_get_state(pb->pwm, &state); /* we can get rid of duty_cycle temporal variable */ state.duty_cycle = compute_duty_cycle(pb, brightness); pwm_apply_state(pb->pwm, &state); pwm_backlight_power_on(pb); } else pwm_backlight_power_off(pb); ... }
This reads very well. I'm happy to go with this approach.
static void pwm_backlight_power_on(struct pwm_bl_data *pb) { struct pwm_state state;
pwm_get_state(pb->pwm, &state);
if (state.enabled) return;
...state.enabled = true; pwm_apply_state(pb->pwm, &state);
...}
static void pwm_backlight_power_off(struct pwm_bl_data *pb) { struct pwm_state state;
...pwm_get_state(pb->pwm, &state); state.enabled = false; state.duty_cycle = 0; pwm_apply_state(pb->pwm, &state);
...}
And I think that we can get rid of pb->enabled variable.
Best regards, Enric
Daniel.
dri-devel@lists.freedesktop.org
-
Daniel Thompson -
Enric Balletbo i Serra