[U-Boot] [PATCH 1/2] lcd: add functions to setup up simplefb device tree

[Stephen Warren <swarren@wwwdotorg.org>]

On 05/08/2013 09:33 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Tue, May 7, 2013 at 10:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> simple-framebuffer is a new device tree binding that describes a pre-
>> configured frame-buffer memory region and its format. The Linux kernel
>> contains a driver that supports this binding. Implement functions to
>> create a DT node (or fill in an existing node) with parameters that
>> describe the framebuffer format that U-Boot is using.
>>
>> This will be immediately used by the Raspberry Pi board in U-Boot, and
>> likely will be used by the Samsung ARM ChromeBook support soon too. It
>> could well be used by many other boards (e.g. Tegra boards with built-in
>> LCD panels, which aren't yet supported by the Linux kernel).

> This looks OK - is a better place for it than the common lcd code? I
> presume it is here because it accesses panel_info?

I believe it really is common code; the DT content this code generates
is defined by a generic binding and isn't SoC-/board-specific. Perhaps a
common DT-related file would be OK too, although as you mention I'm not
sure if any other file would have access to the required LCD variables.

> Also is there a binding file we can bring in?

Yes, there's a simple-framebuffer.txt I could copy from the kernel.

>> +int lcd_dt_simplefb_add_node(void *blob)
> 
> Can we not automatically find the node and update it?

Some DTs might have the node already exist and want to edit it, whereas
others might not contain it at all and need it added. This is rather up
to the author of individual boards' DTs. I wanted to make the code
explicitly select between those two options so that the U-Boot board
author always thought about exactly which behaviour they wanted.

>> +int lcd_dt_simplefb_enable_existing_node(void *blob)
>> +{
>> +       int off;
>> +
>> +       off = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer");
> 
> Do we ever need to support more than one node, iwc perhaps we want to
> pass in the offset? If not, then see above question re searching.

I don't think U-Boot supports multiple panels does it? If the DT
contained multiple nodes to start with, I think they'd have to all be
initially disabled since some firmware would be required to fill in the
fields before they were useful.

As such, finding and editing the first node in all cases seems to make
sense for now. If this ever becomes false, we can add an index parameter
quite easily without significant impact.

>> diff --git a/include/lcd.h b/include/lcd.h

>> +#if defined(CONFIG_LCD_DT_SIMPLEFB)
> 
> Probably don't need this #ifdef? It will complicate things if we use
> the autoconf series.

What's the autoconf series? I did this in order to get compile errors
rather than link errors if the functions were used without being
enabled, but I guess most linkers generate useful error messages so
perhaps this isn't needed.