* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Boris Brezillon @ 2015-07-20 7:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com>
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 <boris.brezillon@free-electrons.com> 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(-)
>
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Thierry Reding @ 2015-07-20 7:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 01/15] pwm: add the pwm_is_enabled() helper
From: Thierry Reding @ 2015-07-20 7:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-2-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]
On Wed, Jul 01, 2015 at 10:21:47AM +0200, Boris Brezillon wrote:
> Some PWM drivers are testing the PWMF_ENABLED flag. Create an helper
> function to hide the logic behind enabled test.
> This will allow us to smoothly move from the current approach to an atomic
> PWM update approach.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/pwm/core.c | 4 ++--
> drivers/pwm/pwm-atmel-tcb.c | 2 +-
> drivers/pwm/pwm-atmel.c | 6 +++---
> drivers/pwm/pwm-bcm-kona.c | 4 ++--
> drivers/pwm/pwm-ep93xx.c | 4 ++--
> drivers/pwm/pwm-imx.c | 2 +-
> drivers/pwm/pwm-mxs.c | 4 ++--
> drivers/pwm/pwm-renesas-tpu.c | 2 +-
> drivers/pwm/pwm-tegra.c | 6 +++---
> drivers/pwm/pwm-tiecap.c | 10 +++++-----
> drivers/pwm/pwm-tiehrpwm.c | 6 +++---
> drivers/pwm/sysfs.c | 2 +-
> include/linux/pwm.h | 5 +++++
> 13 files changed, 31 insertions(+), 26 deletions(-)
Applied, thanks.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 02/15] pwm: fix pwm_get_period and pwm_get_duty_cycle prototypes
From: Thierry Reding @ 2015-07-20 7:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-3-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 519 bytes --]
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 <boris.brezillon@free-electrons.com>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 03/15] pwm: add pwm_get_polarity helper function
From: Thierry Reding @ 2015-07-20 7:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-4-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 461 bytes --]
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 <boris.brezillon@free-electrons.com>
> ---
> include/linux/pwm.h | 5 +++++
> 1 file changed, 5 insertions(+)
Applied thanks.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 04/15] pwm: make use of pwm_get_xxx helpers where appropriate
From: Thierry Reding @ 2015-07-20 8:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-5-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 599 bytes --]
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 <boris.brezillon@free-electrons.com>
> ---
> 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(-)
Applied, thanks.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Thierry Reding @ 2015-07-20 8:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150702094955.4b1c9254@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]
On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> On Thu, 2 Jul 2015 08:44:45 +0200
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
>
> > 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?
> >
>
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct
From: Thierry Reding @ 2015-07-20 8:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-7-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 339 bytes --]
On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
[...]
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
[...]
> +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.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 07/15] pwm: move the enabled/disabled info to pwm_state struct
From: Thierry Reding @ 2015-07-20 8:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-8-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]
On Wed, Jul 01, 2015 at 10:21:53AM +0200, Boris Brezillon wrote:
> 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 <boris.brezillon@free-electrons.com>
> ---
> drivers/pwm/core.c | 15 ++++++++++++---
> include/linux/pwm.h | 6 +++---
> 2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index a6bc8e6..3e830ce 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -474,8 +474,15 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
> */
> int pwm_enable(struct pwm_device *pwm)
> {
> - if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
> - return pwm->chip->ops->enable(pwm->chip, pwm);
> + if (pwm && !pwm_is_enabled(pwm)) {
> + int err;
> +
> + err = pwm->chip->ops->enable(pwm->chip, pwm);
> + if (!err)
> + pwm->state.enabled = true;
> +
> + return err;
> + }
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Boris Brezillon @ 2015-07-20 8:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720080313.GF29614@ulmo>
Hi Thierry,
On Mon, 20 Jul 2015 10:03:14 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> > On Thu, 2 Jul 2015 08:44:45 +0200
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> >
> > > 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?
> > >
> >
> > 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
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Thierry Reding @ 2015-07-20 8:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-9-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]
On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> 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 <boris.brezillon@free-electrons.com>
> ---
> drivers/video/backlight/pwm_bl.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index ae498c1..fe5597c 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> * via the PWM lookup table.
> */
> pb->period = pwm_get_default_period(pb->pwm);
> - if (!pb->period && (data->pwm_period_ns > 0)) {
> + if (!pb->period && (data->pwm_period_ns > 0))
> pb->period = data->pwm_period_ns;
> - pwm_set_period(pb->pwm, data->pwm_period_ns);
> - }
>
> pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
As far as I remember this line is there in order to pass in a period if
the backlight driver is initialized from board setup files. In such a
case there won't be an period associated with the PWM channel in the
first place.
I think even with the introduction of a default period, we'd be missing
out on the board setup case because there is no standard place where it
is being set, so it must come from the platform data.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Boris Brezillon @ 2015-07-20 8:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720081559.GI29614@ulmo>
On Mon, 20 Jul 2015 10:16:00 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > 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 <boris.brezillon@free-electrons.com>
> > ---
> > drivers/video/backlight/pwm_bl.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index ae498c1..fe5597c 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > * via the PWM lookup table.
> > */
> > pb->period = pwm_get_default_period(pb->pwm);
> > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > + if (!pb->period && (data->pwm_period_ns > 0))
> > pb->period = data->pwm_period_ns;
> > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > - }
> >
> > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
>
> As far as I remember this line is there in order to pass in a period if
> the backlight driver is initialized from board setup files. In such a
> case there won't be an period associated with the PWM channel in the
> first place.
>
> I think even with the introduction of a default period, we'd be missing
> out on the board setup case because there is no standard place where it
> is being set, so it must come from the platform data.
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
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Thierry Reding @ 2015-07-20 8:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720101443.180ebddb@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]
On Mon, Jul 20, 2015 at 10:14:43AM +0200, Boris Brezillon wrote:
> Hi Thierry,
>
> On Mon, 20 Jul 2015 10:03:14 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> > > On Thu, 2 Jul 2015 08:44:45 +0200
> > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > > 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?
> > > >
> > >
> > > 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Boris Brezillon @ 2015-07-20 8:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720082241.GJ29614@ulmo>
On Mon, 20 Jul 2015 10:22:42 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Jul 20, 2015 at 10:14:43AM +0200, Boris Brezillon wrote:
> > Hi Thierry,
> >
> > On Mon, 20 Jul 2015 10:03:14 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Thu, Jul 02, 2015 at 09:49:55AM +0200, Boris Brezillon wrote:
> > > > On Thu, 2 Jul 2015 08:44:45 +0200
> > > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > > 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?
> > > > >
> > > >
> > > > 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
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Thierry Reding @ 2015-07-20 8:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720102143.05949ca2@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]
On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:16:00 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > 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 <boris.brezillon@free-electrons.com>
> > > ---
> > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > index ae498c1..fe5597c 100644
> > > --- a/drivers/video/backlight/pwm_bl.c
> > > +++ b/drivers/video/backlight/pwm_bl.c
> > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > * via the PWM lookup table.
> > > */
> > > pb->period = pwm_get_default_period(pb->pwm);
> > > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > > + if (!pb->period && (data->pwm_period_ns > 0))
> > > pb->period = data->pwm_period_ns;
> > > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > - }
> > >
> > > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> >
> > As far as I remember this line is there in order to pass in a period if
> > the backlight driver is initialized from board setup files. In such a
> > case there won't be an period associated with the PWM channel in the
> > first place.
> >
> > I think even with the introduction of a default period, we'd be missing
> > out on the board setup case because there is no standard place where it
> > is being set, so it must come from the platform data.
>
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Boris Brezillon @ 2015-07-20 8:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720083648.GK29614@ulmo>
On Mon, 20 Jul 2015 10:36:50 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > On Mon, 20 Jul 2015 10:16:00 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > > 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 <boris.brezillon@free-electrons.com>
> > > > ---
> > > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > index ae498c1..fe5597c 100644
> > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > * via the PWM lookup table.
> > > > */
> > > > pb->period = pwm_get_default_period(pb->pwm);
> > > > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > > > + if (!pb->period && (data->pwm_period_ns > 0))
> > > > pb->period = data->pwm_period_ns;
> > > > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > > - }
> > > >
> > > > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> > >
> > > As far as I remember this line is there in order to pass in a period if
> > > the backlight driver is initialized from board setup files. In such a
> > > case there won't be an period associated with the PWM channel in the
> > > first place.
> > >
> > > I think even with the introduction of a default period, we'd be missing
> > > out on the board setup case because there is no standard place where it
> > > is being set, so it must come from the platform data.
> >
> > 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
^ permalink raw reply
* Re: [RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update
From: Thierry Reding @ 2015-07-20 8:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-12-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 6611 bytes --]
On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
> 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 <boris.brezillon@free-electrons.com>
> ---
> drivers/pwm/core.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
> include/linux/pwm.h | 26 +++++++++++++
> 2 files changed, 124 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 30631f5..6dafd8e 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -238,8 +238,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> unsigned int i;
> int ret;
>
> - if (!chip || !chip->dev || !chip->ops || !chip->ops->config ||
> - !chip->ops->enable || !chip->ops->disable || !chip->npwm)
> + if (!chip || !chip->dev || !chip->ops || (!chip->ops->apply &&
> + (!chip->ops->config || !chip->ops->enable ||
> + !chip->ops->disable)) || !chip->npwm)
> return -EINVAL;
This is becoming really unreadable, perhaps split it into two checks, or
even split out the sanity check on the ops into a separate function to
make the negations easier to read:
static bool pwm_ops_check(const struct pwm_ops *ops)
{
/* driver supports legacy, non-atomic operation */
if (ops->config && ops->enable && ops->disable)
return true;
/* driver supports atomic operation */
if (ops->apply)
return true;
return false;
}
and then use this:
if (!chip || !chip->dev || !chip->ops || !chip->npwm)
return -EINVAL;
if (!pwm_ops_check(chip->ops))
return -EINVAL;
> mutex_lock(&pwm_lock);
> @@ -430,7 +431,17 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
> return -EINVAL;
>
> - err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> + if (pwm->chip->ops->apply) {
> + struct pwm_state state = pwm->state;
Shouldn't this use pwm_get_state()?
> +
> + state.period = period_ns;
> + state.duty_cycle = duty_ns;
> +
> + err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
> + } else {
> + err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> + }
> +
> if (err)
> return err;
>
> @@ -455,6 +466,17 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
> if (!pwm || !pwm->chip->ops)
> return -EINVAL;
>
> + if (pwm->chip->ops->apply) {
> + struct pwm_state state = pwm->state;
Same here.
> +
> + state.polarity = polarity;
> + err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
> + if (!err)
> + pwm->state.polarity = polarity;
> +
> + return err;
> + }
> +
> if (!pwm->chip->ops->set_polarity)
> return -ENOSYS;
>
> @@ -477,17 +499,27 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
> */
> int pwm_enable(struct pwm_device *pwm)
> {
> - if (pwm && !pwm_is_enabled(pwm)) {
> - int err;
> + int err;
>
> - err = pwm->chip->ops->enable(pwm->chip, pwm);
> - if (!err)
> - pwm->state.enabled = true;
> + if (!pwm)
> + return -EINVAL;
>
> - return err;
> + if (pwm_is_enabled(pwm))
> + return 0;
> +
> + if (pwm->chip->ops->apply) {
> + struct pwm_state state = pwm->state;
And here.
> +
> + state.enabled = true;
> + err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
There should be a space between the above two lines.
> + } else {
> + err = pwm->chip->ops->enable(pwm->chip, pwm);
> }
>
> - return pwm ? 0 : -EINVAL;
> + if (!err)
> + pwm->state.enabled = true;
> +
> + return err;
> }
> EXPORT_SYMBOL_GPL(pwm_enable);
>
> @@ -497,13 +529,67 @@ EXPORT_SYMBOL_GPL(pwm_enable);
> */
> void pwm_disable(struct pwm_device *pwm)
> {
> - if (pwm && pwm_is_enabled(pwm)) {
> + if (!pwm || !pwm_is_enabled(pwm))
> + return;
> +
> + if (pwm->chip->ops->apply) {
> + struct pwm_state state = pwm->state;
> +
> + state.enabled = false;
> + pwm->chip->ops->apply(pwm->chip, pwm, &state);
> + } else {
> pwm->chip->ops->disable(pwm->chip, pwm);
> - pwm->state.enabled = false;
> }
> +
> + pwm->state.enabled = false;
> }
> EXPORT_SYMBOL_GPL(pwm_disable);
Same comments as for pwm_enable().
>
> +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?
> + } 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;
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index b47244a..7e99679 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
> return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
> }
>
> +/*
> + * pwm_apply_state - apply a new state to the PWM device
> + */
> +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
If you add kerneldoc, please add it properly. It should start with /**
and you need to list at least the parameters and return value.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 10/15] pwm: add the PWM initial state retrieval infra
From: Thierry Reding @ 2015-07-20 9:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-11-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]
On Wed, Jul 01, 2015 at 10:21:56AM +0200, Boris Brezillon wrote:
> 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 <boris.brezillon@free-electrons.com>
> ---
> drivers/pwm/core.c | 3 +++
> include/linux/pwm.h | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3e830ce..30631f5 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -264,6 +264,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> pwm->hwpwm = i;
> pwm_set_default_polarity(pwm, polarity);
>
> + if (chip->ops->init_state)
> + chip->ops->init_state(chip, pwm);
> +
> radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
> }
>
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 0f36a06..b47244a 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -177,6 +177,8 @@ struct pwm_ops {
> struct pwm_device *pwm);
> void (*disable)(struct pwm_chip *chip,
> struct pwm_device *pwm);
> + void (*init_state)(struct pwm_chip *chip,
> + struct pwm_device *pwm);
I think I'd call this reset_state. init has this connotation of setting
a set of default values. For reset it's clearer in my opinion that it's
resetting to the hardware state.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Thierry Reding @ 2015-07-20 9:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720105003.29b5813a@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 4159 bytes --]
On Mon, Jul 20, 2015 at 10:50:03AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:36:50 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > > On Mon, 20 Jul 2015 10:16:00 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > > > 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 <boris.brezillon@free-electrons.com>
> > > > > ---
> > > > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > index ae498c1..fe5597c 100644
> > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > > * via the PWM lookup table.
> > > > > */
> > > > > pb->period = pwm_get_default_period(pb->pwm);
> > > > > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > > > > + if (!pb->period && (data->pwm_period_ns > 0))
> > > > > pb->period = data->pwm_period_ns;
> > > > > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > > > - }
> > > > >
> > > > > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> > > >
> > > > As far as I remember this line is there in order to pass in a period if
> > > > the backlight driver is initialized from board setup files. In such a
> > > > case there won't be an period associated with the PWM channel in the
> > > > first place.
> > > >
> > > > I think even with the introduction of a default period, we'd be missing
> > > > out on the board setup case because there is no standard place where it
> > > > is being set, so it must come from the platform data.
> > >
> > > 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.
> > 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 10/15] pwm: add the PWM initial state retrieval infra
From: Boris Brezillon @ 2015-07-20 9:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720090123.GM29614@ulmo>
On Mon, 20 Jul 2015 11:01:24 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Jul 01, 2015 at 10:21:56AM +0200, Boris Brezillon wrote:
> > 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 <boris.brezillon@free-electrons.com>
> > ---
> > drivers/pwm/core.c | 3 +++
> > include/linux/pwm.h | 2 ++
> > 2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 3e830ce..30631f5 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -264,6 +264,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > pwm->hwpwm = i;
> > pwm_set_default_polarity(pwm, polarity);
> >
> > + if (chip->ops->init_state)
> > + chip->ops->init_state(chip, pwm);
> > +
> > radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
> > }
> >
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 0f36a06..b47244a 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -177,6 +177,8 @@ struct pwm_ops {
> > struct pwm_device *pwm);
> > void (*disable)(struct pwm_chip *chip,
> > struct pwm_device *pwm);
> > + void (*init_state)(struct pwm_chip *chip,
> > + struct pwm_device *pwm);
>
> I think I'd call this reset_state. init has this connotation of setting
> a set of default values. For reset it's clearer in my opinion that it's
> resetting to the hardware state.
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
^ permalink raw reply
* Re: [RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update
From: Boris Brezillon @ 2015-07-20 9:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720085939.GL29614@ulmo>
On Mon, 20 Jul 2015 10:59:40 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
> > 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 <boris.brezillon@free-electrons.com>
> > ---
> > drivers/pwm/core.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++------
> > include/linux/pwm.h | 26 +++++++++++++
> > 2 files changed, 124 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 30631f5..6dafd8e 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -238,8 +238,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > unsigned int i;
> > int ret;
> >
> > - if (!chip || !chip->dev || !chip->ops || !chip->ops->config ||
> > - !chip->ops->enable || !chip->ops->disable || !chip->npwm)
> > + if (!chip || !chip->dev || !chip->ops || (!chip->ops->apply &&
> > + (!chip->ops->config || !chip->ops->enable ||
> > + !chip->ops->disable)) || !chip->npwm)
> > return -EINVAL;
>
> This is becoming really unreadable, perhaps split it into two checks, or
> even split out the sanity check on the ops into a separate function to
> make the negations easier to read:
>
> static bool pwm_ops_check(const struct pwm_ops *ops)
> {
> /* driver supports legacy, non-atomic operation */
> if (ops->config && ops->enable && ops->disable)
> return true;
>
> /* driver supports atomic operation */
> if (ops->apply)
> return true;
>
> return false;
> }
>
> and then use this:
>
> if (!chip || !chip->dev || !chip->ops || !chip->npwm)
> return -EINVAL;
>
> if (!pwm_ops_check(chip->ops))
> return -EINVAL;
>
Sure, I'll change that to make it more readable.
> > mutex_lock(&pwm_lock);
> > @@ -430,7 +431,17 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> > if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
> > return -EINVAL;
> >
> > - err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
> > + if (pwm->chip->ops->apply) {
> > + struct pwm_state state = pwm->state;
>
> Shouldn't this use pwm_get_state()?
Yes, I'll fix all of them
[...]
>
> > +
> > + state.enabled = true;
> > + err = pwm->chip->ops->apply(pwm->chip, pwm, &state);
>
> There should be a space between the above two lines.
I'll add an empty line.
>
> >
> > +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.
>
> > + } 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;
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index b47244a..7e99679 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
> > return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
> > }
> >
> > +/*
> > + * pwm_apply_state - apply a new state to the PWM device
> > + */
> > +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
>
> If you add kerneldoc, please add it properly. It should start with /**
> and you need to list at least the parameters and return value.
Yes, I'll fix that.
BTW, I remember that you were expecting another name for this function
(pwm_update IIRC).
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Boris Brezillon @ 2015-07-20 9:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720091003.GN29614@ulmo>
On Mon, 20 Jul 2015 11:10:04 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Jul 20, 2015 at 10:50:03AM +0200, Boris Brezillon wrote:
> > On Mon, 20 Jul 2015 10:36:50 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > > > On Mon, 20 Jul 2015 10:16:00 +0200
> > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > > > > 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 <boris.brezillon@free-electrons.com>
> > > > > > ---
> > > > > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > > index ae498c1..fe5597c 100644
> > > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > > > * via the PWM lookup table.
> > > > > > */
> > > > > > pb->period = pwm_get_default_period(pb->pwm);
> > > > > > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > > > > > + if (!pb->period && (data->pwm_period_ns > 0))
> > > > > > pb->period = data->pwm_period_ns;
> > > > > > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > > > > - }
> > > > > >
> > > > > > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> > > > >
> > > > > As far as I remember this line is there in order to pass in a period if
> > > > > the backlight driver is initialized from board setup files. In such a
> > > > > case there won't be an period associated with the PWM channel in the
> > > > > first place.
> > > > >
> > > > > I think even with the introduction of a default period, we'd be missing
> > > > > out on the board setup case because there is no standard place where it
> > > > > is being set, so it must come from the platform data.
> > > >
> > > > 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
^ permalink raw reply
* Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period
From: Thierry Reding @ 2015-07-20 10:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720115704.0c64d070@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 4089 bytes --]
On Mon, Jul 20, 2015 at 11:57:04AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 11:10:04 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Mon, Jul 20, 2015 at 10:50:03AM +0200, Boris Brezillon wrote:
> > > On Mon, 20 Jul 2015 10:36:50 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > > On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > > > > On Mon, 20 Jul 2015 10:16:00 +0200
> > > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > >
> > > > > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > > > > > 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 <boris.brezillon@free-electrons.com>
> > > > > > > ---
> > > > > > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > > > index ae498c1..fe5597c 100644
> > > > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > > > > * via the PWM lookup table.
> > > > > > > */
> > > > > > > pb->period = pwm_get_default_period(pb->pwm);
> > > > > > > - if (!pb->period && (data->pwm_period_ns > 0)) {
> > > > > > > + if (!pb->period && (data->pwm_period_ns > 0))
> > > > > > > pb->period = data->pwm_period_ns;
> > > > > > > - pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > > > > > - }
> > > > > > >
> > > > > > > pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> > > > > >
> > > > > > As far as I remember this line is there in order to pass in a period if
> > > > > > the backlight driver is initialized from board setup files. In such a
> > > > > > case there won't be an period associated with the PWM channel in the
> > > > > > first place.
> > > > > >
> > > > > > I think even with the introduction of a default period, we'd be missing
> > > > > > out on the board setup case because there is no standard place where it
> > > > > > is being set, so it must come from the platform data.
> > > > >
> > > > > 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 06/15] pwm: define a new pwm_state struct
From: Boris Brezillon @ 2015-07-20 10:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720080458.GG29614@ulmo>
On Mon, 20 Jul 2015 10:04:59 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Jul 01, 2015 at 10:21:52AM +0200, Boris Brezillon wrote:
> [...]
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> [...]
> > +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
^ permalink raw reply
* Re: [RFC PATCH 11/15] pwm: add the core infrastructure to allow atomic update
From: Thierry Reding @ 2015-07-20 10:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150720114827.2e5d52a5@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2001 bytes --]
On Mon, Jul 20, 2015 at 11:48:27AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 10:59:40 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Wed, Jul 01, 2015 at 10:21:57AM +0200, Boris Brezillon wrote:
[...]
> > > +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.
[...]
> > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > index b47244a..7e99679 100644
> > > --- a/include/linux/pwm.h
> > > +++ b/include/linux/pwm.h
> > > @@ -151,6 +151,29 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
> > > return pwm ? pwm->state.polarity : PWM_POLARITY_NORMAL;
> > > }
> > >
> > > +/*
> > > + * pwm_apply_state - apply a new state to the PWM device
> > > + */
> > > +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
> >
> > If you add kerneldoc, please add it properly. It should start with /**
> > and you need to list at least the parameters and return value.
>
> Yes, I'll fix that.
> BTW, I remember that you were expecting another name for this function
> (pwm_update IIRC).
I don't mind the pwm_apply_state() name very much. It's pretty accurate
with regards to what it does.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox