Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes
@ 2020-07-14 11:36 Hans de Goede
  2020-07-14 11:36 ` [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes Hans de Goede
                   ` (3 more replies)
  0 siblings, 4 replies; 22+ messages in thread
From: Hans de Goede @ 2020-07-14 11:36 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus, Rob Herring
  Cc: Hans de Goede, Tobias Schramm, linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes
  2020-07-14 11:36 PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes Hans de Goede
@ 2020-07-14 11:36 ` Hans de Goede
  2020-07-21  2:26   ` Rob Herring
  2020-07-14 11:36 ` [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode() Hans de Goede
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 22+ messages in thread
From: Hans de Goede @ 2020-07-14 11:36 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus, Rob Herring
  Cc: Hans de Goede, Tobias Schramm, linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-07-14 11:36 PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes Hans de Goede
  2020-07-14 11:36 ` [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes Hans de Goede
@ 2020-07-14 11:36 ` Hans de Goede
  2020-07-15 16:39   ` Guenter Roeck
                     ` (2 more replies)
  2020-07-14 11:36 ` [PATCH 3/4] usb: typec: tcpm: Add support for altmodes Hans de Goede
  2020-07-14 11:36 ` [PATCH 4/4] platform/x86/intel_cht_int33fe: Add displayport altmode fwnode to the connector fwnode Hans de Goede
  3 siblings, 3 replies; 22+ messages in thread
From: Hans de Goede @ 2020-07-14 11:36 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus, Rob Herring
  Cc: Hans de Goede, Tobias Schramm, linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/4] usb: typec: tcpm: Add support for altmodes
  2020-07-14 11:36 PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes Hans de Goede
  2020-07-14 11:36 ` [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes Hans de Goede
  2020-07-14 11:36 ` [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode() Hans de Goede
@ 2020-07-14 11:36 ` Hans de Goede
  2020-07-15 16:41   ` Guenter Roeck
  2020-07-14 11:36 ` [PATCH 4/4] platform/x86/intel_cht_int33fe: Add displayport altmode fwnode to the connector fwnode Hans de Goede
  3 siblings, 1 reply; 22+ messages in thread
From: Hans de Goede @ 2020-07-14 11:36 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus, Rob Herring
  Cc: Hans de Goede, Tobias Schramm, linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 4/4] platform/x86/intel_cht_int33fe: Add displayport altmode fwnode to the connector fwnode
  2020-07-14 11:36 PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes Hans de Goede
                   ` (2 preceding siblings ...)
  2020-07-14 11:36 ` [PATCH 3/4] usb: typec: tcpm: Add support for altmodes Hans de Goede
@ 2020-07-14 11:36 ` Hans de Goede
  3 siblings, 0 replies; 22+ messages in thread
From: Hans de Goede @ 2020-07-14 11:36 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus, Rob Herring
  Cc: Hans de Goede, Tobias Schramm, linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-07-14 11:36 ` [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode() Hans de Goede
@ 2020-07-15 16:39   ` Guenter Roeck
  2020-07-15 21:14     ` Hans de Goede
  2020-07-27 13:05   ` Heikki Krogerus
  2020-08-26 13:17   ` Heikki Krogerus
  2 siblings, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2020-07-15 16:39 UTC (permalink / raw)
  To: Hans de Goede, Greg Kroah-Hartman, Heikki Krogerus, Rob Herring
  Cc: Tobias Schramm, linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/4] usb: typec: tcpm: Add support for altmodes
  2020-07-14 11:36 ` [PATCH 3/4] usb: typec: tcpm: Add support for altmodes Hans de Goede
@ 2020-07-15 16:41   ` Guenter Roeck
  0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2020-07-15 16:41 UTC (permalink / raw)
  To: Hans de Goede, Greg Kroah-Hartman, Heikki Krogerus, Rob Herring
  Cc: Tobias Schramm, linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-07-15 16:39   ` Guenter Roeck
@ 2020-07-15 21:14     ` Hans de Goede
  2020-07-16  0:01       ` Guenter Roeck
  0 siblings, 1 reply; 22+ messages in thread
From: Hans de Goede @ 2020-07-15 21:14 UTC (permalink / raw)
  To: Guenter Roeck, Greg Kroah-Hartman, Heikki Krogerus, Rob Herring
  Cc: Tobias Schramm, linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-07-15 21:14     ` Hans de Goede
@ 2020-07-16  0:01       ` Guenter Roeck
  0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2020-07-16  0:01 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Greg Kroah-Hartman, Heikki Krogerus, Rob Herring, Tobias Schramm,
	linux-usb, devicetree

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes
  2020-07-14 11:36 ` [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes Hans de Goede
@ 2020-07-21  2:26   ` Rob Herring
  2020-07-21  5:49     ` Prashant Malani
  2020-07-22  7:18     ` Hans de Goede
  0 siblings, 2 replies; 22+ messages in thread
From: Rob Herring @ 2020-07-21  2:26 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus,
	Tobias Schramm, linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes
  2020-07-21  2:26   ` Rob Herring
@ 2020-07-21  5:49     ` Prashant Malani
  2020-07-22  7:18     ` Hans de Goede
  1 sibling, 0 replies; 22+ messages in thread
From: Prashant Malani @ 2020-07-21  5:49 UTC (permalink / raw)
  To: Rob Herring
  Cc: Hans de Goede, Greg Kroah-Hartman, Guenter Roeck,
	Heikki Krogerus, Tobias Schramm,
	open list:USB NETWORKING DRIVERS,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

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
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes
  2020-07-21  2:26   ` Rob Herring
  2020-07-21  5:49     ` Prashant Malani
@ 2020-07-22  7:18     ` Hans de Goede
  1 sibling, 0 replies; 22+ messages in thread
From: Hans de Goede @ 2020-07-22  7:18 UTC (permalink / raw)
  To: Rob Herring
  Cc: Greg Kroah-Hartman, Guenter Roeck, Heikki Krogerus,
	Tobias Schramm, linux-usb, devicetree

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
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-07-14 11:36 ` [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode() Hans de Goede
  2020-07-15 16:39   ` Guenter Roeck
@ 2020-07-27 13:05   ` Heikki Krogerus
  2020-08-10  7:19     ` Hans de Goede
  2020-08-26 13:17   ` Heikki Krogerus
  2 siblings, 1 reply; 22+ messages in thread
From: Heikki Krogerus @ 2020-07-27 13:05 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Greg Kroah-Hartman, Guenter Roeck, Rob Herring, Tobias Schramm,
	linux-usb, devicetree

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-07-27 13:05   ` Heikki Krogerus
@ 2020-08-10  7:19     ` Hans de Goede
  2020-08-11 14:38       ` Heikki Krogerus
  0 siblings, 1 reply; 22+ messages in thread
From: Hans de Goede @ 2020-08-10  7:19 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Greg Kroah-Hartman, Guenter Roeck, Rob Herring, Tobias Schramm,
	linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-08-10  7:19     ` Hans de Goede
@ 2020-08-11 14:38       ` Heikki Krogerus
  2020-08-12  8:36         ` Hans de Goede
  0 siblings, 1 reply; 22+ messages in thread
From: Heikki Krogerus @ 2020-08-11 14:38 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Greg Kroah-Hartman, Guenter Roeck, Rob Herring, Tobias Schramm,
	linux-usb, devicetree

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-08-11 14:38       ` Heikki Krogerus
@ 2020-08-12  8:36         ` Hans de Goede
  2020-08-12 12:49           ` Heikki Krogerus
  0 siblings, 1 reply; 22+ messages in thread
From: Hans de Goede @ 2020-08-12  8:36 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Greg Kroah-Hartman, Guenter Roeck, Rob Herring, Tobias Schramm,
	linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-08-12  8:36         ` Hans de Goede
@ 2020-08-12 12:49           ` Heikki Krogerus
  2020-08-13 14:30             ` Hans de Goede
  2020-08-26 12:37             ` Hans de Goede
  0 siblings, 2 replies; 22+ messages in thread
From: Heikki Krogerus @ 2020-08-12 12:49 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Greg Kroah-Hartman, Guenter Roeck, Rob Herring, Tobias Schramm,
	linux-usb, devicetree

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-08-12 12:49           ` Heikki Krogerus
@ 2020-08-13 14:30             ` Hans de Goede
  2020-08-26 12:37             ` Hans de Goede
  1 sibling, 0 replies; 22+ messages in thread
From: Hans de Goede @ 2020-08-13 14:30 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Greg Kroah-Hartman, Guenter Roeck, Rob Herring, Tobias Schramm,
	linux-usb, devicetree

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.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-08-12 12:49           ` Heikki Krogerus
  2020-08-13 14:30             ` Hans de Goede
@ 2020-08-26 12:37             ` Hans de Goede
  2020-08-26 13:06               ` Heikki Krogerus
  1 sibling, 1 reply; 22+ messages in thread
From: Hans de Goede @ 2020-08-26 12:37 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Greg Kroah-Hartman, Guenter Roeck, Rob Herring, Tobias Schramm,
	linux-usb, devicetree

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-08-26 12:37             ` Hans de Goede
@ 2020-08-26 13:06               ` Heikki Krogerus
  0 siblings, 0 replies; 22+ messages in thread
From: Heikki Krogerus @ 2020-08-26 13:06 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Greg Kroah-Hartman, Guenter Roeck, Rob Herring, Tobias Schramm,
	linux-usb, devicetree

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-07-14 11:36 ` [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode() Hans de Goede
  2020-07-15 16:39   ` Guenter Roeck
  2020-07-27 13:05   ` Heikki Krogerus
@ 2020-08-26 13:17   ` Heikki Krogerus
  2021-04-08 18:59     ` Hans de Goede
  2 siblings, 1 reply; 22+ messages in thread
From: Heikki Krogerus @ 2020-08-26 13:17 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Greg Kroah-Hartman, Guenter Roeck, Rob Herring, Tobias Schramm,
	linux-usb, devicetree

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()
  2020-08-26 13:17   ` Heikki Krogerus
@ 2021-04-08 18:59     ` Hans de Goede
  0 siblings, 0 replies; 22+ messages in thread
From: Hans de Goede @ 2021-04-08 18:59 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Greg Kroah-Hartman, Guenter Roeck, Rob Herring, Tobias Schramm,
	linux-usb, devicetree


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


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, back to index

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-14 11:36 PATCH 0/4] usbd: typec: fusb302: Add support for specifying supported alternate-modes through devicetree/fwnodes Hans de Goede
2020-07-14 11:36 ` [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes Hans de Goede
2020-07-21  2:26   ` Rob Herring
2020-07-21  5:49     ` Prashant Malani
2020-07-22  7:18     ` Hans de Goede
2020-07-14 11:36 ` [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode() Hans de Goede
2020-07-15 16:39   ` Guenter Roeck
2020-07-15 21:14     ` Hans de Goede
2020-07-16  0:01       ` Guenter Roeck
2020-07-27 13:05   ` Heikki Krogerus
2020-08-10  7:19     ` Hans de Goede
2020-08-11 14:38       ` Heikki Krogerus
2020-08-12  8:36         ` Hans de Goede
2020-08-12 12:49           ` Heikki Krogerus
2020-08-13 14:30             ` Hans de Goede
2020-08-26 12:37             ` Hans de Goede
2020-08-26 13:06               ` Heikki Krogerus
2020-08-26 13:17   ` Heikki Krogerus
2021-04-08 18:59     ` Hans de Goede
2020-07-14 11:36 ` [PATCH 3/4] usb: typec: tcpm: Add support for altmodes Hans de Goede
2020-07-15 16:41   ` Guenter Roeck
2020-07-14 11:36 ` [PATCH 4/4] platform/x86/intel_cht_int33fe: Add displayport altmode fwnode to the connector fwnode Hans de Goede

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git