Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 8/12/20 2:49 PM, Heikki Krogerus wrote:
> On Wed, Aug 12, 2020 at 10:36:32AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 8/11/20 4:38 PM, Heikki Krogerus wrote:
>>> Hi,
>>>
>>>>>> +void typec_port_register_altmodes_from_fwnode(struct typec_port *port,
>>>>>> +	const struct typec_altmode_ops *ops, void *drvdata,
>>>>>> +	struct typec_altmode **altmodes, size_t n,
>>>>>> +	struct fwnode_handle *fwnode)
>>>>>> +{
>>>>>> +	struct fwnode_handle *altmodes_node, *child;
>>>>>> +	struct typec_altmode_desc desc;
>>>>>> +	struct typec_altmode *alt;
>>>>>> +	size_t index = 0;
>>>>>> +	u32 svid, vdo;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	altmodes_node = fwnode_get_named_child_node(fwnode, "altmodes");
>>>>>> +	if (!altmodes_node)
>>>>>> +		return;
>>>>>
>>>>> Do we need that? Why not just make the sub-nodes describing the
>>>>> alternate modes direct children of the connector node instead of
>>>>> grouping them under a special sub-node?
>>>>
>>>> If you envision how this will look in e.g. DTS sources then I think
>>>> you will see that this grouping keeps the DTS source code more
>>>> readable. Grouping things together like this is somewhat normal in
>>>> devicetree files. E.g. PMIC's or other regulator providers typical
>>>> have a "regulators" node grouping all their regulators; and also the OF
>>>> graph bindings which are used in the USB-connector node start with a
>>>> "ports" parent / grouping node.
>>>>
>>>>> If the child node of the connector has device properties "svid" and
>>>>> "vdo" then it is an alt mode that the connector supports, and it can't
>>>>> be anything else, no?
>>>>
>>>> If you want to get rid of the altmodes parent/grouping node, then the
>>>> usual way to do this would be to add a compatible string to the nodes,
>>>> rather then check for the existence of some properties.
>>>
>>> I'm looking at this from ACPI PoW. We do not have compatible string in
>>> ACPI (and in case you are wondering, the _HID PRP0001 is not a
>>> reliable solution for that).
>>
>> Note my main use-case for this is the ACPI case too, remember the
>> infamous drivers/platform/x86/intel_cht_int33fe_typec.c that is my
>> main consumer for this patch. Although there the info is lacking in ACPI
>> so I need to inject it with c-code.
>>
>>> If you wish to group the altmodes under a subnode, then that's fine, but
>>> the "altmodes" node will need to be optional, just like the "ports"
>>> OF-graph node is optional. So we need to be able to support systems
>>> where the alternate mode subnodes are directly under the connector as
>>> well.
>>
>> So for the ports case, AFAIK not having a ports subnode to group them
>> is only used in the case there are no other type of subnodes.
>>
>> With the existing usb-connector devicetree-bindings we will have both
>> ports subnodes and altmode subnodes. The usb-connector devicetree-bindings
>> already specify that the port subnodes *must* be grouped together under
>> a single ports subnode (for usb-connector nodes).
>>
>> So it seems logical and much cleaner to me to also group the altmodes
>> together under an altmodes subnode. This also solves the problem of
>> having to due heuristics to tell different kinds of subnodes apart.
>>
>> Question: why do you write: "we need to be able to support systems
>> where the alternate mode subnodes are directly under the connector as
>> well" are there already systems out there (or on their way) which
>> contain ACPI table which contain a fwnode adhering to the usb-connector
>> bindings + having subnodes which set a svid + vdo ?
> 
> There are indeed platforms on their way, but I'll see if I can still
> influence what goes into the ACPI tables of those platforms.
> 
>> Because unless such systems already exist I don't see why we need to
>> be able to support them ?  New systems can use whatever scheme we
>> can come-up with and unless existing systems already have what we
>> need, except for the altmodes grouping node, then we will need some
>> translating code which generates the expected swnodes anyways and
>> then the translator can easily inject the grouping node.
>>
>> So I do not see why we would " need to be able to support systems
>> where the alternate mode subnodes are directly under the connector as
>> well" ?
>>
>> If you insist I can make the altmodes node optional and simply
>> skip any child nodes which do not have both a svid and a vdo
>> property, but having the subnode (and then logging an error on
>> missing svid or vdo props) seems cleaner to me.
> 
> I'm trying to get the way the USB Type-C connectors are described
> in ACPI (including the alternate modes) documented somewhere. I think
> I already mentioned that to you already. There is now a discussion
> with our Windows folks how to move forward with that. In any case,
> additional nodes like that "altmodes" node are really problematic in
> Windows because of way they handle the nodes, and to be honest, I
> don't see any way I could convince those guys to accept it.
> 
> But all that is really not your problem. I have now a feeling that the
> way we will end up describing the alternate modes in ACPI will not be
> compatible with DT :-(. So I guess we can just go ahead with this, and
> then add support for ACPI later?

So since you wrote "So I guess we can just go ahead with this" O was
wondering what the next steps are for getting this series (minus the
DT-binding patch) upstream ?

Regards,

Hans