Re: [PATCH v1 1/3] usb: typec: Separate USB Power Delivery from USB Type-C

[Greg Kroah-Hartman]

On Fri, Feb 04, 2022 at 12:04:41PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Thu, Feb 03, 2022 at 03:55:19PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Feb 03, 2022 at 05:46:55PM +0300, Heikki Krogerus wrote:
> > > +/* These additional details are only available with vSafe5V supplies */
> > > +static struct kobj_attribute dual_role_power_attr = __ATTR_RO(dual_role_power);
> > > +static struct kobj_attribute usb_suspend_supported_attr = __ATTR_RO(usb_suspend_supported);
> > > +static struct kobj_attribute unconstrained_power_attr = __ATTR_RO(unconstrained_power);
> > > +static struct kobj_attribute usb_communication_capable_attr = __ATTR_RO(usb_communication_capable);
> > > +static struct kobj_attribute dual_role_data_attr = __ATTR_RO(dual_role_data);
> > > +static struct kobj_attribute
> > > +unchunked_extended_messages_supported_attr = __ATTR_RO(unchunked_extended_messages_supported);
> > 
> > Note, no 'struct device' should ever have a "raw" kobject hanging off of
> > it.  If so, something went wrong.
> > 
> > If you do this, userspace will never be notified of the attributes and
> > any userspace representation of the tree will be messed up.
> > 
> > Please, use an attribute directory with a name, or if you really need to
> > go another level deep, use a real 'struct device'.  As-is here, I can't
> > take it.
> 
> OK, got it. I don't think we can avoid the deeper levels, not without
> making this really cryptic, and not really usable in all cases. These
> objects are trying to represent (parts) of the protocol - the
> messages, the objects in those messages, and later the responses to
> those messages.
> 
> But I'm also trying to avoid having to claim that these objects are
> "devices", because honestly, claiming that the packages used in
> communication are devices is confusing, and just wrong. If we take
> that road, then we really should redefine what struct device is
> supposed to represent, and rename it also.

Fair enough, this isn't really a device, it's an "attribute" of your
device you are wanting to show.  It's just that you are really "deep".

You asked for:

/sys/class/typec/port0/usb_power_delivery
|-- revision
|-- sink_capabilities/
|   |-- 1:fixed_supply/
|   |   |-- dual_role_data
|   |   |-- dual_role_power
|   |   |-- fast_role_swap_current
|   |   |-- operational_current
|   |   |-- unchunked_extended_messages_supported
|   |   |-- unconstrained_power
|   |   |-- usb_communication_capable
|   |   |-- usb_suspend_supported
|   |   `-- voltage
|   |-- 2:variable_supply/
|   |   |-- maximum_voltage
|   |   |-- minimum_voltage
|   |   `-- operational_current
|   `-- 3:battery/
|       |-- maximum_voltage
|       |-- minimum_voltage
|       `-- operational_power
`-- source_capabilities/
    `-- 1:fixed_supply/
        |-- dual_role_data
        |-- dual_role_power
        |-- maximum_current
        |-- unchunked_extended_messages_supported
        |-- unconstrained_power
        |-- usb_communication_capable
        |-- usb_suspend_supported
        `-- voltage


To start with, your "attribute" is really "usb_power_delivery" here, so
you can just use an attribute group name to get the "revision" file.

But then the later ones could be flat in that directory as well, using a
':' to split as you did already, and the above could turn into:

/sys/class/typec/port0/usb_power_delivery
|-- revision
|-- sink_capabilites:1:fixed_supply:dual_role_data
|-- sink_capabilites:1:fixed_supply:dual_role_power
|-- sink_capabilites:1:fixed_supply:fase_role_swap_current
....
|-- sink_capabilites:2:variable_supply:maximum_voltage
|-- sink_capabilites:2:variable_supply:minimum_voltage
...
|-- source_capabilities:1:fixed_supply:dual_role_data
|-- source_capabilities:1:fixed_supply:dual_role_power
|-- source_capabilities:1:fixed_supply:maximum_current
...

But ick, that's also a mess as you are now forced to parse filenames in
userspace in a different way than "normal".

Is there anything special about the number here?  It's the "position"
which will be unique.  So make that position a device, as that's kind of
what it is (like usb endpoints are devices)

Then you could make a bus for the positions and all would be good, and
you could turn this into:


/sys/class/typec/port0/usb_power_delivery
|-- revision
|-- sink_capabilities:1/
|   `-- fixed_supply/
|       |-- dual_role_data
|       |-- dual_role_power
|       |-- fast_role_swap_current
|       |-- operational_current
|       |-- unchunked_extended_messages_supported
|       |-- unconstrained_power
|       |-- usb_communication_capable
|       |-- usb_suspend_supported
|       `-- voltage
|-- sink_capabilities:2/
|   `-- variable_supply/
|       |-- maximum_voltage
|       |-- minimum_voltage
|       `-- operational_current
|-- sink_capabilities:3/
|   `-- battery/
|       |-- maximum_voltage
|       |-- minimum_voltage
|       `-- operational_power
`-- source_capabilities:1/
    `-- fixed_supply/
        |-- dual_role_data
        |-- dual_role_power
        |-- maximum_current
        |-- unchunked_extended_messages_supported
        |-- unconstrained_power
        |-- usb_communication_capable
        |-- usb_suspend_supported
        `-- voltage

Would that work?

> So would it be OK that, instead of registering these objects as
> devices, we just introduce a kset where we can group them
> (/sys/kernel/usb_power_delivery)?

You want to show this as attched to a specific port somehow, so that
location is not going to work.

thanks,

greg k-h