[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Ian Campbell <ijc@hellion.org.uk>]

On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> From: Luc Verhaegen <libv@skynet.be>
> 
> Add simplefb support, note this depends on the kernel having support for
> the clocks property which has recently been added to the simplefb devicetree
> binding.

Link please, Linus's tree[0] doesn't seem to have it yet, I suppose it
is in some maintainer tree at the moment? It's not even in linux-next
yet though[1]. Maybe simple-framebuffer.txt isn't the right place to
look?

[0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
[1] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt

> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
> +void
> +sunxi_simplefb_setup(void *blob)
> +{
> +	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
> +	const char *node = "/chosen/framebuffer0-hdmi";
> +	const char *format = "x8r8g8b8";

The bindings doc currently says:
        
        - format: The format of the framebuffer surface. Valid values are:
          - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
          - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
        
Perhaps x8r8g8b8 is defined in the updated version?

> +	const char *okay = "okay";
> +	char name[32];
> +	fdt32_t cells[2];
> +	int offset, stride, ret;
> +
> +	if (!sunxi_display.enabled)
> +		return;
> +
> +	offset = fdt_path_offset(blob, node);

I think you should use fdt_node_offset_by_compatible here instead of
hardcoding a path. common/lcd.c does it this way too, it also doesn't
appear to use a node under /chosen. Perhaps this is changed in the more
uptodate binding which I've not seen yet.

> +	cells[0] = cpu_to_fdt32(gd->fb_base);
> +	cells[1] = cpu_to_fdt32(CONFIG_SUNXI_FB_SIZE);
> +	ret = fdt_setprop(blob, offset, "reg", cells, sizeof(cells[0]) * 2);

What arranges that #address-cells and #size-cells are both 1 at this
point? Does u-boot not have a helper to setup a cells array including
looking those up?

Also the bindings doc seems to imply that size should be the actual
configured size of the video ram region ("(1600 * 1200 * 2)") rather
than the size of the reserved memory region. Maybe it's not important.

> +	ret = fdt_setprop(blob, offset, "format", format, strlen(format) + 1);
> +	if (ret < 0)
> +		goto error;
> +
> +	ret = fdt_setprop(blob, offset, "status", okay, strlen(okay) + 1);

You can use fdt_setprop_string for these two, I think.

Ian.