Re: [PATCH 1/2] video: ARM CLCD: Add DT support

Hi,

On 09/06/2013 07:23 PM, Pawel Moll wrote:

nit: Shouldn't the abbreviation be CLCDC ?

+See also Documentation/devicetree/bindings/arm/primecell.txt
+
+Required properties:
+
+- compatible: must be one of:
+			"arm,pl110", "arm,primecell"
+			"arm,pl111", "arm,primecell"
+- reg: base address and size of the control registers block
+- interrupts: either a single interrupt specifier representing the
+		combined interrupt output (CLCDINTR) or an array of
+		four interrupt specifiers for CLCDMBEINTR,
+		CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR; in the
+		latter case interrupt names must be specified
+		(see below)
+- interrupt-names: when four interrupts are specified, their names:
+			"mbe", "vcomp", "lnbu", "fuf"
+			must be specified in order respective to the
+			interrupt specifiers
+- clocks: contains phandle and clock specifier pairs for the entries
+		in the clock-names property. See
+		Documentation/devicetree/binding/clock/clock-bindings.txt
+- clocks names: should contain "clcdclk" and "apb_pclk"
+
+Optional properties:
+
+- video-ram: phandle to a node describing specialized video memory
+		(that is *not* described in the top level "memory" node)
+                that must be used as a framebuffer, eg. due to restrictions
+		of the system interconnect; the node must contain a
+		standard reg property describing the address and the size
+		of the memory area

It seems the "specialized video memory" is described by some vendor specific
DT binding ? Is it documented ? It sounds like you are unnecessarily 
repeating
the memory node details here.

Perhaps this binding/driver should use the common reserved memory bindings,
see thread http://www.spinics.net/lists/devicetree/msg02880.html

+- max-framebuffer-size: maximum size in bytes of the framebuffer the
+			system can handle, eg. in terms of available
+			memory bandwidth
+
+In the simplest case of a display panel being connected directly to the
+CLCD, it can be described in the node:
+
+- panel-dimensions: (optional) array of two numbers (width and height)
+			describing physical dimension in mm of the panel
+- panel-type: (required) must be "tft" or "stn", defines panel type
+- panel-tft-interface: (for "tft" panel type) array of 3 numbers defining
+			widths in bits of the R, G and B components
+- panel-tft-rb-swapped: (for "tft" panel type) if present means that
+			the R&  B components are swapped on the board
+- panel-stn-color: (for "stn" panel type) if present means that
+			the panel is a colour STN display, if missing
+			is a monochrome display
+- panel-stn-dual: (for "stn" panel type) if present means that there
+			are two STN displays connected
+- panel-stn-4bit: (for monochrome "stn" panel) if present means
+			that the monochrome display is connected
+			via 4 bit-wide interface

Are this vendor specific or common properties ? Shouldn't those be prefixed
with the vendor name ?

Anyway I think we need an RFC to possibly standardize properties that are
common across multiple panels and have them listed in a common DT binding.

It sounds a bit disappointing that for same class devices there are being
introduced custom DT properties for each available device. For instance,
the first 3 properties above look like they could apply to various display
panels and controllers.

Couldn't these be 'unsigned int' ?

+	err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3);
+	if (err)
+		return err;
+
+	r = rgb[0];
+	g = rgb[1];
+	b = rgb[2];
+
+	/* Bypass pixel clock divider, data output on the falling edge */
+	panel->tim2 = TIM2_BCD | TIM2_IPC;
+
+	/* TFT display, vert. comp. interrupt at the start of the back porch */
+	panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
+
+	if (r>= 4&&  g>= 4&&  b>= 4)
+		panel->caps |= CLCD_CAP_444;
+	if (r>= 5&&  g>= 5&&  b>= 5)
+		panel->caps |= CLCD_CAP_5551;
+	if (r>= 5&&  g>= 6&&  b>= 5)
+		panel->caps |= CLCD_CAP_565;
+	if (r>= 8&&  g>= 8&&  b>= 8)
+		panel->caps |= CLCD_CAP_888;
+
+	if (of_get_property(node, "panel-tft-rb-swapped", NULL))
+		panel->caps&= ~CLCD_CAP_RGB;
+	else
+		panel->caps&= ~CLCD_CAP_BGR;
+
+	return 0;
+}
+
+static int clcdfb_of_init_display(struct clcd_fb *fb)
+{
+	struct device_node *node = fb->dev->dev.of_node;
+	u32 max_size;
+	u32 dimensions[2];
+	char *mode_name;
+	int len, err;
+	const char *panel_type;
+
+	fb->panel = devm_kzalloc(&fb->dev->dev, sizeof(*fb->panel), GFP_KERNEL);
+	if (!fb->panel)
+		return -ENOMEM;
+
+	err = of_get_fb_videomode(fb->dev->dev.of_node,&fb->panel->mode,

'node' could be reused here.

+			OF_USE_NATIVE_MODE);
+	if (err)
+		return err;
+
+	len = snprintf(NULL, 0, "%ux%u@%u", fb->panel->mode.xres,
+			fb->panel->mode.yres, fb->panel->mode.refresh);
+	mode_name = devm_kzalloc(&fb->dev->dev, len + 1, GFP_KERNEL);
+	snprintf(mode_name, len + 1, "%ux%u@%u", fb->panel->mode.xres,
+			fb->panel->mode.yres, fb->panel->mode.refresh);

Don't you want to just use kasprintf() here instead and free mode_name
manually in the remove() callback ?

+	fb->panel->mode.name = mode_name;
+
+	err = of_property_read_u32(node, "max-framebuffer-size",&max_size);
+	if (!err)
+		fb->panel->bpp = max_size / (fb->panel->mode.xres *
+				fb->panel->mode.yres) * 8;
+	else
+		fb->panel->bpp = 32;
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+	fb->panel->cntl |= CNTL_BEBO;
+#endif
+
+	if (of_property_read_u32_array(node, "panel-dimensions",
+			dimensions, 2) = 0) {
+		fb->panel->width = dimensions[0];
+		fb->panel->height = dimensions[1];
+	} else {
+		fb->panel->width = -1;
+		fb->panel->height = -1;
+	}
+
+	panel_type = of_get_property(node, "panel-type",&len);
+	if (strncmp(panel_type, "tft", len) = 0)
+		return clcdfb_of_get_tft_panel(node, fb->panel);
+	else
+		return -EINVAL;
+}
+
+static int clcdfb_of_vram_setup(struct clcd_fb *fb)
+{
+	const __be32 *prop = of_get_property(fb->dev->dev.of_node, "video-ram",
+			NULL);
+	struct device_node *node = of_find_node_by_phandle(be32_to_cpup(prop));

This looks like open coding function of_parse_phandle(), why not just:

	struct device_node *node = of_parse_phandle(fb->dev->dev.of_node,
					"video-ram", 0);
?

+	u64 size;
+	int err;
+
+	if (!node)
+		return -ENODEV;
+
+	err = clcdfb_of_init_display(fb);
+	if (err)
+		return err;
+
+	fb->fb.screen_base = of_iomap(node, 0);
+	if (!fb->fb.screen_base)
+		return -ENOMEM;
+
+	fb->fb.fix.smem_start = of_translate_address(node,
+			of_get_address(node, 0,&size, NULL));
+	fb->fb.fix.smem_len = size;
+
+	return 0;
+}

--
Thanks,
Sylwester