From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Subject: Re: [PATCH 2/2] dt-bindings: simplefb-sunxi: Add sunxi simplefb extensions Date: Fri, 21 Nov 2014 15:36:04 +0100 Message-ID: <20141121143604.GF4752@lukather> References: <1416224086-5698-1-git-send-email-hdegoede@redhat.com> <1416224086-5698-3-git-send-email-hdegoede@redhat.com> <20141117124721.GQ6414@lukather> <20141117155514.GU6414@lukather> Reply-To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="xkXJwpr35CY/Lc3I" Return-path: Content-Disposition: inline In-Reply-To: List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Grant Likely Cc: Hans de Goede , Tomi Valkeinen , Ian Campbell , Rob Herring , David Herrmann , Geert Uytterhoeven , "linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , devicetree , linux-sunxi List-Id: devicetree@vger.kernel.org --xkXJwpr35CY/Lc3I Content-Type: text/plain; charset=UTF-8 Content-Disposition: inline On Wed, Nov 19, 2014 at 02:05:48PM +0000, Grant Likely wrote: > On Mon, Nov 17, 2014 at 3: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 > >> 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 > >> >> --- > >> >> .../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", and that this driver doesn't need to > > differentiate which clocks it needs to protect and which are not > > needed. Especially with the combination of the output that you now > > hardcode. > > As long as the node claims compatibility with "simple-framebuffer", > then it must abide by the simple framebuffer rules. It cannot depend > on the driver having special knowledge about what to do with the > clocks. It is fine for a driver that understands "sunxi,framebuffer" > to enable additional features over and above simple-framebuffer, > providing it doesn't break the vanilla "simple-framebuffer" interface. > > The simple-framebuffer binding is clear that it will treat the clocks > as dependencies only. Isn't the compatible list precisely made to be able to define more specific behaviours if needed? If I get a list with "sunxi,framebuffer", "simple-framebuffer", I've always thought that sunxi,framebuffer was defining exactly what the device was about, and that simple-framebuffer was only aimed at providing roughly the same functionnality, possibly with degraded features, and with a compatible DT bindings. I don't see how if we require a clock-names list in sunxi,framebuffer that would break the binding of simple-framebuffer. It's just more information, that might prove useful in the future. > > Of course, if now we don't care at all about this ABI thing, it's > > completely fine for me. But I'll archive this email preciously. > > I do not in any way mean to say it is okay to break ABI. > > Also, my comments above are advice that I would give to U-Boot > authors, not something that should go in the binding. The > simple-framebuffer binding doesn't need to know about the clock names > at all, and if the assumption is that firmware will merely enable a > pre-populated framebuffer node in the tree, then it probably doesn't > need to parse the clocks property either. However, if it actually is > better for the U-Boot implementation to have clocks with names in the > node, then I'm not going to object. I'm not speaking about u-boot here, but the kernel itself. What if we want from that pipeline property to enable (or rather keep enabled) only a few clocks, and not all of them? Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --xkXJwpr35CY/Lc3I--