PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes

PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes

[Hans de Goede]

Hi All,

This is a replacement series for an earlier attempt by me for this
from quite a while ago:

https://patchwork.kernel.org/patch/11199517/

As discussed there, this series implements an altmodes devicetree-fwnode
under the usb-connector node which has 1 child-node per supported
altmode and in that child-node the svid and vdo for the supported
altmode are specified.

Note this patch-set does not contain any devicetree users of the
new bindings. The new support/binding is used on X86 Cherry Trail
devices with a fusb302 Type-C controller (special variant of the
INT33FE device in ACPI). But this patch should also help getting
Display Port altmode to work with the mainline kernel on boards
like the Pine RockPro64 and Pinebook Pro, which is why I've added
Tobias Schramm to the Cc since he has done mainline devicetree
work for the Pinebook Pro in the past.

The 1st patch adds the dt-bindings docs. I'm not sure if this one
should go upstream through the USB tree together with patches 2-3 or
if this should go upstream separately, Rob ?

Patches 2-3 add support for the new binding to Type-C controller drivers
using the tcpm framework, such as the fusb302 driver.

Patch 4 uses swnodes to add the altmode info on the earlier mentioned
X86 CHT devices, making DP-altmode work there for the first time.

Regards,

Hans

[PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes

[Hans de Goede]

This commit adds the minimum bindings required to allow describing which
altmodes a port supports. Currently this is limited to just specifying:

1. The svid, which is the id of the altmode, e.g. displayport altmode has
a svid of 0xff01.

2. The vdo, a 32 bit integer, typically used as a bitmask describing the
capabilities of the altmode, the bits in the vdo are specified in the
specification of the altmode, the dt-binding simply refers to the
specification as that is the canonical source of the meaning of the bits.

Later on we may want to extend the binding with extra properties specific
to some altmode, but for now this is sufficient to e.g. hook up
displayport alternate-mode.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note I hope I got the yaml correct, this is my first time writing a
dt-binding in the new yaml style. I did run:
make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/connector/usb-connector.yaml
and that was happy.
---
 .../bindings/connector/usb-connector.yaml     | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml
index 9bd52e63c935..389e800c9fe8 100644
--- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
+++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
@@ -119,6 +119,27 @@ properties:
       offer the power, Capability Mismatch is set. Required for power sink and
       power dual role.
 
+  altmodes:
+    description:
+      List of child nodes that specify the altmodes supported by the
+      "usb-c-connector". The child nodes must be named foo-altmode, e.g.
+      "displayport-altmode".
+
+    patternProperties:
+      "^.*-altmode$":
+        type: object
+        description: The altmodes node has 1 child-node per supported altmode.
+        properties:
+          svid:
+            description: USB Type-C / PD altmode-svid, see the USB specifications
+              for details.
+          vdo:
+            description: USB Type-C / PD altmode-vdo, see the USB specifications
+              for details.
+        required:
+          - svid
+          - vdo
+
   ports:
     description: OF graph bindings (specified in bindings/graph.txt) that model
       any data bus to the connector unless the bus is between parent node and
-- 
2.26.2

Re: [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes

[Rob Herring]

On Tue, Jul 14, 2020 at 01:36:14PM +0200, Hans de Goede wrote:
> This commit adds the minimum bindings required to allow describing which
> altmodes a port supports. Currently this is limited to just specifying:
> 
> 1. The svid, which is the id of the altmode, e.g. displayport altmode has
> a svid of 0xff01.
> 
> 2. The vdo, a 32 bit integer, typically used as a bitmask describing the
> capabilities of the altmode, the bits in the vdo are specified in the
> specification of the altmode, the dt-binding simply refers to the
> specification as that is the canonical source of the meaning of the bits.

What if this information should be derived from information already in 
DT (or would be there if alt mode connections are described)?

> 
> Later on we may want to extend the binding with extra properties specific
> to some altmode, but for now this is sufficient to e.g. hook up
> displayport alternate-mode.

I don't think this is sufficient as it doesn't describe how alternate 
modes are connected to various components. This has been discussed some 
here[1] with the CrOS folks. Maybe this is orthogonal, IDK, but I really 
need something that is somewhat complete and not sprinkle a few new 
properties at a time.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note I hope I got the yaml correct, this is my first time writing a
> dt-binding in the new yaml style. I did run:
> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/connector/usb-connector.yaml
> and that was happy.

That aspect of it looks fine.

Rob

[1] https://lkml.org/lkml/2020/4/22/1819

Re: [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes

[Prashant Malani]

HI Rob,

On Mon, Jul 20, 2020 at 7:26 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Jul 14, 2020 at 01:36:14PM +0200, Hans de Goede wrote:
> > This commit adds the minimum bindings required to allow describing which
> > altmodes a port supports. Currently this is limited to just specifying:
> >
> > 1. The svid, which is the id of the altmode, e.g. displayport altmode has
> > a svid of 0xff01.
> >
> > 2. The vdo, a 32 bit integer, typically used as a bitmask describing the
> > capabilities of the altmode, the bits in the vdo are specified in the
> > specification of the altmode, the dt-binding simply refers to the
> > specification as that is the canonical source of the meaning of the bits.
>
> What if this information should be derived from information already in
> DT (or would be there if alt mode connections are described)?
>
> >
> > Later on we may want to extend the binding with extra properties specific
> > to some altmode, but for now this is sufficient to e.g. hook up
> > displayport alternate-mode.
>
> I don't think this is sufficient as it doesn't describe how alternate
> modes are connected to various components. This has been discussed some
> here[1] with the CrOS folks. Maybe this is orthogonal, IDK, but I really
> need something that is somewhat complete and not sprinkle a few new
> properties at a time.

Just thought I'd add a link[2] to some amendments which were made to
the "switch"
proposal in the thread you added a link to (it has many responses, so
not sure if my
responses got lost in the thread). I also wrote a short summary[3].

Please ignore these if you've already taken a look at them :)

Best regards,

-Prashant

[2] https://lkml.org/lkml/2020/6/12/602
[3] https://lkml.org/lkml/2020/7/10/234
>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > Note I hope I got the yaml correct, this is my first time writing a
> > dt-binding in the new yaml style. I did run:
> > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/connector/usb-connector.yaml
> > and that was happy.
>
> That aspect of it looks fine.
>
> Rob
>
> [1] https://lkml.org/lkml/2020/4/22/1819
>

Re: [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes

[Hans de Goede]

Hi,

On 7/21/20 4:26 AM, Rob Herring wrote:
> On Tue, Jul 14, 2020 at 01:36:14PM +0200, Hans de Goede wrote:
>> This commit adds the minimum bindings required to allow describing which
>> altmodes a port supports. Currently this is limited to just specifying:
>>
>> 1. The svid, which is the id of the altmode, e.g. displayport altmode has
>> a svid of 0xff01.
>>
>> 2. The vdo, a 32 bit integer, typically used as a bitmask describing the
>> capabilities of the altmode, the bits in the vdo are specified in the
>> specification of the altmode, the dt-binding simply refers to the
>> specification as that is the canonical source of the meaning of the bits.
> 
> What if this information should be derived from information already in
> DT (or would be there if alt mode connections are described)?
> 
>>
>> Later on we may want to extend the binding with extra properties specific
>> to some altmode, but for now this is sufficient to e.g. hook up
>> displayport alternate-mode.
> 
> I don't think this is sufficient as it doesn't describe how alternate
> modes are connected to various components. This has been discussed some
> here[1] with the CrOS folks. Maybe this is orthogonal, IDK, but I really
> need something that is somewhat complete and not sprinkle a few new
> properties at a time.

Right, but that is an orthogonal problem, this is telling the Type-C
controller which modes it is allowed to negotiate and which capabilties
(altmode specific, stored in the vdo) it should advertise.

I agree that if the connector is connected to a mux and how that mux is then
connected to the SoC, or if the SoC has a multi-mode phy also needs to be
specified in some cases. But that is mostly a separate problem.
One thing which we will want to add to this part of the bindings when that
other part is in place is a link to the endpoint *after* the mux, that is
after the mode- and role-switch in Prashant's example here:
https://lkml.org/lkml/2020/6/12/602

The Type-C controller may receive out-of-band messages related to the
altmode (through USB-PD messages) which need to be communicated to
the endpoint, so in the case of display-port altmode, the dp0_out_ep
from Prashant's example. Note the link/object reference I'm suggesting
here deliberately skips the mux, since the oob messages need to be
send through the endpoint without the mux being involved since they are
oob after all.

Specifically there is no pin on the Type-C connector for the display-port
hotplug-detect pin, so hot(un)plug is signaled through altmode specific
USB-PD messages.

Note that this binding and the 2 patches implementing it for x86
devices (*), are already useful / functional. The user just needs to
manually run "xrandr" to force the video-output driver to manually
recheck for new/changed monitors, just like an old VGA ports without
load detection.

I haven't fully figured out how to wire up the hotplug signal in the
kernel yet, which is why the link to the DP endpoint is not yet part of
the bindings.

*) Using sw-fw-nodes to pass the info from a drivers/platform/x86/
driver to the Type-C controller code which uses fw_nodes to get this info

So since this is x86 only for now; and AFAIK you don't want to take bindings
upstream until there is an actual DT user anyways, my main goal of including
this was to see if we are at least on the right way with this. With x86 it
is all in the kernel, so if the binding changes a bit we can easily adjust the
drivers/platform/x86/ code generating the nodes at the same time as we
update the Type-C controller code to implement the final binding. But it
would be good to know that we are at least going in the right direction.

BTW note that making the binding look like this was proposed by Heikki,
the Type-C subsys maintainer, I ended up implementing this because Heikki
did no have the time for it.

Regards,

Hans





> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note I hope I got the yaml correct, this is my first time writing a
>> dt-binding in the new yaml style. I did run:
>> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/connector/usb-connector.yaml
>> and that was happy.
> 
> That aspect of it looks fine.
> 
> Rob
> 
> [1] https://lkml.org/lkml/2020/4/22/1819
> 

Re: [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes

[Prashant Malani]

Hi Rob,

Restarting this thread, since I think we can re-use Patch 1/4, and I
dind't want some of the context to be lost by starting a new thread.

On Wed, Jul 22, 2020 at 09:18:02AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/21/20 4:26 AM, Rob Herring wrote:
> > On Tue, Jul 14, 2020 at 01:36:14PM +0200, Hans de Goede wrote:
> > > This commit adds the minimum bindings required to allow describing which
> > > altmodes a port supports. Currently this is limited to just specifying:
> > > 
> > > 1. The svid, which is the id of the altmode, e.g. displayport altmode has
> > > a svid of 0xff01.
> > > 
> > > 2. The vdo, a 32 bit integer, typically used as a bitmask describing the
> > > capabilities of the altmode, the bits in the vdo are specified in the
> > > specification of the altmode, the dt-binding simply refers to the
> > > specification as that is the canonical source of the meaning of the bits.
> > 
> > What if this information should be derived from information already in
> > DT (or would be there if alt mode connections are described)?
> > 
> > > 
> > > Later on we may want to extend the binding with extra properties specific
> > > to some altmode, but for now this is sufficient to e.g. hook up
> > > displayport alternate-mode.
> > 
> > I don't think this is sufficient as it doesn't describe how alternate
> > modes are connected to various components. This has been discussed some
> > here[1] with the CrOS folks. Maybe this is orthogonal, IDK, but I really
> > need something that is somewhat complete and not sprinkle a few new
> > properties at a time.
> 
> Right, but that is an orthogonal problem, this is telling the Type-C
> controller which modes it is allowed to negotiate and which capabilties
> (altmode specific, stored in the vdo) it should advertise.
> 

I concur. There is value in listing the alternate modes supported by a
connector in the connector bindings. PD negotiation (which may include
alternate mode entry) is something which is handled
by the port driver / TCPM itself, so this sounds like a reasonable place
to explicitly describe this information rather than derive it from OF
graph.

While it is important to describe how the connector is routed through the
switches and eventually to the various controllers (DP, xHCI, USB4 etc.),
it doesn't sound like we should make the Type C port driver rely on those
graph connections to derive what alternate modes to support.

FWIW, I did provide a more fleshed out example of how the OF graph
connections from port to various PHYs would work a while back, but
didn't get much feedback on it [1]

> BTW note that making the binding look like this was proposed by Heikki,
> the Type-C subsys maintainer, I ended up implementing this because Heikki
> did no have the time for it.

If the binding itself looks fine, then I'd request we move forward with
including it in the usb-connector bindings rather than stalling on the
OF graph switch bindings.
Heikki had mentioned [2] that we can adjust the ACPI bindings to accommodate
device tree requirements, and it looks like the current implementation is already in
the mainline connector class code [3], just the binding is missing.

I would be happy to re-upload this patch, with follow on patches which:
- Add the altmodes node to a Chrome OS device tree file
- Update the cros-ec-typec port driver to call the function introduced in [3].

I've tested this locally and it works fine.

[1]:
https://lore.kernel.org/lkml/CACeCKacUa1-ttBmKS_Q_xZCsArgGWkB4s9eG0c5Lc5RHa1W35Q@mail.gmail.com/
[2]:
https://lore.kernel.org/linux-usb/Ya8vxq+%2FP%2FWG4kHo@kuha.fi.intel.com/
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b458a4c5d7302947556e12c83cfe4da769665d0

Would be good to hear your thoughts on the above.

Thanks,

-Prashant

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

[Hans de Goede]

This can be used by Type-C controller drivers which use a standard
usb-connector fwnode, with altmodes sub-node, to describe the available
altmodes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/class.c | 56 +++++++++++++++++++++++++++++++++++++++
 include/linux/usb/typec.h |  7 +++++
 2 files changed, 63 insertions(+)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index c9234748537a..47de2b2e3d54 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1607,6 +1607,62 @@ typec_port_register_altmode(struct typec_port *port,
 }
 EXPORT_SYMBOL_GPL(typec_port_register_altmode);
 
+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;
+
+	child = NULL;
+	while ((child = fwnode_get_next_child_node(altmodes_node, child))) {
+		ret = fwnode_property_read_u32(child, "svid", &svid);
+		if (ret) {
+			dev_err(&port->dev, "Error reading svid for altmode %s\n",
+				fwnode_get_name(child));
+			continue;
+		}
+
+		ret = fwnode_property_read_u32(child, "vdo", &vdo);
+		if (ret) {
+			dev_err(&port->dev, "Error reading vdo for altmode %s\n",
+				fwnode_get_name(child));
+			continue;
+		}
+
+		if (index >= n) {
+			dev_err(&port->dev, "Error not enough space for altmode %s\n",
+				fwnode_get_name(child));
+			continue;
+		}
+
+		desc.svid = svid;
+		desc.vdo = vdo;
+		desc.mode = index + 1;
+		alt = typec_port_register_altmode(port, &desc);
+		if (IS_ERR(alt)) {
+			dev_err(&port->dev, "Error registering altmode %s\n",
+				fwnode_get_name(child));
+			continue;
+		}
+
+		alt->ops = ops;
+		typec_altmode_set_drvdata(alt, drvdata);
+		altmodes[index] = alt;
+		index++;
+	}
+}
+EXPORT_SYMBOL_GPL(typec_port_register_altmodes_from_fwnode);
+
 /**
  * typec_register_port - Register a USB Type-C Port
  * @parent: Parent device
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 5daa1c49761c..fbe4bccb3a98 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -17,6 +17,7 @@ struct typec_partner;
 struct typec_cable;
 struct typec_plug;
 struct typec_port;
+struct typec_altmode_ops;
 
 struct fwnode_handle;
 struct device;
@@ -121,6 +122,12 @@ struct typec_altmode
 struct typec_altmode
 *typec_port_register_altmode(struct typec_port *port,
 			     const struct typec_altmode_desc *desc);
+
+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);
+
 void typec_unregister_altmode(struct typec_altmode *altmode);
 
 struct typec_port *typec_altmode2port(struct typec_altmode *alt);
-- 
2.26.2

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

[Guenter Roeck]

On 7/14/20 4:36 AM, Hans de Goede wrote:
> This can be used by Type-C controller drivers which use a standard
> usb-connector fwnode, with altmodes sub-node, to describe the available
> altmodes.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/class.c | 56 +++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h |  7 +++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index c9234748537a..47de2b2e3d54 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1607,6 +1607,62 @@ typec_port_register_altmode(struct typec_port *port,
>  }
>  EXPORT_SYMBOL_GPL(typec_port_register_altmode);
>  
> +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;
> +
> +	child = NULL;
> +	while ((child = fwnode_get_next_child_node(altmodes_node, child))) {
> +		ret = fwnode_property_read_u32(child, "svid", &svid);
> +		if (ret) {
> +			dev_err(&port->dev, "Error reading svid for altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;

The properties are mandatory. I think the errors should not be ignored.

> +		}
> +
> +		ret = fwnode_property_read_u32(child, "vdo", &vdo);
> +		if (ret) {
> +			dev_err(&port->dev, "Error reading vdo for altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;
> +		}
> +
> +		if (index >= n) {
> +			dev_err(&port->dev, "Error not enough space for altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;

Seems to be pointless to continue here.

> +		}
> +
> +		desc.svid = svid;
> +		desc.vdo = vdo;
> +		desc.mode = index + 1;
> +		alt = typec_port_register_altmode(port, &desc);
> +		if (IS_ERR(alt)) {
> +			dev_err(&port->dev, "Error registering altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;

Maybe there is a reason to ignore all those errors. If so,
that should be explained.

> +		}
> +
> +		alt->ops = ops;
> +		typec_altmode_set_drvdata(alt, drvdata);
> +		altmodes[index] = alt;
> +		index++;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(typec_port_register_altmodes_from_fwnode);
> +
>  /**
>   * typec_register_port - Register a USB Type-C Port
>   * @parent: Parent device
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index 5daa1c49761c..fbe4bccb3a98 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -17,6 +17,7 @@ struct typec_partner;
>  struct typec_cable;
>  struct typec_plug;
>  struct typec_port;
> +struct typec_altmode_ops;
>  
>  struct fwnode_handle;
>  struct device;
> @@ -121,6 +122,12 @@ struct typec_altmode
>  struct typec_altmode
>  *typec_port_register_altmode(struct typec_port *port,
>  			     const struct typec_altmode_desc *desc);
> +
> +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);
> +
>  void typec_unregister_altmode(struct typec_altmode *altmode);
>  
>  struct typec_port *typec_altmode2port(struct typec_altmode *alt);
> 

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

[Hans de Goede]

Hi Guenter,

Thank you for all the reviews.

On 7/15/20 6:39 PM, Guenter Roeck wrote:
> On 7/14/20 4:36 AM, Hans de Goede wrote:
>> This can be used by Type-C controller drivers which use a standard
>> usb-connector fwnode, with altmodes sub-node, to describe the available
>> altmodes.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/class.c | 56 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/usb/typec.h |  7 +++++
>>   2 files changed, 63 insertions(+)
>>
>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>> index c9234748537a..47de2b2e3d54 100644
>> --- a/drivers/usb/typec/class.c
>> +++ b/drivers/usb/typec/class.c
>> @@ -1607,6 +1607,62 @@ typec_port_register_altmode(struct typec_port *port,
>>   }
>>   EXPORT_SYMBOL_GPL(typec_port_register_altmode);
>>   
>> +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;
>> +
>> +	child = NULL;
>> +	while ((child = fwnode_get_next_child_node(altmodes_node, child))) {
>> +		ret = fwnode_property_read_u32(child, "svid", &svid);
>> +		if (ret) {
>> +			dev_err(&port->dev, "Error reading svid for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
> 
> The properties are mandatory. I think the errors should not be ignored.

I have done this this way deliberately, let me try to explain:

We basically have 2 choices here:

1) Log an error and continue, skipping/ignoring the faulty altmode fw-child-node
2) Make the error fatal, rollback all changes made so far and return an error
to our caller

First of all, these errors should never happen and if they do happen then the
person adding the new alt-mode to the dt, will presumably test that this alt-mode
works, sees that it does not, check the logs and know why. So for stable shipping
kernels I would expect to never hit this path.

Secondly even if we somehow do hit this path in a stable shipping kernel, then
what should our caller do if we return an error? Our caller basically has 2 choices:

1. Log and otherwise ignore the error
2. Completely abort the registration of the Type-C controller, possibly breaking
things like USB over the port, charging, etc.

2. Seems rather drastic and is not necessary, except for the alt-modes all the
other functionality of the port will work fine if the call fails. So our caller
should probably choose 1. as error handling strategy. If it does that, then for
the error logging it can rely on typec_port_register_altmodes_from_fwnode() having
already logged an error, so it can just treat it as returning void.

But if our caller does not care, would it then not be better to just ignore
any bad alt-mode child nodes and still try to register an alt-mode for the
good ones?

Thirdly adding code to unwind the registration of the alt-modes done so far
+ adding code to our caller to abort the port registration would be adding a
bunch of extra, fragile code for something which should (and likely will)
never happen. So we ware adding a bunch of code here which in essence is
pretty much never tested and thus is almost assured to either be broken
from day 1, or to become broken over time.

The kernel is likely full of error handling paths with bugs because it is
trying to handle errors which never happen. Sometimes this is necessary
because e.g. a driver's probe function cannot continue without acquiring
a certain resource. But in this case we can easily avoid the broken error
handling code syndrome; and keep the code nice and simple, by just skipping
over broken nodes.

> 
>> +		}
>> +
>> +		ret = fwnode_property_read_u32(child, "vdo", &vdo);
>> +		if (ret) {
>> +			dev_err(&port->dev, "Error reading vdo for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		if (index >= n) {
>> +			dev_err(&port->dev, "Error not enough space for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
> 
> Seems to be pointless to continue here.

Continuing here makes sure that if the dts contains 10 alt-modes and n = 8 that we print
an error for both alt-modes which we are not registering because there is not enough space
in the passed in array for storing alt-mode pointers.

It also ensures that we error check any further nodes for missing svid/vdo properties.


> 
>> +		}
>> +
>> +		desc.svid = svid;
>> +		desc.vdo = vdo;
>> +		desc.mode = index + 1;
>> +		alt = typec_port_register_altmode(port, &desc);
>> +		if (IS_ERR(alt)) {
>> +			dev_err(&port->dev, "Error registering altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
> 
> Maybe there is a reason to ignore all those errors. If so,
> that should be explained.

See above, note this is another error which should never happen, I think
this can only happen in case of -ENOMEM, which itself can only happen
for allocations of an order greater then n=2 AFAIK, and I don't think
typec_port_register_altmode() does any such allocations.

I guess some comment here is warranted, but my full explanation above
is way too long. So maybe a simple comment like this?  :

			/* Should never happen, keep the error handling as simple as possible */

Regards,

Hans


> 
>> +		}
>> +
>> +		alt->ops = ops;
>> +		typec_altmode_set_drvdata(alt, drvdata);
>> +		altmodes[index] = alt;
>> +		index++;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(typec_port_register_altmodes_from_fwnode);
>> +
>>   /**
>>    * typec_register_port - Register a USB Type-C Port
>>    * @parent: Parent device
>> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
>> index 5daa1c49761c..fbe4bccb3a98 100644
>> --- a/include/linux/usb/typec.h
>> +++ b/include/linux/usb/typec.h
>> @@ -17,6 +17,7 @@ struct typec_partner;
>>   struct typec_cable;
>>   struct typec_plug;
>>   struct typec_port;
>> +struct typec_altmode_ops;
>>   
>>   struct fwnode_handle;
>>   struct device;
>> @@ -121,6 +122,12 @@ struct typec_altmode
>>   struct typec_altmode
>>   *typec_port_register_altmode(struct typec_port *port,
>>   			     const struct typec_altmode_desc *desc);
>> +
>> +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);
>> +
>>   void typec_unregister_altmode(struct typec_altmode *altmode);
>>   
>>   struct typec_port *typec_altmode2port(struct typec_altmode *alt);
>>
> 

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

[Guenter Roeck]

On Wed, Jul 15, 2020 at 11:14:03PM +0200, Hans de Goede wrote:
> Hi Guenter,
> 
> Thank you for all the reviews.
> 
> On 7/15/20 6:39 PM, Guenter Roeck wrote:
> > On 7/14/20 4:36 AM, Hans de Goede wrote:
> > > This can be used by Type-C controller drivers which use a standard
> > > usb-connector fwnode, with altmodes sub-node, to describe the available
> > > altmodes.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >   drivers/usb/typec/class.c | 56 +++++++++++++++++++++++++++++++++++++++
> > >   include/linux/usb/typec.h |  7 +++++
> > >   2 files changed, 63 insertions(+)
> > > 
> > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > > index c9234748537a..47de2b2e3d54 100644
> > > --- a/drivers/usb/typec/class.c
> > > +++ b/drivers/usb/typec/class.c
> > > @@ -1607,6 +1607,62 @@ typec_port_register_altmode(struct typec_port *port,
> > >   }
> > >   EXPORT_SYMBOL_GPL(typec_port_register_altmode);
> > > +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;
> > > +
> > > +	child = NULL;
> > > +	while ((child = fwnode_get_next_child_node(altmodes_node, child))) {
> > > +		ret = fwnode_property_read_u32(child, "svid", &svid);
> > > +		if (ret) {
> > > +			dev_err(&port->dev, "Error reading svid for altmode %s\n",
> > > +				fwnode_get_name(child));
> > > +			continue;
> > 
> > The properties are mandatory. I think the errors should not be ignored.
> 
> I have done this this way deliberately, let me try to explain:
> 
> We basically have 2 choices here:
> 
> 1) Log an error and continue, skipping/ignoring the faulty altmode fw-child-node
> 2) Make the error fatal, rollback all changes made so far and return an error
> to our caller
> 
> First of all, these errors should never happen and if they do happen then the
> person adding the new alt-mode to the dt, will presumably test that this alt-mode
> works, sees that it does not, check the logs and know why. So for stable shipping
> kernels I would expect to never hit this path.
> 
> Secondly even if we somehow do hit this path in a stable shipping kernel, then
> what should our caller do if we return an error? Our caller basically has 2 choices:
> 
> 1. Log and otherwise ignore the error
> 2. Completely abort the registration of the Type-C controller, possibly breaking
> things like USB over the port, charging, etc.
> 
> 2. Seems rather drastic and is not necessary, except for the alt-modes all the
> other functionality of the port will work fine if the call fails. So our caller
> should probably choose 1. as error handling strategy. If it does that, then for
> the error logging it can rely on typec_port_register_altmodes_from_fwnode() having
> already logged an error, so it can just treat it as returning void.
> 
> But if our caller does not care, would it then not be better to just ignore
> any bad alt-mode child nodes and still try to register an alt-mode for the
> good ones?
> 
> Thirdly adding code to unwind the registration of the alt-modes done so far
> + adding code to our caller to abort the port registration would be adding a
> bunch of extra, fragile code for something which should (and likely will)
> never happen. So we ware adding a bunch of code here which in essence is
> pretty much never tested and thus is almost assured to either be broken
> from day 1, or to become broken over time.
> 
> The kernel is likely full of error handling paths with bugs because it is
> trying to handle errors which never happen. Sometimes this is necessary
> because e.g. a driver's probe function cannot continue without acquiring
> a certain resource. But in this case we can easily avoid the broken error
> handling code syndrome; and keep the code nice and simple, by just skipping
> over broken nodes.
> 

IMO all errors should be handled. I don't really trust implementers to do
the right thing. I have seen too many patches which don't even compile.
Correct, this should not happen, and it won't happen if the DT is correct.
Only a complete abort will really force the implementer to fix the problem.

Yes, error handling is not always properly implemented. In such cases, error
handling should be fixed, not abandoned.

I understand that we are well into philosophy. I'll let others decide if
they want to accept your patch as-is.

Thanks,
Guenter

> > 
> > > +		}
> > > +
> > > +		ret = fwnode_property_read_u32(child, "vdo", &vdo);
> > > +		if (ret) {
> > > +			dev_err(&port->dev, "Error reading vdo for altmode %s\n",
> > > +				fwnode_get_name(child));
> > > +			continue;
> > > +		}
> > > +
> > > +		if (index >= n) {
> > > +			dev_err(&port->dev, "Error not enough space for altmode %s\n",
> > > +				fwnode_get_name(child));
> > > +			continue;
> > 
> > Seems to be pointless to continue here.
> 
> Continuing here makes sure that if the dts contains 10 alt-modes and n = 8 that we print
> an error for both alt-modes which we are not registering because there is not enough space
> in the passed in array for storing alt-mode pointers.
> 
> It also ensures that we error check any further nodes for missing svid/vdo properties.
> 
> 
> > 
> > > +		}
> > > +
> > > +		desc.svid = svid;
> > > +		desc.vdo = vdo;
> > > +		desc.mode = index + 1;
> > > +		alt = typec_port_register_altmode(port, &desc);
> > > +		if (IS_ERR(alt)) {
> > > +			dev_err(&port->dev, "Error registering altmode %s\n",
> > > +				fwnode_get_name(child));
> > > +			continue;
> > 
> > Maybe there is a reason to ignore all those errors. If so,
> > that should be explained.
> 
> See above, note this is another error which should never happen, I think
> this can only happen in case of -ENOMEM, which itself can only happen
> for allocations of an order greater then n=2 AFAIK, and I don't think
> typec_port_register_altmode() does any such allocations.
> 
> I guess some comment here is warranted, but my full explanation above
> is way too long. So maybe a simple comment like this?  :
> 
> 			/* Should never happen, keep the error handling as simple as possible */
> 
> Regards,
> 
> Hans
> 
> 
> > 
> > > +		}
> > > +
> > > +		alt->ops = ops;
> > > +		typec_altmode_set_drvdata(alt, drvdata);
> > > +		altmodes[index] = alt;
> > > +		index++;
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL_GPL(typec_port_register_altmodes_from_fwnode);
> > > +
> > >   /**
> > >    * typec_register_port - Register a USB Type-C Port
> > >    * @parent: Parent device
> > > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> > > index 5daa1c49761c..fbe4bccb3a98 100644
> > > --- a/include/linux/usb/typec.h
> > > +++ b/include/linux/usb/typec.h
> > > @@ -17,6 +17,7 @@ struct typec_partner;
> > >   struct typec_cable;
> > >   struct typec_plug;
> > >   struct typec_port;
> > > +struct typec_altmode_ops;
> > >   struct fwnode_handle;
> > >   struct device;
> > > @@ -121,6 +122,12 @@ struct typec_altmode
> > >   struct typec_altmode
> > >   *typec_port_register_altmode(struct typec_port *port,
> > >   			     const struct typec_altmode_desc *desc);
> > > +
> > > +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);
> > > +
> > >   void typec_unregister_altmode(struct typec_altmode *altmode);
> > >   struct typec_port *typec_altmode2port(struct typec_altmode *alt);
> > > 
> > 
> 

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

[Heikki Krogerus]

Hi Hans,

On Tue, Jul 14, 2020 at 01:36:15PM +0200, Hans de Goede wrote:
> This can be used by Type-C controller drivers which use a standard
> usb-connector fwnode, with altmodes sub-node, to describe the available
> altmodes.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/class.c | 56 +++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h |  7 +++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index c9234748537a..47de2b2e3d54 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1607,6 +1607,62 @@ typec_port_register_altmode(struct typec_port *port,
>  }
>  EXPORT_SYMBOL_GPL(typec_port_register_altmode);
>  
> +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 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?


> +	child = NULL;
> +	while ((child = fwnode_get_next_child_node(altmodes_node, child))) {
> +		ret = fwnode_property_read_u32(child, "svid", &svid);
> +		if (ret) {
> +			dev_err(&port->dev, "Error reading svid for altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;
> +		}
> +
> +		ret = fwnode_property_read_u32(child, "vdo", &vdo);
> +		if (ret) {
> +			dev_err(&port->dev, "Error reading vdo for altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;
> +		}
> +
> +		if (index >= n) {
> +			dev_err(&port->dev, "Error not enough space for altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;
> +		}
> +
> +		desc.svid = svid;
> +		desc.vdo = vdo;
> +		desc.mode = index + 1;
> +		alt = typec_port_register_altmode(port, &desc);
> +		if (IS_ERR(alt)) {
> +			dev_err(&port->dev, "Error registering altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;
> +		}
> +
> +		alt->ops = ops;
> +		typec_altmode_set_drvdata(alt, drvdata);
> +		altmodes[index] = alt;
> +		index++;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(typec_port_register_altmodes_from_fwnode);
> +
>  /**
>   * typec_register_port - Register a USB Type-C Port
>   * @parent: Parent device
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index 5daa1c49761c..fbe4bccb3a98 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -17,6 +17,7 @@ struct typec_partner;
>  struct typec_cable;
>  struct typec_plug;
>  struct typec_port;
> +struct typec_altmode_ops;
>  
>  struct fwnode_handle;
>  struct device;
> @@ -121,6 +122,12 @@ struct typec_altmode
>  struct typec_altmode
>  *typec_port_register_altmode(struct typec_port *port,
>  			     const struct typec_altmode_desc *desc);
> +
> +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);
> +
>  void typec_unregister_altmode(struct typec_altmode *altmode);
>  
>  struct typec_port *typec_altmode2port(struct typec_altmode *alt);
> -- 
> 2.26.2

thanks,

-- 
heikki

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

[Hans de Goede]

Hi,

On 7/27/20 3:05 PM, Heikki Krogerus wrote:
> Hi Hans,
> 
> On Tue, Jul 14, 2020 at 01:36:15PM +0200, Hans de Goede wrote:
>> This can be used by Type-C controller drivers which use a standard
>> usb-connector fwnode, with altmodes sub-node, to describe the available
>> altmodes.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/class.c | 56 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/usb/typec.h |  7 +++++
>>   2 files changed, 63 insertions(+)
>>
>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>> index c9234748537a..47de2b2e3d54 100644
>> --- a/drivers/usb/typec/class.c
>> +++ b/drivers/usb/typec/class.c
>> @@ -1607,6 +1607,62 @@ typec_port_register_altmode(struct typec_port *port,
>>   }
>>   EXPORT_SYMBOL_GPL(typec_port_register_altmode);
>>   
>> +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.

Regards,

Hans


> 
> 
>> +	child = NULL;
>> +	while ((child = fwnode_get_next_child_node(altmodes_node, child))) {
>> +		ret = fwnode_property_read_u32(child, "svid", &svid);
>> +		if (ret) {
>> +			dev_err(&port->dev, "Error reading svid for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		ret = fwnode_property_read_u32(child, "vdo", &vdo);
>> +		if (ret) {
>> +			dev_err(&port->dev, "Error reading vdo for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		if (index >= n) {
>> +			dev_err(&port->dev, "Error not enough space for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		desc.svid = svid;
>> +		desc.vdo = vdo;
>> +		desc.mode = index + 1;
>> +		alt = typec_port_register_altmode(port, &desc);
>> +		if (IS_ERR(alt)) {
>> +			dev_err(&port->dev, "Error registering altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		alt->ops = ops;
>> +		typec_altmode_set_drvdata(alt, drvdata);
>> +		altmodes[index] = alt;
>> +		index++;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(typec_port_register_altmodes_from_fwnode);
>> +
>>   /**
>>    * typec_register_port - Register a USB Type-C Port
>>    * @parent: Parent device
>> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
>> index 5daa1c49761c..fbe4bccb3a98 100644
>> --- a/include/linux/usb/typec.h
>> +++ b/include/linux/usb/typec.h
>> @@ -17,6 +17,7 @@ struct typec_partner;
>>   struct typec_cable;
>>   struct typec_plug;
>>   struct typec_port;
>> +struct typec_altmode_ops;
>>   
>>   struct fwnode_handle;
>>   struct device;
>> @@ -121,6 +122,12 @@ struct typec_altmode
>>   struct typec_altmode
>>   *typec_port_register_altmode(struct typec_port *port,
>>   			     const struct typec_altmode_desc *desc);
>> +
>> +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);
>> +
>>   void typec_unregister_altmode(struct typec_altmode *altmode);
>>   
>>   struct typec_port *typec_altmode2port(struct typec_altmode *alt);
>> -- 
>> 2.26.2
> 
> thanks,
> 

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

[Heikki Krogerus]

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).

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.

thanks,

-- 
heikki

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

[Hans de Goede]

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 ?

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.

Regards,

Hans

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

[Heikki Krogerus]

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?

thanks,

-- 
heikki

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

[Hans de Goede]

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.

Yes that would be very good to have.

> 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.

Ok.

> 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?

Sounds good. Note my use case is not really DT though, it is software
fw-nodes injected by drivers/platform/x86/intel_cht_int33fe_typec.c
because the ACPI tables on devices do not contain any info on altmodes
at all. I tried to come up with something which works for the DT case
too. But as long as we do not have actual DT users we can actually change
things if necessary since the interface between the injected
software fw-nodes and the typec_port_register_altmodes_from_fwnode()
code parsing it is purely an in kernel interface (*).

Regards,

Hans


*) This stops being purely in kernel once we get actual dts files
using it.

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

[Hans de Goede]

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

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

[Heikki Krogerus]

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

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

[Heikki Krogerus]

On Tue, Jul 14, 2020 at 01:36:15PM +0200, Hans de Goede wrote:
> This can be used by Type-C controller drivers which use a standard
> usb-connector fwnode, with altmodes sub-node, to describe the available
> altmodes.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/class.c | 56 +++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h |  7 +++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index c9234748537a..47de2b2e3d54 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1607,6 +1607,62 @@ typec_port_register_altmode(struct typec_port *port,
>  }
>  EXPORT_SYMBOL_GPL(typec_port_register_altmode);
>  
> +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)

That last fwnode parameter should not be needed. The port device
should have the alternate mode nodes under it as child nodes. That is,
unless I'm missing (or forgetting) something?

> +{
> +	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");

So this would be:

        altmodes_node = fwnode_get_named_child_node(port->dev.fwnode, "altmodes");

> +	if (!altmodes_node)
> +		return;
> +
> +	child = NULL;
> +	while ((child = fwnode_get_next_child_node(altmodes_node, child))) {
> +		ret = fwnode_property_read_u32(child, "svid", &svid);
> +		if (ret) {
> +			dev_err(&port->dev, "Error reading svid for altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;
> +		}
> +
> +		ret = fwnode_property_read_u32(child, "vdo", &vdo);
> +		if (ret) {
> +			dev_err(&port->dev, "Error reading vdo for altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;
> +		}
> +
> +		if (index >= n) {
> +			dev_err(&port->dev, "Error not enough space for altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;
> +		}
> +
> +		desc.svid = svid;
> +		desc.vdo = vdo;
> +		desc.mode = index + 1;
> +		alt = typec_port_register_altmode(port, &desc);
> +		if (IS_ERR(alt)) {
> +			dev_err(&port->dev, "Error registering altmode %s\n",
> +				fwnode_get_name(child));
> +			continue;
> +		}
> +
> +		alt->ops = ops;
> +		typec_altmode_set_drvdata(alt, drvdata);
> +		altmodes[index] = alt;
> +		index++;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(typec_port_register_altmodes_from_fwnode);
> +
>  /**
>   * typec_register_port - Register a USB Type-C Port
>   * @parent: Parent device

thanks,

-- 
heikki

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

[Hans de Goede]

Hi,

Sorry for being very very slow with getting around to
cleaning this up, there just always was some higher prio
item getting in the way; and any downtime I had I needed time
to recuperate.

On 8/26/20 3:17 PM, Heikki Krogerus wrote:
> On Tue, Jul 14, 2020 at 01:36:15PM +0200, Hans de Goede wrote:
>> This can be used by Type-C controller drivers which use a standard
>> usb-connector fwnode, with altmodes sub-node, to describe the available
>> altmodes.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/usb/typec/class.c | 56 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/usb/typec.h |  7 +++++
>>  2 files changed, 63 insertions(+)
>>
>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>> index c9234748537a..47de2b2e3d54 100644
>> --- a/drivers/usb/typec/class.c
>> +++ b/drivers/usb/typec/class.c
>> @@ -1607,6 +1607,62 @@ typec_port_register_altmode(struct typec_port *port,
>>  }
>>  EXPORT_SYMBOL_GPL(typec_port_register_altmode);
>>  
>> +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)
> 
> That last fwnode parameter should not be needed. The port device
> should have the alternate mode nodes under it as child nodes. That is,
> unless I'm missing (or forgetting) something?

Ack, I'll fix this, test the code and send a v2 soon.

Regards,

Hans



> 
>> +{
>> +	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");
> 
> So this would be:
> 
>         altmodes_node = fwnode_get_named_child_node(port->dev.fwnode, "altmodes");
> 
>> +	if (!altmodes_node)
>> +		return;
>> +
>> +	child = NULL;
>> +	while ((child = fwnode_get_next_child_node(altmodes_node, child))) {
>> +		ret = fwnode_property_read_u32(child, "svid", &svid);
>> +		if (ret) {
>> +			dev_err(&port->dev, "Error reading svid for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		ret = fwnode_property_read_u32(child, "vdo", &vdo);
>> +		if (ret) {
>> +			dev_err(&port->dev, "Error reading vdo for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		if (index >= n) {
>> +			dev_err(&port->dev, "Error not enough space for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		desc.svid = svid;
>> +		desc.vdo = vdo;
>> +		desc.mode = index + 1;
>> +		alt = typec_port_register_altmode(port, &desc);
>> +		if (IS_ERR(alt)) {
>> +			dev_err(&port->dev, "Error registering altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		alt->ops = ops;
>> +		typec_altmode_set_drvdata(alt, drvdata);
>> +		altmodes[index] = alt;
>> +		index++;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(typec_port_register_altmodes_from_fwnode);
>> +
>>  /**
>>   * typec_register_port - Register a USB Type-C Port
>>   * @parent: Parent device
> 
> thanks,
> 

[PATCH 3/4] usb: typec: tcpm: Add support for altmodes

[Hans de Goede]

Add support for altmodes described in the usb-connector fwnode
associated with the Type-C controller by calling the new
typec_port_register_altmodes_from_fwnode() helper for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 82b19ebd7838..b4a66e6bf68c 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -4793,6 +4793,12 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 		goto out_role_sw_put;
 	}
 
+	typec_port_register_altmodes_from_fwnode(port->typec_port,
+						 &tcpm_altmode_ops, port,
+						 port->port_altmode,
+						 ALTMODE_DISCOVERY_MAX,
+						 tcpc->fwnode);
+
 	mutex_lock(&port->lock);
 	tcpm_init(port);
 	mutex_unlock(&port->lock);
-- 
2.26.2

Re: [PATCH 3/4] usb: typec: tcpm: Add support for altmodes

[Guenter Roeck]

On 7/14/20 4:36 AM, Hans de Goede wrote:
> Add support for altmodes described in the usb-connector fwnode
> associated with the Type-C controller by calling the new
> typec_port_register_altmodes_from_fwnode() helper for this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 82b19ebd7838..b4a66e6bf68c 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4793,6 +4793,12 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  		goto out_role_sw_put;
>  	}
>  
> +	typec_port_register_altmodes_from_fwnode(port->typec_port,
> +						 &tcpm_altmode_ops, port,
> +						 port->port_altmode,
> +						 ALTMODE_DISCOVERY_MAX,
> +						 tcpc->fwnode);

As mentioned in the other patch, errors from this function should not
be ignored (or there should be a detailed explanation why it is ok
to ignore them).

Thanks,
Guenter

> +
>  	mutex_lock(&port->lock);
>  	tcpm_init(port);
>  	mutex_unlock(&port->lock);
> 

[PATCH 4/4] platform/x86/intel_cht_int33fe: Add displayport altmode fwnode to the connector fwnode

[Hans de Goede]

Add a displayport altmode fwnode to the usb-connector fwnode,
devices which use this driver support display-port altmode through
the PI3USB30532 USB switch, this enables support for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../platform/x86/intel_cht_int33fe_typec.c    | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/platform/x86/intel_cht_int33fe_typec.c b/drivers/platform/x86/intel_cht_int33fe_typec.c
index 48638d1c56e5..b61bad9cc8d2 100644
--- a/drivers/platform/x86/intel_cht_int33fe_typec.c
+++ b/drivers/platform/x86/intel_cht_int33fe_typec.c
@@ -124,12 +124,31 @@ static const struct software_node usb_connector_node = {
 	.properties = usb_connector_properties,
 };
 
+static const struct software_node altmodes_node = {
+	.name = "altmodes",
+	.parent = &usb_connector_node,
+};
+
+static const struct property_entry dp_altmode_properties[] = {
+	PROPERTY_ENTRY_U32("svid", 0xff01),
+	PROPERTY_ENTRY_U32("vdo", 0x0c0086),
+	{ }
+};
+
+static const struct software_node dp_altmode_node = {
+	.name = "displayport-altmode",
+	.parent = &altmodes_node,
+	.properties = dp_altmode_properties,
+};
+
 static const struct software_node *node_group[] = {
 	&fusb302_node,
 	&max17047_node,
 	&pi3usb30532_node,
 	&displayport_node,
 	&usb_connector_node,
+	&altmodes_node,
+	&dp_altmode_node,
 	NULL
 };
 
-- 
2.26.2

Re: PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes

[Prashant Malani]

Hi Hans,

Sorry for posting on an old thread, but I was wondering if there was
still a plan to submit this? This is something we'd like to use on
Chrome OS too.

It sounded like the primary discussion was whether to have an "altmodes"
property encaspulating the various alt modes, but not sure what the
final consensus on that was (sounded to me like your current
implementation was fine for now, and ACPI use cases would be handled
later?).

Best regards,

-Prashant

On Tue, Jul 14, 2020 at 01:36:13PM +0200, Hans de Goede wrote:
> Hi All,
> 
> This is a replacement series for an earlier attempt by me for this
> from quite a while ago:
> 
> https://patchwork.kernel.org/patch/11199517/
> 
> As discussed there, this series implements an altmodes devicetree-fwnode
> under the usb-connector node which has 1 child-node per supported
> altmode and in that child-node the svid and vdo for the supported
> altmode are specified.
> 
> Note this patch-set does not contain any devicetree users of the
> new bindings. The new support/binding is used on X86 Cherry Trail
> devices with a fusb302 Type-C controller (special variant of the
> INT33FE device in ACPI). But this patch should also help getting
> Display Port altmode to work with the mainline kernel on boards
> like the Pine RockPro64 and Pinebook Pro, which is why I've added
> Tobias Schramm to the Cc since he has done mainline devicetree
> work for the Pinebook Pro in the past.
> 
> The 1st patch adds the dt-bindings docs. I'm not sure if this one
> should go upstream through the USB tree together with patches 2-3 or
> if this should go upstream separately, Rob ?
> 
> Patches 2-3 add support for the new binding to Type-C controller drivers
> using the tcpm framework, such as the fusb302 driver.
> 
> Patch 4 uses swnodes to add the altmode info on the earlier mentioned
> X86 CHT devices, making DP-altmode work there for the first time.
> 
> Regards,
> 
> Hans
> 

Re: PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes

[Hans de Goede]

Hi Prashant,

On 12/2/21 20:29, Prashant Malani wrote:
> Hi Hans,
> 
> Sorry for posting on an old thread, but I was wondering if there was
> still a plan to submit this? This is something we'd like to use on
> Chrome OS too.
> 
> It sounded like the primary discussion was whether to have an "altmodes"
> property encaspulating the various alt modes, but not sure what the
> final consensus on that was (sounded to me like your current
> implementation was fine for now, and ACPI use cases would be handled
> later?).

Support for this has already landed, but so far has only been tested
on a x86/ACPI device, where the firmware-nodes parsed by the new
typec_port_register_altmodes() helper are setup through software-nodes,
see:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b458a4c5d7302947556e12c83cfe4da769665d0
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=55d8b34772e0728a224198ba605eed8cfc570aa0
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3d28466e5f4f8

In theory this should be usable for devicetree as is. But that would
require documenting the current in kernel swnode bindings as
official devicetree bindings and getting that through the devicetree
bindings review process.

I have deliberately not done this because the devicetree maintainers
have asked for properties / swnode "bindings" used only inside
the kernel (1) to NOT be documented as official devicetree bindings,
they (the dt bindings maintainers) want to first see at least one
real devicetree users before adding things like this to the
official devicetree bindings docs.

Note if the way typec_port_register_altmodes() parses the fwnode
properties needs to change as result of the devicetree bindings review
process, please let me know, because then the swnode-s created in
drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c
need to change to match so as to not regress things on those devices.

Regards,

Hans


1) between different kernel parts, as platform_data equivalent really





 Tue, Jul 14, 2020 at 01:36:13PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> This is a replacement series for an earlier attempt by me for this
>> from quite a while ago:
>>
>> https://patchwork.kernel.org/patch/11199517/
>>
>> As discussed there, this series implements an altmodes devicetree-fwnode
>> under the usb-connector node which has 1 child-node per supported
>> altmode and in that child-node the svid and vdo for the supported
>> altmode are specified.
>>
>> Note this patch-set does not contain any devicetree users of the
>> new bindings. The new support/binding is used on X86 Cherry Trail
>> devices with a fusb302 Type-C controller (special variant of the
>> INT33FE device in ACPI). But this patch should also help getting
>> Display Port altmode to work with the mainline kernel on boards
>> like the Pine RockPro64 and Pinebook Pro, which is why I've added
>> Tobias Schramm to the Cc since he has done mainline devicetree
>> work for the Pinebook Pro in the past.
>>
>> The 1st patch adds the dt-bindings docs. I'm not sure if this one
>> should go upstream through the USB tree together with patches 2-3 or
>> if this should go upstream separately, Rob ?
>>
>> Patches 2-3 add support for the new binding to Type-C controller drivers
>> using the tcpm framework, such as the fusb302 driver.
>>
>> Patch 4 uses swnodes to add the altmode info on the earlier mentioned
>> X86 CHT devices, making DP-altmode work there for the first time.
>>
>> Regards,
>>
>> Hans
>>
> 

Re: PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes

[Prashant Malani]

Hi Hans,

On Fri, Dec 3, 2021 at 2:14 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Prashant,
>
> On 12/2/21 20:29, Prashant Malani wrote:
> > Hi Hans,
> >
> > Sorry for posting on an old thread, but I was wondering if there was
> > still a plan to submit this? This is something we'd like to use on
> > Chrome OS too.
> >
> > It sounded like the primary discussion was whether to have an "altmodes"
> > property encaspulating the various alt modes, but not sure what the
> > final consensus on that was (sounded to me like your current
> > implementation was fine for now, and ACPI use cases would be handled
> > later?).
>
> Support for this has already landed, but so far has only been tested
> on a x86/ACPI device, where the firmware-nodes parsed by the new
> typec_port_register_altmodes() helper are setup through software-nodes,
> see:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b458a4c5d7302947556e12c83cfe4da769665d0
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=55d8b34772e0728a224198ba605eed8cfc570aa0
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3d28466e5f4f8
>
> In theory this should be usable for devicetree as is. But that would
> require documenting the current in kernel swnode bindings as
> official devicetree bindings and getting that through the devicetree
> bindings review process.

That's good to hear :)

>
> I have deliberately not done this because the devicetree maintainers
> have asked for properties / swnode "bindings" used only inside
> the kernel (1) to NOT be documented as official devicetree bindings,
> they (the dt bindings maintainers) want to first see at least one
> real devicetree users before adding things like this to the
> official devicetree bindings docs.
>
> Note if the way typec_port_register_altmodes() parses the fwnode
> properties needs to change as result of the devicetree bindings review
> process, please let me know, because then the swnode-s created in
> drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c
> need to change to match so as to not regress things on those devices.

Heikki, can we reconcile this with the format you had in mind for ACPI
devices which specify this in ASL files?
If not, would you rather:
1. We handle the actual ACPI format differently in
typec_port_register_altmodes() itself ? If so, is it documented
in some place that we can reference?
<or>
2. Parse whatever ACPI format there is in the port drivers probe
function, and then inject software nodes like in the int33fe driver
[1]
<or>
3. Is there scope for updating the ACPI altmode descriptor format to
match what is being parsed by typec_port_register_altmodes()

Thanks,

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c#n221

Re: PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes

[Heikki Krogerus]

Hi,

On Fri, Dec 03, 2021 at 12:22:35PM -0800, Prashant Malani wrote:
> Hi Hans,
> 
> On Fri, Dec 3, 2021 at 2:14 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi Prashant,
> >
> > On 12/2/21 20:29, Prashant Malani wrote:
> > > Hi Hans,
> > >
> > > Sorry for posting on an old thread, but I was wondering if there was
> > > still a plan to submit this? This is something we'd like to use on
> > > Chrome OS too.
> > >
> > > It sounded like the primary discussion was whether to have an "altmodes"
> > > property encaspulating the various alt modes, but not sure what the
> > > final consensus on that was (sounded to me like your current
> > > implementation was fine for now, and ACPI use cases would be handled
> > > later?).
> >
> > Support for this has already landed, but so far has only been tested
> > on a x86/ACPI device, where the firmware-nodes parsed by the new
> > typec_port_register_altmodes() helper are setup through software-nodes,
> > see:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b458a4c5d7302947556e12c83cfe4da769665d0
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=55d8b34772e0728a224198ba605eed8cfc570aa0
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3d28466e5f4f8
> >
> > In theory this should be usable for devicetree as is. But that would
> > require documenting the current in kernel swnode bindings as
> > official devicetree bindings and getting that through the devicetree
> > bindings review process.
> 
> That's good to hear :)
> 
> >
> > I have deliberately not done this because the devicetree maintainers
> > have asked for properties / swnode "bindings" used only inside
> > the kernel (1) to NOT be documented as official devicetree bindings,
> > they (the dt bindings maintainers) want to first see at least one
> > real devicetree users before adding things like this to the
> > official devicetree bindings docs.
> >
> > Note if the way typec_port_register_altmodes() parses the fwnode
> > properties needs to change as result of the devicetree bindings review
> > process, please let me know, because then the swnode-s created in
> > drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c
> > need to change to match so as to not regress things on those devices.
> 
> Heikki, can we reconcile this with the format you had in mind for ACPI
> devices which specify this in ASL files?

I don't know. I'm not sure what are the changes that need to be made
in order to fit this thing into the devicetree bindings (or are there
any)?

Assuming that the proposal is still that each connector device node
would have a sub-node "altmodes" which then has a child node for each
supported alt mode, then the ASL for the first USB Type-C port (as an
example) should look roughly like this (this is prepared on top the
ACPI tables from Intel Tigerlake based Chromebook system):

        Scope (\_SB.H_EC.USBC.CON0)
        {
                Name (_DSD, Package () {
                        ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                        Package () {
                                Package () { "altmodes", "ALT0" },
                        }
                })

                /* The "altmodes" sub-node */
                Name (ALT0, Package () {
                        ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                        Package () {
                                Package () { "tbt", "ALT1" },
                                Package () { "dp", "ALT2" },
                        }
                })

                /* Thunderbolt 3 Alternate Mode */
                Name (ALT1, Package() {
                        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                        Package () {
                                Package () { "svid", 0x8087 },
                                Package () { "vdo", 0x00000001 },
                        },
                })

                /* DisplayPort Alternate Mode */
                Name (ALT2, Package() {
                        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                        Package () {
                                Package () { "svid", 0xff01 },
                                Package () { "vdo", 0x001c1c47 },
                        },
                })
        }

