Re: [PATCH 0/5] Add of_ functions for device_link_add()

[Benjamin GAIGNARD <benjamin.gaignard@st.com>]

On 4/26/19 1:02 AM, Rob Herring wrote:
> On Thu, Apr 25, 2019 at 2:24 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Thu, Apr 25, 2019 at 11:08 AM Rob Herring <robh+dt@kernel.org> wrote:
>>> On Wed, Apr 24, 2019 at 5:19 AM Benjamin Gaignard
>>> <benjamin.gaignard@st.com> wrote:
>>>> It could happen that we need to control suspend/resume ordering between
>>>> devices without obvious consumer/supplier link. For example when touchscreens
>>>> and DSI panels share the same reset line, in this case we need to be sure
>>>> of pm_runtime operations ordering between those two devices to correctly
>>>> perform reset.
>>>> DSI panel and touchscreen aren't sharing any heriachical relationship (unlike
>>>> I2C client and I2C bus or regulator client and regulator provider) so we need
>>>> to describe this in device-tree.
>>> Needing to know which touchscreen is attached to a panel could be
>>> important to describe if you have multiple displays.
>>>
>>> Doesn't the reset subsystem already have some support for shared
>>> resets? Seems like it could provide clients with struct device or
>>> device_node ptrs to other devices sharing a reset.
Sharing reset will not help here because we don't want to have two reset
occuring in the same line but only one when the both devices are resumed.
That why we to force suspend/resume ordering and so add a link between the
devices.
>>>
>>>> This series introduce of_device_links_{add,remove} and devm_of_device_links_add()
>>>> helpers to find and parse 'links-add' property in a device-tree node.
>>> Going to document that property somewhere? :)
I have put a description of this property in each device bindings but if 
you can tell me
where to put it, I will follow your advice.
>>>
>>> I think this is too generic and coupled to Linux. It doesn't have any
>>> information as to what is the dependency or connection nor what the
>>> direction of the dependency is.
Could something like 'link-suppliers' or 'pm-runtime-dependencies'
are more explicit property name to describe direction, goal, and connection
between consumer and supplier ?
>>>
>>> I'm not convinced we need to solve this generically vs. defining
>>> something for this specific example.
>> I am pretty sure there will be more drivers needing complex
>> dependencies. Doesn't ACPI allow defining relationship between devices
>> that goes beyond the tree structure?
> Can't speak to ACPI, but I assume you where implying ACPI supports
> this so DT should too.
>
> Almost every binding we have is defining relationships between
> devices. Interrupts, clocks, gpio, pinctrl, etc. all do this. To use a
> similar example to the one here, we already define the relationship
> between a display and a backlight with a 'backlight' property in the
> display node. Why should touchscreen be any different than backlight?
It is different because it is only about suspend/resume ordering.
There is no need for a panel to knows about touchscreen unlike
than backlight that it could drive.
I have the same need to order suspend/resume operations between
GPU and display to be sure that GPU is suspend before display and resume
after it.

>
> What really concerns me here is folks just add relationships to
> 'links-add' which are already captured in DT (such as backlight) just
> because they'll get it for free and not have to go add support to
> handle each specific binding.
It won't be for free since I don't want to put it in device core so each 
driver
will have to add a call to a function to enable it and will have to 
explain why doing it.

Benjamin

>
> Rob