Hi Rob, On Tuesday, 16 January 2018 16:35:26 EET Rob Herring wrote: > On Mon, Jan 15, 2018 at 5:46 PM, Frank Rowand wrote: > > On 01/15/18 12:29, Laurent Pinchart wrote: > >> On Monday, 15 January 2018 22:12:33 EET Frank Rowand wrote: > >>> On 01/15/18 11:22, Laurent Pinchart wrote: > >>>> On Monday, 15 January 2018 21:12:44 EET Frank Rowand wrote: > >>>>> On 01/15/18 09:09, Rob Herring wrote: > >>>>>> On Fri, Jan 12, 2018 at 5:14 PM, Laurent Pinchart wrote: > >>>>>>> The internal LVDS encoders now have their own DT bindings. Before > >>>>>>> switching the driver infrastructure to those new bindings, implement > >>>>>>> backward-compatibility through live DT patching. > >>>>>> > >>>>>> Uhh, we just got rid of TI's patching and now adding this one. I > >>>>>> guess > >>> > >>> Let me first answer the question that you ask later. You ask "Can we > >>> work on this together to find a solution that would suit us both ?" > >>> > >>> My answer to that is emphatically YES. I will definitely work with you > >>> to try to find a good solution. > >> > >> \o/ > >> > >>>>> Please no. What we just got rid of was making it difficult for me to > >>>>> make changes to the overlay infrastructure. There are issues with > >>>>> overlays that need to be fixed before overlays become really usable. > >>>>> I am about to propose the next change, which involves removing a > >>>>> chunk of code that I will not be able to remove if this patch is > >>>>> accepted (the proposal is awaiting me collecting some data about > >>>>> the impact of the change, which I expect to complete this week). > >>> > >>> I should have thought just a little bit more before I hit send. The > >>> situation is even worse than I wrote. One of the next steps (in > >>> addition to what I wrote about above) is to change the overlay apply > >>> function to accept a flattened device tree (FDT), not an expanded > >>> device tree. In this changed model, the unflattened overlay is > >>> not available to be modified before it is applied. > >> > >> That makes sense if we consider overlays to be immutable objects that we > >> apply without offering a way to modify them. I won't challenge that API > >> decision, as my use of an overlay here is a bit of a hack indeed. > >> > >>> It is important for the devicetree infrastructure to have ownership > >>> of the FDT that is used to create the unflattened tree. (Notice > >>> that in the proposed patch, rcar_du_of_get_overlay() follows the > >>> TI example of doing a kmemdup of the blob (FDT), then uses the > >>> copy for the unflatten. The kmemdup() in this case is to create > >>> a persistent copy of the FDT.) The driver having ownership of > >>> this copy, and having the ability to free it is one of the many > >>> problems with the current overlay implementation. > >> > >> Yes, that's something I've identified as well. Lots of work has been done > >> to clean up the OF core and we're clearly not done yet. I'm happy to see > >> all the improvements you're working on. > >> > >>>>> Can you please handle both the old and new bindings through driver > >>>>> code instead? > >>>> > >>>> I could, but it would be pointless. The point here is to allow cleanups > >>>> in the driver. The LVDS encoder handling code is very intrusive in its > >>>> current form and I need to get rid of it. There would be zero point in > >>>> moving to the new infrastructure, as the main point is to get rid of > >>>> the old code which prevents moving forward. As a consequence that would > >>>> block new boards from receiving proper upstream support. An easy option > >>>> is to break backward compatibility. I'm personally fine with that, but > >>>> I assume other people would complain :-) > >>>> > >>>> I can, on the other hand, work with you to see how live DT patching > >>>> could be implemented in this driver without blocking your code. When > >>>> developing this patch series I start by patching the device tree > >>>> manually without relying on overlays at all, but got blocked by the > >>>> fact that I need to allocate phandles for new nodes I create. If there > >>>> was an API to allocate an unused phandle I could avoid using the > >>>> overlay infrastructure at all. Or there could be other > >>> > >>> It seems reasonable to provide a way to autogenerate a phandle (if > >>> requested) by the devicetree code that creates a new node. Were you > >>> using a function from drivers/of/dynamic.c to create the node? > >> > >> Not to allocate the node, no. I allocated the device_node structure > >> manually with kzalloc(), and inserted it in the device tree with > >> of_attach_node(). Is that the right approach ? I haven't been able to > >> test the code as I stopped when I realized I couldn't allocate phandles. > >> > >>>> options I'm not thinking of as I don't know what the changes you're > >>>> working on are. Can we work on this together to find a solution that > >>>> would suit us both ? > >>> > >>> Again, yes, I would be glad to work with you on this. > >> > >> How would you like to proceed ? I can try the manual approach again but > >> need information about how I could cleanly implement phandle allocation. > >> I will likely then run into other issues for which I might need help. > > > > Here are my first thoughts, based on 4.15-rc7: > > > > Question to Rob and Frank: should of_attach_node() be called directly, or > > should it be called indirectly by creating a change set that adds the > > node? > > > > Frank's off the cuff answer (but would like to think more about it): since > > the driver is modifying its own devicetree data, and thus no other entity > > needs to be notified about the changes, there is no need to add the > > complexity of using a change set. > > There's exactly 2 users outside of the DT core. That's generally a > sign of an API I'd like to make private. > > > The following is how of_attach_node() could be modified to dynamically > > create a phandle on request. > > How would this work for all the phandle references that need to be fixed up? As I know which properties contain a phandle that needs to be fixed up, my plan is to update those properties manually with the value of the newly allocated phandle. What I need here is the ability to insert the following structure in the device tree: lvds@feb90000 { compatible = "renesas,r8a7796-lvds"; (*) reg = <0 0xfeb90000 0 0x14>; (*) clocks = <&cpg CPG_MOD 727>; (*) ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; lvds0_in: endpoint { remote-endpoint = <&du_out_lvds0>; (*) }; }; port@1 { reg = <1>; lvds0_out: endpoint { remote-endpoint = <&panel_in>; (*) }; }; }; }; with the node unit address and all properties marked with a (*) computed at runtime based on information extract from the device tree. Additionally I need phandles for the lvds0_in and lvds0_out nodes, and set the value of two properties in the tree with those phandles. I've used overlays as that was the only way I found to allocate phandles, but any other API will work for me as well. -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel