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
