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