* [PATCH v2 1/5] gxt4500: enable on non-PPC architectures
From: Ondrej Zary @ 2015-10-01 21:22 UTC (permalink / raw)
To: Paul Mackerras; +Cc: =Tomi Valkeinen, linux-fbdev, Kernel development list
In-Reply-To: <1443734575-12468-1-git-send-email-linux@rainbow-software.org>
These chips can be present at least on x86 too - Fire GL2 AGP has GXT6000P but
this driver is currently limited to PPC.
Enable it for all architectures and add chip configuration for little-endian.
Tested on x86 with Fire GL2 AGP.
Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
drivers/video/fbdev/Kconfig | 5 +++--
drivers/video/fbdev/gxt4500.c | 7 +++++++
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 8b1d371..10ff920 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -2132,7 +2132,7 @@ config FB_UDL
config FB_IBM_GXT4500
tristate "Framebuffer support for IBM GXT4000P/4500P/6000P/6500P adaptors"
- depends on FB && PPC
+ depends on FB
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
@@ -2140,7 +2140,8 @@ config FB_IBM_GXT4500
Say Y here to enable support for the IBM GXT4000P/6000P and
GXT4500P/6500P display adaptor based on Raster Engine RC1000,
found on some IBM System P (pSeries) machines. This driver
- doesn't use Geometry Engine GT1000.
+ doesn't use Geometry Engine GT1000. This driver also supports
+ AGP Fire GL2/3/4 cards on x86.
config FB_PS3
tristate "PS3 GPU framebuffer driver"
diff --git a/drivers/video/fbdev/gxt4500.c b/drivers/video/fbdev/gxt4500.c
index f19133a..ae68696 100644
--- a/drivers/video/fbdev/gxt4500.c
+++ b/drivers/video/fbdev/gxt4500.c
@@ -670,8 +670,15 @@ static int gxt4500_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
pci_set_drvdata(pdev, info);
+#ifdef __BIG_ENDIAN
/* Set byte-swapping for DFA aperture for all pixel sizes */
pci_write_config_dword(pdev, CFG_ENDIAN0, 0x333300);
+#else /* __LITTLE_ENDIAN */
+ /* not sure what this means but fgl23 driver does that */
+ pci_write_config_dword(pdev, CFG_ENDIAN0, 0x2300);
+/* pci_write_config_dword(pdev, CFG_ENDIAN0 + 4, 0x400000);*/
+ pci_write_config_dword(pdev, CFG_ENDIAN0 + 8, 0x98530000);
+#endif
info->fbops = &gxt4500_ops;
info->flags = FBINFO_FLAG_DEFAULT;
--
Ondrej Zary
^ permalink raw reply related
* [PATCH v2 2/5] gxt4500: fix 16bpp 565 mode
From: Ondrej Zary @ 2015-10-01 21:22 UTC (permalink / raw)
To: Paul Mackerras; +Cc: =Tomi Valkeinen, linux-fbdev, Kernel development list
In-Reply-To: <1443734575-12468-1-git-send-email-linux@rainbow-software.org>
Fix wrong colors in 16bpp 565 mode.
Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
drivers/video/fbdev/gxt4500.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/gxt4500.c b/drivers/video/fbdev/gxt4500.c
index ae68696..3c481d0 100644
--- a/drivers/video/fbdev/gxt4500.c
+++ b/drivers/video/fbdev/gxt4500.c
@@ -525,7 +525,7 @@ static int gxt4500_setcolreg(unsigned int reg, unsigned int red,
u32 val = reg;
switch (par->pixfmt) {
case DFA_PIX_16BIT_565:
- val |= (reg << 11) | (reg << 6);
+ val |= (reg << 11) | (reg << 5);
break;
case DFA_PIX_16BIT_1555:
val |= (reg << 10) | (reg << 5);
--
Ondrej Zary
^ permalink raw reply related
* [PATCH v2 3/5] gxt4500: fix color order
From: Ondrej Zary @ 2015-10-01 21:22 UTC (permalink / raw)
To: Paul Mackerras; +Cc: =Tomi Valkeinen, linux-fbdev, Kernel development list
In-Reply-To: <1443734575-12468-1-git-send-email-linux@rainbow-software.org>
The color order in truecolor modes is wrong. This does not affect console but
is visible e.g. in X11 which has wrong colors.
Swap blue and red colors to fix the problem.
Fixes https://forums.gentoo.org/viewtopic-t-692740-start-0.html
Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
drivers/video/fbdev/gxt4500.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/gxt4500.c b/drivers/video/fbdev/gxt4500.c
index 3c481d0..3ceddb8 100644
--- a/drivers/video/fbdev/gxt4500.c
+++ b/drivers/video/fbdev/gxt4500.c
@@ -347,11 +347,12 @@ static void gxt4500_unpack_pixfmt(struct fb_var_screeninfo *var,
break;
}
if (pixfmt != DFA_PIX_8BIT) {
- var->green.offset = var->red.length;
- var->blue.offset = var->green.offset + var->green.length;
+ var->blue.offset = 0;
+ var->green.offset = var->blue.length;
+ var->red.offset = var->green.offset + var->green.length;
if (var->transp.length)
var->transp.offset - var->blue.offset + var->blue.length;
+ var->red.offset + var->red.length;
}
}
--
Ondrej Zary
^ permalink raw reply related
* [PATCH v2 4/5] gxt4500: Use arch_phys_wc_* for framebuffer
From: Ondrej Zary @ 2015-10-01 21:22 UTC (permalink / raw)
To: Paul Mackerras; +Cc: =Tomi Valkeinen, linux-fbdev, Kernel development list
In-Reply-To: <1443734575-12468-1-git-send-email-linux@rainbow-software.org>
Add arch_phys_wc_* calls to allow write-combining using MTRR.
Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
drivers/video/fbdev/gxt4500.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/gxt4500.c b/drivers/video/fbdev/gxt4500.c
index 3ceddb8..31de650 100644
--- a/drivers/video/fbdev/gxt4500.c
+++ b/drivers/video/fbdev/gxt4500.c
@@ -142,7 +142,7 @@ static const unsigned char watfmt[] = {
struct gxt4500_par {
void __iomem *regs;
-
+ int wc_cookie;
int pixfmt; /* pixel format, see DFA_PIX_* values */
/* PLL parameters */
@@ -671,6 +671,9 @@ static int gxt4500_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
pci_set_drvdata(pdev, info);
+ par->wc_cookie = arch_phys_wc_add(info->fix.smem_start,
+ info->fix.smem_len);
+
#ifdef __BIG_ENDIAN
/* Set byte-swapping for DFA aperture for all pixel sizes */
pci_write_config_dword(pdev, CFG_ENDIAN0, 0x333300);
@@ -735,6 +738,7 @@ static void gxt4500_remove(struct pci_dev *pdev)
return;
par = info->par;
unregister_framebuffer(info);
+ arch_phys_wc_del(par->wc_cookie);
fb_dealloc_cmap(&info->cmap);
iounmap(par->regs);
iounmap(info->screen_base);
--
Ondrej Zary
^ permalink raw reply related
* [PATCH v2 5/5] gxt4500: enable panning
From: Ondrej Zary @ 2015-10-01 21:22 UTC (permalink / raw)
To: Paul Mackerras; +Cc: =Tomi Valkeinen, linux-fbdev, Kernel development list
In-Reply-To: <1443734575-12468-1-git-send-email-linux@rainbow-software.org>
The driver implements pan_display but the corresponding flags are not set.
Add FBINFO_HWACCEL_XPAN and FBINFO_HWACCEL_YPAN to flags to allow HW
accelerated panning (for fast scrolling).
Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
drivers/video/fbdev/gxt4500.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/gxt4500.c b/drivers/video/fbdev/gxt4500.c
index 31de650..f438546 100644
--- a/drivers/video/fbdev/gxt4500.c
+++ b/drivers/video/fbdev/gxt4500.c
@@ -685,7 +685,8 @@ static int gxt4500_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
#endif
info->fbops = &gxt4500_ops;
- info->flags = FBINFO_FLAG_DEFAULT;
+ info->flags = FBINFO_FLAG_DEFAULT | FBINFO_HWACCEL_XPAN |
+ FBINFO_HWACCEL_YPAN;
err = fb_alloc_cmap(&info->cmap, 256, 0);
if (err) {
--
Ondrej Zary
^ permalink raw reply related
* [BUG] RCU stall in cursor_timer_handler
From: Alistair Popple @ 2015-10-03 1:38 UTC (permalink / raw)
To: linux-fbdev, Scot Doyle
Cc: Benjamin Herrenschmidt, plagnioj, linux-kernel, airlied,
Pavel Machek, Greg Kroah-Hartman
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);
}
Adding WARN_ON(ops->cur_blink_jiffies < 1) to the above occasionally triggers and further testing shows ops->cur_blink_jiffies = 0, therefore I suspect cur_blink_jiffies is being used uninitialised. This patch seems to work around the problem:
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index 1aaf893..45d2a0a 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -416,6 +416,8 @@ static void fbcon_add_cursor_timer(struct fb_info *info)
INIT_WORK(&info->queue, fb_flashcursor);
init_timer(&ops->cursor_timer);
+ if (ops->cur_blink_jiffies < 1)
+ ops->cur_blink_jiffies = msecs_to_jiffies(200);
ops->cursor_timer.function = cursor_timer_handler;
ops->cursor_timer.expires = jiffies + ops->cur_blink_jiffies;
ops->cursor_timer.data = (unsigned long ) info;
It looks like the issue was introduced by:
commit 27a4c827c34ac4256a190cc9d24607f953c1c459
Author: Scot Doyle <lkml14@scotdoyle.com>
Date: Thu Mar 26 13:56:38 2015 +0000
fbcon: use the cursor blink interval provided by vt
vt now provides a cursor blink interval via vc_data. Use this
interval instead of the currently hardcoded 200 msecs. Store it in
fbcon_ops to avoid locking the console in cursor_timer_handler().
Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Regards,
Alistair
[0000006280989170] Generic RTC Driver v1.07
[0000006281152624] powernv_rng: Registered powernv hwrng.
[0000006281250526] [drm] Initialized drm 1.1.0 20060810
[0000006281323420] [drm] radeon kernel modesetting enabled.
[0000006281570416] ast 0001:10:00.0: enabling device (0140 -> 0142)
[0000006281932424] [drm] platform has no IO space, trying MMIO
[0000006281978870] [drm] AST 2400 detected
[0000006282070130] [drm] VGA not enabled on entry, requesting chip POST
[0000006282131234] [drm] Analog VGA only
[0000006282174414] [drm] dram 1632000000 7 16 00c00000
[0000006282324794] [TTM] Zone kernel: Available graphics memory: 66806240 kiB
[0000006282376184] [TTM] Zone dma32: Available graphics memory: 2097152 kiB
[0000006282551070] [TTM] Initializing pool allocator
[0000027281989546] INFO: rcu_sched self-detected stall on CPU
[0000027281996420] 154: (2100 ticks this GP) idleâ5/140000000000002/0 softirq23/326 fqs 99
[0000027282010008] (t!00 jiffies g=-260 c=-261 qC21)
[0000027282013264] Task dump for CPU 154:
[0000027282017276] swapper/136 R running task 0 1 0 0x00000804
[0000027282018170] Call Trace:
[0000027282023368] [c000001fe0301c50] [c00000000007fe98] sched_show_task+0x178/0x184 (unreliable)
[0000027282027166] [c000001fe0301cc0] [c0000000000a7150] rcu_dump_cpu_stacks+0xa0/0xd0
[0000027282030432] [c000001fe0301d10] [c0000000000a9ff8] rcu_check_callbacks+0x294/0x734
[0000027282032980] [c000001fe0301e30] [c0000000000ac2c4] update_process_times+0x3c/0x74
[0000027282036196] [c000001fe0301e60] [c0000000000bb864] tick_sched_handle.isra.16+0x60/0x6c
[0000027282038920] [c000001fe0301e90] [c0000000000bb8bc] tick_sched_timer+0x4c/0x84
[0000027282041588] [c000001fe0301ed0] [c0000000000acee4] __hrtimer_run_queues+0xf8/0x1ac
[0000027282044084] [c000001fe0301f60] [c0000000000ad1b0] hrtimer_interrupt+0xac/0x1d8
[0000027282047074] [c000001fe0302010] [c000000000017150] __timer_interrupt+0x7c/0xe0
[0000027282049460] [c000001fe0302050] [c000000000017314] timer_interrupt+0xac/0xc4
[0000027282052522] [c000001fe0302080] [c000000000002328] decrementer_common+0x128/0x180
[0000027282057424] --- interrupt: 901 at _restgpr0_29+0x8/0x18
LR = call_timer_fn+0x98/0xf4
[0000027282061014] [c000001fe0302370] [c0000000000ab5fc] run_timer_softirq+0x1dc/0x208 (unreliable)
[0000027282064008] [c000001fe0302400] [c000000000059df0] __do_softirq+0x158/0x2b4
[0000027282066428] [c000001fe03024f0] [c00000000005a1fc] irq_exit+0x74/0xcc
[0000027282068796] [c000001fe0302510] [c000000000017318] timer_interrupt+0xb0/0xc4
[0000027282071316] [c000001fe0302540] [c000000000002328] decrementer_common+0x128/0x180
[0000027282075708] --- interrupt: 901 at _memset_io+0x5c/0x98
LR = ttm_bo_move_memcpy+0xb8/0x46c
[0000027282078796] [c000001fe0302830] [c000000000496140] ttm_bo_move_memcpy+0x68/0x46c (unreliable)
[0000027282082222] [c000001fe0302970] [c0000000005b4338] ast_bo_move+0x20/0x34
[0000027282085106] [c000001fe0302990] [c000000000494520] ttm_bo_handle_move_mem+0x230/0x3d8
[0000027282087558] [c000001fe0302ab0] [c000000000495454] ttm_bo_validate+0x20c/0x28c
[0000027282090390] [c000001fe0302b60] [c0000000005b4a58] ast_bo_pin+0x90/0xbc
[0000027282093668] [c000001fe0302b90] [c0000000005b276c] ast_crtc_do_set_base.isra.8.constprop.13+0x120/0x298
[0000027282096382] [c000001fe0302c40] [c0000000005b354c] ast_crtc_mode_set+0xc68/0xc74
[0000027282099952] [c000001fe0302cf0] [c00000000045faa4] drm_crtc_helper_set_mode+0x348/0x4d8
[0000027282103026] [c000001fe0302f80] [c000000000460420] drm_crtc_helper_set_config+0x6c4/0x944
[0000027282106578] [c000001fe03030a0] [c00000000047bbc0] drm_mode_set_config_internal+0x68/0xf0
[0000027282109416] [c000001fe03030e0] [c00000000046aa40] restore_fbdev_mode+0xe8/0x110
[0000027282112658] [c000001fe0303120] [c00000000046c7a4] drm_fb_helper_restore_fbdev_mode_unlocked+0x38/0x7c
[0000027282115350] [c000001fe0303160] [c00000000046c82c] drm_fb_helper_set_par+0x44/0x58
[0000027282118480] [c000001fe03031c0] [c000000000402af0] fbcon_init+0x360/0x474
[0000027282121374] [c000001fe03032b0] [c000000000443850] visual_init+0xec/0x13c
[0000027282123870] [c000001fe03032f0] [c00000000044551c] do_bind_con_driver+0x280/0x398
[0000027282126484] [c000001fe03033b0] [c000000000445a38] do_take_over_console+0x1dc/0x1ec
[0000027282129536] [c000001fe0303460] [c000000000402c98] do_fbcon_takeover+0x94/0x114
[0000027282132768] [c000001fe0303480] [c0000000000754dc] notifier_call_chain+0x6c/0xa8
[0000027282135978] [c000001fe03034d0] [c0000000000758c4] __blocking_notifier_call_chain+0x58/0x74
[0000027282138972] [c000001fe0303520] [c0000000004088e4] fb_notifier_call_chain+0x2c/0x40
[0000027282142054] [c000001fe0303540] [c00000000040b0f0] register_framebuffer+0x298/0x2d8
[0000027282144998] [c000001fe0303630] [c00000000046cb88] drm_fb_helper_initial_config+0x348/0x3f8
[0000027282147988] [c000001fe03036d0] [c0000000005b4108] ast_fbdev_init+0xc8/0x100
[0000027282150858] [c000001fe0303710] [c0000000005b1468] ast_driver_load+0x778/0x7b4
[0000027282153364] [c000001fe03037a0] [c000000000475dac] drm_dev_register+0xb0/0x14c
[0000027282155930] [c000001fe03037f0] [c000000000477f74] drm_get_pci_dev+0x114/0x1bc
[0000027282158636] [c000001fe0303880] [c0000000005b08b0] ast_pci_probe+0x20/0x34
[0000027282161526] [c000001fe03038a0] [c0000000003f3160] pci_device_probe+0x78/0xdc
[0000027282164264] [c000001fe0303920] [c0000000005bfc20] driver_probe_device+0x128/0x2b0
[0000027282166980] [c000001fe03039b0] [c0000000005bfe40] __driver_attach+0x98/0xc8
[0000027282169602] [c000001fe03039f0] [c0000000005bdbcc] bus_for_each_dev+0xa4/0xb8
[0000027282172304] [c000001fe0303a40] [c0000000005bf6e0] driver_attach+0x2c/0x40
[0000027282174942] [c000001fe0303a60] [c0000000005bf1b8] bus_add_driver+0x100/0x240
[0000027282177590] [c000001fe0303af0] [c0000000005c06cc] driver_register+0xc0/0x110
[0000027282180170] [c000001fe0303b70] [c0000000003f2d40] __pci_register_driver+0x68/0x74
[0000027282182632] [c000001fe0303ba0] [c000000000478084] drm_pci_init+0x68/0x10c
[0000027282185604] [c000001fe0303c30] [c0000000010977fc] ast_init+0x3c/0x50
[0000027282188640] [c000001fe0303c50] [c00000000000ade4] do_one_initcall+0x134/0x1d0
[0000027282191604] [c000001fe0303d20] [c000000001073cac] kernel_init_freeable+0x198/0x254
[0000027282194406] [c000001fe0303dc0] [c00000000000b578] kernel_init+0x24/0x114
[0000027282197296] [c000001fe0303e30] [c0000000000094ac] ret_from_kernel_thread+0x5c/0xb0
[0000033872001022] NMI watchdog: BUG: soft lockup - CPU#154 stuck for 22s! [swapper/136:1]
[0000033872003536] Modules linked in:
[0000033872007714] CPU: 154 PID: 1 Comm: swapper/136 Not tainted 4.2.0-openpower1 #60
[0000033872010008] task: c000001fe0280000 ti: c000001fe0300000 task.ti: c000001fe0300000
[0000033872011464] NIP: c00000000000dca8 LR: c00000000000dca8 CTR: c000000000400c6c
[0000033872012960] REGS: c000001fe03020a0 TRAP: 0901 Not tainted (4.2.0-openpower1)
[0000033872018728] MSR: 9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 44008024 XER: 20000000
[0000033872028578] CFAR: c000000000093994 SOFTE: 1
GPR00: c000000000b73384 c000001fe0302320 c000000001776800 0000000000000900
GPR04: 00000000000001ce c000001fe0280958 c000001fe0280958 0000000000000001
GPR08: 0000000000000000 000000000000013f 0000000000000000 0000000000005bd0
GPR12: 0000000000008800 c00000000fe9ce00
[0000033872031252] NIP [c00000000000dca8] arch_local_irq_restore+0x5c/0x80
[0000033872033570] LR [c00000000000dca8] arch_local_irq_restore+0x5c/0x80
[0000033872034104] Call Trace:
[0000033872036578] [c000001fe0302320] [c000001fddc920d8] 0xc000001fddc920d8 (unreliable)
[0000033872039890] [c000001fe0302340] [c000000000b73384] _raw_spin_unlock_irq+0x44/0x6c
[0000033872042698] [c000001fe0302370] [c0000000000ab5e8] run_timer_softirq+0x1c8/0x208
[0000033872045086] [c000001fe0302400] [c000000000059df0] __do_softirq+0x158/0x2b4
[0000033872047264] [c000001fe03024f0] [c00000000005a1fc] irq_exit+0x74/0xcc
[0000033872049056] [c000001fe0302510] [c000000000017318] timer_interrupt+0xb0/0xc4
[0000033872050982] [c000001fe0302540] [c000000000002328] decrementer_common+0x128/0x180
[0000033872053750] --- interrupt: 901 at _memset_io+0x5c/0x98
LR = ttm_bo_move_memcpy+0xb8/0x46c
[0000033872056014] [c000001fe0302830] [c000000000496140] ttm_bo_move_memcpy+0x68/0x46c (unreliable)
[0000033872058166] [c000001fe0302970] [c0000000005b4338] ast_bo_move+0x20/0x34
[0000033872060102] [c000001fe0302990] [c000000000494520] ttm_bo_handle_move_mem+0x230/0x3d8
[0000033872061828] [c000001fe0302ab0] [c000000000495454] ttm_bo_validate+0x20c/0x28c
[0000033872063952] [c000001fe0302b60] [c0000000005b4a58] ast_bo_pin+0x90/0xbc
[0000033872066264] [c000001fe0302b90] [c0000000005b276c] ast_crtc_do_set_base.isra.8.constprop.13+0x120/0x298
[0000033872068250] [c000001fe0302c40] [c0000000005b354c] ast_crtc_mode_set+0xc68/0xc74
[0000033872070578] [c000001fe0302cf0] [c00000000045faa4] drm_crtc_helper_set_mode+0x348/0x4d8
[0000033872072840] [c000001fe0302f80] [c000000000460420] drm_crtc_helper_set_config+0x6c4/0x944
[0000033872075094] [c000001fe03030a0] [c00000000047bbc0] drm_mode_set_config_internal+0x68/0xf0
[0000033872077068] [c000001fe03030e0] [c00000000046aa40] restore_fbdev_mode+0xe8/0x110
[0000033872079358] [c000001fe0303120] [c00000000046c7a4] drm_fb_helper_restore_fbdev_mode_unlocked+0x38/0x7c
[0000033872081366] [c000001fe0303160] [c00000000046c82c] drm_fb_helper_set_par+0x44/0x58
[0000033872083428] [c000001fe03031c0] [c000000000402af0] fbcon_init+0x360/0x474
[0000033872085142] [c000001fe03032b0] [c000000000443850] visual_init+0xec/0x13c
[0000033872086958] [c000001fe03032f0] [c00000000044551c] do_bind_con_driver+0x280/0x398
[0000033872088796] [c000001fe03033b0] [c000000000445a38] do_take_over_console+0x1dc/0x1ec
[0000033872090818] [c000001fe0303460] [c000000000402c98] do_fbcon_takeover+0x94/0x114
[0000033872093108] [c000001fe0303480] [c0000000000754dc] notifier_call_chain+0x6c/0xa8
[0000033872095484] [c000001fe03034d0] [c0000000000758c4] __blocking_notifier_call_chain+0x58/0x74
[0000033872097708] [c000001fe0303520] [c0000000004088e4] fb_notifier_call_chain+0x2c/0x40
[0000033872099958] [c000001fe0303540] [c00000000040b0f0] register_framebuffer+0x298/0x2d8
[0000033872102108] [c000001fe0303630] [c00000000046cb88] drm_fb_helper_initial_config+0x348/0x3f8
[0000033872104178] [c000001fe03036d0] [c0000000005b4108] ast_fbdev_init+0xc8/0x100
[0000033872106108] [c000001fe0303710] [c0000000005b1468] ast_driver_load+0x778/0x7b4
[0000033872107796] [c000001fe03037a0] [c000000000475dac] drm_dev_register+0xb0/0x14c
[0000033872109608] [c000001fe03037f0] [c000000000477f74] drm_get_pci_dev+0x114/0x1bc
[0000033872111488] [c000001fe0303880] [c0000000005b08b0] ast_pci_probe+0x20/0x34
[0000033872113446] [c000001fe03038a0] [c0000000003f3160] pci_device_probe+0x78/0xdc
[0000033872115484] [c000001fe0303920] [c0000000005bfc20] driver_probe_device+0x128/0x2b0
[0000033872117444] [c000001fe03039b0] [c0000000005bfe40] __driver_attach+0x98/0xc8
[0000033872119284] [c000001fe03039f0] [c0000000005bdbcc] bus_for_each_dev+0xa4/0xb8
[0000033872121194] [c000001fe0303a40] [c0000000005bf6e0] driver_attach+0x2c/0x40
[0000033872123082] [c000001fe0303a60] [c0000000005bf1b8] bus_add_driver+0x100/0x240
[0000033872125046] [c000001fe0303af0] [c0000000005c06cc] driver_register+0xc0/0x110
[0000033872126934] [c000001fe0303b70] [c0000000003f2d40] __pci_register_driver+0x68/0x74
[0000033872128750] [c000001fe0303ba0] [c000000000478084] drm_pci_init+0x68/0x10c
[0000033872131108] [c000001fe0303c30] [c0000000010977fc] ast_init+0x3c/0x50
[0000033872133328] [c000001fe0303c50] [c00000000000ade4] do_one_initcall+0x134/0x1d0
[0000033872135374] [c000001fe0303d20] [c000000001073cac] kernel_init_freeable+0x198/0x254
[0000033872137484] [c000001fe0303dc0] [c00000000000b578] kernel_init+0x24/0x114
[0000033872139614] [c000001fe0303e30] [c0000000000094ac] ret_from_kernel_thread+0x5c/0xb0
[0000033872140422] Instruction dump:
[0000033872143730] f821ffe1 419e000c e92d0020 7d210164 39200000 992d0252 4bffff11 39400001
[0000033872146660] 994d0252 2fa30000 419e0010 4bff41c5 <60000000> 48000010 e92d0020 61298000
[0000090311985354] INFO: rcu_sched self-detected stall on CPU
[0000090311992284] 154: (8403 ticks this GP) idleâ5/140000000000002/0 softirq23/326 fqs„02
[0000090312003940] (t„03 jiffies g=-260 c=-261 qC21)
[0000090312007380] Task dump for CPU 154:
[0000090312011140] swapper/136 R running task 0 1 0 0x00000804
[0000090312011742] Call Trace:
[0000090312015782] [c000001fe0301ac0] [c00000000007fe98] sched_show_task+0x178/0x184 (unreliable)
[0000090312019000] [c000001fe0301b30] [c0000000000a7150] rcu_dump_cpu_stacks+0xa0/0xd0
[0000090312022140] [c000001fe0301b80] [c0000000000a9ff8] rcu_check_callbacks+0x294/0x734
[0000090312024654] [c000001fe0301ca0] [c0000000000ac2c4] update_process_times+0x3c/0x74
[0000090312027530] [c000001fe0301cd0] [c0000000000bb864] tick_sched_handle.isra.16+0x60/0x6c
[0000090312030168] [c000001fe0301d00] [c0000000000bb8bc] tick_sched_timer+0x4c/0x84
[0000090312032744] [c000001fe0301d40] [c0000000000acee4] __hrtimer_run_queues+0xf8/0x1ac
[0000090312035308] [c000001fe0301dd0] [c0000000000ad1b0] hrtimer_interrupt+0xac/0x1d8
[0000090312037774] [c000001fe0301e80] [c000000000017150] __timer_interrupt+0x7c/0xe0
[0000090312040240] [c000001fe0301ec0] [c000000000017314] timer_interrupt+0xac/0xc4
[0000090312042900] [c000001fe0301ef0] [c000000000002328] decrementer_common+0x128/0x180
[0000090312047390] --- interrupt: 901 at arch_local_irq_restore+0x5c/0x80
LR = arch_local_irq_restore+0x5c/0x80
[0000090312049800] [c000001fe03021e0] [0000000000000001] 0x1 (unreliable)
[0000090312053164] [c000001fe0302200] [c000000000b73314] _raw_spin_unlock_irqrestore+0x4c/0x78
[0000090312056214] [c000001fe0302230] [c0000000000abb84] mod_timer+0x174/0x184
[0000090312059170] [c000001fe0302290] [c000000000400cc4] cursor_timer_handler+0x58/0x64
[0000090312062098] [c000001fe03022c0] [c0000000000ab380] call_timer_fn+0x80/0xf4
[0000090312065090] [c000001fe0302370] [c0000000000ab5fc] run_timer_softirq+0x1dc/0x208
[0000090312067768] [c000001fe0302400] [c000000000059df0] __do_softirq+0x158/0x2b4
[0000090312070292] [c000001fe03024f0] [c00000000005a1fc] irq_exit+0x74/0xcc
[0000090312072792] [c000001fe0302510] [c000000000017318] timer_interrupt+0xb0/0xc4
[0000090312075484] [c000001fe0302540] [c000000000002328] decrementer_common+0x128/0x180
[0000090312079112] --- interrupt: 901 at _memset_io+0x5c/0x98
LR = ttm_bo_move_memcpy+0xb8/0x46c
[0000090312082140] [c000001fe0302830] [c000000000496140] ttm_bo_move_memcpy+0x68/0x46c (unreliable)
[0000090312085032] [c000001fe0302970] [c0000000005b4338] ast_bo_move+0x20/0x34
[0000090312087760] [c000001fe0302990] [c000000000494520] ttm_bo_handle_move_mem+0x230/0x3d8
[0000090312090228] [c000001fe0302ab0] [c000000000495454] ttm_bo_validate+0x20c/0x28c
[0000090312093072] [c000001fe0302b60] [c0000000005b4a58] ast_bo_pin+0x90/0xbc
[0000090312096396] [c000001fe0302b90] [c0000000005b276c] ast_crtc_do_set_base.isra.8.constprop.13+0x120/0x298
[0000090312099130] [c000001fe0302c40] [c0000000005b354c] ast_crtc_mode_set+0xc68/0xc74
[0000090312102302] [c000001fe0302cf0] [c00000000045faa4] drm_crtc_helper_set_mode+0x348/0x4d8
[0000090312105422] [c000001fe0302f80] [c000000000460420] drm_crtc_helper_set_config+0x6c4/0x944
[0000090312108648] [c000001fe03030a0] [c00000000047bbc0] drm_mode_set_config_internal+0x68/0xf0
[0000090312111410] [c000001fe03030e0] [c00000000046aa40] restore_fbdev_mode+0xe8/0x110
[0000090312114648] [c000001fe0303120] [c00000000046c7a4] drm_fb_helper_restore_fbdev_mode_unlocked+0x38/0x7c
[0000090312117378] [c000001fe0303160] [c00000000046c82c] drm_fb_helper_set_par+0x44/0x58
[0000090312120182] [c000001fe03031c0] [c000000000402af0] fbcon_init+0x360/0x474
[0000090312122640] [c000001fe03032b0] [c000000000443850] visual_init+0xec/0x13c
[0000090312125264] [c000001fe03032f0] [c00000000044551c] do_bind_con_driver+0x280/0x398
[0000090312127952] [c000001fe03033b0] [c000000000445a38] do_take_over_console+0x1dc/0x1ec
[0000090312130728] [c000001fe0303460] [c000000000402c98] do_fbcon_takeover+0x94/0x114
[0000090312133790] [c000001fe0303480] [c0000000000754dc] notifier_call_chain+0x6c/0xa8
[0000090312137046] [c000001fe03034d0] [c0000000000758c4] __blocking_notifier_call_chain+0x58/0x74
[0000090312140108] [c000001fe0303520] [c0000000004088e4] fb_notifier_call_chain+0x2c/0x40
[0000090312143188] [c000001fe0303540] [c00000000040b0f0] register_framebuffer+0x298/0x2d8
[0000090312146234] [c000001fe0303630] [c00000000046cb88] drm_fb_helper_initial_config+0x348/0x3f8
[0000090312149108] [c000001fe03036d0] [c0000000005b4108] ast_fbdev_init+0xc8/0x100
[0000090312151764] [c000001fe0303710] [c0000000005b1468] ast_driver_load+0x778/0x7b4
[0000090312154236] [c000001fe03037a0] [c000000000475dac] drm_dev_register+0xb0/0x14c
[0000090312156834] [c000001fe03037f0] [c000000000477f74] drm_get_pci_dev+0x114/0x1bc
[0000090312159422] [c000001fe0303880] [c0000000005b08b0] ast_pci_probe+0x20/0x34
[0000090312162084] [c000001fe03038a0] [c0000000003f3160] pci_device_probe+0x78/0xdc
[0000090312164940] [c000001fe0303920] [c0000000005bfc20] driver_probe_device+0x128/0x2b0
[0000090312167796] [c000001fe03039b0] [c0000000005bfe40] __driver_attach+0x98/0xc8
[0000090312170448] [c000001fe03039f0] [c0000000005bdbcc] bus_for_each_dev+0xa4/0xb8
[0000090312173108] [c000001fe0303a40] [c0000000005bf6e0] driver_attach+0x2c/0x40
[0000090312175774] [c000001fe0303a60] [c0000000005bf1b8] bus_add_driver+0x100/0x240
[0000090312178492] [c000001fe0303af0] [c0000000005c06cc] driver_register+0xc0/0x110
[0000090312181166] [c000001fe0303b70] [c0000000003f2d40] __pci_register_driver+0x68/0x74
[0000090312183802] [c000001fe0303ba0] [c000000000478084] drm_pci_init+0x68/0x10c
[0000090312186740] [c000001fe0303c30] [c0000000010977fc] ast_init+0x3c/0x50
[0000090312189740] [c000001fe0303c50] [c00000000000ade4] do_one_initcall+0x134/0x1d0
[0000090312192534] [c000001fe0303d20] [c000000001073cac] kernel_init_freeable+0x198/0x254
[0000090312195434] [c000001fe0303dc0] [c00000000000b578] kernel_init+0x24/0x114
[0000090312198324] [c000001fe0303e30] [c0000000000094ac] ret_from_kernel_thread+0x5c/0xb0
[0000153341979290] INFO: rcu_sched self-detected stall on CPU
[0000153341986890] 154: (14706 ticks this GP) idleâ5/140000000000002/0 softirq23/326 fqs\x14705
[0000153341998800] (t\x14706 jiffies g=-260 c=-261 qC21)
[0000153342001640] Task dump for CPU 154:
[0000153342005166] swapper/136 R running task 0 1 0 0x00000804
[0000153342005750] Call Trace:
[0000153342009706] [c000001fe0301ac0] [c00000000007fe98] sched_show_task+0x178/0x184 (unreliable)
[0000153342012772] [c000001fe0301b30] [c0000000000a7150] rcu_dump_cpu_stacks+0xa0/0xd0
[0000153342015820] [c000001fe0301b80] [c0000000000a9ff8] rcu_check_callbacks+0x294/0x734
[0000153342018586] [c000001fe0301ca0] [c0000000000ac2c4] update_process_times+0x3c/0x74
[0000153342021430] [c000001fe0301cd0] [c0000000000bb864] tick_sched_handle.isra.16+0x60/0x6c
[0000153342024078] [c000001fe0301d00] [c0000000000bb8bc] tick_sched_timer+0x4c/0x84
[0000153342026578] [c000001fe0301d40] [c0000000000acee4] __hrtimer_run_queues+0xf8/0x1ac
[0000153342029166] [c000001fe0301dd0] [c0000000000ad1b0] hrtimer_interrupt+0xac/0x1d8
[0000153342031606] [c000001fe0301e80] [c000000000017150] __timer_interrupt+0x7c/0xe0
[0000153342034098] [c000001fe0301ec0] [c000000000017314] timer_interrupt+0xac/0xc4
[0000153342036692] [c000001fe0301ef0] [c000000000002328] decrementer_common+0x128/0x180
[0000153342041096] --- interrupt: 901 at arch_local_irq_restore+0x5c/0x80
LR = arch_local_irq_restore+0x5c/0x80
[0000153342043432] [c000001fe03021e0] [0000000000000001] 0x1 (unreliable)
[0000153342046750] [c000001fe0302200] [c000000000b73314] _raw_spin_unlock_irqrestore+0x4c/0x78
[0000153342049764] [c000001fe0302230] [c0000000000abb84] mod_timer+0x174/0x184
[0000153342052754] [c000001fe0302290] [c000000000400cc4] cursor_timer_handler+0x58/0x64
[0000153342055840] [c000001fe03022c0] [c0000000000ab380] call_timer_fn+0x80/0xf4
[0000153342058916] [c000001fe0302370] [c0000000000ab5fc] run_timer_softirq+0x1dc/0x208
[0000153342061538] [c000001fe0302400] [c000000000059df0] __do_softirq+0x158/0x2b4
[0000153342064102] [c000001fe03024f0] [c00000000005a1fc] irq_exit+0x74/0xcc
[0000153342066602] [c000001fe0302510] [c000000000017318] timer_interrupt+0xb0/0xc4
[0000153342069238] [c000001fe0302540] [c000000000002328] decrementer_common+0x128/0x180
[0000153342072780] --- interrupt: 901 at _memset_io+0x5c/0x98
LR = ttm_bo_move_memcpy+0xb8/0x46c
[0000153342075870] [c000001fe0302830] [c000000000496140] ttm_bo_move_memcpy+0x68/0x46c (unreliable)
[0000153342078764] [c000001fe0302970] [c0000000005b4338] ast_bo_move+0x20/0x34
[0000153342081394] [c000001fe0302990] [c000000000494520] ttm_bo_handle_move_mem+0x230/0x3d8
[0000153342083828] [c000001fe0302ab0] [c000000000495454] ttm_bo_validate+0x20c/0x28c
[0000153342086694] [c000001fe0302b60] [c0000000005b4a58] ast_bo_pin+0x90/0xbc
[0000153342090058] [c000001fe0302b90] [c0000000005b276c] ast_crtc_do_set_base.isra.8.constprop.13+0x120/0x298
[0000153342092796] [c000001fe0302c40] [c0000000005b354c] ast_crtc_mode_set+0xc68/0xc74
[0000153342095946] [c000001fe0302cf0] [c00000000045faa4] drm_crtc_helper_set_mode+0x348/0x4d8
[0000153342098984] [c000001fe0302f80] [c000000000460420] drm_crtc_helper_set_config+0x6c4/0x944
[0000153342102108] [c000001fe03030a0] [c00000000047bbc0] drm_mode_set_config_internal+0x68/0xf0
[0000153342104910] [c000001fe03030e0] [c00000000046aa40] restore_fbdev_mode+0xe8/0x110
[0000153342108082] [c000001fe0303120] [c00000000046c7a4] drm_fb_helper_restore_fbdev_mode_unlocked+0x38/0x7c
[0000153342110756] [c000001fe0303160] [c00000000046c82c] drm_fb_helper_set_par+0x44/0x58
[0000153342113452] [c000001fe03031c0] [c000000000402af0] fbcon_init+0x360/0x474
[0000153342115912] [c000001fe03032b0] [c000000000443850] visual_init+0xec/0x13c
[0000153342118484] [c000001fe03032f0] [c00000000044551c] do_bind_con_driver+0x280/0x398
[0000153342121128] [c000001fe03033b0] [c000000000445a38] do_take_over_console+0x1dc/0x1ec
[0000153342124048] [c000001fe0303460] [c000000000402c98] do_fbcon_takeover+0x94/0x114
[0000153342127104] [c000001fe0303480] [c0000000000754dc] notifier_call_chain+0x6c/0xa8
[0000153342130412] [c000001fe03034d0] [c0000000000758c4] __blocking_notifier_call_chain+0x58/0x74
[0000153342133426] [c000001fe0303520] [c0000000004088e4] fb_notifier_call_chain+0x2c/0x40
[0000153342136474] [c000001fe0303540] [c00000000040b0f0] register_framebuffer+0x298/0x2d8
[0000153342139420] [c000001fe0303630] [c00000000046cb88] drm_fb_helper_initial_config+0x348/0x3f8
[0000153342142264] [c000001fe03036d0] [c0000000005b4108] ast_fbdev_init+0xc8/0x100
[0000153342145022] [c000001fe0303710] [c0000000005b1468] ast_driver_load+0x778/0x7b4
[0000153342147432] [c000001fe03037a0] [c000000000475dac] drm_dev_register+0xb0/0x14c
[0000153342150016] [c000001fe03037f0] [c000000000477f74] drm_get_pci_dev+0x114/0x1bc
[0000153342152666] [c000001fe0303880] [c0000000005b08b0] ast_pci_probe+0x20/0x34
[0000153342155376] [c000001fe03038a0] [c0000000003f3160] pci_device_probe+0x78/0xdc
[0000153342158140] [c000001fe0303920] [c0000000005bfc20] driver_probe_device+0x128/0x2b0
[0000153342160858] [c000001fe03039b0] [c0000000005bfe40] __driver_attach+0x98/0xc8
[0000153342163488] [c000001fe03039f0] [c0000000005bdbcc] bus_for_each_dev+0xa4/0xb8
[0000153342166180] [c000001fe0303a40] [c0000000005bf6e0] driver_attach+0x2c/0x40
[0000153342168832] [c000001fe0303a60] [c0000000005bf1b8] bus_add_driver+0x100/0x240
[0000153342171524] [c000001fe0303af0] [c0000000005c06cc] driver_register+0xc0/0x110
[0000153342174164] [c000001fe0303b70] [c0000000003f2d40] __pci_register_driver+0x68/0x74
[0000153342176734] [c000001fe0303ba0] [c000000000478084] drm_pci_init+0x68/0x10c
^ permalink raw reply related
* Re: [BUG] RCU stall in cursor_timer_handler
From: Scot Doyle @ 2015-10-03 5:12 UTC (permalink / raw)
To: Alistair Popple
Cc: linux-fbdev, Benjamin Herrenschmidt, plagnioj, linux-kernel,
airlied, Pavel Machek, Greg Kroah-Hartman
In-Reply-To: <2505354.BllTl4uaOg@new-mexico>
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?
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 related
* [PATCH] drivers/video/fbdev/i740fb: remove unused variable
From: Sudip Mukherjee @ 2015-10-03 11:28 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen
Cc: linux-kernel, linux-fbdev, Sudip Mukherjee
The value of d_best is always 0 and never changes.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/video/fbdev/i740fb.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/video/fbdev/i740fb.c b/drivers/video/fbdev/i740fb.c
index 452e116..cf5ccd0 100644
--- a/drivers/video/fbdev/i740fb.c
+++ b/drivers/video/fbdev/i740fb.c
@@ -346,11 +346,10 @@ static void i740_calc_vclk(u32 freq, struct i740fb_par *par)
const u32 err_target = freq / (1000 * I740_RFREQ / I740_FFIX);
u32 err_best = 512 * I740_FFIX;
u32 f_err, f_vco;
- int m_best = 0, n_best = 0, p_best = 0, d_best = 0;
+ int m_best = 0, n_best = 0, p_best = 0;
int m, n;
p_best = min(15, ilog2(I740_MAX_VCO_FREQ / (freq / I740_RFREQ_FIX)));
- d_best = 0;
f_vco = (freq * (1 << p_best)) / I740_RFREQ_FIX;
freq = freq / I740_RFREQ_FIX;
@@ -363,7 +362,7 @@ static void i740_calc_vclk(u32 freq, struct i740fb_par *par)
m = 3;
{
- u32 f_out = (((m * I740_REF_FREQ * (4 << 2 * d_best))
+ u32 f_out = (((m * I740_REF_FREQ * 4)
/ n) + ((1 << p_best) / 2)) / (1 << p_best);
f_err = (freq - f_out);
@@ -386,8 +385,7 @@ static void i740_calc_vclk(u32 freq, struct i740fb_par *par)
par->video_clk2_n = (n_best - 2) & 0xFF;
par->video_clk2_mn_msbs = ((((n_best - 2) >> 4) & VCO_N_MSBS)
| (((m_best - 2) >> 8) & VCO_M_MSBS));
- par->video_clk2_div_sel - ((p_best << 4) | (d_best ? 4 : 0) | REF_DIV_1);
+ par->video_clk2_div_sel = ((p_best << 4) | REF_DIV_1);
}
static int i740fb_decode_var(const struct fb_var_screeninfo *var,
--
1.9.1
^ permalink raw reply related
* [PATCH] video: fbdev: add Marvell PXA framebuffer binding
From: Robert Jarzmik @ 2015-10-03 16:11 UTC (permalink / raw)
To: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala
Cc: devicetree, linux-kernel, Robert Jarzmik,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev
Add documentation for the PXA frambuffer devicetree binding.
Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
---
.../devicetree/bindings/video/marvell,pxafb.txt | 75 ++++++++++++++++++++++
1 file changed, 75 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/marvell,pxafb.txt
diff --git a/Documentation/devicetree/bindings/video/marvell,pxafb.txt b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
new file mode 100644
index 000000000000..489055bf3c57
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
@@ -0,0 +1,75 @@
+PXA LCDC Framebuffer
+-----------------------------------------------------
+
+Required properties:
+- compatible :
+ "marvell,pxa2xx-fb",
+- 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.
+- interrupts : framebuffer controller interrupt
+- display: a phandle pointing to the display node
+
+Required nodes:
+- display: a display node is required to initialize the lcd panel
+ This should be in the board dts.
+- default-mode: a videomode within the display with timing parameters
+ as specified below.
+- bits-per-pixel: pixel data bus width of the LCD panel
+
+Optional properties:
+- lcd-supply: Regulator for LCD supply voltage.
+- enable-transparency-bit: if framebuffer colorspace reserves a bit for
+ transparency
+- enable-greyscale-cmap: true if palette is a grayscale based instead of color
+
+Example:
+
+ fb0: video@0x44000000 {
+ compatible = "marvell,pxa2xx-fb";
+ reg = <0x44000000 0x10000>;
+ interrupts = <17>;
+ clocks = <&clks CLK_LCD>;
+ interrupts = <23>;
+ display = <&display0>;
+ status = "okay";
+
+ enable-transparency-bit = <0>;
+ enable-greyscale-cmap = <0>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ };
+
+PXA LCDC Display
+-----------------------------------------------------
+Required properties (as per of_videomode_helper):
+ - lcd-type: either "mono-stn", "mono-dstn", "color-stn", "color-dstn",
+ "color-tft", "smart-panel"
+ - bits-per-pixel: LCD data bus width
+
+Optional properties (as per of_videomode_helper):
+ - power-regulator: power supply regulator to the LCD to power it on or off
+
+Example:
+ display0: display {
+ lcd-type = "color-tft";
+ bits-per-pixel = <16>;
+
+ display-timings {
+ native-mode = <&timing0>;
+ timing0: 240p {
+ /* 240x320p24 */
+ clock-frequency = <4545000>;
+ hactive = <240>;
+ vactive = <320>;
+ hfront-porch = <4>;
+ hback-porch = <6>;
+ hsync-len = <4>;
+ vback-porch = <5>;
+ vfront-porch = <3>;
+ vsync-len = <2>;
+ pixelclk-active = <0>;
+ de-active = <1>;
+ };
+ };
+ };
--
2.1.4
^ permalink raw reply related
* [PATCH 0/2] video: fbdev: pxafb: devicetree conversion
From: Robert Jarzmik @ 2015-10-03 16:11 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen
Cc: linux-fbdev, linux-kernel, Robert Jarzmik
This patchset aims at bringing support for the pxa framebuffer driver to a
devicetree pxa platform.
This was tested on a pxa27x platform, in both a devicetree build and a classic
platform data one.
Robert Jarzmik (2):
video: fbdev: pxafb: loosen the platform data bond
video: fbdev: pxafb: initial devicetree conversion
drivers/video/fbdev/Kconfig | 2 +
drivers/video/fbdev/pxafb.c | 212 +++++++++++++++++++++++++++++++++++++++-----
drivers/video/fbdev/pxafb.h | 2 +
3 files changed, 195 insertions(+), 21 deletions(-)
--
2.1.4
^ permalink raw reply
* [PATCH 1/2] video: fbdev: pxafb: loosen the platform data bond
From: Robert Jarzmik @ 2015-10-03 16:11 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen
Cc: linux-fbdev, linux-kernel, Robert Jarzmik
In-Reply-To: <1443888714-12362-1-git-send-email-robert.jarzmik@free.fr>
In order to prepare the transition to a mixed platform data and
device-tree initialization, remove all the platform data references all
over the driver.
Copy the platform data into the internal structure of the pxafb, and
only use this afterward.
Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
drivers/video/fbdev/pxafb.c | 54 ++++++++++++++++++++++++++++-----------------
drivers/video/fbdev/pxafb.h | 2 ++
2 files changed, 36 insertions(+), 20 deletions(-)
diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index 94813af97f09..ed4b1a5dc306 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -457,7 +457,7 @@ static int pxafb_adjust_timing(struct pxafb_info *fbi,
static int pxafb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
{
struct pxafb_info *fbi = container_of(info, struct pxafb_info, fb);
- struct pxafb_mach_info *inf = dev_get_platdata(fbi->dev);
+ struct pxafb_mach_info *inf = fbi->inf;
int err;
if (inf->fixed_modes) {
@@ -1230,7 +1230,7 @@ static unsigned int __smart_timing(unsigned time_ns, unsigned long lcd_clk)
static void setup_smart_timing(struct pxafb_info *fbi,
struct fb_var_screeninfo *var)
{
- struct pxafb_mach_info *inf = dev_get_platdata(fbi->dev);
+ struct pxafb_mach_info *inf = fbi->inf;
struct pxafb_mode_info *mode = &inf->modes[0];
unsigned long lclk = clk_get_rate(fbi->clk);
unsigned t1, t2, t3, t4;
@@ -1258,14 +1258,13 @@ static void setup_smart_timing(struct pxafb_info *fbi,
static int pxafb_smart_thread(void *arg)
{
struct pxafb_info *fbi = arg;
- struct pxafb_mach_info *inf = dev_get_platdata(fbi->dev);
+ struct pxafb_mach_info *inf = fbi->inf;
if (!inf->smart_update) {
pr_err("%s: not properly initialized, thread terminated\n",
__func__);
return -EINVAL;
}
- inf = dev_get_platdata(fbi->dev);
pr_debug("%s(): task starting\n", __func__);
@@ -1788,11 +1787,11 @@ decode_mode:
fbi->video_mem_size = video_mem_size;
}
-static struct pxafb_info *pxafb_init_fbinfo(struct device *dev)
+static struct pxafb_info *pxafb_init_fbinfo(struct device *dev,
+ struct pxafb_mach_info *inf)
{
struct pxafb_info *fbi;
void *addr;
- struct pxafb_mach_info *inf = dev_get_platdata(dev);
/* Alloc the pxafb_info and pseudo_palette in one step */
fbi = kmalloc(sizeof(struct pxafb_info) + sizeof(u32) * 16, GFP_KERNEL);
@@ -1801,6 +1800,7 @@ static struct pxafb_info *pxafb_init_fbinfo(struct device *dev)
memset(fbi, 0, sizeof(struct pxafb_info));
fbi->dev = dev;
+ fbi->inf = inf;
fbi->clk = clk_get(dev, NULL);
if (IS_ERR(fbi->clk)) {
@@ -1852,10 +1852,9 @@ static struct pxafb_info *pxafb_init_fbinfo(struct device *dev)
}
#ifdef CONFIG_FB_PXA_PARAMETERS
-static int parse_opt_mode(struct device *dev, const char *this_opt)
+static int parse_opt_mode(struct device *dev, const char *this_opt,
+ struct pxafb_mach_info *inf)
{
- struct pxafb_mach_info *inf = dev_get_platdata(dev);
-
const char *name = this_opt+5;
unsigned int namelen = strlen(name);
int res_specified = 0, bpp_specified = 0;
@@ -1911,9 +1910,9 @@ done:
return 0;
}
-static int parse_opt(struct device *dev, char *this_opt)
+static int parse_opt(struct device *dev, char *this_opt,
+ struct pxafb_mach_info *inf)
{
- struct pxafb_mach_info *inf = dev_get_platdata(dev);
struct pxafb_mode_info *mode = &inf->modes[0];
char s[64];
@@ -1922,7 +1921,7 @@ static int parse_opt(struct device *dev, char *this_opt)
if (!strncmp(this_opt, "vmem:", 5)) {
video_mem_size = memparse(this_opt + 5, NULL);
} else if (!strncmp(this_opt, "mode:", 5)) {
- return parse_opt_mode(dev, this_opt);
+ return parse_opt_mode(dev, this_opt, inf);
} else if (!strncmp(this_opt, "pixclock:", 9)) {
mode->pixclock = simple_strtoul(this_opt+9, NULL, 0);
sprintf(s, "pixclock: %ld\n", mode->pixclock);
@@ -2011,7 +2010,8 @@ static int parse_opt(struct device *dev, char *this_opt)
return 0;
}
-static int pxafb_parse_options(struct device *dev, char *options)
+static int pxafb_parse_options(struct device *dev, char *options,
+ struct pxafb_mach_info *inf)
{
char *this_opt;
int ret;
@@ -2023,7 +2023,7 @@ static int pxafb_parse_options(struct device *dev, char *options)
/* could be made table driven or similar?... */
while ((this_opt = strsep(&options, ",")) != NULL) {
- ret = parse_opt(dev, this_opt);
+ ret = parse_opt(dev, this_opt, inf);
if (ret)
return ret;
}
@@ -2095,19 +2095,33 @@ static void pxafb_check_options(struct device *dev, struct pxafb_mach_info *inf)
static int pxafb_probe(struct platform_device *dev)
{
struct pxafb_info *fbi;
- struct pxafb_mach_info *inf;
+ struct pxafb_mach_info *inf, *pdata;
struct resource *r;
- int irq, ret;
+ int i, irq, ret;
dev_dbg(&dev->dev, "pxafb_probe\n");
- inf = dev_get_platdata(&dev->dev);
ret = -ENOMEM;
- fbi = NULL;
+ pdata = dev_get_platdata(&dev->dev);
+ inf = devm_kmalloc(&dev->dev, sizeof(*inf), GFP_KERNEL);
if (!inf)
goto failed;
+ if (pdata) {
+ *inf = *pdata;
+ inf->modes + devm_kmalloc_array(&dev->dev, pdata->num_modes,
+ sizeof(inf->modes[0]), GFP_KERNEL);
+ if (!inf->modes)
+ goto failed;
+ for (i = 0; i < inf->num_modes; i++)
+ inf->modes[i] = pdata->modes[i];
+ }
+
+ fbi = NULL;
+ if (!pdata)
+ goto failed;
- ret = pxafb_parse_options(&dev->dev, g_options);
+ ret = pxafb_parse_options(&dev->dev, g_options, inf);
if (ret < 0)
goto failed;
@@ -2125,7 +2139,7 @@ static int pxafb_probe(struct platform_device *dev)
goto failed;
}
- fbi = pxafb_init_fbinfo(&dev->dev);
+ fbi = pxafb_init_fbinfo(&dev->dev, inf);
if (!fbi) {
/* only reason for pxafb_init_fbinfo to fail is kmalloc */
dev_err(&dev->dev, "Failed to initialize framebuffer device\n");
diff --git a/drivers/video/fbdev/pxafb.h b/drivers/video/fbdev/pxafb.h
index 26ba9fa3f737..5dc414e26fc8 100644
--- a/drivers/video/fbdev/pxafb.h
+++ b/drivers/video/fbdev/pxafb.h
@@ -167,6 +167,8 @@ struct pxafb_info {
void (*lcd_power)(int, struct fb_var_screeninfo *);
void (*backlight_power)(int);
+
+ struct pxafb_mach_info *inf;
};
#define TO_INF(ptr,member) container_of(ptr,struct pxafb_info,member)
--
2.1.4
^ permalink raw reply related
* [PATCH 2/2] video: fbdev: pxafb: initial devicetree conversion
From: Robert Jarzmik @ 2015-10-03 16:11 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen
Cc: linux-fbdev, linux-kernel, Robert Jarzmik
In-Reply-To: <1443888714-12362-1-git-send-email-robert.jarzmik@free.fr>
This patch brings a first support of pxa framebuffer devices to a
devicetree pxa platform, as was before platform data.
There are restrictions with this port, the biggest one being the lack of
support of smart panels. Moreover the conversion doesn't provide a way
to declare multiple framebuffer configurations with different bits per
pixel, only the LCD hardware bus width is used.
The patch was tested on both pxa25x, pxa27x and pxa3xx platform (namely
lubbock, mainstone and zylonite).
Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
drivers/video/fbdev/Kconfig | 2 +
drivers/video/fbdev/pxafb.c | 162 +++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 161 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 8b1d371b5404..1a24ca5a0624 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -1878,6 +1878,8 @@ config FB_PXA
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
+ select VIDEOMODE_HELPERS if OF
+ select FB_MODE_HELPERS if OF
---help---
Frame buffer driver for the built-in LCD controller in the Intel
PXA2x0 processor.
diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index ed4b1a5dc306..602622c56186 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -55,6 +55,8 @@
#include <linux/kthread.h>
#include <linux/freezer.h>
#include <linux/console.h>
+#include <video/of_display_timing.h>
+#include <video/videomode.h>
#include <mach/hardware.h>
#include <asm/io.h>
@@ -2092,6 +2094,152 @@ static void pxafb_check_options(struct device *dev, struct pxafb_mach_info *inf)
#define pxafb_check_options(...) do {} while (0)
#endif
+#if defined(CONFIG_OF)
+static const char * const lcd_types[] = {
+ "unknown", "mono-stn", "mono-dstn", "color-stn", "color-dstn",
+ "color-tft", "smart-panel", NULL
+};
+
+static int of_get_pxafb_display(struct device *dev, struct device_node *disp,
+ struct pxafb_mach_info *info)
+{
+ struct display_timings *timings;
+ struct videomode vm;
+ int i, ret = -EINVAL;
+ u32 bpp;
+ const char *s;
+
+ ret = of_property_read_u32(disp, "bits-per-pixel", &bpp);
+ if (ret) {
+ dev_err(dev, "no bits per pixel specified: %d\n", ret);
+ return ret;
+ }
+
+ ret = of_property_read_string(disp, "lcd-type", &s);
+ if (ret) {
+ dev_err(dev, "no lcd type: %d\n", ret);
+ return ret;
+ }
+
+ for (i = 0; lcd_types[i]; i++)
+ if (!strcmp(s, lcd_types[i]))
+ break;
+ if (!i || !lcd_types[i]) {
+ dev_err(dev, "lcd-type %s is unknown\n", s);
+ return -EINVAL;
+ }
+ info->lcd_conn |= LCD_CONN_TYPE(i);
+ info->lcd_conn |= LCD_CONN_WIDTH(bpp);
+
+ timings = of_get_display_timings(disp);
+ if (!timings)
+ goto out;
+
+ ret = -ENOMEM;
+ info->modes = kmalloc_array(timings->num_timings,
+ sizeof(info->modes[0]), GFP_KERNEL);
+ if (!info->modes)
+ goto out;
+ info->num_modes = timings->num_timings;
+
+ for (i = 0; i < timings->num_timings; i++) {
+ ret = videomode_from_timings(timings, &vm, i);
+ if (ret) {
+ dev_err(dev, "videomode_from_timings %d failed: %d\n",
+ i, ret);
+ goto out;
+ }
+ if (vm.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
+ info->lcd_conn |= LCD_PCLK_EDGE_RISE;
+ if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+ info->lcd_conn |= LCD_PCLK_EDGE_FALL;
+ if (vm.flags & DISPLAY_FLAGS_DE_HIGH)
+ info->lcd_conn |= LCD_BIAS_ACTIVE_HIGH;
+ if (vm.flags & DISPLAY_FLAGS_DE_LOW)
+ info->lcd_conn |= LCD_BIAS_ACTIVE_LOW;
+ if (vm.flags & DISPLAY_FLAGS_HSYNC_HIGH)
+ info->modes[i].sync |= FB_SYNC_HOR_HIGH_ACT;
+ if (vm.flags & DISPLAY_FLAGS_VSYNC_HIGH)
+ info->modes[i].sync |= FB_SYNC_VERT_HIGH_ACT;
+
+ info->modes[i].pixclock = 1000000000UL / (vm.pixelclock / 1000);
+ info->modes[i].xres = vm.hactive;
+ info->modes[i].yres = vm.vactive;
+ info->modes[i].hsync_len = vm.hsync_len;
+ info->modes[i].left_margin = vm.hback_porch;
+ info->modes[i].right_margin = vm.hfront_porch;
+ info->modes[i].vsync_len = vm.vsync_len;
+ info->modes[i].upper_margin = vm.vback_porch;
+ info->modes[i].lower_margin = vm.vfront_porch;
+ info->modes[i].bpp = bpp;
+ }
+ ret = 0;
+
+out:
+ display_timings_release(timings);
+ return ret;
+}
+
+static int of_get_pxafb_mode_info(struct device *dev,
+ struct pxafb_mach_info *info)
+{
+ struct device_node *display;
+ u32 depth = 0, transparency = 0;
+ int ret, i;
+
+ of_property_read_u32(dev->of_node, "depth", &depth);
+ of_property_read_u32(dev->of_node, "enable-transparency-bit",
+ &transparency);
+
+ display = of_parse_phandle(dev->of_node, "display", 0);
+ if (!display) {
+ dev_err(dev, "no display defined\n");
+ return -EINVAL;
+ }
+
+ ret = of_get_pxafb_display(dev, display, info);
+ of_node_put(display);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < info->num_modes; i++) {
+ info->modes[i].depth = depth;
+ info->modes[i].transparency = transparency;
+ }
+
+ return 0;
+}
+
+static struct pxafb_mach_info *of_pxafb_of_mach_info(struct device *dev)
+{
+ int ret;
+ struct pxafb_mach_info *info;
+
+ if (!dev->of_node)
+ return NULL;
+ info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return ERR_PTR(-ENOMEM);
+ ret = of_get_pxafb_mode_info(dev, info);
+ if (ret) {
+ kfree(info->modes);
+ return ERR_PTR(ret);
+ }
+
+ /*
+ * On purpose, neither lccrX registers nor video memory size can be
+ * specified through device-tree, they are considered more a debug hack
+ * available through command line.
+ */
+ return info;
+}
+#else
+static struct pxafb_mach_info *of_pxafb_of_mach_info(struct device *dev)
+{
+ return NULL;
+}
+#endif
+
static int pxafb_probe(struct platform_device *dev)
{
struct pxafb_info *fbi;
@@ -2104,8 +2252,7 @@ static int pxafb_probe(struct platform_device *dev)
ret = -ENOMEM;
pdata = dev_get_platdata(&dev->dev);
inf = devm_kmalloc(&dev->dev, sizeof(*inf), GFP_KERNEL);
- if (!inf)
- goto failed;
+
if (pdata) {
*inf = *pdata;
inf->modes @@ -2117,8 +2264,9 @@ static int pxafb_probe(struct platform_device *dev)
inf->modes[i] = pdata->modes[i];
}
- fbi = NULL;
if (!pdata)
+ inf = of_pxafb_of_mach_info(&dev->dev);
+ if (IS_ERR_OR_NULL(inf))
goto failed;
ret = pxafb_parse_options(&dev->dev, g_options, inf);
@@ -2313,11 +2461,19 @@ static int pxafb_remove(struct platform_device *dev)
return 0;
}
+static const struct of_device_id pxafb_of_dev_id[] = {
+ {
+ .compatible = "marvell,pxa2xx-fb",
+ },
+};
+MODULE_DEVICE_TABLE(of, pxafb_of_dev_id);
+
static struct platform_driver pxafb_driver = {
.probe = pxafb_probe,
.remove = pxafb_remove,
.driver = {
.name = "pxa2xx-fb",
+ .of_match_table = pxafb_of_dev_id,
#ifdef CONFIG_PM
.pm = &pxafb_pm_ops,
#endif
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 2/2] video: fbdev: pxafb: initial devicetree conversion
From: Philipp Zabel @ 2015-10-03 17:02 UTC (permalink / raw)
To: Robert Jarzmik
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev,
LKML
In-Reply-To: <1443888714-12362-3-git-send-email-robert.jarzmik@free.fr>
Hi Robert,
On Sat, Oct 3, 2015 at 6:11 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> This patch brings a first support of pxa framebuffer devices to a
> devicetree pxa platform, as was before platform data.
>
> There are restrictions with this port, the biggest one being the lack of
> support of smart panels. Moreover the conversion doesn't provide a way
> to declare multiple framebuffer configurations with different bits per
> pixel, only the LCD hardware bus width is used.
>
> The patch was tested on both pxa25x, pxa27x and pxa3xx platform (namely
> lubbock, mainstone and zylonite).
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
Thanks a lot for working on this! Out of interest, do you plan to
convert MIOA701 to DT?
[...]
> + of_property_read_u32(dev->of_node, "depth", &depth);
[...]
> + of_property_read_u32(dev->of_node, "enable-transparency-bit",
[...]
> + display = of_parse_phandle(dev->of_node, "display", 0);
[...]
> + ret = of_property_read_u32(disp, "bits-per-pixel", &bpp);
[...]
> + ret = of_property_read_string(disp, "lcd-type", &s);
[...]
> + timings = of_get_display_timings(disp);
These DT properties need some kind of binding documentation.
[...]
> @@ -2313,11 +2461,19 @@ static int pxafb_remove(struct platform_device *dev)
> return 0;
> }
>
> +static const struct of_device_id pxafb_of_dev_id[] = {
> + {
> + .compatible = "marvell,pxa2xx-fb",
At least in the old Intel manuals, this was called the LCD Controller,
all register names are LCsomething.
Please let's not just put the Linux driver name in the device tree and
call this pxa2xx-lcd-controller or a shortened version thereof.
best regards
Philipp
^ permalink raw reply
* Re: [PATCH 2/2] video: fbdev: pxafb: initial devicetree conversion
From: Robert Jarzmik @ 2015-10-03 17:08 UTC (permalink / raw)
To: Philipp Zabel
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev,
LKML
In-Reply-To: <CA+gwMceWchEhPQ0Prs5Ne+D+Fq1uybPkB48tRNWzj=i14TW8Yg@mail.gmail.com>
Philipp Zabel <philipp.zabel@gmail.com> writes:
> Hi Robert,
>
> On Sat, Oct 3, 2015 at 6:11 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>> This patch brings a first support of pxa framebuffer devices to a
>> devicetree pxa platform, as was before platform data.
>>
>> There are restrictions with this port, the biggest one being the lack of
>> support of smart panels. Moreover the conversion doesn't provide a way
>> to declare multiple framebuffer configurations with different bits per
>> pixel, only the LCD hardware bus width is used.
>>
>> The patch was tested on both pxa25x, pxa27x and pxa3xx platform (namely
>> lubbock, mainstone and zylonite).
>>
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>
> Thanks a lot for working on this! Out of interest, do you plan to
> convert MIOA701 to DT?
Actually, I already had. If you take all the pending patches scattered across
all the subsystems (around 40 by my last count), then mioa701 is converted, see
in [1]. If we take -next tree, I think the count will be closer to 20 or so.
>> @@ -2313,11 +2461,19 @@ static int pxafb_remove(struct platform_device *dev)
>> return 0;
>> }
>>
>> +static const struct of_device_id pxafb_of_dev_id[] = {
>> + {
>> + .compatible = "marvell,pxa2xx-fb",
>
> At least in the old Intel manuals, this was called the LCD Controller,
> all register names are LCsomething.
> Please let's not just put the Linux driver name in the device tree and
> call this pxa2xx-lcd-controller or a shortened version thereof.
Ok, I'm very open for a name change, devicetree is not my speciality, so
basically I'll take any advice :)
Cheers.
--
Robert
[1] mioa701 dt
/*
* Copyright (C) Robert Jarzmik <robert.jarzmik@free.fr>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* publishhed by the Free Software Foundation.
*/
/dts-v1/;
#include "pxa27x.dtsi"
#include "include/dt-bindings/gpio/gpio.h"
#include "include/dt-bindings/clock/pxa-clock.h"
/ {
model = "Mitac Mio A701 Board";
/* compatible = "mitac,mioa701"; */
compatible = "marvell,pxa270";
chosen {
bootargs = "mtdparts=docg3.0:256k@3456k(barebox)ro,256k(barebox-logo),128k(barebox-env),4M(kernel),-(root) ubi.mtd=4 rootfstype=ubifs root=ubi0:linux_root ro";
};
memory {
reg = <0xa0000000 0x04000000>;
reserved-memory {
#address-cells = <1>;
#size-cells = <1>;
pstore_region:region@0xa2000000 {
compatible = "linux,contiguous-memory-region";
reg = <0xa2000000 1048576>;
};
};
};
cpus {
cpu {
cpu-supply = <&vcc_core>;
};
};
pxabus {
gpio: gpio@40e00000 {
status = "okay";
};
ffuart: uart@40100000 {
status = "okay";
};
btuart: uart@40200000 {
status = "okay";
};
stuart: uart@40700000 {
status = "okay";
};
usb2phy: gpio-vbus@13 {
compatible = "usb-nop-xceiv";
vbus-detect-gpio = <&gpio 13 GPIO_ACTIVE_LOW>;
wakeup;
};
pxa27x_udc: udc@40600000 {
status = "okay";
gpios = <&gpio 22 0>;
phys = <&usb2phy>;
phys-names = "usb2phy";
};
pwri2c: i2c@40f000180 {
status = "okay";
max1586@14 {
compatible = "maxim,max1586";
reg = <0x14>;
v3-gain = <1000000>;
regulators {
vcc_core: v3 {
regulator-name = "vcc_core";
regulator-compatible = "Output_V3";
regulator-min-microvolt = <1000000>;
regulator-max-microvolt = <1705000>;
regulator-always-on;
};
};
};
};
pxai2c1: i2c@40301680 {
mrvl,i2c-fast-mode;
status = "okay";
mt9m111: camera@5d {
compatible = "micron,mt9m111";
reg = <0x5d>;
gpios = <&gpio 56 GPIO_ACTIVE_HIGH>;
remote = <&pxa_camera>;
port {
mt9m111_1: endpoint {
bus-width = <8>;
remote-endpoint = <&pxa_camera>;
};
};
};
};
keypad: keypad@41500000 {
status = "okay";
keypad,num-rows = <3>;
keypad,num-columns = <3>;
linux,keymap = <
0x00000067 /* KEY_UP */
0x0001006a /* KEY_RIGHT */
0x000200e2 /* KEY_MEDIA */
0x0100006c /* KEY_DOWN */
0x0101001c /* KEY_ENTER */
0x010200da /* KEY_CONNECT */
0x02000069 /* KEY_LEFT */
0x020100a9 /* KEY_PHONE */
0x020200d4>; /* KEY_CAMERA */
marvell,debounce-interval = <0>;
};
gpio-keys {
compatible = "gpio-keys";
#address-cells = <1>;
#size-cells = <0>;
autorepeat;
status = "okay";
button@0 {
label = "GPIO Key Power";
linux,code = <174>;
gpios = <&gpio 0 0>;
gpio-key,wakeup;
};
button@12 {
label = "HP jack detect";
linux,code = <211>;
gpios = <&gpio 12 0>;
};
button@93 {
label = "Volume Up Key";
linux,code = <115>;
gpios = <&gpio 93 0>;
};
button@94 {
label = "Volume Down Key";
linux,code = <114>;
gpios = <&gpio 94 0>;
};
};
mmc0: mmc@41100000 {
vmmc-supply = <®_vmmc>;
status = "okay";
};
pxa_camera: imaging@50000000 {
status = "okay";
port {
#address-cells = <1>;
#size-cells = <0>;
/* Parallel bus endpoint */
qci: endpoint@0 {
reg = <0>; /* Local endpoint # */
remote-endpoint = <&mt9m111_1>; /* Remote phandle */
bus-width = <8>; /* Used data lines */
hsync-active = <0>; /* Active low */
vsync-active = <0>; /* Active low */
pclk-sample = <1>; /* Rising */
};
};
};
pxafb: video@40500000 {
status = "okay";
display = <&display0>;
enable-transparency-bit = <0>;
enable-greyscale-cmap = <0>;
};
};
regulators {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <0>;
reg_vmmc: regulator@0 {
compatible = "regulator-fixed";
regulator-name = "vmmc";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
regulator-always-on;
};
};
backlight {
compatible = "pwm-backlight";
pwms = <&pwm0 0 40960000>;
pwm-names = "backlight";
brightness-levels = <0 4 8 16 32 64 128 255>;
default-brightness-level = <2>;
};
docg3: flash@0 {
compatible = "m-systems,diskonchip-g3";
reg = <0x0 0x2000>;
};
display0: display@0 {
lcd-type = "color-tft";
bits-per-pixel = <16>;
display-timings {
native-mode = <&timing0>;
timing0: 240p {
/* 240x320p24 */
clock-frequency = <4545000>;
hactive = <240>;
vactive = <320>;
hfront-porch = <4>;
hback-porch = <6>;
hsync-len = <4>;
vback-porch = <5>;
vfront-porch = <3>;
vsync-len = <2>;
};
};
};
};
^ permalink raw reply
* Re: [PATCH] video: fbdev: add Marvell PXA framebuffer binding
From: Philipp Zabel @ 2015-10-03 17:14 UTC (permalink / raw)
To: Robert Jarzmik
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
devicetree, LKML, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev
In-Reply-To: <1443888694-12311-1-git-send-email-robert.jarzmik@free.fr>
On Sat, Oct 3, 2015 at 6:11 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Add documentation for the PXA frambuffer devicetree binding.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: linux-fbdev@vger.kernel.org
>
> ---
> .../devicetree/bindings/video/marvell,pxafb.txt | 75 ++++++++++++++++++++++
> 1 file changed, 75 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/video/marvell,pxafb.txt
>
> diff --git a/Documentation/devicetree/bindings/video/marvell,pxafb.txt b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
> new file mode 100644
> index 000000000000..489055bf3c57
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
> @@ -0,0 +1,75 @@
> +PXA LCDC Framebuffer
> +-----------------------------------------------------
> +
> +Required properties:
> +- compatible :
> + "marvell,pxa2xx-fb",
Should be "marvell,pxa2xx-lcd-controller", "marvell,pxa2xx-lcdc" or
something like this.
> +- 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.
> +- interrupts : framebuffer controller interrupt
> +- display: a phandle pointing to the display node
> +
> +Required nodes:
> +- display: a display node is required to initialize the lcd panel
> + This should be in the board dts.
I'd prefer to use an of-graph link to a panel node with a proper
compatible value for the panel, instead of this custom display
property.
That way, if somebody ever decides convert the fbdev driver to a drm
driver, we don't have to change the device tree and can directly use
drm_panel.
> +- default-mode: a videomode within the display with timing parameters
> + as specified below.
> +- bits-per-pixel: pixel data bus width of the LCD panel
Would bus-width be better here?
> +Optional properties:
> +- lcd-supply: Regulator for LCD supply voltage.
How does this differ from the regulator below?
> +- enable-transparency-bit: if framebuffer colorspace reserves a bit for
> + transparency
That doesn't belong in the device tree.
> +- enable-greyscale-cmap: true if palette is a grayscale based instead of color
I suspect this doesn't belong in the device tree either. Does this
specify the pixel format of the memory framebuffer?
> +Example:
> +
> + fb0: video@0x44000000 {
> + compatible = "marvell,pxa2xx-fb";
> + reg = <0x44000000 0x10000>;
> + interrupts = <17>;
> + clocks = <&clks CLK_LCD>;
> + interrupts = <23>;
> + display = <&display0>;
> + status = "okay";
> +
> + enable-transparency-bit = <0>;
> + enable-greyscale-cmap = <0>;
> + #address-cells = <1>;
> + #size-cells = <1>;
What are the #address/size-cells needed for?
> + };
> +
> +PXA LCDC Display
> +-----------------------------------------------------
> +Required properties (as per of_videomode_helper):
> + - lcd-type: either "mono-stn", "mono-dstn", "color-stn", "color-dstn",
> + "color-tft", "smart-panel"
> + - bits-per-pixel: LCD data bus width
This is already found in the lcd controller node above.
> +Optional properties (as per of_videomode_helper):
> + - power-regulator: power supply regulator to the LCD to power it on or off
regards
Philipp
^ permalink raw reply
* Re: [PATCH] video: fbdev: add Marvell PXA framebuffer binding
From: Robert Jarzmik @ 2015-10-03 17:23 UTC (permalink / raw)
To: Philipp Zabel
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
devicetree-u79uwXL29TY76Z2rM5mHXA, LKML,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CA+gwMceq-+8UX4J7zgMG_DcGYE1RhmiHhKig8i7MVwga53LQRw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Philipp Zabel <philipp.zabel@gmail.com> writes:
> On Sat, Oct 3, 2015 at 6:11 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>> Add documentation for the PXA frambuffer devicetree binding.
>>
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: linux-fbdev@vger.kernel.org
>>
>> ---
>> .../devicetree/bindings/video/marvell,pxafb.txt | 75 ++++++++++++++++++++++
>> 1 file changed, 75 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/video/marvell,pxafb.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/marvell,pxafb.txt b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
>> new file mode 100644
>> index 000000000000..489055bf3c57
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
>> @@ -0,0 +1,75 @@
>> +PXA LCDC Framebuffer
>> +-----------------------------------------------------
>> +
>> +Required properties:
>> +- compatible :
>> + "marvell,pxa2xx-fb",
>
> Should be "marvell,pxa2xx-lcd-controller", "marvell,pxa2xx-lcdc" or
> something like this.
Whichever you see fit.
>
>> +- 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.
>> +- interrupts : framebuffer controller interrupt
>> +- display: a phandle pointing to the display node
>> +
>> +Required nodes:
>> +- display: a display node is required to initialize the lcd panel
>> + This should be in the board dts.
>
> I'd prefer to use an of-graph link to a panel node with a proper
> compatible value for the panel, instead of this custom display
> property.
> That way, if somebody ever decides convert the fbdev driver to a drm
> driver, we don't have to change the device tree and can directly use
> drm_panel.
Ok, if you give me an example it would be easier for me.
>> +- default-mode: a videomode within the display with timing parameters
>> + as specified below.
>> +- bits-per-pixel: pixel data bus width of the LCD panel
>
> Would bus-width be better here?
bus-width yes, but I think I should remove this property, and only keep the one
in the panel/display.
>> +Optional properties:
>> +- lcd-supply: Regulator for LCD supply voltage.
>
> How does this differ from the regulator below?
Ah yes, good point. In the end I couldn't decide which one was the correct one
... My feeling is that it's the display's one, as hardware wise the power is
necessary for the display, not the framebuffer.
>
>> +- enable-transparency-bit: if framebuffer colorspace reserves a bit for
>> + transparency
>
> That doesn't belong in the device tree.
>
>> +- enable-greyscale-cmap: true if palette is a grayscale based instead of color
>
> I suspect this doesn't belong in the device tree either. Does this
> specify the pixel format of the memory framebuffer?
Yes, both these values specify the pixel format. I was thinking this was a
hardware capability of the IP, I was wrong, just cross-checked. I'll remove
these 2 properties.
>> + enable-transparency-bit = <0>;
>> + enable-greyscale-cmap = <0>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>
> What are the #address/size-cells needed for?
Copy-paste from another binding, atmel's I think. Poor leftover obviously.
>> + };
>> +
>> +PXA LCDC Display
>> +-----------------------------------------------------
>> +Required properties (as per of_videomode_helper):
>> + - lcd-type: either "mono-stn", "mono-dstn", "color-stn", "color-dstn",
>> + "color-tft", "smart-panel"
>> + - bits-per-pixel: LCD data bus width
>
> This is already found in the lcd controller node above.
I think the bus-width should be here. It represents the number of data lines
between the SoC and the panel.
Cheers.
--
Robert
^ permalink raw reply
* Re: [PATCH 2/2] video: fbdev: pxafb: initial devicetree conversion
From: Philipp Zabel @ 2015-10-03 18:05 UTC (permalink / raw)
To: Robert Jarzmik
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev,
LKML
In-Reply-To: <87k2r3dgmj.fsf@belgarion.home>
Am Samstag, den 03.10.2015, 19:08 +0200 schrieb Robert Jarzmik:
> > Thanks a lot for working on this! Out of interest, do you plan to
> > convert MIOA701 to DT?
> Actually, I already had. If you take all the pending patches
> scattered across
> all the subsystems (around 40 by my last count), then mioa701 is
> converted, see
> in [1]. If we take -next tree, I think the count will be closer to 20
> or so.
This is amazing. I'm looking forward to follow your good example with
magician and hx4700 once that hits mainline.
regards
Philipp
^ permalink raw reply
* Re: [PATCH] video: fbdev: add Marvell PXA framebuffer binding
From: Philipp Zabel @ 2015-10-03 18:05 UTC (permalink / raw)
To: Robert Jarzmik
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
devicetree, LKML, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev
In-Reply-To: <87fv1rdfxb.fsf@belgarion.home>
Am Samstag, den 03.10.2015, 19:23 +0200 schrieb Robert Jarzmik:
[...]
> a/Documentation/devicetree/bindings/video/marvell,pxafb.txt
> > > b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
> > > new file mode 100644
> > > index 000000000000..489055bf3c57
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
> > > @@ -0,0 +1,75 @@
> > > +PXA LCDC Framebuffer
> > > +-----------------------------------------------------
> > > +
> > > +Required properties:
> > > +- compatible :
> > > + "marvell,pxa2xx-fb",
> >
> > Should be "marvell,pxa2xx-lcd-controller", "marvell,pxa2xx-lcdc" or
> > something like this.
> Whichever you see fit.
Personally, I like the lcdc one better, even though it is an arbitrary
abbreviation of the text found in the manual.
> > > +- 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.
> > > +- interrupts : framebuffer controller interrupt
> > > +- display: a phandle pointing to the display node
> > > +
> > > +Required nodes:
> > > +- display: a display node is required to initialize the lcd
> > > panel
> > > + This should be in the board dts.
> >
> > I'd prefer to use an of-graph link to a panel node with a proper
> > compatible value for the panel, instead of this custom display
> > property.
> > That way, if somebody ever decides convert the fbdev driver to a
> > drm
> > driver, we don't have to change the device tree and can directly
> > use
> > drm_panel.
> Ok, if you give me an example it would be easier for me.
Have a look at the of-graph connection between capture interface and
sensor (QCI and MT9M111) in your example below. The connection between
LCD controller and panel should look similar:
pxabus {
lcd-controller@40500000 {
compatible = "marvell,pxa2xx-lcdc";
/* ... */
port {
lcdc_out: endpoint {
remote-endpoint = <&panel_in>;
bus-width = <16>;
};
};
};
};
panel {
compatible = "toshiba,ltm0305a776";
lcd-type = "color-tft";
power-supply = <&lcd_supply>;
backlight = <&lcd_backlight>;
port {
panel_in: endpoint {
remote-endpoint = <&lcdc_out>;
};
};
}
The bus-width could be made a property of the lcdc_out endpoint for
symmetry with the QCI binding, and as documented in
Documentation/devicetree/bindings/media/video-interfaces.txt
If you later bind a drm_panel driver to the panel node, it can look up
that information (and the timings) just from the compatible string.
[...]
> > > +Optional properties:
> > > +- lcd-supply: Regulator for LCD supply voltage.
> >
> > How does this differ from the regulator below?
> Ah yes, good point. In the end I couldn't decide which one was the
> correct one
> ... My feeling is that it's the display's one, as hardware wise the
> power is
> necessary for the display, not the framebuffer.
Then I'd suggest a power-supply property in the panel node, as is
already documented for simple panels:
Documentation/devicetree/bindings/panel/simple-panel.txt
> > > +PXA LCDC Display
> > > +-----------------------------------------------------
> > > +Required properties (as per of_videomode_helper):
> > > + - lcd-type: either "mono-stn", "mono-dstn", "color-stn", "color
> > > -dstn",
> > > + "color-tft", "smart-panel"
> > > + - bits-per-pixel: LCD data bus width
> >
> > This is already found in the lcd controller node above.
> I think the bus-width should be here. It represents the number of
> data lines
> between the SoC and the panel.
With the of-graph, it can be argued that this is a property of both
endpoints of the bus (imagine an 18-bit panel driven by a 16-bit LCD
controller with some funny wiring).
regards
Philipp
^ permalink raw reply
* [PATCH v2] video: fbdev: add Marvell PXA framebuffer binding
From: Robert Jarzmik @ 2015-10-04 10:31 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 frambuffer devicetree binding.
Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
Since v1: Philipp's review on the whole binding
---
.../devicetree/bindings/video/marvell,pxafb.txt | 80 ++++++++++++++++++++++
1 file changed, 80 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/marvell,pxafb.txt
diff --git a/Documentation/devicetree/bindings/video/marvell,pxafb.txt b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
new file mode 100644
index 000000000000..4d6bd490680d
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/marvell,pxafb.txt
@@ -0,0 +1,80 @@
+PXA LCDC Framebuffer
+--------------------
+
+Required properties:
+ - compatible :
+ "marvell,pxa2xx-lcdc",
+ - 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.
+ - interrupts : framebuffer controller interrupt
+
+Required nodes:
+ - clocks: phandle to input clocks.
+ - port: connection to the LCD panel (see video-interfaces.txt)
+ This nodes must have its properties bus-width and remote-endpoint set.
+ This should be in the board dts.
+
+Example:
+
+ lcd-controller@40500000 {
+ compatible = "marvell,pxa2xx-lcdc";
+ reg = <0x44000000 0x10000>;
+ interrupts = <17>;
+ clocks = <&clks CLK_LCD>;
+ interrupts = <23>;
+ status = "okay";
+
+ port {
+ lcdc_out: endpoint {
+ remote-endpoint = <&panel_in>;
+ bus-width = <16>;
+ };
+ };
+ };
+
+PXA LCDC Display
+----------------
+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)
+
+Example:
+ panel {
+ compatible = "toshiba,ltm0305a776";
+ lcd-type = "color-tft";
+
+ power-supply = <&lcd_supply>;
+ backlight = <&lcd_backlight>;
+
+ port {
+ panel_in: endpoint {
+ remote-endpoint = <&lcdc_out>;
+ };
+ };
+
+ display-timings {
+ native-mode = <&timing0>;
+ timing0: 240p {
+ /* 240x320p24 */
+ clock-frequency = <4545000>;
+ hactive = <240>;
+ vactive = <320>;
+ hfront-porch = <4>;
+ hback-porch = <6>;
+ hsync-len = <4>;
+ vback-porch = <5>;
+ vfront-porch = <3>;
+ vsync-len = <2>;
+ };
+ };
+ };
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] Revert "backlight: pwm: Handle EPROBE_DEFER while requesting the PWM"
From: Thierry Reding @ 2015-10-05 9:35 UTC (permalink / raw)
To: Nicolas Ferre
Cc: Robert Jarzmik, linux-pwm, Boris BREZILLON, Alexandre Belloni,
Jingoo Han, Lee Jones, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
In-Reply-To: <560CE816.1060307@atmel.com>
[-- Attachment #1: Type: text/plain, Size: 3616 bytes --]
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.
I'm afraid that fixing up the legacy platforms to use PWM lookup tables
will be the only proper fix that doesn't risk breaking everyone else. I
sent out patches to do that for PXA a couple of minutes ago. Looking at
the history of the pwm-pxa driver it seems like quite a few PXA boards
must have been broken ever since v3.6 because of the PWM ID assignment
mismatch, and those should be fixed with the patches I sent as well.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] Revert "backlight: pwm: Handle EPROBE_DEFER while requesting the PWM"
From: Boris Brezillon @ 2015-10-05 11:19 UTC (permalink / raw)
To: 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: <20151005093543.GB30219@ulmo>
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.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH] backlight: pwm: reject legacy pwm request for device defined in dt
From: Lee Jones @ 2015-10-05 12:46 UTC (permalink / raw)
To: Vladimir Zapolskiy; +Cc: Jingoo Han, Thierry Reding, linux-pwm, linux-fbdev
In-Reply-To: <20150921232225.GF11284@x1>
On Tue, 22 Sep 2015, Lee Jones wrote:
> On Sun, 14 Jun 2015, Vladimir Zapolskiy wrote:
>
> > 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>
> > ---
> > drivers/video/backlight/pwm_bl.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
>
> Applied, thanks.
Slight change of plan, as the patch does not apply.
Please rebase it on top of the backlight tree and resubmit with my
Ack.
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index 57cb9ec..9991cdb 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -271,15 +271,16 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > }
> >
> > pb->pwm = devm_pwm_get(&pdev->dev, NULL);
> > - if (IS_ERR(pb->pwm)) {
> > + if (IS_ERR(pb->pwm) && !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)) {
> > + dev_err(&pdev->dev, "unable to request PWM\n");
> > + ret = PTR_ERR(pb->pwm);
> > + goto err_alloc;
> > }
> >
> > dev_dbg(&pdev->dev, "got pwm for backlight\n");
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] Revert "backlight: pwm: Handle EPROBE_DEFER while requesting the PWM"
From: Thierry Reding @ 2015-10-05 12:58 UTC (permalink / raw)
To: Boris Brezillon
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: <20151005131912.4da38169@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 4225 bytes --]
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/
I think that conflicts with Nicolas' -EPROBE_DEFER patch, but I think
reverting Nicolas' patch and then applying the above on top might fix
this nicely for everybody.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] Revert "backlight: pwm: Handle EPROBE_DEFER while requesting the PWM"
From: Boris Brezillon @ 2015-10-05 13:30 UTC (permalink / raw)
To: 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: <20151005125803.GA30025@ulmo>
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: 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
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH] Revert "backlight: pwm: Handle EPROBE_DEFER while requesting the PWM"
From: Thierry Reding @ 2015-10-05 14:07 UTC (permalink / raw)
To: Boris Brezillon
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>
[-- Attachment #1: Type: text/plain, Size: 6483 bytes --]
On Mon, Oct 05, 2015 at 03:30:24PM +0200, 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: 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).
Right, the non-DT, slightly less legacy case...
> 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
Haha, I came up with exactly this earlier and I've been trying to think
of ways in which it could potentially break.
Thierry
--- >8 ---
From f7fee34e0c414b4268c59e97937c51e0c91a74cf Mon Sep 17 00:00:00 2001
From: Thierry Reding <thierry.reding@gmail.com>
Date: Mon, 5 Oct 2015 14:38:32 +0200
Subject: [PATCH] pwm: Return -ENODEV if no PWM lookup match is found
When looking up a PWM using the lookup table, assume that all entries
will have been added already, so failure to find a match means that no
corresponding entry has been registered.
This fixes an issue where -EPROBE_DEFER would be returned if the PWM
lookup table is empty. After this fix, -EPROBE_DEFER is reserved for
situations where no provider has yet registered for a matching entry.
Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
drivers/pwm/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3f9df3ea3350..94e5af123660 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -719,8 +719,10 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
}
}
- if (!chosen)
+ if (!chosen) {
+ pwm = ERR_PTR(-ENODEV);
goto out;
+ }
chip = pwmchip_find_by_name(chosen->provider);
if (!chip)
--
2.5.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply related
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