From: Boris Brezillon <hidden> Date: 2015-07-01 08:21:46
Hello Thierry,
This series adds support for atomic PWM update, or ITO, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
This implementation is still experimental, and I may have missed some key
aspect, so any feedback are welcome.
Also note that I haven't protected the state update with any locking.
That's because the existing config does not protect against concurrent
access to a requested PWM device (see the pwm_config implementation).
I guess the PWM framework assume the user will implement the proper locking
scheme if it has to concurrently access the device.
The 5 first patches prepare the addition of the pwm_state concept, which
will be used to allow atomic updates.
The following patches introduce the pwm_state struct, initial state
retrieval and atomic update concepts.
Patches 12 and 13 are showing how one can implement the initial state
retrieval and atomic update features in a PWM driver (in this specific
case I implemented it in the rockchip driver).
The last 2 patches are making use of those changes to improve the
pwm-regulator driver (initializing the regulator state based on the
initial PWM state).
Best Regards,
Boris
Boris Brezillon (15):
pwm: add the pwm_is_enabled() helper
pwm: fix pwm_get_period and pwm_get_duty_cycle prototypes
pwm: add pwm_get_polarity helper function
pwm: make use of pwm_get_xxx helpers where appropriate
pwm: introduce default period and polarity concepts
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: implement ->enable(), ->disable() and ->is_enabled
methods
regulator: pwm: properly initialize the ->state field
drivers/leds/leds-pwm.c | 2 +-
drivers/pwm/core.c | 136 ++++++++++++++++++++++++++++++-----
drivers/pwm/pwm-atmel-tcb.c | 2 +-
drivers/pwm/pwm-atmel.c | 6 +-
drivers/pwm/pwm-bcm-kona.c | 7 +-
drivers/pwm/pwm-ep93xx.c | 4 +-
drivers/pwm/pwm-imx.c | 5 +-
drivers/pwm/pwm-mxs.c | 4 +-
drivers/pwm/pwm-pxa.c | 2 +-
drivers/pwm/pwm-renesas-tpu.c | 2 +-
drivers/pwm/pwm-rockchip.c | 122 ++++++++++++++++++++++++-------
drivers/pwm/pwm-sun4i.c | 3 +-
drivers/pwm/pwm-tegra.c | 6 +-
drivers/pwm/pwm-tiecap.c | 10 +--
drivers/pwm/pwm-tiehrpwm.c | 6 +-
drivers/pwm/sysfs.c | 13 ++--
drivers/regulator/pwm-regulator.c | 60 ++++++++++++++--
drivers/video/backlight/lm3630a_bl.c | 4 +-
drivers/video/backlight/pwm_bl.c | 6 +-
drivers/video/fbdev/ssd1307fb.c | 2 +-
include/linux/pwm.h | 82 ++++++++++++++++++---
21 files changed, 380 insertions(+), 104 deletions(-)
--
1.9.1
@@ -347,7 +347,7 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,tcbpwm->duty=duty;/* If the PWM is enabled, call enable to apply the new conf */-if(test_bit(PWMF_ENABLED,&pwm->flags))+if(pwm_is_enabled(pwm))atmel_tcb_pwm_enable(chip,pwm);return0;
@@ -134,7 +134,7 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,}/* If the PWM channel is enabled, write the settings to the HW */-if(test_bit(PWMF_ENABLED,&pwm->flags)){+if(pwm_is_enabled(pwm)){value=readl(kp->base+PRESCALE_OFFSET);value&=~PRESCALE_MASK(chan);value|=prescale<<PRESCALE_SHIFT(chan);
@@ -287,7 +287,7 @@ static int kona_pwmc_remove(struct platform_device *pdev)unsignedintchan;for(chan=0;chan<kp->chip.npwm;chan++)-if(test_bit(PWMF_ENABLED,&kp->chip.pwms[chan].flags))+if(pwm_is_enabled(&kp->chip.pwms[chan]))clk_disable_unprepare(kp->clk);returnpwmchip_remove(&kp->chip);
@@ -301,7 +301,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm,pwm->duty=duty;/* If the channel is disabled we're done. */-if(!test_bit(PWMF_ENABLED,&_pwm->flags))+if(!pwm_is_enabled(_pwm))return0;if(duty_only&&pwm->timer_on){
@@ -97,7 +97,7 @@ static int ecap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,writew(reg_val,pc->mmio_base+ECCTL2);-if(!test_bit(PWMF_ENABLED,&pwm->flags)){+if(!pwm_is_enabled(pwm)){/* Update active registers if not running */writel(duty_cycles,pc->mmio_base+CAP2);writel(period_cycles,pc->mmio_base+CAP1);
@@ -111,7 +111,7 @@ static int ecap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,writel(period_cycles,pc->mmio_base+CAP3);}-if(!test_bit(PWMF_ENABLED,&pwm->flags)){+if(!pwm_is_enabled(pwm)){reg_val=readw(pc->mmio_base+ECCTL2);/* Disable APWM mode to put APWM output Low */reg_val&=~ECCTL2_APWM_MODE;
From: Boris Brezillon <hidden> Date: 2015-07-01 08:21:48
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 <redacted>
---
include/linux/pwm.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-07-01 08:21:49
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 <redacted>
---
include/linux/pwm.h | 5 +++++
1 file changed, 5 insertions(+)
From: Boris Brezillon <hidden> Date: 2015-07-01 08:21:50
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 <redacted>
---
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(-)
From: Boris Brezillon <hidden> Date: 2015-07-01 08:21:51
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.
This patch also modifies all the places where the default helpers should
be used in place of the standard ones.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/leds/leds-pwm.c | 2 +-
drivers/pwm/core.c | 14 +++++++-------
drivers/pwm/pwm-pxa.c | 2 +-
drivers/pwm/pwm-sun4i.c | 3 ++-
drivers/regulator/pwm-regulator.c | 2 +-
drivers/video/backlight/lm3630a_bl.c | 4 ++--
drivers/video/backlight/pwm_bl.c | 2 +-
drivers/video/fbdev/ssd1307fb.c | 2 +-
include/linux/pwm.h | 15 +++++++++++++++
9 files changed, 31 insertions(+), 15 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-07-01 08:21:52
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 | 20 ++++++++++++--------
2 files changed, 15 insertions(+), 11 deletions(-)
@@ -431,8 +431,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-07-01 08:21:53
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>
---
drivers/pwm/core.c | 15 ++++++++++++---
include/linux/pwm.h | 6 +++---
2 files changed, 15 insertions(+), 6 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-07-01 08:21:54
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>
---
drivers/video/backlight/pwm_bl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-07-01 08:21:55
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>
---
include/linux/pwm.h | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-07-01 08:21:56
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 <redacted>
---
drivers/pwm/core.c | 3 +++
include/linux/pwm.h | 2 ++
2 files changed, 5 insertions(+)
From: Boris Brezillon <hidden> Date: 2015-07-01 08:21:57
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 <redacted>
---
drivers/pwm/core.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
include/linux/pwm.h | 26 +++++++++++++
2 files changed, 124 insertions(+), 12 deletions(-)
@@ -238,8 +238,9 @@ 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->ops->apply&&+(!chip->ops->config||!chip->ops->enable||+!chip->ops->disable))||!chip->npwm)return-EINVAL;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){+structpwm_statestate=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-07-01 08:21:59
Implement the ->apply() function to add support for atomic update.
Signed-off-by: Boris Brezillon <redacted>
---
drivers/pwm/pwm-rockchip.c | 75 ++++++++++++++++++++++++++++++----------------
1 file changed, 49 insertions(+), 26 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-07-01 08:22:00
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 <redacted>
---
drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
From: Boris Brezillon <hidden> Date: 2015-07-01 08:22:01
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>
---
drivers/regulator/pwm-regulator.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
quoted hunk
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 <redacted>
---
drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/pwm-regulator.c
b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
From: Boris Brezillon <hidden> Date: 2015-07-01 12:05:31
Hi Heiko,
On Wed, 01 Jul 2015 13:58:09 +0200
Heiko St?bner [off-list ref] wrote:
Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
quoted
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 <redacted>
---
drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/pwm-regulator.c
b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
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 ;-)
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Am Mittwoch, 1. Juli 2015, 14:05:31 schrieb Boris Brezillon:
Hi Heiko,
On Wed, 01 Jul 2015 13:58:09 +0200
Heiko St?bner [off-list ref] wrote:
quoted
Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
quoted
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 <redacted>
---
drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/pwm-regulator.c
b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
Am Mittwoch, 1. Juli 2015, 14:05:31 schrieb Boris Brezillon:
quoted
Hi Heiko,
On Wed, 01 Jul 2015 13:58:09 +0200
Heiko St?bner [off-list ref] wrote:
quoted
Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
quoted
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 <redacted>
---
drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/pwm-regulator.c
b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
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 ;-)
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 :-)
Also note that I haven't tested the series on a real board (just compile
tested) because I don't have the board with me right now, but I wanted
to post the RFC early so that we can discuss the concepts.
Anyway, any feedback on the implementation (including bug reports) is
welcome.
This is the version I actually tested on the veyron board:
https://github.com/bbrezillon/linux-rk/tree/rk-3.14
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
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
- enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
+ enable_conf = PWM_DUTY_POSITIVE;
- if ((val & enable_conf) == enable_conf)
+ if ((val & enable_conf) != enable_conf)
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>
---
might be nice to have too ;-)
drivers/pwm/core.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
Hi Boris,
Am Mittwoch, 1. Juli 2015, 10:21:46 schrieb Boris Brezillon:
Hello Thierry,
This series adds support for atomic PWM update, or ITO, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
This implementation is still experimental, and I may have missed some key
aspect, so any feedback are welcome.
Also note that I haven't protected the state update with any locking.
That's because the existing config does not protect against concurrent
access to a requested PWM device (see the pwm_config implementation).
I guess the PWM framework assume the user will implement the proper locking
scheme if it has to concurrently access the device.
The 5 first patches prepare the addition of the pwm_state concept, which
will be used to allow atomic updates.
The following patches introduce the pwm_state struct, initial state
retrieval and atomic update concepts.
Patches 12 and 13 are showing how one can implement the initial state
retrieval and atomic update features in a PWM driver (in this specific
case I implemented it in the rockchip driver).
The last 2 patches are making use of those changes to improve the
pwm-regulator driver (initializing the regulator state based on the
initial PWM state).
at first I got very strange readings (very wrong values and wrong polarity),
which resulted from the issues I pointed out in the replies to individual
patches. After fixing these, the pwm read-back now returns exactly the expected
values :-) .
And with the original voltage table from the Chromeos-devicetrees, the pwm-
regulator also returns the expected 1.2V that coreboot initially set.
Heiko
On Wed, Jul 01, 2015 at 10:21:51AM +0200, Boris Brezillon wrote:
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.
Would it make sense to put the prototypes of
pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
then?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello Boris,
On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
This series adds support for atomic PWM update, or ITO, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
on first reading the subject of your series I thought it was about
asserting that the newly set config is active before the call to
pwm_config (et al) returns. That's a problem I addressed a few times in
the past. I wonder if it's only me or if a different wording should be
used for "update all parameters with a single function call".
But other than that I like the series.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
From: Tomi Valkeinen <hidden> Date: 2015-07-02 07:17:00
On 02/07/15 10:03, Uwe Kleine-K?nig wrote:
Hello Boris,
On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
quoted
This series adds support for atomic PWM update, or ITO, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
on first reading the subject of your series I thought it was about
asserting that the newly set config is active before the call to
pwm_config (et al) returns. That's a problem I addressed a few times in
the past. I wonder if it's only me or if a different wording should be
used for "update all parameters with a single function call".
In my vocabulary "blocking" means that the work is done before the
function returns, and "atomic" means the work is done in one step.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150702/6327f9f4/attachment.sig>
From: Boris Brezillon <hidden> Date: 2015-07-02 07:30:13
Hi Uwe,
On Thu, 2 Jul 2015 09:03:43 +0200
Uwe Kleine-K?nig [off-list ref] wrote:
Hello Boris,
On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
quoted
This series adds support for atomic PWM update, or ITO, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
on first reading the subject of your series I thought it was about
asserting that the newly set config is active before the call to
pwm_config (et al) returns. That's a problem I addressed a few times in
the past. I wonder if it's only me or if a different wording should be
used for "update all parameters with a single function call".
Yep, I can reword it differently.
But other than that I like the series.
Great!
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Thu, Jul 02, 2015 at 10:17:00AM +0300, Tomi Valkeinen wrote:
On 02/07/15 10:03, Uwe Kleine-K?nig wrote:
quoted
Hello Boris,
On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
quoted
This series adds support for atomic PWM update, or ITO, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
on first reading the subject of your series I thought it was about
asserting that the newly set config is active before the call to
pwm_config (et al) returns. That's a problem I addressed a few times in
the past. I wonder if it's only me or if a different wording should be
used for "update all parameters with a single function call".
In my vocabulary "blocking" means that the work is done before the
function returns, and "atomic" means the work is done in one step.
blocking is IMHO something slightly different, maybe "synchronous" is a
good term for "done when the call returns".
For write(2) I'd say
- blocking means to only return when the write request has reached the
kernel, but not necessarily the medium. I.e. the caller doesn't need
to care further; and
- atomic means that the contents of two concurrent writers don't mix in
the resulting file content; and
- synchronous means that once write() returns the data is on the
medium.
So atomic seems to be fine to use here.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
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
Or just:
if(val & PWM_DUTY_POSITIVE)
Thanks for the fix.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
you're referencing the v2 init here, but only add it in the next patch?
[pwm: rockchip: add support for atomic update]
Yep, this function should be added in this patch.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Wed, Jul 01, 2015 at 10:21:51AM +0200, Boris Brezillon wrote:
quoted
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.
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 ?
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Hi Boris,
Am Mittwoch, 1. Juli 2015, 10:21:46 schrieb Boris Brezillon:
quoted
Hello Thierry,
This series adds support for atomic PWM update, or ITO, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
This implementation is still experimental, and I may have missed some key
aspect, so any feedback are welcome.
Also note that I haven't protected the state update with any locking.
That's because the existing config does not protect against concurrent
access to a requested PWM device (see the pwm_config implementation).
I guess the PWM framework assume the user will implement the proper locking
scheme if it has to concurrently access the device.
The 5 first patches prepare the addition of the pwm_state concept, which
will be used to allow atomic updates.
The following patches introduce the pwm_state struct, initial state
retrieval and atomic update concepts.
Patches 12 and 13 are showing how one can implement the initial state
retrieval and atomic update features in a PWM driver (in this specific
case I implemented it in the rockchip driver).
The last 2 patches are making use of those changes to improve the
pwm-regulator driver (initializing the regulator state based on the
initial PWM state).
at first I got very strange readings (very wrong values and wrong polarity),
which resulted from the issues I pointed out in the replies to individual
patches. After fixing these, the pwm read-back now returns exactly the expected
values :-) .
Sorry about that, as I said I only compile tested the series :-/.
Anyway, thanks for providing fixes for these bugs, they'll be applied
in the next version.
And with the original voltage table from the Chromeos-devicetrees, the pwm-
regulator also returns the expected 1.2V that coreboot initially set.
Great! And thanks for testing the patches.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
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>
---
might be nice to have too ;-)
I would print those values even if the PWM is not enabled.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
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>
---
now values are printed independent of the enable status :-)
drivers/pwm/core.c | 5 +++++
1 file changed, 5 insertions(+)
On Wed, Jul 01, 2015 at 10:22:01AM +0200, Boris Brezillon wrote:
The ->state field is currently initialized to 0, thus referencing the
voltage selector at index 0, which might not reflect the current voltage
value.
This looks like it'll need reworking on top of Lee's recent changes to
implement continuous voltage support for PWM regulators - we may be
calculating the values to set dynamically at runtime.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150714/e2e961c0/attachment.sig>
From: Boris Brezillon <hidden> Date: 2015-07-14 11:02:22
Hi Mark,
On Tue, 14 Jul 2015 11:50:19 +0100
Mark Brown [off-list ref] wrote:
On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote:
quoted
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.
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.
Thierry, Mark, let me know what you prefer.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
From: Boris Brezillon <hidden> Date: 2015-07-14 11:03:53
On Tue, 14 Jul 2015 11:51:55 +0100
Mark Brown [off-list ref] wrote:
On Wed, Jul 01, 2015 at 10:22:01AM +0200, Boris Brezillon wrote:
quoted
The ->state field is currently initialized to 0, thus referencing the
voltage selector at index 0, which might not reflect the current voltage
value.
This looks like it'll need reworking on top of Lee's recent changes to
implement continuous voltage support for PWM regulators - we may be
calculating the values to set dynamically at runtime.
Yep, I was planning on doing that in the next iteration, but I'd like
to have Thierry's feedback before sending a new version.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Tue, Jul 14, 2015 at 01:02:22PM +0200, Boris Brezillon wrote:
Mark Brown [off-list ref] wrote:
quoted
On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote:
quoted
Implement the ->enable(), ->disable() and ->is_enabled methods and remove
the PWM call in ->set_voltage_sel().
quoted
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.
From: Boris Brezillon <hidden> Date: 2015-07-14 11:16:28
On Tue, 14 Jul 2015 12:08:19 +0100
Mark Brown [off-list ref] wrote:
On Tue, Jul 14, 2015 at 01:02:22PM +0200, Boris Brezillon wrote:
quoted
Mark Brown [off-list ref] wrote:
quoted
quoted
On Wed, Jul 01, 2015 at 10:22:00AM +0200, Boris Brezillon wrote:
quoted
Implement the ->enable(), ->disable() and ->is_enabled methods and remove
the PWM call in ->set_voltage_sel().
quoted
quoted
This doesn't apply, please check and resend.
quoted
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?
Oh, indeed. This patch has no dependency on the PWM changes. I'll send
it on its own then.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
From: Boris Brezillon <hidden> Date: 2015-07-20 07:16:48
Hi Thierry,
I'd like to send a new version of this series fixing the problems
reported by Heiko, but I remember you were not happy with the naming
convention I have chosen for the atomic update function.
Could you have a quick look at this series (I'm not asking for a
detailed review) and let me know which things you'd like me to rename.
Best Regards,
Boris
On Wed, 1 Jul 2015 10:21:46 +0200
Boris Brezillon [off-list ref] wrote:
Hello Thierry,
This series adds support for atomic PWM update, or ITO, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
This implementation is still experimental, and I may have missed some key
aspect, so any feedback are welcome.
Also note that I haven't protected the state update with any locking.
That's because the existing config does not protect against concurrent
access to a requested PWM device (see the pwm_config implementation).
I guess the PWM framework assume the user will implement the proper locking
scheme if it has to concurrently access the device.
The 5 first patches prepare the addition of the pwm_state concept, which
will be used to allow atomic updates.
The following patches introduce the pwm_state struct, initial state
retrieval and atomic update concepts.
Patches 12 and 13 are showing how one can implement the initial state
retrieval and atomic update features in a PWM driver (in this specific
case I implemented it in the rockchip driver).
The last 2 patches are making use of those changes to improve the
pwm-regulator driver (initializing the regulator state based on the
initial PWM state).
Best Regards,
Boris
Boris Brezillon (15):
pwm: add the pwm_is_enabled() helper
pwm: fix pwm_get_period and pwm_get_duty_cycle prototypes
pwm: add pwm_get_polarity helper function
pwm: make use of pwm_get_xxx helpers where appropriate
pwm: introduce default period and polarity concepts
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: implement ->enable(), ->disable() and ->is_enabled
methods
regulator: pwm: properly initialize the ->state field
drivers/leds/leds-pwm.c | 2 +-
drivers/pwm/core.c | 136 ++++++++++++++++++++++++++++++-----
drivers/pwm/pwm-atmel-tcb.c | 2 +-
drivers/pwm/pwm-atmel.c | 6 +-
drivers/pwm/pwm-bcm-kona.c | 7 +-
drivers/pwm/pwm-ep93xx.c | 4 +-
drivers/pwm/pwm-imx.c | 5 +-
drivers/pwm/pwm-mxs.c | 4 +-
drivers/pwm/pwm-pxa.c | 2 +-
drivers/pwm/pwm-renesas-tpu.c | 2 +-
drivers/pwm/pwm-rockchip.c | 122 ++++++++++++++++++++++++-------
drivers/pwm/pwm-sun4i.c | 3 +-
drivers/pwm/pwm-tegra.c | 6 +-
drivers/pwm/pwm-tiecap.c | 10 +--
drivers/pwm/pwm-tiehrpwm.c | 6 +-
drivers/pwm/sysfs.c | 13 ++--
drivers/regulator/pwm-regulator.c | 60 ++++++++++++++--
drivers/video/backlight/lm3630a_bl.c | 4 +-
drivers/video/backlight/pwm_bl.c | 6 +-
drivers/video/fbdev/ssd1307fb.c | 2 +-
include/linux/pwm.h | 82 ++++++++++++++++++---
21 files changed, 380 insertions(+), 104 deletions(-)
On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
Hello Thierry,
This series adds support for atomic PWM update, or ITO, the capability
to update all the parameters of a PWM device (enabled/disabled, period,
duty and polarity) in one go.
This implementation is still experimental, and I may have missed some key
aspect, so any feedback are welcome.
Also note that I haven't protected the state update with any locking.
That's because the existing config does not protect against concurrent
access to a requested PWM device (see the pwm_config implementation).
I guess the PWM framework assume the user will implement the proper locking
scheme if it has to concurrently access the device.
Actually the reason why no protection is needed is because there can
ever only be a single user of a PWM channel. pwm_request() will only
hand out a handle to the PWM channel if it hasn't been requested
before.
And in the unlikely case where a single user wants to use the same PWM
channel from different threads, then it will have to use locking for
other reasons already, so accesses to the PWM can be serialized using
the same mechanism.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/5ba28c7a/attachment.sig>
On Wed, Jul 01, 2015 at 10:21:48AM +0200, Boris Brezillon wrote:
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 <redacted>
---
include/linux/pwm.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Applied with a slightly reworded commit message. The prototypes weren't
"broken" so there's nothing to "fix". Also s/pwm/PWM/.
Thanks,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/ddba6f65/attachment.sig>
On Wed, Jul 01, 2015 at 10:21:49AM +0200, Boris Brezillon wrote:
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 <redacted>
---
include/linux/pwm.h | 5 +++++
1 file changed, 5 insertions(+)
On Wed, Jul 01, 2015 at 10:21:50AM +0200, Boris Brezillon wrote:
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 <redacted>
---
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(-)
On Wed, Jul 01, 2015 at 10:21:51AM +0200, Boris Brezillon wrote:
quoted
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.
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 ?
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?
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/b044f07f/attachment.sig>
On Wed, Jul 01, 2015 at 10:21:53AM +0200, Boris Brezillon wrote:
quoted hunk
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>
---
drivers/pwm/core.c | 15 ++++++++++++---
include/linux/pwm.h | 6 +++---
2 files changed, 15 insertions(+), 6 deletions(-)
Technically there's now a race between the pwm_is_enabled() and
pwm->state.enabled = true; statements, but as discussed in the cover
letter I think that's fine because of the assumptions about concurrent
usage of PWMs.
The most important check (PWMF_REQUESTED) is still atomic, so it is
still up to drivers to properly lock concurrent access to a PWM device
and the core will make sure that a device can only be requested once.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/4840dad3/attachment.sig>
On Wed, Jul 01, 2015 at 10:21:51AM +0200, Boris Brezillon wrote:
quoted
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.
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 ?
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.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
quoted hunk
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>
---
drivers/video/backlight/pwm_bl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)*viathePWMlookuptable.*/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.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/ba0f22ea/attachment.sig>
On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
quoted
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>
---
drivers/video/backlight/pwm_bl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)*viathePWMlookuptable.*/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.
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.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Wed, Jul 01, 2015 at 10:21:51AM +0200, Boris Brezillon wrote:
quoted
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.
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 ?
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.
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.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/e0727831/attachment.sig>
On Wed, Jul 01, 2015 at 10:21:51AM +0200, Boris Brezillon wrote:
quoted
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.
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 ?
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.
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.
Hm, I was trying to keep the series bisectable. If we do that
after introducing the default state concept, then some drivers will
retrieve invalid values until the patches introducing the default
helpers and changing the different drivers to call the default helpers
where appropriate are introduced.
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.
Yes, but we don't live in an ideal world ;-), and the value set in an
old bootloaders might be considered wrong at some point, and new dts
versions might decide to change a bit the period value.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
quoted
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>
---
drivers/video/backlight/pwm_bl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)*viathePWMlookuptable.*/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.
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.
Of course the right thing to do would be to replace all initialization
of the data->pwm_period_ns by proper PWM lookup tables, but that's
proven difficult in the past since very few people still have access to
hardware where that code gets executed.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/fa5731d1/attachment.sig>
On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
quoted
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>
---
drivers/video/backlight/pwm_bl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)*viathePWMlookuptable.*/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.
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).
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.
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.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
quoted hunk
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 <redacted>
---
drivers/pwm/core.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
include/linux/pwm.h | 26 +++++++++++++
2 files changed, 124 insertions(+), 12 deletions(-)
@@ -238,8 +238,9 @@ 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->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;
quoted hunk
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;
If you add kerneldoc, please add it properly. It should start with /**
and you need to list at least the parameters and return value.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/40a5e6a4/attachment.sig>
On Wed, Jul 01, 2015 at 10:21:56AM +0200, Boris Brezillon wrote:
quoted hunk
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 <redacted>
---
drivers/pwm/core.c | 3 +++
include/linux/pwm.h | 2 ++
2 files changed, 5 insertions(+)
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.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/13fd205a/attachment.sig>
On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
quoted
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>
---
drivers/video/backlight/pwm_bl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)*viathePWMlookuptable.*/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.
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).
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
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.
Yes, that's what PWM lookup tables are meant to address. I tried to
convert existing users a number of times, but never got any replies and
since it's board code I couldn't merge this through the PWM tree...
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/3accacfa/attachment.sig>
On Wed, Jul 01, 2015 at 10:21:56AM +0200, Boris Brezillon wrote:
quoted
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 <redacted>
---
drivers/pwm/core.c | 3 +++
include/linux/pwm.h | 2 ++
2 files changed, 5 insertions(+)
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.
I'm fine with the reset_state name, I'll change that in my v2.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
quoted
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 <redacted>
---
drivers/pwm/core.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
include/linux/pwm.h | 26 +++++++++++++
2 files changed, 124 insertions(+), 12 deletions(-)
@@ -238,8 +238,9 @@ 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->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;
Sure, I'll change that to make it more readable.
quoted
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;
There should be a space between the above two lines.
I'll add an empty line.
quoted
+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)))
+ 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
+ } 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;
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).
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
quoted
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>
---
drivers/video/backlight/pwm_bl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)*viathePWMlookuptable.*/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.
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).
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.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
quoted
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>
---
drivers/video/backlight/pwm_bl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
@@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)*viathePWMlookuptable.*/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.
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).
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.
I think we should be able to live without the assignment. Perhaps when
replacing it, add a comment saying that this is for very legacy cases
only and that PWM lookup tables are the right way to fix this.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/fb99a330/attachment.sig>
+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;
};
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
[...]
quoted
quoted
+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)))
+ 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.
Yeah, it could be a static function in core.c. What I want to avoid is
having to change a bunch of code if ever state assignment becomes
something other than merely copying a structure.
[...]
+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.
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.
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.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/771ec3c9/attachment.sig>
+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.
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.
I prefer the single space approach too, so I won't complain ;-).
quoted
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.
I agree.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com