linux-fbdev-devel — linux-fbdev mailing list for project developers

Re: [Linux-fbdev-devel] [PATCH 1/2] fb: Rework locking to fix lock ordering on takeover

[Takashi I. <ti...@su...>]

At Wed, 23 Jan 2013 17:25:08 +0100,
Daniel Vetter wrote:
> 
> From: Alan Cox <al...@lx...>
> 
> Adjust the console layer to allow a take over call where the caller already
> holds the locks. Make the fb layer lock in order.
> 
> This s partly a band aid, the fb layer is terminally confused about the
> locking rules it uses for its notifiers it seems.
> 
> Signed-off-by: Alan Cox <al...@li...>
> [danvet: Tiny whitespace cleanup.]
> Reported-and-tested-by: Hugh Dickins <hu...@go...>
> Reported-and-tested-by: Sasha Levin <lev...@gm...>
> References: https://lkml.org/lkml/2012/10/25/516
> Signed-off-by: Daniel Vetter <dan...@ff...>

FYI, the latest patch of this is found in mm tree:
  http://ozlabs.org/~akpm/mmots/broken-out/fb-rework-locking-to-fix-lock-ordering-on-takeover.patch

Also I hit the same problem in another code paths (for unbind and
unregister):
  http://marc.info/?t=135309396400003&r=1&w=2

My additional patch is found in mm tree, too:
  http://ozlabs.org/~akpm/mmots/broken-out/fb-yet-another-band-aid-for-fixing-lockdep-mess.patch


Takashi

