From: Boris Brezillon <hidden> Date: 2015-11-16 08:57:04
Hello,
This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
Best Regards,
Boris
Changes since v3:
- rebased on pwm/for-next after pulling 4.4-rc1
- replace direct access to pwm fields by pwm_get/set_xxx() helpers, thus
fixing some build errors
- split changes to allow each maintainer to review/ack or take the
modification through its subsystem
Changes since v2:
- rebased on top of 4.3-rc2
- reintroduced pwm-regulator patches
Changes since v1:
- dropped applied patches
- squashed Heiko's fixes into the rockchip driver changes
- made a few cosmetic changes
- added kerneldoc comments
- added Heiko's patch to display more information in debugfs
- dropped pwm-regulator patches (should be submitted separately)
Boris Brezillon (23):
pwm: rcar: make use of pwm_is_enabled()
pwm: use pwm_get_xxx() helpers where appropriate
clk: pwm: use pwm_get_xxx() helpers where appropriate
hwmon: pwm-fan: use pwm_get_xxx() helpers where appropriate
misc: max77693-haptic: use pwm_get_xxx() helpers where appropriate
pwm: introduce default period and polarity concepts
pwm: use pwm_get/set_default_xxx() helpers where appropriate
leds: pwm: use pwm_get/set_default_xxx() helpers where appropriate
regulator: pwm: use pwm_get/set_default_xxx() helpers where
appropriate
backlight: pwm: use pwm_get/set_default_xxx() helpers where
appropriate
fbdev: use pwm_get/set_default_xxx() helpers where appropriate
misc: max77693: use pwm_get/set_default_xxx() helpers where
appropriate
hwmon: pwm-fan: use pwm_get/set_default_xxx() helpers where
appropriate
clk: pwm: use pwm_get/set_default_xxx() helpers where appropriate
pwm: define a new pwm_state struct
pwm: move the enabled/disabled info to pwm_state struct
backlight: pwm_bl: remove useless call to pwm_set_period
pwm: declare a default PWM state
pwm: add the PWM initial state retrieval infra
pwm: add the core infrastructure to allow atomic update
pwm: rockchip: add initial state retrieval
pwm: rockchip: add support for atomic update
regulator: pwm: properly initialize the ->state field
Heiko Stübner (1):
pwm: add information about polarity, duty cycle and period to debugfs
drivers/clk/clk-pwm.c | 11 +--
drivers/hwmon/pwm-fan.c | 16 ++--
drivers/input/misc/max77693-haptic.c | 9 +-
drivers/leds/leds-pwm.c | 2 +-
drivers/pwm/core.c | 169 +++++++++++++++++++++++++++++++----
drivers/pwm/pwm-crc.c | 2 +-
drivers/pwm/pwm-lpc18xx-sct.c | 2 +-
drivers/pwm/pwm-pxa.c | 2 +-
drivers/pwm/pwm-rcar.c | 2 +-
drivers/pwm/pwm-rockchip.c | 119 +++++++++++++++++++-----
drivers/pwm/pwm-sun4i.c | 3 +-
drivers/regulator/pwm-regulator.c | 30 ++++++-
drivers/video/backlight/lm3630a_bl.c | 4 +-
drivers/video/backlight/pwm_bl.c | 10 ++-
drivers/video/fbdev/ssd1307fb.c | 2 +-
include/linux/pwm.h | 89 +++++++++++++++---
16 files changed, 386 insertions(+), 86 deletions(-)
--
2.1.4
From: Boris Brezillon <hidden> Date: 2015-11-16 08:57:00
Commit 5c31252c4a86 ("pwm: Add the pwm_is_enabled() helper") introduced a
new function to test whether a PWM device is enabled or not without
manipulating PWM internal fields.
Hiding this is necessary if we want to smoothly move to the atomic PWM
config approach without impacting PWM drivers.
Fix this driver to use pwm_is_enabled() instead of directly accessing the
->flags field.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/pwm/pwm-rcar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -157,7 +157,7 @@ static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,returndiv;/* Let the core driver set pwm->period if disabled and duty_ns = 0 */-if(!test_bit(PWMF_ENABLED,&pwm->flags)&&!duty_ns)+if(!pwm_is_enabled(pwm)&&!duty_ns)return0;rcar_pwm_update(rp,RCAR_PWMCR_SYNC,RCAR_PWMCR_SYNC,RCAR_PWMCR);
@@ -47,8 +47,8 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)if(ctx->pwm_value=pwm)gotoexit_set_pwm_err;-duty=DIV_ROUND_UP(pwm*(ctx->pwm->period-1),MAX_PWM);-ret=pwm_config(ctx->pwm,duty,ctx->pwm->period);+duty=DIV_ROUND_UP(pwm*(pwm_get_period((ctx->pwm))-1),MAX_PWM);+ret=pwm_config(ctx->pwm,duty,pwm_get_period((ctx->pwm)));if(ret)gotoexit_set_pwm_err;
@@ -234,10 +234,10 @@ static int pwm_fan_probe(struct platform_device *pdev)platform_set_drvdata(pdev,ctx);/* Set duty cycle to maximum allowed */-duty_cycle=ctx->pwm->period-1;+duty_cycle=pwm_get_period((ctx->pwm))-1;ctx->pwm_value=MAX_PWM;-ret=pwm_config(ctx->pwm,duty_cycle,ctx->pwm->period);+ret=pwm_config(ctx->pwm,duty_cycle,pwm_get_period((ctx->pwm)));if(ret){dev_err(&pdev->dev,"Failed to configure PWM\n");returnret;
@@ -309,8 +309,9 @@ static int pwm_fan_resume(struct device *dev)if(ctx->pwm_value=0)return0;-duty=DIV_ROUND_UP(ctx->pwm_value*(ctx->pwm->period-1),MAX_PWM);-ret=pwm_config(ctx->pwm,duty,ctx->pwm->period);+duty=DIV_ROUND_UP(ctx->pwm_value*(pwm_get_period((ctx->pwm))-1),+MAX_PWM);+ret=pwm_config(ctx->pwm,duty,pwm_get_period((ctx->pwm)));if(ret)returnret;returnpwm_enable(ctx->pwm);
From: Boris Brezillon <hidden> Date: 2015-11-16 08:57:34
Add an ->apply() method to the pwm_ops struct to allow PWM drivers to
implement atomic update.
This method will be preferred 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 <redacted>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
drivers/pwm/core.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++---
include/linux/pwm.h | 27 ++++++++++++
2 files changed, 146 insertions(+), 5 deletions(-)
@@ -244,8 +263,10 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,unsignedinti;intret;-if(!chip||!chip->dev||!chip->ops||!chip->ops->config||-!chip->ops->enable||!chip->ops->disable||!chip->npwm)+if(!chip||!chip->dev||!chip->ops||!chip->npwm)+return-EINVAL;++if(!pwm_ops_check(chip->ops))return-EINVAL;mutex_lock(&pwm_lock);
@@ -446,7 +467,19 @@ 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){+structpwm_statestate;++pwm_get_state(pwm,&state);+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)returnerr;
From: Boris Brezillon <hidden> Date: 2015-11-16 08:57:39
From: Heiko Stübner <heiko@sntech.de>
The pwm-states make it possible to also output the polarity, duty cycle
and period information in the debugfs pwm summary-outout.
This makes it easier to gather overview information about pwms without
needing to walk through the sysfs attributes of every pwm.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Boris Brezillon <redacted>
---
drivers/pwm/core.c | 5 +++++
1 file changed, 5 insertions(+)
From: Boris Brezillon <hidden> Date: 2015-11-16 08:57:43
Implement the ->apply() function to add support for atomic update.
Signed-off-by: Boris Brezillon <redacted>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
drivers/pwm/pwm-rockchip.c | 53 +++++++++++++++++++++++++---------------------
1 file changed, 29 insertions(+), 24 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-11-16 08:58:35
The ->state field is currently initialized to 0, thus referencing the
voltage selector at index 0, which might not reflect the current voltage
value.
If possible, retrieve the current voltage selector from the PWM state, else
return -EINVAL.
Signed-off-by: Boris Brezillon <redacted>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Mark Brown <broonie@kernel.org>
---
drivers/regulator/pwm-regulator.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
From: Boris Brezillon <hidden> Date: 2015-11-16 08:59:12
Add a ->reset_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 <redacted>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
drivers/pwm/core.c | 3 +++
include/linux/pwm.h | 4 ++++
2 files changed, 7 insertions(+)
From: Boris Brezillon <hidden> Date: 2015-11-16 08:59:36
Prepare the addition of the PWM initial state retrieval by adding a default
state where all the parameters retrieved from DT, platform data or
statically forced by the hardware will be stored.
Once done we will be able to store the initial state in the ->state field
without risking to loose the default parameters.
Update the pwm_set/get_default_xxx helpers accordingly.
Signed-off-by: Boris Brezillon <redacted>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
include/linux/pwm.h | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-11-16 09:00:00
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 <redacted>
Acked-by: Lee Jones <redacted>
---
drivers/video/backlight/pwm_bl.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-11-16 09:00:28
The PWM state, represented by its period, duty_cycle and polarity,
is currently directly stored in the PWM device.
Declare a pwm_state structure embedding those field so that we can later
use this struct to atomically update all the PWM parameters at once.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/pwm/core.c | 6 +++---
include/linux/pwm.h | 30 +++++++++++++++++++-----------
2 files changed, 22 insertions(+), 14 deletions(-)
@@ -447,8 +447,8 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)if(err)returnerr;-pwm->duty_cycle=duty_ns;-pwm->period=period_ns;+pwm->state.duty_cycle=duty_ns;+pwm->state.period=period_ns;return0;}
From: Boris Brezillon <hidden> Date: 2015-11-16 09:00:32
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 <redacted>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
drivers/pwm/core.c | 17 +++++++++++++----
include/linux/pwm.h | 7 ++++---
2 files changed, 17 insertions(+), 7 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-11-16 09:00:38
pwm_set/get_default_xxx() helpers have been introduced to differentiate
the default PWM states (those retrieved through DT, PWM lookup table or
statically assigned by the driver) and the current ones.
Make use of those helpers where appropriate.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/clk/clk-pwm.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -71,23 +71,23 @@ static int clk_pwm_probe(struct platform_device *pdev)if(IS_ERR(pwm))returnPTR_ERR(pwm);-if(!pwm_get_period((pwm))){+if(!pwm_get_default_period((pwm))){dev_err(&pdev->dev,"invalid PWM period\n");return-EINVAL;}if(of_property_read_u32(node,"clock-frequency",&clk_pwm->fixed_rate))-clk_pwm->fixed_rate=NSEC_PER_SEC/pwm_get_period((pwm));+clk_pwm->fixed_rate=NSEC_PER_SEC/pwm_get_default_period((pwm));-if(pwm_get_period((pwm))!=NSEC_PER_SEC/clk_pwm->fixed_rate&&-pwm_get_period((pwm))!=DIV_ROUND_UP(NSEC_PER_SEC,clk_pwm->fixed_rate)){+if(pwm_get_default_period((pwm))!=NSEC_PER_SEC/clk_pwm->fixed_rate&&+pwm_get_default_period((pwm))!=DIV_ROUND_UP(NSEC_PER_SEC,clk_pwm->fixed_rate)){dev_err(&pdev->dev,"clock-frequency does not match PWM period\n");return-EINVAL;}-ret=pwm_config(pwm,(pwm_get_period((pwm))+1)>>1,-pwm_get_period((pwm)));+ret=pwm_config(pwm,(pwm_get_default_period((pwm))+1)>>1,+pwm_get_default_period((pwm)));if(ret<0)returnret;
From: Boris Brezillon <hidden> Date: 2015-11-16 09:01:33
pwm_set/get_default_xxx() helpers have been introduced to differentiate
the default PWM states (those retrieved through DT, PWM lookup table or
statically assigned by the driver) and the current ones.
Make use of those helpers where appropriate.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/hwmon/pwm-fan.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
@@ -47,8 +47,9 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)if(ctx->pwm_value=pwm)gotoexit_set_pwm_err;-duty=DIV_ROUND_UP(pwm*(pwm_get_period((ctx->pwm))-1),MAX_PWM);-ret=pwm_config(ctx->pwm,duty,pwm_get_period((ctx->pwm)));+duty=DIV_ROUND_UP(pwm*(pwm_get_default_period((ctx->pwm))-1),+MAX_PWM);+ret=pwm_config(ctx->pwm,duty,pwm_get_default_period((ctx->pwm)));if(ret)gotoexit_set_pwm_err;
@@ -234,10 +235,11 @@ static int pwm_fan_probe(struct platform_device *pdev)platform_set_drvdata(pdev,ctx);/* Set duty cycle to maximum allowed */-duty_cycle=pwm_get_period((ctx->pwm))-1;+duty_cycle=pwm_get_default_period((ctx->pwm))-1;ctx->pwm_value=MAX_PWM;-ret=pwm_config(ctx->pwm,duty_cycle,pwm_get_period((ctx->pwm)));+ret=pwm_config(ctx->pwm,duty_cycle,+pwm_get_default_period((ctx->pwm)));if(ret){dev_err(&pdev->dev,"Failed to configure PWM\n");returnret;
@@ -309,9 +311,10 @@ static int pwm_fan_resume(struct device *dev)if(ctx->pwm_value=0)return0;-duty=DIV_ROUND_UP(ctx->pwm_value*(pwm_get_period((ctx->pwm))-1),+duty=DIV_ROUND_UP(ctx->pwm_value*+(pwm_get_default_period((ctx->pwm))-1),MAX_PWM);-ret=pwm_config(ctx->pwm,duty,pwm_get_period((ctx->pwm)));+ret=pwm_config(ctx->pwm,duty,pwm_get_default_period((ctx->pwm)));if(ret)returnret;returnpwm_enable(ctx->pwm);
From: Boris Brezillon <hidden> Date: 2015-11-16 09:02:41
pwm_set/get_default_xxx() helpers have been introduced to differentiate
the default PWM states (those retrieved through DT, PWM lookup table or
statically assigned by the driver) and the current ones.
Make use of those helpers where appropriate.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/input/misc/max77693-haptic.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-11-16 09:02:53
pwm_set/get_default_xxx() helpers have been introduced to differentiate
the default PWM states (those retrieved through DT, PWM lookup table or
statically assigned by the driver) and the current ones.
Make use of those helpers where appropriate.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Boris Brezillon <hidden> Date: 2015-11-16 09:03:02
pwm_set/get_default_xxx() helpers have been introduced to differentiate
the default PWM states (those retrieved through DT, PWM lookup table or
statically assigned by the driver) and the current ones.
Make use of those helpers where appropriate.
Signed-off-by: Boris Brezillon <redacted>
Acked-by: Lee Jones <redacted>
---
drivers/video/backlight/lm3630a_bl.c | 4 ++--
drivers/video/backlight/pwm_bl.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-11-16 09:03:11
pwm_set/get_default_xxx() helpers have been introduced to differentiate
the default PWM states (those retrieved through DT, PWM lookup table or
statically assigned by the driver) and the current ones.
Make use of those helpers where appropriate.
Signed-off-by: Boris Brezillon <redacted>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
drivers/regulator/pwm-regulator.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-11-16 09:03:21
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.
PWM users might call the pwm_get_default_period to query the default
period value. There is currently no helper to query the default
polarity, but it might be added later on if there is a need for it.
Signed-off-by: Boris Brezillon <redacted>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
include/linux/pwm.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
From: Boris Brezillon <hidden> Date: 2015-11-16 09:03:29
pwm_set/get_default_xxx() helpers have been introduced to differentiate
the default PWM states (those retrieved through DT, PWM lookup table or
statically assigned by the driver) and the current ones.
Make use of those helpers where appropriate.
Signed-off-by: Boris Brezillon <redacted>
Acked-by: Jacek Anaszewski <redacted>
---
drivers/leds/leds-pwm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Boris Brezillon <hidden> Date: 2015-11-16 09:05:21
pwm_set/get_default_xxx() helpers have been introduced to differentiate
the default PWM states (those retrieved through DT, PWM lookup table or
statically assigned by the driver) and the current PWM ones.
Make use of those helpers where appropriate.
Signed-off-by: Boris Brezillon <redacted>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Alexandre Belloni <redacted>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
drivers/pwm/core.c | 14 +++++++-------
drivers/pwm/pwm-pxa.c | 2 +-
drivers/pwm/pwm-sun4i.c | 3 ++-
3 files changed, 10 insertions(+), 9 deletions(-)
@@ -71,22 +71,23 @@ static int clk_pwm_probe(struct platform_device *pdev)if(IS_ERR(pwm))returnPTR_ERR(pwm);-if(!pwm->period){+if(!pwm_get_period((pwm))){dev_err(&pdev->dev,"invalid PWM period\n");return-EINVAL;}if(of_property_read_u32(node,"clock-frequency",&clk_pwm->fixed_rate))-clk_pwm->fixed_rate=NSEC_PER_SEC/pwm->period;+clk_pwm->fixed_rate=NSEC_PER_SEC/pwm_get_period((pwm));-if(pwm->period!=NSEC_PER_SEC/clk_pwm->fixed_rate&&-pwm->period!=DIV_ROUND_UP(NSEC_PER_SEC,clk_pwm->fixed_rate)){+if(pwm_get_period((pwm))!=NSEC_PER_SEC/clk_pwm->fixed_rate&&+pwm_get_period((pwm))!=DIV_ROUND_UP(NSEC_PER_SEC,clk_pwm->fixed_rate)){dev_err(&pdev->dev,"clock-frequency does not match PWM period\n");return-EINVAL;}-ret=pwm_config(pwm,(pwm->period+1)>>1,pwm->period);+ret=pwm_config(pwm,(pwm_get_period((pwm))+1)>>1,+pwm_get_period((pwm)));if(ret<0)returnret;
From: Mark Brown <broonie@kernel.org> Date: 2015-11-16 10:57:25
On Mon, Nov 16, 2015 at 09:56:32AM +0100, Boris Brezillon wrote:
quoted hunk
+++ b/drivers/regulator/pwm-regulator.c
@@ -56,7 +56,7 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,intdutycycle;intret;-pwm_reg_period=pwm_get_period(drvdata->pwm);+pwm_reg_period=pwm_get_default_period(drvdata->pwm);dutycycle=(pwm_reg_period*drvdata->duty_cycle_table[selector].dutycycle)/100;
It's not clear to me that we're not looking for the current period here
or in the other use. Won't configuring based on a period other than the
one that has been set give the wrong answer?
From: Boris Brezillon <hidden> Date: 2015-11-16 12:24:08
Hi Mark,
On Mon, 16 Nov 2015 10:55:58 +0000
Mark Brown [off-list ref] wrote:
On Mon, Nov 16, 2015 at 09:56:32AM +0100, Boris Brezillon wrote:
quoted
+++ b/drivers/regulator/pwm-regulator.c
@@ -56,7 +56,7 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,intdutycycle;intret;-pwm_reg_period=pwm_get_period(drvdata->pwm);+pwm_reg_period=pwm_get_default_period(drvdata->pwm);dutycycle=(pwm_reg_period*drvdata->duty_cycle_table[selector].dutycycle)/100;
It's not clear to me that we're not looking for the current period here
or in the other use. Won't configuring based on a period other than the
one that has been set give the wrong answer?
Hm, maybe that's naming problem. What I call the 'default' period here
is actually the period configured in your board file (using a PWM lookup
table) or your DT. This value represent the period requested by the PWM
user not a default value specified by the PWM chip driver.
The reason we're not using the 'current' period value is because it may
have been set by the bootloader, and may be inappropriate for our use
case (ie. the period may be to small to represent the different
voltages).
ITOH, we're using the current period value when calculating the current
voltage, because we want to get the correct voltage value, and the PWM
device may still use the configuration set by the bootloader (not the
default one specified in your board or DT files).
I hope this clarifies the differences between the current and default
period, and why we should use the default value here.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Double parentheses over argument are not needed so just:
pwm_get_period(haptic->pwm_dev) + ...
Beside that patch looks good, so with removing parentheses here and below:
Reviewed-by: Krzysztof Kozlowski <redacted>
Best regards,
Krzysztof
quoted hunk
int error;
- error = pwm_config(haptic->pwm_dev, delta, haptic->pwm_dev->period);
+ error = pwm_config(haptic->pwm_dev, delta,
+ pwm_get_period((haptic->pwm_dev)));
if (error) {
dev_err(haptic->dev, "failed to configure pwm: %d\n", error);
return error;
@@ -245,7 +246,7 @@ static int max77693_haptic_play_effect(struct input_dev *dev, void *data, * The formula to convert magnitude to pwm_duty as follows: * - pwm_duty = (magnitude * pwm_period) / MAX_MAGNITUDE(0xFFFF) */- period_mag_multi = (u64)haptic->pwm_dev->period * haptic->magnitude;+ period_mag_multi = (u64)pwm_get_period((haptic->pwm_dev)) * haptic->magnitude; haptic->pwm_duty = (unsigned int)(period_mag_multi >> MAX_MAGNITUDE_SHIFT);
Hi Boris,
Am Montag, 16. November 2015, 09:56:23 schrieb Boris Brezillon:
Hello,
This series adds support for atomic PWM update, or IOW, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
you already include my tested-by tags, but anyway, the series still works
nicely on 4.4-rc1 on my veyron chromebook.
Heiko
Double parentheses over argument are not needed so just:
pwm_get_period(haptic->pwm_dev) + ...
Actually it was generated with coccinelle, hence I didn't fix existing
coding style issues, but I have no problem fixing them.
There was no existing coding style issue. Your coccinelle script introduces it.
You might want to consider updating your script and remove the unnecessary (( ))
from it.
Guenter
Thanks,
Boris
quoted
Beside that patch looks good, so with removing parentheses here and below:
Reviewed-by: Krzysztof Kozlowski <redacted>
Best regards,
Krzysztof
quoted
int error;
- error = pwm_config(haptic->pwm_dev, delta, haptic->pwm_dev->period);
+ error = pwm_config(haptic->pwm_dev, delta,
+ pwm_get_period((haptic->pwm_dev)));
if (error) {
dev_err(haptic->dev, "failed to configure pwm: %d\n", error);
return error;
@@ -245,7 +246,7 @@ static int max77693_haptic_play_effect(struct input_dev *dev, void *data, * The formula to convert magnitude to pwm_duty as follows: * - pwm_duty = (magnitude * pwm_period) / MAX_MAGNITUDE(0xFFFF) */- period_mag_multi = (u64)haptic->pwm_dev->period * haptic->magnitude;+ period_mag_multi = (u64)pwm_get_period((haptic->pwm_dev)) * haptic->magnitude; haptic->pwm_duty = (unsigned int)(period_mag_multi >> MAX_MAGNITUDE_SHIFT);
Use pwm_get_xxx() helpers instead of directly accessing the pwm->xxx field.
Doing that will ease adaptation of the PWM framework to support atomic
update.
Signed-off-by: Boris Brezillon <redacted>
---
Patch generated with the following coccinelle script:
--->8---
virtual patch
@@
struct pwm_device *p;
expression e;
@@
(
-(p)->polarity = e;
+pwm_set_polarity((p), e);
|
-(p)->polarity
+pwm_get_polarity((p))
Double parentheses over argument are not needed so just:
pwm_get_period(haptic->pwm_dev) + ...
Actually it was generated with coccinelle, hence I didn't fix existing
coding style issues, but I have no problem fixing them.
There was no existing coding style issue. Your coccinelle script introduces it.
You might want to consider updating your script and remove the unnecessary (( ))
from it.
My bad, you are right: my script is buggy. I'll fix that.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
pwm_set/get_default_xxx() helpers have been introduced to differentiate
the default PWM states (those retrieved through DT, PWM lookup table or
statically assigned by the driver) and the current ones.
Make use of those helpers where appropriate.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/hwmon/pwm-fan.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-11-16 16:53:30
On Mon, 16 Nov 2015 07:59:23 -0800
Guenter Roeck [off-list ref] wrote:
On 11/16/2015 12:56 AM, Boris Brezillon wrote:
quoted
Use pwm_get_xxx() helpers instead of directly accessing the pwm->xxx field.
Doing that will ease adaptation of the PWM framework to support atomic
update.
Signed-off-by: Boris Brezillon <redacted>
---
Patch generated with the following coccinelle script:
--->8---
virtual patch
@@
struct pwm_device *p;
expression e;
@@
(
-(p)->polarity = e;
+pwm_set_polarity((p), e);
|
-(p)->polarity
+pwm_get_polarity((p))
I don't get this one. You mean I should drop one the parenthesis around
p, right?
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Mon, 16 Nov 2015 07:59:23 -0800
Guenter Roeck [off-list ref] wrote:
quoted
On 11/16/2015 12:56 AM, Boris Brezillon wrote:
quoted
Use pwm_get_xxx() helpers instead of directly accessing the pwm->xxx field.
Doing that will ease adaptation of the PWM framework to support atomic
update.
Signed-off-by: Boris Brezillon <redacted>
---
Patch generated with the following coccinelle script:
--->8---
virtual patch
@@
struct pwm_device *p;
expression e;
@@
(
-(p)->polarity = e;
+pwm_set_polarity((p), e);
|
-(p)->polarity
+pwm_get_polarity((p))
I don't get this one. You mean I should drop one the parenthesis around
p, right?
Same as above - s/(p)/p/. It should never be necessary to write
pwm_set_duty_cycle((p), e)
since
pwm_set_duty_cycle(p, e)
should be the same.
On the other side, I did not see this expression used in any of the patches,
though maybe I missed it.
Thanks,
Guenter
What is the deal with the double parentheses?
Think I saw that in some of the other patches as well.
It comes from a typo in my coccinelle script. I already fixed it and
regenerated the faulty patches, so please ignore this aspect while
reviewing (this will be addressed in the next version).
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
It's not clear to me that we're not looking for the current period here
or in the other use. Won't configuring based on a period other than the
one that has been set give the wrong answer?
Hm, maybe that's naming problem. What I call the 'default' period here
is actually the period configured in your board file (using a PWM lookup
table) or your DT. This value represent the period requested by the PWM
user not a default value specified by the PWM chip driver.
The reason we're not using the 'current' period value is because it may
have been set by the bootloader, and may be inappropriate for our use
case (ie. the period may be to small to represent the different
voltages).
ITOH, we're using the current period value when calculating the current
voltage, because we want to get the correct voltage value, and the PWM
device may still use the configuration set by the bootloader (not the
default one specified in your board or DT files).
I hope this clarifies the differences between the current and default
period, and why we should use the default value here.
To be honest I'm still a bit confused here. When do we actually apply
the default setting and why do we keep on having to constantly override
it rather than doing this once at boot? It feels wrong to be using it
every time we set anything. I'd expect it to be something we only need
to do at probe time or which would automatically be handled by the PWM
framework (but that'd have issues changing the state and potentially
breaking things if done in an uncoordiated fashion).
It's not clear to me that we're not looking for the current period here
or in the other use. Won't configuring based on a period other than the
one that has been set give the wrong answer?
quoted
Hm, maybe that's naming problem. What I call the 'default' period here
is actually the period configured in your board file (using a PWM lookup
table) or your DT. This value represent the period requested by the PWM
user not a default value specified by the PWM chip driver.
quoted
The reason we're not using the 'current' period value is because it may
have been set by the bootloader, and may be inappropriate for our use
case (ie. the period may be to small to represent the different
voltages).
quoted
ITOH, we're using the current period value when calculating the current
voltage, because we want to get the correct voltage value, and the PWM
device may still use the configuration set by the bootloader (not the
default one specified in your board or DT files).
quoted
I hope this clarifies the differences between the current and default
period, and why we should use the default value here.
To be honest I'm still a bit confused here. When do we actually apply
the default setting and why do we keep on having to constantly override
it rather than doing this once at boot?
That's why I said the 'default' name may be inappropriate. The
default values are actually never directly applied by the PWM framework.
It's the default value for a specific PWM user, so it can be applied by
the PWM user when he wants. It's more here as a reference, nothing
forces the PWM user to use this specific value.
It feels wrong to be using it
every time we set anything. I'd expect it to be something we only need
to do at probe time or which would automatically be handled by the PWM
framework (but that'd have issues changing the state and potentially
breaking things if done in an uncoordiated fashion).
The whole point of this series is to smoothly take over the bootloader
config. This is why we are keeping the PWM untouched until someone
really wants to change the regulator output. We should be able to apply
the 'default' PWM period when probing the device, but this means first
extracting the current voltage from the PWM state and then applying a
new dutycycle and the default period in a single operation. Not sure
it's worth the trouble.
Doing it in the PWM framework is not really possible, because the PWM
lookup table and DT definitions are only defining the 'default' period
value not the 'default' dutycycle, and applying that automatically when
requesting the PWM means generating a glitch on the PWM signal
(dutycycle will be set to 0 until the user changes it using
pwm_config() or pwm_apply_state()) which is exactly what we're trying to
solve here.
Also, note that you have to pass the period anyway when configuring the
PWM, so passing the default one or the current one should be pretty
much the same in term of performances (unless the PWM driver is able
to optimize its setting if the period does not change).
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Mon, Nov 16, 2015 at 09:56:28AM +0100, Boris Brezillon wrote:
Use pwm_get_xxx() helpers instead of directly accessing the pwm->xxx field.
Doing that will ease adaptation of the PWM framework to support atomic
update.
Signed-off-by: Boris Brezillon <redacted>
Could you please tag input-related bits with "Input: driver - ... "
instead of "misc: ... "?
Thanks!
--
Dmitry
It's not clear to me that we're not looking for the current period here
or in the other use. Won't configuring based on a period other than the
one that has been set give the wrong answer?
quoted
Hm, maybe that's naming problem. What I call the 'default' period here
is actually the period configured in your board file (using a PWM lookup
table) or your DT. This value represent the period requested by the PWM
user not a default value specified by the PWM chip driver.
quoted
The reason we're not using the 'current' period value is because it may
have been set by the bootloader, and may be inappropriate for our use
case (ie. the period may be to small to represent the different
voltages).
quoted
ITOH, we're using the current period value when calculating the current
voltage, because we want to get the correct voltage value, and the PWM
device may still use the configuration set by the bootloader (not the
default one specified in your board or DT files).
quoted
I hope this clarifies the differences between the current and default
period, and why we should use the default value here.
To be honest I'm still a bit confused here. When do we actually apply
the default setting and why do we keep on having to constantly override
it rather than doing this once at boot? It feels wrong to be using it
every time we set anything. I'd expect it to be something we only need
to do at probe time or which would automatically be handled by the PWM
framework (but that'd have issues changing the state and potentially
breaking things if done in an uncoordiated fashion).
Thierry, I didn't hear from you after the long discussion we had on IRC
a few weeks ago.
The conclusion of this discussion was that using
pwm_get_default_period() was not acceptable (even after renaming it
differently, like pwm_get_reference_period()), because it was
disturbing to get the default/reference period each time we wanted to
configure the PWM differently.
Another suggestion was to automatically reconfigure the PWM duty_ns
value based on the initial PWM state (retrieved through hardware
readout) and the default period value (specified in the PWM lookup table
or the DT). But this implied supporting hardware readout in all PWM
drivers, which prevents a smooth migration to this new approach.
I also proposed to provide helpers to hide the duty cycle to active
time calculation in the PWM core, so that PWM users just have to choose
their scale (percent, or any other custom scale) and set their duty
cycle based on this scale instead of specifying an active/on time in
nanosecond. You didn't seem to like this idea, but I gave it a try
(see here [1]), and think it might be worth looking at it.
The commit you should look at are [2], [3] and [4], and the idea is to
clarify the notion of duty-cycle, which, according to wikipedia [5]
(and a lot of other references) is supposed to be expressed in a
relative unit (percent, or any other scale as said earlier).
After renaming the pwm_set/get_duty_cycle() helpers into
pwm_set/get_active_time() we can define a new pwm_set_duty_cycle()
helper to let the PWM user configure its PWM device relatively to a
chosen scale, without asking him to choose the PWM period (the
conversion is done based on the default/reference period).
The pwm_get_duty_cycle() is doing the reverse conversion: it returns the
duty-cycle expressed relatively to the scale (here the current PWM
period is used to handle the case where the PWM user hasn't configure
the PWM yet, but want to retrieve the current duty-cycle extracted from
hardware readout).
Please let me know what you think of this approach, and if you're happy
with it I'll rework my series accordingly.
Best Regards,
Boris
[1]https://github.com/bbrezillon/linux-rk/commits/atomic-pwm-alt
[2]https://github.com/bbrezillon/linux-rk/commit/d7d4d04e147d4ec349c59f70e141877661930c6d
[3]https://github.com/bbrezillon/linux-rk/commit/66ce78f308f3eb1a9c536689352f208fd51c9030
[4]https://github.com/bbrezillon/linux-rk/commit/07882a2dd21f0d17d83640ff55204cc7a7d4c8f7
[5]https://en.wikipedia.org/wiki/Duty_cycle
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
From: Boris Brezillon <hidden> Date: 2015-12-30 11:01:32
Hi Thierry,
I'm trying to get these "atomic PWM config" and "initial PWM state
retrieval" stuff in for at least 3 releases. I can understand that some
things have to be discussed and reworked in order to be good enough for
mainline, but that's not what I'm seeing here.
You and Mark raised some concerns about the usage of the
pwm_{get,set}_default_xxx() helpers which I tried to address by
proposing something else. I asked you to comment on it a few weeks ago,
but you never did.
You also told me that you would search for an alternative solution, but
never came back to me.
So I think it's now time to take a decision, whether you want to take
this series with some minor reworks (changing function names to clarify
what is a default and current PWM state), or decide that you expect
something else (but in that case I'd like you to explain what you want).
And by the way, the behavior you're complaining about is already
currently in place: even if the pwm_get_period() function does not
contain the 'default' word in its name, what's actually returned is the
default (or reference) period (the one retrieved from the DT or the PWM
lookup table) not the period currently in use on the PWM device (the
same goes for other helpers).
Can we please settle on something for 4.6 so that I can repost a series
when 4.5-rc1 is out?
Thanks,
Boris
On Wed, 2 Dec 2015 11:37:20 +0100
Boris Brezillon [off-list ref] wrote:
Hi,
On Mon, 16 Nov 2015 18:42:38 +0000
Mark Brown [off-list ref] wrote:
quoted
On Mon, Nov 16, 2015 at 01:23:59PM +0100, Boris Brezillon wrote:
quoted
Mark Brown [off-list ref] wrote:
quoted
On Mon, Nov 16, 2015 at 09:56:32AM +0100, Boris Brezillon wrote:
It's not clear to me that we're not looking for the current period here
or in the other use. Won't configuring based on a period other than the
one that has been set give the wrong answer?
quoted
Hm, maybe that's naming problem. What I call the 'default' period here
is actually the period configured in your board file (using a PWM lookup
table) or your DT. This value represent the period requested by the PWM
user not a default value specified by the PWM chip driver.
quoted
The reason we're not using the 'current' period value is because it may
have been set by the bootloader, and may be inappropriate for our use
case (ie. the period may be to small to represent the different
voltages).
quoted
ITOH, we're using the current period value when calculating the current
voltage, because we want to get the correct voltage value, and the PWM
device may still use the configuration set by the bootloader (not the
default one specified in your board or DT files).
quoted
I hope this clarifies the differences between the current and default
period, and why we should use the default value here.
To be honest I'm still a bit confused here. When do we actually apply
the default setting and why do we keep on having to constantly override
it rather than doing this once at boot? It feels wrong to be using it
every time we set anything. I'd expect it to be something we only need
to do at probe time or which would automatically be handled by the PWM
framework (but that'd have issues changing the state and potentially
breaking things if done in an uncoordiated fashion).
Thierry, I didn't hear from you after the long discussion we had on IRC
a few weeks ago.
The conclusion of this discussion was that using
pwm_get_default_period() was not acceptable (even after renaming it
differently, like pwm_get_reference_period()), because it was
disturbing to get the default/reference period each time we wanted to
configure the PWM differently.
Another suggestion was to automatically reconfigure the PWM duty_ns
value based on the initial PWM state (retrieved through hardware
readout) and the default period value (specified in the PWM lookup table
or the DT). But this implied supporting hardware readout in all PWM
drivers, which prevents a smooth migration to this new approach.
I also proposed to provide helpers to hide the duty cycle to active
time calculation in the PWM core, so that PWM users just have to choose
their scale (percent, or any other custom scale) and set their duty
cycle based on this scale instead of specifying an active/on time in
nanosecond. You didn't seem to like this idea, but I gave it a try
(see here [1]), and think it might be worth looking at it.
The commit you should look at are [2], [3] and [4], and the idea is to
clarify the notion of duty-cycle, which, according to wikipedia [5]
(and a lot of other references) is supposed to be expressed in a
relative unit (percent, or any other scale as said earlier).
After renaming the pwm_set/get_duty_cycle() helpers into
pwm_set/get_active_time() we can define a new pwm_set_duty_cycle()
helper to let the PWM user configure its PWM device relatively to a
chosen scale, without asking him to choose the PWM period (the
conversion is done based on the default/reference period).
The pwm_get_duty_cycle() is doing the reverse conversion: it returns the
duty-cycle expressed relatively to the scale (here the current PWM
period is used to handle the case where the PWM user hasn't configure
the PWM yet, but want to retrieve the current duty-cycle extracted from
hardware readout).
Please let me know what you think of this approach, and if you're happy
with it I'll rework my series accordingly.
Best Regards,
Boris
[1]https://github.com/bbrezillon/linux-rk/commits/atomic-pwm-alt
[2]https://github.com/bbrezillon/linux-rk/commit/d7d4d04e147d4ec349c59f70e141877661930c6d
[3]https://github.com/bbrezillon/linux-rk/commit/66ce78f308f3eb1a9c536689352f208fd51c9030
[4]https://github.com/bbrezillon/linux-rk/commit/07882a2dd21f0d17d83640ff55204cc7a7d4c8f7
[5]https://en.wikipedia.org/wiki/Duty_cycle
@@ -71,22 +71,23 @@ static int clk_pwm_probe(struct platform_device *pdev)if(IS_ERR(pwm))returnPTR_ERR(pwm);-if(!pwm->period){+if(!pwm_get_period((pwm))){
The change itself looks fine, but the semantic patch added extra parens.
Can you remove them? After doing so feel free to add:
Acked-by: Michael Turquette <redacted>
From: Michael Turquette <hidden> Date: 2015-12-30 21:05:19
Hi Boris,
Quoting Boris Brezillon (2015-11-16 00:56:37)
quoted hunk
pwm_set/get_default_xxx() helpers have been introduced to differentiate
the default PWM states (those retrieved through DT, PWM lookup table or
statically assigned by the driver) and the current ones.
Make use of those helpers where appropriate.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/clk/clk-pwm.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -71,23 +71,23 @@ static int clk_pwm_probe(struct platform_device *pdev)if(IS_ERR(pwm))returnPTR_ERR(pwm);-if(!pwm_get_period((pwm))){+if(!pwm_get_default_period((pwm))){
The change itself looks fine, but the semantic patch added extra parens.
Can you remove them? After doing so feel free to add:
Acked-by: Michael Turquette <redacted>
From: Boris Brezillon <hidden> Date: 2016-03-16 14:30:30
Hi Thierry,
Can you please apply this patch?
It's completely independent from the rest of the series.
Thanks,
Boris
On Mon, 16 Nov 2015 09:56:24 +0100
Boris Brezillon [off-list ref] wrote:
quoted hunk
Commit 5c31252c4a86 ("pwm: Add the pwm_is_enabled() helper") introduced a
new function to test whether a PWM device is enabled or not without
manipulating PWM internal fields.
Hiding this is necessary if we want to smoothly move to the atomic PWM
config approach without impacting PWM drivers.
Fix this driver to use pwm_is_enabled() instead of directly accessing the
->flags field.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/pwm/pwm-rcar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -157,7 +157,7 @@ static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,returndiv;/* Let the core driver set pwm->period if disabled and duty_ns = 0 */-if(!test_bit(PWMF_ENABLED,&pwm->flags)&&!duty_ns)+if(!pwm_is_enabled(pwm)&&!duty_ns)return0;rcar_pwm_update(rp,RCAR_PWMCR_SYNC,RCAR_PWMCR_SYNC,RCAR_PWMCR);