* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-18 13:48 UTC (permalink / raw)
To: Alan Stern; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <Pine.LNX.4.44L0.1208180913440.17394-100000@netrider.rowland.org>
On Sat, 18 August 2012 Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 18 Aug 2012, Bruno Prémont wrote:
> > On Thu, 16 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> > > On Thu, 16 Aug 2012, Bruno Prémont wrote:
> > >
> > > > > I don't really understand this explanation. Once usb_kill_urb() returns,
> > > > > the URB should be available for future use (and therefore all queues
> > > > > completely drained).
> > > >
> > > > I won't have time today to check, though my guess is that on each
> > > > echo $usb-id > bind; echo $usb-id > unbind
> > > > under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
> > > > not get cleared.
> > > >
> > > > Is usb_kill_urb() called when unbinding just the specific hid driver?
> > >
> > > Yup, through hid_hw_stop() -> usbhid_stop().
> > >
> > > > If so my short timing between bind/unbind must be triggering something
> > > > else...
> > > >
> > > > Otherwise I'm missing something as at first time I got no "output queue full"
> > > > messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
> > > > iteration increased in number.
> > > >
> > > > Anyhow, on Friday evening/week-end I will continue digging and report back with my
> > > > findings.
> >
> > Huh, after changing some of the hid-picolcd data in order to have less racy
> > coupling between hid and framebuffer I'm now dying way too often in _mmx_memcpy
> > and most of the time I don't get a (complete) trace...
>
> There was a similar problem reported recently. It turned out to be
> caused by a __devinitconst annotation attached to a usb_device_id
> table.
>
> If there are any __devinit* annotations in the hid-picolcd driver, you
> should see if removing them helps.
There is no such annotation around in hid-picolcd.
One thing I just though about, how does usbhid handle the calls to
usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
been called?
I will attempt to see if it makes a difference to shortcut my
usbhid_submit_report() calls from the point on I have called hid_hw_close()...
Bruno
^ permalink raw reply
* [PATCH] fbdev/amifb: Remove write-only variable amifb_inverse
From: Geert Uytterhoeven @ 2012-08-18 13:30 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: linux-fbdev, linux-m68k, Geert Uytterhoeven
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
drivers/video/amifb.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/video/amifb.c b/drivers/video/amifb.c
index 887df9d..7fa1bf8 100644
--- a/drivers/video/amifb.c
+++ b/drivers/video/amifb.c
@@ -949,7 +949,6 @@ static int round_down_bpp = 1; /* for mode probing */
static int amifb_ilbm = 0; /* interleaved or normal bitplanes */
-static int amifb_inverse = 0;
static u32 amifb_hfmin __initdata; /* monitor hfreq lower limit (Hz) */
static u32 amifb_hfmax __initdata; /* monitor hfreq upper limit (Hz) */
@@ -2355,7 +2354,6 @@ static int __init amifb_setup(char *options)
if (!*this_opt)
continue;
if (!strcmp(this_opt, "inverse")) {
- amifb_inverse = 1;
fb_invert_cmaps();
} else if (!strcmp(this_opt, "ilbm"))
amifb_ilbm = 1;
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 0/7] HID: picoLCD updates
From: Alan Stern @ 2012-08-18 13:19 UTC (permalink / raw)
To: Bruno Prémont; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120818144039.0356cc7c@neptune.home>
On Sat, 18 Aug 2012, Bruno Prémont wrote:
> Hi Jiri,
>
> [CCing Alan Stern]
>
> On Thu, 16 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> > On Thu, 16 Aug 2012, Bruno Prémont wrote:
> >
> > > > I don't really understand this explanation. Once usb_kill_urb() returns,
> > > > the URB should be available for future use (and therefore all queues
> > > > completely drained).
> > >
> > > I won't have time today to check, though my guess is that on each
> > > echo $usb-id > bind; echo $usb-id > unbind
> > > under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
> > > not get cleared.
> > >
> > > Is usb_kill_urb() called when unbinding just the specific hid driver?
> >
> > Yup, through hid_hw_stop() -> usbhid_stop().
> >
> > > If so my short timing between bind/unbind must be triggering something
> > > else...
> > >
> > > Otherwise I'm missing something as at first time I got no "output queue full"
> > > messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
> > > iteration increased in number.
> > >
> > > Anyhow, on Friday evening/week-end I will continue digging and report back with my
> > > findings.
>
> Huh, after changing some of the hid-picolcd data in order to have less racy
> coupling between hid and framebuffer I'm now dying way too often in _mmx_memcpy
> and most of the time I don't get a (complete) trace...
There was a similar problem reported recently. It turned out to be
caused by a __devinitconst annotation attached to a usb_device_id
table.
If there are any __devinit* annotations in the hid-picolcd driver, you
should see if removing them helps.
Alan Stern
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-18 12:40 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev, Alan Stern
In-Reply-To: <alpine.LNX.2.00.1208161846240.28475@pobox.suse.cz>
[-- Attachment #1: Type: text/plain, Size: 9570 bytes --]
Hi Jiri,
[CCing Alan Stern]
On Thu, 16 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> On Thu, 16 Aug 2012, Bruno Prémont wrote:
>
> > > I don't really understand this explanation. Once usb_kill_urb() returns,
> > > the URB should be available for future use (and therefore all queues
> > > completely drained).
> >
> > I won't have time today to check, though my guess is that on each
> > echo $usb-id > bind; echo $usb-id > unbind
> > under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
> > not get cleared.
> >
> > Is usb_kill_urb() called when unbinding just the specific hid driver?
>
> Yup, through hid_hw_stop() -> usbhid_stop().
>
> > If so my short timing between bind/unbind must be triggering something
> > else...
> >
> > Otherwise I'm missing something as at first time I got no "output queue full"
> > messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
> > iteration increased in number.
> >
> > Anyhow, on Friday evening/week-end I will continue digging and report back with my
> > findings.
Huh, after changing some of the hid-picolcd data in order to have less racy
coupling between hid and framebuffer I'm now dying way too often in _mmx_memcpy
and most of the time I don't get a (complete) trace...
The only full trace I got was:
[ 3857.426136] BUG: unable to handle kernel paging request at dbdf9000
[ 3857.432555] IP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c
[ 3857.435906] *pde = 1bebb063 *pte = 1bdf9161
[ 3857.435906] Oops: 0003 [#1]
[ 3857.435906] Modules linked in: hid_picolcd fb_sys_fops sysimgblt sysfillrect syscopyarea drm_kms_helper nfs3 nfs_acl nfs lockd sunrpc
[ 3857.435906] Pid: 1935, comm: bash Not tainted 3.6.0-rc1-jupiter-00363-g0e8ccbc #1 NVIDIA Corporation. nFORCE-MCP/MS-6373
[ 3857.435906] EIP: 0060:[<c11e2be7>] EFLAGS: 00010013 CPU: 0
[ 3857.435906] EIP is at _mmx_memcpy+0x27/0x16c
[ 3857.435906] EAX: dd40a000 EBX: dd6ea010 ECX: 035af77b EDX: dbdf4230
[ 3857.435906] ESI: dbe0d1b0 EDI: dbdf9000 EBP: dd40bea8 ESP: dd40be8c
[ 3857.435906] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
[ 3857.435906] CR0: 8005003b CR2: dbdf9000 CR3: 1d7ee000 CR4: 000007d0
[ 3857.435906] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 3857.435906] DR6: ffff0ff0 DR7: 00000400
[ 3857.435906] Process bash (pid: 1935, ti=dd40a000 task=dcdb6120 task.ti=dbce2000)
[ 3857.435906] Stack:
[ 3857.435906] 0d6d6d6f dd6eb49c dd40beb8 dbde0080 dd6ea010 dbde0080 dbd31010 dd40bed0
[ 3857.435906] c133b364 00000000 00000000 ddfaa0e0 dbd31010 dbdf4230 dd6ea010 00000046
[ 3857.435906] dbd31010 dd40bef0 c133bea0 00000000 dbd0c3f0 00000000 dbd0c3f0 00000000
[ 3857.435906] Call Trace:
[ 3857.435906] [<c133b364>] hid_submit_out+0xa4/0x130
[ 3857.435906] [<c133bea0>] hid_irq_out+0xa0/0x100
[ 3857.435906] [<c12e425e>] usb_hcd_giveback_urb+0x4e/0x90
[ 3857.435906] [<c12f9bd2>] finish_urb+0xb2/0xf0
[ 3857.435906] [<c12fa67d>] takeback_td+0x3d/0x100
[ 3857.435906] [<c12faccf>] dl_done_list+0x14f/0x180
[ 3857.435906] [<c12fc351>] ohci_irq+0x191/0x300
[ 3857.435906] [<c1077790>] ? unmask_irq+0x20/0x20
[ 3857.435906] [<c12e3ace>] usb_hcd_irq+0x1e/0x40
[ 3857.435906] [<c107549d>] handle_irq_event_percpu+0x6d/0x1c0
[ 3857.435906] [<c1077790>] ? unmask_irq+0x20/0x20
[ 3857.435906] [<c1075618>] handle_irq_event+0x28/0x40
[ 3857.435906] [<c107781a>] handle_fasteoi_irq+0x8a/0xe0
[ 3857.435906] <IRQ>
[ 3857.435906] [<c1003e0a>] ? do_IRQ+0x3a/0xb0
[ 3857.435906] [<c140c870>] ? common_interrupt+0x30/0x38
[ 3857.435906] [<c1035143>] ? __do_softirq+0x53/0x1c0
[ 3857.435906] [<c10350f0>] ? local_bh_enable_ip+0x80/0x80
[ 3857.435906] <IRQ>
[ 3857.435906] [<c103544e>] ? irq_exit+0x3e/0xa0
[ 3857.435906] [<c101dfde>] ? smp_apic_timer_interrupt+0x6e/0x80
[ 3857.435906] [<c140bee1>] ? apic_timer_interrupt+0x31/0x38
[ 3857.435906] [<c102fdba>] ? vprintk_emit+0x34a/0x390
[ 3857.435906] [<c1409f58>] ? printk+0x38/0x3a
[ 3857.435906] [<de94b021>] ? picolcd_init_framebuffer+0x261/0x350 [hid_picolcd]
[ 3857.435906] [<de949bd1>] ? picolcd_probe+0x3d1/0x5a0 [hid_picolcd]
[ 3857.435906] [<c13321f7>] ? hid_device_probe+0x67/0xf0
[ 3857.435906] [<c128ac17>] ? driver_sysfs_add+0x57/0x80
[ 3857.435906] [<c128af1d>] ? driver_probe_device+0xbd/0x1d0
[ 3857.435906] [<c1331aab>] ? hid_match_device+0x7b/0x90
[ 3857.435906] [<c1289e65>] ? driver_bind+0x75/0xd0
[ 3857.435906] [<c1289df0>] ? driver_unbind+0x90/0x90
[ 3857.435906] [<c1289537>] ? drv_attr_store+0x27/0x30
[ 3857.435906] [<c111a59c>] ? sysfs_write_file+0xac/0xf0
[ 3857.435906] [<c10cca0c>] ? vfs_write+0x9c/0x130
[ 3857.435906] [<c10da2ef>] ? sys_dup3+0x11f/0x160
[ 3857.435906] [<c111a4f0>] ? sysfs_poll+0x90/0x90
[ 3857.435906] [<c10ccc7d>] ? sys_write+0x3d/0x70
[ 3857.435906] [<c140c357>] ? sysenter_do_call+0x12/0x26
[ 3857.435906] Code: 90 90 90 90 55 89 e5 57 56 89 d6 53 83 ec 10 89 45 f0 89 4d e4 89 e0 25 00 e0 ff ff f7 40 14 00 ff ff 07 74 17 c1 e9 02 8b 7d f0 <f3> a5 8b 4d e4 83 e1 03 74 02 f3 a4 e9 27 01 00 00 8b 45 1
[ 3857.435906] EIP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c SS:ESP 0068:dd40be8c
[ 3857.435906] CR2: 00000000dbdf9000
[ 3857.435906] ---[ end trace 10ae520e65e3b763 ]---
[ 3857.435906] Kernel panic - not syncing: Fatal exception in interrupt
It happens around the time of calling
device_create_file(dev, &dev_attr_fb_update_rate);
that is, after picolcd_fb_reset(data, 1) has returned but before
calling fb_deferred_io_init(info); according to printk() I put
around the various calls in picolcd_init_framebuffer().
The other case I got part of a trace it was again in _mmx_memcpy()
but the trace output stopped in the middle of register dumping.
The full trace happened after the following sequence:
- system boot
- hotplug of picolcd (triggering modprobe hid-picolcd)
- cd /sys/bus/hid/drivers/hid-picolcd/
- echo 0003:04D8:C002.0003 > unbind
- wait some time
- echo 0003:04D8:C002.0003 > bind
Any idea what it could be? It might be during processing of first
reports send in order to clear the picolcd display.
As (un)binding in hid-picolcd crashes I tried doing so a level above,
at usbhid driver on usb bus which resulted in the following:
- system boot
- hotplug of picolcd (triggering modprobe hid-picolcd)
- cd /sys/bus/usb/drivers/usbhid/
- echo 2-1:1.0 > unbind
- wait some time
- echo 2-1:1.0 > bind
- wait some time
- echo 2-1:1.0 > unbind
produced the following trace:
[ 1418.387466] BUG: unable to handle kernel NULL pointer dereference at 00000030
[ 1418.390013] IP: [<c1339c40>] hid_submit_ctrl+0x80/0x2a0
[ 1418.390013] *pde = 00000000
[ 1418.390013] Oops: 0002 [#1]
[ 1418.390013] Modules linked in: hid_picolcd fb_sys_fops sysimgblt sysfillrect syscopyarea drm_kms_helper nfs3 nfs_acl nfs lockd sunrpc
[ 1418.390013] Pid: 1994, comm: kworker/0:0 Not tainted 3.6.0-rc1-jupiter-00363-g0e8ccbc #1 NVIDIA Corporation. nFORCE-MCP/MS-6373
[ 1418.390013] EIP: 0060:[<c1339c40>] EFLAGS: 00010086 CPU: 0
[ 1418.390013] EIP is at hid_submit_ctrl+0x80/0x2a0
[ 1418.390013] EAX: 80000200 EBX: dc2d0000 ECX: dcfe2020 EDX: 00000000
[ 1418.390013] ESI: 00000040 EDI: dc1f8f00 EBP: dd743e2c ESP: dd743df4
[ 1418.390013] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
[ 1418.390013] CR0: 8005003b CR2: 00000030 CR3: 1cd42000 CR4: 000007d0
[ 1418.390013] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 1418.390013] DR6: ffff0ff0 DR7: 00000400
[ 1418.390013] Process kworker/0:0 (pid: 1994, ti=dd742000 task=dcdd0d40 task.ti=dd742000)
[ 1418.390013] Stack:
[ 1418.390013] 0000003e dc2f9020 dcf64c7c 00000005 00000002 dd743e1c 00000002 dc1f8f00
[ 1418.390013] 00000040 dcfe2020 dc2cb4d0 dc2d0000 00000000 dcfe2020 dd743e44 c1339ef6
[ 1418.390013] 00000008 dc2d0000 dc2cb4d0 dcfe2020 dd743e74 c133bbeb 00000023 000000ff
[ 1418.390013] Call Trace:
[ 1418.390013] [<c1339ef6>] usbhid_restart_ctrl_queue+0x96/0xe0
[ 1418.390013] [<c133bbeb>] __usbhid_submit_report+0x29b/0x320
[ 1418.390013] [<c1330200>] ? hid_parser_local+0x260/0x2d0
[ 1418.390013] [<c133bd2b>] usbhid_submit_report+0x1b/0x30
[ 1418.390013] [<de996cb9>] picolcd_fb_deferred_io+0x3b9/0x4c0 [hid_picolcd]
[ 1418.390013] [<c120e118>] fb_deferred_io_work+0x78/0xc0
[ 1418.390013] [<c1045e3f>] process_one_work+0x1ff/0x380
[ 1418.390013] [<c120e0a0>] ? fb_deferred_io_fault+0xa0/0xa0
[ 1418.390013] [<c10467f7>] worker_thread+0x237/0x360
[ 1418.390013] [<c10465c0>] ? manage_workers.clone.25+0x250/0x250
[ 1418.390013] [<c10465c0>] ? manage_workers.clone.25+0x250/0x250
[ 1418.390013] [<c104a30d>] kthread+0x6d/0x80
[ 1418.390013] [<c104a2a0>] ? kthread_freezable_should_stop+0x70/0x70
[ 1418.390013] [<c140c87e>] kernel_thread_helper+0x6/0xd
[ 1418.390013] Code: 01 89 45 e8 0f 85 91 00 00 00 8b 4d ec 8b 53 1c 8b 7d e4 8b 75 e8 8b 81 68 0c 00 00 8b 00 8b 40 9c c1 e0 08 0d 00 00 00 80 85 ff <89> 42 30 8b 43 1c 89 70 54 0f 84 19 01 00 00 81 fe ff 01 0
[ 1418.390013] EIP: [<c1339c40>] hid_submit_ctrl+0x80/0x2a0 SS:ESP 0068:dd743df4
[ 1418.390013] CR2: 0000000000000030
[ 1418.390013] ---[ end trace 089e20ed16261a0a ]---
pretty early during unbind process, e.g. way before picolcd even started to
dispose framebuffer.
Either I'm doing something really wrong or I'm the first one to produce
lots of outbound reports thus exploring corner-cases...
PS: attached is a diff between the hid-picolcd from this patch series to what
I'm testing right now.
Thanks,
Bruno
[-- Attachment #2: pico.diff --]
[-- Type: text/x-patch, Size: 16453 bytes --]
drivers/hid/hid-picolcd.h | 19 +++-
drivers/hid/hid-picolcd_core.c | 6 +
drivers/hid/hid-picolcd_fb.c | 222 +++++++++++++++++++++-------------------
3 files changed, 138 insertions(+), 109 deletions(-)
diff --git a/drivers/hid/hid-picolcd.h b/drivers/hid/hid-picolcd.h
index 9200be1..dc4c795 100644
--- a/drivers/hid/hid-picolcd.h
+++ b/drivers/hid/hid-picolcd.h
@@ -90,11 +90,6 @@ struct picolcd_data {
#ifdef CONFIG_HID_PICOLCD_FB
/* Framebuffer stuff */
- u8 fb_update_rate;
- u8 fb_bpp;
- u8 fb_force;
- u8 *fb_vbitmap; /* local copy of what was sent to PicoLCD */
- u8 *fb_bitmap; /* framebuffer */
struct fb_info *fb_info;
#endif /* CONFIG_HID_PICOLCD_FB */
#ifdef CONFIG_HID_PICOLCD_LCD
@@ -119,9 +114,21 @@ struct picolcd_data {
int status;
#define PICOLCD_BOOTLOADER 1
#define PICOLCD_FAILED 2
-#define PICOLCD_READY_FB 4
};
+#ifdef CONFIG_HID_PICOLCD_FB
+struct picolcd_fb_data {
+ /* Framebuffer stuff */
+ spinlock_t lock;
+ struct picolcd_data *picolcd;
+ u8 update_rate;
+ u8 bpp;
+ u8 force;
+ u8 ready;
+ u8 *vbitmap; /* local copy of what was sent to PicoLCD */
+ u8 *bitmap; /* framebuffer */
+};
+#endif /* CONFIG_HID_PICOLCD_FB */
/* Find a given report */
#define picolcd_in_report(id, dev) picolcd_report(id, dev, HID_INPUT_REPORT)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index e08ffd2..153acc2 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -539,6 +539,7 @@ static int picolcd_probe(struct hid_device *hdev,
int error = -ENOMEM;
dbg_hid(PICOLCD_NAME " hardware probe...\n");
+ printk(KERN_INFO "hid-picolcd: probe() 1/4\n");
/*
* Let's allocate the picolcd data structure, set some reasonable
@@ -559,6 +560,7 @@ static int picolcd_probe(struct hid_device *hdev,
data->status |= PICOLCD_BOOTLOADER;
hid_set_drvdata(hdev, data);
+ printk(KERN_INFO "hid-picolcd: probe(%p) 2/4\n", data);
/* Parse the device reports and start it up */
error = hid_parse(hdev);
if (error) {
@@ -578,6 +580,7 @@ static int picolcd_probe(struct hid_device *hdev,
goto err_cleanup_hid_hw;
}
+ printk(KERN_INFO "hid-picolcd: probe(%p) 3/4\n", data);
error = device_create_file(&hdev->dev, &dev_attr_operation_mode_delay);
if (error) {
hid_err(hdev, "failed to create sysfs attributes\n");
@@ -597,6 +600,7 @@ static int picolcd_probe(struct hid_device *hdev,
if (error)
goto err_cleanup_sysfs2;
+ printk(KERN_INFO "hid-picolcd: probe(%p, [fb=%p]) 4/4\n", data, data->fb_info);
dbg_hid(PICOLCD_NAME " activated and initialized\n");
return 0;
@@ -621,6 +625,7 @@ static void picolcd_remove(struct hid_device *hdev)
struct picolcd_data *data = hid_get_drvdata(hdev);
unsigned long flags;
+ printk(KERN_INFO "hid-picolcd: remove(%p [fb=%p]) ...\n", data, data->fb_info);
dbg_hid(PICOLCD_NAME " hardware remove...\n");
spin_lock_irqsave(&data->lock, flags);
data->status |= PICOLCD_FAILED;
@@ -649,6 +654,7 @@ static void picolcd_remove(struct hid_device *hdev)
picolcd_exit_cir(data);
picolcd_exit_keys(data);
+ printk(KERN_INFO "hid-picolcd: remove(%p [fb=%p]) done\n", data, data->fb_info);
mutex_destroy(&data->mutex);
/* Finally, clean up the picolcd data itself */
kfree(data);
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index 4d8e22c..86b7a24 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -98,16 +98,19 @@ static const struct fb_var_screeninfo picolcdfb_var = {
};
/* Send a given tile to PicoLCD */
-static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
+static int picolcd_fb_send_tile(struct picolcd_data *data, u8 *vbitmap,
+ int chip, int tile)
{
- struct picolcd_data *data = hid_get_drvdata(hdev);
- struct hid_report *report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, hdev);
- struct hid_report *report2 = picolcd_out_report(REPORT_LCD_DATA, hdev);
+ struct hid_report *report1, *report2;
unsigned long flags;
u8 *tdata;
int i;
- if (!report1 || report1->maxfield != 1 || !report2 || report2->maxfield != 1)
+ report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, data->hdev);
+ if (!report1 || report1->maxfield != 1)
+ return -ENODEV;
+ report2 = picolcd_out_report(REPORT_LCD_DATA, data->hdev);
+ if (!report2 || report2->maxfield != 1)
return -ENODEV;
spin_lock_irqsave(&data->lock, flags);
@@ -128,7 +131,7 @@ static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
hid_set_field(report2->field[0], 2, 0x00);
hid_set_field(report2->field[0], 3, 32);
- tdata = data->fb_vbitmap + (tile * 4 + chip) * 64;
+ tdata = vbitmap + (tile * 4 + chip) * 64;
for (i = 0; i < 64; i++)
if (i < 32)
hid_set_field(report1->field[0], 11 + i, tdata[i]);
@@ -189,6 +192,7 @@ void picolcd_fb_refresh(struct picolcd_data *data)
int picolcd_fb_reset(struct picolcd_data *data, int clear)
{
struct hid_report *report = picolcd_out_report(REPORT_LCD_CMD, data->hdev);
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
int i, j;
unsigned long flags;
static const u8 mapcmd[8] = { 0x00, 0x02, 0x00, 0x64, 0x3f, 0x00, 0x64, 0xc0 };
@@ -207,20 +211,19 @@ int picolcd_fb_reset(struct picolcd_data *data, int clear)
hid_set_field(report->field[0], j, 0);
usbhid_submit_report(data->hdev, report, USB_DIR_OUT);
}
-
- data->status |= PICOLCD_READY_FB;
spin_unlock_irqrestore(&data->lock, flags);
- if (data->fb_bitmap) {
- if (clear) {
- memset(data->fb_vbitmap, 0, PICOLCDFB_SIZE);
- memset(data->fb_bitmap, 0, PICOLCDFB_SIZE*data->fb_bpp);
- }
- data->fb_force = 1;
+ if (clear) {
+ memset(fbdata->vbitmap, 0, PICOLCDFB_SIZE);
+ memset(fbdata->bitmap, 0, PICOLCDFB_SIZE*fbdata->bpp);
}
+ fbdata->force = 1;
/* schedule first output of framebuffer */
- picolcd_fb_refresh(data);
+ if (fbdata->ready)
+ schedule_delayed_work(&data->fb_info->deferred_work, 0);
+ else
+ fbdata->ready = 1;
return 0;
}
@@ -230,20 +233,15 @@ static void picolcd_fb_update(struct fb_info *info)
{
int chip, tile, n;
unsigned long flags;
+ struct picolcd_fb_data *fbdata = info->par;
struct picolcd_data *data;
mutex_lock(&info->lock);
- data = info->par;
- if (!data)
- goto out;
- spin_lock_irqsave(&data->lock, flags);
- if (!(data->status & PICOLCD_READY_FB)) {
- spin_unlock_irqrestore(&data->lock, flags);
- picolcd_fb_reset(data, 0);
- } else {
- spin_unlock_irqrestore(&data->lock, flags);
- }
+ spin_lock_irqsave(&fbdata->lock, flags);
+ if (!fbdata->ready && fbdata->picolcd)
+ picolcd_fb_reset(fbdata->picolcd, 0);
+ spin_unlock_irqrestore(&fbdata->lock, flags);
/*
* Translate the framebuffer into the format needed by the PicoLCD.
@@ -254,32 +252,38 @@ static void picolcd_fb_update(struct fb_info *info)
*/
n = 0;
for (chip = 0; chip < 4; chip++)
- for (tile = 0; tile < 8; tile++)
- if (picolcd_fb_update_tile(data->fb_vbitmap,
- data->fb_bitmap, data->fb_bpp, chip, tile) ||
- data->fb_force) {
- n += 2;
- if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
- mutex_unlock(&info->lock);
- usbhid_wait_io(data->hdev);
- mutex_lock(&info->lock);
- data = info->par;
- if (!data)
- goto out;
- spin_lock_irqsave(&data->lock, flags);
- if (data->status & PICOLCD_FAILED) {
- spin_unlock_irqrestore(&data->lock, flags);
- goto out;
- }
- spin_unlock_irqrestore(&data->lock, flags);
- n = 0;
- }
- picolcd_fb_send_tile(data->hdev, chip, tile);
+ for (tile = 0; tile < 8; tile++) {
+ if (!fbdata->force && !picolcd_fb_update_tile(
+ fbdata->vbitmap, fbdata->bitmap,
+ fbdata->bpp, chip, tile))
+ continue;
+ n += 2;
+ if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+ mutex_unlock(&info->lock);
+ if (!data)
+ return;
+ usbhid_wait_io(data->hdev);
+ mutex_lock(&info->lock);
+ n = 0;
}
- data->fb_force = false;
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+ if (!data)
+ goto out;
+ picolcd_fb_send_tile(data, fbdata->vbitmap, chip, tile);
+ }
+ fbdata->force = false;
if (n) {
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
mutex_unlock(&info->lock);
- usbhid_wait_io(data->hdev);
+ if (data)
+ usbhid_wait_io(data->hdev);
return;
}
out:
@@ -336,20 +340,24 @@ static ssize_t picolcd_fb_write(struct fb_info *info, const char __user *buf,
static int picolcd_fb_blank(int blank, struct fb_info *info)
{
- if (!info->par)
- return -ENODEV;
/* We let fb notification do this for us via lcd/backlight device */
return 0;
}
static void picolcd_fb_destroy(struct fb_info *info)
{
+ struct picolcd_fb_data *fbdata = info->par;
+
+ printk(KERN_INFO "hid-picolcd: fb_destroy(%p [fb=%p]) ...\n", fbdata->picolcd, info);
/* make sure no work is deferred */
fb_deferred_io_cleanup(info);
+ /* No thridparty should ever unregister our framebuffer! */
+ WARN_ON(fbdata->picolcd != NULL);
+
vfree((u8 *)info->fix.smem_start);
framebuffer_release(info);
- printk(KERN_DEBUG "picolcd_fb_destroy(%p)\n", info);
+ printk(KERN_INFO "hid-picolcd: fb_destroy(-- [fb=%p]) done\n", info);
}
static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
@@ -376,17 +384,15 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
static int picolcd_set_par(struct fb_info *info)
{
- struct picolcd_data *data = info->par;
+ struct picolcd_fb_data *fbdata = info->par;
u8 *tmp_fb, *o_fb;
- if (!data)
- return -ENODEV;
- if (info->var.bits_per_pixel == data->fb_bpp)
+ if (info->var.bits_per_pixel == fbdata->bpp)
return 0;
/* switch between 1/8 bit depths */
if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
return -EINVAL;
- o_fb = data->fb_bitmap;
+ o_fb = fbdata->bitmap;
tmp_fb = kmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel, GFP_KERNEL);
if (!tmp_fb)
return -ENOMEM;
@@ -415,7 +421,7 @@ static int picolcd_set_par(struct fb_info *info)
}
kfree(tmp_fb);
- data->fb_bpp = info->var.bits_per_pixel;
+ fbdata->bpp = info->var.bits_per_pixel;
return 0;
}
@@ -453,7 +459,8 @@ static ssize_t picolcd_fb_update_rate_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct picolcd_data *data = dev_get_drvdata(dev);
- unsigned i, fb_update_rate = data->fb_update_rate;
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
+ unsigned i, fb_update_rate = fbdata->update_rate;
size_t ret = 0;
for (i = 1; i <= PICOLCDFB_UPDATE_RATE_LIMIT; i++)
@@ -472,6 +479,7 @@ static ssize_t picolcd_fb_update_rate_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{
struct picolcd_data *data = dev_get_drvdata(dev);
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
int i;
unsigned u;
@@ -487,8 +495,8 @@ static ssize_t picolcd_fb_update_rate_store(struct device *dev,
else if (u == 0)
u = PICOLCDFB_UPDATE_RATE_DEFAULT;
- data->fb_update_rate = u;
- data->fb_info->fbdefio->delay = HZ / data->fb_update_rate;
+ fbdata->update_rate = u;
+ data->fb_info->fbdefio->delay = HZ / fbdata->update_rate;
return count;
}
@@ -500,30 +508,18 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
{
struct device *dev = &data->hdev->dev;
struct fb_info *info = NULL;
+ struct picolcd_fb_data *fbdata = NULL;
int i, error = -ENOMEM;
- u8 *fb_vbitmap = NULL;
- u8 *fb_bitmap = NULL;
u32 *palette;
- fb_bitmap = vmalloc(PICOLCDFB_SIZE*8);
- if (fb_bitmap == NULL) {
- dev_err(dev, "can't get a free page for framebuffer\n");
- goto err_nomem;
- }
-
- fb_vbitmap = kmalloc(PICOLCDFB_SIZE, GFP_KERNEL);
- if (fb_vbitmap == NULL) {
- dev_err(dev, "can't alloc vbitmap image buffer\n");
- goto err_nomem;
- }
-
- data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
/* The extra memory is:
* - 256*u32 for pseudo_palette
* - struct fb_deferred_io
*/
info = framebuffer_alloc(256 * sizeof(u32) +
- sizeof(struct fb_deferred_io), dev);
+ sizeof(struct fb_deferred_io) +
+ sizeof(struct picolcd_fb_data) +
+ PICOLCDFB_SIZE, dev);
if (info == NULL) {
dev_err(dev, "failed to allocate a framebuffer\n");
goto err_nomem;
@@ -531,74 +527,94 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
info->fbdefio = info->par;
*info->fbdefio = picolcd_fb_defio;
- palette = info->par + sizeof(struct fb_deferred_io);
+ info->par += sizeof(struct fb_deferred_io);
+ palette = info->par;
+ info->par += 256 * sizeof(u32);
for (i = 0; i < 256; i++)
palette[i] = i > 0 && i < 16 ? 0xff : 0;
info->pseudo_palette = palette;
- info->screen_base = (char __force __iomem *)fb_bitmap;
info->fbops = &picolcdfb_ops;
info->var = picolcdfb_var;
info->fix = picolcdfb_fix;
info->fix.smem_len = PICOLCDFB_SIZE*8;
- info->fix.smem_start = (unsigned long)fb_bitmap;
- info->par = data;
info->flags = FBINFO_FLAG_DEFAULT;
- data->fb_vbitmap = fb_vbitmap;
- data->fb_bitmap = fb_bitmap;
- data->fb_bpp = picolcdfb_var.bits_per_pixel;
+ fbdata = info->par;
+ spin_lock_init(&fbdata->lock);
+ fbdata->picolcd = data;
+ fbdata->update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
+ fbdata->bpp = picolcdfb_var.bits_per_pixel;
+ fbdata->force = 1;
+ fbdata->vbitmap = info->par + sizeof(struct picolcd_fb_data);
+ fbdata->bitmap = vmalloc(PICOLCDFB_SIZE*8);
+ if (fbdata->bitmap == NULL) {
+ dev_err(dev, "can't get a free page for framebuffer\n");
+ goto err_nomem;
+ }
+ info->screen_base = (char __force __iomem *)fbdata->bitmap;
+ info->fix.smem_start = (unsigned long)fbdata->bitmap;
+ memset(fbdata->vbitmap, 0xff, PICOLCDFB_SIZE);
+ data->fb_info = info;
+
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) 1/5\n", data, info);
error = picolcd_fb_reset(data, 1);
if (error) {
dev_err(dev, "failed to configure display\n");
goto err_cleanup;
}
+
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) 2/5\n", data, info);
error = device_create_file(dev, &dev_attr_fb_update_rate);
if (error) {
dev_err(dev, "failed to create sysfs attributes\n");
goto err_cleanup;
}
+
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) 3/5\n", data, info);
fb_deferred_io_init(info);
- data->fb_info = info;
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) 4/5\n", data, info);
error = register_framebuffer(info);
if (error) {
dev_err(dev, "failed to register framebuffer\n");
goto err_sysfs;
}
- /* schedule first output of framebuffer */
- data->fb_force = 1;
- schedule_delayed_work(&info->deferred_work, 0);
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) 5/5\n", data, info);
return 0;
err_sysfs:
- fb_deferred_io_cleanup(info);
device_remove_file(dev, &dev_attr_fb_update_rate);
+ fb_deferred_io_cleanup(info);
err_cleanup:
- data->fb_vbitmap = NULL;
- data->fb_bitmap = NULL;
- data->fb_bpp = 0;
data->fb_info = NULL;
err_nomem:
+ if (fbdata)
+ vfree(fbdata->bitmap);
framebuffer_release(info);
- vfree(fb_bitmap);
- kfree(fb_vbitmap);
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) failed\n", data, info);
return error;
}
void picolcd_exit_framebuffer(struct picolcd_data *data)
{
struct fb_info *info = data->fb_info;
- u8 *fb_vbitmap = data->fb_vbitmap;
-
- if (!info)
- return;
+ struct picolcd_fb_data *fbdata = info->par;
+ unsigned long flags;
+ printk(KERN_INFO "hid-picolcd: exit_framebuffer(%p [fb=%p]) ...\n", data, info);
device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
- info->par = NULL;
+
+ /* disconnect framebuffer from HID dev */
+ spin_lock_irqsave(&fbdata->lock, flags);
+ fbdata->picolcd = NULL;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+
+ /* make sure there is no running update - thus that fbdata->picolcd
+ * once obtained under lock is guaranteed not to get free() under
+ * the feet of the deferred work */
+ flush_delayed_work_sync(&info->deferred_work);
+
+ data->fb_info = NULL;
unregister_framebuffer(info);
- data->fb_vbitmap = NULL;
- data->fb_bitmap = NULL;
- data->fb_bpp = 0;
- data->fb_info = NULL;
- kfree(fb_vbitmap);
+ printk(KERN_INFO "hid-picolcd: exit_framebuffer(%p [fb=%p]) done\n", data, info);
}
^ permalink raw reply related
* Re: [UDL] general protection fault in fb_deferred_io_mkwrite()
From: Thomas Meyer @ 2012-08-18 11:00 UTC (permalink / raw)
To: Bernie Thompson; +Cc: linux-fbdev, linux-kernel, airlied, dri-devel
In-Reply-To: <CAO1w=s8AdkbSNMe8v0HqkosYNKp05+Udk4d-r1TbRb3GMC-MNw@mail.gmail.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 1715 bytes --]
Am Sonntag, den 12.08.2012, 14:22 -0700 schrieb Bernie Thompson:
> On Sun, Aug 12, 2012 at 3:34 AM, Thomas Meyer <thomas@m3y3r.de> wrote:
> guilty driver is probably udl_fb.c
> any ideas?
>
>
> Hi Thomas,
Hi Bernie!
> We were seeing similar issues in udlfb (the original fbdev version of
> this driver), which were fixed earlier this year by getting all
> rendering operations out of probe/disconnect -- those which might
> trigger fb_defio page faults in an inappropriate context, or be
> long-running. Here's some more detail:
> http://plugable.com/2012/06/21/displaylink-usb-devices-on-linux-kernel-3-4-0/comment-page-1/#comment-5896
>
>
> Unfortunately, I haven't had time to get going with udl myself, so
> haven't been able to port and confirm. Thanks for raising and staying
> on this.
Okay, I see. I'll switch to FB_UDL for now and remove DRM_UDL from my
config.
Is somebody working on porting commit
https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h21547d3c9c3bc653261f26d554cfabc4a083de to the DRM_UDL driver?
In Airlie's tree seems to be no commit related to this:
http://cgit.freedesktop.org/~airlied/linux/
with kind regards
thomas
>
>
> Best wishes,
> Bernie
>
>
> [ 45.633336] RIP [<ffffffff8123becc>]
> fb_deferred_io_mkwrite+0xdc/0xf0
> [ 45.633336] RSP <ffff880126559c98>
> [ 45.711547] ---[ end trace d4732d5a0bf375fb ]---
> [ 45.720961] released /dev/fb1 user=1 count=0
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC 0/5] Generic panel framework
From: Laurent Pinchart @ 2012-08-18 1:16 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev, Marcus Lorentzon, dri-devel, Kyungmin Park,
Richard Purdie, Sebastien Guiriec, Bryan Wu, linux-leds,
linux-media
In-Reply-To: <1345203751.3158.99.camel@deskari>
Hi Tomi,
On Friday 17 August 2012 14:42:31 Tomi Valkeinen wrote:
> On Fri, 2012-08-17 at 13:10 +0200, Laurent Pinchart wrote:
> > What kind of directory structure do you have in mind ? Panels are already
> > isolated in drivers/video/panel/ so we could already ditch the panel-
> > prefix in drivers.
>
> The same directory also contains files for the framework and buses. But
> perhaps there's no need for additional directories if the amount of
> non-panel files is small. And you can easily see from the name that they
> are not panel drivers (e.g. mipi_dbi_bus.c).
I don't expect the directory to contain many non-panel files, so let's keep it
as-is for now.
mipi-dbi-bus might not belong to include/video/panel/ though, as it can be
used for non-panel devices (at least in theory). The future mipi-dsi-bus
certainly will.
> > Would you also create include/video/panel/ ?
>
> Perhaps that would be good. Well, having all the files prefixed with
> panel- is not bad as such, but just feel extra.
>
> > > ---
> > >
> > > Should we aim for DT only solution from the start? DT is the direction
> > > we are going, and I feel the older platform data stuff would be
> > > deprecated soon.
> >
> > Don't forget about non-ARM architectures :-/ We need panel drivers for SH
> > as well, which doesn't use DT. I don't think that would be a big issue, a
> > DT- compliant solution should be easy to use through board code and
> > platform data as well.
>
> I didn't forget about them as I didn't even think about them ;). I
> somehow had the impression that other architectures would also use DT,
> sooner or later. I could be mistaken, though.
>
> And true, it's not a big issue to support both DT and non-DT versions,
> but I've been porting omap stuff for DT and keeping the old platform
> data stuff also there, and it just feels burdensome. For very simple
> panels it's easy, but when you've passing lots of parameters the code
> starts to get longer.
>
> > > This one would be rather impossible with the upper layer handling the
> > > enabling of the video stream. Thus I see that the panel driver needs to
> > > control the sequences, and the Sharp panel driver's enable would look
> > > something like:
> > >
> > > regulator_enable(...);
> > > sleep();
> > > dpi_enable_video();
> > > sleep();
> > > gpip_set(..);
> >
> > I have to admit I have no better solution to propose at the moment, even
> > if I don't really like making the panel control the video stream. When
> > several devices will be present in the chain all of them might have
> > similar annoying requirements, and my feeling is that the resulting code
> > will be quite messy. At the end of the day the only way to really find
> > out is to write an implementation.
>
> If we have a chain of devices, and each device uses the bus interface
> from the previous device in the chain, there shouldn't be a problem. In
> that model each device can handle the task however is best for it.
>
> I think the problems come from the divided control we'll have. I mean,
> if the panel driver would decide itself what to send to its output, and
> it would provide the data (think of an i2c device), this would be very
> simple. And it actually is for things like configuration data etc, but
> not so for video stream.
Would you be able to send incremental patches on top of v2 to implement the
solution you have in mind ? It would be neat if you could also implement mipi-
dsi-bus for the OMAP DSS and test the code with a real device :-)
> > > It could cause some locking issues, though. First the panel's remove
> > > could take a lock, but the remove sequence would cause the display
> > > driver to call disable on the panel, which could again try to take the
> > > same lock...
> >
> > We have two possible ways of calling panel operations, either directly
> > (panel->bus->ops->enable(...)) or indirectly (panel_enable(...)).
> >
> > The former is what V4L2 currently does with subdevs, and requires display
> > drivers to hold a reference to the panel. The later can do without a
> > direct reference only if we use a global lock, which is something I would
> > like to
>
> Wouldn't panel_enable() just do the same panel->bus->ops->enable()
> anyway, and both require a panel reference? I don't see the difference.
Indeed, you're right. I'm not sure what I was thinking about.
> > avoid. A panel-wide lock wouldn't work, as the access function would need
> > to take the lock on a panel instance that can be removed at any time.
>
> Can't this be handled with some kind of get/put refcounting? If there's
> a ref, it can't be removed.
Trouble will come when the display driver will hold a reference to the panel,
and the panel will hold a reference to the display driver (for instance
because the display driver provides the DBI/DSI bus, or because it provides a
clock used by the panel).
> Generally about locks, if we define that panel ops may only be called
> exclusively, does it simplify things? I think we can make such
> requirements, as there should be only one display framework that handles
> the panel. Then we don't need locking for things like enable/disable.
Pushing locking to callers would indeed simplify panel drivers, but we need to
make sure we won't need to expose a panel to several callers in the future.
> Of course we need to be careful about things where calls come from
> "outside" the display framework. I guess one such thing is rmmod, but if
> that causes a notification to the display framework, which again handles
> locking, it shouldn't be a problem.
>
> Another thing to be careful about is if the panel internally uses irqs,
> workqueues, sysfs files or such. In that case it needs to handle
> locking.
Of course panels will need to manage concurrency for their own infrastructure.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Mark Brown @ 2012-08-17 23:04 UTC (permalink / raw)
To: Mitch Bradley
Cc: Stephen Warren, Alexandre Courbot, linux-fbdev, Stephen Warren,
linux-doc, linux-kernel, Rob Herring, Anton Vorontsov,
linux-tegra, David Woodhouse, devicetree-discuss
In-Reply-To: <502D61C6.5050101@firmworks.com>
[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]
On Thu, Aug 16, 2012 at 11:10:30AM -1000, Mitch Bradley wrote:
> On 8/16/2012 8:38 AM, Stephen Warren wrote:
> > Device tree bindings shouldn't reference Linux documentation; the
> > bindings are supposed to be OS-agnostic.
> While it is true that bindings should try to be OS-agnostic, there is
> the practical matter of where to put documentation so that it is widely
> accessible. The Linux source tree is one of the most accessible things
> there is, considering how widely it is replicated.
> As the original instigator of the policy that the device tree should
> describe the hardware "OS-neutrally", I personally don't have a problem
> with bindings referring to Linux documentation. I wouldn't like
> references to proprietary and inaccessible documentation.
OS agnosticness isn't the only issue here - the other problem with using
Linux documentation is that except for things that are specifically
userspace interfaces and the DT bindings nothing is intended to be
stable so bindings defined in terms of Linux documentation may randomly
change. We're not doing an awesome job of that with DT right now but we
should try and so we ought to avoid including non-ABI things in ABIs
like this.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH] fbdev: Make the switch from generic to native driver less alarming
From: Adam Jackson @ 2012-08-17 17:10 UTC (permalink / raw)
To: linux-fbdev
Calling this "conflicting" just makes people think there's a problem
when there's not.
Signed-off-by: Adam Jackson <ajax@redhat.com>
---
drivers/video/fbmem.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 0dff12a..42be978 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1582,8 +1582,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
(primary && gen_aper && gen_aper->count &&
gen_aper->ranges[0].base = VGA_FB_PHYS)) {
- printk(KERN_INFO "fb: conflicting fb hw usage "
- "%s vs %s - removing generic driver\n",
+ printk(KERN_INFO "fb: switching to %s from %s\n",
name, registered_fb[i]->fix.id);
do_unregister_framebuffer(registered_fb[i]);
}
--
1.7.7.6
^ permalink raw reply related
* Re: [RFC 3/5] video: panel: Add MIPI DBI bus support
From: Laurent Pinchart @ 2012-08-17 14:06 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <1345208790.3158.133.camel@deskari>
Hi Tomi,
On Friday 17 August 2012 16:06:30 Tomi Valkeinen wrote:
> On Fri, 2012-08-17 at 14:33 +0200, Laurent Pinchart wrote:
> > > But first, the data type should be byte, not unsigned long. How would
> > > you write 8 bits or 16 bits with your API?
> >
> > u8 and u16 both fit in an unsigned long :-) Please see below.
>
> Ah, I see, so the driver would just discard 24 bits or 16 bits from the
> ulong.
That's right.
> I somehow thought that if you have 8 bit bus, and you call the write with
> ulong, 4 bytes will be written.
>
> > > Then again, I'd hope to have DCS somehow as a separate library, which
> > > would then use DBI/DSI/whatnot to actually send the data.
> > >
> > > I'm not quite sure how easy that is because of the differences between
> > > the busses.
> > >
> > > > Is DBI limited to 8-bit data transfers for commands ? Pixels can be
> > > > transferred 16-bit at a time, commands might as well. While DCS only
> > > > specifies 8-bit command/data, DBI panels that are not DCS compliant
> > > > can use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel,
> > > > does so).
> > >
> > > I have to say I don't remember much about DBI =). Looking at OMAP's
> > > driver, which was made for omap2 and hasn't been much updated since, I
> > > see that there are 4 modes, 8/9/12/16 bits. I think that defines how
> > > many of the parallel data lines are used.
> >
> > SYS-80 also has an 18-bits mode, where bits 0 and 9 are always ignored
> > when transferring instructions and data other than pixels (for pixels the
> > 18-bits bus width can be used to transfer RGB666 in a single clock cycle).
> >
> > See page 87 of
> > http://read.pudn.com/downloads91/sourcecode/others/348230/e61505_103a.pdf.
> >
> > > However, I don't think that matters for the panel driver when it wants
> > > to send data. The panel driver should just call dbi_write(buf, buf_len),
> > > and the dbi driver would send the data in the buffer according to the
> > > bus width.
> >
> > According to the DCS specification, commands and parameters are
> > transferred using 8-bit data. Non-DCS panels can however use wider
> > commands and parameters (all commands and parameters are 16-bits wide for
> > the R61505 for instance).
> >
> > We can add an API to switch the DBI bus width on the fly. For Renesas
> > hardware this would "just" require shifting bits around to output the
> > 8-bit or 16-bit commands on the right data lines (the R61505 uses D17-D9
> > in 8-bit mode, while the DCS specification mentions D7-D0) based on how
> > the panel is connected and on which lines the panel expects data.
> >
> > As commands can be expressed on either 8 or 16 bits I would use a 16 type
> > for them.
>
> I won't put my head on the block, but I don't think DBI has any
> restriction on the size of the command. A "command" just means a data
> transfer while keeping the D/CX line low, and "data" when the line is
> high. Similar transfers for n bytes can be done in both modes.
Right. I'll see if the API could be simplified by having a single write
callback function with a data/command parameter.
> > For parameters, we can either express everything as u8 * in the DBI bus
> > operations, or use a union similar to i2c_smbus_data
> >
> > union i2c_smbus_data {
> >
> > __u8 byte;
> > __u16 word;
> > __u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for
> > length */
> >
> > /* and one more for user-space
> > compatibility */
> >
> > };
>
> There's no DBI_BLOCK_MAX, so at least identical union won't work. I
> think it's simplest to have u8 * function as a base, and then a few
> helpers to write the most common datatypes.
OK, that sounds good to me.
> So we could have on the lowest level something like:
>
> dbi_write_command(u8 *buf, size_t size);
> dbi_write_data(u8 *buf, size_t size);
>
> And possible helpers:
>
> dbi_write_data(u8 *cmd_buf, size_t cmd_size, u8 *data_buf, size_t
> data_size);
>
> dbi_write_dcs(u8 cmd, u8 *data, size_t size);
>
> And variations:
>
> dbi_write_dcs_0(u8 cmd);
> dbi_write_dcs_1(u8 cmd, u8 data);
>
> etc. So a simple helper to send 16 bits would be:
>
> dbi_write_data(u16 data)
> {
> // or are the bytes the other way around...
> u8 buf[2] = { data & 0xff, (data >> 8) & 0xff };
> return dbi_write_data(buf, 2);
> }
>
> > Helper functions would be available to perform 8-bit, 16-bit or n*8 bits
> > transfers.
> >
> > Would that work for your use cases ?
> >
> > > Also note that some chips need to change the bus width on the fly. The
> > > chip used on N800 wants configuration to be done with 8-bits, and pixel
> > > transfers with 16-bits. Who knows why...
> >
> > On which data lines is configuration performed ? D7-D0 ?
>
> I guess so, but does it matter? All the bus driver needs to know is how
> to send 8/16/.. bit data. On OMAP we just write the data to a 32 bit
> register, and the HW takes the lowest n bits. Do the bits represent the
> data lines directly on Renesans?
Yes they do. For a SYS-80 panel configured in 18-bits mode, I'll have to write
((data & 0xff00) << 2) | ((data & 0x00ff) << 1)
(d15:8 -> D17:10, d7:0 -> D8:1 where d is the word to be written, and D the
physical lines)
to the hardware data register and trigger the write. In 8 bits mode, there
would be two write operations with
(data & 0xff00) << 2
(data & 0x00ff) << 10
(d15:8 -> D17:10, d7:0 -> D17:10)
However, when writing a 8-bit command to a DBI panel in either 16- or 8-bits
mode, there would be a single write with
d7:0 -> D7:0
How to shift the data thus depends both on the bus width and on which data
lines the panel expects data to be present.
I wrote drivers for two DBI panels based on existing board code, the R61505
and the R61517.
The R61505 datasheet is available online, the panel is a SYS-80 panel that
supports 8-, 9-, 16- and 18-bits bus widths. It aligns data towards the MSB
when using a bus width lower than 18.
The R61517 datasheet doesn't seem to be freely available. The panel seems to
be DBI-compliant as it uses a subset of the DCS commands and a wide range of
panel-specific commands. The panel is connected using a 16-bit bus, all
commands and parameters are 8-bits wide and aligned towards the LSB.
To properly transfer commands and parameters, the DBI host will need to know
on how many bits to perform transfers, and how to align data on the bus. For
the former, your mipi_dbi_set_bus_width() function could be used, although
probably not out of the box. The R61505 panel would call
mipi_dbi_set_bus_width() to set the bus width to 16 (as commands and
parameters are 16-bits wide), but if the panel is connected using only 8 or 9
data lines, the host would need to split the 16-bits writes into two 8-bits
writes. Should that be done transparently ? mipi_dbi_set_bus_width() could
possibly act as a mipi_dbi_set_max_bus_width(), but that might be a bit too
hackish.
I'd like to hide as much of the complexity as possible in mipi-dbi-bus.c but I
don't know whether that's possible.
> The omap driver actually only implements 8 and 16 bit modes, not the 9 and
> 12 bit modes. I'm not sure what kind of shifting is needed for those.
There's no 12-bits mode in DBI-2 as far as I can tell.
We will need to support 8-, 9- and 16-bits modes for DBI-2, and additionally
18-bits mode for SYS-80.
> > > > We might just need to provide fake timings. Video mode timings are at
> > > > the core of display support in all drivers so we can't just get rid of
> > > > them. The h/v front/back porch and sync won't be used by display
> > > > drivers for DBI/DSI panels anyway.
> > >
> > > Right. But we should probably think if we can, at the panel level,
> > > easily separate conventional panels and smart panels. Then this
> > > framework wouldn't need to fake the timings, and it'd be up to the
> > > higher level to decide if and how to fake them. Then again, this is no
> > > biggie. Just thought that at the lowest level it'd be nice to be
> > > "correct" and leave faking to upper layers =).
> >
> > But we would then have two different APIs at the lower level depending on
> > the panel type. I'm not sure that's a good thing.
>
> Different API for what? Why anyway need panel type specific functions.
> In the panel struct we could just have an union of the different types
> of parameters for different types of panels.
>
> But if this complicates things, it's not a biggie. Just something that
> has been in my mind when dealing with smart panels and assigning dummy
> video timings for them =).
Please feel free to make a proposal for this when I'll post v2. A patch would
be nice :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH V4 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-17 13:54 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345115913-6773-1-git-send-email-cmahapatra@ti.com>
[-- Attachment #1: Type: text/plain, Size: 5554 bytes --]
On Thu, 2012-08-16 at 16:48 +0530, Chandrabhanu Mahapatra wrote:
> All the cpu_is checks have been moved to dss_init_features function providing a
> much more generic and cleaner interface. The OMAP version and revision specific
> initializations in various functions are cleaned and the necessary data are
> moved to dss_features structure which is local to dss.c.
>
> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> +static int __init dss_init_features(struct device *dev)
> +{
> + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
> + if (!dss.feat) {
> + dev_err(dev, "Failed to allocate local DSS Features\n");
> + return -ENOMEM;
> + }
> +
> + if (cpu_is_omap24xx())
> + dss.feat = &omap24xx_dss_features;
> + else if (cpu_is_omap34xx())
> + dss.feat = &omap34xx_dss_features;
> + else if (cpu_is_omap3630())
> + dss.feat = &omap3630_dss_features;
> + else if (cpu_is_omap44xx())
> + dss.feat = &omap44xx_dss_features;
> + else
> + return -ENODEV;
> +
> + return 0;
> +}
This is not correct (and same problem in dispc). You allocate the feat
struct and assign the pointer to dss.feat, but then overwrite dss.feat
pointer with the pointer to omap24xx_dss_features (which is freed
later). You need to memcpy it.
I also get a crash on omap3 overo board when loading omapdss:
loading nfs/work/linux/drivers/video/omap2/dss/omapdss.ko debug=y def_disp=lcd43
[ 20.411224] Unable to handle kernel NULL pointer dereference at virtual address 00000008
[ 20.419921] pgd = ce8a8000
[ 20.422790] [00000008] *pgd=8e8c5831, *pte=00000000, *ppte=00000000
[ 20.429473] Internal error: Oops: 17 [#1] SMP ARM
[ 20.434448] Modules linked in: omapdss(+)
[ 20.438690] CPU: 0 Tainted: G W (3.5.0-rc2-00058-g1c1e55c #93)
[ 20.446350] PC is at omap_dsshw_probe+0xa4/0x290 [omapdss]
[ 20.452148] LR is at 0x2e39
[ 20.455108] pc : [<bf043288>] lr : [<00002e39>] psr: 80000013
[ 20.455108] sp : ce89ddd0 ip : c0b797e0 fp : 00006133
[ 20.467224] r10: 00000028 r9 : c0c5c07c r8 : bf02eadc
[ 20.472717] r7 : 00000000 r6 : c06e9644 r5 : cf0cf808 r4 : bf02f430
[ 20.479614] r3 : cf0cf808 r2 : 00000000 r1 : 00000000 r0 : 00000000
[ 20.486511] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 20.494049] Control: 10c5387d Table: 8e8a8019 DAC: 00000015
[ 20.500091] Process insmod (pid: 664, stack limit = 0xce89c2f8)
[ 20.506347] Stack: (0xce89ddd0 to 0xce89e000)
[ 20.510955] ddc0: cf0cf808 c0c96ed8 c0c96ee8 c02c22e4
[ 20.519592] dde0: c02c22cc c02c0f28 22222222 cf0cf808 bf02eadc cf0cf83c 00000000 00000001
[ 20.528198] de00: 00000028 c02c113c bf02eadc c02c10a8 00000000 c02bf6c8 cf0192a8 cf0cec10
[ 20.536834] de20: bf02eadc c072fab8 cf3e7440 c02c05dc bf0249e0 00000000 cf04ce40 bf02eadc
[ 20.545471] de40: c0748880 ce89c000 00000000 00000001 c0c5c07c 00000028 00006133 c02c1670
[ 20.554107] de60: 00000000 bf02eac8 c0748880 ce89c000 00000000 00000001 00000028 c02c26e0
[ 20.562744] de80: 00000003 00000000 c0748880 bf043124 00000000 c0748880 ce89c000 00000000
[ 20.571380] dea0: 00000001 c0008730 bf02f29c 00000001 00000001 bf0430cc c071bcd0 00000000
[ 20.580017] dec0: bf02f29c c006823c 00000000 ce827ec0 cf0001c0 00000000 bf02f29c 00000001
[ 20.588623] dee0: ce851480 00000001 c0c5c07c 00000028 00006133 c0099d20 bf02f2a8 00007fff
[ 20.597259] df00: c0098aa4 c012480c 00000000 c0098890 bf02f3f0 ce89c000 c06d32d8 d08fe09c
[ 20.605895] df20: d0a19624 000a7008 d08ce000 001f2ab7 d0a18bd4 d0a18948 d0aba984 00030ed0
[ 20.614532] df40: 0003b0e0 00000000 00000000 00000042 00000043 00000026 0000002a 00000014
[ 20.623138] df60: 00000000 bf022024 00000043 00000000 00000000 00000000 00000000 c0623b14
[ 20.631774] df80: 001f2ab7 001f2ab7 00000004 beb48e7c 00000080 c0013f28 ce89c000 00000000
[ 20.640411] dfa0: 00000000 c0013d60 001f2ab7 00000004 b6c49008 001f2ab7 000a7008 beb48e7c
[ 20.649047] dfc0: 001f2ab7 00000004 beb48e7c 00000080 000a47f8 00000000 b6f80000 00000000
[ 20.657684] dfe0: beb48bb8 beb48ba8 00019dfc b6f10020 60000010 b6c49008 00000000 00000000
[ 20.666442] [<bf043288>] (omap_dsshw_probe+0xa4/0x290 [omapdss]) from [<c02c22e4>] (platform_drv_
probe+0x18/0x1c)
[ 20.677276] [<c02c22e4>] (platform_drv_probe+0x18/0x1c) from [<c02c0f28>] (driver_probe_device+0x
9c/0x21c)
[ 20.687499] [<c02c0f28>] (driver_probe_device+0x9c/0x21c) from [<c02c113c>] (__driver_attach+0x94
/0x98)
[ 20.697418] [<c02c113c>] (__driver_attach+0x94/0x98) from [<c02bf6c8>] (bus_for_each_dev+0x50/0x7
c)
[ 20.706970] [<c02bf6c8>] (bus_for_each_dev+0x50/0x7c) from [<c02c05dc>] (bus_add_driver+0xa0/0x2a
8)
[ 20.716522] [<c02c05dc>] (bus_add_driver+0xa0/0x2a8) from [<c02c1670>] (driver_register+0x78/0x17
4)
[ 20.726074] [<c02c1670>] (driver_register+0x78/0x174) from [<c02c26e0>] (platform_driver_probe+0x
18/0x9c)
[ 20.736267] [<c02c26e0>] (platform_driver_probe+0x18/0x9c) from [<bf043124>] (omap_dss_init+0x58/
0x118 [omapdss])
[ 20.747192] [<bf043124>] (omap_dss_init+0x58/0x118 [omapdss]) from [<c0008730>] (do_one_initcall+
0x34/0x194)
[ 20.757568] [<c0008730>] (do_one_initcall+0x34/0x194) from [<c0099d20>] (sys_init_module+0xdc/0x1
cc4)
[ 20.767333] [<c0099d20>] (sys_init_module+0xdc/0x1cc4) from [<c0013d60>] (ret_fast_syscall+0x0/0x
3c)
[ 20.776947] Code: ea00000d e5941248 e3a00000 e584600c (e5911008)
[ 20.783599] ---[ end trace bcb6e89e4ea810ae ]---
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 3/5] video: panel: Add MIPI DBI bus support
From: Tomi Valkeinen @ 2012-08-17 13:06 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <2019849.eCaIrHMssh@avalon>
[-- Attachment #1: Type: text/plain, Size: 6024 bytes --]
On Fri, 2012-08-17 at 14:33 +0200, Laurent Pinchart wrote:
> > But first, the data type should be byte, not unsigned long. How would
> > you write 8 bits or 16 bits with your API?
>
> u8 and u16 both fit in an unsigned long :-) Please see below.
Ah, I see, so the driver would just discard 24 bits or 16 bits from the
ulong. I somehow thought that if you have 8 bit bus, and you call the
write with ulong, 4 bytes will be written.
> > Then again, I'd hope to have DCS somehow as a separate library, which would
> > then use DBI/DSI/whatnot to actually send the data.
> >
> > I'm not quite sure how easy that is because of the differences between
> > the busses.
> >
> > > Is DBI limited to 8-bit data transfers for commands ? Pixels can be
> > > transferred 16-bit at a time, commands might as well. While DCS only
> > > specifies 8-bit command/data, DBI panels that are not DCS compliant can
> > > use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel, does
> > > so).
> >
> > I have to say I don't remember much about DBI =). Looking at OMAP's
> > driver, which was made for omap2 and hasn't been much updated since, I
> > see that there are 4 modes, 8/9/12/16 bits. I think that defines how
> > many of the parallel data lines are used.
>
> SYS-80 also has an 18-bits mode, where bits 0 and 9 are always ignored when
> transferring instructions and data other than pixels (for pixels the 18-bits
> bus width can be used to transfer RGB666 in a single clock cycle).
>
> See page 87 of
> http://read.pudn.com/downloads91/sourcecode/others/348230/e61505_103a.pdf.
>
> > However, I don't think that matters for the panel driver when it wants
> > to send data. The panel driver should just call dbi_write(buf, buf_len),
> > and the dbi driver would send the data in the buffer according to the
> > bus width.
>
> According to the DCS specification, commands and parameters are transferred
> using 8-bit data. Non-DCS panels can however use wider commands and parameters
> (all commands and parameters are 16-bits wide for the R61505 for instance).
>
> We can add an API to switch the DBI bus width on the fly. For Renesas hardware
> this would "just" require shifting bits around to output the 8-bit or 16-bit
> commands on the right data lines (the R61505 uses D17-D9 in 8-bit mode, while
> the DCS specification mentions D7-D0) based on how the panel is connected and
> on which lines the panel expects data.
>
> As commands can be expressed on either 8 or 16 bits I would use a 16 type for
> them.
I won't put my head on the block, but I don't think DBI has any
restriction on the size of the command. A "command" just means a data
transfer while keeping the D/CX line low, and "data" when the line is
high. Similar transfers for n bytes can be done in both modes.
> For parameters, we can either express everything as u8 * in the DBI bus
> operations, or use a union similar to i2c_smbus_data
>
> union i2c_smbus_data {
> __u8 byte;
> __u16 word;
> __u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
> /* and one more for user-space compatibility */
> };
There's no DBI_BLOCK_MAX, so at least identical union won't work. I
think it's simplest to have u8 * function as a base, and then a few
helpers to write the most common datatypes.
So we could have on the lowest level something like:
dbi_write_command(u8 *buf, size_t size);
dbi_write_data(u8 *buf, size_t size);
And possible helpers:
dbi_write_data(u8 *cmd_buf, size_t cmd_size, u8 *data_buf, size_t
data_size);
dbi_write_dcs(u8 cmd, u8 *data, size_t size);
And variations:
dbi_write_dcs_0(u8 cmd);
dbi_write_dcs_1(u8 cmd, u8 data);
etc. So a simple helper to send 16 bits would be:
dbi_write_data(u16 data)
{
// or are the bytes the other way around...
u8 buf[2] = { data & 0xff, (data >> 8) & 0xff };
return dbi_write_data(buf, 2);
}
> Helper functions would be available to perform 8-bit, 16-bit or n*8 bits
> transfers.
>
> Would that work for your use cases ?
>
> > Also note that some chips need to change the bus width on the fly. The
> > chip used on N800 wants configuration to be done with 8-bits, and pixel
> > transfers with 16-bits. Who knows why...
>
> On which data lines is configuration performed ? D7-D0 ?
I guess so, but does it matter? All the bus driver needs to know is how
to send 8/16/.. bit data. On OMAP we just write the data to a 32 bit
register, and the HW takes the lowest n bits. Do the bits represent the
data lines directly on Renesans?
The omap driver actually only implements 8 and 16 bit modes, not the 9
and 12 bit modes. I'm not sure what kind of shifting is needed for
those.
> > > We might just need to provide fake timings. Video mode timings are at the
> > > core of display support in all drivers so we can't just get rid of them.
> > > The h/v front/back porch and sync won't be used by display drivers for
> > > DBI/DSI panels anyway.
> >
> > Right. But we should probably think if we can, at the panel level, easily
> > separate conventional panels and smart panels. Then this framework wouldn't
> > need to fake the timings, and it'd be up to the higher level to decide if
> > and how to fake them. Then again, this is no biggie. Just thought that at
> > the lowest level it'd be nice to be "correct" and leave faking to upper
> > layers =).
>
> But we would then have two different APIs at the lower level depending on the
> panel type. I'm not sure that's a good thing.
Different API for what? Why anyway need panel type specific functions.
In the panel struct we could just have an union of the different types
of parameters for different types of panels.
But if this complicates things, it's not a biggie. Just something that
has been in my mind when dealing with smart panels and assigning dummy
video timings for them =).
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] OMAPDSS: APPLY: Remove omap_dss_device references in wait_for_go functions
From: Tomi Valkeinen @ 2012-08-17 12:35 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345200551-28712-3-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3338 bytes --]
On Fri, 2012-08-17 at 16:19 +0530, Archit Taneja wrote:
> The functions dss_mgr_wait_for_go() and dss_mgr_wait_for_go_ovl() check if there
> is an enabled display connected to the manager before trying to see the state of
> the GO bit.
>
> The checks related to the display can be replaced by checking the state of the
> manager, i.e, whether the manager is enabled or not. This makes more sense than
> checking with the connected display as the GO bit behaviour is more connected
> with the manager state rather than the display state. A GO bit can only be set
> if the manager is enabled. If a manager isn't enabled, we can safely assume that
> the GO bit is not set.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/video/omap2/dss/apply.c | 32 +++++++++++++++++++++-----------
> 1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index 52a5940..74f1a58 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -424,17 +424,23 @@ static void wait_pending_extra_info_updates(void)
> int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr)
> {
> unsigned long timeout = msecs_to_jiffies(500);
> - struct mgr_priv_data *mp;
> + struct mgr_priv_data *mp = get_mgr_priv(mgr);
> u32 irq;
> + unsigned long flags;
> int r;
> int i;
> - struct omap_dss_device *dssdev = mgr->device;
>
> - if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
> + if (mgr_manual_update(mgr))
This needs to be inside the spinlock also.
> return 0;
>
> - if (mgr_manual_update(mgr))
> + spin_lock_irqsave(&data_lock, flags);
> +
> + if (!mp->enabled) {
> + spin_unlock_irqrestore(&data_lock, flags);
> return 0;
> + }
> +
> + spin_unlock_irqrestore(&data_lock, flags);
>
> r = dispc_runtime_get();
> if (r)
> @@ -442,10 +448,8 @@ int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr)
>
> irq = dispc_mgr_get_vsync_irq(mgr->id);
>
> - mp = get_mgr_priv(mgr);
> i = 0;
> while (1) {
> - unsigned long flags;
> bool shadow_dirty, dirty;
>
> spin_lock_irqsave(&data_lock, flags);
> @@ -489,21 +493,28 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl)
> {
> unsigned long timeout = msecs_to_jiffies(500);
> struct ovl_priv_data *op;
> - struct omap_dss_device *dssdev;
> + struct mgr_priv_data *mp;
> u32 irq;
> + unsigned long flags;
> int r;
> int i;
>
> if (!ovl->manager)
> return 0;
And this should be inside spinlock (yes, you didn't change that, but now
that you're changing these... =)
> - dssdev = ovl->manager->device;
> + mp = get_mgr_priv(ovl->manager);
>
> - if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
> + if (ovl_manual_update(ovl))
Inside spinlock here too.
Actually, shouldn't the whole wait_for functions be locked with the
apply mutex? Otherwise the output can be disabled/changed while waiting.
On the other hand, that could be quite a long lock, and I don't see
anything in the code that could really break if the output is disabled
or similar. Perhaps it's fine to just hit the timeout in case something
has been changed. If we add a mutex, we risk breaking something that
currently works =).
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 3/5] video: panel: Add MIPI DBI bus support
From: Laurent Pinchart @ 2012-08-17 12:33 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <1345200709.11073.27.camel@lappyti>
Hi Tomi,
On Friday 17 August 2012 13:51:49 Tomi Valkeinen wrote:
> On Fri, 2012-08-17 at 12:02 +0200, Laurent Pinchart wrote:
> > On Friday 17 August 2012 12:03:02 Tomi Valkeinen wrote:
> > > On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote:
> > > > +/*
> > > > ----------------------------------------------------------------------
> > > > ---
> > > > ---- + * Bus operations
> > > > + */
> > > > +
> > > > +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned
> > > > long
> > > > cmd) +{
> > > > + dev->bus->ops->write_command(dev->bus, cmd);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(panel_dbi_write_command);
> > > > +
> > > > +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long
> > > > data) +{
> > > > + dev->bus->ops->write_data(dev->bus, data);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(panel_dbi_write_data);
> > > > +
> > > > +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev)
> > > > +{
> > > > + return dev->bus->ops->read_data(dev->bus);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(panel_dbi_read_data);
> > >
> > > I'm not that familiar with how to implement bus drivers, can you
> > > describe in pseudo code how the SoC's DBI driver would register these?
> >
> > Sure.
> >
> > The DBI bus driver first needs to create a panel_dbi_bus_ops instance:
> >
> > static const struct panel_dbi_bus_ops sh_mobile_lcdc_dbi_bus_ops = {
> >
> > .write_command = lcdc_dbi_write_command,
> > .write_data = lcdc_dbi_write_data,
> > .read_data = lcdc_dbi_read_data,
> >
> > };
>
> Thanks for the example, I think it cleared up things a bit.
>
> As I mentioned earlier, I really think "panel" is not right here. While
> the whole framework may be called panel framework, the bus drivers are
> not panels, and we should support external chips also, which are not
> panels either.
I agree. I've renamed panel_dbi_* to mipi_dbi_*.
> > > I think write/read data functions are a bit limited. Shouldn't they be
> > > something like write_data(const u8 *buf, int size) and read_data(u8
> > > *buf, int len)?
> >
> > Good point. My hardware doesn't support multi-byte read/write operations
> > directly so I haven't thought about adding those.
>
> OMAP HW doesn't support it either. Well, not quite true, as OMAP's
> system DMA could be used to write a buffer to the DBI output. But that's
> really the same as doing the write with a a loop with CPU.
>
> But first, the data type should be byte, not unsigned long. How would
> you write 8 bits or 16 bits with your API?
u8 and u16 both fit in an unsigned long :-) Please see below.
> And second, if the function takes just u8, you'd need lots of calls to do
> simple writes.
I agree, an array write function is a good idea.
> > Can your hardware group command + data writes in a single operation ? If
> > so we should expose that at the API level as well.
>
> No it can't. But with DCS that is a common operation, so we could have
> some helpers to send command + data with one call.
Agreed.
> Then again, I'd hope to have DCS somehow as a separate library, which would
> then use DBI/DSI/whatnot to actually send the data.
>
> I'm not quite sure how easy that is because of the differences between
> the busses.
>
> > Is DBI limited to 8-bit data transfers for commands ? Pixels can be
> > transferred 16-bit at a time, commands might as well. While DCS only
> > specifies 8-bit command/data, DBI panels that are not DCS compliant can
> > use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel, does
> > so).
>
> I have to say I don't remember much about DBI =). Looking at OMAP's
> driver, which was made for omap2 and hasn't been much updated since, I
> see that there are 4 modes, 8/9/12/16 bits. I think that defines how
> many of the parallel data lines are used.
SYS-80 also has an 18-bits mode, where bits 0 and 9 are always ignored when
transferring instructions and data other than pixels (for pixels the 18-bits
bus width can be used to transfer RGB666 in a single clock cycle).
See page 87 of
http://read.pudn.com/downloads91/sourcecode/others/348230/e61505_103a.pdf.
> However, I don't think that matters for the panel driver when it wants
> to send data. The panel driver should just call dbi_write(buf, buf_len),
> and the dbi driver would send the data in the buffer according to the
> bus width.
According to the DCS specification, commands and parameters are transferred
using 8-bit data. Non-DCS panels can however use wider commands and parameters
(all commands and parameters are 16-bits wide for the R61505 for instance).
We can add an API to switch the DBI bus width on the fly. For Renesas hardware
this would "just" require shifting bits around to output the 8-bit or 16-bit
commands on the right data lines (the R61505 uses D17-D9 in 8-bit mode, while
the DCS specification mentions D7-D0) based on how the panel is connected and
on which lines the panel expects data.
As commands can be expressed on either 8 or 16 bits I would use a 16 type for
them.
For parameters, we can either express everything as u8 * in the DBI bus
operations, or use a union similar to i2c_smbus_data
union i2c_smbus_data {
__u8 byte;
__u16 word;
__u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
/* and one more for user-space compatibility */
};
Helper functions would be available to perform 8-bit, 16-bit or n*8 bits
transfers.
Would that work for your use cases ?
> Also note that some chips need to change the bus width on the fly. The
> chip used on N800 wants configuration to be done with 8-bits, and pixel
> transfers with 16-bits. Who knows why...
On which data lines is configuration performed ? D7-D0 ?
> So I think this, and generally most of the configuration, should be
> somewhat dynamic, so that the panel driver can change them when it
> needs.
>
> > > Something that's totally missing is configuring the DBI bus. There are a
> > > bunch of timing related values that need to be configured. See
> > > include/video/omapdss.h struct rfbi_timings. While the struct is OMAP
> > > specific, if I recall right most of the values match to the MIPI DBI
> > > spec.
> >
> > I've left that out currently, and thought about passing that information
> > as platform data to the DBI bus driver. That's the easiest solution, but I
> > agree that it's a hack. Panel should expose their timing requirements to
> > the DBI host. API wise that wouldn't be difficult (we only need to add
> > timing information to the panel platform data and add a function to the
> > DBI API to retrieve it), one of challenges might be to express it in a
> > way that's both universal enough and easy to use for DBI bus drivers.
>
> As I pointed above, I think the panel driver shouldn't expose it, but
> the panel driver should somehow set it. Or at least allowed to change it
> in some manner. This is actually again, the same problem as with enable
> and transfer: who controls what's going on.
>
> How I think it should work is something like:
>
> mipi_dbi_set_timings(dbi_dev, mytimings);
> mipi_dbi_set_bus_width(dbi_dev, 8);
> mipi_dbi_write(dbi_dev, ...);
> mipi_dbi_set_bus_width(dbi_dev, 16);
> start_frame_transfer(dbi_dev, ...);
I'll first implement bus width setting.
> > > And this makes me wonder, you use DBI bus for SYS-80 panel. The busses
> > > may look similar in operation, but are they really so similar when you
> > > take into account the timings (and perhaps something else, it's been
> > > years since I read the MIPI DBI spec)?
> >
> > I'll have to check all the details. SYS-80 is similar to DBI-B, but
> > supports a wider bus width of 18 bits. I think the interfaces are similar
> > enough to use a single bus implementation, possibly with quirks and/or
> > options (see SCCB support in I2C for instance, with flags to ignore acks,
> > force a stop bit generation, ...). We would duplicate lots of code if we
> > had two different implementations, and would prevent a DBI panel to be
> > attached to a SYS-80 host and vice-versa (the format is known to work).
>
> Ah ok, if a DBI panel can be connected to SYS-80 output and vice versa,
> then I agree they are similar enough.
Not all combination will work (a SYS panel that requires 16-bit transfers
won't work with a DBI host that only supports 8-bit), but some do.
> > We might just need to provide fake timings. Video mode timings are at the
> > core of display support in all drivers so we can't just get rid of them.
> > The h/v front/back porch and sync won't be used by display drivers for
> > DBI/DSI panels anyway.
>
> Right. But we should probably think if we can, at the panel level, easily
> separate conventional panels and smart panels. Then this framework wouldn't
> need to fake the timings, and it'd be up to the higher level to decide if
> and how to fake them. Then again, this is no biggie. Just thought that at
> the lowest level it'd be nice to be "correct" and leave faking to upper
> layers =).
But we would then have two different APIs at the lower level depending on the
panel type. I'm not sure that's a good thing.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC 0/5] Generic panel framework
From: Tomi Valkeinen @ 2012-08-17 11:42 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <15644929.x7ZB0fPYJx@avalon>
[-- Attachment #1: Type: text/plain, Size: 4644 bytes --]
On Fri, 2012-08-17 at 13:10 +0200, Laurent Pinchart wrote:
> What kind of directory structure do you have in mind ? Panels are already
> isolated in drivers/video/panel/ so we could already ditch the panel- prefix
> in drivers.
The same directory also contains files for the framework and buses. But
perhaps there's no need for additional directories if the amount of
non-panel files is small. And you can easily see from the name that they
are not panel drivers (e.g. mipi_dbi_bus.c).
> Would you also create include/video/panel/ ?
Perhaps that would be good. Well, having all the files prefixed with
panel- is not bad as such, but just feel extra.
> > ---
> >
> > Should we aim for DT only solution from the start? DT is the direction we
> > are going, and I feel the older platform data stuff would be deprecated
> > soon.
>
> Don't forget about non-ARM architectures :-/ We need panel drivers for SH as
> well, which doesn't use DT. I don't think that would be a big issue, a DT-
> compliant solution should be easy to use through board code and platform data
> as well.
I didn't forget about them as I didn't even think about them ;). I
somehow had the impression that other architectures would also use DT,
sooner or later. I could be mistaken, though.
And true, it's not a big issue to support both DT and non-DT versions,
but I've been porting omap stuff for DT and keeping the old platform
data stuff also there, and it just feels burdensome. For very simple
panels it's easy, but when you've passing lots of parameters the code
starts to get longer.
> > This one would be rather impossible with the upper layer handling the
> > enabling of the video stream. Thus I see that the panel driver needs to
> > control the sequences, and the Sharp panel driver's enable would look
> > something like:
> >
> > regulator_enable(...);
> > sleep();
> > dpi_enable_video();
> > sleep();
> > gpip_set(..);
>
> I have to admit I have no better solution to propose at the moment, even if I
> don't really like making the panel control the video stream. When several
> devices will be present in the chain all of them might have similar annoying
> requirements, and my feeling is that the resulting code will be quite messy.
> At the end of the day the only way to really find out is to write an
> implementation.
If we have a chain of devices, and each device uses the bus interface
from the previous device in the chain, there shouldn't be a problem. In
that model each device can handle the task however is best for it.
I think the problems come from the divided control we'll have. I mean,
if the panel driver would decide itself what to send to its output, and
it would provide the data (think of an i2c device), this would be very
simple. And it actually is for things like configuration data etc, but
not so for video stream.
> > It could cause some locking issues, though. First the panel's remove
> > could take a lock, but the remove sequence would cause the display
> > driver to call disable on the panel, which could again try to take the
> > same lock...
>
> We have two possible ways of calling panel operations, either directly (panel-
> >bus->ops->enable(...)) or indirectly (panel_enable(...)).
>
> The former is what V4L2 currently does with subdevs, and requires display
> drivers to hold a reference to the panel. The later can do without a direct
> reference only if we use a global lock, which is something I would like to
Wouldn't panel_enable() just do the same panel->bus->ops->enable()
anyway, and both require a panel reference? I don't see the difference.
> avoid. A panel-wide lock wouldn't work, as the access function would need to
> take the lock on a panel instance that can be removed at any time.
Can't this be handled with some kind of get/put refcounting? If there's
a ref, it can't be removed.
Generally about locks, if we define that panel ops may only be called
exclusively, does it simplify things? I think we can make such
requirements, as there should be only one display framework that handles
the panel. Then we don't need locking for things like enable/disable.
Of course we need to be careful about things where calls come from
"outside" the display framework. I guess one such thing is rmmod, but if
that causes a notification to the display framework, which again handles
locking, it shouldn't be a problem.
Another thing to be careful about is if the panel internally uses irqs,
workqueues, sysfs files or such. In that case it needs to handle
locking.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 0/5] Generic panel framework
From: Laurent Pinchart @ 2012-08-17 11:10 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <1345192694.3158.49.camel@deskari>
Hi Tomi,
Thanks a lot for the review.
On Friday 17 August 2012 11:38:14 Tomi Valkeinen wrote:
> On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote:
> > I will appreciate all reviews, comments, criticisms, ideas, remarks, ...
> > If
>
> Oookay, where to start... ;)
>
> A few cosmetic/general comments first.
>
> I find the file naming a bit strange. You have panel.c, which is the
> core framework, panel-dbi.c, which is the DBI bus, panel-r61517.c, which
> is driver for r61517 panel...
>
> Perhaps something in this direction (in order): panel-core.c,
> mipi-dbi-bus.c, panel-r61517.c?
That looks good to me. I'll then rename panel_dbi_* to mipi_dbi_*.
> And we probably end up with quite a lot of panel drivers, perhaps we should
> already divide these into separate directories, and then we wouldn't need to
> prefix each panel with "panel-" at all.
What kind of directory structure do you have in mind ? Panels are already
isolated in drivers/video/panel/ so we could already ditch the panel- prefix
in drivers.
Would you also create include/video/panel/ ?
> ---
>
> Should we aim for DT only solution from the start? DT is the direction we
> are going, and I feel the older platform data stuff would be deprecated
> soon.
Don't forget about non-ARM architectures :-/ We need panel drivers for SH as
well, which doesn't use DT. I don't think that would be a big issue, a DT-
compliant solution should be easy to use through board code and platform data
as well.
> ---
>
> Something missing from the intro is how this whole thing should be used.
> It doesn't help if we know how to turn on the panel, we also need to
> display something on it =). So I think some kind of diagram/example of
> how, say, drm would use this thing, and also how the SoC specific DBI
> bus driver would be done, would clarify things.
Of course. If I had all that information already I would have shared it :-)
This is really a first RFC, my goal is to make sure that I'm going in the
right direction.
> ---
>
> We have discussed face to face about the different hardware setups and
> scenarios that we should support, but I'll list some of them here for
> others:
>
> 1) We need to support chains of external display chips and panels. A
> simple example is a chip that takes DSI in, and outputs DPI. In that
> case we'd have a chain of SoC -> DSI2DPI -> DPI panel.
>
> In final products I think two external devices is the maximum (at least
> I've never seen three devices in a row), but in theory and in
> development environments the chain can be arbitrarily long. Also the
> connections are not necessarily 1-to-1, but a device can take one input
> while it has two outputs, or a device can take two inputs.
>
> Now, I think two external devices is a must requirement. I'm not sure if
> supporting more is an important requirement. However, if we support two
> devices, it could be that it's trivial to change the framework to
> support n devices.
>
> 2) Panels and display chips are all but standard. They very often have
> their own sequences how to do things, have bugs, or implement some
> feature in slightly different way than some other panel. This is why the
> panel driver should be able to control or define the way things happen.
>
> As an example, Sharp LQ043T1DG01 panel
> (www.sharpsme.com/download/LQ043T1DG01-SP-072106pdf). It is enabled with
> the following sequence:
>
> - Enable VCC and AVDD regulators
> - Wait min 50ms
> - Enable full video stream (pck, syncs, pixels) from SoC
> - Wait min 0.5ms
> - Set DISP GPIO, which turns on the display panel
>
> Here we could split the enabling of panel to two parts, prepare (in this
> case starts regulators and waits 50ms) and finish (wait 0.5ms and set
> DISP GPIO), and the upper layer would start the video stream in between.
>
> I realize this could be done with the PANEL_ENABLE_* levels in your RFC,
> but I don't think the concepts quite match:
>
> - PANEL_ENABLE_BLANK level is needed for "smart panels", as we need to
> configure them and send the initial frame at that operating level. With
> dummy panels there's really no such level, there's just one enable
> sequence that is always done right away.
>
> - I find waiting at the beginning of a function very ugly (what are we
> waiting for?) and we'd need that when changing the panel to
> PANEL_ENABLE_ON level.
>
> - It's still limited if the panel is a stranger one (see following
> example).
>
> Consider the following theoretical panel enable example, taken to absurd
> level just to show the general problem:
>
> - Enable regulators
> - Enable video stream
> - Wait 50ms
> - Disable video stream
> - Set enable GPIO
> - Enable video stream
>
> This one would be rather impossible with the upper layer handling the
> enabling of the video stream. Thus I see that the panel driver needs to
> control the sequences, and the Sharp panel driver's enable would look
> something like:
>
> regulator_enable(...);
> sleep();
> dpi_enable_video();
> sleep();
> gpip_set(..);
I have to admit I have no better solution to propose at the moment, even if I
don't really like making the panel control the video stream. When several
devices will be present in the chain all of them might have similar annoying
requirements, and my feeling is that the resulting code will be quite messy.
At the end of the day the only way to really find out is to write an
implementation.
> Note that even with this model we still need the PANEL_ENABLE levels you
> have.
>
> ---
>
> I'm not sure I understand the panel unload problem you mentioned. Nobody
> should have direct references to the panel functions, so there shouldn't
> be any automatic references that would prevent module unloading. So when
> the user does rmmod panel-mypanel, the panel driver's remove will be
> called. It'll unregister itself from the panel framework, which causes
> notifications and the display driver will stop using the panel. After
> that nobody has pointers to the panel, and it can safely be unloaded.
>
> It could cause some locking issues, though. First the panel's remove
> could take a lock, but the remove sequence would cause the display
> driver to call disable on the panel, which could again try to take the
> same lock...
We have two possible ways of calling panel operations, either directly (panel-
>bus->ops->enable(...)) or indirectly (panel_enable(...)).
The former is what V4L2 currently does with subdevs, and requires display
drivers to hold a reference to the panel. The later can do without a direct
reference only if we use a global lock, which is something I would like to
avoid. A panel-wide lock wouldn't work, as the access function would need to
take the lock on a panel instance that can be removed at any time.
Note that this issue is not specific to panels, V4L2 will need a solution as
well when V4L2 subdevs will be instantiated from the DT.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC 3/5] video: panel: Add MIPI DBI bus support
From: Tomi Valkeinen @ 2012-08-17 10:51 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <1682445.1yJVVY1ksn@avalon>
[-- Attachment #1: Type: text/plain, Size: 7009 bytes --]
On Fri, 2012-08-17 at 12:02 +0200, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the review.
>
> On Friday 17 August 2012 12:03:02 Tomi Valkeinen wrote:
> > On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote:
> > > +/*
> > > -------------------------------------------------------------------------
> > > ---- + * Bus operations
> > > + */
> > > +
> > > +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned long
> > > cmd) +{
> > > + dev->bus->ops->write_command(dev->bus, cmd);
> > > +}
> > > +EXPORT_SYMBOL_GPL(panel_dbi_write_command);
> > > +
> > > +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long
> > > data) +{
> > > + dev->bus->ops->write_data(dev->bus, data);
> > > +}
> > > +EXPORT_SYMBOL_GPL(panel_dbi_write_data);
> > > +
> > > +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev)
> > > +{
> > > + return dev->bus->ops->read_data(dev->bus);
> > > +}
> > > +EXPORT_SYMBOL_GPL(panel_dbi_read_data);
> >
> > I'm not that familiar with how to implement bus drivers, can you
> > describe in pseudo code how the SoC's DBI driver would register these?
>
> Sure.
>
> The DBI bus driver first needs to create a panel_dbi_bus_ops instance:
>
> static const struct panel_dbi_bus_ops sh_mobile_lcdc_dbi_bus_ops = {
> .write_command = lcdc_dbi_write_command,
> .write_data = lcdc_dbi_write_data,
> .read_data = lcdc_dbi_read_data,
> };
Thanks for the example, I think it cleared up things a bit.
As I mentioned earlier, I really think "panel" is not right here. While
the whole framework may be called panel framework, the bus drivers are
not panels, and we should support external chips also, which are not
panels either.
> > I think write/read data functions are a bit limited. Shouldn't they be
> > something like write_data(const u8 *buf, int size) and read_data(u8
> > *buf, int len)?
>
> Good point. My hardware doesn't support multi-byte read/write operations
> directly so I haven't thought about adding those.
OMAP HW doesn't support it either. Well, not quite true, as OMAP's
system DMA could be used to write a buffer to the DBI output. But that's
really the same as doing the write with a a loop with CPU.
But first, the data type should be byte, not unsigned long. How would
you write 8 bits or 16 bits with your API? And second, if the function
takes just u8, you'd need lots of calls to do simple writes.
> Can your hardware group command + data writes in a single operation ? If so we
> should expose that at the API level as well.
No it can't. But with DCS that is a common operation, so we could have
some helpers to send command + data with one call. Then again, I'd hope
to have DCS somehow as a separate library, which would then use
DBI/DSI/whatnot to actually send the data.
I'm not quite sure how easy that is because of the differences between
the busses.
> Is DBI limited to 8-bit data transfers for commands ? Pixels can be
> transferred 16-bit at a time, commands might as well. While DCS only specifies
> 8-bit command/data, DBI panels that are not DCS compliant can use 16-bit
> command/data (the R61505 panel, albeit a SYS-80 panel, does so).
I have to say I don't remember much about DBI =). Looking at OMAP's
driver, which was made for omap2 and hasn't been much updated since, I
see that there are 4 modes, 8/9/12/16 bits. I think that defines how
many of the parallel data lines are used.
However, I don't think that matters for the panel driver when it wants
to send data. The panel driver should just call dbi_write(buf, buf_len),
and the dbi driver would send the data in the buffer according to the
bus width.
Also note that some chips need to change the bus width on the fly. The
chip used on N800 wants configuration to be done with 8-bits, and pixel
transfers with 16-bits. Who knows why...
So I think this, and generally most of the configuration, should be
somewhat dynamic, so that the panel driver can change them when it
needs.
> > Something that's totally missing is configuring the DBI bus. There are a
> > bunch of timing related values that need to be configured. See
> > include/video/omapdss.h struct rfbi_timings. While the struct is OMAP
> > specific, if I recall right most of the values match to the MIPI DBI
> > spec.
>
> I've left that out currently, and thought about passing that information as
> platform data to the DBI bus driver. That's the easiest solution, but I agree
> that it's a hack. Panel should expose their timing requirements to the DBI
> host. API wise that wouldn't be difficult (we only need to add timing
> information to the panel platform data and add a function to the DBI API to
> retrieve it), one of challenges might be to express it in a way that's both
> universal enough and easy to use for DBI bus drivers.
As I pointed above, I think the panel driver shouldn't expose it, but
the panel driver should somehow set it. Or at least allowed to change it
in some manner. This is actually again, the same problem as with enable
and transfer: who controls what's going on.
How I think it should work is something like:
mipi_dbi_set_timings(dbi_dev, mytimings);
mipi_dbi_set_bus_width(dbi_dev, 8);
mipi_dbi_write(dbi_dev, ...);
mipi_dbi_set_bus_width(dbi_dev, 16);
start_frame_transfer(dbi_dev, ...);
> > And this makes me wonder, you use DBI bus for SYS-80 panel. The busses
> > may look similar in operation, but are they really so similar when you
> > take into account the timings (and perhaps something else, it's been
> > years since I read the MIPI DBI spec)?
>
> I'll have to check all the details. SYS-80 is similar to DBI-B, but supports a
> wider bus width of 18 bits. I think the interfaces are similar enough to use a
> single bus implementation, possibly with quirks and/or options (see SCCB
> support in I2C for instance, with flags to ignore acks, force a stop bit
> generation, ...). We would duplicate lots of code if we had two different
> implementations, and would prevent a DBI panel to be attached to a SYS-80 host
> and vice-versa (the format is known to work).
Ah ok, if a DBI panel can be connected to SYS-80 output and vice versa,
then I agree they are similar enough.
> We might just need to provide fake timings. Video mode timings are at the core
> of display support in all drivers so we can't just get rid of them. The h/v
> front/back porch and sync won't be used by display drivers for DBI/DSI panels
> anyway.
Right. But we should probably think if we can, at the panel level,
easily separate conventional panels and smart panels. Then this
framework wouldn't need to fake the timings, and it'd be up to the
higher level to decide if and how to fake them. Then again, this is no
biggie. Just thought that at the lowest level it'd be nice to be
"correct" and leave faking to upper layers =).
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH 3/3] OMAPDSS: Remove unnecessary acb/acbi pin fields from omap_dss_device
From: Archit Taneja @ 2012-08-17 10:51 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <1345200551-28712-1-git-send-email-archit@ti.com>
Passive matrix support was removed recently. The acb and acbi pin declarations
in omap_dss_device struct weren't removed by accident. Remove these fields
from omap_dss_device.
Signed-off-by: Archit Taneja <archit@ti.com>
---
include/video/omapdss.h | 4 ----
1 file changed, 4 deletions(-)
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index b868123..bc686f4 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -558,10 +558,6 @@ struct omap_dss_device {
struct {
struct omap_video_timings timings;
- int acbi; /* ac-bias pin transitions per interrupt */
- /* Unit: line clocks */
- int acb; /* ac-bias pin frequency */
-
enum omap_dss_dsi_pixel_format dsi_pix_fmt;
enum omap_dss_dsi_mode dsi_mode;
struct omap_dss_dsi_videomode_timings dsi_vm_timings;
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/3] OMAPDSS: APPLY: Remove omap_dss_device references in wait_for_go functions
From: Archit Taneja @ 2012-08-17 10:51 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <1345200551-28712-1-git-send-email-archit@ti.com>
The functions dss_mgr_wait_for_go() and dss_mgr_wait_for_go_ovl() check if there
is an enabled display connected to the manager before trying to see the state of
the GO bit.
The checks related to the display can be replaced by checking the state of the
manager, i.e, whether the manager is enabled or not. This makes more sense than
checking with the connected display as the GO bit behaviour is more connected
with the manager state rather than the display state. A GO bit can only be set
if the manager is enabled. If a manager isn't enabled, we can safely assume that
the GO bit is not set.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/apply.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 52a5940..74f1a58 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -424,17 +424,23 @@ static void wait_pending_extra_info_updates(void)
int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr)
{
unsigned long timeout = msecs_to_jiffies(500);
- struct mgr_priv_data *mp;
+ struct mgr_priv_data *mp = get_mgr_priv(mgr);
u32 irq;
+ unsigned long flags;
int r;
int i;
- struct omap_dss_device *dssdev = mgr->device;
- if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
+ if (mgr_manual_update(mgr))
return 0;
- if (mgr_manual_update(mgr))
+ spin_lock_irqsave(&data_lock, flags);
+
+ if (!mp->enabled) {
+ spin_unlock_irqrestore(&data_lock, flags);
return 0;
+ }
+
+ spin_unlock_irqrestore(&data_lock, flags);
r = dispc_runtime_get();
if (r)
@@ -442,10 +448,8 @@ int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr)
irq = dispc_mgr_get_vsync_irq(mgr->id);
- mp = get_mgr_priv(mgr);
i = 0;
while (1) {
- unsigned long flags;
bool shadow_dirty, dirty;
spin_lock_irqsave(&data_lock, flags);
@@ -489,21 +493,28 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl)
{
unsigned long timeout = msecs_to_jiffies(500);
struct ovl_priv_data *op;
- struct omap_dss_device *dssdev;
+ struct mgr_priv_data *mp;
u32 irq;
+ unsigned long flags;
int r;
int i;
if (!ovl->manager)
return 0;
- dssdev = ovl->manager->device;
+ mp = get_mgr_priv(ovl->manager);
- if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
+ if (ovl_manual_update(ovl))
return 0;
- if (ovl_manual_update(ovl))
+ spin_lock_irqsave(&data_lock, flags);
+
+ if (!mp->enabled) {
+ spin_unlock_irqrestore(&data_lock, flags);
return 0;
+ }
+
+ spin_unlock_irqrestore(&data_lock, flags);
r = dispc_runtime_get();
if (r)
@@ -514,7 +525,6 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl)
op = get_ovl_priv(ovl);
i = 0;
while (1) {
- unsigned long flags;
bool shadow_dirty, dirty;
spin_lock_irqsave(&data_lock, flags);
--
1.7.9.5
^ permalink raw reply related
* [PATCH 1/3] OMAPDSS: DSI: Pass dsi platform device wherever possible
From: Archit Taneja @ 2012-08-17 10:51 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <1345200551-28712-1-git-send-email-archit@ti.com>
Many of the DSI functions receive the connected panel's omap_dss_device pointer
as an argument. The platform device pointer is then derived via omap_dss_device
pointers.
Most of these functions don't really require omap_dss_device pointer anymore
since we now keep copies of parameters in the driver data which were previously
available only via omap_dss_device. Replace the arguments with platform device
pointers for such functions.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/dsi.c | 89 ++++++++++++++++++-----------------------
1 file changed, 39 insertions(+), 50 deletions(-)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 96d0024..659b6cd 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -2014,9 +2014,8 @@ static unsigned dsi_get_line_buf_size(struct platform_device *dsidev)
}
}
-static int dsi_set_lane_config(struct omap_dss_device *dssdev)
+static int dsi_set_lane_config(struct platform_device *dsidev)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
static const u8 offsets[] = { 0, 4, 8, 12, 16 };
static const enum dsi_lane_function functions[] = {
@@ -2151,10 +2150,9 @@ static void dsi_cio_timings(struct platform_device *dsidev)
}
/* lane masks have lane 0 at lsb. mask_p for positive lines, n for negative */
-static void dsi_cio_enable_lane_override(struct omap_dss_device *dssdev,
+static void dsi_cio_enable_lane_override(struct platform_device *dsidev,
unsigned mask_p, unsigned mask_n)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
int i;
u32 l;
@@ -2201,9 +2199,8 @@ static void dsi_cio_disable_lane_override(struct platform_device *dsidev)
REG_FLD_MOD(dsidev, DSI_DSIPHY_CFG10, 0, 22, 17);
}
-static int dsi_cio_wait_tx_clk_esc_reset(struct omap_dss_device *dssdev)
+static int dsi_cio_wait_tx_clk_esc_reset(struct platform_device *dsidev)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
int t, i;
bool in_use[DSI_MAX_NR_LANES];
@@ -2251,9 +2248,8 @@ static int dsi_cio_wait_tx_clk_esc_reset(struct omap_dss_device *dssdev)
}
/* return bitmask of enabled lanes, lane0 being the lsb */
-static unsigned dsi_get_lane_mask(struct omap_dss_device *dssdev)
+static unsigned dsi_get_lane_mask(struct platform_device *dsidev)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
unsigned mask = 0;
int i;
@@ -2266,16 +2262,15 @@ static unsigned dsi_get_lane_mask(struct omap_dss_device *dssdev)
return mask;
}
-static int dsi_cio_init(struct omap_dss_device *dssdev)
+static int dsi_cio_init(struct platform_device *dsidev)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
int r;
u32 l;
DSSDBGF();
- r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dssdev));
+ r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dsidev));
if (r)
return r;
@@ -2292,7 +2287,7 @@ static int dsi_cio_init(struct omap_dss_device *dssdev)
goto err_scp_clk_dom;
}
- r = dsi_set_lane_config(dssdev);
+ r = dsi_set_lane_config(dsidev);
if (r)
goto err_scp_clk_dom;
@@ -2327,7 +2322,7 @@ static int dsi_cio_init(struct omap_dss_device *dssdev)
mask_p |= 1 << i;
}
- dsi_cio_enable_lane_override(dssdev, mask_p, 0);
+ dsi_cio_enable_lane_override(dsidev, mask_p, 0);
}
r = dsi_cio_power(dsidev, DSI_COMPLEXIO_POWER_ON);
@@ -2344,7 +2339,7 @@ static int dsi_cio_init(struct omap_dss_device *dssdev)
dsi_if_enable(dsidev, false);
REG_FLD_MOD(dsidev, DSI_CLK_CTRL, 1, 20, 20); /* LP_CLK_ENABLE */
- r = dsi_cio_wait_tx_clk_esc_reset(dssdev);
+ r = dsi_cio_wait_tx_clk_esc_reset(dsidev);
if (r)
goto err_tx_clk_esc_rst;
@@ -2385,13 +2380,12 @@ err_cio_pwr:
dsi_cio_disable_lane_override(dsidev);
err_scp_clk_dom:
dsi_disable_scp_clk(dsidev);
- dss_dsi_disable_pads(dsi->module_id, dsi_get_lane_mask(dssdev));
+ dss_dsi_disable_pads(dsi->module_id, dsi_get_lane_mask(dsidev));
return r;
}
-static void dsi_cio_uninit(struct omap_dss_device *dssdev)
+static void dsi_cio_uninit(struct platform_device *dsidev)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
/* DDR_CLK_ALWAYS_ON */
@@ -2399,7 +2393,7 @@ static void dsi_cio_uninit(struct omap_dss_device *dssdev)
dsi_cio_power(dsidev, DSI_COMPLEXIO_POWER_OFF);
dsi_disable_scp_clk(dsidev);
- dss_dsi_disable_pads(dsi->module_id, dsi_get_lane_mask(dssdev));
+ dss_dsi_disable_pads(dsi->module_id, dsi_get_lane_mask(dsidev));
}
static void dsi_config_tx_fifo(struct platform_device *dsidev,
@@ -2992,10 +2986,9 @@ int dsi_vc_send_null(struct omap_dss_device *dssdev, int channel)
}
EXPORT_SYMBOL(dsi_vc_send_null);
-static int dsi_vc_write_nosync_common(struct omap_dss_device *dssdev,
+static int dsi_vc_write_nosync_common(struct platform_device *dsidev,
int channel, u8 *data, int len, enum dss_dsi_content_type type)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
int r;
if (len = 0) {
@@ -3026,7 +3019,9 @@ static int dsi_vc_write_nosync_common(struct omap_dss_device *dssdev,
int dsi_vc_dcs_write_nosync(struct omap_dss_device *dssdev, int channel,
u8 *data, int len)
{
- return dsi_vc_write_nosync_common(dssdev, channel, data, len,
+ struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+
+ return dsi_vc_write_nosync_common(dsidev, channel, data, len,
DSS_DSI_CONTENT_DCS);
}
EXPORT_SYMBOL(dsi_vc_dcs_write_nosync);
@@ -3034,7 +3029,9 @@ EXPORT_SYMBOL(dsi_vc_dcs_write_nosync);
int dsi_vc_generic_write_nosync(struct omap_dss_device *dssdev, int channel,
u8 *data, int len)
{
- return dsi_vc_write_nosync_common(dssdev, channel, data, len,
+ struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+
+ return dsi_vc_write_nosync_common(dsidev, channel, data, len,
DSS_DSI_CONTENT_GENERIC);
}
EXPORT_SYMBOL(dsi_vc_generic_write_nosync);
@@ -3045,7 +3042,7 @@ static int dsi_vc_write_common(struct omap_dss_device *dssdev, int channel,
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
int r;
- r = dsi_vc_write_nosync_common(dssdev, channel, data, len, type);
+ r = dsi_vc_write_nosync_common(dsidev, channel, data, len, type);
if (r)
goto err;
@@ -3123,10 +3120,9 @@ int dsi_vc_generic_write_2(struct omap_dss_device *dssdev, int channel,
}
EXPORT_SYMBOL(dsi_vc_generic_write_2);
-static int dsi_vc_dcs_send_read_request(struct omap_dss_device *dssdev,
+static int dsi_vc_dcs_send_read_request(struct platform_device *dsidev,
int channel, u8 dcs_cmd)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
int r;
@@ -3144,10 +3140,9 @@ static int dsi_vc_dcs_send_read_request(struct omap_dss_device *dssdev,
return 0;
}
-static int dsi_vc_generic_send_read_request(struct omap_dss_device *dssdev,
+static int dsi_vc_generic_send_read_request(struct platform_device *dsidev,
int channel, u8 *reqdata, int reqlen)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
u16 data;
u8 data_type;
@@ -3296,7 +3291,7 @@ int dsi_vc_dcs_read(struct omap_dss_device *dssdev, int channel, u8 dcs_cmd,
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
int r;
- r = dsi_vc_dcs_send_read_request(dssdev, channel, dcs_cmd);
+ r = dsi_vc_dcs_send_read_request(dsidev, channel, dcs_cmd);
if (r)
goto err;
@@ -3327,7 +3322,7 @@ static int dsi_vc_generic_read(struct omap_dss_device *dssdev, int channel,
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
int r;
- r = dsi_vc_generic_send_read_request(dssdev, channel, reqdata, reqlen);
+ r = dsi_vc_generic_send_read_request(dsidev, channel, reqdata, reqlen);
if (r)
return r;
@@ -3609,14 +3604,12 @@ static void dsi_set_hs_tx_timeout(struct platform_device *dsidev,
(total_ticks * 1000) / (fck / 1000 / 1000));
}
-static void dsi_config_vp_num_line_buffers(struct omap_dss_device *dssdev)
+static void dsi_config_vp_num_line_buffers(struct platform_device *dsidev)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
int num_line_buffers;
if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
- struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
int bpp = dsi_get_pixel_size(dsi->pix_fmt);
unsigned line_buf_size = dsi_get_line_buf_size(dsidev);
struct omap_video_timings *timings = &dsi->timings;
@@ -3637,9 +3630,8 @@ static void dsi_config_vp_num_line_buffers(struct omap_dss_device *dssdev)
REG_FLD_MOD(dsidev, DSI_CTRL, num_line_buffers, 13, 12);
}
-static void dsi_config_vp_sync_events(struct omap_dss_device *dssdev)
+static void dsi_config_vp_sync_events(struct platform_device *dsidev)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
bool vsync_end = dsi->vm_timings.vp_vsync_end;
bool hsync_end = dsi->vm_timings.vp_hsync_end;
@@ -3656,9 +3648,8 @@ static void dsi_config_vp_sync_events(struct omap_dss_device *dssdev)
dsi_write_reg(dsidev, DSI_CTRL, r);
}
-static void dsi_config_blanking_modes(struct omap_dss_device *dssdev)
+static void dsi_config_blanking_modes(struct platform_device *dsidev)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
int blanking_mode = dsi->vm_timings.blanking_mode;
int hfp_blanking_mode = dsi->vm_timings.hfp_blanking_mode;
@@ -3913,11 +3904,11 @@ static int dsi_proto_config(struct omap_dss_device *dssdev)
dsi_write_reg(dsidev, DSI_CTRL, r);
- dsi_config_vp_num_line_buffers(dssdev);
+ dsi_config_vp_num_line_buffers(dsidev);
if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
- dsi_config_vp_sync_events(dssdev);
- dsi_config_blanking_modes(dssdev);
+ dsi_config_vp_sync_events(dsidev);
+ dsi_config_blanking_modes(dsidev);
dsi_config_cmd_mode_interleaving(dssdev);
}
@@ -3929,9 +3920,8 @@ static int dsi_proto_config(struct omap_dss_device *dssdev)
return 0;
}
-static void dsi_proto_timings(struct omap_dss_device *dssdev)
+static void dsi_proto_timings(struct platform_device *dsidev)
{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
unsigned tlpx, tclk_zero, tclk_prepare, tclk_trail;
unsigned tclk_pre, tclk_post;
@@ -4312,8 +4302,7 @@ static void dsi_framedone_timeout_work_callback(struct work_struct *work)
static void dsi_framedone_irq_callback(void *data, u32 mask)
{
- struct omap_dss_device *dssdev = (struct omap_dss_device *) data;
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+ struct platform_device *dsidev = (struct platform_device *) data;
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
/* Note: We get FRAMEDONE when DISPC has finished sending pixels and
@@ -4397,7 +4386,7 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
irq = dispc_mgr_get_framedone_irq(dssdev->manager->id);
r = omap_dispc_register_isr(dsi_framedone_irq_callback,
- (void *) dssdev, irq);
+ (void *) dsidev, irq);
if (r) {
DSSERR("can't get FRAMEDONE irq\n");
goto err;
@@ -4438,7 +4427,7 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
err1:
if (dsi->mode = OMAP_DSS_DSI_CMD_MODE)
omap_dispc_unregister_isr(dsi_framedone_irq_callback,
- (void *) dssdev, irq);
+ (void *) dsidev, irq);
err:
return r;
}
@@ -4454,7 +4443,7 @@ static void dsi_display_uninit_dispc(struct omap_dss_device *dssdev)
irq = dispc_mgr_get_framedone_irq(dssdev->manager->id);
omap_dispc_unregister_isr(dsi_framedone_irq_callback,
- (void *) dssdev, irq);
+ (void *) dsidev, irq);
}
}
@@ -4504,13 +4493,13 @@ static int dsi_display_init_dsi(struct omap_dss_device *dssdev)
DSSDBG("PLL OK\n");
- r = dsi_cio_init(dssdev);
+ r = dsi_cio_init(dsidev);
if (r)
goto err2;
_dsi_print_reset_status(dsidev);
- dsi_proto_timings(dssdev);
+ dsi_proto_timings(dsidev);
dsi_set_lp_clk_divisor(dssdev);
if (1)
@@ -4530,7 +4519,7 @@ static int dsi_display_init_dsi(struct omap_dss_device *dssdev)
return 0;
err3:
- dsi_cio_uninit(dssdev);
+ dsi_cio_uninit(dsidev);
err2:
dss_select_dispc_clk_source(OMAP_DSS_CLK_SRC_FCK);
dss_select_dsi_clk_source(dsi->module_id, OMAP_DSS_CLK_SRC_FCK);
@@ -4561,7 +4550,7 @@ static void dsi_display_uninit_dsi(struct omap_dss_device *dssdev,
dss_select_dispc_clk_source(OMAP_DSS_CLK_SRC_FCK);
dss_select_dsi_clk_source(dsi->module_id, OMAP_DSS_CLK_SRC_FCK);
dss_select_lcd_clk_source(dssdev->manager->id, OMAP_DSS_CLK_SRC_FCK);
- dsi_cio_uninit(dssdev);
+ dsi_cio_uninit(dsidev);
dsi_pll_uninit(dsidev, disconnect_lanes);
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH 0/3] OMAPDSS: Miscellaneous cleanup patches
From: Archit Taneja @ 2012-08-17 10:51 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <343817088-29645-1-git-send-email-archit@ti.com>
These are minor cleanup patches which will be useful for the future series on
outputs. It's harder to add output entities in DSS when there are more
omap_dss_device references in the driver. The first 2 reduces that a bit. The
third patch just removes some left over fields from omap_dss_device
Refenence Tree:
git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git 1-misc-clean-for-outputs
Archit Taneja (3):
OMAPDSS: DSI: Pass dsi platform device wherever possible
OMAPDSS: APPLY: Remove omap_dss_device references in wait_for_go
functions
OMAPDSS: Remove unnecessary acb/acbi pin fields from omap_dss_device
drivers/video/omap2/dss/apply.c | 32 +++++++++-----
drivers/video/omap2/dss/dsi.c | 89 +++++++++++++++++----------------------
include/video/omapdss.h | 4 --
3 files changed, 60 insertions(+), 65 deletions(-)
--
1.7.9.5
^ permalink raw reply
* Re: [RFC 3/5] video: panel: Add MIPI DBI bus support
From: Laurent Pinchart @ 2012-08-17 10:02 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <1345194182.3158.66.camel@deskari>
Hi Tomi,
Thank you for the review.
On Friday 17 August 2012 12:03:02 Tomi Valkeinen wrote:
> On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote:
> > +/*
> > -------------------------------------------------------------------------
> > ---- + * Bus operations
> > + */
> > +
> > +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned long
> > cmd) +{
> > + dev->bus->ops->write_command(dev->bus, cmd);
> > +}
> > +EXPORT_SYMBOL_GPL(panel_dbi_write_command);
> > +
> > +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long
> > data) +{
> > + dev->bus->ops->write_data(dev->bus, data);
> > +}
> > +EXPORT_SYMBOL_GPL(panel_dbi_write_data);
> > +
> > +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev)
> > +{
> > + return dev->bus->ops->read_data(dev->bus);
> > +}
> > +EXPORT_SYMBOL_GPL(panel_dbi_read_data);
>
> I'm not that familiar with how to implement bus drivers, can you
> describe in pseudo code how the SoC's DBI driver would register these?
Sure.
The DBI bus driver first needs to create a panel_dbi_bus_ops instance:
static const struct panel_dbi_bus_ops sh_mobile_lcdc_dbi_bus_ops = {
.write_command = lcdc_dbi_write_command,
.write_data = lcdc_dbi_write_data,
.read_data = lcdc_dbi_read_data,
};
and a panel_dbi_bus instance, usually embedded in its private driver data
structure, and initialize it by setting its dev and ops fields:
ch->dbi_bus.dev = ch->lcdc->dev;
ch->dbi_bus.ops = &sh_mobile_lcdc_dbi_bus_ops;
In my current implementation, the panel_dbi_device is created in board code:
static struct panel_dbi_device migor_panel_device = {
.name = "r61505",
.id = 0,
.dev = {
.platform_data = &migor_panel_info,
},
};
A pointer to that structure is passed to the DBI master driver, which then
registers the device:
panel_dbi_device_register(dbi, &ch->dbi_bus);
With a DT-based solution the DBI core will expose a function to register DBI
devices from OF nodes.
The bus itself is currently not registered with the DBI code because there was
no need to.
> I think write/read data functions are a bit limited. Shouldn't they be
> something like write_data(const u8 *buf, int size) and read_data(u8
> *buf, int len)?
Good point. My hardware doesn't support multi-byte read/write operations
directly so I haven't thought about adding those.
Can your hardware group command + data writes in a single operation ? If so we
should expose that at the API level as well.
Is DBI limited to 8-bit data transfers for commands ? Pixels can be
transferred 16-bit at a time, commands might as well. While DCS only specifies
8-bit command/data, DBI panels that are not DCS compliant can use 16-bit
command/data (the R61505 panel, albeit a SYS-80 panel, does so).
> Something that's totally missing is configuring the DBI bus. There are a
> bunch of timing related values that need to be configured. See
> include/video/omapdss.h struct rfbi_timings. While the struct is OMAP
> specific, if I recall right most of the values match to the MIPI DBI
> spec.
I've left that out currently, and thought about passing that information as
platform data to the DBI bus driver. That's the easiest solution, but I agree
that it's a hack. Panel should expose their timing requirements to the DBI
host. API wise that wouldn't be difficult (we only need to add timing
information to the panel platform data and add a function to the DBI API to
retrieve it), one of challenges might be to express it in a way that's both
universal enough and easy to use for DBI bus drivers.
> And this makes me wonder, you use DBI bus for SYS-80 panel. The busses
> may look similar in operation, but are they really so similar when you
> take into account the timings (and perhaps something else, it's been
> years since I read the MIPI DBI spec)?
I'll have to check all the details. SYS-80 is similar to DBI-B, but supports a
wider bus width of 18 bits. I think the interfaces are similar enough to use a
single bus implementation, possibly with quirks and/or options (see SCCB
support in I2C for instance, with flags to ignore acks, force a stop bit
generation, ...). We would duplicate lots of code if we had two different
implementations, and would prevent a DBI panel to be attached to a SYS-80 host
and vice-versa (the format is known to work).
> Then there's the start_transfer. This is something I'm not sure what is
> the best way to handle it, but the same problems that I mentioned in the
> previous post related to enable apply here also. For example, what if
> the panel needs to be update in two parts? This is done in Nokia N9.
> From panel's perspective, it'd be best to handle it somewhat like this
> (although asynchronously, probably):
>
> write_update_area(0, 0, xres, yres / 2);
> write_memory_start()
> start_pixel_transfer();
>
> wait_transfer_done();
>
> write_update_area(0, yres / 2, xres, yres / 2);
> write_memory_start()
> start_pixel_transfer();
>
> Why I said I'm not sure about this is that it does complicate things, as
> the actual pixel data often comes from the display subsystem hardware,
> which should probably be controlled by the display driver.
I have no solution for this at the moment. That's an advanced (but definitely
required) feature, I've tried to concentrate on the basics first.
> I think there also needs to be some kind of transfer_done notifier, for
> both the display driver and the panel driver. Although if the display
> driver handles starting the actual pixel transfer, then it'll get the
> transfer_done via whatever interrupt the SoC has.
>
> Also as food for thought, videomode timings does not really make sense
> for DBI panels, at least when you just consider the DBI side. There's
> really just the resolution of the display, and then the DBI timings. No
> pck, syncs, etc. Of course in the end there's the actual panel, which
> does have these video timings. But often they cannot be configured, and
> often you don't even know them as the specs don't tell them.
We might just need to provide fake timings. Video mode timings are at the core
of display support in all drivers so we can't just get rid of them. The h/v
front/back porch and sync won't be used by display drivers for DBI/DSI panels
anyway.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC 3/5] video: panel: Add MIPI DBI bus support
From: Tomi Valkeinen @ 2012-08-17 9:03 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <1345164583-18924-4-git-send-email-laurent.pinchart@ideasonboard.com>
[-- Attachment #1: Type: text/plain, Size: 3118 bytes --]
On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote:
> +/* -----------------------------------------------------------------------------
> + * Bus operations
> + */
> +
> +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned long cmd)
> +{
> + dev->bus->ops->write_command(dev->bus, cmd);
> +}
> +EXPORT_SYMBOL_GPL(panel_dbi_write_command);
> +
> +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long data)
> +{
> + dev->bus->ops->write_data(dev->bus, data);
> +}
> +EXPORT_SYMBOL_GPL(panel_dbi_write_data);
> +
> +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev)
> +{
> + return dev->bus->ops->read_data(dev->bus);
> +}
> +EXPORT_SYMBOL_GPL(panel_dbi_read_data);
I'm not that familiar with how to implement bus drivers, can you
describe in pseudo code how the SoC's DBI driver would register these?
I think write/read data functions are a bit limited. Shouldn't they be
something like write_data(const u8 *buf, int size) and read_data(u8
*buf, int len)?
Something that's totally missing is configuring the DBI bus. There are a
bunch of timing related values that need to be configured. See
include/video/omapdss.h struct rfbi_timings. While the struct is OMAP
specific, if I recall right most of the values match to the MIPI DBI
spec.
And this makes me wonder, you use DBI bus for SYS-80 panel. The busses
may look similar in operation, but are they really so similar when you
take into account the timings (and perhaps something else, it's been
years since I read the MIPI DBI spec)?
Then there's the start_transfer. This is something I'm not sure what is
the best way to handle it, but the same problems that I mentioned in the
previous post related to enable apply here also. For example, what if
the panel needs to be update in two parts? This is done in Nokia N9.
From panel's perspective, it'd be best to handle it somewhat like this
(although asynchronously, probably):
write_update_area(0, 0, xres, yres / 2);
write_memory_start()
start_pixel_transfer();
wait_transfer_done();
write_update_area(0, yres / 2, xres, yres / 2);
write_memory_start()
start_pixel_transfer();
Why I said I'm not sure about this is that it does complicate things, as
the actual pixel data often comes from the display subsystem hardware,
which should probably be controlled by the display driver.
I think there also needs to be some kind of transfer_done notifier, for
both the display driver and the panel driver. Although if the display
driver handles starting the actual pixel transfer, then it'll get the
transfer_done via whatever interrupt the SoC has.
Also as food for thought, videomode timings does not really make sense
for DBI panels, at least when you just consider the DBI side. There's
really just the resolution of the display, and then the DBI timings. No
pck, syncs, etc. Of course in the end there's the actual panel, which
does have these video timings. But often they cannot be configured, and
often you don't even know them as the specs don't tell them.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v4 0/3] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-08-17 8:54 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann, Leela Krishna Amudala, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org
In-Reply-To: <201208162347.34810.rjw@sisk.pl>
On 08/17/2012 06:47 AM, Rafael J. Wysocki wrote:
> On Thursday, August 16, 2012, Alexandre Courbot wrote:
>> Overdue revision of this new feature, some changes required additional thought
>> and rework.
>>
>> The most important change is in the way power sequences are expressed in the
>> device tree. In order to avoid having to specify #address-cells, #size-cells and
>> reg properties, the @ notation in the step names is dropped, and instead a
>> fixed, sequential naming is adopted. The type of the resource used by a step is
>> decided by the presence of some recognized properties:
>>
>> power-on-sequence {
>> step0 {
>> regulator = "power";
>> enable;
>> };
>> step1 {
>> delay = <10000>;
>> };
>> step2 {
>> pwm = "backlight";
>> enable;
>> };
>> ...
>>
>> To me this looks safe, clear and close to the platform data representation, but
>> needs approval from DT experts.
>>
>> Resources are still referenced by name instead of having their phandles defined
>> directly inside the sequences, as previous discussion came to the conclusion
>> that doing so would require controversial changes to the regulator and PWM
>> frameworks, and that having the resources declared at the device level was
>> making sense logically speaking.
>>
>> Other changes/fixes since last revision:
>> * Move to drivers/power/ (hope this is ok with the maintainers?)
>> * Use microseconds for delay
>> * Use devm for PWM resources and remove cleanup function as all resources are
>> devm-managed
>> * Remove "-gpio" suffix for GPIO reference in the driver
>> * Remove params structure
>> * Make power_seq structure private
>> * Number of steps in a sequence is explicitly stated instead of resorting to a
>> "stop" sequence step
>> * Delays are a step instead of being a step parameter
>> * Use flexible member arrays to limit number of memory allocations
>> * Add documentation to DT bindings
>>
>> There was a lot of feedback on the previous version (thanks!) so if I forgot
>> to address some important point, please bring it to my attention again.
>>
>> Alexandre Courbot (3):
>> Runtime Interpreted Power Sequences
>> pwm_backlight: use power sequences
>> tegra: add pwm backlight device tree nodes
>
> May I ask that the next version of this patchset be CCed to
> linux-pm@vger.kernel.org?
Will do.
Alex.
^ permalink raw reply
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-08-17 8:52 UTC (permalink / raw)
To: Stephen Warren
Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann, Leela Krishna Amudala, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org
In-Reply-To: <502D3E29.1010501@wwwdotorg.org>
On 08/17/2012 03:38 AM, Stephen Warren wrote:
> On 08/16/2012 12:08 AM, Alexandre Courbot wrote:
>> Some device drivers (panel backlights especially) need to follow precise
>> sequences for powering on and off, involving gpios, regulators, PWMs
>> with a precise powering order and delays to respect between each steps.
>> These sequences are board-specific, and do not belong to a particular
>> driver - therefore they have been performed by board-specific hook
>> functions to far.
>>
>> With the advent of the device tree and of ARM kernels that are not
>> board-tied, we cannot rely on these board-specific hooks anymore but
>> need a way to implement these sequences in a portable manner. This patch
>> introduces a simple interpreter that can execute such power sequences
>> encoded either as platform data or within the device tree.
>
>> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt
>
>> +Specifying Power Sequences in the Device Tree
>> +======================>> +In the device tree, power sequences are specified as sub-nodes of the device
>> +node and reference resources declared by that device.
>> +
>> +For an introduction about runtime interpreted power sequences, see
>> +Documentation/power/power_seq.txt and include/linux/power_seq.h.
>
> Device tree bindings shouldn't reference Linux documentation; the
> bindings are supposed to be OS-agnostic.
Ok, I should be able to do without this reference anyway.
>
>> +Power Sequences Structure
>> +-------------------------
>> +Power sequences are sub-nodes that are named such as the device driver can find
>> +them. The driver's documentation should list the sequence names it recognizes.
>
> That's a little roundabout. That might be better as simply:
>
> Valid power sequence names are defined by each device's binding. For a
> power sequence named "foo", a node named "foo-power-sequence" defines
> that sequence.
>
>> +Inside a power sequence node are sub-nodes that describe the different steps
>> +of the sequence. Each step must be named sequentially, with the first step
>> +named step0, the second step1, etc. Failure to follow this rule will result in a
>> +parsing error.
>
> Node names shouldn't be interpreted by the code; nodes should all be
> named after the type of object the represent. Hence, every step should
> be named just "step" for example.
>
> The node's unit address (@0) should be used to distinguish the nodes.
> This requires reg properties within each node to match the unit address,
> and hence #address-cells and #size-cells properties in the power
> sequence itself.
While this logic is perfectly suitable and adapted for devices, I think
we should keep in mind that power sequences and their steps are not
devices, but just arbitrary bits of information. Having adress cells and
reg properties is useful when one needs to reference a node through a
phandle, but this is *never* going to happen with sequence steps (unless
we go insane and decide to allow goto-like statements :P). So having
#address-cells, #size-cells, and reg serve absolutely no purpose here
but cluttering the DT. If there is a hard rule that needs to be
enforced, then let that be, but the other discussion you started (thanks
for that by the way) does not seem to suggest so.
> Note that this somewhat conflicts with accessing the top-level power
> sequence by name too; perhaps that should be re-thought too. I must
> admit this DT rule kinda sucks.
>
>> +Power Sequences Steps
>> +---------------------
>> +Every step of a sequence describes an action to be performed on a resource. It
>> +generally includes a property named after the resource type, and which value
>> +references the resource to be used. Depending on the resource type, additional
>> +properties can be defined to control the action to be performed.
>
> I think you need to add a property that indicates what type of resource
> each step applies to. Sure, this is implicit in that if a "gpio"
> property exists, the step is a GPIO step, but in order to make that
> work, you'd have to search each node for each possible resource type's
> property name. It'd be far better to read a single type="gpio" property,
> then parse the step based on that.
Indeed right now all resource types must be checked. Having a type
property would make that easier. I like to keep the DT as compact and
expressive as possible, but I guess one more property per step would not
hurt and is perhaps easier to understand too.
>> +Example
>> +-------
>> +Here are example sequences declared within a backlight device that use all the
>> +supported resources types:
>> +
>> + backlight {
>> + compatible = "pwm-backlight";
>> + ...
>> +
>> + /* resources used by the sequences */
>> + pwms = <&pwm 2 5000000>;
>> + pwm-names = "backlight";
>> + power-supply = <&backlight_reg>;
>> + enable-gpio = <&gpio 28 0>;
>> +
>> + power-on-sequence {
>> + step0 {
>> + regulator = "power";
>> + enable;
>> + };
>> + step1 {
>> + delay = <10000>;
>> + };
>> + step2 {
>> + pwm = "backlight";
>> + enable;
>> + };
>> + step3 {
>> + gpio = "enable";
>> + enable;
>> + };
>> + };
>> +
>> + power-off-sequence {
>> + step0 {
>> + gpio = "enable";
>> + disable;
>> + };
>> + step1 {
>> + pwm = "backlight";
>> + disable;
>> + };
>> + step2 {
>> + delay = <10000>;
>> + };
>> + step3 {
>> + regulator = "power";
>> + disable;
>> + };
>> + };
>> + };
>
> I notice that for clocks, pwms, and interrupts, the initial step of the
> lookup is via a single property that lists all know resources of that
> type. Regulators and GPIOs don't follow this style though. Using the
> same mechanism for power-sequences would yield something like the
> following, which would avoid the "node names must be significant" issue
> I mention above, although it does make everything rather more wordy.
>
> [start my proposal]
>> backlight {
>> compatible = "pwm-backlight";
>>
>> /* resources used by the sequences */
>> pwms = <&pwm 2 5000000>;
>> pwm-names = "backlight";
>> power-supply = <&backlight_reg>;
>> bl-enable-gpio = <&gpio 28 0>;
>> pwr-seqs = <&bl_on &bl_off>;
>> pwr-seq-names = "on", "off";
>>
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> bl_on: pwr-seq@0 {
>> reg = <0>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>> step@0 {
>> reg = <0>;
>> type = "regulator";
>> regulator = "power";
>> enable;
>> };
>> step@1 {
>> reg = <1>;
>> type = "delay";
>> delay = <10000>;
>> };
>> step@2 {
>> reg = <2>;
>> type = "pwm";
>> pwm = "backlight";
>> enable;
>> };
>> step@3 {
>> reg = <3>;
>> type = "gpio";
>> gpio = "bl-enable";
>> enable;
>> };
>> };
>>
>> bl_off: pwr-seq@1 {
>> reg = <1>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>> step@0 {
>> reg = <0>;
>> type = "gpio";
>> gpio = "bl-enable";
>> disable;
>> };
>> step@1 {
>> reg = <1>;
>> type = "pwm";
>> pwm = "backlight";
>> disable;
>> };
>> step@2 {
>> reg = <2>;
>> type = "delay";
>> delay = <10000>;
>> };
>> step@3 {
>> reg = <3>;
>> type = "regulator";
>> regulator = "power";
>> disable;
>> };
>> };
>> };
>>
> [end my proposal]
Mmmm, so looking at this, what strikes me is that the amount of
unused/redundant information is actually higher than the amount of
information the driver will effectively use. If these conventions can be
ignored here, we definitely should do that.
>
>> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
>
>> +Usage by Drivers and Resources Management
>> +-----------------------------------------
>> +Power sequences make use of resources that must be properly allocated and
>> +managed. The power_seq_build() function builds a power sequence from the
>> +platform data. It also takes care of resolving and allocating the resources
>> +referenced by the sequence if needed:
>> +
>> + struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
>> + struct platform_power_seq *pseq);
>> +
>> +The 'dev' argument is the device in the name of which the resources are to be
>> +allocated.
>> +
>> +The 'ress' argument is a list to which the resolved resources are appended. This
>> +avoids allocating a resource referenced in several power sequences multiple
>> +times.
>
> I see in other parts of the thread, there has been discussion re:
> needing the separate ress parameter, and requiring the driver to pass
> this in to multiple power_seq_build calls.
>
> The way the pinctrl subsystem solved this was to have a single function
> that parsed all pinctrl setting (c.f. all power sequences) at once, and
> return a single handle. Later, the driver passes this handle
> pinctrl_lookup_state(), along with the requested state (c.f. sequence
> name) to search for, and finally passes that handle to
> pinctrl_select_state(). Doing something similar here would result in:
>
> struct power_seqs *power_seq_get(struct device *dev);
> void power_seq_put(struct power_seqs *seqs);
> struct power_seq *power_seq_lookup(struct power_seqs *seqs,
> const char *name);
> int power_seq_executestruct power_seq *seq);
>
> struct power_seqs *devm_power_seq_get(struct device *dev);
> void devm_power_seq_put(struct power_seqs *seqs);
Well, if both of you bring this then I have no choice but seriously
consider that. :) If other subsystems follow the same scheme then this
is an additional point for this.
>
>> +On success, the function returns a devm allocated resolved sequence that is
>
> Perhaps the function should be named devm_power_seq_build(), in order to
> make this obvious to people who only read the client code, and not this
> documentation.
Agreed.
Thanks,
Alex.
^ permalink raw reply
* Re: [RFC 0/5] Generic panel framework
From: Tomi Valkeinen @ 2012-08-17 8:38 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <1345164583-18924-1-git-send-email-laurent.pinchart@ideasonboard.com>
[-- Attachment #1: Type: text/plain, Size: 4980 bytes --]
Hi,
On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote:
> I will appreciate all reviews, comments, criticisms, ideas, remarks, ... If
Oookay, where to start... ;)
A few cosmetic/general comments first.
I find the file naming a bit strange. You have panel.c, which is the
core framework, panel-dbi.c, which is the DBI bus, panel-r61517.c, which
is driver for r61517 panel...
Perhaps something in this direction (in order): panel-core.c,
mipi-dbi-bus.c, panel-r61517.c? And we probably end up with quite a lot
of panel drivers, perhaps we should already divide these into separate
directories, and then we wouldn't need to prefix each panel with
"panel-" at all.
---
Should we aim for DT only solution from the start? DT is the direction
we are going, and I feel the older platform data stuff would be
deprecated soon.
---
Something missing from the intro is how this whole thing should be used.
It doesn't help if we know how to turn on the panel, we also need to
display something on it =). So I think some kind of diagram/example of
how, say, drm would use this thing, and also how the SoC specific DBI
bus driver would be done, would clarify things.
---
We have discussed face to face about the different hardware setups and
scenarios that we should support, but I'll list some of them here for
others:
1) We need to support chains of external display chips and panels. A
simple example is a chip that takes DSI in, and outputs DPI. In that
case we'd have a chain of SoC -> DSI2DPI -> DPI panel.
In final products I think two external devices is the maximum (at least
I've never seen three devices in a row), but in theory and in
development environments the chain can be arbitrarily long. Also the
connections are not necessarily 1-to-1, but a device can take one input
while it has two outputs, or a device can take two inputs.
Now, I think two external devices is a must requirement. I'm not sure if
supporting more is an important requirement. However, if we support two
devices, it could be that it's trivial to change the framework to
support n devices.
2) Panels and display chips are all but standard. They very often have
their own sequences how to do things, have bugs, or implement some
feature in slightly different way than some other panel. This is why the
panel driver should be able to control or define the way things happen.
As an example, Sharp LQ043T1DG01 panel
(www.sharpsme.com/download/LQ043T1DG01-SP-072106pdf). It is enabled with
the following sequence:
- Enable VCC and AVDD regulators
- Wait min 50ms
- Enable full video stream (pck, syncs, pixels) from SoC
- Wait min 0.5ms
- Set DISP GPIO, which turns on the display panel
Here we could split the enabling of panel to two parts, prepare (in this
case starts regulators and waits 50ms) and finish (wait 0.5ms and set
DISP GPIO), and the upper layer would start the video stream in between.
I realize this could be done with the PANEL_ENABLE_* levels in your RFC,
but I don't think the concepts quite match:
- PANEL_ENABLE_BLANK level is needed for "smart panels", as we need to
configure them and send the initial frame at that operating level. With
dummy panels there's really no such level, there's just one enable
sequence that is always done right away.
- I find waiting at the beginning of a function very ugly (what are we
waiting for?) and we'd need that when changing the panel to
PANEL_ENABLE_ON level.
- It's still limited if the panel is a stranger one (see following
example).
Consider the following theoretical panel enable example, taken to absurd
level just to show the general problem:
- Enable regulators
- Enable video stream
- Wait 50ms
- Disable video stream
- Set enable GPIO
- Enable video stream
This one would be rather impossible with the upper layer handling the
enabling of the video stream. Thus I see that the panel driver needs to
control the sequences, and the Sharp panel driver's enable would look
something like:
regulator_enable(...);
sleep();
dpi_enable_video();
sleep();
gpip_set(..);
Note that even with this model we still need the PANEL_ENABLE levels you
have.
---
I'm not sure I understand the panel unload problem you mentioned. Nobody
should have direct references to the panel functions, so there shouldn't
be any automatic references that would prevent module unloading. So when
the user does rmmod panel-mypanel, the panel driver's remove will be
called. It'll unregister itself from the panel framework, which causes
notifications and the display driver will stop using the panel. After
that nobody has pointers to the panel, and it can safely be unloaded.
It could cause some locking issues, though. First the panel's remove
could take a lock, but the remove sequence would cause the display
driver to call disable on the panel, which could again try to take the
same lock...
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox