Re: [PATCH 1/2 v3] Driver for Freescale 8610 and 5121 DIU

From
Scott Wood <hidden>
Date
2008-03-25 22:19:31
Also in
linuxppc-dev, lkml
Reply to
York Sun, 2008-03-25
York Sun wrote:
+    Example (MPC8610HPCD)
+	diu@2c000 {
display@2c000
+		device_type = "lcd";
No device_type.
+		compatible = "fsl-diu";
"fsl,diu".
quoted hunk ↗ jump to hunk
+    s) Freescale on board FPGA
+
+    This is the memory-mapped registers for on board FPGA.
+
+    Required properities:
+    - compatible : should be "fsl,fpga-pixis".
+    - reg : should contain the address and the lenght of the FPPGA register
+      set.
+
+    Example (MPC8610HPCD)
+	fpga {
board-control@e8000000 {
+config FB_FSL_DIU
+	tristate "Freescale MPC8610/MPC5121 DIU framebuffer support"
Are these the only chips that will ever implement this?
+	depends on FB && (MPC8610 || MPC5121)
depends on FB && FSL_SOC
quoted hunk ↗ jump to hunk
+		cmfbi = machine_data->fsl_diu_info[index+1]->par;
+		if ((mfbi->x_aoi_d + var->xres) >
+				machine_data->fsl_diu_info[0]->var.xres)
+			mfbi->x_aoi_d = machine_data->fsl_diu_info[0]->var.xres
+					- var->xres;
+		if (mfbi->x_aoi_d < 0)
+			mfbi->x_aoi_d = 0;
+		if ((var->xres + mfbi->x_aoi_d) >
+				machine_data->fsl_diu_info[0]->var.xres)
+			var->xres = machine_data->fsl_diu_info[0]->var.xres
+					- mfbi->x_aoi_d;
+
+		if (cmfbi->count > 0) {		/* AOI1 is open */
+			if ((mfbi->y_aoi_d + var->yres) > cmfbi->y_aoi_d)
+				mfbi->y_aoi_d = cmfbi->y_aoi_d - var->yres;
+			if (mfbi->y_aoi_d < 0)
+				mfbi->y_aoi_d = 0;
+			if ((var->yres + mfbi->y_aoi_d) > cmfbi->y_aoi_d)
+				var->yres = cmfbi->y_aoi_d - mfbi->y_aoi_d;
+		} else {				/* AOI1 is close */
+			if ((mfbi->y_aoi_d + var->yres) >
+					machine_data->fsl_diu_info[0]->var.yres)
+				mfbi->y_aoi_d =
+					machine_data->fsl_diu_info[0]->var.yres
+					- var->yres;
+			if (mfbi->y_aoi_d < 0)
+				mfbi->y_aoi_d = 0;
+			if ((var->yres + mfbi->y_aoi_d) >
+					machine_data->fsl_diu_info[0]->var.yres)
+				var->yres =
+					machine_data->fsl_diu_info[0]->var.yres
+					- mfbi->y_aoi_d;
+		}
+		break;
+	case 2:			/* AOI 1 */
+	case 4:
+		pmfbi = machine_data->fsl_diu_info[index-1]->par;
+		if ((mfbi->x_aoi_d + var->xres) >
+				machine_data->fsl_diu_info[0]->var.xres)
+			mfbi->x_aoi_d = machine_data->fsl_diu_info[0]->var.xres
+					- var->xres;
+		if (mfbi->x_aoi_d < 0)
+			mfbi->x_aoi_d = 0;
+		if ((var->xres + mfbi->x_aoi_d) >
+				machine_data->fsl_diu_info[0]->var.xres)
+			var->xres = machine_data->fsl_diu_info[0]->var.xres
+					- mfbi->x_aoi_d;
+
+		if (pmfbi->count > 0) {		/* AOI0 is open */
+			if ((mfbi->y_aoi_d + var->yres) >
+					machine_data->fsl_diu_info[0]->var.yres)
+				mfbi->y_aoi_d =
+					machine_data->fsl_diu_info[0]->var.yres
+					- var->yres;
+			if (mfbi->y_aoi_d < (pmfbi->y_aoi_d +
+				machine_data->fsl_diu_info[index-1]->var.yres))
+				mfbi->y_aoi_d = pmfbi->y_aoi_d +
+				  machine_data->fsl_diu_info[index-1]->var.yres;
+			if ((var->yres + mfbi->y_aoi_d) >
+					machine_data->fsl_diu_info[0]->var.yres)
+				var->yres =
+					machine_data->fsl_diu_info[0]->var.yres
+					- mfbi->y_aoi_d;
+		} else {				/* AOI0 is close */
+			if ((mfbi->y_aoi_d + var->yres) >
+				machine_data->fsl_diu_info[0]->var.yres)
+				mfbi->y_aoi_d =
+					machine_data->fsl_diu_info[0]->var.yres
+					- var->yres;
+			if (mfbi->y_aoi_d < 0)
+				mfbi->y_aoi_d = 0;
+			if ((var->yres + mfbi->y_aoi_d) >
+				machine_data->fsl_diu_info[0]->var.yres)
+				var->yres =
+					machine_data->fsl_diu_info[0]->var.yres
+					- mfbi->y_aoi_d;
Could you split some of this up into separate functions at lower indentation levels to make it more readable?
quoted hunk ↗ jump to hunk
+#define CNVT_TOHW(val, width) ((((val)<<(width))+0x7FFF-(val))>>16)
+			red = CNVT_TOHW(red, info->var.red.length);
+			green = CNVT_TOHW(green, info->var.green.length);
+			blue = CNVT_TOHW(blue, info->var.blue.length);
+			transp = CNVT_TOHW(transp, info->var.transp.length);
+#undef CNVT_TOHW
Define a function for this.
quoted hunk ↗ jump to hunk
+	switch (cmd) {
+	case MFB_SET_PIXFMT:
+		if (!arg)
+			return -EINVAL;
+		if (copy_from_user((void *)&pix_fmt, (void *)arg,
+				sizeof(pix_fmt)))
(void __user *)arg
No cast on &pix_fmt.
quoted hunk ↗ jump to hunk
+#if defined(CONFIG_NOT_COHERENT_CACHE)
+		else if (status & INT_VSYNC) {
+			int i;
+			unsigned int *ptr;
+			ptr  = coherence_data;
+			for (i = 0; i < 1024*8; i++)
+				*ptr++ = 0;
As I said in an internal review, this is not enough, and it's inefficient.  Read rather than write, and do so only once per cache line, but the size you access has to be at least 13/8 of the cache size for an 8-way plru cache.

-Scott
Thread (5 messages) 5 messages, 3 authors, last reply 2008-03-27
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help