So with that, this series should work as is. Let me know if you need
me to explain that in more detail. The Hierarchical Data Extension
_DSD UUID is documented here:
https://uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.1.pdf

But as said, I'm now not sure what the final design should look like?

The ACPI format we can in any case quite likely make work with what
ever requirements/limitation the devicetree has. We just need to
understand what those are.

After that I would really like to see the format documented for
ACPI. Though, I'm not sure where should it be documented. I think we
are talking about some kind of BIOS writing guide or similar.

thanks,

-- 
heikki

Re: PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes

[Hans de Goede]

Hi,

On 12/7/21 10:56, Heikki Krogerus wrote:
> Hi,
> 
> On Fri, Dec 03, 2021 at 12:22:35PM -0800, Prashant Malani wrote:
>> Hi Hans,
>>
>> On Fri, Dec 3, 2021 at 2:14 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi Prashant,
>>>
>>> On 12/2/21 20:29, Prashant Malani wrote:
>>>> Hi Hans,
>>>>
>>>> Sorry for posting on an old thread, but I was wondering if there was
>>>> still a plan to submit this? This is something we'd like to use on
>>>> Chrome OS too.
>>>>
>>>> It sounded like the primary discussion was whether to have an "altmodes"
>>>> property encaspulating the various alt modes, but not sure what the
>>>> final consensus on that was (sounded to me like your current
>>>> implementation was fine for now, and ACPI use cases would be handled
>>>> later?).
>>>
>>> Support for this has already landed, but so far has only been tested
>>> on a x86/ACPI device, where the firmware-nodes parsed by the new
>>> typec_port_register_altmodes() helper are setup through software-nodes,
>>> see:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b458a4c5d7302947556e12c83cfe4da769665d0
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=55d8b34772e0728a224198ba605eed8cfc570aa0
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3d28466e5f4f8
>>>
>>> In theory this should be usable for devicetree as is. But that would
>>> require documenting the current in kernel swnode bindings as
>>> official devicetree bindings and getting that through the devicetree
>>> bindings review process.
>>
>> That's good to hear :)
>>
>>>
>>> I have deliberately not done this because the devicetree maintainers
>>> have asked for properties / swnode "bindings" used only inside
>>> the kernel (1) to NOT be documented as official devicetree bindings,
>>> they (the dt bindings maintainers) want to first see at least one
>>> real devicetree users before adding things like this to the
>>> official devicetree bindings docs.
>>>
>>> Note if the way typec_port_register_altmodes() parses the fwnode
>>> properties needs to change as result of the devicetree bindings review
>>> process, please let me know, because then the swnode-s created in
>>> drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c
>>> need to change to match so as to not regress things on those devices.
>>
>> Heikki, can we reconcile this with the format you had in mind for ACPI
>> devices which specify this in ASL files?
> 
> I don't know. I'm not sure what are the changes that need to be made
> in order to fit this thing into the devicetree bindings (or are there
> any)?
> 
> Assuming that the proposal is still that each connector device node
> would have a sub-node "altmodes" which then has a child node for each
> supported alt mode,

