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

[Simon Glass]

Hi Stephen,

On Fri, May 10, 2013 at 9:35 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 05/08/2013 09:33 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Tue, May 7, 2013 at 10:21 PM, Stephen Warren <swarren at 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.

Yes, it looks good.

>
>>> +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.

OK.

>
>>> +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.

Sounds reasonable. I guess U-Boot will support multiple panels once
driver model is fully installed, but for now it does not.

>
>>> 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.
>

It was posted 26 Feb - first patch is here:
http://patchwork.ozlabs.org/patch/223278/

Also while you are in review mode, I'd appreciate any comment you have
on U-Boot driver model:

http://patchwork.ozlabs.org/patch/242451/

Regards,
Simon