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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Wed, Aug 26, 2020 at 02:37:28PM +0200, Hans de Goede wrote:
> 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 ?

Sorry Hans. I forgot about this topic. I do have one question. I'll
ask it separately against the patch.

thanks,

-- 
heikki