On Fri, Feb 04, 2022 at 02:59:26PM +0100, Greg Kroah-Hartman wrote:
> 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?
Unfortunately the object position is only defined for these
capability messages, not for the other messages. It's not going to
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.
But the idea with that kset would be that you have a separate
directory for each port there for this stuff:
/sys/kernel/usb_power_delivery/port0
|-- revision
...
And those directories we could then link to the actual device:
/sys/class/typec/port0/usb_power_delivery -> ../../../kernel/usb_power_delivery/port0
thanks,
--
heikki