Right, this is the format that the current implementation of
typec_port_register_altmodes() expects.

Regards,

Hans


> then the ASL for the first USB Type-C port (as an
> example) should look roughly like this (this is prepared on top the
> ACPI tables from Intel Tigerlake based Chromebook system):
> 
>         Scope (\_SB.H_EC.USBC.CON0)
>         {
>                 Name (_DSD, Package () {
>                         ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>                         Package () {
>                                 Package () { "altmodes", "ALT0" },
>                         }
>                 })
> 
>                 /* The "altmodes" sub-node */
>                 Name (ALT0, Package () {
>                         ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>                         Package () {
>                                 Package () { "tbt", "ALT1" },
>                                 Package () { "dp", "ALT2" },
>                         }
>                 })
> 
>                 /* Thunderbolt 3 Alternate Mode */
>                 Name (ALT1, Package() {
>                         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                         Package () {
>                                 Package () { "svid", 0x8087 },
>                                 Package () { "vdo", 0x00000001 },
>                         },
>                 })
> 
>                 /* DisplayPort Alternate Mode */
>                 Name (ALT2, Package() {
>                         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                         Package () {
>                                 Package () { "svid", 0xff01 },
>                                 Package () { "vdo", 0x001c1c47 },
>                         },
>                 })
>         }
> 
> So with that, this series should work as is. Let me know if you need
> me to explain that in more detail. The Hierarchical Data Extension
> _DSD UUID is documented here:
> https://uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.1.pdf
> 
> But as said, I'm now not sure what the final design should look like?
> 
> The ACPI format we can in any case quite likely make work with what
> ever requirements/limitation the devicetree has. We just need to
> understand what those are.
> 
> After that I would really like to see the format documented for
> ACPI. Though, I'm not sure where should it be documented. I think we
> are talking about some kind of BIOS writing guide or similar.
> 
> thanks,
>