Re: [Linux-fbdev-devel] [PATCH] [#7001] framebuffer: old image of last app would retain for a while after starting new app

[Mike F. <va...@ge...>]

On Friday 16 March 2012 01:05:10 Zhang, Sonic wrote:
> From: Mike Frysinger [mailto:va...@ge...]
>> On Thursday 15 March 2012 23:34:08 Zhang, Sonic wrote:
>>> From: Mike Frysinger [mailto:va...@ge...]
>>>> On Thursday 15 March 2012 05:23:50 Wu, Aaron wrote:
>>>>> Old image of last application would retain for a while when
>>>>> starting a new application, this patch clear the frambuffer before
>>>>> displaying every time the fb is opened.
>>>> 
>>>> i'm not sure the behavior you describe is wrong.  in fact, i'm pretty
>>>> sure it sounds correct.  if an app writes an image to the framebuffer
>>>> and then quits, that image should stay there indefinitely until
>>>> something else opens the framebuffer and draws their own image.
>>> 
>>> Before the second application draws its own image, the image of last
>>> application has already been displayed on the LCD when the second
>>> application opens the FB device(PPI is enabled when it is opened).
>>> This is not a correct behavior.
>> 
>> sure it is.  if the app wants to clear the screen, it can do so.  generally
>> it's going to anyways by drawing an entire frame.  having the frame buffer
>> driver always clear the screen introduces wasted memory overhead as it
>> does the memset(), and user-visible jank as the device transitions from an
>> initial splash screen to the main userspace app.
>> 
>> this is a policy decision that doesn't really belong in kernel space.  and
>> if it did, it should be agreed upon by all frame buffer users and not just
>> changing a few drivers.
> 
> How can the application clean the screen before it opens the FB device?

why does it need to be before open ?  if the kernel does the memset or 
userspace does the memset, the frame still gets cleared.

in looking at the blackfin framebuffer drivers, i'm not sure they're correct.  i 
don't think the PPI/DMA should be shutdown when the last user space client 
closes it.  fb_release is for releasing all resources when tearing down the 
driver, and fb_blank is runtime management (turning off vsync/hsync and 
powering down the screen).
-mike

Re: [Linux-fbdev-devel] [PATCH] [#7001] framebuffer: old image of last app would retain for a while after starting new app

[Mike F. <va...@ge...>]

On Thursday 15 March 2012 23:34:08 Zhang, Sonic wrote:
> From: Mike Frysinger [mailto:va...@ge...]
>> On Thursday 15 March 2012 05:23:50 Wu, Aaron wrote:
>> > Old image of last application would retain for a while when starting a
>> > new application, this patch clear the frambuffer before displaying
>> > every time the fb is opened.
>> 
>> i'm not sure the behavior you describe is wrong.  in fact, i'm pretty sure
>> it sounds correct.  if an app writes an image to the framebuffer and then
>> quits, that image should stay there indefinitely until something else
>> opens the framebuffer and draws their own image.
> 
> Before the second application draws its own image, the image of last
> application has already been displayed on the LCD when the second
> application opens the FB device(PPI is enabled when it is opened). This is
> not a correct behavior.

sure it is.  if the app wants to clear the screen, it can do so.  generally 
it's going to anyways by drawing an entire frame.  having the frame buffer 
driver always clear the screen introduces wasted memory overhead as it does 
the memset(), and user-visible jank as the device transitions from an initial 
splash screen to the main userspace app.

this is a policy decision that doesn't really belong in kernel space.  and if 
it did, it should be agreed upon by all frame buffer users and not just 
changing a few drivers.
-mike

Re: [Linux-fbdev-devel] [uclinux-dist-devel] [PATCH] [#7001] framebuffer: old image of last app would retain for a while after starting new app

[Mike F. <va...@ge...>]

On Thursday 15 March 2012 05:23:50 Wu, Aaron wrote:
> Old image of last application would retain for a while when starting a new
> application, this patch clear the frambuffer before displaying every time
> the fb is opened.

i'm not sure the behavior you describe is wrong.  in fact, i'm pretty sure it 
sounds correct.  if an app writes an image to the framebuffer and then quits, 
that image should stay there indefinitely until something else opens the 
framebuffer and draws their own image.
-mike

[Linux-fbdev-devel] [PATCH] fbdev: fix nearest mode search

[Michal J. <mi...@gm...>]

In the framebuffer subsystem the abs() macro is often used as a part of
the calculation of a Manhattan metric, which in turn is used as a measure
of similarity between video modes.  The arguments of abs() are sometimes
unsigned numbers.  This worked fine until commit a49c59c0, which changed
the definition of abs() to prevent truncation.  As a result of this
change, in the following piece of code:

  u32 a = 0, b = 1;
  u32 c = abs(a - b);

'c' will end up with a value of 0xffffffff instead of the expected 0x1.

A problem caused by this change and visible by the end user is that
framebuffer drivers relying on functions from modedb.c will fail to
find high resolution video modes similar to that explicitly requested
by the user if an exact match cannot be found (see e.g.
https://bugs.gentoo.org/show_bug.cgi?id=296539).

Fix this problem by casting all arguments of abs() to an int prior
to the macro evaluation in modedb.c and uvesafb.c.

Signed-off-by: Michal Januszewski <mi...@gm...>
---
diff --git a/drivers/video/modedb.c b/drivers/video/modedb.c
index 0a4dbdc..878bea1 100644
--- a/drivers/video/modedb.c
+++ b/drivers/video/modedb.c
@@ -636,8 +636,10 @@ done:
 			if (refresh_specified && db[i].refresh == refresh) {
 				return 1;
 			} else {
-				if (abs(db[i].refresh - refresh) < diff) {
-					diff = abs(db[i].refresh - refresh);
+				if (abs((int)(db[i].refresh - refresh)) <
+				    diff) {
+					diff = abs((int)(db[i].refresh -
+						refresh));
 					best = i;
 				}
 			}
@@ -654,8 +656,8 @@ done:
 	for (i = 0; i < dbsize; i++) {
 		DPRINTK("Trying %ix%i\n", db[i].xres, db[i].yres);
 		if (!fb_try_mode(var, info, &db[i], bpp)) {
-			tdiff = abs(db[i].xres - xres) +
-				abs(db[i].yres - yres);
+			tdiff = abs((int)(db[i].xres - xres)) +
+				abs((int)(db[i].yres - yres));
 
 			/*
 			 * Penalize modes with resolutions smaller
@@ -851,13 +853,13 @@ const struct fb_videomode *fb_find_nearest_mode(const struct fb_videomode *mode,
 		modelist = list_entry(pos, struct fb_modelist, list);
 		cmode = &modelist->mode;
 
-		d = abs(cmode->xres - mode->xres) +
-			abs(cmode->yres - mode->yres);
+		d = abs((int)(cmode->xres - mode->xres)) +
+			abs((int)(cmode->yres - mode->yres));
 		if (diff > d) {
 			diff = d;
 			best = cmode;
 		} else if (diff == d) {
-			d = abs(cmode->refresh - mode->refresh);
+			d = abs((int)(cmode->refresh - mode->refresh));
 			if (diff_refresh > d) {
 				diff_refresh = d;
 				best = cmode;
diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index 7b8839e..6621427 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -320,9 +320,9 @@ static int uvesafb_vbe_find_mode(struct uvesafb_par *par,
 	int i, match = -1, h = 0, d = 0x7fffffff;
 
 	for (i = 0; i < par->vbe_modes_cnt; i++) {
-		h = abs(par->vbe_modes[i].x_res - xres) +
-		    abs(par->vbe_modes[i].y_res - yres) +
-		    abs(depth - par->vbe_modes[i].depth);
+		h = abs((int)(par->vbe_modes[i].x_res - xres)) +
+		    abs((int)(par->vbe_modes[i].y_res - yres)) +
+		    abs((int)(depth - par->vbe_modes[i].depth));
 
 		/*
 		 * We have an exact match in terms of resolution
@@ -1375,7 +1375,7 @@ static int uvesafb_check_var(struct fb_var_screeninfo *var,
 	 * which is theoretically incorrect, but which we'll try to handle
 	 * here.
 	 */
-	if (depth == 0 || abs(depth - var->bits_per_pixel) >= 8)
+	if (depth == 0 || abs((int)(depth - var->bits_per_pixel)) >= 8)
 		depth = var->bits_per_pixel;
 
 	match = uvesafb_vbe_find_mode(par, var->xres, var->yres, depth,

Re: [Linux-fbdev-devel] [Bugme-new] [Bug 18912] New: BUG: unable to handle kernel NULL pointer dereference at (null)

[Andrew M. <ak...@li...>]

(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

On Mon, 20 Sep 2010 22:34:17 GMT bug...@bu... wrote:

> https://bugzilla.kernel.org/show_bug.cgi?id=18912
> 
>            Summary: BUG: unable to handle kernel NULL pointer dereference
>                     at (null)
>            Product: IO/Storage
>            Version: 2.5
>     Kernel Version: 2.6.35
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: Other
>         AssignedTo: io_...@ke...
>         ReportedBy: tho...@gm...
>         Regression: No
> 
> 
> Created an attachment (id=30872)
>  --> (https://bugzilla.kernel.org/attachment.cgi?id=30872)
> full dmesg
> 
> at restart with ubuntu mainline kernel from there
> (http://kernel.ubuntu.com/~kernel-ppa/mainline/v2.6.35.4-maverick/) I get the
> following bug:

This is a bit strange.  We dereferenced a NULL pointer in
__mutex_lock_slowpath(), but where did it come from?  fb_release() does

	mutex_lock(&info->lock);

but for that to be exactly zero, `info' must have been a small negative
number.


> [   43.635104] BUG: unable to handle kernel NULL pointer dereference at (null)
> [   43.635125] IP: [<ffffffff81576249>] __mutex_lock_slowpath+0xa9/0x170
> [   43.635144] PGD 2333cd067 PUD 2333ce067 PMD 0 
> [   43.635159] Oops: 0002 [#1] SMP 
> [   43.635171] last sysfs file: /sys/devices/platform/f71882fg.2560/temp3_input
> [   43.635185] CPU 4 
> [   43.635188] Modules linked in: ipt_LOG ip6t_LOG ip6t_rt ipt_REDIRECT
> xt_multiport xt_recent xt_tcpudp xt_limit nf_conntrack_ipv6 xt_state
> ip6table_filter ip6_tables iptable_mangle iptable_nat nf_nat iptable_filter
> nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables nouveau ttm
> drm_kms_helper drm i7core_edac i2c_algo_bit edac_core f71882fg coretemp raid10
> raid456 async_raid6_recov async_pq raid6_pq async_xor xor async_memcpy async_tx
> raid0 multipath linear raid1 r8169 mii ahci libahci
> [   43.635293] 
> [   43.635297] Pid: 451, comm: plymouthd Not tainted 2.6.35-02063504-generic
> #201008271919 MSI X58 Pro-E (MS-7522)/MS-7522
> [   43.635305] RIP: 0010:[<ffffffff81576249>]  [<ffffffff81576249>]
> __mutex_lock_slowpath+0xa9/0x170
> [   43.635315] RSP: 0018:ffff8802331cbe18  EFLAGS: 00010246
> [   43.635321] RAX: ffff8802331cbe28 RBX: ffff88023429000c RCX:
> 00000000ffffffff
> [   43.635327] RDX: 0000000000000000 RSI: 00000000ffffffff RDI:
> ffff88023429000c
> [   43.635334] RBP: ffff8802331cbe78 R08: 0000000000000000 R09:
> 0000000000000000
> [   43.635340] R10: 00007fff508c4d70 R11: 0000000000000246 R12:
> ffff880234290008
> [   43.635346] R13: ffff880232fa8000 R14: ffff880232fa8000 R15:
> ffff880234290010
> [   43.635353] FS:  00007f374ac96700(0000) GS:ffff880001e80000(0000)
> knlGS:0000000000000000
> [   43.635360] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   43.635365] CR2: 0000000000000000 CR3: 00000002331a0000 CR4:
> 00000000000006a0
> [   43.635372] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [   43.635378] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
> 0000000000000400
> [   43.635385] Process plymouthd (pid: 451, threadinfo ffff8802331ca000, task
> ffff880232fa8000)
> [   43.635392] Stack:
> [   43.635395]  ffff8802331cbe28 ffffffff815774fe ffff880234290010
> ffffffff8113e2a6
> [   43.635407] <0> ffff880001e8f970 ffffea0007ad5ff0 ffff880231892cf0
> ffff880234290008
> [   43.635420] <0> ffff880234290008 ffff8802342980d0 ffff880226b35c00
> ffff8802342980d0
> [   43.635434] Call Trace:
> [   43.635440]  [<ffffffff815774fe>] ? _raw_spin_lock+0xe/0x20
> [   43.635447]  [<ffffffff8113e2a6>] ? add_partial+0x56/0x90
> [   43.635453]  [<ffffffff81575ecb>] mutex_lock+0x2b/0x50
> [   43.635460]  [<ffffffff812e4839>] fb_release+0x29/0x70
> [   43.635467]  [<ffffffff81152d63>] __fput+0xf3/0x220
> [   43.635472]  [<ffffffff81152eac>] fput+0x1c/0x30
> [   43.635478]  [<ffffffff8114f6bd>] filp_close+0x5d/0x90
> [   43.635484]  [<ffffffff8115095f>] sys_close+0xaf/0x110
> [   43.635492]  [<ffffffff8100b072>] system_call_fastpath+0x16/0x1b
> [   43.635591] Code: 75 07 41 83 7e 20 63 7f b1 49 8d 5c 24 04 4d 8d 7c 24 08
> 48 89 df e8 b7 12 00 00 49 8b 57 08 48 8d 45 b0 4c 89 7d b0 49 89 47 08 <48> 89
> 02 48 89 55 b8 ba ff ff ff ff 4c 89 75 c0 89 d0 41 87 04 
> [   43.635716] RIP  [<ffffffff81576249>] __mutex_lock_slowpath+0xa9/0x170
> [   43.635724]  RSP <ffff8802331cbe18>
> [   43.635728] CR2: 0000000000000000
> [   43.635732] ---[ end trace 977d55bf15636e33 ]---
> 
> 
> Kernel is 2.6.35-02063504-generic
> 

Re: [Linux-fbdev-devel] [PATCH 1/3] FB: Add some members for CPU Interface.

[Pawel O. <p.o...@sa...>]

InKi Dae wrote:
>Signed-off-by: InKi Dae &lt;<a href="mailto:p.o...@sa...">ink...@sa...</a>&gt;<br>
>Signed-off-by: Kyungmin Park &lt;<a
>href="mailto:kyu...@sa...">kyu...@sa...</a>&gt;<br>

Hi InKi,

please do not include my e-mail address like this. I had absolutely nothing to do with this patch series whatsoever.

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center

Re: [Linux-fbdev-devel] [PATCH v1 2/2] S5PV210: Add MIPI-DSI Driver.

[Guennadi L. <g.l...@gm...>]

On Mon, 5 Jul 2010, In-Ki Dae wrote:

> Hi, Guennadi,
> 
> You mean is that it uses include/video/mipi_display.h file instead of plat/dsim.h?
> I cann't find mipi_display.h in mainline kernel.

Sorry for not mentioning straight away - it is in linux-next:

http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=blob;f=include/video/mipi_display.h;h=ddcc8ca7316b51695bd827d9370d9027173e3576;hb=HEAD

Thanks
Guennadi

> otherwise, it creates generic parts of my dsim.h header file as mipi_display.h and
> use that header file?
> 
> Please, give me more detailed.
> 
> Thank you.
> 
> ------- Original Message -------
> Sender : Guennadi Liakhovetski<g.l...@gm...> 
> Date   : 2010-07-05 04:33 (GMT+09:00)
> Title  : Re: [PATCH v1 2/2] S5PV210: Add MIPI-DSI Driver.
> 
> On Sat, 3 Jul 2010, InKi Dae wrote:
> 
> > this patch addes MIPI-DSI Driver.
> > 
> > to use this driver, some structures below should be added to machine
> > specific file.
> > 
> > struct dsim_config
> > - define clock info, data lane count and video mode info for MIPI-DSI
> > Controller.
> > 
> > struct dsim_lcd_config
> > - define interface mode, channel ID, Pixel format and so on.
> > 
> > struct s5p_platform_dsim
> > - define callbacks for initializing D-PHY, MIPI reset and trigger
> > releated interfaces of s3c-fb.c file.
> > 
> > struct mipi_ddi_platform_data
> > - define following callbacks.
> > - a function for transferring and receiving command data to mipi based
> > lcd panel.
> > - a function for getting framedone status of mipi-dsi controller.
> > - a function for clearing framedone interrupt of mipi-dsi controller.
> > - a function for checking i80 framedone status of display controller.(fimd)
> > - a function for triggering to display controller.(fimd)
> > 
> > Signed-off-by: InKi Dae <ink...@sa... <mailto:p.o...@sa...>>
> > Signed-off-by: Kyungmin Park <kyu...@sa...
> > <mailto:kyu...@sa...>>
> > ---
> > 
> > diff --git a/arch/arm/plat-samsung/include/plat/dsim.h b/arch/arm/plat-samsung/include/plat/dsim.h
> > new file mode 100644
> > index 0000000..49d8946
> > --- /dev/null
> > +++ b/arch/arm/plat-samsung/include/plat/dsim.h
> > @@ -0,0 +1,493 @@
> 
> [snip]
> 
> > +enum dsim_fifo_state {
> > +	DSIM_RX_DATA_FULL = (1 << 25),
> > +	DSIM_RX_DATA_EMPTY = (1 << 24),
> > +	SFR_HEADER_FULL = (1 << 23),
> > +	SFR_HEADER_EMPTY = (1 << 22),
> > +	SFR_PAYLOAD_FULL = (1 << 21),
> > +	SFR_PAYLOAD_EMPTY = (1 << 20),
> > +	I80_HEADER_FULL = (1 << 19),
> > +	I80_HEADER_EMPTY = (1 << 18),
> > +	I80_PALOAD_FULL = (1 << 17),
> > +	I80_PALOAD_EMPTY = (1 << 16),
> > +	SUB_DISP_HEADER_FULL = (1 << 15),
> > +	SUB_DISP_HEADER_EMPTY = (1 << 14),
> > +	SUB_DISP_PAYLOAD_FULL = (1 << 13),
> > +	SUB_DISP_PAYLOAD_EMPTY = (1 << 12),
> > +	MAIN_DISP_HEADER_FULL = (1 << 11),
> > +	MAIN_DISP_HEADER_EMPTY = (1 << 10),
> > +	MAIN_DISP_PAYLOAD_FULL = (1 << 9),
> > +	MAIN_DISP_PAYLOAD_EMPTY = (1 << 8),
> > +};
> 
> Please use include/video/mipi_display.h for these transaction types and, 
> possibly, for other generic MIPI DSI defines.
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 
> 
> 
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [Linux-fbdev-devel] [PATCH v1 2/2] S5PV210: Add MIPI-DSI Driver.

[Guennadi L. <g.l...@gm...>]

On Sat, 3 Jul 2010, InKi Dae wrote:

> this patch addes MIPI-DSI Driver.
> 
> to use this driver, some structures below should be added to machine
> specific file.
> 
> struct dsim_config
> - define clock info, data lane count and video mode info for MIPI-DSI
> Controller.
> 
> struct dsim_lcd_config
> - define interface mode, channel ID, Pixel format and so on.
> 
> struct s5p_platform_dsim
> - define callbacks for initializing D-PHY, MIPI reset and trigger
> releated interfaces of s3c-fb.c file.
> 
> struct mipi_ddi_platform_data
> - define following callbacks.
> - a function for transferring and receiving command data to mipi based
> lcd panel.
> - a function for getting framedone status of mipi-dsi controller.
> - a function for clearing framedone interrupt of mipi-dsi controller.
> - a function for checking i80 framedone status of display controller.(fimd)
> - a function for triggering to display controller.(fimd)
> 
> Signed-off-by: InKi Dae <ink...@sa... <mailto:p.o...@sa...>>
> Signed-off-by: Kyungmin Park <kyu...@sa...
> <mailto:kyu...@sa...>>
> ---
> 
> diff --git a/arch/arm/plat-samsung/include/plat/dsim.h b/arch/arm/plat-samsung/include/plat/dsim.h
> new file mode 100644
> index 0000000..49d8946
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/dsim.h
> @@ -0,0 +1,493 @@

[snip]

> +enum dsim_fifo_state {
> +	DSIM_RX_DATA_FULL = (1 << 25),
> +	DSIM_RX_DATA_EMPTY = (1 << 24),
> +	SFR_HEADER_FULL = (1 << 23),
> +	SFR_HEADER_EMPTY = (1 << 22),
> +	SFR_PAYLOAD_FULL = (1 << 21),
> +	SFR_PAYLOAD_EMPTY = (1 << 20),
> +	I80_HEADER_FULL = (1 << 19),
> +	I80_HEADER_EMPTY = (1 << 18),
> +	I80_PALOAD_FULL = (1 << 17),
> +	I80_PALOAD_EMPTY = (1 << 16),
> +	SUB_DISP_HEADER_FULL = (1 << 15),
> +	SUB_DISP_HEADER_EMPTY = (1 << 14),
> +	SUB_DISP_PAYLOAD_FULL = (1 << 13),
> +	SUB_DISP_PAYLOAD_EMPTY = (1 << 12),
> +	MAIN_DISP_HEADER_FULL = (1 << 11),
> +	MAIN_DISP_HEADER_EMPTY = (1 << 10),
> +	MAIN_DISP_PAYLOAD_FULL = (1 << 9),
> +	MAIN_DISP_PAYLOAD_EMPTY = (1 << 8),
> +};

Please use include/video/mipi_display.h for these transaction types and, 
possibly, for other generic MIPI DSI defines.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [Linux-fbdev-devel] [PATCH 2/2] S5PV210: Add MIPI-DSI Driver.

[Ben D. <be...@si...>]

On 02/07/10 09:51, InKi Dae wrote:
> this patch addes MIPI-DSI Driver.
> 
> to use this driver, some structures below should be added to machine
> specific file.
> 
> struct dsim_config
> - define clock info, data lane count and video mode info for MIPI-DSI
> Controller.
> 
> struct dsim_lcd_config
> - define interface mode, channel ID, Pixel format and so on.
> 
> struct s5p_platform_dsim
> - define callbacks for initializing D-PHY, MIPI reset and trigger
> releated interfaces of s3c-fb.c file.
> 
> Signed-off-by: InKi Dae <ink...@sa... <mailto:p.o...@sa...>>
> Signed-off-by: Kyungmin Park <kyu...@sa...
> <mailto:kyu...@sa...>>
> ---
> 
> diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> index 2a25ab4..f716678 100644
> --- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> +++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> @@ -162,6 +162,7 @@
>  
>  /* MIPI */
>  #define S5P_MIPI_DPHY_EN		(3)
> +#define S5P_MIPI_M_RESETN		(1 << 1)
>  
>  /* S5P_DAC_CONTROL */
>  #define S5P_DAC_ENABLE			(1)
> diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
> index b1d82cc..3cd43f2 100644
> --- a/arch/arm/plat-samsung/Makefile
> +++ b/arch/arm/plat-samsung/Makefile
> @@ -49,6 +49,9 @@ obj-$(CONFIG_S3C_DEV_RTC)	+= dev-rtc.o
>  obj-$(CONFIG_SAMSUNG_DEV_ADC)	+= dev-adc.o
>  obj-$(CONFIG_SAMSUNG_DEV_TS)	+= dev-ts.o
>  
> +# Device setup - MIPI-DSI
> +obj-$(CONFIG_S5P_MIPI_DSI)  += setup-dsim.o
> +
>  # DMA support
>  
>  obj-$(CONFIG_S3C_DMA)		+= dma.o
> diff --git a/arch/arm/plat-samsung/include/plat/dsim.h b/arch/arm/plat-samsung/include/plat/dsim.h
> new file mode 100644
> index 0000000..28bc595
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/dsim.h
> @@ -0,0 +1,470 @@


> + * driver structure for mipi-dsi based lcd panel.
> + *
> + * this structure should be registered by lcd panel driver.
> + * mipi-dsi driver seeks lcd panel registered through name field
> + * and calls these callback functions in appropriate time.
> + */
> +struct mipi_lcd_driver {
> +	s8	name[64];

how about an 'char *' here instead of reserving 64bytes?

> +	s32	(*init)(struct device *dev);
> +	void	(*display_on)(struct device *dev);
> +	s32	(*set_link)(struct mipi_ddi_platform_data *pd);
> +	s32	(*probe)(struct device *dev);
> +	s32	(*remove)(struct device *dev);
> +	void	(*shutdown)(struct device *dev);
> +	s32	(*suspend)(struct device *dev, pm_message_t mesg);
> +	s32	(*resume)(struct device *dev);
> +};

Some of this should be already covered under the existing
lcd interface?

> diff --git a/arch/arm/plat-samsung/include/plat/mipi_ddi.h b/arch/arm/plat-samsung/include/plat/mipi_ddi.h
> new file mode 100644
> index 0000000..57ed613
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/mipi_ddi.h
> @@ -0,0 +1,98 @@
> +/* linux/arm/arch/mach-s5pc110/include/mach/mipi_ddi.h
> + *
> + * definitions for DDI based MIPI-DSI.
> + *
> + * Copyright (c) 2009 Samsung Electronics
> + * InKi Dae <ink...@sa...>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#ifndef _MIPI_DDI_H
> +#define _MIPI_DDI_H
> +
> +enum mipi_ddi_interface {
> +	RGB_IF = 0x4000,
> +	I80_IF = 0x8000,
> +	YUV_601 = 0x10000,
> +	YUV_656 = 0x20000,
> +	MIPI_VIDEO = 0x1000,
> +	MIPI_COMMAND = 0x2000,
> +};
> +
> +enum mipi_ddi_panel_select {
> +	DDI_MAIN_LCD = 0,
> +	DDI_SUB_LCD = 1,
> +};
> +
> +enum mipi_ddi_model {
> +	S6DR117 = 0,
> +};
> +
> +enum mipi_ddi_parameter {
> +	/* DSIM video interface parameter */
> +	DSI_VIRTUAL_CH_ID = 0,
> +	DSI_FORMAT = 1,
> +	DSI_VIDEO_MODE_SEL = 2,
> +};
> +
> +struct lcd_device;
> +struct fb_info;
> +
> +struct mipi_ddi_platform_data {
> +	void *dsim_data;
> +	/*
> +	 * it is used for command mode lcd panel and
> +	 * when all contents of framebuffer in panel module are transfered
> +	 * to lcd panel it occurs te signal.
> +	 *
> +	 * note:
> +	 * - in case of command mode(cpu mode), it should be triggered only
> +	 *   when TE signal of lcd panel and frame done interrupt of display
> +	 *   controller or mipi controller occurs.
> +	 */
> +	unsigned int te_irq;
> +
> +	/*
> +	 * it is used for PM stable time at te interrupt handler and
> +	 * could be used according to lcd panel characteristic or not.
> +	 */
> +	unsigned int resume_complete;
> +
> +	int (*lcd_reset) (struct lcd_device *ld);
> +	int (*lcd_power_on) (struct lcd_device *ld, int enable);
> +	int (*backlight_on) (int enable);
> +
> +	/* transfer command to lcd panel at LP mode. */
> +	int (*cmd_write) (void *dsim_data, unsigned int data_id,
> +		unsigned int data0, unsigned int data1);
> +	int (*cmd_read) (void *dsim_data, unsigned int data_id,
> +		unsigned int data0, unsigned int data1);
> +	/*
> +	 * get the status that all screen data have been transferred
> +	 * to mipi-dsi.
> +	 */
> +	int (*get_dsim_frame_done) (void *dsim_data);
> +	int (*clear_dsim_frame_done) (void *dsim_data);
> +
> +	/*
> +	 * changes mipi transfer mode to LP or HS mode.
> +	 *
> +	 * LP mode needs when some commands like gamma values transfers
> +	 * to lcd panel.
> +	 */
> +	int (*change_dsim_transfer_mode) (int mode);
> +
> +	/* get frame done status of display controller. */
> +	int (*get_fb_frame_done) (struct fb_info *info);
> +	/* trigger display controller in case of cpu mode. */
> +	void (*trigger) (struct fb_info *info);
> +
> +	unsigned int reset_delay;
> +	unsigned int power_on_delay;
> +	unsigned int power_off_delay;
> +};
> +
> +#endif /* _MIPI_DDI_H */
> diff --git a/arch/arm/plat-samsung/include/plat/regs-dsim.h b/arch/arm/plat-samsung/include/plat/regs-dsim.h
> new file mode 100644
> index 0000000..dc83089
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/regs-dsim.h
> @@ -0,0 +1,281 @@
 +
> +/* S5P_DSIM_TIMEOUT */
> +#define DSIM_LPDR_TOUT_SHIFT	(0)
> +#define DSIM_BTA_TOUT_SHIFT	(16)
> +#define DSIM_LPDR_TOUT(x)	(((x) & 0xffff) << DSIM_LPDR_TOUT_SHIFT)
> +#define DSIM_BTA_TOUT(x)	(((x) & 0xff) << DSIM_BTA_TOUT_SHIFT)
> +
> +/* S5P_DSIM_CLKCTRL */
> +#define DSIM_ESC_PRESCALER_SHIFT	(0)
> +#define DSIM_LANE_ESC_CLKEN_SHIFT	(19)
> +#define DSIM_BYTE_CLKEN_SHIFT		(24)
> +#define DSIM_BYTE_CLK_SRC_SHIFT		(25)
> +#define DSIM_PLL_BYPASS_SHIFT		(27)
> +#define DSIM_ESC_CLKEN_SHIFT		(28)
> +#define DSIM_TX_REQUEST_HSCLK_SHIFT	(31)
> +#define DSIM_ESC_PRESCALER(x)		(((x) & 0xffff) << \
> +						DSIM_ESC_PRESCALER_SHIFT)
> +#define DSIM_LANE_ESC_CLKEN(x)		(((x) & 0x1f) << \
> +						DSIM_LANE_ESC_CLKEN_SHIFT)
> +#define DSIM_BYTE_CLK_ENABLE		(1 << DSIM_BYTE_CLKEN_SHIFT)
> +#define DSIM_BYTE_CLK_DISABLE		(0 << DSIM_BYTE_CLKEN_SHIFT)
> +#define DSIM_BYTE_CLKSRC(x)		(((x) & 0x3) << DSIM_BYTE_CLK_SRC_SHIFT)
> +#define DSIM_PLL_BYPASS_PLL		(0 << DSIM_PLL_BYPASS_SHIFT)
> +#define DSIM_PLL_BYPASS_EXTERNAL	(1 << DSIM_PLL_BYPASS_SHIFT)
> +#define DSIM_ESC_CLKEN_ENABLE		(1 << DSIM_ESC_CLKEN_SHIFT)
> +#define DSIM_ESC_CLKEN_DISABLE		(0 << DSIM_ESC_CLKEN_SHIFT)


> +
> +#include <plat/dsim.h>
> +#include <plat/clock.h>
> +#include <plat/regs-dsim.h>
> +
> +static int s5p_dsim_enable_d_phy(struct dsim_global *dsim, unsigned int enable)

I suppose enable should be bool,

> +{
> +	unsigned int reg;
> +
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim is NULL.\n");
> +		return -EFAULT;
> +	}

Is it likely to be NULL? If unlikely then a simple warning and return
-EFAULT.

> +
> +	reg = (readl(S5P_MIPI_CONTROL)) & ~(1 << 0);

extra () not really needed

> +	reg |= (enable << 0);
> +	writel(reg, S5P_MIPI_CONTROL);
> +
> +	dev_dbg(dsim->dev, "%s : %x\n", __func__, reg);
> +
> +	return 0;
> +}
> +
> +static int s5p_dsim_enable_dsi_master(struct dsim_global *dsim,
> +	unsigned int enable)
> +{
> +	unsigned int reg;
> +
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	reg = (readl(S5P_MIPI_CONTROL)) & ~(1 << 2);
> +	reg |= (enable << 2);
> +	writel(reg, S5P_MIPI_CONTROL);
> +
> +	dev_dbg(dsim->dev, "%s : %x\n", __func__, reg);
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_part_reset(struct dsim_global *dsim)
> +{
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	writel(S5P_MIPI_M_RESETN, S5P_MIPI_PHY_CON0);
> +
> +	dev_dbg(dsim->dev, "%s\n", __func__);
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_init_d_phy(struct dsim_global *dsim)
> +{
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	/* enable D-PHY */
> +	s5p_dsim_enable_d_phy(dsim, 1);
> +
> +	/* enable DSI master block */
> +	s5p_dsim_enable_dsi_master(dsim, 1);

you ould probably have omitted the comments on these.

> +
> +	dev_dbg(dsim->dev, "%s\n", __func__);
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_mipi_power(struct dsim_global *dsim, void *p_mipi_1_1v,
> +	void *p_mipi_1_8v, int enable)

enable could be bool.

> +{
> +	struct regulator *r_mipi_1_1v = NULL, *r_mipi_1_8v = NULL;

No need to init to NULL when you just cast them a few lines done.

> +	int ret = -1;
> +
> +	r_mipi_1_1v = (struct regulator *) p_mipi_1_1v;
> +	r_mipi_1_8v = (struct regulator *) p_mipi_1_8v;

It would be better just to call these regulators and stick with one type
for these.


> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim is NULL.\n");
> +		return -EFAULT;
> +	}

this is getting repetitive, is it really necessary?


> +
> +	if (IS_ERR(r_mipi_1_1v) || IS_ERR(r_mipi_1_8v)) {
> +		dev_err(dsim->dev, "r_mipi_1_1v or r_mipi_1_8v is NULL.\n");
> +		return -EINVAL;
> +	}
> +
> +	if (enable) {
> +		if (r_mipi_1_1v)
> +			ret = regulator_enable(r_mipi_1_1v);
> +
> +		if (ret < 0) {
> +			dev_err(dsim->dev,
> +				"failed to enable regulator mipi_1_1v.\n");
> +			return ret;
> +		}
> +
> +		if (r_mipi_1_8v)
> +			ret = regulator_enable(r_mipi_1_8v);
> +
> +		if (ret < 0) {
> +			dev_err(dsim->dev,
> +				"failed to enable regulator mipi_1_8v.\n");
> +			return ret;
> +		}
> +	} else {
> +		if (r_mipi_1_1v)
> +			ret = regulator_force_disable(r_mipi_1_1v);
> +		if (ret < 0) {
> +			dev_err(dsim->dev,
> +				"failed to disable regulator mipi_1_1v.\n");
> +			return ret;
> +		}
> +
> +		if (r_mipi_1_8v)
> +			ret = regulator_force_disable(r_mipi_1_8v);
> +		if (ret < 0) {
> +			dev_err(dsim->dev,
> +				"failed to disable regulator mipi_1_8v.\n");
> +			return ret;
> +		}
> +	}
> +
> +	return ret;
> +}
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 3d94a14..c916ac1 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -1930,7 +1930,7 @@ config FB_TMIO_ACCELL
>  
>  config FB_S3C
>  	tristate "Samsung S3C framebuffer support"
> -	depends on FB && ARCH_S3C64XX
> +	depends on FB && (ARCH_S3C64XX || ARCH_S5PV210)
>  	select FB_CFB_FILLRECT
>  	select FB_CFB_COPYAREA
>  	select FB_CFB_IMAGEBLIT
> @@ -1975,6 +1975,13 @@ config FB_S3C2410_DEBUG
>  	  Turn on debugging messages. Note that you can set/unset at run time
>  	  through sysfs
>  
> +config S5P_MIPI_DSI
> +	tristate "Samsung SoC MIPI-DSI support."
> +	depends on FB_S3C && ARCH_S5PV210
> +	default n
> +	---help---
> +	  This enables support for MIPI-DSI device.
> +
>  config FB_NUC900
>          bool "NUC900 LCD framebuffer support"
>          depends on FB && ARCH_W90X900
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index ddc2af2..d841433 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -115,6 +115,8 @@ obj-$(CONFIG_FB_SH7760)		  += sh7760fb.o
>  obj-$(CONFIG_FB_IMX)              += imxfb.o
>  obj-$(CONFIG_FB_S3C)		  += s3c-fb.o
>  obj-$(CONFIG_FB_S3C2410)	  += s3c2410fb.o
> +obj-$(CONFIG_S5P_MIPI_DSI)	  += s5p-dsim.o s5p_dsim_common.o \
> +    					s5p_dsim_lowlevel.o
>  obj-$(CONFIG_FB_FSL_DIU)	  += fsl-diu-fb.o
>  obj-$(CONFIG_FB_COBALT)           += cobalt_lcdfb.o
>  obj-$(CONFIG_FB_PNX4008_DUM)	  += pnx4008/
> diff --git a/drivers/video/s5p-dsim.c b/drivers/video/s5p-dsim.c
> new file mode 100644
> index 0000000..96893bc
> --- /dev/null
> +++ b/drivers/video/s5p-dsim.c
> @@ -0,0 +1,483 @@
> +/* linux/drivers/video/samsung/s5p-dsim.c
> + *
> + * Samsung MIPI-DSIM driver.
> + *
> + * InKi Dae, <ink...@sa...>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/clk.h>
> +#include <linux/mutex.h>
> +#include <linux/wait.h>
> +#include <linux/fs.h>
> +#include <linux/mm.h>
> +#include <linux/fb.h>
> +#include <linux/ctype.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/memory.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/kthread.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/notifier.h>
> +
> +#include <plat/fb.h>
> +#include <plat/regs-dsim.h>
> +#include <plat/dsim.h>
> +#include <plat/mipi_ddi.h>
> +
> +#include <mach/map.h>
> +
> +#include "s5p_dsim_common.h"
> +
> +struct mipi_lcd_info {
> +	struct list_head	list;
> +	struct mipi_lcd_driver	*mipi_drv;
> +};
> +
> +static LIST_HEAD(lcd_info_list);
> +static DEFINE_MUTEX(mipi_lock);
> +
> +struct dsim_global dsim;
> +
> +struct s5p_platform_dsim *to_dsim_plat(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +
> +	return (struct s5p_platform_dsim *)pdev->dev.platform_data;
> +}

that's return dev->platform_data.

> +/*
> + * notifier callback function for fb_blank
> + * - this function would be called by device specific fb_blank.
> + */
> +static int s5p_dsim_notifier_callback(struct notifier_block *self,
> +	unsigned long event, void *data)
> +{
> +	pm_message_t pm;
> +
> +	pm.event = 0;

do we really need to produce this pm structure.

> +	switch (event) {
> +	case FB_BLANK_UNBLANK:
> +	case FB_BLANK_NORMAL:
> +		if (dsim.pd->mipi_power)
> +			dsim.pd->mipi_power(&dsim, (void *) dsim.r_mipi_1_1v,
> +				(void *) dsim.r_mipi_1_8v, 1);
> +
> +		clk_enable(dsim.clock);
> +
> +		if (dsim.mipi_drv->resume)
> +			dsim.mipi_drv->resume(dsim.dev);
> +
> +		s5p_dsim_init_dsim(&dsim);
> +		s5p_dsim_init_link(&dsim);
> +
> +		s5p_dsim_set_hs_enable(&dsim);
> +		s5p_dsim_set_data_transfer_mode(&dsim,
> +			DSIM_TRANSFER_BYCPU, 1);
> +
> +		/* it needs delay for stabilization */
> +		mdelay(dsim.pd->delay_for_stabilization);
> +
> +		if (dsim.mipi_drv->init)
> +			dsim.mipi_drv->init(dsim.dev);
> +		else
> +			dev_warn(dsim.dev, "init func is null.\n");
> +
> +		s5p_dsim_set_display_mode(&dsim, dsim.dsim_lcd_info, NULL);
> +
> +		s5p_dsim_set_data_transfer_mode(&dsim, DSIM_TRANSFER_BYLCDC, 1);
> +		dsim.mipi_ddi_pd->resume_complete = 1;
> +
> +		dev_dbg(dsim.dev, "FB_BLANK_NORMAL or UNBLANK.\n");
> +
> +		break;
> +	case FB_BLANK_POWERDOWN:
> +		dsim.mipi_ddi_pd->resume_complete = 0;
> +
> +		if (dsim.mipi_drv->suspend)
> +			dsim.mipi_drv->suspend(dsim.dev, pm);
> +
> +		clk_disable(dsim.clock);
> +
> +		if (dsim.pd->mipi_power)
> +			dsim.pd->mipi_power(&dsim, (void *) dsim.r_mipi_1_1v,
> +				(void *) dsim.r_mipi_1_8v, 0);
> +
> +		dev_dbg(dsim.dev, "FB_BLANK_POWERDOWN.\n");
> +		break;
> +	default:
> +		dev_warn(dsim.dev, "unknown FB_BLANK command.\n");
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int s5p_dsim_register_notif(struct device *dev)
> +{
> +	memset(&dsim.s3cfb_notif, 0, sizeof(struct notifier_block));
> +	dsim.s3cfb_notif.notifier_call = s5p_dsim_notifier_callback;
> +
> +	return 0/*s3cfb_register_client(&dsim.s3cfb_notif)*/;
> +}
> +
> +static irqreturn_t s5p_dsim_interrupt_handler(int irq, void *dev_id)
> +{
> +	disable_irq(irq);
> +
> +	/* additional work. */
> +
> +	enable_irq(irq);
> +
> +	return IRQ_HANDLED;
> +}

?

> +
> +int s5p_dsim_register_lcd_driver(struct mipi_lcd_driver *lcd_drv)
> +{
> +	struct mipi_lcd_info	*lcd_info = NULL;
> +
> +	lcd_info = kmalloc(sizeof(struct mipi_lcd_info), GFP_KERNEL);
> +	if (lcd_info == NULL)
> +		return -ENOMEM;
> +
> +	lcd_info->mipi_drv = kmalloc(sizeof(struct mipi_lcd_driver),
> +		GFP_KERNEL);
> +	if (lcd_info->mipi_drv == NULL)
> +		return -ENOMEM;
> +
> +
> +	memcpy(lcd_info->mipi_drv, lcd_drv, sizeof(struct mipi_lcd_driver));
> +
> +	mutex_lock(&mipi_lock);
> +	list_add_tail(&lcd_info->list, &lcd_info_list);
> +	mutex_unlock(&mipi_lock);
> +
> +	dev_dbg(dsim.dev, "registered panel driver(%s) to mipi-dsi driver.\n",
> +		lcd_drv->name);
> +
> +	return 0;
> +}
> +
> +/*
> + * This function is wrapper for changing transfer mode.
> + * It is used to in panel driver before and after changing gamma value.
> + */
> +static int s5p_dsim_change_transfer_mode(int mode)
> +{
> +	if (mode < 0 || mode > 1) {
> +		dev_err(dsim.dev, "mode range should be 0 or 1.\n");
> +		return -EFAULT;
> +	}
> +
> +	if (mode == 0)
> +		s5p_dsim_set_data_transfer_mode(&dsim,
> +			DSIM_TRANSFER_BYCPU, mode);
> +	else
> +		s5p_dsim_set_data_transfer_mode(&dsim,
> +			DSIM_TRANSFER_BYLCDC, mode);
> +
> +	return 0;
> +}
> +
> +struct mipi_lcd_driver *scan_mipi_driver(const char *name)
> +{
> +	struct mipi_lcd_info *lcd_info;
> +	struct mipi_lcd_driver *mipi_drv = NULL;
> +
> +	mutex_lock(&mipi_lock);
> +
> +	dev_dbg(dsim.dev, "find lcd panel driver(%s).\n",
> +		name);
> +
> +	list_for_each_entry(lcd_info, &lcd_info_list, list) {
> +		mipi_drv = lcd_info->mipi_drv;
> +
> +		if ((strcmp(mipi_drv->name, name)) == 0) {
> +			mutex_unlock(&mipi_lock);
> +			dev_dbg(dsim.dev, "found!!!(%s).\n", mipi_drv->name);
> +			return mipi_drv;
> +		}
> +	}
> +
> +	dev_warn(dsim.dev, "failed to find lcd panel driver(%s).\n",
> +		name);
> +
> +	mutex_unlock(&mipi_lock);
> +
> +	return NULL;
> +}
> +
> +static int s5p_dsim_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	int ret = -1;
> +
> +	dsim.pd = to_dsim_plat(&pdev->dev);
> +	dsim.dev = &pdev->dev;
> +
> +	/* set dsim config data, dsim lcd config data and lcd panel data. */
> +	dsim.dsim_info = dsim.pd->dsim_info;
> +	dsim.dsim_lcd_info = dsim.pd->dsim_lcd_info;
> +	dsim.lcd_panel_info =
> +		(struct fb_videomode *) dsim.dsim_lcd_info->lcd_panel_info;

why isn't this in the correct type to begin with.

> +	dsim.mipi_ddi_pd =
> +		(struct mipi_ddi_platform_data *)
> +			dsim.dsim_lcd_info->mipi_ddi_pd;

and again.

> +	dsim.mipi_ddi_pd->resume_complete = 0;
> +
> +	dsim.r_mipi_1_1v = regulator_get(&pdev->dev, "VMIPI_1.1V");
> +	if (IS_ERR(dsim.r_mipi_1_1v)) {
> +		dev_err(&pdev->dev, "failed to get regulator VMIPI_1.1V.\n");
> +		goto regulator_get_err;
> +	}
> +
> +	dsim.r_mipi_1_8v = regulator_get(&pdev->dev, "VMIPI_1.8V");
> +	if (IS_ERR(dsim.r_mipi_1_8v)) {
> +		dev_err(&pdev->dev, "failed to get regulator VMIPI_1.8V.\n");
> +		goto regulator_get_err;
> +	}
> +
> +	/* clock */
> +	dsim.clock = clk_get(&pdev->dev, dsim.pd->clk_name);
> +	if (IS_ERR(dsim.clock)) {
> +		dev_err(&pdev->dev, "failed to get dsim clock source\n");
> +		return -EINVAL;
> +	}
> +
> +	clk_enable(dsim.clock);
> +
> +	/* io memory */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to get io memory region\n");
> +		ret = -EINVAL;
> +		goto err_clk_disable;
> +	}
> +
> +	/* request mem region */
> +	res = request_mem_region(res->start,
> +				 res->end - res->start + 1, pdev->name);
resource_size()

> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to request io memory region\n");
> +		ret = -EINVAL;
> +		goto err_clk_disable;
> +	}
> +
> +	/* ioremap for register block */
> +	dsim.reg_base = (unsigned int) ioremap(res->start,
> +		res->end - res->start + 1);

ARGH. dsim.reg_base should be 'void __iomem *'

> +	if (!dsim.reg_base) {
> +		dev_err(&pdev->dev, "failed to remap io region\n");
> +		ret = -EINVAL;
> +		goto err_clk_disable;
> +	}
> +
> +	/* it is used for MIPI-DSI based lcd panel driver. */
> +	dsim.mipi_ddi_pd->dsim_data = (void *)&dsim;
> +
> +	/*
> +	 * it uses frame done interrupt handler
> +	 * only in case of MIPI Video mode.
> +	 */
> +	if (dsim.dsim_lcd_info->e_interface == DSIM_VIDEO) {
> +		dsim.irq = platform_get_irq(pdev, 0);
> +		if (request_irq(dsim.irq, s5p_dsim_interrupt_handler,
> +				IRQF_TRIGGER_RISING, "mipi-dsi", &dsim)) {

do internal interrupts really need a trigger flag?

> +			dev_err(&pdev->dev, "request_irq failed.\n");
> +			goto err_trigger_irq;
> +		}
> +	}
> +
> +	if (dsim.pd->mipi_power)
> +		dsim.pd->mipi_power(&dsim, (void *) dsim.r_mipi_1_1v,
> +			(void *) dsim.r_mipi_1_8v, 1);
> +	else {
> +		dev_err(&pdev->dev, "mipi_power is NULL.\n");
> +		goto mipi_power_err;
> +	}
> +
> +	/* find lcd panel driver registered to mipi-dsi driver. */
> +	dsim.mipi_drv = scan_mipi_driver(dsim.pd->lcd_panel_name);
> +	if (dsim.mipi_drv == NULL) {
> +		dev_err(&pdev->dev, "mipi_drv is NULL.\n");
> +		goto mipi_drv_err;
> +	}
> +
> +	/* register callback functions that lcd panel driver needs. */
> +	dsim.mipi_ddi_pd->cmd_write = s5p_dsim_wr_data;
> +	dsim.mipi_ddi_pd->cmd_read = NULL;
> +	dsim.mipi_ddi_pd->get_dsim_frame_done =
> +		s5p_dsim_get_frame_done_status;
> +	dsim.mipi_ddi_pd->clear_dsim_frame_done = s5p_dsim_clear_frame_done;
> +	dsim.mipi_ddi_pd->change_dsim_transfer_mode =
> +		s5p_dsim_change_transfer_mode;
> +	dsim.mipi_ddi_pd->get_fb_frame_done = dsim.pd->get_fb_frame_done;
> +	dsim.mipi_ddi_pd->trigger = dsim.pd->trigger;

this looks like it should have been in a struture to be copied.

> +	/* set lcd panel driver link */
> +	ret = dsim.mipi_drv->set_link(dsim.mipi_ddi_pd);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to set link.\n");
> +		goto mipi_drv_err;
> +	}
> +
> +	dsim.mipi_drv->probe(&pdev->dev);
> +
> +	s5p_dsim_init_dsim(&dsim);
> +	s5p_dsim_init_link(&dsim);
> +
> +	s5p_dsim_set_hs_enable(&dsim);
> +	s5p_dsim_set_data_transfer_mode(&dsim, DSIM_TRANSFER_BYCPU, 1);
> +
> +	/* it needs delay for stabilization */
> +	mdelay(dsim.pd->delay_for_stabilization);
> +
> +	/* initialize lcd panel */
> +	if (dsim.mipi_drv->init)
> +		dsim.mipi_drv->init(&pdev->dev);
> +	else
> +		dev_warn(&pdev->dev, "init func is null.\n");
> +
> +	if (dsim.mipi_drv->display_on)
> +		dsim.mipi_drv->display_on(&pdev->dev);
> +	else
> +		dev_warn(&pdev->dev, "display_on func is null.\n");
> +
> +	s5p_dsim_set_display_mode(&dsim, dsim.dsim_lcd_info, NULL);
> +
> +	s5p_dsim_set_data_transfer_mode(&dsim, DSIM_TRANSFER_BYLCDC, 1);
> +
> +	s5p_dsim_register_notif(&pdev->dev);
> +
> +	/* in case of command mode, trigger. */
> +	if (dsim.dsim_lcd_info->e_interface == DSIM_COMMAND) {
> +		if (dsim.pd->trigger)
> +			dsim.pd->trigger(registered_fb[0]);
> +		else
> +			dev_warn(&pdev->dev, "trigger is null.\n");
> +	}
> +
> +	dev_info(&pdev->dev, "mipi-dsi driver(%s mode) has been probed.\n",
> +		(dsim.dsim_lcd_info->e_interface == DSIM_COMMAND) ?
> +			"CPU" : "RGB");
> +
> +	return 0;
> +
> +err_trigger_irq:
> +mipi_drv_err:
> +	dsim.pd->mipi_power(&dsim, (void *) dsim.r_mipi_1_1v,
> +		(void *) dsim.r_mipi_1_8v, 0);
> +
> +mipi_power_err:
> +	iounmap((void __iomem *) dsim.reg_base);
> +
> +err_clk_disable:
> +	clk_disable(dsim.clock);
> +
> +regulator_get_err:
> +
> +	return ret;
> +
> +}
> +
> +static int s5p_dsim_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +int s5p_dsim_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	dsim.mipi_ddi_pd->resume_complete = 0;
> +
> +	if (dsim.mipi_drv->suspend)
> +		dsim.mipi_drv->suspend(&pdev->dev, state);
> +
> +	clk_disable(dsim.clock);
> +
> +	if (dsim.pd->mipi_power)
> +		dsim.pd->mipi_power(&dsim, (void *) dsim.r_mipi_1_1v,
> +			(void *) dsim.r_mipi_1_8v, 0);
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_resume(struct platform_device *pdev)
> +{
> +	if (dsim.pd->mipi_power)
> +		dsim.pd->mipi_power(&dsim, (void *) dsim.r_mipi_1_1v,
> +			(void *) dsim.r_mipi_1_8v, 1);
> +
> +	clk_enable(dsim.clock);
> +
> +	if (dsim.mipi_drv->resume)
> +		dsim.mipi_drv->resume(&pdev->dev);
> +
> +	s5p_dsim_init_dsim(&dsim);
> +	s5p_dsim_init_link(&dsim);
> +
> +	s5p_dsim_set_hs_enable(&dsim);
> +	s5p_dsim_set_data_transfer_mode(&dsim, DSIM_TRANSFER_BYCPU, 1);
> +
> +	/* it needs delay for stabilization */
> +	mdelay(dsim.pd->delay_for_stabilization);
> +
> +	/* initialize lcd panel */
> +	if (dsim.mipi_drv->init)
> +		dsim.mipi_drv->init(&pdev->dev);
> +	else
> +		dev_warn(&pdev->dev, "init func is null.\n");
> +
> +	s5p_dsim_set_display_mode(&dsim, dsim.dsim_lcd_info, NULL);
> +
> +	s5p_dsim_set_data_transfer_mode(&dsim, DSIM_TRANSFER_BYLCDC, 1);
> +
> +	dsim.mipi_ddi_pd->resume_complete = 1;
> +
> +	return 0;
> +}
> +#else
> +#define s5p_dsim_suspend NULL
> +#define s5p_dsim_resume NULL
> +#endif
> +
> +static struct platform_driver s5p_dsim_driver = {
> +	.probe = s5p_dsim_probe,
> +	.remove = s5p_dsim_remove,
> +	.suspend = s5p_dsim_suspend,
> +	.resume = s5p_dsim_resume,
> +	.driver = {
> +		   .name = "s5p-dsim",
> +		   .owner = THIS_MODULE,
> +	},
> +};
> +
> +static int s5p_dsim_register(void)
> +{
> +	platform_driver_register(&s5p_dsim_driver);
> +
> +	return 0;
> +}
> +
> +static void s5p_dsim_unregister(void)
> +{
> +	platform_driver_unregister(&s5p_dsim_driver);
> +}
> +
> +module_init(s5p_dsim_register);
> +module_exit(s5p_dsim_unregister);
> +
> +MODULE_AUTHOR("InKi Dae <ink...@sa...>");
> +MODULE_DESCRIPTION("Samusung MIPI-DSIM driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/video/s5p_dsim_common.c b/drivers/video/s5p_dsim_common.c
> new file mode 100644
> index 0000000..77724dc
> --- /dev/null
> +++ b/drivers/video/s5p_dsim_common.c
> @@ -0,0 +1,753 @@
> +/* linux/drivers/video/samsung/s5p_dsim_common.c
> + *
> + * Samsung MIPI-DSIM common driver.
> + *
> + * InKi Dae, <ink...@sa...>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/mutex.h>
> +#include <linux/wait.h>
> +#include <linux/fs.h>
> +#include <linux/mm.h>
> +#include <linux/fb.h>
> +#include <linux/ctype.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/memory.h>
> +#include <linux/delay.h>
> +#include <linux/kthread.h>
> +
> +#include <plat/fb.h>
> +#include <plat/regs-dsim.h>
> +
> +#include <mach/map.h>
> +#include <plat/dsim.h>
> +#include <plat/mipi_ddi.h>
> +
> +#include "s5p_dsim_lowlevel.h"
> +
> +static void s5p_dsim_long_data_wr(struct dsim_global *dsim, unsigned int data0,
> +	unsigned int data1)
> +{
> +	unsigned int data_cnt = 0, payload = 0;
> +
> +	/* in case that data count is more then 4 */
> +	for (data_cnt = 0; data_cnt < data1; data_cnt += 4) {
> +		/*
> +		 * after sending 4bytes per one time,
> +		 * send remainder data less then 4.
> +		 */
> +		if ((data1 - data_cnt) < 4) {
> +			if ((data1 - data_cnt) == 3) {
> +				payload = *(u8 *)(data0 + data_cnt) |
> +				    (*(u8 *)(data0 + (data_cnt + 1))) << 8 |
> +					(*(u8 *)(data0 + (data_cnt + 2))) << 16;

Erm, why wheren't these types kept as 'u8 *', this amount of casting
should be ringing alarm bells all over the place.

> +			dev_dbg(dsim->dev, "count = 3 payload = %x, %x %x %x\n",
> +				payload, *(u8 *)(data0 + data_cnt),
> +				*(u8 *)(data0 + (data_cnt + 1)),
> +				*(u8 *)(data0 + (data_cnt + 2)));
> +			} else if ((data1 - data_cnt) == 2) {
> +				payload = *(u8 *)(data0 + data_cnt) |
> +					(*(u8 *)(data0 + (data_cnt + 1))) << 8;
> +			dev_dbg(dsim->dev,
> +				"count = 2 payload = %x, %x %x\n", payload,
> +				*(u8 *)(data0 + data_cnt),
> +				*(u8 *)(data0 + (data_cnt + 1)));
> +			} else if ((data1 - data_cnt) == 1) {
> +				payload = *(u8 *)(data0 + data_cnt);
> +			}
> +
> +			s5p_dsim_wr_tx_data(dsim, payload);
> +		/* send 4bytes per one time. */
> +		} else {
> +			payload = *(u8 *)(data0 + data_cnt) |
> +				(*(u8 *)(data0 + (data_cnt + 1))) << 8 |
> +				(*(u8 *)(data0 + (data_cnt + 2))) << 16 |
> +				(*(u8 *)(data0 + (data_cnt + 3))) << 24;
> +
> +			dev_dbg(dsim->dev,
> +				"count = 4 payload = %x, %x %x %x %x\n",
> +				payload, *(u8 *)(data0 + data_cnt),
> +				*(u8 *)(data0 + (data_cnt + 1)),
> +				*(u8 *)(data0 + (data_cnt + 2)),
> +				*(u8 *)(data0 + (data_cnt + 3)));
> +
> +			s5p_dsim_wr_tx_data(dsim, payload);
> +		}
> +	}
> +}
> +
> +int s5p_dsim_wr_data(void *dsim_data, unsigned int data_id,
> +	unsigned int data0, unsigned int data1)
> +{
> +	struct dsim_global *dsim = NULL;
> +	unsigned int timeout = 5000 * 2;
> +	unsigned long delay_val, udelay;
> +	unsigned char check_rx_ack = 0;
> +
> +	dsim = (struct dsim_global *)dsim_data;
> +
> +	if (dsim == NULL) {
> +		dev_err(dsim->dev, "dsim_data is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +
> +	if (dsim->state == DSIM_STATE_ULPS) {
> +		dev_err(dsim->dev, "state is ULPS.\n");
> +
> +		return -EINVAL;
> +	}
> +
> +	delay_val = 1000000 / dsim->dsim_info->esc_clk;
> +	udelay = 10 * delay_val;
> +
> +	mdelay(udelay);
> +
> +	/* only if transfer mode is LPDT, wait SFR becomes empty. */
> +	if (dsim->state == DSIM_STATE_STOP) {
> +		while (!(s5p_dsim_get_fifo_state(dsim) &
> +				SFR_HEADER_EMPTY)) {
> +			if ((timeout--) > 0)
> +				mdelay(1);
> +			else {
> +				dev_err(dsim->dev,
> +					"SRF header fifo is not empty.\n");
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	switch (data_id) {
> +	/* short packet types of packet types for command. */
> +	case GEN_SHORT_WR_NO_PARA:
> +	case GEN_SHORT_WR_1_PARA:
> +	case GEN_SHORT_WR_2_PARA:
> +	case DCS_WR_NO_PARA:
> +	case DCS_WR_1_PARA:
> +	case SET_MAX_RTN_PKT_SIZE:
> +		s5p_dsim_wr_tx_header(dsim, (unsigned char) data_id,
> +			(unsigned char) data0, (unsigned char) data1);
> +		if (check_rx_ack)
> +			/* process response func should be implemented */
> +			return 0;
> +		else
> +			return -EINVAL;
> +
> +	/* general command */
> +	case CMD_OFF:
> +	case CMD_ON:
> +	case SHUT_DOWN:
> +	case TURN_ON:
> +		s5p_dsim_wr_tx_header(dsim, (unsigned char) data_id,
> +			(unsigned char) data0, (unsigned char) data1);
> +		if (check_rx_ack)
> +			/* process response func should be implemented. */
> +			return 0;
> +		else
> +			return -EINVAL;
> +
> +	/* packet types for video data */
> +	case VSYNC_START:
> +	case VSYNC_END:
> +	case HSYNC_START:
> +	case HSYNC_END:
> +	case EOT_PKT:
> +		return 0;
> +
> +	/* short and response packet types for command */
> +	case GEN_RD_1_PARA:
> +	case GEN_RD_2_PARA:
> +	case GEN_RD_NO_PARA:
> +	case DCS_RD_NO_PARA:
> +		s5p_dsim_clear_interrupt(dsim, 0xffffffff);
> +		s5p_dsim_wr_tx_header(dsim, (unsigned char) data_id,
> +			(unsigned char) data0, (unsigned char) data1);
> +		/* process response func should be implemented. */
> +		return 0;
> +
> +	/* long packet type and null packet */
> +	case NULL_PKT:
> +	case BLANKING_PKT:
> +		return 0;
> +	case GEN_LONG_WR:
> +	case DCS_LONG_WR:
> +	{
> +		unsigned int size, data_cnt = 0, payload = 0;
> +
> +		size = data1 * 4;
> +
> +		/* if data count is less then 4, then send 3bytes data.  */
> +		if (data1 < 4) {
> +			payload = *(u8 *)(data0) |
> +				*(u8 *)(data0 + 1) << 8 |
> +				*(u8 *)(data0 + 2) << 16;
> +
> +			s5p_dsim_wr_tx_data(dsim, payload);
> +
> +			dev_dbg(dsim->dev, "count = %d payload = %x,%x %x %x\n",
> +				data1, payload,
> +				*(u8 *)(data0 + data_cnt),
> +				*(u8 *)(data0 + (data_cnt + 1)),
> +				*(u8 *)(data0 + (data_cnt + 2)));
> +		/* in case that data count is more then 4 */
> +		} else
> +			s5p_dsim_long_data_wr(dsim, data0, data1);
> +
> +		/* put data into header fifo */
> +		s5p_dsim_wr_tx_header(dsim, (unsigned char) data_id,
> +			(unsigned char) (((unsigned short) data1) & 0xff),
> +			(unsigned char) ((((unsigned short) data1) & 0xff00) >>
> +				8));
> +
> +	}
> +	if (check_rx_ack)
> +		/* process response func should be implemented. */
> +		return 0;
> +	else
> +		return -EINVAL;
> +
> +	/* packet typo for video data */
> +	case RGB565_PACKED:
> +	case RGB666_PACKED:
> +	case RGB666_LOOSLY:
> +	case RGB888_PACKED:
> +		if (check_rx_ack)
> +			/* process response func should be implemented. */
> +			return 0;
> +		else
> +			return -EINVAL;
> +	default:
> +		dev_warn(dsim->dev,
> +			"data id %x is not supported current DSI spec.\n",
> +			data_id);
> +
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_init_header_fifo(struct dsim_global *dsim)
> +{
> +	unsigned int cnt;
> +
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim_global pointer is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	for (cnt = 0; cnt < DSIM_HEADER_FIFO_SZ; cnt++)
> +		dsim->header_fifo_index[cnt] = -1;
> +	return 0;
> +}
> +
> +int s5p_dsim_pll_on(struct dsim_global *dsim, unsigned char enable)

how about 'bool' for enable.

> +{
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim_global pointer is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	if (enable) {
> +		int sw_timeout = 1000;
> +		s5p_dsim_clear_interrupt(dsim, DSIM_PLL_STABLE);
> +		s5p_dsim_enable_pll(dsim, 1);
> +		while (1) {
> +			sw_timeout--;
> +			if (s5p_dsim_is_pll_stable(dsim))
> +				return 0;
> +			if (sw_timeout == 0)
> +				return -EINVAL;
> +		}
> +	} else
> +		s5p_dsim_enable_pll(dsim, 0);
> +
> +	return 0;
> +}
> +
> +unsigned long s5p_dsim_change_pll(struct dsim_global *dsim,
> +	unsigned char pre_divider, unsigned short main_divider,
> +	unsigned char scaler)
> +{
> +	unsigned long dfin_pll, dfvco, dpll_out;
> +	unsigned char freq_band;
> +
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim_global pointer is NULL.\n");
> +		return 0;
> +	}
> +
> +	dfin_pll = (MIPI_FIN / pre_divider);
> +
> +	if (dfin_pll < 6 * 1000 * 1000 || dfin_pll > 12 * 1000 * 1000) {
> +		dev_warn(dsim->dev, "warning!!\n");
> +		dev_warn(dsim->dev, "fin_pll range is 6MHz ~ 12MHz\n");
> +		dev_warn(dsim->dev, "fin_pll of mipi dphy pll is %luMHz\n",
> +			(dfin_pll / 1000000));
> +
> +		s5p_dsim_enable_afc(dsim, 0, 0);
> +	} else {
> +		if (dfin_pll < 7 * 1000000)
> +			s5p_dsim_enable_afc(dsim, 1, 0x1);
> +		else if (dfin_pll < 8 * 1000000)
> +			s5p_dsim_enable_afc(dsim, 1, 0x0);
> +		else if (dfin_pll < 9 * 1000000)
> +			s5p_dsim_enable_afc(dsim, 1, 0x3);
> +		else if (dfin_pll < 10 * 1000000)
> +			s5p_dsim_enable_afc(dsim, 1, 0x2);
> +		else if (dfin_pll < 11 * 1000000)
> +			s5p_dsim_enable_afc(dsim, 1, 0x5);
> +		else
> +			s5p_dsim_enable_afc(dsim, 1, 0x4);
> +	}
> +
> +	dfvco = dfin_pll * main_divider;
> +	dev_dbg(dsim->dev, "dfvco = %lu, dfin_pll = %lu, main_divider = %d\n",
> +		dfvco, dfin_pll, main_divider);
> +	if (dfvco < 500000000 || dfvco > 1000000000) {
> +		dev_warn(dsim->dev, "Caution!!\n");
> +		dev_warn(dsim->dev, "fvco range is 500MHz ~ 1000MHz\n");
> +		dev_warn(dsim->dev, "fvco of mipi dphy pll is %luMHz\n",
> +			(dfvco / 1000000));
> +	}
> +
> +	dpll_out = dfvco / (1 << scaler);
> +	dev_dbg(dsim->dev, "dpll_out = %lu, dfvco = %lu, scaler = %d\n",
> +		dpll_out, dfvco, scaler);
> +	if (dpll_out < 100 * 1000000)
> +		freq_band = 0x0;
> +	else if (dpll_out < 120 * 1000000)
> +		freq_band = 0x1;
> +	else if (dpll_out < 170 * 1000000)
> +		freq_band = 0x2;
> +	else if (dpll_out < 220 * 1000000)
> +		freq_band = 0x3;
> +	else if (dpll_out < 270 * 1000000)
> +		freq_band = 0x4;
> +	else if (dpll_out < 320 * 1000000)
> +		freq_band = 0x5;
> +	else if (dpll_out < 390 * 1000000)
> +		freq_band = 0x6;
> +	else if (dpll_out < 450 * 1000000)
> +		freq_band = 0x7;
> +	else if (dpll_out < 510 * 1000000)
> +		freq_band = 0x8;
> +	else if (dpll_out < 560 * 1000000)
> +		freq_band = 0x9;
> +	else if (dpll_out < 640 * 1000000)
> +		freq_band = 0xa;
> +	else if (dpll_out < 690 * 1000000)
> +		freq_band = 0xb;
> +	else if (dpll_out < 770 * 1000000)
> +		freq_band = 0xc;
> +	else if (dpll_out < 870 * 1000000)
> +		freq_band = 0xd;
> +	else if (dpll_out < 950 * 1000000)
> +		freq_band = 0xe;
> +	else
> +		freq_band = 0xf;

something says a divide down before the ompatr would have been
a good idea, it is almost a table.

> +	dev_dbg(dsim->dev, "freq_band = %d\n", freq_band);
> +
> +	s5p_dsim_pll_freq(dsim, pre_divider, main_divider, scaler);
> +
> +	{
> +	    unsigned char temp0, temp1;
> +
> +	    temp0 = 0;
> +	    s5p_dsim_hs_zero_ctrl(dsim, temp0);
> +	    temp1 = 0;
> +	    s5p_dsim_prep_ctrl(dsim, temp1);
> +	}
> +
> +	/* Freq Band */
> +	s5p_dsim_pll_freq_band(dsim, freq_band);
> +
> +	/* Stable time */
> +	s5p_dsim_pll_stable_time(dsim,
> +		dsim->dsim_info->pll_stable_time);
> +
> +	/* Enable PLL */
> +	dev_dbg(dsim->dev, "FOUT of mipi dphy pll is %luMHz\n",
> +		(dpll_out / 1000000));
> +
> +	return dpll_out;
> +}
> +
> +int s5p_dsim_set_clock(struct dsim_global *dsim,
> +	unsigned char byte_clk_sel, unsigned char enable)
> +{
> +	unsigned int esc_div;
> +	unsigned long esc_clk_error_rate;
> +
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim_global pointer is NULL.\n");
> +		return -EINVAL;
> +	}

again, how about making this code WARN_ON?


> +	if (enable) {
> +		dsim->e_clk_src = byte_clk_sel;
> +
> +		/* Escape mode clock and byte clock source */
> +		s5p_dsim_set_byte_clock_src(dsim, byte_clk_sel);
> +
> +		/* DPHY, DSIM Link : D-PHY clock out */
> +		if (byte_clk_sel == DSIM_PLL_OUT_DIV8) {
> +			dsim->hs_clk = s5p_dsim_change_pll(dsim,
> +				dsim->dsim_info->p, dsim->dsim_info->m,
> +				dsim->dsim_info->s);
> +			if (dsim->hs_clk == 0) {
> +				dev_err(dsim->dev,
> +					"failed to get hs clock.\n");
> +				return -EINVAL;
> +			}
> +
> +			dsim->byte_clk = dsim->hs_clk / 8;
> +			s5p_dsim_enable_pll_bypass(dsim, 0);
> +			s5p_dsim_pll_on(dsim, 1);
> +		/* DPHY : D-PHY clock out, DSIM link : external clock out */
> +		} else if (byte_clk_sel == DSIM_EXT_CLK_DIV8)
> +			dev_warn(dsim->dev,
> +				"this project is not support \
> +				external clock source for MIPI DSIM\n");
> +		else if (byte_clk_sel == DSIM_EXT_CLK_BYPASS)
> +			dev_warn(dsim->dev,
> +				"this project is not support \
> +				external clock source for MIPI DSIM\n");
> +
> +		/* escape clock divider */
> +		esc_div = dsim->byte_clk / (dsim->dsim_info->esc_clk);
> +		dev_dbg(dsim->dev,
> +			"esc_div = %d, byte_clk = %lu, esc_clk = %lu\n",
> +			esc_div, dsim->byte_clk, dsim->dsim_info->esc_clk);
> +		if ((dsim->byte_clk / esc_div) >= 20000000 ||
> +			(dsim->byte_clk / esc_div) > dsim->dsim_info->esc_clk)
> +			esc_div += 1;
> +
> +		dsim->escape_clk = dsim->byte_clk / esc_div;
> +		dev_dbg(dsim->dev,
> +			"escape_clk = %lu, byte_clk = %lu, esc_div = %d\n",
> +			dsim->escape_clk, dsim->byte_clk, esc_div);
> +
> +		/*
> +		 * enable escclk on lane
> +		 *
> +		 * in case of evt0, DSIM_TRUE is enable and
> +		 * DSIM_FALSE is enable for evt1.
> +		 */
> +		if (dsim->pd->platform_rev == 1)
> +			s5p_dsim_enable_byte_clock(dsim, DSIM_FALSE);
> +		else
> +			s5p_dsim_enable_byte_clock(dsim, DSIM_TRUE);
> +
> +		/* enable byte clk and escape clock */
> +		s5p_dsim_set_esc_clk_prs(dsim, 1, esc_div);
> +		/* escape clock on lane */
> +		s5p_dsim_enable_esc_clk_on_lane(dsim,
> +			(DSIM_LANE_CLOCK | dsim->data_lane), 1);
> +
> +		dev_dbg(dsim->dev, "byte clock is %luMHz\n",
> +			(dsim->byte_clk / 1000000));
> +		dev_dbg(dsim->dev, "escape clock that user's need is %lu\n",
> +			(dsim->dsim_info->esc_clk / 1000000));
> +		dev_dbg(dsim->dev, "escape clock divider is %x\n", esc_div);
> +		dev_dbg(dsim->dev, "escape clock is %luMHz\n",
> +			((dsim->byte_clk / esc_div) / 1000000));
> +
> +		if ((dsim->byte_clk / esc_div) > dsim->escape_clk) {
> +			esc_clk_error_rate = dsim->escape_clk /
> +				(dsim->byte_clk / esc_div);
> +			dev_warn(dsim->dev, "error rate is %lu over.\n",
> +				(esc_clk_error_rate / 100));
> +		} else if ((dsim->byte_clk / esc_div) < (dsim->escape_clk)) {
> +			esc_clk_error_rate = (dsim->byte_clk / esc_div) /
> +				dsim->escape_clk;
> +			dev_warn(dsim->dev, "error rate is %lu under.\n",
> +				(esc_clk_error_rate / 100));
> +		}
> +	} else {
> +		s5p_dsim_enable_esc_clk_on_lane(dsim,
> +			(DSIM_LANE_CLOCK | dsim->data_lane), 0);
> +		s5p_dsim_set_esc_clk_prs(dsim, 0, 0);
> +
> +		/*
> +		 * in case of evt0, DSIM_FALSE is disable and
> +		 * DSIM_TRUE is disable for evt1.
> +		 */
> +		if (dsim->pd->platform_rev == 1)
> +			s5p_dsim_enable_byte_clock(dsim, DSIM_TRUE);
> +		else
> +			s5p_dsim_enable_byte_clock(dsim, DSIM_FALSE);
> +
> +		if (byte_clk_sel == DSIM_PLL_OUT_DIV8)
> +			s5p_dsim_pll_on(dsim, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_init_dsim(struct dsim_global *dsim)
> +{
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim_global pointer is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	if (dsim->pd->init_d_phy)
> +		dsim->pd->init_d_phy(dsim);
> +
> +	dsim->state = DSIM_STATE_RESET;
> +
> +	switch (dsim->dsim_info->e_no_data_lane) {
> +	case DSIM_DATA_LANE_1:
> +		dsim->data_lane = DSIM_LANE_DATA0;
> +		break;
> +	case DSIM_DATA_LANE_2:
> +		dsim->data_lane = DSIM_LANE_DATA0 | DSIM_LANE_DATA1;
> +		break;
> +	case DSIM_DATA_LANE_3:
> +		dsim->data_lane = DSIM_LANE_DATA0 | DSIM_LANE_DATA1 |
> +			DSIM_LANE_DATA2;
> +		break;
> +	case DSIM_DATA_LANE_4:
> +		dsim->data_lane = DSIM_LANE_DATA0 | DSIM_LANE_DATA1 |
> +			DSIM_LANE_DATA2 | DSIM_LANE_DATA3;
> +		break;
> +	default:
> +		dev_info(dsim->dev, "data lane is invalid.\n");
> +		return -EINVAL;
> +	};
> +
> +	s5p_dsim_init_header_fifo(dsim);
> +	s5p_dsim_sw_reset(dsim);
> +	s5p_dsim_dp_dn_swap(dsim, dsim->dsim_info->e_lane_swap);
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_enable_frame_done_int(struct dsim_global *dsim, int enable)
> +{
> +	/* enable only frame done interrupt */
> +	s5p_dsim_set_interrupt_mask(dsim, INTMSK_FRAME_DONE, enable);
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_set_display_mode(struct dsim_global *dsim,
> +	struct dsim_lcd_config *main_lcd, struct dsim_lcd_config *sub_lcd)
> +{
> +	struct fb_videomode *mlcd_video = NULL;
> +	struct fb_cmdmode *mlcd_command = NULL;
> +	struct s3c_fb_pd_win *pd;
> +	unsigned int width = 0, height = 0;
> +
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim_global pointer is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	pd = (struct s3c_fb_pd_win *)main_lcd->lcd_panel_info;
> +
> +	/* in case of VIDEO MODE (RGB INTERFACE) */
> +	if (dsim->dsim_lcd_info->e_interface == (u32) DSIM_VIDEO) {
> +		mlcd_video = (struct fb_videomode *)&pd->win_mode;
> +		width = mlcd_video->xres;
> +		height = mlcd_video->yres;
> +
> +		if (dsim->dsim_info->auto_vertical_cnt == DSIM_FALSE) {
> +			s5p_dsim_set_main_disp_vporch(dsim,
> +				mlcd_video->upper_margin,
> +				mlcd_video->lower_margin, 0);
> +			s5p_dsim_set_main_disp_hporch(dsim,
> +				mlcd_video->left_margin,
> +				mlcd_video->right_margin);
> +			s5p_dsim_set_main_disp_sync_area(dsim,
> +				mlcd_video->vsync_len,
> +				mlcd_video->hsync_len);
> +		}
> +	} else {	/* in case of COMMAND MODE (CPU or I80 INTERFACE) */
> +		mlcd_command = (struct fb_cmdmode *)&pd->cmd_mode;
> +		width = mlcd_command->xres;
> +		height = mlcd_command->yres;
> +	}
> +
> +	s5p_dsim_set_main_disp_resol(dsim, height, width);
> +
> +	if (sub_lcd != NULL)
> +			dev_warn(dsim->dev, "sub lcd isn't supported yet.\n");
> +
> +	s5p_dsim_display_config(dsim, dsim->dsim_lcd_info, NULL);
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_init_link(struct dsim_global *dsim)
> +{
> +	unsigned int time_out = 100;
> +
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim_global pointer is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	switch (dsim->state) {
> +	case DSIM_STATE_RESET:
> +		s5p_dsim_sw_reset(dsim);
> +	case DSIM_STATE_INIT:
> +		s5p_dsim_init_fifo_pointer(dsim, 0x1f);
> +
> +		/* dsi configuration */
> +		s5p_dsim_init_config(dsim, dsim->dsim_lcd_info,
> +			NULL, dsim->dsim_info);
> +		s5p_dsim_enable_lane(dsim, DSIM_LANE_CLOCK, 1);
> +		s5p_dsim_enable_lane(dsim, dsim->data_lane, 1);
> +
> +		/* set clock configuration */
> +		s5p_dsim_set_clock(dsim, dsim->dsim_info->e_byte_clk,
> +			1);
> +
> +		/* check clock and data lane state is stop state */
> +		while (!(s5p_dsim_is_lane_state(dsim, DSIM_LANE_CLOCK)
> +			    == DSIM_LANE_STATE_STOP) &&
> +			!(s5p_dsim_is_lane_state(dsim,
> +				dsim->data_lane) == DSIM_LANE_STATE_STOP)) {
> +			time_out--;
> +			if (time_out == 0) {
> +				dev_info(dsim->dev,
> +					"DSI Master is not stop state.\n");
> +				dev_info(dsim->dev,
> +					"Check initialization process\n");
> +
> +				return -EINVAL;
> +			}
> +		}
> +
> +		if (time_out != 0) {
> +			dev_info(dsim->dev,
> +				"initialization of DSI Master is successful\n");
> +			dev_info(dsim->dev, "DSI Master state is stop state\n");
> +		}
> +
> +		dsim->state = DSIM_STATE_STOP;
> +
> +		/* BTA sequence counters */
> +		s5p_dsim_set_stop_state_counter(dsim,
> +			dsim->dsim_info->stop_holding_cnt);
> +		s5p_dsim_set_bta_timeout(dsim,
> +			dsim->dsim_info->bta_timeout);
> +		s5p_dsim_set_lpdr_timeout(dsim,
> +			dsim->dsim_info->rx_timeout);
> +
> +		/* default LPDT by both cpu and lcd controller */
> +		s5p_dsim_set_data_mode(dsim, DSIM_TRANSFER_BOTH,
> +			DSIM_STATE_STOP);
> +
> +		return 0;
> +	default:
> +		dev_info(dsim->dev, "DSI Master is already init.\n");
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_set_hs_enable(struct dsim_global *dsim)
> +{
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim_global pointer is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	if (dsim->state == DSIM_STATE_STOP) {
> +		if (dsim->e_clk_src != DSIM_EXT_CLK_BYPASS) {
> +			dsim->state = DSIM_STATE_HSCLKEN;
> +			s5p_dsim_set_data_mode(dsim,
> +				DSIM_TRANSFER_BOTH, DSIM_STATE_HSCLKEN);
> +			s5p_dsim_enable_hs_clock(dsim, 1);
> +
> +			return 0;
> +		} else
> +			dev_warn(dsim->dev,
> +				"clock source is external bypass.\n");
> +	} else
> +		dev_warn(dsim->dev, "DSIM is not stop state.\n");
> +
> +	return 0;
> +}
> +
> +int s5p_dsim_set_data_transfer_mode(struct dsim_global *dsim,
> +	unsigned char data_path, unsigned char hs_enable)
> +{
> +	int ret = -1;
> +
> +	if (dsim == NULL) {
> +		printk(KERN_ERR "dsim_global pointer is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	if (hs_enable) {
> +		if (dsim->state == DSIM_STATE_HSCLKEN) {
> +			s5p_dsim_set_data_mode(dsim, data_path,
> +				DSIM_STATE_HSCLKEN);
> +			ret = 0;
> +		} else {
> +			dev_err(dsim->dev, "HS Clock lane is not enabled.\n");
> +			ret = -EINVAL;
> +		}
> +	} else {
> +		if (dsim->state == DSIM_STATE_INIT || dsim->state ==
> +			DSIM_STATE_ULPS) {
> +			dev_err(dsim->dev,
> +				"DSI Master is not STOP or HSDT state.\n");
> +			ret = -EINVAL;
> +		} else {
> +			s5p_dsim_set_data_mode(dsim, data_path,
> +				DSIM_STATE_STOP);
> +			ret = 0;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +int s5p_dsim_get_frame_done_status(void *dsim_data)
> +{
> +	struct dsim_global *dsim = NULL;
> +
> +	dsim = (struct dsim_global *)dsim_data;
> +
> +	if (dsim == NULL) {
> +		dev_err(dsim->dev, "dsim_global pointer is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	return _s5p_dsim_get_frame_done_status(dsim);
> +}
> +
> +int s5p_dsim_clear_frame_done(void *dsim_data)
> +{
> +	struct dsim_global *dsim = NULL;
> +
> +	dsim = (struct dsim_global *)dsim_data;
> +
> +	if (dsim == NULL) {
> +		dev_err(dsim->dev, "dsim_global pointer is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	_s5p_dsim_clear_frame_done(dsim);
> +
> +	return 0;
> +}
> +
> +MODULE_AUTHOR("InKi Dae <ink...@sa...>");
> +MODULE_DESCRIPTION("Samusung MIPI-DSIM common driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/video/s5p_dsim_common.h b/drivers/video/s5p_dsim_common.h
> new file mode 100644
> index 0000000..deefca1
> --- /dev/null
> +++ b/drivers/video/s5p_dsim_common.h
> @@ -0,0 +1,38 @@
> +/* linux/drivers/video/samsung/s5p_dsim_common.h
> + *
> + * Header file for Samsung MIPI-DSI common driver.
> + *
> + * Copyright (c) 2009 Samsung Electronics
> + * InKi Dae <ink...@sa...>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#ifndef _S5P_DSIM_COMMON_H
> +#define _S5P_DSIM_COMMON_H
> +
> +extern int s5p_dsim_wr_data(void *dsim_data, unsigned int data_id,
> +	unsigned int data0, unsigned int data1);
> +extern int s5p_dsim_init_header_fifo(struct dsim_global *dsim);
> +extern int s5p_dsim_pll_on(struct dsim_global *dsim, unsigned char enable);
> +extern unsigned long s5p_dsim_change_pll(struct dsim_global *dsim,
> +	unsigned char pre_divider, unsigned short main_divider,
> +	unsigned char scaler);
> +extern int s5p_dsim_set_clock(struct dsim_global *dsim,
> +	unsigned char byte_clk_sel, unsigned char enable);
> +extern int s5p_dsim_init_dsim(struct dsim_global *dsim);
> +extern int s5p_dsim_set_display_mode(struct dsim_global *dsim,
> +	struct dsim_lcd_config *main_lcd, struct dsim_lcd_config *sub_lcd);
> +extern int s5p_dsim_init_link(struct dsim_global *dsim);
> +extern int s5p_dsim_set_hs_enable(struct dsim_global *dsim);
> +extern int s5p_dsim_set_data_transfer_mode(struct dsim_global *dsim,
> +	unsigned char data_path, unsigned char hs_enable);
> +extern int s5p_dsim_get_frame_done_status(void *dsim_data);
> +extern int s5p_dsim_clear_frame_done(void *dsim_data);
> +extern int s5p_dsim_enable_frame_done_int(struct dsim_global *dsim, int enable);
> +
> +extern struct fb_info *registered_fb[FB_MAX] __read_mostly;
> +
> +#endif /* _S5P_DSIM_COMMON_H */
> diff --git a/drivers/video/s5p_dsim_lowlevel.c b/drivers/video/s5p_dsim_lowlevel.c
> new file mode 100644
> index 0000000..6a27395
> --- /dev/null
> +++ b/drivers/video/s5p_dsim_lowlevel.c
> @@ -0,0 +1,562 @@
> +/* linux/drivers/video/samsung/s5p-dsim.c
> + *
> + * Samsung MIPI-DSIM lowlevel driver.
> + *
> + * InKi Dae, <ink...@sa...>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/mutex.h>
> +#include <linux/wait.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/mm.h>
> +#include <linux/ctype.h>
> +#include <linux/io.h>
> +
> +#include <mach/map.h>
> +
> +#include <plat/dsim.h>
> +#include <plat/mipi_ddi.h>
> +#include <plat/regs-dsim.h>
> +
> +void s5p_dsim_func_reset(struct dsim_global *dsim)
> +{
> +	unsigned int cfg = 0;
> +
> +	cfg = DSIM_FUNCRST;
> +
> +	writel(cfg, dsim->reg_base + S5P_DSIM_SWRST);
> +}

so much easier to do
	writel(DSIM_FUNCRST, dsim->reg_base + S5P_DSIM_SWRST);

much less space needed.


> +void s5p_dsim_sw_reset(struct dsim_global *dsim)
> +{
> +	unsigned int cfg = 0;
> +
> +	cfg = DSIM_SWRST;
> +
> +	writel(cfg, dsim->reg_base + S5P_DSIM_SWRST);
> +}
> +
> +void s5p_dsim_set_interrupt_mask(struct dsim_global *dsim, unsigned int mode,
> +	unsigned int mask)
> +{

bool for mask.

> +	unsigned int reg = readl(dsim->reg_base + S5P_DSIM_INTMSK);
> +
> +	if (mask)
> +		reg |= mode;
> +	else
> +		reg &= ~(mode);

no need for () around mode.

> +	writel(reg, dsim->reg_base + S5P_DSIM_INTMSK);
> +}
> +
> +void s5p_dsim_init_fifo_pointer(struct dsim_global *dsim, unsigned char cfg)
> +{
> +	unsigned int reg;
> +
> +	reg = readl(dsim->reg_base + S5P_DSIM_FIFOCTRL);
> +
> +	writel(reg & ~(cfg), dsim->reg_base + S5P_DSIM_FIFOCTRL);
> +	mdelay(10);
> +	reg |= cfg;
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_FIFOCTRL);
> +}
> +
> +/*
> + * this function set PLL P, M and S value in D-PHY
> + */
> +void s5p_dsim_set_phy_tunning(struct dsim_global *dsim, unsigned int value)
> +{
> +	writel(DSIM_AFC_CTL(value), dsim->reg_base + S5P_DSIM_PHYACCHR);
> +}
> +
> +void s5p_dsim_set_main_disp_resol(struct dsim_global *dsim,
> +	unsigned short vert_resol, unsigned short hori_resol)
> +{
> +	unsigned int reg;
> +
> +	/* standby should be set after configuration so set to not ready*/
> +	reg = (readl(dsim->reg_base + S5P_DSIM_MDRESOL)) &
> +		~(DSIM_MAIN_STAND_BY);
> +	writel(reg, dsim->reg_base + S5P_DSIM_MDRESOL);
> +
> +	reg &= ~(0x7ff << 16) & ~(0x7ff << 0);
> +	reg |= DSIM_MAIN_VRESOL(vert_resol) | DSIM_MAIN_HRESOL(hori_resol);
> +
> +	reg |= DSIM_MAIN_STAND_BY;
> +	writel(reg, dsim->reg_base + S5P_DSIM_MDRESOL);
> +}
> +
> +void s5p_dsim_set_main_disp_vporch(struct dsim_global *dsim,
> +	unsigned int cmd_allow, unsigned int vfront, unsigned int vback)
> +{
> +	unsigned int reg;
> +
> +	reg = (readl(dsim->reg_base + S5P_DSIM_MVPORCH)) &
> +		~(DSIM_CMD_ALLOW_MASK) & ~(DSIM_STABLE_VFP_MASK) &
> +		~(DSIM_MAIN_VBP_MASK);
> +
> +	reg |= ((cmd_allow & 0xf) << DSIM_CMD_ALLOW_SHIFT) |
> +		((vfront & 0x7ff) << DSIM_STABLE_VFP_SHIFT) |
> +		((vback & 0x7ff) << DSIM_MAIN_VBP_SHIFT);
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_MVPORCH);
> +}
> +
> +void s5p_dsim_set_main_disp_hporch(struct dsim_global *dsim,
> +	unsigned short front, unsigned short back)
> +{
> +	unsigned int reg;
> +
> +	reg = (readl(dsim->reg_base + S5P_DSIM_MHPORCH)) &
> +		~(DSIM_MAIN_HFP_MASK) & ~(DSIM_MAIN_HBP_MASK);
> +
> +	reg |= (front << DSIM_MAIN_HFP_SHIFT) | (back << DSIM_MAIN_HBP_SHIFT);
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_MHPORCH);
> +}
> +
> +void s5p_dsim_set_main_disp_sync_area(struct dsim_global *dsim,
> +	unsigned short vert, unsigned short hori)
> +{
> +	unsigned int reg;
> +
> +	reg = (readl(dsim->reg_base + S5P_DSIM_MSYNC)) &
> +		~(DSIM_MAIN_VSA_MASK) & ~(DSIM_MAIN_HSA_MASK);
> +
> +	reg |= ((vert & 0x3ff) << DSIM_MAIN_VSA_SHIFT) |
> +		(hori << DSIM_MAIN_HSA_SHIFT);
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_MSYNC);
> +}
> +
> +void s5p_dsim_set_sub_disp_resol(struct dsim_global *dsim,
> +	unsigned short vert, unsigned short hori)
> +{
> +	unsigned int reg;
> +
> +	reg = (readl(dsim->reg_base + S5P_DSIM_SDRESOL)) &
> +		~(DSIM_SUB_STANDY_MASK);
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_SDRESOL);
> +
> +	reg &= ~(DSIM_SUB_VRESOL_MASK) | ~(DSIM_SUB_HRESOL_MASK);
> +	reg |= ((vert & 0x7ff) << DSIM_SUB_VRESOL_SHIFT) |
> +		((hori & 0x7ff) << DSIM_SUB_HRESOL_SHIFT);
> +	writel(reg, dsim->reg_base + S5P_DSIM_SDRESOL);
> +
> +	reg |= (1 << DSIM_SUB_STANDY_SHIFT);
> +	writel(reg, dsim->reg_base + S5P_DSIM_SDRESOL);
> +}
> +
> +void s5p_dsim_init_config(struct dsim_global *dsim,
> +	struct dsim_lcd_config *main_lcd_info,
> +	struct dsim_lcd_config *sub_lcd_info, struct dsim_config *dsim_info)
> +{
> +	unsigned int cfg = (readl(dsim->reg_base + S5P_DSIM_CONFIG)) &
> +		~(1 << 28) & ~(0x1f << 20) & ~(0x3 << 5);
> +
> +	cfg =	(dsim_info->auto_flush << 29) |
> +		(dsim_info->eot_disable << 28) |
> +		(dsim_info->auto_vertical_cnt << DSIM_AUTO_MODE_SHIFT) |
> +		(dsim_info->hse << DSIM_HSE_MODE_SHIFT) |
> +		(dsim_info->hfp << DSIM_HFP_MODE_SHIFT) |
> +		(dsim_info->hbp << DSIM_HBP_MODE_SHIFT) |
> +		(dsim_info->hsa << DSIM_HSA_MODE_SHIFT) |
> +		(dsim_info->e_no_data_lane << DSIM_NUM_OF_DATALANE_SHIFT);
> +
> +	writel(cfg, dsim->reg_base + S5P_DSIM_CONFIG);
> +}
> +
> +void s5p_dsim_display_config(struct dsim_global *dsim,
> +	struct dsim_lcd_config *main_lcd, struct dsim_lcd_config *sub_lcd)
> +{
> +	u32 reg = (readl(dsim->reg_base + S5P_DSIM_CONFIG)) &
> +		~(0x3 << 26) & ~(1 << 25) & ~(0x3 << 18) & ~(0x7 << 12) &
> +		~(0x3 << 16) & ~(0x7 << 8);
> +
> +	if (main_lcd->e_interface == DSIM_VIDEO)
> +		reg |= (1 << 25);
> +	else if (main_lcd->e_interface == DSIM_COMMAND)
> +		reg &= ~(1 << 25);
> +	else {
> +		dev_err(dsim->dev, "this ddi is not MIPI interface.\n");
> +		return;
> +	}
> +
> +	/* main lcd */
> +	reg |= ((u8) (main_lcd->parameter[DSI_VIDEO_MODE_SEL]) & 0x3) << 26 |
> +		((u8) (main_lcd->parameter[DSI_VIRTUAL_CH_ID]) & 0x3) << 18 |
> +		((u8) (main_lcd->parameter[DSI_FORMAT]) & 0x7) << 12;
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_CONFIG);
> +}
> +
> +void s5p_dsim_enable_lane(struct dsim_global *dsim, unsigned char lane,
> +	unsigned char enable)
> +{
> +	unsigned int reg;
> +
> +	reg = readl(dsim->reg_base + S5P_DSIM_CONFIG);
> +
> +	if (lane == DSIM_LANE_CLOCK) {
> +		if (enable)
> +			reg |= (1 << 0);
> +		else
> +			reg &= ~(1 << 0);
> +	} else {
> +		if (enable)
> +			reg |= (lane << 1);
> +		else
> +			reg &= ~(lane << 1);
> +	}
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_CONFIG);
> +}
> +
> +
> +void s5p_dsim_set_data_lane_number(struct dsim_global *dsim,
> +	unsigned char count)
> +{
> +	unsigned int cfg = 0;
> +
> +	/* get the data lane number. */
> +	cfg = DSIM_NUM_OF_DATA_LANE(count);
> +
> +	writel(cfg, dsim->reg_base + S5P_DSIM_CONFIG);
> +}
> +
> +void s5p_dsim_enable_afc(struct dsim_global *dsim, unsigned char enable,
> +	unsigned char afc_code)
> +{
> +	unsigned int reg = readl(dsim->reg_base + S5P_DSIM_PHYACCHR);
> +
> +	if (enable) {
> +		reg |= (1 << 14);
> +		reg &= ~(0x7 << 5);
> +		reg |= (afc_code & 0x7) << 5;
> +	} else
> +		reg &= ~(1 << 14);
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_PHYACCHR);
> +}
> +
> +void s5p_dsim_enable_pll_bypass(struct dsim_global *dsim,
> +	unsigned char enable)

unsigned int enable.

> +{
> +	unsigned int reg = (readl(dsim->reg_base + S5P_DSIM_CLKCTRL)) &
> +		~(DSIM_PLL_BYPASS_EXTERNAL);
> +
> +	reg |= enable << DSIM_PLL_BYPASS_SHIFT;
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_CLKCTRL);
> +}
> +
> +void s5p_dsim_set_pll_pms(struct dsim_global *dsim, unsigned char p,
> +	unsigned short m, unsigned short s)
> +{
> +	unsigned int reg = readl(dsim->reg_base + S5P_DSIM_PLLCTRL);
> +
> +	reg |= ((p & 0x3f) << 13) | ((m & 0x1ff) << 4) | ((s & 0x7) << 1);
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_PLLCTRL);
> +}
> +
> +void s5p_dsim_pll_freq_band(struct dsim_global *dsim, unsigned char freq_band)
> +{
> +	unsigned int reg = (readl(dsim->reg_base + S5P_DSIM_PLLCTRL)) &
> +		~(0x1f << DSIM_FREQ_BAND_SHIFT);
> +
> +	reg |= ((freq_band & 0x1f) << DSIM_FREQ_BAND_SHIFT);
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_PLLCTRL);
> +}
> +
> +void s5p_dsim_pll_freq(struct dsim_global *dsim, unsigned char pre_divider,
> +	unsigned short main_divider, unsigned char scaler)
> +{
> +	unsigned int reg = (readl(dsim->reg_base + S5P_DSIM_PLLCTRL)) &
> +		~(0x7ffff << 1);
> +
> +	reg |= (pre_divider & 0x3f) << 13 | (main_divider & 0x1ff) << 4 |
> +		(scaler & 0x7) << 1;
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_PLLCTRL);
> +}
> +
> +void s5p_dsim_pll_stable_time(struct dsim_global *dsim,
> +	unsigned int lock_time)
> +{
> +	writel(lock_time, dsim->reg_base + S5P_DSIM_PLLTMR);
> +}
> +
> +void s5p_dsim_enable_pll(struct dsim_global *dsim, unsigned char enable)
> +{
> +	unsigned int reg = (readl(dsim->reg_base + S5P_DSIM_PLLCTRL)) &
> +		~(0x1 << DSIM_PLL_EN_SHIFT);
> +
> +	reg |= ((enable & 0x1) << DSIM_PLL_EN_SHIFT);
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_PLLCTRL);
> +}
> +
> +void s5p_dsim_set_byte_clock_src(struct dsim_global *dsim, unsigned char src)
> +{
> +	unsigned int reg = (readl(dsim->reg_base + S5P_DSIM_CLKCTRL)) &
> +		~(0x3 << DSIM_BYTE_CLK_SRC_SHIFT);
> +
> +	reg |= ((unsigned int) src) << DSIM_BYTE_CLK_SRC_SHIFT;
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_CLKCTRL);
> +}
> +
> +void s5p_dsim_enable_byte_clock(struct dsim_global *dsim,
> +	unsigned char enable)
> +{
> +	unsigned int reg = (readl(dsim->reg_base + S5P_DSIM_CLKCTRL)) &
> +		~(1 << DSIM_BYTE_CLKEN_SHIFT);
> +
> +	reg |= enable << DSIM_BYTE_CLKEN_SHIFT;
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_CLKCTRL);
> +}
> +
> +void s5p_dsim_set_esc_clk_prs(struct dsim_global *dsim, unsigned char enable,
> +	unsigned short prs_val)
> +{
> +	unsigned int reg = (readl(dsim->reg_base + S5P_DSIM_CLKCTRL)) &
> +		~(1 << DSIM_ESC_CLKEN_SHIFT) & ~(0xffff);
> +
> +	reg |= enable << DSIM_ESC_CLKEN_SHIFT;
> +	if (enable)
> +		reg |= prs_val;
> +
> +	writel(reg, dsim->reg_base + S5P_DSIM_CLKCTRL);
> +}
> +
> +void s5p_dsim_enable_esc_clk_on_lane(struct dsim_global *dsim,
> +	unsigned char lane_sel, unsigned char enable)
> +{
> +	unsigned int reg = readl(dsim->reg_base + S5P_DSIM_CLKCTRL);
> +
> +	if (enable) {
> +		if (lane_sel & DSIM_LANE_CLOCK)
> +			reg |= 1 << DSIM_LANE_ESC_CLKEN_SHIFT;
> +		if (lane_sel & DSIM_LANE_DATA0)
> +			reg |= 1 << (DSIM_LANE_ESC_CLKEN_SHIFT + 1);
> +		if (lane_sel & DSIM_LANE_DATA1)
> +			reg |= 1 << (DSIM_LANE_ESC_CLKEN_SHIFT + 2);
> +		if (lane_sel & DSIM_LANE_DATA2)
> +			reg |= 1 << (DSIM_LANE_ESC_CLKEN_SHIFT + 3);
> +		if (lane_sel & DSIM_LANE_DATA2)
> +			reg |= 1 << (DSIM_LANE_ESC_CLKEN_SHIFT + 4);
> +	} else {
> +		if (lane_sel & DSIM_LANE_CLOCK)
> +			reg &= ~(1 << DSIM_LANE_ESC_CLKEN_SHIFT);
> +		if (lane_sel & DSIM_LANE_DATA0)
> +			reg &= ~(1 << (DSIM_LANE_ESC_CLKEN_SHIFT + 1));
> +		if (lane_sel & DSIM_LANE_DATA1)
> +			reg &= ~(1 << (DSIM_LANE_ESC_CLKEN_SHIFT + 2));
> +		if (lane_sel & DSIM_L...
 
[truncated message content]

Re: [Linux-fbdev-devel] [PATCH 0/3] ARM: S5PV210: Add MIPI-DSI support.

[Ben D. <be...@si...>]

On 29/06/10 09:31, InKi Dae wrote:
> Hello all,
> 
> This patch is for MIPI-DSI controller based on S5PV210 (or S5PC110).
> 
> LCD Panel can use following interfaces,
> - RGB or CPU Interface.
> - RGB or CPU Interface based on MIPI-DSI.
> 
> In case of small size lcd panel, it was ok only rgb or cpu interface not
> using mipi controller.
> But big size panel(more then 480x800) needs MIPI-DSI Interface or other
> hardwares like LVDS

Hmm, I must have imagined connecting up an 1024x768 panel to an RGB
interface then.

Re: [Linux-fbdev-devel] [PATCH 1/3] FB: Add some members for CPU Interface.

[Andrew M. <ak...@li...>]

On Fri, 02 Jul 2010 01:21:12 +0000 (GMT) In-Ki Dae <ink...@sa...> wrote:

> Hi, Andrew,
> 
> cpu timing variables could be used generically.
> now fb.h and modedb.c file have been considered only for video mode.
> but the way of transfering screen data includes also command mode(cpu mode or i80 mode)
> so I thought it should be added things for it to fb_varscreeninfo struct.
> 
> Could you please tell me why cpu timing variables are specific to s3c-fb?

I dunno - I was just looking at the code.

I don't even know what "cpu timing variables" _are_, in the context of
video drivers.  Look at this:

> > CPU interface needs cs, wr setup, wr act and hold delay.
> > I added some members for them to common framework.

It's gobbledigook!  Please, write patch descriptions which help poor
dumb people who don't have detailed knowledge of the particular
hardware understand your patch?

Re: [Linux-fbdev-devel] [PATCH 1/3] FB: Add some members for CPU Interface.

[Andrew M. <ak...@li...>]

On Tue, 29 Jun 2010 19:28:14 +0900
InKi Dae <ink...@sa...> wrote:

> 
> CPU interface needs cs, wr setup, wr act and hold delay.
> I added some members for them to common framework.
> 
> Signed-off-by: InKi Dae <ink...@sa... <mailto:p.o...@sa...>>
> Signed-off-by: Kyungmin Park <kyu...@sa...
> <mailto:kyu...@sa...>>

These email addresses are mangled.

> --- a/drivers/video/modedb.c
> +++ b/drivers/video/modedb.c
> ...
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h

The patch seems pretty specific to s3c-fb.  Is it possible and sensible
to somhow avoid adding code to generic files?  Can we push more (or
all) of these changes into s3c-fb.c or into the arch/arm support code?

Re: [Linux-fbdev-devel] [PATCH] s3c-fb: enable fimd driver for s5pv210.

[Ben D. <ben...@fl...>]

On Tue, May 18, 2010 at 10:51:24PM +0900, InKi Dae wrote:
> I'm sorry for being late.
> 
> this patch is for s5pv210.
> to support s5pv210, CH_ENABLE bit of SHADOW register should be enabled to enable window layer on s5pv210.

I'd like to hold this until the chnages for multi-hardware
support and header file cleanup are done.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [Linux-fbdev-devel] [PATCH] s3c-fb: added patch series for s5pv210 and some features.

[Ben D. <ben...@fl...>]

On Mon, May 10, 2010 at 05:22:54PM -0400, Andrew Morton wrote:
> On Fri, 7 May 2010 16:59:56 +0900 InKi Dae <da...@gm...> wrote:
> 
> > this includes three patch series and divides previous patch
> > into smaller part.
> > 
> > so Andrew, please remove my previous patch,
> > s3c-fb-add-s5pv210-support-and-some-features.patch from mm tree.
> > 
> > patch contents are as following.
> > 
> > 1. s3c_fb_1_3.patch
> >   - as patch for supporting s5pv210, CH_ENABLE bit of SHAD
> >     register should be enabled to enable window layer on s5pv210.
> > 
> > 2. s3c_fb_2_3.patch
> >    - s5pv210 has five window layers (window0 ~ 4), among them,
> >      window0 ~ 2 could be used for local path with fimc(capture device)
> >      and fimd writeback feature so this patch makes default window
> >      layer for UI to be set at machine code.
> > 
> > 3. s3c_fb_3_3.patch
> >    - it has been working fine at 16bpp but in case of pixel format
> >      more then 24bpp it would occur distortedness situation
> >      on that mode. so this patch set the word swap control bit of
> >      WINCONx to 1 as default value. but it should be set to 0
> >      in case that each ENLOCAL bit of WINCON0 ~ 2 registers
> >      is enabled. this issue would be solved with local path feature soon.
> 
> I prefer not to receive patches this way, sorry.  I will need to detach
> the attachments, copy-n-paste the changelogs and signoffs and then
> invent meaningful English-language titles for each patch.

Please include me on these patches as I am working on adding S3C2443/2416
support to the driver and removing the need for arch-based header files
for this.

I may ask Andrew if we can merge the updates via my samsung tree as it is
possible that we need to touch arch code and driver code at the same time.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [Linux-fbdev-devel] [PATCH] s3c-fb: added patch series for s5pv210 and some features.

[Andrew M. <ak...@li...>]

On Fri, 7 May 2010 16:59:56 +0900 InKi Dae <da...@gm...> wrote:

> this includes three patch series and divides previous patch
> into smaller part.
> 
> so Andrew, please remove my previous patch,
> s3c-fb-add-s5pv210-support-and-some-features.patch from mm tree.
> 
> patch contents are as following.
> 
> 1. s3c_fb_1_3.patch
>   - as patch for supporting s5pv210, CH_ENABLE bit of SHAD
>     register should be enabled to enable window layer on s5pv210.
> 
> 2. s3c_fb_2_3.patch
>    - s5pv210 has five window layers (window0 ~ 4), among them,
>      window0 ~ 2 could be used for local path with fimc(capture device)
>      and fimd writeback feature so this patch makes default window
>      layer for UI to be set at machine code.
> 
> 3. s3c_fb_3_3.patch
>    - it has been working fine at 16bpp but in case of pixel format
>      more then 24bpp it would occur distortedness situation
>      on that mode. so this patch set the word swap control bit of
>      WINCONx to 1 as default value. but it should be set to 0
>      in case that each ENLOCAL bit of WINCON0 ~ 2 registers
>      is enabled. this issue would be solved with local path feature soon.

I prefer not to receive patches this way, sorry.  I will need to detach
the attachments, copy-n-paste the changelogs and signoffs and then
invent meaningful English-language titles for each patch.

The problem is the last part.  The titles I choose may be
inappropriate, and if anyone in a year's time tries to find the history
of a patch by googling for the title, they may not be able to find this
email thread.  A well-chosen title becomes a nice search key for looking
up the patch in reviewion control systems and on the 'net.

There are reasons for the conventions, so please let's be conventional.
Send it as three sequences-numbered emails, one with a carefully chosen
title/Subject: and each one with its own changelog, signoffs, etc.

Also, if you _must_ use attachments, please use text/plain mimetypes,
thanks.

Re: [Linux-fbdev-devel] [PATCH] fix Kconfig breakage in drivers/video

[Andrew M. <ak...@li...>]

On Tue, 23 Mar 2010 12:11:37 -0700
Andrew Morton <ak...@li...> wrote:

> On Mon, 15 Mar 2010 08:39:20 +0200
> Baruch Siach <ba...@tk...> wrote:
> 
> > Hi Krzysztof,
> > 
> > On Sun, Mar 14, 2010 at 10:53:29PM +0100, Krzysztof Helt wrote:
> > > From: Krzysztof Helt <krz...@wp...>
> > > 
> > > There are two new options inside the CONFIG_FB section which do not
> > > depend on the CONFIG_FB. This break Kconfig menu layout.
> > > 
> > > Fix it and reduce few conditions for sparc frame buffers
> > > as the FB_SBUS == ((FB = y) && SPARC).
> > > 
> > > Signed-off-by: Krzysztof Helt <krz...@wp...>
> > > ---
> > > 
> > > --- linux-orig/drivers/video/Kconfig	2010-03-14 23:54:39.000000000 +0100
> > > +++ linux-git/drivers/video/Kconfig	2010-03-15 00:05:59.000000000 +0100
> > > @@ -400,12 +400,9 @@ config FB_SA1100
> > >  	  If you plan to use the LCD display with your SA-1100 system, say
> > >  	  Y here.
> > >  
> > > -config HAVE_FB_IMX
> > > -	bool
> > > -
> > >  config FB_IMX
> > >  	tristate "Motorola i.MX LCD support"
> > > -	depends on FB && (HAVE_FB_IMX || ARCH_MX1 || ARCH_MX2)
> > > +	depends on FB && (ARCH_MX1 || ARCH_MX2)
> > 
> > This hunk reverts f6014419 (imxfb: add support for i.MX25). Is this 
> > intentional?
> > 
> 
> Did this question get addressed?
> 
> Thanks.

<a month passes>

Krzysztof?

Re: [Linux-fbdev-devel] [PATCH] added S6E63M0 AMOLED LCD Panel driver.

[Andrew M. <ak...@li...>]

On Tue, 30 Mar 2010 19:55:01 -0400
Andrew Morton <ak...@li...> wrote:

> On Wed, 31 Mar 2010 11:41:54 +0900 InKi Dae <da...@gm...> wrote:
> 
> > Hi Andrew,
> > 
> > all the calls to s6e63m0_panel_send_sequence() would return -EINVAL.
> > by api_async() of driver/spi/spi.c
> 
> No, spi_async() does
> 
> 	master->transfer(spi, message);
> 
> which can return at least EIO, EINPROGRESS, EINVAL or ETIMEDOUT.
> 
> > so I think that those return values aren't changed to other.
> > 
> > and final step is to check only whether the return value is 0 or not.
> > if you still think that this code has minor problem or you want it to
> > be corrected
> > then I will patch this code to be corrected anytime.
> 
> It's a bug.
> 
> Also s6e63m0_power_on() is sloppy.  It again or's together disparate
> errnos.  Then if _anything_ failed it returns hardwired -EIO, but it
> should instead propagate the callee's errno back up to the caller.
> 
> And s6e63m0_power_on() can return -EFAULT in several places, which is
> nonsensical.
> 
> None of this is very critical, just ... sloppy.
> 

ping?

Re: [Linux-fbdev-devel] [PATCH 07/16] viafb: Add 1200x900 DCON/LCD panel modes for OLPC XO-1.5

[Jonathan C. <co...@lw...>]

On Fri, 09 Apr 2010 23:27:30 +0200
Florian Tobias Schandinat <Flo...@gm...> wrote:

> What will happen if someone with no VX855 will try to enter 1200x900 
> mode? Probably the world won't be destroyed but I have a really ugly 
> feeling that the driver is not well prepared for this situation. 

OK, I'm certainly exposing my relative lack of understanding of
framebuffer issues (I'm here to add a camera driver, remember? :),
but...is this really something that older chipsets can't handle?  Or is
it just that nobody has had that size of panel before?

> Haven't 
> tested yet as I'm waiting for the official forward port but I think it 
> might be better to reschedule this patch for later or add some PLL 
> values that are supposed to work (using/executing the formulas in the 
> VIA open source X driver)

If it's really not acceptable, then it will stay out, but I would
rather get it merged.  I will leave it in the series for now; it can
come out later if need be.

Thanks,

jon

Re: [Linux-fbdev-devel] [PATCH 05/16] viafb: Determine type of 2D engine and store it in chip_info

[Jonathan C. <co...@lw...>]

[Getting back to the older stuff...]

On Fri, 09 Apr 2010 22:34:16 +0200
Florian Tobias Schandinat <Flo...@gm...> wrote:

> >> Just a minor nit:
> >> Could we change the default so that if someone adds support for a new 
> >> IGP (and misses this function) we default to either the newest or 
> >> preferably to none? I've just seen too much poorly maintained code in 
> >> this driver and defaulting to the oldest is hence a bad idea.
> >> Otherwise it's fine.  
> > 
> > That would require making an exhaustive list of older chipset types.
> > It could probably be inferred through inspection of the code, but I
> > worry about making assumptions in this area...  
> 
> Such list already exists. gfx_chip_name = pdi->driver_data in hw.c (and 
> only there) so what is needed is the list viafb_pci_table in viafbdev.c 
> (relatively at the end) of all chips:

I've spent a bit of time looking at this.  What's really needed is a
better way of abstracting the chip types so that we can maybe get rid
of all those switch statements throughout the driver.  For the purposes
of getting this work in, I'm not quite prepared to make that change,
though I could certainly consider doing it in the future.

In the absence of that, the only course of action which makes sense is
to simply fail the initialization if an unknown chip type shows up
there.  That's easy, and I can do it.  But, given that this was a
"minor nit," can we leave it as-is for now?  There's a *lot* of things
to clean up in this driver, I'd like to make it better a step at a time
rather than trying to do the whole thing at once.

Thanks,

jon

Re: [Linux-fbdev-devel] [PATCH 08/16] viafb: Do not probe for LVDS/TMDS on OLPC XO-1.5

[Jonathan C. <co...@lw...>]

On Sat, 10 Apr 2010 02:42:52 +0200
Florian Tobias Schandinat <Flo...@gm...> wrote:

> > That said, machine_is_olpc() is properly defined for all
> > configurations, so the #ifdefs can (and should) come out.  
> 
> I'm not sure I get you right here. If you talk about removing the 
> defines and only letting the machine check that is something that I 
> would accept now. 

Yes, that is what I mean; the ifdefs don't need to be there.  Had I
thought that through I would have removed them before posting the patch.

jon

Re: [Linux-fbdev-devel] [RFC] Initial OLPC Viafb merge

[Jonathan C. <co...@lw...>]

On Sat, 10 Apr 2010 01:32:36 +0200
Florian Tobias Schandinat <Flo...@gm...> wrote:

> Please correct me if I am wrong but the remaining 6 patches concerning 
> suspend&resume look like a real big FIXME. So at the end it is expected 
> to work only on VX855 and needs something called OFW?

OFW = OpenFirmware.  You could say BIOS instead.  It's pretty normal to
expect the BIOS to put things into a semi-rational state at resume time.

> It doesn't seem to make much sense to review each of them because the 
> following patches might or might not correct some of the issues of the 
> other. It is really a pain to have 6 patches trying to add a single 
> feature. Is there any way to fix this mess. (I assume you didn't merge 
> them due to authorship issues?)

It's true that one needs to look at the end product.  I only looked at
the S/R code recently, and tried to fix some of the biggest issues that
would keep it out of the mainline.

> I think it might be better to drop those for now and wait for viafb to 
> be in a better shape before adding this feature. The mode setting should 
> be in a pretty good shape just 1 or 2 kernel versions ahead so that the 
> dependency on OFW can be dropped I think.
> 
> Sorry but I really think this is not in a shape where merging it is an 
> option. I think it would be better to skip those suspend/resume patches 
> for the next merge window.

Well, if we want to keep s/r out of tree, we can do that.  It will
complicate the merge of the other stuff, since it's got hooks into the
GPIO and camera code too.  But, like everything else I've posted so
far, it's not the work that I personally set out to do.  I can push
that work on others :)

That said, the suspend/resume support in this patch set makes suspend
work on one chipset, and probably comes pretty close on the others
without breaking anything there.  I don't see the harm in merging it;
it makes the code better than it is now.  I would rather not have to
separate it out from the rest.  But I'll not fight over this one; if
there's real opposition then we can force OLPC to continue to carry it
out of tree.

jon

Re: [Linux-fbdev-devel] [PATCH 08/16] viafb: Do not probe for LVDS/TMDS on OLPC XO-1.5

[Jonathan C. <co...@lw...>]

On Fri, 09 Apr 2010 23:40:55 +0200
Florian Tobias Schandinat <Flo...@gm...> wrote:

> I don't like the idea of OLPC specific code. Isn't there any way to 
> speed this up in general?

Architecture-specific code happens.  OLPCs are wired differently; if
you go trying to do LVDS out those GPIO ports on an OLPC, you'll not
end up talking to the hardware you think you're talking to.  The best
thing to do is to avoid it altogether.

> There is not yet even an option for OLPC_XO_1_5 (in contrast to 
> CONFIG_OLPC) in mainline. Is such a thing planned?

Yes, it is.  That's part of the remaining OLPC support code which has
also been brought forward to 2.6.34 with the intention of mainlining it.

> I can't really see anything that would speak for accepting this patch 
> now in current mainline, sorry.

If you can come up with a better solution to the problem, I'm all
ears.  But without it you'll have a hard time running mainline kernels
on XO 1.5 systems.  It is all coming, but the OLPC folks are scrambling
to get everything together; I don't think we really need to make things
harder for them.

That said, machine_is_olpc() is properly defined for all
configurations, so the #ifdefs can (and should) come out.

jon

Re: [Linux-fbdev-devel] [PATCH 04/16] viafb: Retain GEMODE reserved bits

[Jonathan C. <co...@lw...>]

On Fri, 09 Apr 2010 22:23:06 +0200
Florian Tobias Schandinat <Flo...@gm...> wrote:

> > That had crossed my mind; there is quite a bit of duplicated code
> > between those two very long functions.  At the time I was focused on
> > making things work, and I didn't want to mess with code that I couldn't
> > actually test.  So further cleanup is on my list, but I would prefer to
> > defer it for a little bit.  
> 
> The code (and the spec regarding the reserved bits also) is obviously 
> identical so please don't ignore it.

In fact, I already came to this conclusion and have added a patch to
have both functions use the same code.

Thanks,

jon

Re: [Linux-fbdev-devel] [PATCH 06/16] viafb: complete support for VX800/VX855 accelerated framebuffer

[Jonathan C. <co...@lw...>]

On Fri, 09 Apr 2010 06:21:18 +0200
Florian Tobias Schandinat <Flo...@gm...> wrote:

> > +	for (i = 0; i <= highest_reg; i+= 4)
> > +		writel(0x0, engine + i);
> > +
> 
> this obsoletes
> 	/* Init 2D engine reg to reset 2D engine */
> 	writel(0x0, engine + VIA_REG_KEYCONTROL);
> as VIA_REG_KEYCONTROL is 0x02C.

Ah, good point, the separate write is superfluous.  Removed.


[...]
> > +#define VIA_REG_MONOPATBGC_M1   0x05C       /* Add FG color of Pattern. */
> > +#define VIA_REG_COLORPAT_M1     0x100       /* from 0x100 to 0x1ff */
> > +
> 
> All of these defines are unused. I admit that it is my fault. I've not 
> yet found a way I like to use them as the meaning of the same hardware 
> address changed. I think the most clean long term solution would be to 
> put the engines in separate files and the definitions in private headers 
> to at least avoid the need to decode the engine version in the name.
> However as the old headers already contain a bunch of trash feel free to 
> ignore this issue and add it for now.

Harald's initial patch included a mechanism for remapping register writes
on the fly depending on which engine was in use; these defines were used
for that purpose.  Your reworking of the 2D code obliterated that patch,
and made it mostly unnecessary.  I kept the defines around, though, because
I thought that documenting the registers can only be a good thing.

Thanks,

jon