* Re: [PATCHv4 0/3] backlight: led-backlight driver
From: Tomi Valkeinen @ 2015-10-08 9:35 UTC (permalink / raw)
To: Jacek Anaszewski, Jingoo Han, Lee Jones, linux-leds, linux-fbdev
Cc: Andrew Lunn
In-Reply-To: <1443605522-1118-1-git-send-email-tomi.valkeinen@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]
Hi,
On 30/09/15 12:31, Tomi Valkeinen wrote:
> This series aims to add a led-backlight driver, similar to pwm-backlight, but
> using a LED class device underneath.
>
> LED framework has no support for DT or getting a LED class driver from another
> kernel driver, so I added minimal functionality to led-class to get
> led-backlight working.
>
> Changes to v3:
> - Change a comment to refer to of_led_get()
>
> Changes to v2:
> - power supply is now optional
> - cosmetic changes
> - no-op function for led_put() when !CONFIG_LEDS_CLASS
>
> Changes to v1:
> - Split LED OF parts into separate .h and .c files
> - Check for CONFIG_OF and CONFIG_LEDS_CLASS where relevant to leave unused code
> out.
> - Improved error prints and comments a bit
> - Added put_device() into led_put(), as the device was gotten from
> class_find_device() which requires a put_device() call.
>
> Tomi
>
> Tomi Valkeinen (3):
> leds: Add of_led_get() and led_put()
> backlight: add led-backlight driver
> devicetree: Add led-backlight binding
>
> .../bindings/video/backlight/led-backlight.txt | 30 +++
> drivers/leds/Makefile | 6 +-
> drivers/leds/led-class.c | 13 +-
> drivers/leds/led-of.c | 85 +++++++
> drivers/leds/leds.h | 1 +
> drivers/video/backlight/Kconfig | 7 +
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/led_bl.c | 246 +++++++++++++++++++++
> include/linux/leds.h | 10 +
> include/linux/of_leds.h | 26 +++
> 10 files changed, 423 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> create mode 100644 drivers/leds/led-of.c
> create mode 100644 drivers/video/backlight/led_bl.c
> create mode 100644 include/linux/of_leds.h
There's been no more comments on this. Should this be merged via led or
backlight trees?
Possible conflicts probably happen on the led side, as this changes the
led core files, so perhaps that's easier way?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCHv4 0/3] backlight: led-backlight driver
From: Jacek Anaszewski @ 2015-10-08 10:09 UTC (permalink / raw)
To: Tomi Valkeinen, Jingoo Han, Lee Jones
Cc: linux-leds, linux-fbdev, Andrew Lunn
In-Reply-To: <561638F2.2010003@ti.com>
On 10/08/2015 11:35 AM, Tomi Valkeinen wrote:
> Hi,
>
> On 30/09/15 12:31, Tomi Valkeinen wrote:
>> This series aims to add a led-backlight driver, similar to pwm-backlight, but
>> using a LED class device underneath.
>>
>> LED framework has no support for DT or getting a LED class driver from another
>> kernel driver, so I added minimal functionality to led-class to get
>> led-backlight working.
>>
>> Changes to v3:
>> - Change a comment to refer to of_led_get()
>>
>> Changes to v2:
>> - power supply is now optional
>> - cosmetic changes
>> - no-op function for led_put() when !CONFIG_LEDS_CLASS
>>
>> Changes to v1:
>> - Split LED OF parts into separate .h and .c files
>> - Check for CONFIG_OF and CONFIG_LEDS_CLASS where relevant to leave unused code
>> out.
>> - Improved error prints and comments a bit
>> - Added put_device() into led_put(), as the device was gotten from
>> class_find_device() which requires a put_device() call.
>>
>> Tomi
>>
>> Tomi Valkeinen (3):
>> leds: Add of_led_get() and led_put()
>> backlight: add led-backlight driver
>> devicetree: Add led-backlight binding
>>
>> .../bindings/video/backlight/led-backlight.txt | 30 +++
>> drivers/leds/Makefile | 6 +-
>> drivers/leds/led-class.c | 13 +-
>> drivers/leds/led-of.c | 85 +++++++
>> drivers/leds/leds.h | 1 +
>> drivers/video/backlight/Kconfig | 7 +
>> drivers/video/backlight/Makefile | 1 +
>> drivers/video/backlight/led_bl.c | 246 +++++++++++++++++++++
>> include/linux/leds.h | 10 +
>> include/linux/of_leds.h | 26 +++
>> 10 files changed, 423 insertions(+), 2 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>> create mode 100644 drivers/leds/led-of.c
>> create mode 100644 drivers/video/backlight/led_bl.c
>> create mode 100644 include/linux/of_leds.h
>
> There's been no more comments on this. Should this be merged via led or
> backlight trees?
>
> Possible conflicts probably happen on the led side, as this changes the
> led core files, so perhaps that's easier way?
Yes, it would be better to merge it via LED tree, also because the
driver being added depends on drivers/leds/led-of.c which is introduced
in this set. Obviously I need backlight maintainer's ack. Jingoo, Lee,
are you ok with this changes?
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH] uvesafb: make scaling configurable on Nvidia cards
From: Mikulas Patocka @ 2015-10-08 11:51 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Michal Januszewski, linux-fbdev, Jean-Christophe Plagniol-Villard,
linux-kernel
In-Reply-To: <56163806.40608@ti.com>
On Thu, 8 Oct 2015, Tomi Valkeinen wrote:
>
> On 03/09/15 00:23, Mikulas Patocka wrote:
> > [ I sent this some times ago, but didn't get any response ]
> >
> >
> > Nvidia cards have a BIOS function 0x4f14 that allows to set flat panel
> > scaling. This patch adds a module parameter "scaling" that uses this
> > function to set the scaling. By default, the parameter is -1, so that the
> > driver doesn't attempt to call the scaling function.
> >
> > This patch is useful when using the binary Nvidia graphics driver - in
> > that case, the console may be only in text mode or VESA mode. By default,
> > the video card does scaling that degrades font quality and changes aspect
> > ratio. This patch makes it possible to turn off the scaling and improve
> > font quality on the console.
> >
> > The allowed values depend on VESA BIOS. On my card, the following values
> > are allowed:
> > -1 - do not change the scaling
> > 0 - scale to full screen
> > 1, 2 - don't scale
> > 3 - scale and preserve aspect ratio
> > 4 - scale with black border around
> >
> > Example use:
> > echo 1 >/sys/module/uvesafb/parameters/scaling; fbset 1280x1024-60 -depth 32 -a
> > - this sets unscaled 1280x1024 video mode that has much sharper font than
> > the scaled modes.
>
> I have to say I don't know much about x86 video, but isn't uvesafb
> supposed to be a generic driver? Adding nvidia specific features there
> doesn't sound like a right way to handle this.
>
> Tomi
Where do you think that this functionality should be added? To Nouveau? -
Nouveau can't be used with nVidia binary driver, and this functionality is
useful especially with nVidia binary driver (it allows to get nice console
with sharp fonts).
BTW. I don't know if this BIOS function is supported on AMD or not. Could
someone try it? I don't have an AMD card.
Mikulas
^ permalink raw reply
* Re: [PATCH] uvesafb: make scaling configurable on Nvidia cards
From: Tomi Valkeinen @ 2015-10-08 12:21 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Michal Januszewski, linux-fbdev, Jean-Christophe Plagniol-Villard,
linux-kernel
In-Reply-To: <alpine.LRH.2.02.1510080744170.20879@file01.intranet.prod.int.rdu2.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]
On 08/10/15 14:51, Mikulas Patocka wrote:
>> I have to say I don't know much about x86 video, but isn't uvesafb
>> supposed to be a generic driver? Adding nvidia specific features there
>> doesn't sound like a right way to handle this.
>>
>> Tomi
>
> Where do you think that this functionality should be added? To Nouveau? -
> Nouveau can't be used with nVidia binary driver, and this functionality is
> useful especially with nVidia binary driver (it allows to get nice console
> with sharp fonts).
I don't know where is should be. But with a quick look I don't see any
gfx card specific features in uvesafb, and the doc clearly says it's a
generic driver. So if we add this nvidia feature there, shall we add
other features for other cards also? It's a kind of slippery road.
So how does uvesafb work with nvidia driver? uvesafb is used for initial
console, and nvidia driver takes over later when (and if) it's loaded?
And it's not possible to change the scaling via nvidia's driver?
> BTW. I don't know if this BIOS function is supported on AMD or not. Could
> someone try it? I don't have an AMD card.
Quick googling shows that 0x4f14 might be used by other gfx cards (Epia)
for something else. But I can't really say for sure. So in the minimum I
think the parameter should be renamed to "nvidia_scaling" or similar.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2] video: fbdev: add Marvell PXA framebuffer binding
From: Rob Herring @ 2015-10-08 13:30 UTC (permalink / raw)
To: Robert Jarzmik
Cc: Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Philipp Zabel,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <87y4ffaj3m.fsf-4ty26DBLk+jEm7gnYqmdkQ@public.gmane.org>
On Tue, Oct 6, 2015 at 2:32 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Rob Herring <robh@kernel.org> writes:
>
>> On Sun, Oct 4, 2015 at 5:31 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>>> Add documentation for the PXA frambuffer devicetree binding.
>>
>> Strictly speaking this is a binding for PXA display controller, not a
>> Linux FB driver. There are lots of "framebuffer" and "DRM" bindings
>> which I'm trying to curb.
> Yes, that's very true. That deserves a new commit message and a new file name
> (marvell,pxa2xx-lcd).
>>> .../devicetree/bindings/video/marvell,pxafb.txt | 80 ++++++++++++++++++++++
>>
>> Please put in bindings/display/ as I'm consolidating all the display
>> related bindings there[1].
> Of course, for v3.
>
>>> +++ b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
>>> @@ -0,0 +1,80 @@
>>> +PXA LCDC Framebuffer
>>> +--------------------
>>> +
>>> +Required properties:
>>> + - compatible :
>>> + "marvell,pxa2xx-lcdc",
>>
>> No differences in h/w for any of the chips?
> All pxa25x, pxa27x and pxa3xx are compatible.
> AFAIK, pxa3xx has an IP with additional registers. But :
> - these (this) register(s) is not necessary for the display controller to work
> (it's more a control to shift red/green/blue values, and energy management)
> - all the registers in pxa2{5,7}x are the same in pxa3xx
>
> The pxafb driver acts today on the subset of registers which are the same across
> all pxaXXX variants. This is what made me think only one compatible property was
> required.
>
> If I'm wrong, I could add "marvell,pxa3xx-lcdc", is that what you think I should
> do ?
I would prefer specific compatibles (e.g. marvell,pxa270-lcdc) in
addition to a generic one. You never know when a specific chip will
have some quirk even though "the IP is the same".
>>
>>> + - reg : Should contain 1 register ranges(address and length).
>>> + Can contain an additional register range(address and length)
>>> + for fixed framebuffer memory. Useful for dedicated memories.
>>
>> This is memory that can't be used for anything else? We already have
>> reserved-memory for this if it is just RAM. There's also a binding for
>> on-chip SRAM which should probably be used if the memory is usable for
>> other things.
> That is a wrong copy paste I made. I was more thinking of having only 1 register
> range, and no video memory reservation ...
>
>>> +PXA LCDC Display
>>
>> This should not be specific to PXA, but for this panel. This should be
>> in bindings/display/panel/.
> Sure, for v3.
>
>>> +----------------
>>> +Required properties (as per of_videomode_helper):
>>> + - lcd-type: either "mono-stn", "mono-dstn", "color-stn", "color-dstn",
>>> + "color-tft", "smart-panel"
>>> +
>>> +Optional properties (as per of_videomode_helper):
>>> + - power-supply: power supply regulator to the LCD to power it on or off
>>> + (see regulator.txt)
>>> + - backlight: backlight control (see backlight.txt)
>>> +
>>> +Required nodes:
>>> + - port: connection to the LCD controller
>>> + - display-timings: panel timings (see display-timing.txt)
>>
>> If lcd-type is smart-panel, then this node would not make sense.
> Ah I see, so this could be optional maybe ?
Right.
>
> Actually when I'll move the panel definition to bindings/display/panel, is this
> what I should do :
> - create a file marvell,pxa2xx-panel
> - input all these properties into this file
Yes, but the file should be called toshiba,ltm0305a776.txt. It is
irrelevant to the binding that it is pared with pxa2xx. In theory, I
could attach this panel to any SOC.
>
> And then, when a board maintainer will create a devicetree description, he will
> write something like :
> compatible = "toshiba,ltm0305a776";
> compatible = "marvell,pxa2xx-panel";
Drop this compatible.
> lcd-type = "color-tft";
> ...
>
> If that's the case, I wonder how to "enforce" that a panel used with
> marvell,pxa2xx-lcdc (through the of_graph 'port' node) be compatible with
> marvell,pxa2xx-panel ?
I'm not sure what you mean. Putting the panel into the dts ensures
that. The FB driver may check for toshiba,ltm0305a776 or a list of
panels. However, a DRM driver would probably not check that.
Rob
^ permalink raw reply
* Re: [PATCH] uvesafb: make scaling configurable on Nvidia cards
From: Mikulas Patocka @ 2015-10-08 13:55 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Michal Januszewski, linux-fbdev, Jean-Christophe Plagniol-Villard,
linux-kernel
In-Reply-To: <56165FCB.30504@ti.com>
On Thu, 8 Oct 2015, Tomi Valkeinen wrote:
>
> On 08/10/15 14:51, Mikulas Patocka wrote:
>
> >> I have to say I don't know much about x86 video, but isn't uvesafb
> >> supposed to be a generic driver? Adding nvidia specific features there
> >> doesn't sound like a right way to handle this.
> >>
> >> Tomi
> >
> > Where do you think that this functionality should be added? To Nouveau? -
> > Nouveau can't be used with nVidia binary driver, and this functionality is
> > useful especially with nVidia binary driver (it allows to get nice console
> > with sharp fonts).
>
> I don't know where is should be. But with a quick look I don't see any
> gfx card specific features in uvesafb, and the doc clearly says it's a
> generic driver. So if we add this nvidia feature there, shall we add
> other features for other cards also? It's a kind of slippery road.
>
> So how does uvesafb work with nvidia driver? uvesafb is used for initial
> console, and nvidia driver takes over later when (and if) it's loaded?
> And it's not possible to change the scaling via nvidia's driver?
The nvidia binary driver doesn't handle console, it only handles Xwindow.
You can use either the VGA 80x25 text console or you can set graphics
console with uvesafb - in both cases, the video card hardware scales the
screen and the console font is blurry. The patch allows to turn off the
scaling.
> > BTW. I don't know if this BIOS function is supported on AMD or not. Could
> > someone try it? I don't have an AMD card.
>
> Quick googling shows that 0x4f14 might be used by other gfx cards (Epia)
> for something else. But I can't really say for sure. So in the minimum I
> think the parameter should be renamed to "nvidia_scaling" or similar.
Someone should try the patch on AMD card and say if it can set scaling
there.
Mikulas
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH v2] video: fbdev: add Marvell PXA framebuffer binding
From: Robert Jarzmik @ 2015-10-08 19:15 UTC (permalink / raw)
To: Rob Herring, Philipp Zabel
Cc: Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev@vger.kernel.org
In-Reply-To: <CAL_JsqLatWZa1mYL1v83ryJntyLtHDfSfr0gXkvPXJrpCE=qcg@mail.gmail.com>
Rob Herring <robh@kernel.org> writes:
>> The pxafb driver acts today on the subset of registers which are the same across
>> all pxaXXX variants. This is what made me think only one compatible property was
>> required.
>>
>> If I'm wrong, I could add "marvell,pxa3xx-lcdc", is that what you think I should
>> do ?
>
> I would prefer specific compatibles (e.g. marvell,pxa270-lcdc) in
> addition to a generic one. You never know when a specific chip will
> have some quirk even though "the IP is the same".
Okay.
>> And then, when a board maintainer will create a devicetree description, he will
>> write something like :
>> compatible = "toshiba,ltm0305a776";
>> compatible = "marvell,pxa2xx-panel";
>
> Drop this compatible.
>
>> lcd-type = "color-tft";
>> ...
>>
>> If that's the case, I wonder how to "enforce" that a panel used with
>> marvell,pxa2xx-lcdc (through the of_graph 'port' node) be compatible with
>> marvell,pxa2xx-panel ?
>
> I'm not sure what you mean. Putting the panel into the dts ensures
> that. The FB driver may check for toshiba,ltm0305a776 or a list of
> panels. However, a DRM driver would probably not check that.
>
> Rob
What I mean is that the LDLC controller _must_ be programmmed with the correct
panel type, ie. one register of the LDLC should be set according to this type.
The type is a hardware property of the panel, and yet it is absolutely mandatory
to have it set in the panel.
What I mean is : what is the good way to enforce that this property is set
somewhere in the devicetree description ? Philipp adviced for it to be transfered
to the ldlc description (ie. marvell,pxa2xx-ldlc), while I was thinking of
having it in a panel description.
Cheers.
--
Robert
^ permalink raw reply
* Re: [PATCH v2] video: fbdev: add Marvell PXA framebuffer binding
From: Rob Herring @ 2015-10-09 0:46 UTC (permalink / raw)
To: Robert Jarzmik
Cc: Philipp Zabel, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev@vger.kernel.org
In-Reply-To: <87lhbd9npn.fsf@belgarion.home>
On Thu, Oct 8, 2015 at 2:15 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Rob Herring <robh@kernel.org> writes:
>>> And then, when a board maintainer will create a devicetree description, he will
>>> write something like :
>>> compatible = "toshiba,ltm0305a776";
>>> compatible = "marvell,pxa2xx-panel";
>>
>> Drop this compatible.
>>
>>> lcd-type = "color-tft";
>>> ...
>>>
>>> If that's the case, I wonder how to "enforce" that a panel used with
>>> marvell,pxa2xx-lcdc (through the of_graph 'port' node) be compatible with
>>> marvell,pxa2xx-panel ?
>>
>> I'm not sure what you mean. Putting the panel into the dts ensures
>> that. The FB driver may check for toshiba,ltm0305a776 or a list of
>> panels. However, a DRM driver would probably not check that.
>>
>> Rob
> What I mean is that the LDLC controller _must_ be programmmed with the correct
> panel type, ie. one register of the LDLC should be set according to this type.
>
> The type is a hardware property of the panel, and yet it is absolutely mandatory
> to have it set in the panel.
>
> What I mean is : what is the good way to enforce that this property is set
> somewhere in the devicetree description ? Philipp adviced for it to be transfered
> to the ldlc description (ie. marvell,pxa2xx-ldlc), while I was thinking of
> having it in a panel description.
Either of those options are fine. Neither should need
marvell,pxa2xx-panel though. I'd lean towards putting it in the panel,
but in that case it should be generic for panels which I think it is.
It should probably be optional with not present meaning color-tft
(since every other type is practically dead), but you could say
required on pxa2xx.
The only way to enforce it ATM, is panicking or something if the LCDC
finds it is not set. That's not any worse that checking for
marvell,pxa2xx-panel.
Rob
^ permalink raw reply
* Re: [BUG] RCU stall in cursor_timer_handler
From: Alistair Popple @ 2015-10-09 5:31 UTC (permalink / raw)
To: Scot Doyle
Cc: linux-fbdev, Benjamin Herrenschmidt, plagnioj, linux-kernel,
airlied, Pavel Machek, Greg Kroah-Hartman
In-Reply-To: <alpine.DEB.2.11.1510030350110.20774@local>
Hi Scot,
On Sat, 3 Oct 2015 05:12:15 Scot Doyle wrote:
> On Sat, 3 Oct 2015, Alistair Popple wrote:
> > Hi,
> >
> > We have been intermittently seeing the below RCU stall at boot on a
> > PPC64LE 4.2.1 kernel which has been preventing the system from booting.
> > Further investigation indicates that ops->cur_blink_jiffies is
> > potentially being used uninitialised in cursor_timer_handler():
> >
> > static void cursor_timer_handler(unsigned long dev_addr)
> > {
> > struct fb_info *info = (struct fb_info *) dev_addr;
> > struct fbcon_ops *ops = info->fbcon_par;
> >
> > queue_work(system_power_efficient_wq, &info->queue);
> > mod_timer(&ops->cursor_timer, jiffies + ops->cur_blink_jiffies);
> > }
> ...
>
>
> Hi Alistair, thanks so much for the detailed report. Does this patch
> correct the stalls?
Sorry for the delay getting back to you. I have tested this patch and have not
observed the stall again after 71 boots of the system (usually the issue would
occur every couple of boots), so it seems to have solved the problem as far as
I can tell.
Thanks!
Regards,
Alistair
> diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
> index 1aaf893..92f3949 100644
> --- a/drivers/video/console/fbcon.c
> +++ b/drivers/video/console/fbcon.c
> @@ -1093,6 +1093,7 @@ static void fbcon_init(struct vc_data *vc, int init)
> con_copy_unimap(vc, svc);
>
> ops = info->fbcon_par;
> + ops->cur_blink_jiffies = msecs_to_jiffies(vc->vc_cur_blink_ms);
> p->con_rotate = initial_rotation;
> set_blitting_type(vc, info);
>
^ permalink raw reply
* [PATCH] fbcon: initialize blink interval before calling fb_set_par
From: Scot Doyle @ 2015-10-09 15:08 UTC (permalink / raw)
To: Tomi Valkeinen, Jean-Christophe Plagniol-Villard
Cc: Greg Kroah-Hartman, Alistair Popple, Pavel Machek,
Benjamin Herrenschmidt, airlied, linux-fbdev, linux-kernel
In-Reply-To: <3998353.WdEhcyQWSK@new-mexico>
Since commit 27a4c827c34ac4256a190cc9d24607f953c1c459
fbcon: use the cursor blink interval provided by vt
a PPC64LE kernel fails to boot when fbcon_add_cursor_timer uses an
uninitialized ops->cur_blink_jiffies. Prevent by initializing
in fbcon_init before the call to info->fbops->fb_set_par.
Reported-and-tested-by: Alistair Popple <alistair@popple.id.au>
Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
---
drivers/video/console/fbcon.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index 1aaf893..92f3949 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -1093,6 +1093,7 @@ static void fbcon_init(struct vc_data *vc, int init)
con_copy_unimap(vc, svc);
ops = info->fbcon_par;
+ ops->cur_blink_jiffies = msecs_to_jiffies(vc->vc_cur_blink_ms);
p->con_rotate = initial_rotation;
set_blitting_type(vc, info);
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] fbcon: initialize blink interval before calling fb_set_par
From: Benjamin Herrenschmidt @ 2015-10-09 20:48 UTC (permalink / raw)
To: Scot Doyle, Tomi Valkeinen, Jean-Christophe Plagniol-Villard
Cc: Greg Kroah-Hartman, Alistair Popple, Pavel Machek, airlied,
linux-fbdev, linux-kernel
In-Reply-To: <alpine.DEB.2.11.1510091500100.1816@local>
On Fri, 2015-10-09 at 15:08 +0000, Scot Doyle wrote:
> Since commit 27a4c827c34ac4256a190cc9d24607f953c1c459
> fbcon: use the cursor blink interval provided by vt
>
> a PPC64LE kernel fails to boot when fbcon_add_cursor_timer uses an
> uninitialized ops->cur_blink_jiffies. Prevent by initializing
> in fbcon_init before the call to info->fbops->fb_set_par.
> Reported-and-tested-by: Alistair Popple <alistair@popple.id.au>
> Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
Please add
CC: <stable@vger.kernel.org> [v4.2]
This bug will cause any machine using fbcon to occasionally fail
to boot due to having a timer callback try to mod_timer with
"jiffies + 0" without an exit condition.
Cheers,
Ben.
> ---
> drivers/video/console/fbcon.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/console/fbcon.c
> b/drivers/video/console/fbcon.c
> index 1aaf893..92f3949 100644
> --- a/drivers/video/console/fbcon.c
> +++ b/drivers/video/console/fbcon.c
> @@ -1093,6 +1093,7 @@ static void fbcon_init(struct vc_data *vc, int
> init)
> con_copy_unimap(vc, svc);
>
> ops = info->fbcon_par;
> + ops->cur_blink_jiffies = msecs_to_jiffies(vc
> ->vc_cur_blink_ms);
> p->con_rotate = initial_rotation;
> set_blitting_type(vc, info);
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux
> -fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 00/12] pwm: add support for atomic update
From: Boris Brezillon @ 2015-10-09 21:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1442828009-6241-1-git-send-email-boris.brezillon@free-electrons.com>
Hi Thierry,
On Mon, 21 Sep 2015 11:33:17 +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.
Anything preventing this series from being applied (AKA ping)?
>
> Best Regards,
>
> Boris
>
> Changes since v2:
> - rebased on top of 4.3-rc2
> - reintroduced pwm-regulator patches
>
> Changes since v1:
> - dropped applied patches
> - squashed Heiko's fixes into the rockchip driver changes
> - made a few cosmetic changes
> - added kerneldoc comments
> - added Heiko's patch to display more information in debugfs
> - dropped pwm-regulator patches (should be submitted separately)
>
> Boris Brezillon (11):
> pwm: introduce default period and polarity concepts
> pwm: define a new pwm_state struct
> pwm: move the enabled/disabled info to pwm_state struct
> backlight: pwm_bl: remove useless call to pwm_set_period
> pwm: declare a default PWM state
> pwm: add the PWM initial state retrieval infra
> pwm: add the core infrastructure to allow atomic update
> pwm: rockchip: add initial state retrieval
> pwm: rockchip: add support for atomic update
> regulator: pwm: implement ->enable(), ->disable() and ->is_enabled
> methods
> regulator: pwm: properly initialize the ->state field
>
> Heiko Stübner (1):
> pwm: add information about polarity, duty cycle and period to debugfs
>
> drivers/leds/leds-pwm.c | 2 +-
> drivers/pwm/core.c | 169 +++++++++++++++++++++++++++++++----
> drivers/pwm/pwm-pxa.c | 2 +-
> drivers/pwm/pwm-rockchip.c | 119 +++++++++++++++++++-----
> drivers/pwm/pwm-sun4i.c | 3 +-
> drivers/regulator/pwm-regulator.c | 65 ++++++++++++--
> drivers/video/backlight/lm3630a_bl.c | 4 +-
> drivers/video/backlight/pwm_bl.c | 10 ++-
> drivers/video/fbdev/ssd1307fb.c | 2 +-
> include/linux/pwm.h | 89 +++++++++++++++---
> 10 files changed, 392 insertions(+), 73 deletions(-)
>
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH 00/20] tree-wide convert to memremap()
From: Dan Williams @ 2015-10-09 22:15 UTC (permalink / raw)
To: linux-kernel
Cc: linux-fbdev, David Airlie, dri-devel, Jaroslav Kysela, Lv Zheng,
H. Peter Anvin, Daniel Vetter, linux-arch, Rafael J. Wysocki,
Bob Moore, Mike Waychison, Tomi Valkeinen, Geert Uytterhoeven,
Russell King, Ross Zwisler, Jean-Christophe Plagniol-Villard,
Len Brown, Jeeja KP, Joerg Roedel, Arnd Bergmann, intel-gfx,
Mark Brown, Borislav Petkov, Takashi Iwai, Thomas
The memremap() api [1] was merged in 4.3 [2] with an initial
implementation for x86 and a conversion of the pmem driver. Complete the
conversion for the rest of the kernel.
Feel free to either ack or directly apply a conversion-patch as I will
defer the final removal patches until all the conversions have landed.
[1]: https://lwn.net/Articles/653585/
[2]: commit 92281dee825f arch: introduce memremap()
---
Dan Williams (20):
x86: introduce arch_memremap()
arm: introduce arch_memremap()
ia64: introduce arch_memremap()
sh: introduce arch_memremap()
m68k: introduce arch_memremap()
arm: switch from ioremap_cache to memremap
x86: switch from ioremap_cache to memremap
gma500: switch from acpi_os_ioremap to memremap
i915: switch from acpi_os_ioremap to memremap
acpi: switch from ioremap_cache to memremap
sound, skylake: switch from ioremap_cache to memremap
memconsole: fix __iomem mishandling, switch to memremap
intel-iommu: switch from ioremap_cache to memremap
pxa2xx-flash: switch from ioremap_cache to memremap
sfi: switch from ioremap_cache to memremap
fbdev: switch from ioremap_wt to memremap
arch: kill ioremap_cached()
arch: kill ioremap_fullcache()
arch: remove ioremap_cache, replace with arch_memremap
arch: remove ioremap_wt, optionally replace with arch_memremap
Documentation/x86/pat.txt | 6 +--
arch/arc/include/asm/io.h | 1
arch/arm/Kconfig | 1
arch/arm/include/asm/io.h | 7 ---
arch/arm/include/asm/xen/page.h | 4 +-
arch/arm/mm/ioremap.c | 12 ++++-
arch/arm/mm/mmu.c | 2 -
arch/arm/mm/nommu.c | 11 +++--
arch/arm64/Kconfig | 1
arch/arm64/include/asm/acpi.h | 11 -----
arch/arm64/include/asm/dmi.h | 8 ++--
arch/arm64/include/asm/io.h | 2 -
arch/arm64/kernel/efi.c | 9 ++--
arch/arm64/kernel/smp_spin_table.c | 19 ++++-----
arch/arm64/mm/ioremap.c | 20 +++------
arch/avr32/include/asm/io.h | 1
arch/frv/include/asm/io.h | 12 -----
arch/ia64/Kconfig | 1
arch/ia64/include/asm/io.h | 6 ---
arch/ia64/mm/ioremap.c | 10 +++++
arch/m32r/include/asm/io.h | 1
arch/m68k/Kconfig | 1
arch/m68k/include/asm/io_mm.h | 13 ------
arch/m68k/include/asm/io_no.h | 11 -----
arch/m68k/include/asm/raw_io.h | 1
arch/m68k/mm/kmap.c | 17 +++++++-
arch/m68k/mm/sun3kmap.c | 7 +++
arch/metag/include/asm/io.h | 6 ---
arch/microblaze/include/asm/io.h | 2 -
arch/mn10300/include/asm/io.h | 1
arch/nios2/include/asm/io.h | 1
arch/s390/include/asm/io.h | 1
arch/sh/Kconfig | 1
arch/sh/include/asm/io.h | 7 ---
arch/sh/mm/ioremap.c | 9 ++++
arch/sparc/include/asm/io_32.h | 1
arch/sparc/include/asm/io_64.h | 1
arch/tile/include/asm/io.h | 2 -
arch/unicore32/include/asm/io.h | 4 --
arch/unicore32/mm/ioremap.c | 8 ----
arch/x86/Kconfig | 1
arch/x86/include/asm/efi.h | 3 +
arch/x86/include/asm/io.h | 4 --
arch/x86/kernel/crash_dump_64.c | 6 +--
arch/x86/kernel/kdebugfs.c | 8 ++--
arch/x86/kernel/ksysfs.c | 28 ++++++-------
arch/x86/mm/ioremap.c | 43 ++++++++-----------
arch/xtensa/include/asm/io.h | 12 -----
drivers/acpi/apei/einj.c | 9 ++--
drivers/acpi/apei/erst.c | 6 +--
drivers/acpi/nvs.c | 6 +--
drivers/acpi/osl.c | 70 +++++++++-----------------------
drivers/firmware/google/memconsole.c | 7 ++-
drivers/gpu/drm/gma500/opregion.c | 8 ++--
drivers/gpu/drm/gma500/psb_drv.h | 2 -
drivers/gpu/drm/gma500/psb_lid.c | 8 ++--
drivers/gpu/drm/i915/i915_debugfs.c | 2 -
drivers/gpu/drm/i915/i915_drv.h | 12 +++--
drivers/gpu/drm/i915/intel_bios.c | 7 +--
drivers/gpu/drm/i915/intel_opregion.c | 73 ++++++++++++++++-----------------
drivers/gpu/drm/i915/intel_panel.c | 2 -
drivers/iommu/intel-iommu.c | 20 +++++----
drivers/iommu/intel_irq_remapping.c | 8 ++--
drivers/mtd/maps/pxa2xx-flash.c | 6 +--
drivers/nvdimm/Kconfig | 2 -
drivers/sfi/sfi_core.c | 4 +-
drivers/video/fbdev/Kconfig | 2 -
drivers/video/fbdev/amifb.c | 5 +-
drivers/video/fbdev/atafb.c | 5 +-
drivers/video/fbdev/hpfb.c | 6 +--
include/acpi/acpi_io.h | 6 +--
include/asm-generic/io.h | 8 ----
include/asm-generic/iomap.h | 4 --
include/linux/io.h | 1
kernel/Makefile | 2 -
kernel/memremap.c | 27 +++++-------
lib/Kconfig | 6 ++-
sound/soc/intel/skylake/skl-nhlt.c | 10 ++---
sound/soc/intel/skylake/skl.h | 6 +--
79 files changed, 284 insertions(+), 400 deletions(-)
^ permalink raw reply
* [PATCH 16/20] fbdev: switch from ioremap_wt to memremap
From: Dan Williams @ 2015-10-09 22:17 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arch, linux-fbdev, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
In-Reply-To: <20151009221537.32203.5867.stgit@dwillia2-desk3.jf.intel.com>
In preparation for deprecating ioremap_wt() convert its usage in
drivers/video/fbdev to memremap.
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/video/fbdev/amifb.c | 5 +++--
drivers/video/fbdev/atafb.c | 5 +++--
drivers/video/fbdev/hpfb.c | 6 +++---
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/amifb.c b/drivers/video/fbdev/amifb.c
index 1d702e13aaff..0cde451046d0 100644
--- a/drivers/video/fbdev/amifb.c
+++ b/drivers/video/fbdev/amifb.c
@@ -52,6 +52,7 @@
#include <linux/ioport.h>
#include <linux/platform_device.h>
#include <linux/uaccess.h>
+#include <linux/io.h>
#include <asm/irq.h>
#include <asm/amigahw.h>
@@ -3705,8 +3706,8 @@ default_chipset:
* access the videomem with writethrough cache
*/
info->fix.smem_start = (u_long)ZTWO_PADDR(videomemory);
- videomemory = (u_long)ioremap_wt(info->fix.smem_start,
- info->fix.smem_len);
+ videomemory = (u_long)memremap(info->fix.smem_start, info->fix.smem_len,
+ MEMREMAP_WT);
if (!videomemory) {
dev_warn(&pdev->dev,
"Unable to map videomem cached writethrough\n");
diff --git a/drivers/video/fbdev/atafb.c b/drivers/video/fbdev/atafb.c
index d6ce613e12ad..5615e8c31ea2 100644
--- a/drivers/video/fbdev/atafb.c
+++ b/drivers/video/fbdev/atafb.c
@@ -55,12 +55,12 @@
#include <linux/delay.h>
#include <linux/init.h>
#include <linux/interrupt.h>
+#include <linux/io.h>
#include <asm/setup.h>
#include <linux/uaccess.h>
#include <asm/pgtable.h>
#include <asm/irq.h>
-#include <asm/io.h>
#include <asm/atarihw.h>
#include <asm/atariints.h>
@@ -3185,7 +3185,8 @@ int __init atafb_init(void)
/* Map the video memory (physical address given) to somewhere
* in the kernel address space.
*/
- external_screen_base = ioremap_wt(external_addr, external_len);
+ external_screen_base = memremap(external_addr, external_len,
+ MEMREMAP_WT);
if (external_vgaiobase)
external_vgaiobase (unsigned long)ioremap(external_vgaiobase, 0x10000);
diff --git a/drivers/video/fbdev/hpfb.c b/drivers/video/fbdev/hpfb.c
index 9476d196f510..d08c320b06fc 100644
--- a/drivers/video/fbdev/hpfb.c
+++ b/drivers/video/fbdev/hpfb.c
@@ -14,8 +14,8 @@
#include <linux/init.h>
#include <linux/fb.h>
#include <linux/dio.h>
+#include <linux/io.h>
-#include <asm/io.h>
#include <asm/uaccess.h>
static struct fb_info fb_info = {
@@ -241,8 +241,8 @@ static int hpfb_init_one(unsigned long phys_base, unsigned long virt_base)
fb_info.fix.line_length = fb_width;
fb_height = (in_8(fb_regs + HPFB_FBHMSB) << 8) | in_8(fb_regs + HPFB_FBHLSB);
fb_info.fix.smem_len = fb_width * fb_height;
- fb_start = (unsigned long)ioremap_wt(fb_info.fix.smem_start,
- fb_info.fix.smem_len);
+ fb_start = (unsigned long) memremap(fb_info.fix.smem_start,
+ fb_info.fix.smem_len, MEMREMAP_WT);
hpfb_defined.xres = (in_8(fb_regs + HPFB_DWMSB) << 8) | in_8(fb_regs + HPFB_DWLSB);
hpfb_defined.yres = (in_8(fb_regs + HPFB_DHMSB) << 8) | in_8(fb_regs + HPFB_DHLSB);
hpfb_defined.xres_virtual = hpfb_defined.xres;
^ permalink raw reply related
* [PATCH 0/5] add missing of_node_put
From: Julia Lawall @ 2015-10-10 12:30 UTC (permalink / raw)
To: linux-arm-kernel
for_each_child_of_node performs an of_node_get on each iteration, so
a break out of the loop requires an of_node_put.
The complete semantic patch that fixes this problem is
(http://coccinelle.lip6.fr):
// <smpl>
@@
expression root,e;
local idexpression child;
iterator name for_each_child_of_node;
@@
for_each_child_of_node(root, child) {
... when != of_node_put(child)
when != e = child
+ of_node_put(child);
? break;
...
}
... when != child
@@
expression root,e;
local idexpression child;
@@
for_each_child_of_node(root, child) {
... when != of_node_put(child)
when != e = child
(
return child;
|
+ of_node_put(child);
? return ...;
)
...
}
// </smpl>
---
arch/arm/kernel/devtree.c | 1 +
arch/arm/mach-shmobile/pm-rmobile.c | 4 +++-
drivers/power/charger-manager.c | 4 +++-
drivers/regulator/of_regulator.c | 1 +
drivers/video/backlight/88pm860x_bl.c | 1 +
5 files changed, 9 insertions(+), 2 deletions(-)
^ permalink raw reply
* [PATCH 1/5] backlight: 88pm860x_bl: add missing of_node_put
From: Julia Lawall @ 2015-10-10 12:30 UTC (permalink / raw)
To: Jingoo Han
Cc: kernel-janitors, Lee Jones, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel,
Russell King - ARM Linux, Thomas Petazzoni, Andrew Lunn,
Bjorn Helgaas, Jason Cooper
In-Reply-To: <1444480254-14399-1-git-send-email-Julia.Lawall@lip6.fr>
for_each_child_of_node performs an of_node_get on each iteration, so
a break out of the loop requires an of_node_put.
The semantic patch that fixes this problem is as follows
(http://coccinelle.lip6.fr):
// <smpl>
@@
expression root,e;
local idexpression child;
iterator name for_each_child_of_node;
@@
for_each_child_of_node(root, child) {
... when != of_node_put(child)
when != e = child
+ of_node_put(child);
? break;
...
}
... when != child
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
drivers/video/backlight/88pm860x_bl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/backlight/88pm860x_bl.c b/drivers/video/backlight/88pm860x_bl.c
index 2da5862..6d8dc2c 100644
--- a/drivers/video/backlight/88pm860x_bl.c
+++ b/drivers/video/backlight/88pm860x_bl.c
@@ -180,6 +180,7 @@ static int pm860x_backlight_dt_init(struct platform_device *pdev,
data->iset = PM8606_WLED_CURRENT(iset);
of_property_read_u32(np, "marvell,88pm860x-pwm",
&data->pwm);
+ of_node_put(np);
break;
}
}
^ permalink raw reply related
* [PATCH v3] video: fbdev: add Marvell PXA LCD controller binding
From: Robert Jarzmik @ 2015-10-12 11:30 UTC (permalink / raw)
To: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Philipp Zabel
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA, Robert Jarzmik
Add documentation for the PXA LCD controller devicetree binding.
Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
Since v1: Philipp's review on the whole binding
Since v2: moved to display/, Philipp's and Rob's review
dropped panel description, should be a simple panel with a
lcd-type optional node that could be added to simple-panel.
---
.../bindings/display/marvell,pxa2xx-lcdc.txt | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/marvell,pxa2xx-lcdc.txt
diff --git a/Documentation/devicetree/bindings/display/marvell,pxa2xx-lcdc.txt b/Documentation/devicetree/bindings/display/marvell,pxa2xx-lcdc.txt
new file mode 100644
index 000000000000..309c47f25b87
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/marvell,pxa2xx-lcdc.txt
@@ -0,0 +1,34 @@
+PXA LCD Controller
+------------------
+
+Required properties:
+ - compatible : one of these
+ "marvell,pxa2xx-lcdc",
+ "marvell,pxa270-lcdc",
+ "marvell,pxa300-lcdc"
+ - reg : should contain 1 register range (address and length).
+ - interrupts : framebuffer controller interrupt.
+ - clocks: phandle to input clocks
+
+Required nodes:
+ - port: connection to the LCD panel (see video-interfaces.txt)
+ This node must have its properties bus-width and remote-endpoint set.
+ If the panel is not a TFT color panel, then a "lcd-type" property in
+ the panel should specify the panel type.
+ This panel node should be in the board dts.
+
+Example:
+ lcd-controller@40500000 {
+ compatible = "marvell,pxa2xx-lcdc";
+ reg = <0x44000000 0x10000>;
+ interrupts = <17>;
+ clocks = <&clks CLK_LCD>;
+ status = "okay";
+
+ port {
+ lcdc_out: endpoint {
+ remote-endpoint = <&panel_in>;
+ bus-width = <16>;
+ };
+ };
+ };
--
2.1.4
^ permalink raw reply related
* [PATCH v2] backlight: pwm: reject legacy pwm request for device defined in dt
From: Vladimir Zapolskiy @ 2015-10-12 12:29 UTC (permalink / raw)
To: Thierry Reding, Lee Jones, Jingoo Han
Cc: Nicolas Ferre, linux-pwm, linux-fbdev
Platform PWM backlight data provided by board's device tree should be
complete enough to successfully request a pwm device using pwm_get()
API. This change fixes a bug, when an arbitrary (first found) PWM is
connected to a "pwm-backlight" compatible device, when explicit PWM
device reference is not given.
Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
already describes "pwms" as a required property, instead of blind
selection of a potentially wrong PWM reject legacy PWM device
registration request, leave legacy API only for non-dt cases.
Based on initial implementation done by Dmitry Eremin-Solenikov.
Reported-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
The change is based on lee-backlight/for-backlight-next
Changes from v1 to v2:
* rebased on top of Nicolas' commit
68feaca0b13 ("backlight: pwm: Handle EPROBE_DEFER while requesting the PWM")
Links to previous discussions of the change:
* https://patchwork.ozlabs.org/patch/483993/
* https://patchwork.ozlabs.org/patch/398849/
drivers/video/backlight/pwm_bl.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index eff379b..ae3c6b6 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -271,19 +271,18 @@ static int pwm_backlight_probe(struct platform_device *pdev)
}
pb->pwm = devm_pwm_get(&pdev->dev, NULL);
- if (IS_ERR(pb->pwm)) {
- ret = PTR_ERR(pb->pwm);
- if (ret = -EPROBE_DEFER)
- goto err_alloc;
-
+ if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
+ && !pdev->dev.of_node) {
dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
pb->legacy = true;
pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
- if (IS_ERR(pb->pwm)) {
- dev_err(&pdev->dev, "unable to request legacy PWM\n");
- ret = PTR_ERR(pb->pwm);
- goto err_alloc;
- }
+ }
+
+ if (IS_ERR(pb->pwm)) {
+ ret = PTR_ERR(pb->pwm);
+ if (ret != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "unable to request PWM\n");
+ goto err_alloc;
}
dev_dbg(&pdev->dev, "got pwm for backlight\n");
--
2.5.0
^ permalink raw reply related
* Re: [PATCH] Revert "backlight: pwm: Handle EPROBE_DEFER while requesting the PWM"
From: Vladimir Zapolskiy @ 2015-10-12 12:39 UTC (permalink / raw)
To: Boris Brezillon, Thierry Reding
Cc: Nicolas Ferre, Robert Jarzmik, linux-pwm, Alexandre Belloni,
Jingoo Han, Lee Jones, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
In-Reply-To: <20151005153024.3a245b0b@bbrezillon>
On 05.10.2015 16:30, Boris Brezillon wrote:
> On Mon, 5 Oct 2015 14:58:03 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
>> On Mon, Oct 05, 2015 at 01:19:12PM +0200, Boris Brezillon wrote:
>>> Hi Thierry,
>>>
>>> On Mon, 5 Oct 2015 11:35:43 +0200
>>> Thierry Reding <thierry.reding@gmail.com> wrote:
>>>
>>>> On Thu, Oct 01, 2015 at 10:00:22AM +0200, Nicolas Ferre wrote:
>>>>> Le 30/09/2015 21:29, Robert Jarzmik a écrit :
>>>>>> Robert Jarzmik <robert.jarzmik@free.fr> writes:
>>>>>>
>>>>>>> This reverts commit 68feaca0b13e453aa14ee064c1736202b48b342f.
>>>>>>> This commit breaks legacy platforms, for which :
>>>>>>> (a) no pwm table is added (legacy platforms)
>>>>>>> (b) in this case, in pwm_get(), pmw_lookup_list is empty, and therefore
>>>>>>> chosen = NULL, and therefore pwm_get() returns NULL, and pwm_get()
>>>>>>> returns -EPROBE_DEFER
>>>>>>> (c) as a consequence, this code is unreachable in pwm_bl.c :
>>>>>>> if (IS_ERR(pb->pwm)) {
>>>>>>> ret = PTR_ERR(pb->pwm);
>>>>>>> dev_info(&pdev->dev, "%s:%d(): %d\n", __func__, __LINE__, ret);
>>>>>>> if (ret = -EPROBE_DEFER)
>>>>>>> goto err_alloc;
>>>>>>>
>>>>>>> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
>>>>>>> pb->legacy = true;
>>>>>>> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
>>>>>>>
>>>>>>> As this code is unreachable, all legacy platforms relying on pwm_id are
>>>>>>> broken, amongst which pxa have been tested as broken.
>>>>>>>
>>>>>>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>>>>>> Thierry, would you have a look please ?
>>>>>> As I said before, all legacy platform relying on pwm_id are broken. I'd like to
>>>>>> be sure this lands in the next -rc series.
>>>>>
>>>>> Well, as I answered on the linux-pwm mailing-list (I was not in copy) here:
>>>>> http://article.gmane.org/gmane.linux.pwm/2744
>>>>> I wonder if it's not easier to fix the platforms and add the pwm tables...
>>>>>
>>>>> Otherwise, Boris proposed this fix:
>>>>> 8<-----------------------------------------------------------
>>>>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>>>>> index eff379b..00483d4 100644
>>>>> --- a/drivers/video/backlight/pwm_bl.c
>>>>> +++ b/drivers/video/backlight/pwm_bl.c
>>>>> @@ -273,15 +273,15 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>>>>> pb->pwm = devm_pwm_get(&pdev->dev, NULL);
>>>>> if (IS_ERR(pb->pwm)) {
>>>>> ret = PTR_ERR(pb->pwm);
>>>>> - if (ret = -EPROBE_DEFER)
>>>>> - goto err_alloc;
>>>>>
>>>>> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
>>>>> pb->legacy = true;
>>>>> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
>>>>> if (IS_ERR(pb->pwm)) {
>>>>> dev_err(&pdev->dev, "unable to request legacy PWM\n");
>>>>> - ret = PTR_ERR(pb->pwm);
>>>>> + if (ret != -EPROBE_DEFER)
>>>>> + ret = PTR_ERR(pb->pwm);
>>>>> +
>>>>> goto err_alloc;
>>>>> }
>>>>> }
>>>>>
>>>>> which is not tested and may add an extra non-valid error log.
>>>>
>>>> This is a little risky in my opinion. Not only does it print two error
>>>> messages for non-legacy platforms (that would be another regression if
>>>> you want to be nit-picking), but it is subtly buggy. If you have a
>>>> system with multiple PWM providers, you could end up failing the first
>>>> pwm_get() with -EPROBE_DEFER but then continue to the legacy case, and
>>>> this could succeed because data->pwm_id = 0, and that other provider
>>>> could be exporting the PWM with this ID. If I remember correctly this
>>>> was one of the reasons why the offending commit was merged in the first
>>>> place.
>>>
>>> Just for the record, when I proposed this fix to Nicolas, I clearly
>>> stated that this was not the way to go, and that fixing the offending
>>> platforms to use PWM lookup table was the only sane solution, though I
>>> didn't thought about the invalid PWM id case leading to buggy behavior.
>>
>> As chance would have it, this bubbled to the top of my inbox today:
>>
>> http://patchwork.ozlabs.org/patch/483993/
>
> AFAICT, this is not valid either. This patch is assuming -EPROBE_DEFER
> can only be returned in the DT case, which is not the case:
FYI at the time I've created and sent the change for review (Oct. 11,
2014 (!), https://patchwork.ozlabs.org/patch/398849/) there was no any
known to me intention to handle -EPROBE_DEFER .
Last week Lee mentioned that the patch does not apply due to added
-EPROBE_DEFER handling, so I've sent today to linux-pwm a rebased
version of the same change, which hopefully is acceptable, please take a
look.
> it is also returned if the PWMs were declared with a lookup table
> but the driver is not registered yet (module not loaded, or driver
> registration taking place after the PWM backlight driver).
>
> If we were about to differentiate the missing PWM definition from
> the missing driver case, we should do something like this [1].
>
> Best Regards,
>
> Boris
>
> [1]http://code.bulix.org/2oozbq-89125
>
>
--
With best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH v2] backlight: pwm: reject legacy pwm request for device defined in dt
From: Nicolas Ferre @ 2015-10-12 13:16 UTC (permalink / raw)
To: Vladimir Zapolskiy, Thierry Reding, Lee Jones, Jingoo Han,
Boris BREZILLON, Robert Jarzmik
Cc: linux-pwm, linux-fbdev
In-Reply-To: <1444652943-19712-1-git-send-email-vladimir_zapolskiy@mentor.com>
Le 12/10/2015 14:29, Vladimir Zapolskiy a écrit :
> Platform PWM backlight data provided by board's device tree should be
> complete enough to successfully request a pwm device using pwm_get()
> API. This change fixes a bug, when an arbitrary (first found) PWM is
> connected to a "pwm-backlight" compatible device, when explicit PWM
> device reference is not given.
>
> Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> already describes "pwms" as a required property, instead of blind
> selection of a potentially wrong PWM reject legacy PWM device
> registration request, leave legacy API only for non-dt cases.
>
> Based on initial implementation done by Dmitry Eremin-Solenikov.
>
> Reported-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> Acked-by: Lee Jones <lee.jones@linaro.org>
It seems good to me:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
(Adding some people to the Cc: list).
> ---
> The change is based on lee-backlight/for-backlight-next
>
> Changes from v1 to v2:
> * rebased on top of Nicolas' commit
> 68feaca0b13 ("backlight: pwm: Handle EPROBE_DEFER while requesting the PWM")
>
> Links to previous discussions of the change:
> * https://patchwork.ozlabs.org/patch/483993/
> * https://patchwork.ozlabs.org/patch/398849/
>
> drivers/video/backlight/pwm_bl.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index eff379b..ae3c6b6 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -271,19 +271,18 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> }
>
> pb->pwm = devm_pwm_get(&pdev->dev, NULL);
> - if (IS_ERR(pb->pwm)) {
> - ret = PTR_ERR(pb->pwm);
> - if (ret = -EPROBE_DEFER)
> - goto err_alloc;
> -
> + if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
> + && !pdev->dev.of_node) {
> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
> pb->legacy = true;
> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
> - if (IS_ERR(pb->pwm)) {
> - dev_err(&pdev->dev, "unable to request legacy PWM\n");
> - ret = PTR_ERR(pb->pwm);
> - goto err_alloc;
> - }
> + }
> +
> + if (IS_ERR(pb->pwm)) {
> + ret = PTR_ERR(pb->pwm);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "unable to request PWM\n");
> + goto err_alloc;
> }
>
> dev_dbg(&pdev->dev, "got pwm for backlight\n");
>
--
Nicolas Ferre
^ permalink raw reply
* Re: [PATCH v2] backlight: pwm: reject legacy pwm request for device defined in dt
From: Boris Brezillon @ 2015-10-12 13:30 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Nicolas Ferre, Thierry Reding, Lee Jones, Jingoo Han,
Robert Jarzmik, linux-pwm, linux-fbdev
In-Reply-To: <561BB2BC.9090907@atmel.com>
Hi Vladimir,
On Mon, 12 Oct 2015 15:16:44 +0200
Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> Le 12/10/2015 14:29, Vladimir Zapolskiy a écrit :
> > Platform PWM backlight data provided by board's device tree should be
> > complete enough to successfully request a pwm device using pwm_get()
> > API. This change fixes a bug, when an arbitrary (first found) PWM is
> > connected to a "pwm-backlight" compatible device, when explicit PWM
> > device reference is not given.
> >
> > Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> > already describes "pwms" as a required property, instead of blind
> > selection of a potentially wrong PWM reject legacy PWM device
> > registration request, leave legacy API only for non-dt cases.
> >
> > Based on initial implementation done by Dmitry Eremin-Solenikov.
> >
> > Reported-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> > Acked-by: Thierry Reding <thierry.reding@gmail.com>
> > Acked-by: Lee Jones <lee.jones@linaro.org>
>
> It seems good to me:
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>
> (Adding some people to the Cc: list).
>
>
> > ---
> > The change is based on lee-backlight/for-backlight-next
> >
> > Changes from v1 to v2:
> > * rebased on top of Nicolas' commit
> > 68feaca0b13 ("backlight: pwm: Handle EPROBE_DEFER while requesting the PWM")
> >
> > Links to previous discussions of the change:
> > * https://patchwork.ozlabs.org/patch/483993/
> > * https://patchwork.ozlabs.org/patch/398849/
> >
> > drivers/video/backlight/pwm_bl.c | 19 +++++++++----------
> > 1 file changed, 9 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index eff379b..ae3c6b6 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -271,19 +271,18 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > }
> >
> > pb->pwm = devm_pwm_get(&pdev->dev, NULL);
> > - if (IS_ERR(pb->pwm)) {
> > - ret = PTR_ERR(pb->pwm);
> > - if (ret = -EPROBE_DEFER)
> > - goto err_alloc;
> > -
> > + if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
> > + && !pdev->dev.of_node) {
> > dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
> > pb->legacy = true;
> > pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
> > - if (IS_ERR(pb->pwm)) {
> > - dev_err(&pdev->dev, "unable to request legacy PWM\n");
> > - ret = PTR_ERR(pb->pwm);
> > - goto err_alloc;
> > - }
> > + }
> > +
> > + if (IS_ERR(pb->pwm)) {
> > + ret = PTR_ERR(pb->pwm);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "unable to request PWM\n");
> > + goto err_alloc;
> > }
> >
> > dev_dbg(&pdev->dev, "got pwm for backlight\n");
> >
>
>
I still think it would be cleaner to do what Thierry proposed here [1].
IMO, embedding the complexity of different error cases depending on the
way PWM devices were defined (OF, pdata, ...) is rather risky and
make the code even more complicated.
Best Regards,
Boris
[1]https://lkml.org/lkml/2015/10/5/319
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v2] backlight: pwm: reject legacy pwm request for device defined in dt
From: Vladimir Zapolskiy @ 2015-10-12 13:54 UTC (permalink / raw)
To: Boris Brezillon
Cc: Nicolas Ferre, Thierry Reding, Lee Jones, Jingoo Han,
Robert Jarzmik, linux-pwm, linux-fbdev
In-Reply-To: <20151012153029.62f948d2@bbrezillon>
Hi Boris,
On 12.10.2015 16:30, Boris Brezillon wrote:
> Hi Vladimir,
>
> On Mon, 12 Oct 2015 15:16:44 +0200
> Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>
>> Le 12/10/2015 14:29, Vladimir Zapolskiy a écrit :
>>> Platform PWM backlight data provided by board's device tree should be
>>> complete enough to successfully request a pwm device using pwm_get()
>>> API. This change fixes a bug, when an arbitrary (first found) PWM is
>>> connected to a "pwm-backlight" compatible device, when explicit PWM
>>> device reference is not given.
>>>
>>> Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>>> already describes "pwms" as a required property, instead of blind
>>> selection of a potentially wrong PWM reject legacy PWM device
>>> registration request, leave legacy API only for non-dt cases.
>>>
>>> Based on initial implementation done by Dmitry Eremin-Solenikov.
>>>
>>> Reported-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>> Acked-by: Thierry Reding <thierry.reding@gmail.com>
>>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>
>> It seems good to me:
>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>
>> (Adding some people to the Cc: list).
>>
>>
>>> ---
>>> The change is based on lee-backlight/for-backlight-next
>>>
>>> Changes from v1 to v2:
>>> * rebased on top of Nicolas' commit
>>> 68feaca0b13 ("backlight: pwm: Handle EPROBE_DEFER while requesting the PWM")
>>>
>>> Links to previous discussions of the change:
>>> * https://patchwork.ozlabs.org/patch/483993/
>>> * https://patchwork.ozlabs.org/patch/398849/
>>>
>>> drivers/video/backlight/pwm_bl.c | 19 +++++++++----------
>>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>>> index eff379b..ae3c6b6 100644
>>> --- a/drivers/video/backlight/pwm_bl.c
>>> +++ b/drivers/video/backlight/pwm_bl.c
>>> @@ -271,19 +271,18 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>>> }
>>>
>>> pb->pwm = devm_pwm_get(&pdev->dev, NULL);
>>> - if (IS_ERR(pb->pwm)) {
>>> - ret = PTR_ERR(pb->pwm);
>>> - if (ret = -EPROBE_DEFER)
>>> - goto err_alloc;
>>> -
>>> + if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
>>> + && !pdev->dev.of_node) {
>>> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
>>> pb->legacy = true;
>>> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
>>> - if (IS_ERR(pb->pwm)) {
>>> - dev_err(&pdev->dev, "unable to request legacy PWM\n");
>>> - ret = PTR_ERR(pb->pwm);
>>> - goto err_alloc;
>>> - }
>>> + }
>>> +
>>> + if (IS_ERR(pb->pwm)) {
>>> + ret = PTR_ERR(pb->pwm);
>>> + if (ret != -EPROBE_DEFER)
>>> + dev_err(&pdev->dev, "unable to request PWM\n");
>>> + goto err_alloc;
>>> }
>>>
>>> dev_dbg(&pdev->dev, "got pwm for backlight\n");
>>>
>>
>>
>
> I still think it would be cleaner to do what Thierry proposed here [1].
> IMO, embedding the complexity of different error cases depending on the
> way PWM devices were defined (OF, pdata, ...) is rather risky and
> make the code even more complicated.
please correct me if I'm wrong, I suppose Thierry's change fixes
Nicolas' commit 68feaca0b13 only, and the intention of my change is to
fix an absolutely unrelated problem, see the commit message.
So, since still there is a remained chance of getting -EPROBE_DEFER from
pwm_get(), e.g. from of_pwm_get() or failed pwmchip_find_by_name() or
pwm->chip->ops->request() I don't see how Thierry's change alone may
help me to overcome the problem I'm trying to solve here.
> Best Regards,
>
> Boris
>
> [1]https://lkml.org/lkml/2015/10/5/319
>
>
--
With best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH v2] backlight: pwm: reject legacy pwm request for device defined in dt
From: Boris Brezillon @ 2015-10-12 14:06 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Nicolas Ferre, Thierry Reding, Lee Jones, Jingoo Han,
Robert Jarzmik, linux-pwm, linux-fbdev
In-Reply-To: <561BBB9F.6060808@mentor.com>
On Mon, 12 Oct 2015 16:54:39 +0300
Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote:
> Hi Boris,
>
> On 12.10.2015 16:30, Boris Brezillon wrote:
> > Hi Vladimir,
> >
> > On Mon, 12 Oct 2015 15:16:44 +0200
> > Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> >
> >> Le 12/10/2015 14:29, Vladimir Zapolskiy a écrit :
> >>> Platform PWM backlight data provided by board's device tree should be
> >>> complete enough to successfully request a pwm device using pwm_get()
> >>> API. This change fixes a bug, when an arbitrary (first found) PWM is
> >>> connected to a "pwm-backlight" compatible device, when explicit PWM
> >>> device reference is not given.
> >>>
> >>> Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> >>> already describes "pwms" as a required property, instead of blind
> >>> selection of a potentially wrong PWM reject legacy PWM device
> >>> registration request, leave legacy API only for non-dt cases.
> >>>
> >>> Based on initial implementation done by Dmitry Eremin-Solenikov.
> >>>
> >>> Reported-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> >>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >>> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> >>> Acked-by: Lee Jones <lee.jones@linaro.org>
> >>
> >> It seems good to me:
> >> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> >>
> >> (Adding some people to the Cc: list).
> >>
> >>
> >>> ---
> >>> The change is based on lee-backlight/for-backlight-next
> >>>
> >>> Changes from v1 to v2:
> >>> * rebased on top of Nicolas' commit
> >>> 68feaca0b13 ("backlight: pwm: Handle EPROBE_DEFER while requesting the PWM")
> >>>
> >>> Links to previous discussions of the change:
> >>> * https://patchwork.ozlabs.org/patch/483993/
> >>> * https://patchwork.ozlabs.org/patch/398849/
> >>>
> >>> drivers/video/backlight/pwm_bl.c | 19 +++++++++----------
> >>> 1 file changed, 9 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> >>> index eff379b..ae3c6b6 100644
> >>> --- a/drivers/video/backlight/pwm_bl.c
> >>> +++ b/drivers/video/backlight/pwm_bl.c
> >>> @@ -271,19 +271,18 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> >>> }
> >>>
> >>> pb->pwm = devm_pwm_get(&pdev->dev, NULL);
> >>> - if (IS_ERR(pb->pwm)) {
> >>> - ret = PTR_ERR(pb->pwm);
> >>> - if (ret = -EPROBE_DEFER)
> >>> - goto err_alloc;
> >>> -
> >>> + if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
> >>> + && !pdev->dev.of_node) {
> >>> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
> >>> pb->legacy = true;
> >>> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
> >>> - if (IS_ERR(pb->pwm)) {
> >>> - dev_err(&pdev->dev, "unable to request legacy PWM\n");
> >>> - ret = PTR_ERR(pb->pwm);
> >>> - goto err_alloc;
> >>> - }
> >>> + }
> >>> +
> >>> + if (IS_ERR(pb->pwm)) {
> >>> + ret = PTR_ERR(pb->pwm);
> >>> + if (ret != -EPROBE_DEFER)
> >>> + dev_err(&pdev->dev, "unable to request PWM\n");
> >>> + goto err_alloc;
> >>> }
> >>>
> >>> dev_dbg(&pdev->dev, "got pwm for backlight\n");
> >>>
> >>
> >>
> >
> > I still think it would be cleaner to do what Thierry proposed here [1].
> > IMO, embedding the complexity of different error cases depending on the
> > way PWM devices were defined (OF, pdata, ...) is rather risky and
> > make the code even more complicated.
>
> please correct me if I'm wrong, I suppose Thierry's change fixes
> Nicolas' commit 68feaca0b13 only, and the intention of my change is to
> fix an absolutely unrelated problem, see the commit message.
>
> So, since still there is a remained chance of getting -EPROBE_DEFER from
> pwm_get(), e.g. from of_pwm_get() or failed pwmchip_find_by_name() or
> pwm->chip->ops->request() I don't see how Thierry's change alone may
> help me to overcome the problem I'm trying to solve here.
The only valid case where EPROBE_DEFER should be returned is when we
have a device that is not ready to be used yet (but we're sure that we
have this device declared, using either the PWM lookup table or the DT
definition in the PWM subsystem case).
Thierry's patch makes sure that EPROBE_DEFER is not returned when the
PWM device definition is not found using in the PWM lookup tables or
the DT definition, and in this case the pwm_bl code will fallback to
the legacy PWM API, which AFAICT is what you're trying to solve.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v2] backlight: pwm: reject legacy pwm request for device defined in dt
From: Vladimir Zapolskiy @ 2015-10-12 14:19 UTC (permalink / raw)
To: Boris Brezillon
Cc: Nicolas Ferre, Thierry Reding, Lee Jones, Jingoo Han,
Robert Jarzmik, linux-pwm, linux-fbdev
In-Reply-To: <20151012160608.41f04553@bbrezillon>
On 12.10.2015 17:06, Boris Brezillon wrote:
> On Mon, 12 Oct 2015 16:54:39 +0300
> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote:
>
>> Hi Boris,
>>
>> On 12.10.2015 16:30, Boris Brezillon wrote:
>>> Hi Vladimir,
>>>
>>> On Mon, 12 Oct 2015 15:16:44 +0200
>>> Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>>>
>>>> Le 12/10/2015 14:29, Vladimir Zapolskiy a écrit :
>>>>> Platform PWM backlight data provided by board's device tree should be
>>>>> complete enough to successfully request a pwm device using pwm_get()
>>>>> API. This change fixes a bug, when an arbitrary (first found) PWM is
>>>>> connected to a "pwm-backlight" compatible device, when explicit PWM
>>>>> device reference is not given.
>>>>>
>>>>> Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>>>>> already describes "pwms" as a required property, instead of blind
>>>>> selection of a potentially wrong PWM reject legacy PWM device
>>>>> registration request, leave legacy API only for non-dt cases.
>>>>>
>>>>> Based on initial implementation done by Dmitry Eremin-Solenikov.
>>>>>
>>>>> Reported-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>>>> Acked-by: Thierry Reding <thierry.reding@gmail.com>
>>>>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>>>
>>>> It seems good to me:
>>>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>>>
>>>> (Adding some people to the Cc: list).
>>>>
>>>>
>>>>> ---
>>>>> The change is based on lee-backlight/for-backlight-next
>>>>>
>>>>> Changes from v1 to v2:
>>>>> * rebased on top of Nicolas' commit
>>>>> 68feaca0b13 ("backlight: pwm: Handle EPROBE_DEFER while requesting the PWM")
>>>>>
>>>>> Links to previous discussions of the change:
>>>>> * https://patchwork.ozlabs.org/patch/483993/
>>>>> * https://patchwork.ozlabs.org/patch/398849/
>>>>>
>>>>> drivers/video/backlight/pwm_bl.c | 19 +++++++++----------
>>>>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>>>>> index eff379b..ae3c6b6 100644
>>>>> --- a/drivers/video/backlight/pwm_bl.c
>>>>> +++ b/drivers/video/backlight/pwm_bl.c
>>>>> @@ -271,19 +271,18 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>>>>> }
>>>>>
>>>>> pb->pwm = devm_pwm_get(&pdev->dev, NULL);
>>>>> - if (IS_ERR(pb->pwm)) {
>>>>> - ret = PTR_ERR(pb->pwm);
>>>>> - if (ret = -EPROBE_DEFER)
>>>>> - goto err_alloc;
>>>>> -
>>>>> + if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
>>>>> + && !pdev->dev.of_node) {
>>>>> dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
>>>>> pb->legacy = true;
>>>>> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
>>>>> - if (IS_ERR(pb->pwm)) {
>>>>> - dev_err(&pdev->dev, "unable to request legacy PWM\n");
>>>>> - ret = PTR_ERR(pb->pwm);
>>>>> - goto err_alloc;
>>>>> - }
>>>>> + }
>>>>> +
>>>>> + if (IS_ERR(pb->pwm)) {
>>>>> + ret = PTR_ERR(pb->pwm);
>>>>> + if (ret != -EPROBE_DEFER)
>>>>> + dev_err(&pdev->dev, "unable to request PWM\n");
>>>>> + goto err_alloc;
>>>>> }
>>>>>
>>>>> dev_dbg(&pdev->dev, "got pwm for backlight\n");
>>>>>
>>>>
>>>>
>>>
>>> I still think it would be cleaner to do what Thierry proposed here [1].
>>> IMO, embedding the complexity of different error cases depending on the
>>> way PWM devices were defined (OF, pdata, ...) is rather risky and
>>> make the code even more complicated.
>>
>> please correct me if I'm wrong, I suppose Thierry's change fixes
>> Nicolas' commit 68feaca0b13 only, and the intention of my change is to
>> fix an absolutely unrelated problem, see the commit message.
>>
>> So, since still there is a remained chance of getting -EPROBE_DEFER from
>> pwm_get(), e.g. from of_pwm_get() or failed pwmchip_find_by_name() or
>> pwm->chip->ops->request() I don't see how Thierry's change alone may
>> help me to overcome the problem I'm trying to solve here.
>
> The only valid case where EPROBE_DEFER should be returned is when we
> have a device that is not ready to be used yet (but we're sure that we
> have this device declared, using either the PWM lookup table or the DT
> definition in the PWM subsystem case).
That's fine, and it is reflected in my change.
> Thierry's patch makes sure that EPROBE_DEFER is not returned when the
> PWM device definition is not found using in the PWM lookup tables or
> the DT definition,
This is okay, but I'm interested in proper handling of cases other than
EPROBE_DEFER. EPROBE_DEFER and the related issues are on your balance
and I'm attempting to avoid interfering with it here :)
> and in this case the pwm_bl code will fallback to
> the legacy PWM API, which AFAICT is what you're trying to solve.
Fallback must happen exclusively under (IS_ERR(pb->pwm) &&
PTR_ERR(pb->pwm) != -EPROBE_DEFER && !pdev->dev.of_node) condition IMHO.
Before EPROBE_DEFER appeared on the scene the condition was
(IS_ERR(pb->pwm) && !pdev->dev.of_node).
So, the question is if my change requires any updates or not from your
point of view.
--
With best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH v2] backlight: pwm: reject legacy pwm request for device defined in dt
From: Boris Brezillon @ 2015-10-12 15:19 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Nicolas Ferre, Thierry Reding, Lee Jones, Jingoo Han,
Robert Jarzmik, linux-pwm, linux-fbdev
In-Reply-To: <561BC177.2050000@mentor.com>
On Mon, 12 Oct 2015 17:19:35 +0300
Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote:
>
> > Thierry's patch makes sure that EPROBE_DEFER is not returned when the
> > PWM device definition is not found using in the PWM lookup tables or
> > the DT definition,
>
> This is okay, but I'm interested in proper handling of cases other than
> EPROBE_DEFER. EPROBE_DEFER and the related issues are on your balance
> and I'm attempting to avoid interfering with it here :)
I keep thinking we should fix all platforms using the ->pwm_id pdata
field to attach a PWM device to a PWM backlight instead of trying to
guess when falling back to the legacy API is acceptable...
>
> > and in this case the pwm_bl code will fallback to
> > the legacy PWM API, which AFAICT is what you're trying to solve.
>
> Fallback must happen exclusively under (IS_ERR(pb->pwm) &&
> PTR_ERR(pb->pwm) != -EPROBE_DEFER && !pdev->dev.of_node) condition IMHO.
>
> Before EPROBE_DEFER appeared on the scene the condition was
> (IS_ERR(pb->pwm) && !pdev->dev.of_node).
>
> So, the question is if my change requires any updates or not from your
> point of view.
... but from a functional point of view your patch seems correct.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ 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