Message-ID: <20181017133612.GK24771@kuha.fi.intel.com>
Date:   Wed, 17 Oct 2018 16:36:12 +0300
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org
Subject: Re: [RFC PATCH 0/5] device property: Introducing software nodes

On Tue, Oct 16, 2018 at 10:32:44AM -0700, Dmitry Torokhov wrote:
> Hi Heikki,
> 
> On Fri, Oct 12, 2018 at 02:39:29PM +0300, Heikki Krogerus wrote:
> > Hi guys,
> > 
> > To continue the discussion started by Dmitry [1], this is my proposal
> > that I mentioned in my last mail. In short, the idea is that instead
> > of trying to extend the support for the currently used struct
> > property_set, I'm proposing that we introduce a completely new,
> > independent type of fwnode, and replace the struct property_set with
> > it. I'm calling the type "software node" here.
> > 
> > The reason for a complete separation of the software nodes from the
> > generic property handling code is the need to be able to create the
> > nodes independently from the devices that they are bind to.
> 
> It would be great it you would provide an example of creating these
> sowftware property sets separately from devices. How do you tie device
> and its properties if they are not created together?

The properties are bind to the software nodes and not the devices. The
software nodes can then be bind to the devices when they are created.
This is actually exactly the same behaviour that we had with the
property_sets, the only difference being that we can bind the software
node to the device at a later stage.

> For OF we have compatibles and phandles for references, ACPI has
> HIDs and CIDs and notion of references as well. What do we use here,
> especially when software node is created in one subsystem (let's say
> drivers/platform/x86), but device is created somewhere else?

Reference usually means a handle to a node that is outside of the
direct child-parent relationship (hierarchy) for the caller device
node. That I do not support at this point. We support the node
hierarchy which allows us to "refer" the child and parent nodes, and
that is all that we need at this stage.

Support for references is in my plans. I will need that later. But
we'll do that as the next step.

> Another issue that is not clear to me: looking at the USB connector it
> seems you want to have references to fwnodes.

If by references you mean here access to the nodes outside of the
hierarchy, then you've misunderstood. I do not expect that with the
USB connectors.

> How do you resolve them when there are nodes of different class.
> I.e. how do you express software fwnode referencing ACPI or DT node
> when you are supplementing ACPI or DT description of a system with
> these custom/secondary nodes?

So references are not supported as I said, but I don't know if we can
or even want to support references to different types of fwnodes. I'm
not even sure we would ever need to refer an other type of fwnode from
software node. I mean, we should always be able to place a secondary
software node to both fwnodes.

In any case, this topic is outside the scope of this series.

And in case this was not clear, with the hierarchy, different types of
fwnode nodes are not supported. It means that software node can only
have software node parent and children.

> What about the other direction? I.e. can I have a DT system with USB
> connector and augment USB set up with static nodes? Not only
> basic/scalar properties, but links as well?
> 
> As I said, having and example of using this new code to achieve your
> goal with regard to USB connector would be awesome and clear a lot of my
> questions.

OK. I used the attached code to test these on Intel Cherry Trail
board. I'm creating two software nodes there: one for the FUSB302
controller, and one (a child of the FUSB302 node) for the connector.

The fusb302 node is assigned to the i2c client that is registered in
that driver, but the child node is left waiting. The child node has a
property called "name" with value "connector".

The fusb302 driver already requests a handle to the child node named
"connector" in its probe function which it assigns to the port device
that it registers. With the attached patch it will get the child node
also on CherryTrail boards, no changes to the Type-C drivers needed.

Here is the file listing that we see in sysfs (node3 is for FUSB302):

        % find /sys/kernel/software_nodes/ | grep -v properties
        ...
        /sys/kernel/software_nodes/node3
        /sys/kernel/software_nodes/node3/node0
        /sys/kernel/software_nodes/node3/node0/port0
        /sys/kernel/software_nodes/node3/i2c-fusb302
        ...

The node for the FUSB302 controller:

        % ls -l /sys/kernel/software_nodes/node3/
        total 0
        lrwxrwxrwx    1 root     root             0 Oct 17 12:12 i2c-fusb302 -> ../../../devices/pci0000:00/808622C1:00/i2c-0/i2c-fusb302
        drwxr-xr-x    3 root     root             0 Oct 17 12:03 node0
        drwxr-xr-x    2 root     root             0 Oct 17 12:12 properties

The node for the connector (child of node3):

        % ls -l /sys/kernel/software_nodes/node3/node0/
        total 0
        lrwxrwxrwx    1 root     root             0 Oct 17 12:12 port0 -> ../../../../devices/pci0000:00/808622C1:00/i2c-0/i2c-fusb302/typec/port0
        drwxr-xr-x    2 root     root             0 Oct 17 12:12 properties

And this is what the actual connector device directory looks like
(look at the "software_node" symlink:

        % ls -l /sys/class/typec/port0/
        total 0
        -rw-r--r--    1 root     root          4096 Oct 17 12:16 data_role
        lrwxrwxrwx    1 root     root             0 Oct 17 12:16 device -> ../../../i2c-fusb302
        -rw-r--r--    1 root     root          4096 Oct 17 12:16 port_type
        drwxr-xr-x    2 root     root             0 Oct 17 12:16 power
        -r--r--r--    1 root     root          4096 Oct 17 12:16 power_operation_mode
        -rw-r--r--    1 root     root          4096 Oct 17 12:16 power_role
        -rw-r--r--    1 root     root          4096 Oct 17 12:16 preferred_role
        lrwxrwxrwx    1 root     root             0 Oct 17 12:16 software_node -> ../../../../../../../kernel/software_nodes/node3/node0
        lrwxrwxrwx    1 root     root             0 Oct 17 12:16 subsystem -> ../../../../../../../class/typec
        -r--r--r--    1 root     root          4096 Oct 17 12:16 supported_accessory_modes
        -rw-r--r--    1 root     root          4096 Oct 17 12:03 uevent
        -r--r--r--    1 root     root          4096 Oct 17 12:16 usb_power_delivery_revision
        -r--r--r--    1 root     root          4096 Oct 17 12:16 usb_typec_revision
        -rw-r--r--    1 root     root          4096 Oct 17 12:16 vconn_source


thanks,

-- 
heikki

View attachment "intel_cht_int33fe.diff" of type "text/plain" (2613 bytes)

Powered by blists - more mailing lists