Re: [PATCH 2/2] dt-bindings: simplefb-sunxi: Add sunxi simplefb extensions

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/17/2014 04:55 PM, Maxime Ripard wrote:
> On Mon, Nov 17, 2014 at 01:39:18PM +0000, Grant Likely wrote:
>> On Mon, Nov 17, 2014 at 12:47 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>>> On Mon, Nov 17, 2014 at 12:34:46PM +0100, Hans de Goede wrote:
>>>> If pre-filled framebuffer nodes are used, the firmware may need extra
>>>> properties to find the right node. This documents the properties to use
>>>> for this on sunxi platforms.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  .../bindings/video/simple-framebuffer-sunxi.txt    | 33 ++++++++++++++++++++++
>>>>  1 file changed, 33 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt b/Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt
>>>> new file mode 100644
>>>> index 0000000..84ca264
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt
>>>> @@ -0,0 +1,33 @@
>>>> +Sunxi specific Simple Framebuffer bindings
>>>> +
>>>> +This binding documents sunxi specific extensions to the simple-framebuffer
>>>> +bindings. The sunxi simplefb u-boot code relies on the devicetree containing
>>>> +pre-populated simplefb nodes.
>>>> +
>>>> +These extensions are intended so that u-boot can select the right node based
>>>> +on which pipeline and output is being used. As such they are solely intended
>>>> +for firmware / bootloader use, and the OS should ignore them.
>>>> +
>>>> +Required properties:
>>>> +- compatible: "sunxi,framebuffer"
>>>> +- sunxi,pipeline:
>>>> +  <0> for the de_be0 -> lcdc0 -> output pipeline
>>>> +  <1> for the de_be1 -> lcdc1 -> output pipeline
>>>> +- sunxi,output: One of: "hdmi", "lcd", "vga", and "composite"
>>>> +
>>>> +Example:
>>>> +
>>>> +chosen {
>>>> +     #address-cells = <1>;
>>>> +     #size-cells = <1>;
>>>> +     ranges;
>>>> +
>>>> +     framebuffer@0 {
>>>> +             compatible = "sunxi,framebuffer", "simple-framebuffer";
>>>> +             sunxi,pipeline = <0>;
>>>> +             sunxi,output = "hdmi";
>>>> +             clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
>>>> +                      <&ahb_gates 44>;
>>>
>>> If we're going that way, then maybe having to specify clock-names
>>> would be better in order to know which clock is what?
>>
>> I wouldn't go that way with this binding since the driver has no need
>> to differentiate between the clocks, and driver authors shouldn't be
>> encouraged to do so. The purpose of the clocks in this node is only
>> for itemizing dependencies, not for how to configure the clocks.
>> Firmware shouldn't care at all about the clocks list, it only needs to
>> find the correct pre-populated node to fill in and enable.
> 
> Well, if we want to play the "DT as an ABI" stuff, you have no
> guarantee that in the future, simplefb will still be the driver bound
> to "sunxi,framebuffer"

Huh? We do have that guarantee, or at least that a simplefb bindings
compatible driver is, the "sunxi,framebuffer" bindings live in:

Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt

With the filename already being a big hint, further more it states that:

"This binding documents sunxi specific extensions to the simple-framebuffer
bindings. The sunxi simplefb u-boot code relies on the devicetree containing
pre-populated simplefb nodes.

These extensions are intended so that u-boot can select the right node based
on which pipeline is being used. As such they are solely intended for
firmware / bootloader use, and the OS should ignore them."

Which to me seems to guarantee that we're talking about a simple-framebuffer
compatible node here. The scope of these extensions are very narrow, there
only purpose is to allow the bootloader to find the right pre-populated simplefb
node, everything else is specified by the simplefb bindings, not these
extensions.

Regards,

Hans