* Re: [PATCH v5 04/46] pwm: get rid of pwm->lock
From: Thierry Reding @ 2016-04-12 11:46 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette, Stephen Boyd,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Liam Girdwood,
Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Jean Delvare,
Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jingoo Han,
Lee Jones <lee.jo>
In-Reply-To: <20160412133255.73cea027@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]
On Tue, Apr 12, 2016 at 01:32:55PM +0200, Boris Brezillon wrote:
> Hi Thierry,
>
> On Tue, 12 Apr 2016 13:22:46 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Wed, Mar 30, 2016 at 10:03:27PM +0200, Boris Brezillon wrote:
> > > PWM devices are not protected against concurrent accesses. The lock in
> > > pwm_device might let PWM users think it is, but it's actually only
> > > protecting the enabled state.
> > >
> > > Removing this lock should be fine as long as all PWM users are aware that
> > > accesses to the PWM device have to be serialized, which seems to be the
> > > case for all of them except the sysfs interface.
> > > Patch the sysfs code by adding a lock to the pwm_export struct and making
> > > sure it's taken for all accesses to the exported PWM device.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > ---
> > > drivers/pwm/core.c | 19 ++++--------------
> > > drivers/pwm/sysfs.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----------
> > > include/linux/pwm.h | 2 --
> > > 3 files changed, 50 insertions(+), 28 deletions(-)
> >
> > This is a little overzealous. Only accesses that can cause races need to
> > be protected by the lock. All of the *_show() callbacks don't modify the
> > PWM device in any way, so there is no need to protect them against
> > concurrent accesses.
>
> This is probably true for this set of changes, but what will happen
> when we'll switch to the atomic API? There's no guarantee that
> pwm->state = *newstate is done atomically, and you may see a partially
> updated state when calling pwm_get_state() while another thread is
> calling pwm_apply_state().
I'd argue that for sysfs it doesn't matter since the userspace API is
non-atomic anyway. As such it doesn't really matter which part of the
state you're getting because only one field from the query is exposed
to userspace, hence the coherence of the state is irrelevant.
All other users should be taking care of the serialization themselves
already.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept
From: Thierry Reding @ 2016-04-12 11:49 UTC (permalink / raw)
To: Boris Brezillon
Cc: Milo Kim, Kamil Debski, Heiko Stuebner, linux-doc, David Airlie,
Mike Turquette, linux-fbdev, dri-devel, linux-kernel, linux-sunxi,
Alexandre Belloni, Daniel Vetter, Lee Jones, linux-clk,
linux-leds, Krzysztof Kozlowski, linux-samsung-soc,
Alexander Shiyan, Jonathan Corbet, Robert Jarzmik, lm-sensors,
linux-rockchip, Chen-Yu Tsai, Tomi Valkeinen, linux-input,
Jean-Christophe
In-Reply-To: <1459368249-13241-16-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]
On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> 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.
>
> All pwm_get_xxx() helpers are now implemented as wrappers around
> pwm_get_state().
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/pwm/core.c | 8 ++++----
> include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> 2 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6433059..f3f91e7 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> pwm->chip = chip;
> pwm->pwm = chip->base + i;
> pwm->hwpwm = i;
> - pwm->polarity = polarity;
> + pwm->state.polarity = polarity;
Would this not more correctly be assigned to pwm->args.polarity? After
all this is setting up the "initial" state, much like DT or the lookup
tables would for duty cycle and period.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 16/46] pwm: move the enabled/disabled info into pwm_state
From: Thierry Reding @ 2016-04-12 11:51 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette, Stephen Boyd,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Liam Girdwood,
Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Jean Delvare,
Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jingoo Han,
Lee Jones <lee.jo>
In-Reply-To: <1459368249-13241-17-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 373 bytes --]
On Wed, Mar 30, 2016 at 10:03:39PM +0200, Boris Brezillon wrote:
[...]
> @@ -145,7 +146,11 @@ static inline void pwm_get_state(const struct pwm_device *pwm,
>
> static inline bool pwm_is_enabled(const struct pwm_device *pwm)
> {
> - return test_bit(PWMF_ENABLED, &pwm->flags);
> + struct pwm_state pstate;
No need for the p prefix, in my opinion.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept
From: Boris Brezillon @ 2016-04-12 12:04 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette, Stephen Boyd,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Liam Girdwood,
Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Jean Delvare,
Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jingoo Han,
Lee Jones
In-Reply-To: <20160412113912.GK18882-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org>
On Tue, 12 Apr 2016 13:39:12 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > Currently the PWM core mixes the current PWM state with the per-platform
> > reference config (specified through the PWM lookup table, DT definition or
> > directly hardcoded in PWM drivers).
> >
> > Create a pwm_args struct to store this reference config, so that PWM users
> > can differentiate the current config from the reference one.
> >
> > Patch all places where pwm->args should be initialized. We keep the
> > pwm_set_polarity/period() calls until all PWM users are patched to
> > use pwm_args instead of pwm_get_period/polarity().
>
> Perhaps a helper would be useful? Something like:
>
> static inline void
> pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> {
> pwm_set_duty_cycle(pwm, args->duty_cycle);
> pwm_set_period(pwm, args->period);
> }
>
> ? That would make it slightly easier to get rid of it again after all
> clients have been converted.
Sure. I'll add this helper.
>
> With the exception of pwm-clps711x all of these args are set at of_xlate
> time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> might even be possible to move this call to the core, so that removal of
> it will be a one-liner.
Not sure I get that one. Some drivers are implementing their own
->of_xlate() method, how would you get rid of this pwm_apply_args() in
those custom implementations?
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept
From: Boris Brezillon @ 2016-04-12 12:17 UTC (permalink / raw)
To: Thierry Reding
Cc: Milo Kim, Kamil Debski, Heiko Stuebner, linux-doc, David Airlie,
Mike Turquette, linux-fbdev, dri-devel, linux-kernel, linux-sunxi,
Alexandre Belloni, Daniel Vetter, Lee Jones, linux-clk,
linux-leds, Krzysztof Kozlowski, linux-samsung-soc,
Alexander Shiyan, Jonathan Corbet, Robert Jarzmik, lm-sensors,
linux-rockchip, Chen-Yu Tsai, Tomi Valkeinen, linux-input,
Jean-Christophe
In-Reply-To: <20160412114904.GM18882@ulmo.ba.sec>
On Tue, 12 Apr 2016 13:49:04 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > 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.
> >
> > All pwm_get_xxx() helpers are now implemented as wrappers around
> > pwm_get_state().
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/pwm/core.c | 8 ++++----
> > include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > 2 files changed, 46 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 6433059..f3f91e7 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > pwm->chip = chip;
> > pwm->pwm = chip->base + i;
> > pwm->hwpwm = i;
> > - pwm->polarity = polarity;
> > + pwm->state.polarity = polarity;
>
> Would this not more correctly be assigned to pwm->args.polarity? After
> all this is setting up the "initial" state, much like DT or the lookup
> tables would for duty cycle and period.
Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
all the reference info should be extracted from DT, PWM lookup table or
driver specific ->request() implementation, but I can definitely
initialize the args.polarity here too.
Should I keep the pwm->state.polarity assignment (to set the initial
polarity when the driver does not support hardware readout)?
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept
From: Thierry Reding @ 2016-04-12 12:20 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette, Stephen Boyd,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Liam Girdwood,
Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Jean Delvare,
Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jingoo Han,
Lee Jones <lee.jo>
In-Reply-To: <20160412140412.42704302@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]
On Tue, Apr 12, 2016 at 02:04:12PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 13:39:12 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > > Currently the PWM core mixes the current PWM state with the per-platform
> > > reference config (specified through the PWM lookup table, DT definition or
> > > directly hardcoded in PWM drivers).
> > >
> > > Create a pwm_args struct to store this reference config, so that PWM users
> > > can differentiate the current config from the reference one.
> > >
> > > Patch all places where pwm->args should be initialized. We keep the
> > > pwm_set_polarity/period() calls until all PWM users are patched to
> > > use pwm_args instead of pwm_get_period/polarity().
> >
> > Perhaps a helper would be useful? Something like:
> >
> > static inline void
> > pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> > {
> > pwm_set_duty_cycle(pwm, args->duty_cycle);
> > pwm_set_period(pwm, args->period);
> > }
> >
> > ? That would make it slightly easier to get rid of it again after all
> > clients have been converted.
>
> Sure. I'll add this helper.
>
> >
> > With the exception of pwm-clps711x all of these args are set at of_xlate
> > time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> > might even be possible to move this call to the core, so that removal of
> > it will be a one-liner.
>
> Not sure I get that one. Some drivers are implementing their own
> ->of_xlate() method, how would you get rid of this pwm_apply_args() in
> those custom implementations?
I was proposing to have pwm_apply_args() called from the core.
of_pwm_get() is where ->of_xlate() is called from, and the lookup table
arguments would be applied in pwm_get(). Taking into account clps711x,
which sets the arguments in ->request() it might be possible to simply
call pwm_apply_args() from pwm_device_request(), since that's also
called by all other request functions, even the legacy ones.
That said, the amount of code to modify isn't that large, so I'm fine if
you want to keep sprinkling the calls across multiple files, especially
since it's temporary.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept
From: Thierry Reding @ 2016-04-12 12:21 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette, Stephen Boyd,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Liam Girdwood,
Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Jean Delvare,
Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jingoo Han,
Lee Jones <lee.jo>
In-Reply-To: <20160412141718.5fe4cf24@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]
On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 13:49:04 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > 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.
> > >
> > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > pwm_get_state().
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > ---
> > > drivers/pwm/core.c | 8 ++++----
> > > include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > 2 files changed, 46 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index 6433059..f3f91e7 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > pwm->chip = chip;
> > > pwm->pwm = chip->base + i;
> > > pwm->hwpwm = i;
> > > - pwm->polarity = polarity;
> > > + pwm->state.polarity = polarity;
> >
> > Would this not more correctly be assigned to pwm->args.polarity? After
> > all this is setting up the "initial" state, much like DT or the lookup
> > tables would for duty cycle and period.
>
> Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> all the reference info should be extracted from DT, PWM lookup table or
> driver specific ->request() implementation, but I can definitely
> initialize the args.polarity here too.
>
> Should I keep the pwm->state.polarity assignment (to set the initial
> polarity when the driver does not support hardware readout)?
Wouldn't this work automatically as part of the pwm_apply_args() helper
if we extended it with this setting?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept
From: Boris Brezillon @ 2016-04-12 12:45 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-pwm, Mike Turquette, Stephen Boyd, linux-clk, Mark Brown,
Liam Girdwood, Kamil Debski, lm-sensors, Jean Delvare,
Guenter Roeck, Dmitry Torokhov, linux-input, Bryan Wu,
Richard Purdie, Jacek Anaszewski, linux-leds, Maxime Ripard,
Chen-Yu Tsai, linux-sunxi, Joachim Eastwood, Thomas Petazzoni,
Heiko Stuebner, linux-rockchip, Jingoo Han,
Lee Jones <lee.>
In-Reply-To: <20160412122141.GP18882@ulmo.ba.sec>
On Tue, 12 Apr 2016 14:21:41 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > On Tue, 12 Apr 2016 13:49:04 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > 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.
> > > >
> > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > pwm_get_state().
> > > >
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > ---
> > > > drivers/pwm/core.c | 8 ++++----
> > > > include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > > 2 files changed, 46 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > index 6433059..f3f91e7 100644
> > > > --- a/drivers/pwm/core.c
> > > > +++ b/drivers/pwm/core.c
> > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > > pwm->chip = chip;
> > > > pwm->pwm = chip->base + i;
> > > > pwm->hwpwm = i;
> > > > - pwm->polarity = polarity;
> > > > + pwm->state.polarity = polarity;
> > >
> > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > all this is setting up the "initial" state, much like DT or the lookup
> > > tables would for duty cycle and period.
> >
> > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > all the reference info should be extracted from DT, PWM lookup table or
> > driver specific ->request() implementation, but I can definitely
> > initialize the args.polarity here too.
> >
> > Should I keep the pwm->state.polarity assignment (to set the initial
> > polarity when the driver does not support hardware readout)?
>
> Wouldn't this work automatically as part of the pwm_apply_args() helper
> if we extended it with this setting?
Well, as you explained in you answer to patch 5, pwm_apply_args()
should be called on a per-request basis (each time a PWM device is
requested), while the initial polarity setting should only be applied
when registering the PWM chip (and its devices). After that, the
framework takes care of keeping the PWM state in sync with the hardware
state.
Let's take a real (though a bit unusual) example. Say you have a single
PWM device referenced by two different users. Only one user can be
enabled at a time, but each of them has its own reference config
(different polarity, different period).
User1 calls pwm_get() and applies its own polarity and period. Then
user1 is unregistered and release the PWM device, leaving the polarity
and period untouched.
User2 is registered and request the same PWM device, but user2 is
smarter and tries to extract the current PWM state before adapting the
config according to pwm_args. If you just reset pwm->state.polarity
each time pwm_apply_args() is called (and you suggested to call it as
part of the request procedure), then this means the PWM state is no
longer in sync with the hardware state.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept
From: Boris Brezillon @ 2016-04-12 12:55 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette, Stephen Boyd,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Liam Girdwood,
Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Jean Delvare,
Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jingoo Han,
Lee Jones
In-Reply-To: <20160412122029.GO18882-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org>
On Tue, 12 Apr 2016 14:20:29 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Apr 12, 2016 at 02:04:12PM +0200, Boris Brezillon wrote:
> > On Tue, 12 Apr 2016 13:39:12 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > > > Currently the PWM core mixes the current PWM state with the per-platform
> > > > reference config (specified through the PWM lookup table, DT definition or
> > > > directly hardcoded in PWM drivers).
> > > >
> > > > Create a pwm_args struct to store this reference config, so that PWM users
> > > > can differentiate the current config from the reference one.
> > > >
> > > > Patch all places where pwm->args should be initialized. We keep the
> > > > pwm_set_polarity/period() calls until all PWM users are patched to
> > > > use pwm_args instead of pwm_get_period/polarity().
> > >
> > > Perhaps a helper would be useful? Something like:
> > >
> > > static inline void
> > > pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> > > {
> > > pwm_set_duty_cycle(pwm, args->duty_cycle);
> > > pwm_set_period(pwm, args->period);
> > > }
> > >
> > > ? That would make it slightly easier to get rid of it again after all
> > > clients have been converted.
> >
> > Sure. I'll add this helper.
> >
> > >
> > > With the exception of pwm-clps711x all of these args are set at of_xlate
> > > time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> > > might even be possible to move this call to the core, so that removal of
> > > it will be a one-liner.
> >
> > Not sure I get that one. Some drivers are implementing their own
> > ->of_xlate() method, how would you get rid of this pwm_apply_args() in
> > those custom implementations?
>
> I was proposing to have pwm_apply_args() called from the core.
> of_pwm_get() is where ->of_xlate() is called from, and the lookup table
> arguments would be applied in pwm_get(). Taking into account clps711x,
> which sets the arguments in ->request() it might be possible to simply
> call pwm_apply_args() from pwm_device_request(), since that's also
> called by all other request functions, even the legacy ones.
>
> That said, the amount of code to modify isn't that large, so I'm fine if
> you want to keep sprinkling the calls across multiple files, especially
> since it's temporary.
No, I'm fine addressing that, but I just don't get where you'd get the
pwm_args to apply. Do you suggest to pass 'struct pwm_args *' to the
->of_xlate() and ->request() methods?
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept
From: Boris Brezillon @ 2016-04-12 13:06 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette, Stephen Boyd,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Liam Girdwood,
Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Jean Delvare,
Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jingoo Han,
Lee Jones
In-Reply-To: <20160412113912.GK18882-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org>
On Tue, 12 Apr 2016 13:39:12 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > Currently the PWM core mixes the current PWM state with the per-platform
> > reference config (specified through the PWM lookup table, DT definition or
> > directly hardcoded in PWM drivers).
> >
> > Create a pwm_args struct to store this reference config, so that PWM users
> > can differentiate the current config from the reference one.
> >
> > Patch all places where pwm->args should be initialized. We keep the
> > pwm_set_polarity/period() calls until all PWM users are patched to
> > use pwm_args instead of pwm_get_period/polarity().
>
> Perhaps a helper would be useful? Something like:
>
> static inline void
> pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> {
> pwm_set_duty_cycle(pwm, args->duty_cycle);
> pwm_set_period(pwm, args->period);
> }
>
> ? That would make it slightly easier to get rid of it again after all
> clients have been converted.
>
> With the exception of pwm-clps711x all of these args are set at of_xlate
> time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> might even be possible to move this call to the core, so that removal of
> it will be a one-liner.
Okay, I think I misunderstood your suggestion. I thought you wanted
this helper to set the reference config, but you actually want to apply
a new state based on the PWM reference values.
Except that pwm_args does not contain all the required information to
apply a full config (args->duty_cycle and args->enable do not exist).
This being said, in my v6 I moved the content of
pwm_regulator_adjust_pwm_config() (patch 27) into a generic helper
(pwm_adjust_config()). This helper is doing pretty much what you're
suggesting here (but again, I'm not sure I correctly understood your
suggestion :-/).
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept
From: Thierry Reding @ 2016-04-12 13:11 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette, Stephen Boyd,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Liam Girdwood,
Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Jean Delvare,
Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jingoo Han,
Lee Jones <lee.jo>
In-Reply-To: <20160412144508.2ee181fe@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 3948 bytes --]
On Tue, Apr 12, 2016 at 02:45:08PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 14:21:41 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > > On Tue, 12 Apr 2016 13:49:04 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > > 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.
> > > > >
> > > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > > pwm_get_state().
> > > > >
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > ---
> > > > > drivers/pwm/core.c | 8 ++++----
> > > > > include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > > > 2 files changed, 46 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > index 6433059..f3f91e7 100644
> > > > > --- a/drivers/pwm/core.c
> > > > > +++ b/drivers/pwm/core.c
> > > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > > > pwm->chip = chip;
> > > > > pwm->pwm = chip->base + i;
> > > > > pwm->hwpwm = i;
> > > > > - pwm->polarity = polarity;
> > > > > + pwm->state.polarity = polarity;
> > > >
> > > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > > all this is setting up the "initial" state, much like DT or the lookup
> > > > tables would for duty cycle and period.
> > >
> > > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > > all the reference info should be extracted from DT, PWM lookup table or
> > > driver specific ->request() implementation, but I can definitely
> > > initialize the args.polarity here too.
> > >
> > > Should I keep the pwm->state.polarity assignment (to set the initial
> > > polarity when the driver does not support hardware readout)?
> >
> > Wouldn't this work automatically as part of the pwm_apply_args() helper
> > if we extended it with this setting?
>
> Well, as you explained in you answer to patch 5, pwm_apply_args()
> should be called on a per-request basis (each time a PWM device is
> requested), while the initial polarity setting should only be applied
> when registering the PWM chip (and its devices). After that, the
> framework takes care of keeping the PWM state in sync with the hardware
> state.
>
> Let's take a real (though a bit unusual) example. Say you have a single
> PWM device referenced by two different users. Only one user can be
> enabled at a time, but each of them has its own reference config
> (different polarity, different period).
>
> User1 calls pwm_get() and applies its own polarity and period. Then
> user1 is unregistered and release the PWM device, leaving the polarity
> and period untouched.
>
> User2 is registered and request the same PWM device, but user2 is
> smarter and tries to extract the current PWM state before adapting the
> config according to pwm_args. If you just reset pwm->state.polarity
> each time pwm_apply_args() is called (and you suggested to call it as
> part of the request procedure), then this means the PWM state is no
> longer in sync with the hardware state.
In that case neither will be the period or duty cycle. Essentially this
gets us back to square one where we need to decide how to handle current
state vs. initial arguments.
But I don't think this is really going to be an issue because this is
all moot until we've moved over to the atomic API, at which point this
is all going to go away anyway.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept
From: Thierry Reding @ 2016-04-12 13:15 UTC (permalink / raw)
To: Boris Brezillon
Cc: Milo Kim, Kamil Debski, Heiko Stuebner, linux-doc, David Airlie,
Mike Turquette, linux-fbdev, dri-devel, linux-kernel, linux-sunxi,
Alexandre Belloni, Daniel Vetter, Lee Jones, linux-clk,
linux-leds, Krzysztof Kozlowski, linux-samsung-soc,
Alexander Shiyan, Jonathan Corbet, Robert Jarzmik, lm-sensors,
linux-rockchip, Chen-Yu Tsai, Tomi Valkeinen, linux-input,
Jean-Christophe
In-Reply-To: <20160412150627.7084665d@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]
On Tue, Apr 12, 2016 at 03:06:27PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 13:39:12 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > > Currently the PWM core mixes the current PWM state with the per-platform
> > > reference config (specified through the PWM lookup table, DT definition or
> > > directly hardcoded in PWM drivers).
> > >
> > > Create a pwm_args struct to store this reference config, so that PWM users
> > > can differentiate the current config from the reference one.
> > >
> > > Patch all places where pwm->args should be initialized. We keep the
> > > pwm_set_polarity/period() calls until all PWM users are patched to
> > > use pwm_args instead of pwm_get_period/polarity().
> >
> > Perhaps a helper would be useful? Something like:
> >
> > static inline void
> > pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> > {
> > pwm_set_duty_cycle(pwm, args->duty_cycle);
> > pwm_set_period(pwm, args->period);
> > }
> >
> > ? That would make it slightly easier to get rid of it again after all
> > clients have been converted.
> >
> > With the exception of pwm-clps711x all of these args are set at of_xlate
> > time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> > might even be possible to move this call to the core, so that removal of
> > it will be a one-liner.
>
> Okay, I think I misunderstood your suggestion. I thought you wanted
> this helper to set the reference config, but you actually want to apply
> a new state based on the PWM reference values.
>
> Except that pwm_args does not contain all the required information to
> apply a full config (args->duty_cycle and args->enable do not exist).
>
> This being said, in my v6 I moved the content of
> pwm_regulator_adjust_pwm_config() (patch 27) into a generic helper
> (pwm_adjust_config()). This helper is doing pretty much what you're
> suggesting here (but again, I'm not sure I correctly understood your
> suggestion :-/).
I'm not suggesting that pwm_apply_args() apply any state. I think we
both agreed earlier that the initial state (represented by pwm_args) was
never to be automatically applied.
What I was suggesting is that we move all the calls to pwm_set_period()
and pwm_set_duty_cycle() into a central location to make it easier to
remove them later in the series. This is really only temporary, so I
don't mind if we leave the calls sprinkled all over the place. At least
that way I hope we'll avoid confusion about what we're talking about =)
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept
From: Boris Brezillon @ 2016-04-12 13:26 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette, Stephen Boyd,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Liam Girdwood,
Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Jean Delvare,
Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jingoo Han,
Lee Jones
In-Reply-To: <20160412131118.GQ18882-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org>
On Tue, 12 Apr 2016 15:11:18 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Apr 12, 2016 at 02:45:08PM +0200, Boris Brezillon wrote:
> > On Tue, 12 Apr 2016 14:21:41 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > > > On Tue, 12 Apr 2016 13:49:04 +0200
> > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > > > 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.
> > > > > >
> > > > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > > > pwm_get_state().
> > > > > >
> > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > > ---
> > > > > > drivers/pwm/core.c | 8 ++++----
> > > > > > include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > > > > 2 files changed, 46 insertions(+), 16 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > > index 6433059..f3f91e7 100644
> > > > > > --- a/drivers/pwm/core.c
> > > > > > +++ b/drivers/pwm/core.c
> > > > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > > > > pwm->chip = chip;
> > > > > > pwm->pwm = chip->base + i;
> > > > > > pwm->hwpwm = i;
> > > > > > - pwm->polarity = polarity;
> > > > > > + pwm->state.polarity = polarity;
> > > > >
> > > > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > > > all this is setting up the "initial" state, much like DT or the lookup
> > > > > tables would for duty cycle and period.
> > > >
> > > > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > > > all the reference info should be extracted from DT, PWM lookup table or
> > > > driver specific ->request() implementation, but I can definitely
> > > > initialize the args.polarity here too.
> > > >
> > > > Should I keep the pwm->state.polarity assignment (to set the initial
> > > > polarity when the driver does not support hardware readout)?
> > >
> > > Wouldn't this work automatically as part of the pwm_apply_args() helper
> > > if we extended it with this setting?
> >
> > Well, as you explained in you answer to patch 5, pwm_apply_args()
> > should be called on a per-request basis (each time a PWM device is
> > requested), while the initial polarity setting should only be applied
> > when registering the PWM chip (and its devices). After that, the
> > framework takes care of keeping the PWM state in sync with the hardware
> > state.
> >
> > Let's take a real (though a bit unusual) example. Say you have a single
> > PWM device referenced by two different users. Only one user can be
> > enabled at a time, but each of them has its own reference config
> > (different polarity, different period).
> >
> > User1 calls pwm_get() and applies its own polarity and period. Then
> > user1 is unregistered and release the PWM device, leaving the polarity
> > and period untouched.
> >
> > User2 is registered and request the same PWM device, but user2 is
> > smarter and tries to extract the current PWM state before adapting the
> > config according to pwm_args. If you just reset pwm->state.polarity
> > each time pwm_apply_args() is called (and you suggested to call it as
> > part of the request procedure), then this means the PWM state is no
> > longer in sync with the hardware state.
>
> In that case neither will be the period or duty cycle. Essentially this
> gets us back to square one where we need to decide how to handle current
> state vs. initial arguments.
That's not true. Now we clearly differentiate the reference config
(content of pwm_args which is only a subset of what you'll find in
pwm_state) and the PWM state (represented by pwm_state).
We should be safe as long as we keep those 2 elements as 2 orthogonal
concepts:
- pwm_args is supposed to give some hint to the PWM user to help him
configure it's PWM appropriately
- pwm_state is here to reflect the real PWM state, and apply new
configs
>
> But I don't think this is really going to be an issue because this is
> all moot until we've moved over to the atomic API, at which point this
> is all going to go away anyway.
As stated in my answer to patch 5, I think I misunderstood your
suggestion. pwm_apply_args() is supposed to adjust the PWM config to
match the period and polarity specified in pwm_args, right?
If that's the case, my question is, should we really call this function
each time a new user requests a PWM instead of letting those users call
the function on-demand (not all users want to adapt the current PWM
config to the pwm_args, some may just want to apply a completely new
config).
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept
From: Thierry Reding @ 2016-04-12 14:05 UTC (permalink / raw)
To: Boris Brezillon
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette, Stephen Boyd,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Liam Girdwood,
Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Jean Delvare,
Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jingoo Han,
Lee Jones <lee.jo>
In-Reply-To: <20160412152644.45ff517a@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 6036 bytes --]
On Tue, Apr 12, 2016 at 03:26:44PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 15:11:18 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
> > On Tue, Apr 12, 2016 at 02:45:08PM +0200, Boris Brezillon wrote:
> > > On Tue, 12 Apr 2016 14:21:41 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > > On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > > > > On Tue, 12 Apr 2016 13:49:04 +0200
> > > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > >
> > > > > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > > > > 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.
> > > > > > >
> > > > > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > > > > pwm_get_state().
> > > > > > >
> > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > > > ---
> > > > > > > drivers/pwm/core.c | 8 ++++----
> > > > > > > include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > > > > > 2 files changed, 46 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > > > index 6433059..f3f91e7 100644
> > > > > > > --- a/drivers/pwm/core.c
> > > > > > > +++ b/drivers/pwm/core.c
> > > > > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > > > > > pwm->chip = chip;
> > > > > > > pwm->pwm = chip->base + i;
> > > > > > > pwm->hwpwm = i;
> > > > > > > - pwm->polarity = polarity;
> > > > > > > + pwm->state.polarity = polarity;
> > > > > >
> > > > > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > > > > all this is setting up the "initial" state, much like DT or the lookup
> > > > > > tables would for duty cycle and period.
> > > > >
> > > > > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > > > > all the reference info should be extracted from DT, PWM lookup table or
> > > > > driver specific ->request() implementation, but I can definitely
> > > > > initialize the args.polarity here too.
> > > > >
> > > > > Should I keep the pwm->state.polarity assignment (to set the initial
> > > > > polarity when the driver does not support hardware readout)?
> > > >
> > > > Wouldn't this work automatically as part of the pwm_apply_args() helper
> > > > if we extended it with this setting?
> > >
> > > Well, as you explained in you answer to patch 5, pwm_apply_args()
> > > should be called on a per-request basis (each time a PWM device is
> > > requested), while the initial polarity setting should only be applied
> > > when registering the PWM chip (and its devices). After that, the
> > > framework takes care of keeping the PWM state in sync with the hardware
> > > state.
> > >
> > > Let's take a real (though a bit unusual) example. Say you have a single
> > > PWM device referenced by two different users. Only one user can be
> > > enabled at a time, but each of them has its own reference config
> > > (different polarity, different period).
> > >
> > > User1 calls pwm_get() and applies its own polarity and period. Then
> > > user1 is unregistered and release the PWM device, leaving the polarity
> > > and period untouched.
> > >
> > > User2 is registered and request the same PWM device, but user2 is
> > > smarter and tries to extract the current PWM state before adapting the
> > > config according to pwm_args. If you just reset pwm->state.polarity
> > > each time pwm_apply_args() is called (and you suggested to call it as
> > > part of the request procedure), then this means the PWM state is no
> > > longer in sync with the hardware state.
> >
> > In that case neither will be the period or duty cycle. Essentially this
> > gets us back to square one where we need to decide how to handle current
> > state vs. initial arguments.
>
> That's not true. Now we clearly differentiate the reference config
> (content of pwm_args which is only a subset of what you'll find in
> pwm_state) and the PWM state (represented by pwm_state).
>
> We should be safe as long as we keep those 2 elements as 2 orthogonal
> concepts:
> - pwm_args is supposed to give some hint to the PWM user to help him
> configure it's PWM appropriately
> - pwm_state is here to reflect the real PWM state, and apply new
> configs
>
> >
> > But I don't think this is really going to be an issue because this is
> > all moot until we've moved over to the atomic API, at which point this
> > is all going to go away anyway.
>
> As stated in my answer to patch 5, I think I misunderstood your
> suggestion. pwm_apply_args() is supposed to adjust the PWM config to
> match the period and polarity specified in pwm_args, right?
>
> If that's the case, my question is, should we really call this function
> each time a new user requests a PWM instead of letting those users call
> the function on-demand (not all users want to adapt the current PWM
> config to the pwm_args, some may just want to apply a completely new
> config).
I think we're still talking past each other. I didn't mean for this to
be a proper part of the API. Like you said the struct pwm_args doesn't
contain enough data to construct a complete state and apply it.
What I was suggesting is to factor out the individual calls to the
various pwm_set_*() functions into a single call. So we wouldn't be
changing semantics, just refactoring to make it easier to get rid of
again in one of the subsequent patches.
That is, pwm_apply_args() would go away again within this very series,
at the same point that you're currently removing the pwm_set_*() calls.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept
From: Boris Brezillon @ 2016-04-12 14:13 UTC (permalink / raw)
To: Thierry Reding
Cc: Milo Kim, Kamil Debski, Heiko Stuebner, linux-doc, David Airlie,
Mike Turquette, linux-fbdev, dri-devel, linux-kernel, linux-sunxi,
Alexandre Belloni, Daniel Vetter, Lee Jones, linux-clk,
linux-leds, Krzysztof Kozlowski, linux-samsung-soc,
Alexander Shiyan, Jonathan Corbet, Robert Jarzmik, lm-sensors,
linux-rockchip, Chen-Yu Tsai, Tomi Valkeinen, linux-input,
Jean-Christophe
In-Reply-To: <20160412140546.GS18882@ulmo.ba.sec>
On Tue, 12 Apr 2016 16:05:46 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Apr 12, 2016 at 03:26:44PM +0200, Boris Brezillon wrote:
> > On Tue, 12 Apr 2016 15:11:18 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > On Tue, Apr 12, 2016 at 02:45:08PM +0200, Boris Brezillon wrote:
> > > > On Tue, 12 Apr 2016 14:21:41 +0200
> > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > > On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > > > > > On Tue, 12 Apr 2016 13:49:04 +0200
> > > > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > >
> > > > > > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > > > > > 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.
> > > > > > > >
> > > > > > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > > > > > pwm_get_state().
> > > > > > > >
> > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > > > > ---
> > > > > > > > drivers/pwm/core.c | 8 ++++----
> > > > > > > > include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > > > > > > 2 files changed, 46 insertions(+), 16 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > > > > index 6433059..f3f91e7 100644
> > > > > > > > --- a/drivers/pwm/core.c
> > > > > > > > +++ b/drivers/pwm/core.c
> > > > > > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > > > > > > pwm->chip = chip;
> > > > > > > > pwm->pwm = chip->base + i;
> > > > > > > > pwm->hwpwm = i;
> > > > > > > > - pwm->polarity = polarity;
> > > > > > > > + pwm->state.polarity = polarity;
> > > > > > >
> > > > > > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > > > > > all this is setting up the "initial" state, much like DT or the lookup
> > > > > > > tables would for duty cycle and period.
> > > > > >
> > > > > > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > > > > > all the reference info should be extracted from DT, PWM lookup table or
> > > > > > driver specific ->request() implementation, but I can definitely
> > > > > > initialize the args.polarity here too.
> > > > > >
> > > > > > Should I keep the pwm->state.polarity assignment (to set the initial
> > > > > > polarity when the driver does not support hardware readout)?
> > > > >
> > > > > Wouldn't this work automatically as part of the pwm_apply_args() helper
> > > > > if we extended it with this setting?
> > > >
> > > > Well, as you explained in you answer to patch 5, pwm_apply_args()
> > > > should be called on a per-request basis (each time a PWM device is
> > > > requested), while the initial polarity setting should only be applied
> > > > when registering the PWM chip (and its devices). After that, the
> > > > framework takes care of keeping the PWM state in sync with the hardware
> > > > state.
> > > >
> > > > Let's take a real (though a bit unusual) example. Say you have a single
> > > > PWM device referenced by two different users. Only one user can be
> > > > enabled at a time, but each of them has its own reference config
> > > > (different polarity, different period).
> > > >
> > > > User1 calls pwm_get() and applies its own polarity and period. Then
> > > > user1 is unregistered and release the PWM device, leaving the polarity
> > > > and period untouched.
> > > >
> > > > User2 is registered and request the same PWM device, but user2 is
> > > > smarter and tries to extract the current PWM state before adapting the
> > > > config according to pwm_args. If you just reset pwm->state.polarity
> > > > each time pwm_apply_args() is called (and you suggested to call it as
> > > > part of the request procedure), then this means the PWM state is no
> > > > longer in sync with the hardware state.
> > >
> > > In that case neither will be the period or duty cycle. Essentially this
> > > gets us back to square one where we need to decide how to handle current
> > > state vs. initial arguments.
> >
> > That's not true. Now we clearly differentiate the reference config
> > (content of pwm_args which is only a subset of what you'll find in
> > pwm_state) and the PWM state (represented by pwm_state).
> >
> > We should be safe as long as we keep those 2 elements as 2 orthogonal
> > concepts:
> > - pwm_args is supposed to give some hint to the PWM user to help him
> > configure it's PWM appropriately
> > - pwm_state is here to reflect the real PWM state, and apply new
> > configs
> >
> > >
> > > But I don't think this is really going to be an issue because this is
> > > all moot until we've moved over to the atomic API, at which point this
> > > is all going to go away anyway.
> >
> > As stated in my answer to patch 5, I think I misunderstood your
> > suggestion. pwm_apply_args() is supposed to adjust the PWM config to
> > match the period and polarity specified in pwm_args, right?
> >
> > If that's the case, my question is, should we really call this function
> > each time a new user requests a PWM instead of letting those users call
> > the function on-demand (not all users want to adapt the current PWM
> > config to the pwm_args, some may just want to apply a completely new
> > config).
>
> I think we're still talking past each other. I didn't mean for this to
> be a proper part of the API. Like you said the struct pwm_args doesn't
> contain enough data to construct a complete state and apply it.
>
> What I was suggesting is to factor out the individual calls to the
> various pwm_set_*() functions into a single call. So we wouldn't be
> changing semantics, just refactoring to make it easier to get rid of
> again in one of the subsequent patches.
>
> That is, pwm_apply_args() would go away again within this very series,
> at the same point that you're currently removing the pwm_set_*() calls.
Okay, eventually got it :).
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v5 03/46] backlight: lm3630a_bl: stop messing with the pwm->period field
From: Lee Jones @ 2016-04-12 14:16 UTC (permalink / raw)
To: Thierry Reding
Cc: Milo Kim, Kamil Debski, Heiko Stuebner, linux-doc, David Airlie,
Mike Turquette, linux-fbdev, dri-devel, linux-kernel, linux-sunxi,
Alexandre Belloni, Daniel Vetter, Robert Jarzmik, linux-clk,
linux-leds, Boris Brezillon, Krzysztof Kozlowski,
linux-samsung-soc, Alexander Shiyan, Jonathan Corbet, lm-sensors,
linux-rockchip, Chen-Yu Tsai, Tomi Valkeinen, linux-input
In-Reply-To: <20160412110809.GI18882@ulmo.ba.sec>
On Tue, 12 Apr 2016, Thierry Reding wrote:
> On Wed, Mar 30, 2016 at 10:03:26PM +0200, Boris Brezillon wrote:
> > pwm->period field is not supposed to be changed by PWM users. The only
> > ones authorized to change it are the PWM core and PWM drivers.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/video/backlight/lm3630a_bl.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
>
> Applied, thanks.
Applied?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v5 02/46] backlight: pwm_bl: remove useless call to pwm_set_period()
From: Lee Jones @ 2016-04-12 14:16 UTC (permalink / raw)
To: Thierry Reding
Cc: Boris Brezillon, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown,
Liam Girdwood, Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
Jean Delvare, Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20160412110350.GH18882-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org>
On Tue, 12 Apr 2016, Thierry Reding wrote:
> On Wed, Mar 30, 2016 at 10:03:25PM +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 internal PWM state.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Acked-by: Lee Jones <lee.jones@linaro.org>
> > ---
> > drivers/video/backlight/pwm_bl.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
>
> Applied, thanks.
Applied?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v5 02/46] backlight: pwm_bl: remove useless call to pwm_set_period()
From: Thierry Reding @ 2016-04-12 14:25 UTC (permalink / raw)
To: Lee Jones
Cc: Boris Brezillon, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown,
Liam Girdwood, Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
Jean Delvare, Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20160412141653.GJ8094@x1>
[-- Attachment #1: Type: text/plain, Size: 788 bytes --]
On Tue, Apr 12, 2016 at 03:16:53PM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
>
> > On Wed, Mar 30, 2016 at 10:03:25PM +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 internal PWM state.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > Acked-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > > drivers/video/backlight/pwm_bl.c | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > Applied, thanks.
>
> Applied?
You Acked it, so I assumed you were fine with me taking it through the
PWM tree to take care of the dependencies.
Did I assume wrongly?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 03/46] backlight: lm3630a_bl: stop messing with the pwm->period field
From: Thierry Reding @ 2016-04-12 14:26 UTC (permalink / raw)
To: Lee Jones
Cc: Milo Kim, Kamil Debski, Heiko Stuebner, linux-doc, David Airlie,
Mike Turquette, linux-fbdev, dri-devel, linux-kernel, linux-sunxi,
Alexandre Belloni, Daniel Vetter, Robert Jarzmik, linux-clk,
linux-leds, Boris Brezillon, Krzysztof Kozlowski,
linux-samsung-soc, Alexander Shiyan, Jonathan Corbet, lm-sensors,
linux-rockchip, Chen-Yu Tsai, Tomi Valkeinen, linux-input
In-Reply-To: <20160412141613.GI8094@x1>
[-- Attachment #1: Type: text/plain, Size: 823 bytes --]
On Tue, Apr 12, 2016 at 03:16:13PM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
>
> > On Wed, Mar 30, 2016 at 10:03:26PM +0200, Boris Brezillon wrote:
> > > pwm->period field is not supposed to be changed by PWM users. The only
> > > ones authorized to change it are the PWM core and PWM drivers.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > ---
> > > drivers/video/backlight/lm3630a_bl.c | 3 +--
> > > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > Applied, thanks.
>
> Applied?
You didn't specifically Ack this one, but I presumed that since the
change is essentially the same as for pwm-backlight, and this is another
prerequisite for the remainder of the series it should go in through the
PWM tree as well.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH v2 0/3] Support INT3406 Display thermal device
From: Aaron Lu @ 2016-04-13 7:32 UTC (permalink / raw)
To: Jingoo Han, Zhang Rui, Rafael J. Wysocki
Cc: linux-fbdev, linux-acpi, linux-pm, srinivas.pandruvada
The display thermal device represents the LED/LCD display panel
that may or may not include touch support. The main function of
the display thermal device is to allow control of the display
brightness in order to address a thermal condition or to reduce
power consumed by display device.
Due to the way this thermal device changes brightness level is said
to be deprecated so we are using the raw interface to do the actual
backlight change. This requires the backlight core support so two
new APIs are added and exported in patch 1/3. With this, the previous
API backlight_device_registered can be removed and this is done in
patch 2/3. Patch 3/3 adds the new int3406 thermal driver.
The 1st version is here:
http://thread.gmane.org/gmane.linux.acpi.devel/72619
This whole series should go through the thermal tree if applied.
Aaron Lu (3):
video / backlight: add two APIs for drivers to use
video / backlight: remove the backlight_device_registered API
Thermal: add INT3406 thermal driver
drivers/acpi/acpi_video.c | 83 ++++----
drivers/acpi/video_detect.c | 2 +-
drivers/thermal/Kconfig | 28 +--
drivers/thermal/int340x_thermal/Kconfig | 42 ++++
drivers/thermal/int340x_thermal/Makefile | 1 +
drivers/thermal/int340x_thermal/int3406_thermal.c | 236 ++++++++++++++++++++++
drivers/video/backlight/backlight.c | 39 ++--
include/acpi/video.h | 20 ++
include/linux/backlight.h | 3 +-
9 files changed, 374 insertions(+), 80 deletions(-)
create mode 100644 drivers/thermal/int340x_thermal/Kconfig
create mode 100644 drivers/thermal/int340x_thermal/int3406_thermal.c
--
2.5.5
^ permalink raw reply
* [PATCH 1/3] video / backlight: add two APIs for drivers to use
From: Aaron Lu @ 2016-04-13 7:32 UTC (permalink / raw)
To: Jingoo Han, Zhang Rui, Rafael J. Wysocki
Cc: linux-fbdev, linux-acpi, linux-pm, srinivas.pandruvada
In-Reply-To: <1460532771-22779-1-git-send-email-aaron.lu@intel.com>
It is useful to get the backlight device's pointer and use it to set
backlight in some cases(the following patch will make use of it) so add
the two APIs and export them.
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/backlight/backlight.c | 43 +++++++++++++++++++++++++------------
include/linux/backlight.h | 2 ++
2 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index bddc8b17a4d8..18901b9b1eb4 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -164,18 +164,10 @@ static ssize_t brightness_show(struct device *dev,
return sprintf(buf, "%d\n", bd->props.brightness);
}
-static ssize_t brightness_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
+int backlight_device_set_brightness(struct backlight_device *bd,
+ unsigned long brightness)
{
- int rc;
- struct backlight_device *bd = to_backlight_device(dev);
- unsigned long brightness;
-
- rc = kstrtoul(buf, 0, &brightness);
- if (rc)
- return rc;
-
- rc = -ENXIO;
+ int rc = -ENXIO;
mutex_lock(&bd->ops_lock);
if (bd->ops) {
@@ -185,7 +177,7 @@ static ssize_t brightness_store(struct device *dev,
pr_debug("set brightness to %lu\n", brightness);
bd->props.brightness = brightness;
backlight_update_status(bd);
- rc = count;
+ rc = 0;
}
}
mutex_unlock(&bd->ops_lock);
@@ -194,6 +186,23 @@ static ssize_t brightness_store(struct device *dev,
return rc;
}
+EXPORT_SYMBOL(backlight_device_set_brightness);
+
+static ssize_t brightness_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ int rc;
+ struct backlight_device *bd = to_backlight_device(dev);
+ unsigned long brightness;
+
+ rc = kstrtoul(buf, 0, &brightness);
+ if (rc)
+ return rc;
+
+ rc = backlight_device_set_brightness(bd, brightness);
+
+ return rc ? rc : count;
+}
static DEVICE_ATTR_RW(brightness);
static ssize_t type_show(struct device *dev, struct device_attribute *attr,
@@ -380,7 +389,7 @@ struct backlight_device *backlight_device_register(const char *name,
}
EXPORT_SYMBOL(backlight_device_register);
-bool backlight_device_registered(enum backlight_type type)
+struct backlight_device *backlight_device_get_by_type(enum backlight_type type)
{
bool found = false;
struct backlight_device *bd;
@@ -394,7 +403,13 @@ bool backlight_device_registered(enum backlight_type type)
}
mutex_unlock(&backlight_dev_list_mutex);
- return found;
+ return found ? bd : NULL;
+}
+EXPORT_SYMBOL(backlight_device_get_by_type);
+
+bool backlight_device_registered(enum backlight_type type)
+{
+ return backlight_device_get_by_type(type) ? true : false;
}
EXPORT_SYMBOL(backlight_device_registered);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 1e7a69adbe6f..f46b88fa4a09 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -144,6 +144,8 @@ extern void backlight_force_update(struct backlight_device *bd,
extern bool backlight_device_registered(enum backlight_type type);
extern int backlight_register_notifier(struct notifier_block *nb);
extern int backlight_unregister_notifier(struct notifier_block *nb);
+extern struct backlight_device *backlight_device_get_by_type(enum backlight_type type);
+extern int backlight_device_set_brightness(struct backlight_device *bd, unsigned long brightness);
#define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
--
2.5.5
^ permalink raw reply related
* [PATCH 2/3] video / backlight: remove the backlight_device_registered API
From: Aaron Lu @ 2016-04-13 7:32 UTC (permalink / raw)
To: Jingoo Han, Zhang Rui, Rafael J. Wysocki
Cc: linux-fbdev, linux-acpi, linux-pm, srinivas.pandruvada
In-Reply-To: <1460532771-22779-1-git-send-email-aaron.lu@intel.com>
Since we will need the backlight_device_get_by_type API, we can use it
instead of the backlight_device_registered API whenever necessary so
remove the backlight_device_registered API.
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/acpi/video_detect.c | 2 +-
drivers/video/backlight/backlight.c | 6 ------
include/linux/backlight.h | 1 -
3 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 1316ddd92fac..3d1327615f72 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -358,7 +358,7 @@ enum acpi_backlight_type acpi_video_get_backlight_type(void)
if (!(video_caps & ACPI_VIDEO_BACKLIGHT))
return acpi_backlight_vendor;
- if (acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW))
+ if (acpi_osi_is_win8() && backlight_device_get_by_type(BACKLIGHT_RAW))
return acpi_backlight_native;
return acpi_backlight_video;
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 18901b9b1eb4..288318ad21dd 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -407,12 +407,6 @@ struct backlight_device *backlight_device_get_by_type(enum backlight_type type)
}
EXPORT_SYMBOL(backlight_device_get_by_type);
-bool backlight_device_registered(enum backlight_type type)
-{
- return backlight_device_get_by_type(type) ? true : false;
-}
-EXPORT_SYMBOL(backlight_device_registered);
-
/**
* backlight_device_unregister - unregisters a backlight device object.
* @bd: the backlight device object to be unregistered and freed.
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index f46b88fa4a09..5f2fd61ef4fb 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -141,7 +141,6 @@ extern void devm_backlight_device_unregister(struct device *dev,
struct backlight_device *bd);
extern void backlight_force_update(struct backlight_device *bd,
enum backlight_update_reason reason);
-extern bool backlight_device_registered(enum backlight_type type);
extern int backlight_register_notifier(struct notifier_block *nb);
extern int backlight_unregister_notifier(struct notifier_block *nb);
extern struct backlight_device *backlight_device_get_by_type(enum backlight_type type);
--
2.5.5
^ permalink raw reply related
* [PATCH 3/3] Thermal: add INT3406 thermal driver
From: Aaron Lu @ 2016-04-13 7:32 UTC (permalink / raw)
To: Jingoo Han, Zhang Rui, Rafael J. Wysocki
Cc: linux-fbdev, linux-acpi, linux-pm, srinivas.pandruvada
In-Reply-To: <1460532771-22779-1-git-send-email-aaron.lu@intel.com>
INT3406 ACPI device object resembles an ACPI video output device, but its
_BCM is said to be deprecated and should not be used. So we will make
use of the raw interface to do the actual cooling. Also, to re-use some
of the ACPI video module's code, function acpi_video_get_levels has been
exported.
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
drivers/acpi/acpi_video.c | 83 ++++----
drivers/thermal/Kconfig | 28 +--
drivers/thermal/int340x_thermal/Kconfig | 42 ++++
drivers/thermal/int340x_thermal/Makefile | 1 +
drivers/thermal/int340x_thermal/int3406_thermal.c | 236 ++++++++++++++++++++++
include/acpi/video.h | 20 ++
6 files changed, 347 insertions(+), 63 deletions(-)
create mode 100644 drivers/thermal/int340x_thermal/Kconfig
create mode 100644 drivers/thermal/int340x_thermal/int3406_thermal.c
diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 4361bc98ef4c..3d5b8a099351 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -191,19 +191,6 @@ struct acpi_video_device_cap {
u8 _DDC:1; /* Return the EDID for this device */
};
-struct acpi_video_brightness_flags {
- u8 _BCL_no_ac_battery_levels:1; /* no AC/Battery levels in _BCL */
- u8 _BCL_reversed:1; /* _BCL package is in a reversed order */
- u8 _BQC_use_index:1; /* _BQC returns an index value */
-};
-
-struct acpi_video_device_brightness {
- int curr;
- int count;
- int *levels;
- struct acpi_video_brightness_flags flags;
-};
-
struct acpi_video_device {
unsigned long device_id;
struct acpi_video_device_flags flags;
@@ -325,7 +312,7 @@ static const struct thermal_cooling_device_ops video_cooling_ops = {
*/
static int
-acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
+acpi_video_device_lcd_query_levels(acpi_handle handle,
union acpi_object **levels)
{
int status;
@@ -335,7 +322,7 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
*levels = NULL;
- status = acpi_evaluate_object(device->dev->handle, "_BCL", NULL, &buffer);
+ status = acpi_evaluate_object(handle, "_BCL", NULL, &buffer);
if (!ACPI_SUCCESS(status))
return status;
obj = (union acpi_object *)buffer.pointer;
@@ -766,36 +753,28 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
return 0;
}
-
-/*
- * Arg:
- * device : video output device (LCD, CRT, ..)
- *
- * Return Value:
- * Maximum brightness level
- *
- * Allocate and initialize device->brightness.
- */
-
-static int
-acpi_video_init_brightness(struct acpi_video_device *device)
+int acpi_video_get_levels(struct acpi_device *device,
+ struct acpi_video_device_brightness **dev_br)
{
union acpi_object *obj = NULL;
int i, max_level = 0, count = 0, level_ac_battery = 0;
- unsigned long long level, level_old;
union acpi_object *o;
struct acpi_video_device_brightness *br = NULL;
- int result = -EINVAL;
+ int result = 0;
u32 value;
- if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) {
+ if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device->handle,
+ &obj))) {
ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available "
"LCD brightness level\n"));
+ result = -ENODEV;
goto out;
}
- if (obj->package.count < 2)
+ if (obj->package.count < 2) {
+ result = -EINVAL;
goto out;
+ }
br = kzalloc(sizeof(*br), GFP_KERNEL);
if (!br) {
@@ -861,6 +840,38 @@ acpi_video_init_brightness(struct acpi_video_device *device)
"Found unordered _BCL package"));
br->count = count;
+ *dev_br = br;
+
+out:
+ kfree(obj);
+ return result;
+out_free:
+ kfree(br);
+ goto out;
+}
+EXPORT_SYMBOL(acpi_video_get_levels);
+
+/*
+ * Arg:
+ * device : video output device (LCD, CRT, ..)
+ *
+ * Return Value:
+ * Maximum brightness level
+ *
+ * Allocate and initialize device->brightness.
+ */
+
+static int
+acpi_video_init_brightness(struct acpi_video_device *device)
+{
+ int i, max_level = 0;
+ unsigned long long level, level_old;
+ struct acpi_video_device_brightness *br = NULL;
+ int result = -EINVAL;
+
+ result = acpi_video_get_levels(device->dev, &br);
+ if (result)
+ return result;
device->brightness = br;
/* _BQC uses INDEX while _BCL uses VALUE in some laptops */
@@ -903,17 +914,13 @@ set_level:
goto out_free_levels;
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "found %d brightness levels\n", count - 2));
- kfree(obj);
- return result;
+ "found %d brightness levels\n", br->count - 2));
+ return 0;
out_free_levels:
kfree(br->levels);
-out_free:
kfree(br);
-out:
device->brightness = NULL;
- kfree(obj);
return result;
}
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index c37eedc35a24..cb1ba70ceaa7 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -338,31 +338,9 @@ config INTEL_QUARK_DTS_THERMAL
hot & critical. The critical trip point default value is set by
underlying BIOS/Firmware.
-config INT340X_THERMAL
- tristate "ACPI INT340X thermal drivers"
- depends on X86 && ACPI
- select THERMAL_GOV_USER_SPACE
- select ACPI_THERMAL_REL
- select ACPI_FAN
- select INTEL_SOC_DTS_IOSF_CORE
- select THERMAL_WRITABLE_TRIPS
- help
- Newer laptops and tablets that use ACPI may have thermal sensors and
- other devices with thermal control capabilities outside the core
- CPU/SOC, for thermal safety reasons.
- They are exposed for the OS to use via the INT3400 ACPI device object
- as the master, and INT3401~INT340B ACPI device objects as the slaves.
- Enable this to expose the temperature information and cooling ability
- from these objects to userspace via the normal thermal framework.
- This means that a wide range of applications and GUI widgets can show
- the information to the user or use this information for making
- decisions. For example, the Intel Thermal Daemon can use this
- information to allow the user to select his laptop to run without
- turning on the fans.
-
-config ACPI_THERMAL_REL
- tristate
- depends on ACPI
+menu "ACPI INT340X thermal drivers"
+source drivers/thermal/int340x_thermal/Kconfig
+endmenu
config INTEL_PCH_THERMAL
tristate "Intel PCH Thermal Reporting Driver"
diff --git a/drivers/thermal/int340x_thermal/Kconfig b/drivers/thermal/int340x_thermal/Kconfig
new file mode 100644
index 000000000000..0582bd12a239
--- /dev/null
+++ b/drivers/thermal/int340x_thermal/Kconfig
@@ -0,0 +1,42 @@
+#
+# ACPI INT340x thermal drivers configuration
+#
+
+config INT340X_THERMAL
+ tristate "ACPI INT340X thermal drivers"
+ depends on X86 && ACPI
+ select THERMAL_GOV_USER_SPACE
+ select ACPI_THERMAL_REL
+ select ACPI_FAN
+ select INTEL_SOC_DTS_IOSF_CORE
+ help
+ Newer laptops and tablets that use ACPI may have thermal sensors and
+ other devices with thermal control capabilities outside the core
+ CPU/SOC, for thermal safety reasons.
+ They are exposed for the OS to use via the INT3400 ACPI device object
+ as the master, and INT3401~INT340B ACPI device objects as the slaves.
+ Enable this to expose the temperature information and cooling ability
+ from these objects to userspace via the normal thermal framework.
+ This means that a wide range of applications and GUI widgets can show
+ the information to the user or use this information for making
+ decisions. For example, the Intel Thermal Daemon can use this
+ information to allow the user to select his laptop to run without
+ turning on the fans.
+
+config ACPI_THERMAL_REL
+ tristate
+ depends on ACPI
+
+if INT340X_THERMAL
+
+config INT3406_THERMAL
+ tristate "ACPI INT3406 display thermal driver"
+ depends on ACPI_VIDEO
+ help
+ The display thermal device represents the LED/LCD display panel
+ that may or may not include touch support. The main function of
+ the display thermal device is to allow control of the display
+ brightness in order to address a thermal condition or to reduce
+ power consumed by display device.
+
+endif
diff --git a/drivers/thermal/int340x_thermal/Makefile b/drivers/thermal/int340x_thermal/Makefile
index ba77a34f659f..df0df055e7ff 100644
--- a/drivers/thermal/int340x_thermal/Makefile
+++ b/drivers/thermal/int340x_thermal/Makefile
@@ -3,4 +3,5 @@ obj-$(CONFIG_INT340X_THERMAL) += int340x_thermal_zone.o
obj-$(CONFIG_INT340X_THERMAL) += int3402_thermal.o
obj-$(CONFIG_INT340X_THERMAL) += int3403_thermal.o
obj-$(CONFIG_INT340X_THERMAL) += processor_thermal_device.o
+obj-$(CONFIG_INT3406_THERMAL) += int3406_thermal.o
obj-$(CONFIG_ACPI_THERMAL_REL) += acpi_thermal_rel.o
diff --git a/drivers/thermal/int340x_thermal/int3406_thermal.c b/drivers/thermal/int340x_thermal/int3406_thermal.c
new file mode 100644
index 000000000000..1c9b7590f098
--- /dev/null
+++ b/drivers/thermal/int340x_thermal/int3406_thermal.c
@@ -0,0 +1,236 @@
+/*
+ * INT3406 thermal driver for display participant device
+ *
+ * Copyright (C) 2014, Intel Corporation
+ * Authors: Aaron Lu <aaron.lu@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/acpi.h>
+#include <linux/backlight.h>
+#include <linux/thermal.h>
+#include <acpi/video.h>
+
+#define INT3406_BRIGHTNESS_LIMITS_CHANGED 0x80
+
+struct int3406_thermal_data {
+ int upper_limit;
+ int upper_limit_index;
+ int lower_limit;
+ int lower_limit_index;
+ acpi_handle handle;
+ struct acpi_video_device_brightness *br;
+ struct backlight_device *raw_bd;
+ struct thermal_cooling_device *cooling_dev;
+};
+
+static int int3406_thermal_to_raw(int level, struct int3406_thermal_data *d)
+{
+ int max_level = d->br->levels[d->br->count - 1];
+ int raw_max = d->raw_bd->props.max_brightness;
+
+ return level * raw_max / max_level;
+}
+
+static int int3406_thermal_to_acpi(int level, struct int3406_thermal_data *d)
+{
+ int raw_max = d->raw_bd->props.max_brightness;
+ int max_level = d->br->levels[d->br->count - 1];
+
+ return level * max_level / raw_max;
+}
+
+static int
+int3406_thermal_get_max_state(struct thermal_cooling_device *cooling_dev,
+ unsigned long *state)
+{
+ struct int3406_thermal_data *d = cooling_dev->devdata;
+ int index = d->lower_limit_index ? d->lower_limit_index : 2;
+
+ *state = d->br->count - 1 - index;
+ return 0;
+}
+
+static int
+int3406_thermal_set_cur_state(struct thermal_cooling_device *cooling_dev,
+ unsigned long state)
+{
+ struct int3406_thermal_data *d = cooling_dev->devdata;
+ int level, raw_level;
+
+ if (state > d->br->count - 3)
+ return -EINVAL;
+
+ state = d->br->count - 1 - state;
+ level = d->br->levels[state];
+
+ if ((d->upper_limit && level > d->upper_limit) ||
+ (d->lower_limit && level < d->lower_limit))
+ return -EINVAL;
+
+ raw_level = int3406_thermal_to_raw(level, d);
+ return backlight_device_set_brightness(d->raw_bd, raw_level);
+}
+
+static int
+int3406_thermal_get_cur_state(struct thermal_cooling_device *cooling_dev,
+ unsigned long *state)
+{
+ struct int3406_thermal_data *d = cooling_dev->devdata;
+ int raw_level, level, i;
+ int *levels = d->br->levels;
+
+ raw_level = d->raw_bd->props.brightness;
+ level = int3406_thermal_to_acpi(raw_level, d);
+
+ /*
+ * There is no 1:1 mapping between the firmware interface level with the
+ * raw interface level, we will have to find one that is close enough.
+ */
+ for (i = 2; i < d->br->count; i++) {
+ if (level < levels[i]) {
+ if (i = 2)
+ break;
+ if ((level - levels[i - 1]) < (levels[i] - level))
+ i--;
+ break;
+ }
+ }
+
+ *state = d->br->count - 1 - i;
+ return 0;
+}
+
+static const struct thermal_cooling_device_ops video_cooling_ops = {
+ .get_max_state = int3406_thermal_get_max_state,
+ .get_cur_state = int3406_thermal_get_cur_state,
+ .set_cur_state = int3406_thermal_set_cur_state,
+};
+
+static int int3406_thermal_get_index(int *array, int nr, int value)
+{
+ int i;
+
+ for (i = 0; i < nr; i++) {
+ if (array[i] = value)
+ break;
+ }
+ return i = nr ? -ENOENT : i;
+}
+
+static void int3406_thermal_get_limit(struct int3406_thermal_data *d)
+{
+ acpi_status status;
+ unsigned long long lower_limit, upper_limit;
+ int index;
+
+ status = acpi_evaluate_integer(d->handle, "DDDL", NULL, &lower_limit);
+ if (ACPI_SUCCESS(status)) {
+ index = int3406_thermal_get_index(d->br->levels, d->br->count,
+ lower_limit);
+ if (index > 0) {
+ d->lower_limit = (int)lower_limit;
+ d->lower_limit_index = index;
+ }
+ }
+
+ status = acpi_evaluate_integer(d->handle, "DDPC", NULL, &upper_limit);
+ if (ACPI_SUCCESS(status)) {
+ index = int3406_thermal_get_index(d->br->levels, d->br->count,
+ upper_limit);
+ if (index > 0) {
+ d->upper_limit = (int)upper_limit;
+ d->upper_limit_index = index;
+ }
+ }
+}
+
+static void int3406_notify(acpi_handle handle, u32 event, void *data)
+{
+ if (event = INT3406_BRIGHTNESS_LIMITS_CHANGED)
+ int3406_thermal_get_limit(data);
+}
+
+static int int3406_thermal_probe(struct platform_device *pdev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+ struct int3406_thermal_data *d;
+ struct backlight_device *bd;
+ int ret;
+
+ if (!ACPI_HANDLE(&pdev->dev))
+ return -ENODEV;
+
+ d = devm_kzalloc(&pdev->dev, sizeof(*d), GFP_KERNEL);
+ if (!d)
+ return -ENOMEM;
+ d->handle = ACPI_HANDLE(&pdev->dev);
+
+ bd = backlight_device_get_by_type(BACKLIGHT_RAW);
+ if (!bd)
+ return -ENODEV;
+ d->raw_bd = bd;
+
+ ret = acpi_video_get_levels(ACPI_COMPANION(&pdev->dev), &d->br);
+ if (ret)
+ return ret;
+
+ int3406_thermal_get_limit(d);
+
+ d->cooling_dev = thermal_cooling_device_register(acpi_device_bid(adev),
+ d, &video_cooling_ops);
+ if (IS_ERR(d->cooling_dev))
+ goto err;
+
+ ret = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY,
+ int3406_notify, d);
+ if (ret)
+ goto err_cdev;
+
+ platform_set_drvdata(pdev, d);
+
+ return 0;
+
+err_cdev:
+ thermal_cooling_device_unregister(d->cooling_dev);
+err:
+ kfree(d->br);
+ return -ENODEV;
+}
+
+static int int3406_thermal_remove(struct platform_device *pdev)
+{
+ struct int3406_thermal_data *d = platform_get_drvdata(pdev);
+
+ thermal_cooling_device_unregister(d->cooling_dev);
+ kfree(d->br);
+ return 0;
+}
+
+static const struct acpi_device_id int3406_thermal_match[] = {
+ {"INT3406", 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(acpi, int3406_thermal_match);
+
+static struct platform_driver int3406_thermal_driver = {
+ .probe = int3406_thermal_probe,
+ .remove = int3406_thermal_remove,
+ .driver = {
+ .name = "int3406 thermal",
+ .owner = THIS_MODULE,
+ .acpi_match_table = int3406_thermal_match,
+ },
+};
+
+module_platform_driver(int3406_thermal_driver);
+
+MODULE_DESCRIPTION("INT3406 Thermal driver");
+MODULE_LICENSE("GPL");
diff --git a/include/acpi/video.h b/include/acpi/video.h
index 5ca2f2c16458..a4b96c971564 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -4,6 +4,19 @@
#include <linux/errno.h> /* for ENODEV */
#include <linux/types.h> /* for bool */
+struct acpi_video_brightness_flags {
+ u8 _BCL_no_ac_battery_levels:1; /* no AC/Battery levels in _BCL */
+ u8 _BCL_reversed:1; /* _BCL package is in a reversed order */
+ u8 _BQC_use_index:1; /* _BQC returns an index value */
+};
+
+struct acpi_video_device_brightness {
+ int curr;
+ int count;
+ int *levels;
+ struct acpi_video_brightness_flags flags;
+};
+
struct acpi_device;
#define ACPI_VIDEO_CLASS "video"
@@ -37,6 +50,8 @@ extern void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type);
* may change over time and should not be cached.
*/
extern bool acpi_video_handles_brightness_key_presses(void);
+extern int acpi_video_get_levels(struct acpi_device *device,
+ struct acpi_video_device_brightness **dev_br);
#else
static inline int acpi_video_register(void) { return 0; }
static inline void acpi_video_unregister(void) { return; }
@@ -56,6 +71,11 @@ static inline bool acpi_video_handles_brightness_key_presses(void)
{
return false;
}
+static int acpi_video_get_levels(struct acpi_device *device,
+ struct acpi_video_device_brightness **dev_br)
+{
+ return -ENODEV;
+}
#endif
#endif
--
2.5.5
^ permalink raw reply related
* Re: [PATCH v5 03/46] backlight: lm3630a_bl: stop messing with the pwm->period field
From: Lee Jones @ 2016-04-13 8:25 UTC (permalink / raw)
To: Thierry Reding
Cc: Milo Kim, Kamil Debski, Heiko Stuebner, linux-doc, David Airlie,
Mike Turquette, linux-fbdev, dri-devel, linux-kernel, linux-sunxi,
Alexandre Belloni, Daniel Vetter, Robert Jarzmik, linux-clk,
linux-leds, Boris Brezillon, Krzysztof Kozlowski,
linux-samsung-soc, Alexander Shiyan, Jonathan Corbet, lm-sensors,
linux-rockchip, Chen-Yu Tsai, Tomi Valkeinen, linux-input
In-Reply-To: <20160412142608.GB25006@ulmo.ba.sec>
On Tue, 12 Apr 2016, Thierry Reding wrote:
> On Tue, Apr 12, 2016 at 03:16:13PM +0100, Lee Jones wrote:
> > On Tue, 12 Apr 2016, Thierry Reding wrote:
> >
> > > On Wed, Mar 30, 2016 at 10:03:26PM +0200, Boris Brezillon wrote:
> > > > pwm->period field is not supposed to be changed by PWM users. The only
> > > > ones authorized to change it are the PWM core and PWM drivers.
> > > >
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > ---
> > > > drivers/video/backlight/lm3630a_bl.c | 3 +--
> > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > Applied, thanks.
> >
> > Applied?
>
> You didn't specifically Ack this one, but I presumed that since the
> change is essentially the same as for pwm-backlight, and this is another
> prerequisite for the remainder of the series it should go in through the
> PWM tree as well.
If you're taking Backlight patches through a !Backlight tree, you're
going to have to send out a pull-request. There are no conflicts
currently, so I won't use it right away, but I need it in my arsenal
if they do occur.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v5 03/46] backlight: lm3630a_bl: stop messing with the pwm->period field
From: Lee Jones @ 2016-04-13 8:26 UTC (permalink / raw)
To: Thierry Reding
Cc: Boris Brezillon, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown,
Liam Girdwood, Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
Jean Delvare, Guenter Roeck, Dmitry Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA, Bryan Wu, Richard Purdie,
Jacek Anaszewski, linux-leds-u79uwXL29TY76Z2rM5mHXA,
Maxime Ripard, Chen-Yu Tsai, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
Joachim Eastwood, Thomas Petazzoni, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20160413082511.GL8094@x1>
On Wed, 13 Apr 2016, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
>
> > On Tue, Apr 12, 2016 at 03:16:13PM +0100, Lee Jones wrote:
> > > On Tue, 12 Apr 2016, Thierry Reding wrote:
> > >
> > > > On Wed, Mar 30, 2016 at 10:03:26PM +0200, Boris Brezillon wrote:
> > > > > pwm->period field is not supposed to be changed by PWM users. The only
> > > > > ones authorized to change it are the PWM core and PWM drivers.
> > > > >
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > ---
> > > > > drivers/video/backlight/lm3630a_bl.c | 3 +--
> > > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > Applied, thanks.
> > >
> > > Applied?
> >
> > You didn't specifically Ack this one, but I presumed that since the
> > change is essentially the same as for pwm-backlight, and this is another
> > prerequisite for the remainder of the series it should go in through the
> > PWM tree as well.
>
> If you're taking Backlight patches through a !Backlight tree, you're
> going to have to send out a pull-request. There are no conflicts
> currently, so I won't use it right away, but I need it in my arsenal
> if they do occur.
... also, don't forget to change the subject line to make the
rest of the patches in Backlight. The subject description needs to
start with an uppercase char.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ 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