* Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate
From: Thierry Reding @ 2016-04-04 15:20 UTC (permalink / raw)
To: Boris Brezillon
Cc: Guenter Roeck, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown,
Liam Girdwood, Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
Jean Delvare, 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: <20160331090709.5c3bdf70@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]
On Thu, Mar 31, 2016 at 09:07:09AM +0200, Boris Brezillon wrote:
> Hi Guenter,
>
> On Wed, 30 Mar 2016 15:52:44 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
>
> > On Wed, Mar 30, 2016 at 10:03:31PM +0200, Boris Brezillon wrote:
> > > The PWM framework has clarified the concept of reference PWM config
> > > (the platform dependent config retrieved from the DT or the PWM
> > > lookup table) and real PWM state.
> > >
> > > Use pwm_get_args() when the PWM user wants to retrieve this reference
> > > config and not the current state.
> > >
> > > This is part of the rework allowing the PWM framework to support
> > > hardware readout and expose real PWM state even when the PWM has
> > > just been requested (before the user calls pwm_config/enable/disable()).
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > ---
> > > drivers/hwmon/pwm-fan.c | 19 +++++++++++++------
> > > 1 file changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > > index 3e23003..82c5656 100644
> > > --- a/drivers/hwmon/pwm-fan.c
> > > +++ b/drivers/hwmon/pwm-fan.c
> > > @@ -40,15 +40,18 @@ struct pwm_fan_ctx {
> > >
> > > static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> > > {
> > > + struct pwm_args pargs = { };
> >
> > Hi Boris,
> >
> > I guess I am missing some context; sorry for that. Unfortunately,
> > I did not easily find an explanation, so please bear with me.
> >
> > Two questions: Why do we need a local copy of struct pwm_args instead
> > of a pointer to it ? If it can change while being used, isn't it
> > inconsistent anyway ?
>
> It cannot change after pwm_get() is called. For the reason behind
> prototype: I just followed the Thierry's proposal, but I'm perfectly
> fine returning a const struct pwm_args pointer intead of passing
> pwm_args as a parameter.
>
> Thierry, what's your opinion?
I do prefer the current variant because it is more consistent with the
new atomic API, even if not strictly necessary because of the immutable
data.
> > Also, assuming the local copy is necessary, why initialize pargs ?
> > After all, pwm_get_args() just overwrites it.
>
> It's a leftover from a previous version where pwm_get_args was
> implemented this way:
>
> static inline void pwm_get_args(pwm, args)
> {
> if (pwm)
> *args = pwm->args
> }
>
> and this implementation was generating a lot of 'uninitialized
> variable' warnings.
>
> I just decided to drop the 'if (pwm)' test, because, IMO, this
> should be checked way before calling pwm_get_args() is called.
Sounds fine to me.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 32/46] pwm: deprecate pwm_config(), pwm_enable() and pwm_disable()
From: Thierry Reding @ 2016-04-04 15:22 UTC (permalink / raw)
To: Boris Brezillon
Cc: Dmitry Torokhov, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown,
Liam Girdwood, Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
Jean Delvare, Guenter Roeck, 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.jon>
In-Reply-To: <20160331205454.630734ab@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]
On Thu, Mar 31, 2016 at 08:54:54PM +0200, Boris Brezillon wrote:
> Hi Dmitry,
>
> On Thu, 31 Mar 2016 10:38:58 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> > Hi Boris,
> >
> > On Wed, Mar 30, 2016 at 10:03:55PM +0200, Boris Brezillon wrote:
> > > Prefix those function as deprecated to encourage all existing users to
> > > switch to pwm_apply_state().
> >
> > Why not keep at least some of them as wrappers where we do not need to
> > chnage several parameters at once? It is much easier to have a driver
> > do:
> >
> > error = pwm_enable(pwm);
> > if (error)
> > ...
> >
> > rather than declaring the state variable, fectch it, adjust and then
> > apply.
>
> True. Actually deprecating the non-atomic API was not my primary goal.
> Thierry would you mind if we keep both APIs around?
I'm fine with keeping these around, though purely as shortcuts. If users
need to modify two parameters at once (e.g. duty cycle and enable) then
they should be converted to use the atomic API, otherwise there'd be
little point in introduce it.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 34/46] clk: pwm: switch to the atomic API
From: Thierry Reding @ 2016-04-04 15:30 UTC (permalink / raw)
To: Boris Brezillon
Cc: Stephen Boyd, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
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: <20160331085735.3574970b@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]
On Thu, Mar 31, 2016 at 08:57:35AM +0200, Boris Brezillon wrote:
> Hi Stephen,
>
> On Wed, 30 Mar 2016 15:01:49 -0700
> Stephen Boyd <sboyd@codeaurora.org> wrote:
>
> > On 03/30, Boris Brezillon wrote:
> > > diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
> > > index ebcd738..49ec5b1 100644
> > > --- a/drivers/clk/clk-pwm.c
> > > +++ b/drivers/clk/clk-pwm.c
> > > @@ -28,15 +28,29 @@ static inline struct clk_pwm *to_clk_pwm(struct clk_hw *hw)
> > > static int clk_pwm_prepare(struct clk_hw *hw)
> > > {
> > > struct clk_pwm *clk_pwm = to_clk_pwm(hw);
> > > + struct pwm_state pstate;
> > >
> > > - return pwm_enable(clk_pwm->pwm);
> > > + pwm_get_state(clk_pwm->pwm, &pstate);
> > > + if (pstate.enabled)
> > > + return 0;
> > > +
> > > + pstate.enabled = true;
> > > +
> > > + return pwm_apply_state(clk_pwm->pwm, &pstate);
> >
> > This doesn't seem atomic anymore if we're checking the state and
> > then not calling apply_state if it's already enabled. But I
> > assume this doesn't matter because we "own" the pwm here?
>
> Yep. Actually it's not atomic in term of concurrency (maybe the
> 'atomic' word is not appropriate here). Atomicity is here referring to
> the fact that we're now providing all the PWM parameters in the same
> request instead of splitting it in pwm_config() + pwm_enable/disable()
> calls.
It's usually not possible to do really atomic updates with PWM hardware.
The idea is merely that we should be able to submit one request and the
framework (and drivers) will be responsible for making sure it is
applied as a whole or not at all. With the legacy API it is possible for
users to set the duty cycle and period, but then fail to enable/disable
the PWM.
pwm_apply_state() reporting success should indicate that the hardware
state is now what software wanted it to be. That kind of implies that
the application is serialized.
This doesn't imply that hardware state won't change between a call to
pwm_get_state() and pwm_apply_state(), though technically this is what
will usually happen because PWM devices are exclusively used by a single
user. Users are responsible for synchronizing accesses within their own
code.
> Concurrent accesses still have to be controlled by the PWM user (which
> is already the case for this driver, thanks to the locking
> infrastructure in the CCF).
>
> > Otherwise I would think this would be unconditional apply state
> > and duplicates would be ignored in the pwm framework.
> >
>
> Yep, I'll remove the if (pstate.enabled) branch.
Yes, it should be the PWM framework's job to check for changes in state
and discard no-ops.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 36/46] input: misc: max77693: switch to the atomic API
From: Thierry Reding @ 2016-04-04 15:34 UTC (permalink / raw)
To: Boris Brezillon
Cc: Dmitry Torokhov, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, Mark Brown,
Liam Girdwood, Kamil Debski, lm-sensors-GZX6beZjE8VD60Wz+7aTrA,
Jean Delvare, Guenter Roeck, 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.jon>
In-Reply-To: <20160331205718.057a6f38@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 2143 bytes --]
On Thu, Mar 31, 2016 at 08:57:18PM +0200, Boris Brezillon wrote:
> On Thu, 31 Mar 2016 10:48:01 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> > Hi Boris,
> >
> > On Wed, Mar 30, 2016 at 10:03:59PM +0200, Boris Brezillon wrote:
> > > pwm_config/enable/disable() have been deprecated and should be replaced
> > > by pwm_apply_state().
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > ---
> > > drivers/input/misc/max77693-haptic.c | 23 +++++++++++++++++------
> > > 1 file changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c
> > > index cf6aac0..aef7dc4 100644
> > > --- a/drivers/input/misc/max77693-haptic.c
> > > +++ b/drivers/input/misc/max77693-haptic.c
> > > @@ -70,13 +70,16 @@ struct max77693_haptic {
> > >
> > > static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic)
> > > {
> > > + struct pwm_state pstate;
> > > struct pwm_args pargs = { };
> > > - int delta;
> > > int error;
> > >
> > > pwm_get_args(haptic->pwm_dev, &pargs);
> > > - delta = (pargs.period + haptic->pwm_duty) / 2;
> > > - error = pwm_config(haptic->pwm_dev, delta, pargs.period);
> > > + pwm_get_state(haptic->pwm_dev, &pstate);
> > > +
> > > + pstate.period = pargs.period;
> > > + pstate.duty_cycle = (pargs.period + haptic->pwm_duty) / 2;
> > > + error = pwm_apply_state(haptic->pwm_dev, &pstate);
> >
> > This does not make sense with regard to the atomic API. If you look in
> > max77693_haptic_play_work(), right after calling
> > max77693_haptic_set_duty_cycle() we either try to enable or disable the
> > pwm. When switching to this new API we should combine both actions.
>
> True. I'll address that, unless Thierry is fine keeping the non-atomic
> API, in which case I'll just drop patches 32 to 46.
I'm fine with keeping the pwm_enable(), pwm_disable() and pwm_config()
APIs, but they should only be used as shortcuts. Where possible the new
atomic API should be used to combine multiple operations into one.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* RE: [PATCH v2] video: AMBA CLCD: Remove unncessary include in amba-clcd.c
From: Wang, Annie @ 2016-04-05 6:56 UTC (permalink / raw)
To: Wang, Annie, Russell King, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Borislav Petkov, SPG_Linux_Kernel
In-Reply-To: <1457922549-2610-1-git-send-email-annie.wang@amd.com>
Hi Tomi,
Any comments of this patch?
Regards,
Hongcheng Wang(Annie)
>-----Original Message-----
>From: Wang Hongcheng [mailto:annie.wang@amd.com]
>Sent: Monday, March 14, 2016 10:29 AM
>To: Russell King; Jean-Christophe Plagniol-Villard; Tomi Valkeinen; linux-
>fbdev@vger.kernel.org; linux-kernel@vger.kernel.org; Borislav Petkov;
>SPG_Linux_Kernel
>Cc: Wang, Annie
>Subject: [PATCH v2] video: AMBA CLCD: Remove unncessary include in amba-
>clcd.c
>
>The header file asm/sizes.h is unnecessary.
>And it can also be compiled under X86 arch after the removal.
>
>Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
>---
> drivers/video/fbdev/amba-clcd.c | 2 --
> 1 file changed, 2 deletions(-)
>
>diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
>index 9362424..1a1ed80 100644
>--- a/drivers/video/fbdev/amba-clcd.c
>+++ b/drivers/video/fbdev/amba-clcd.c
>@@ -34,8 +34,6 @@
> #include <video/of_display_timing.h>
> #include <video/videomode.h>
>
>-#include <asm/sizes.h>
>-
> #define to_clcd(info) container_of(info, struct clcd_fb, fb)
>
> /* This is limited to 16 characters when displayed by X startup */
>--
>1.9.1
^ permalink raw reply
* Re: [PATCH 1/7] cpufreq: remove redundant CPUFREQ_INCOMPATIBLE notifier event
From: Saravana Kannan @ 2016-04-06 21:29 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Viresh Kumar, Rafael J. Wysocki, Lists linaro-kernel,
linux-pm@vger.kernel.org, Dmitry Eremin-Solenikov,
Fabian Frederick, Jean-Christophe Plagniol-Villard,
Jonathan Corbet, Len Brown, open, list@codeaurora.org:ACPI
In-Reply-To: <CAJZ5v0iUdkYrEzKrGDEiDnexsxfY7o9xOHPoWELk88gau=gAhA@mail.gmail.com>
On 04/06/2016 02:21 PM, Rafael J. Wysocki wrote:
> On Wed, Apr 6, 2016 at 10:30 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
>> On 09/09/2015 05:53 PM, Rafael J. Wysocki wrote:
>>>
>>> Hi,
>>>
>>> On Thu, Sep 10, 2015 at 2:39 AM, Viresh Kumar <viresh.kumar@linaro.org>
>>> wrote:
>>>>
>>>> On 10-09-15, 01:26, Rafael J. Wysocki wrote:
>>>>>
>>>>> On Monday, August 03, 2015 08:36:14 AM Viresh Kumar wrote:
>>>>>>
>>>>>> What's being done from CPUFREQ_INCOMPATIBLE, can also be done with
>>>>>> CPUFREQ_ADJUST. There is nothing special with CPUFREQ_INCOMPATIBLE
>>>>>> notifier.
>>>>>
>>>>>
>>>>> The above part of the changelog is a disaster to me. :-(
>>>>>
>>>>> It not only doesn't explain what really goes on, but it's actively
>>>>> confusing.
>>>>>
>>>>> What really happens is that the core sends CPUFREQ_INCOMPATIBLE
>>>>> notifications
>>>>> unconditionally right after sending the CPUFREQ_ADJUST ones, so the
>>>>> former is
>>>>> just redundant and it's more efficient to merge the two into one.
>>>>
>>>>
>>>> Undoubtedly this looks far better :)
>>>>
>>>> But, isn't this series already applied some time back ?
>>>
>>>
>>> Right, never mind. For some reason that patch was left in the "New"
>>> state.
>>>
>>> The code is OK.
>>
>>
>>
>> I guess I didn't notice this change when it was sent out.
>>
>> The comment that was deleted in this patch clearly states why the
>> INCOMPATIBLE notifier is needed. Some client might want to boost the CPU min
>> freq for performance or other reasons, but thermal might want to limit it.
>> So, by having thermal register for INCOMPATIBLE notifiers to enforce the
>> limits, we provide a way to guarantee it gets the final say.
>>
>> The real fix should have been to change drivers/thermal/cpu_cooling.c to use
>> CPUFREQ_INCOMPATIBLE instead of CPUFREQ_ADJUST.
>>
>> Is there something I'm missing? If not, can we please revert this patch?
>
> Well, nobody was using that event.
>
True, but that's more of a bug in drivers/thermal/cpu-cooling.c and
drivers/acpi/processor_thermal.c. We should revert this patch and fix
those drivers. Does that seem acceptable to you?
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH 1/7] cpufreq: remove redundant CPUFREQ_INCOMPATIBLE notifier event
From: Rafael J. Wysocki @ 2016-04-06 21:45 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rafael J. Wysocki, Viresh Kumar, Rafael J. Wysocki,
Lists linaro-kernel, linux-pm@vger.kernel.org,
Dmitry Eremin-Solenikov, Fabian Frederick,
Jean-Christophe Plagniol-Villard, Jonathan Corbet, Len Brown,
open, list@codeaurora.org:ACPI, list@codeaurora.org:DOCUMENTATION,
list@codeaurora.org:FRAMEBUFFER LAYER, open list,
Nicholas Mc Guire, Russell King, Tomi Valkeinen, Wolfram Sang
In-Reply-To: <57057FB1.2050904@codeaurora.org>
On Wed, Apr 6, 2016 at 11:29 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 04/06/2016 02:21 PM, Rafael J. Wysocki wrote:
>>
>> On Wed, Apr 6, 2016 at 10:30 PM, Saravana Kannan <skannan@codeaurora.org>
>> wrote:
>>>
>>> On 09/09/2015 05:53 PM, Rafael J. Wysocki wrote:
>>>>
[cut]
>>
>> Well, nobody was using that event.
>>
>
> True, but that's more of a bug in drivers/thermal/cpu-cooling.c and
> drivers/acpi/processor_thermal.c. We should revert this patch and fix those
> drivers. Does that seem acceptable to you?
I'd rather see a patch series adding the event back along with some
users. One user at least.
^ permalink raw reply
* Re: [PATCH 1/7] cpufreq: remove redundant CPUFREQ_INCOMPATIBLE notifier event
From: Saravana Kannan @ 2016-04-06 21:49 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Viresh Kumar, Rafael J. Wysocki, Lists linaro-kernel,
linux-pm@vger.kernel.org, Dmitry Eremin-Solenikov,
Fabian Frederick, Jean-Christophe Plagniol-Villard,
Jonathan Corbet, Len Brown, open, list@codeaurora.org:ACPI,
list@codeaurora.org:DOCUMENTATION,
list@codeaurora.org:FRAMEBUFFER LAYER, open list,
Nicholas Mc Guire, Russell King, Tomi Valkeinen, Wolfram Sang
In-Reply-To: <CAJZ5v0igB+iQSZO_GngNwRymVKTCKx2YK4BysO2KFXzpMqWwyw@mail.gmail.com>
On 04/06/2016 02:45 PM, Rafael J. Wysocki wrote:
> On Wed, Apr 6, 2016 at 11:29 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
>> On 04/06/2016 02:21 PM, Rafael J. Wysocki wrote:
>>>
>>> On Wed, Apr 6, 2016 at 10:30 PM, Saravana Kannan <skannan@codeaurora.org>
>>> wrote:
>>>>
>>>> On 09/09/2015 05:53 PM, Rafael J. Wysocki wrote:
>>>>>
>
> [cut]
>
>>>
>>> Well, nobody was using that event.
>>>
>>
>> True, but that's more of a bug in drivers/thermal/cpu-cooling.c and
>> drivers/acpi/processor_thermal.c. We should revert this patch and fix those
>> drivers. Does that seem acceptable to you?
>
> I'd rather see a patch series adding the event back along with some
> users. One user at least.
>
Ok, I'll make those two drivers use them and send it out. It's very
clearly a bug in those drivers.
-Saravana`
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage
From: Boris Brezillon @ 2016-04-07 21:54 UTC (permalink / raw)
To: Mark Brown
Cc: Thierry Reding, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, 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
In-Reply-To: <20160330212410.GX2350-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
Hi Mark,
On Wed, 30 Mar 2016 14:24:10 -0700
Mark Brown <broonie@kernel.org> wrote:
> On Wed, Mar 30, 2016 at 10:03:53PM +0200, Boris Brezillon wrote:
> > The continuous PWM voltage regulator is caching the voltage value in
> > the ->volt_uV field. While most of the time this value should reflect the
> > real voltage, sometime it can be sightly different if the PWM device
> > rounded the set_duty_cycle request.
> > Moreover, this value is not valid until someone has modified the regulator
> > output.
>
> Acked-by: Mark Brown <broonie@kernel.org>
Actually this patch introduces a bug (reported by Stephen):
"
I applied your patch series [PATCH v5 00/46] pwm: add support for
atomic update and found a null pointer dereference when probing a
pwm-regulator at boot. See the below stack trace:
[ 4.282374] [<ffffffc000399088>] pwm_regulator_get_voltage+0x78/0xa0
[ 4.289344] [<ffffffc000390740>] regulator_attr_is_visible+0x7c/0x264
[ 4.296408] [<ffffffc0001f75a0>] internal_create_group+0x14c/0x280
[ 4.303184] [<ffffffc0001f76e8>] sysfs_create_group+0x14/0x1c
[ 4.309483] [<ffffffc0001f77c8>] sysfs_create_groups+0x30/0x78
[ 4.315881] [<ffffffc00043631c>] device_add+0x224/0x4d8
[ 4.321609] [<ffffffc0004365ec>] device_register+0x1c/0x28
[ 4.327623] [<ffffffc0003952a4>] regulator_register+0x2e4/0xc14
[ 4.334112] [<ffffffc000396844>] devm_regulator_register+0x54/0x94
[ 4.340887] [<ffffffc000399328>] pwm_regulator_probe+0x278/0x2b8
[ 4.347473] [<ffffffc000439fe4>] platform_drv_probe+0x58/0xa4
[ 4.353772] [<ffffffc0004387a8>] driver_probe_device+0x114/0x2ac
[ 4.360358] [<ffffffc0004389a4>] __driver_attach+0x64/0x90
[ 4.366371] [<ffffffc000436f50>] bus_for_each_dev+0x74/0x90
[ 4.372478] [<ffffffc000438bd4>] driver_attach+0x20/0x28
[ 4.378299] [<ffffffc00043778c>] bus_add_driver+0xe8/0x1e0
[ 4.384312] [<ffffffc00043959c>] driver_register+0x98/0xe4
[ 4.390326] [<ffffffc00043aa04>] __platform_driver_register+0x48/0x50
[ 4.397388] [<ffffffc000cb2710>] pwm_regulator_driver_init+0x18/0x20
[ 4.404356] [<ffffffc000c8ca7c>] do_one_initcall+0xf8/0x180
[ 4.410466] [<ffffffc000c8cc58>] kernel_init_freeable+0x154/0x1f4
[ 4.417148] [<ffffffc000929cf4>] kernel_init+0x10/0xf8
[ 4.422782] [<ffffffc000084450>] ret_from_fork+0x10/0x40
It looks like the root cause is that regulator_attr_is_visible will
try to get the voltage, but at this point in regulator_register,
rdev->constraints is still null. So
pwm_duty_cycle_percentage_to_voltage will dereference a null
rdev->constraints pointer.
"
The problem is that we need to know the min and max voltage constraints
to calculate the current voltage. ->get_voltage() is called when the
sysfs attributes are created (part of device registration), and
set_machine_constraints() is called after device_register(), thus
leading to the NULL pointer dereference.
Is there any reason for calling set_machine_constraints() after
device_register() in regulator_register()?
Best Regards,
Boris
^ permalink raw reply
* [PATCH 1/4] video: goldfishfb: add devicetree bindings
From: Alan @ 2016-04-11 13:30 UTC (permalink / raw)
To: linux-fbdev
From: Greg Hackmann <ghackmann@google.com>
Add device tree bindings to the Goldfish frame buffer interface.
Signed-off-by: Greg Hackmann <ghackmann@google.com>
Signed-off-by: Jin Qian <jinqian@android.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
Documentation/devicetree/bindings/goldfish/fb.txt | 17 +++++++++++++++++
drivers/video/fbdev/goldfishfb.c | 9 ++++++++-
2 files changed, 25 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/goldfish/fb.txt
diff --git a/Documentation/devicetree/bindings/goldfish/fb.txt b/Documentation/devicetree/bindings/goldfish/fb.txt
new file mode 100644
index 0000000..8e7a6f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/goldfish/fb.txt
@@ -0,0 +1,17 @@
+Android Goldfish Framebuffer
+
+Android goldfish framebuffer device generated by android emulator.
+
+Required properties:
+
+- compatible : should contain "google,goldfish-fb" to match emulator
+- reg : <registers mapping>
+- interrupts : <interrupt mapping>
+
+Example:
+
+ goldfish_fb@9010000 {
+ compatible = "google,goldfish-fb";
+ reg = <0x9010000 0x100>;
+ interrupts = <0x2>;
+ };
diff --git a/drivers/video/fbdev/goldfishfb.c b/drivers/video/fbdev/goldfishfb.c
index 7f6c9e6..f0e651b 100644
--- a/drivers/video/fbdev/goldfishfb.c
+++ b/drivers/video/fbdev/goldfishfb.c
@@ -304,12 +304,19 @@ static int goldfish_fb_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id goldfish_fb_of_match[] = {
+ { .compatible = "google,goldfish-fb", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, goldfish_fb_of_match);
static struct platform_driver goldfish_fb_driver = {
.probe = goldfish_fb_probe,
.remove = goldfish_fb_remove,
.driver = {
- .name = "goldfish_fb"
+ .name = "goldfish_fb",
+ .owner = THIS_MODULE,
+ .of_match_table = goldfish_fb_of_match,
}
};
^ permalink raw reply related
* [PATCH 2/4] goldfish: Enable ACPI-based enumeration for goldfish framebuffer
From: Alan @ 2016-04-11 13:30 UTC (permalink / raw)
To: linux-fbdev
From: Yu Ning <yu.ning@intel.com>
Enable ACPI bindings for the Goldfish framebuffer device.
Signed-off-by: Yu Ning <yu.ning@intel.com>
Signed-off-by: Jin Qian <jinqian@android.com>
Signed-off-by: Alan <alan@linux.intel.com>
---
drivers/video/fbdev/goldfishfb.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/video/fbdev/goldfishfb.c b/drivers/video/fbdev/goldfishfb.c
index f0e651b..58b33e4 100644
--- a/drivers/video/fbdev/goldfishfb.c
+++ b/drivers/video/fbdev/goldfishfb.c
@@ -26,6 +26,7 @@
#include <linux/interrupt.h>
#include <linux/ioport.h>
#include <linux/platform_device.h>
+#include <linux/acpi.h>
enum {
FB_GET_WIDTH = 0x00,
@@ -310,6 +311,12 @@ static const struct of_device_id goldfish_fb_of_match[] = {
};
MODULE_DEVICE_TABLE(of, goldfish_fb_of_match);
+static const struct acpi_device_id goldfish_fb_acpi_match[] = {
+ { "GFSH0004", 0 },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, goldfish_fb_acpi_match);
+
static struct platform_driver goldfish_fb_driver = {
.probe = goldfish_fb_probe,
.remove = goldfish_fb_remove,
@@ -317,6 +324,7 @@ static struct platform_driver goldfish_fb_driver = {
.name = "goldfish_fb",
.owner = THIS_MODULE,
.of_match_table = goldfish_fb_of_match,
+ .acpi_match_table = ACPI_PTR(goldfish_fb_acpi_match),
}
};
^ permalink raw reply related
* [PATCH 3/4] goldfish_fb: Set pixclock = 0
From: Alan @ 2016-04-11 13:30 UTC (permalink / raw)
To: linux-fbdev
From: Christoffer Dall <christoffer.dall@linaro.org>
User space Android code identifies pixclock = 0 as a sign for
emulation and will set the frame rate to 60 fps when reading
this value, which is the desired outcome.
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jin Qian <jinqian@android.com>
Signed-off-by: Alan <alan@linux.intel.com>
---
drivers/video/fbdev/goldfishfb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/goldfishfb.c b/drivers/video/fbdev/goldfishfb.c
index 58b33e4..131fee0 100644
--- a/drivers/video/fbdev/goldfishfb.c
+++ b/drivers/video/fbdev/goldfishfb.c
@@ -235,7 +235,7 @@ static int goldfish_fb_probe(struct platform_device *pdev)
fb->fb.var.activate = FB_ACTIVATE_NOW;
fb->fb.var.height = readl(fb->reg_base + FB_GET_PHYS_HEIGHT);
fb->fb.var.width = readl(fb->reg_base + FB_GET_PHYS_WIDTH);
- fb->fb.var.pixclock = 10000;
+ fb->fb.var.pixclock = 0;
fb->fb.var.red.offset = 11;
fb->fb.var.red.length = 5;
^ permalink raw reply related
* [PATCH 4/4] goldfish: 32 bit framebuffer support
From: Alan @ 2016-04-11 13:30 UTC (permalink / raw)
To: linux-fbdev
From: bohu <bohu@google.com>
Add support for display formats and allow the use of both 16-bit and
32-bit framebuffers in the emulator. Merged with the clean up patch from
Nicolas Capens as requested.
Signed-off-by: Bo Hu <bohu@google.com>
Signed-off-by: Nicolas Capens <capn@google.com>
Signed-off-by: Jin Qian <jinqian@android.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
drivers/video/fbdev/goldfishfb.c | 84 ++++++++++++++++++++++++++++++++------
1 file changed, 70 insertions(+), 14 deletions(-)
diff --git a/drivers/video/fbdev/goldfishfb.c b/drivers/video/fbdev/goldfishfb.c
index 131fee0..2a5fe71 100644
--- a/drivers/video/fbdev/goldfishfb.c
+++ b/drivers/video/fbdev/goldfishfb.c
@@ -38,11 +38,52 @@ enum {
FB_SET_BLANK = 0x18,
FB_GET_PHYS_WIDTH = 0x1c,
FB_GET_PHYS_HEIGHT = 0x20,
+ FB_GET_FORMAT = 0x24,
FB_INT_VSYNC = 1U << 0,
FB_INT_BASE_UPDATE_DONE = 1U << 1
};
+/* These values *must* match the platform definitions found under
+ * <system/graphics.h> in the Goldfish emulator.
+ */
+enum {
+ HAL_PIXEL_FORMAT_RGBA_8888 = 1,
+ HAL_PIXEL_FORMAT_RGBX_8888 = 2,
+ HAL_PIXEL_FORMAT_RGB_888 = 3,
+ HAL_PIXEL_FORMAT_RGB_565 = 4,
+ HAL_PIXEL_FORMAT_BGRA_8888 = 5,
+};
+
+struct framebuffer_config {
+ u8 bytes_per_pixel;
+ u8 red_offset;
+ u8 red_length;
+ u8 green_offset;
+ u8 green_length;
+ u8 blue_offset;
+ u8 blue_length;
+ u8 transp_offset;
+ u8 transp_length;
+};
+
+static const struct framebuffer_config fb_configs[] = {
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0 }, /* Invalid, assume RGB_565 */
+ { 4, 0, 8, 8, 8, 16, 8, 24, 8 }, /* HAL_PIXEL_FORMAT_RGBA_8888 */
+ { 4, 0, 8, 8, 8, 16, 8, 0, 0 }, /* HAL_PIXEL_FORMAT_RGBX_8888 */
+ { 3, 0, 8, 8, 8, 16, 8, 0, 0 }, /* HAL_PIXEL_FORMAT_RGB_888 */
+ { 2, 11, 5, 5, 6, 0, 5, 0, 0 }, /* HAL_PIXEL_FORMAT_RGB_565 */
+ { 4, 16, 8, 8, 8, 0, 8, 24, 8 }, /* HAL_PIXEL_FORMAT_BGRA_8888 */
+};
+
+static const struct framebuffer_config *get_fb_config_from_format(int format)
+{
+ if (format > 0 && format < ARRAY_SIZE(fb_configs))
+ return &fb_configs[format];
+
+ return &fb_configs[HAL_PIXEL_FORMAT_RGB_565]; /* legacy default */
+}
+
struct goldfish_fb {
void __iomem *reg_base;
int irq;
@@ -126,7 +167,8 @@ static int goldfish_fb_set_par(struct fb_info *info)
{
struct goldfish_fb *fb = container_of(info, struct goldfish_fb, fb);
if (fb->rotation != fb->fb.var.rotate) {
- info->fix.line_length = info->var.xres * 2;
+ info->fix.line_length = info->var.xres *
+ (fb->fb.var.bits_per_pixel / 8);
fb->rotation = fb->fb.var.rotate;
writel(fb->rotation, fb->reg_base + FB_SET_ROTATION);
}
@@ -143,8 +185,10 @@ static int goldfish_fb_pan_display(struct fb_var_screeninfo *var,
spin_lock_irqsave(&fb->lock, irq_flags);
base_update_count = fb->base_update_count;
- writel(fb->fb.fix.smem_start + fb->fb.var.xres * 2 * var->yoffset,
- fb->reg_base + FB_SET_BASE);
+ writel(fb->fb.fix.smem_start +
+ fb->fb.var.xres * (
+ fb->fb.var.bits_per_pixel / 8) * var->yoffset,
+ fb->reg_base + FB_SET_BASE);
spin_unlock_irqrestore(&fb->lock, irq_flags);
wait_event_timeout(fb->wait,
fb->base_update_count != base_update_count, HZ / 15);
@@ -186,8 +230,10 @@ static int goldfish_fb_probe(struct platform_device *pdev)
struct resource *r;
struct goldfish_fb *fb;
size_t framesize;
- u32 width, height;
+ u32 width, height, format;
+ int bytes_per_pixel;
dma_addr_t fbpaddr;
+ const struct framebuffer_config *fb_config;
fb = kzalloc(sizeof(*fb), GFP_KERNEL);
if (fb = NULL) {
@@ -217,13 +263,20 @@ static int goldfish_fb_probe(struct platform_device *pdev)
width = readl(fb->reg_base + FB_GET_WIDTH);
height = readl(fb->reg_base + FB_GET_HEIGHT);
+ format = readl(fb->reg_base + FB_GET_FORMAT);
+ fb_config = get_fb_config_from_format(format);
+ if (!fb_config) {
+ ret = -EINVAL;
+ goto err_no_irq;
+ }
+ bytes_per_pixel = fb_config->bytes_per_pixel;
fb->fb.fbops = &goldfish_fb_ops;
fb->fb.flags = FBINFO_FLAG_DEFAULT;
fb->fb.pseudo_palette = fb->cmap;
fb->fb.fix.type = FB_TYPE_PACKED_PIXELS;
fb->fb.fix.visual = FB_VISUAL_TRUECOLOR;
- fb->fb.fix.line_length = width * 2;
+ fb->fb.fix.line_length = width * bytes_per_pixel;
fb->fb.fix.accel = FB_ACCEL_NONE;
fb->fb.fix.ypanstep = 1;
@@ -231,20 +284,22 @@ static int goldfish_fb_probe(struct platform_device *pdev)
fb->fb.var.yres = height;
fb->fb.var.xres_virtual = width;
fb->fb.var.yres_virtual = height * 2;
- fb->fb.var.bits_per_pixel = 16;
+ fb->fb.var.bits_per_pixel = bytes_per_pixel * 8;
fb->fb.var.activate = FB_ACTIVATE_NOW;
fb->fb.var.height = readl(fb->reg_base + FB_GET_PHYS_HEIGHT);
fb->fb.var.width = readl(fb->reg_base + FB_GET_PHYS_WIDTH);
fb->fb.var.pixclock = 0;
- fb->fb.var.red.offset = 11;
- fb->fb.var.red.length = 5;
- fb->fb.var.green.offset = 5;
- fb->fb.var.green.length = 6;
- fb->fb.var.blue.offset = 0;
- fb->fb.var.blue.length = 5;
+ fb->fb.var.red.offset = fb_config->red_offset;
+ fb->fb.var.red.length = fb_config->red_length;
+ fb->fb.var.green.offset = fb_config->green_offset;
+ fb->fb.var.green.length = fb_config->green_length;
+ fb->fb.var.blue.offset = fb_config->blue_offset;
+ fb->fb.var.blue.length = fb_config->blue_length;
+ fb->fb.var.transp.offset = fb_config->transp_offset;
+ fb->fb.var.transp.length = fb_config->transp_length;
- framesize = width * height * 2 * 2;
+ framesize = width * height * 2 * bytes_per_pixel;
fb->fb.screen_base = (char __force __iomem *)dma_alloc_coherent(
&pdev->dev, framesize,
&fbpaddr, GFP_KERNEL);
@@ -295,7 +350,8 @@ static int goldfish_fb_remove(struct platform_device *pdev)
size_t framesize;
struct goldfish_fb *fb = platform_get_drvdata(pdev);
- framesize = fb->fb.var.xres_virtual * fb->fb.var.yres_virtual * 2;
+ framesize = fb->fb.var.xres_virtual * fb->fb.var.yres_virtual *
+ (fb->fb.var.bits_per_pixel / 8);
unregister_framebuffer(&fb->fb);
free_irq(fb->irq, fb);
^ permalink raw reply related
* Re: [PATCH v5 00/46] pwm: add support for atomic update
From: Boris Brezillon @ 2016-04-11 22:42 UTC (permalink / raw)
To: Thierry Reding, linux-pwm-u79uwXL29TY76Z2rM5mHXA
Cc: 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, linux-fbdev
In-Reply-To: <1459368249-13241-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Hi Thierry,
On Wed, 30 Mar 2016 22:03:23 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> Hello,
>
> This series adds support for atomic PWM update, or IOW, the capability
> to update all the parameters of a PWM device (enabled/disabled, period,
> duty and polarity) in one go.
>
> It also adds support for initial PWM state retrieval (or hardware readout),
> which should allow smooth handover between the bootloader and Linux. For
> example, critical PWM users (like critical regulators controlled by a PWM)
> can query the current PWM state, and adapt the PWM config without having
> to disable/enable the PWM, or abruptly change the period/dutycyle/polarity
> config.
>
> Thierry, I hope this version meets your expectations, if that's not the
> case, could you let me know quickly so I can adjust the implementation
> accordingly (I'd really like to get most of those changes in 4.7).
Still haven't had a clear feedback from your side on this series (you
commented on a few details, but nothing on the general approach). Could
you please have at a quick look at it, and let me know if I should
adjust the implementation?
Note that I plan to send a new version addressing comments made by
other maintainers/developers by the end of the week. In the meantime,
could you have a look at the first set of patches (patch 1 to 4 are
completely independent), and apply them if you're happy with it.
As you can see, I now have a lot of patches. This helps in showing the
big picture, but also annoys people when I send this 50+ patchset. So,
if you don't mind, I'd like to drop the changes touching PWM user
drivers (to make them use the atomic API) until we get the other parts
applied.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage
From: Mark Brown @ 2016-04-12 4:42 UTC (permalink / raw)
To: Boris Brezillon
Cc: Thierry Reding, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, 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
In-Reply-To: <20160407235431.788dc24a@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 322 bytes --]
On Thu, Apr 07, 2016 at 11:54:31PM +0200, Boris Brezillon wrote:
> Is there any reason for calling set_machine_constraints() after
> device_register() in regulator_register()?
I'm not sure there's a strong one, we don't really use the class device
for anything, but without doing a full audit I couldn't guarantee that.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage
From: Boris Brezillon @ 2016-04-12 8:37 UTC (permalink / raw)
To: Mark Brown
Cc: Thierry Reding, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, 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
In-Reply-To: <20160412044203.GW3351-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
Hi Mark,
On Tue, 12 Apr 2016 05:42:03 +0100
Mark Brown <broonie@kernel.org> wrote:
> On Thu, Apr 07, 2016 at 11:54:31PM +0200, Boris Brezillon wrote:
>
> > Is there any reason for calling set_machine_constraints() after
> > device_register() in regulator_register()?
>
> I'm not sure there's a strong one, we don't really use the class device
> for anything, but without doing a full audit I couldn't guarantee that.
At first glance I don't see any problem (even the rdev_err/info/...()
functions do not use dev_err/info/...()). The patch will be part of v6
(unless you want me to send it independently).
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage
From: Mark Brown @ 2016-04-12 10:09 UTC (permalink / raw)
To: Boris Brezillon
Cc: Thierry Reding, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, 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
In-Reply-To: <20160412103722.0bfe1d4d@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 755 bytes --]
On Tue, Apr 12, 2016 at 10:37:22AM +0200, Boris Brezillon wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Apr 07, 2016 at 11:54:31PM +0200, Boris Brezillon wrote:
> > I'm not sure there's a strong one, we don't really use the class device
> > for anything, but without doing a full audit I couldn't guarantee that.
> At first glance I don't see any problem (even the rdev_err/info/...()
> functions do not use dev_err/info/...()). The patch will be part of v6
> (unless you want me to send it independently).
I'd rather it didn't get sucked into this series since it seems that
it's getting delayed indefinitely - I can apply it to the regulator tree
and create a tag that can be pulled into other trees as needed if things
do get applied.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage
From: Boris Brezillon @ 2016-04-12 10:31 UTC (permalink / raw)
To: Mark Brown
Cc: Thierry Reding, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Mike Turquette,
Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA, 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
In-Reply-To: <20160412100938.GA14664-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
On Tue, 12 Apr 2016 11:09:38 +0100
Mark Brown <broonie@kernel.org> wrote:
> On Tue, Apr 12, 2016 at 10:37:22AM +0200, Boris Brezillon wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Apr 07, 2016 at 11:54:31PM +0200, Boris Brezillon wrote:
>
> > > I'm not sure there's a strong one, we don't really use the class device
> > > for anything, but without doing a full audit I couldn't guarantee that.
>
> > At first glance I don't see any problem (even the rdev_err/info/...()
> > functions do not use dev_err/info/...()). The patch will be part of v6
> > (unless you want me to send it independently).
>
> I'd rather it didn't get sucked into this series since it seems that
> it's getting delayed indefinitely - I can apply it to the regulator tree
> and create a tag that can be pulled into other trees as needed if things
> do get applied.
Done.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v5 01/46] pwm: rcar: make use of pwm_is_enabled()
From: Thierry Reding @ 2016-04-12 11:01 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-2-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 667 bytes --]
On Wed, Mar 30, 2016 at 10:03:24PM +0200, Boris Brezillon wrote:
> Commit 5c31252c4a86 ("pwm: Add the pwm_is_enabled() helper") introduced a
> new function to test whether a PWM device is enabled or not without
> manipulating PWM internal fields.
> Hiding this is necessary if we want to smoothly move to the atomic PWM
> config approach without impacting PWM drivers.
> Fix this driver to use pwm_is_enabled() instead of directly accessing the
> ->flags field.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/pwm/pwm-rcar.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 02/46] backlight: pwm_bl: remove useless call to pwm_set_period()
From: Thierry Reding @ 2016-04-12 11:03 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-3-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 475 bytes --]
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.
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 11:08 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-4-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 419 bytes --]
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.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 04/46] pwm: get rid of pwm->lock
From: Thierry Reding @ 2016-04-12 11:22 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-5-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]
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.
I've dropped the changes when applying.
Thanks,
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v5 04/46] pwm: get rid of pwm->lock
From: Boris Brezillon @ 2016-04-12 11:32 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: <20160412112246.GJ18882-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org>
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().
--
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 11:39 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-6-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]
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.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* 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
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