[RFC PATCH 0/3] usb: typec: Product Type time

[RFC PATCH 0/3] usb: typec: Product Type time

[Heikki Krogerus]

Hi Prashant,

The original discussion [1].

This proposal is in practice a compromise. I came to the conclusion
that we probable should expose the product type separately after all.
The reason for that is because we may in some cases actually know the
product type even when we don't have access to the Discover Identity
response. UCSI for example in practice gives us at least the cable
product type even though it does not let us know the response to the
Discover Identity command.

So my proposal here is that we add an attribute for the product type
itself, showing the product type as a string. Then we also add the
attribute for the product type specific VDOs which we place under the
identity directory more or less the way you originally proposed.

Note. I have not tested these at all.

[1] https://lore.kernel.org/linux-usb/20201023214328.1262883-2-pmalani@chromium.org/

Heikki Krogerus (2):
  usb: pd: DFP product types
  usb: typec: Add product_type sysfs attribute file for partners and
    cables

Prashant Malani (1):
  usb: typec: Expose Product Type VDOs via sysfs

 Documentation/ABI/testing/sysfs-class-typec |  55 +++++++
 drivers/usb/typec/class.c                   | 173 +++++++++++++++++++-
 include/linux/usb/pd_vdo.h                  |  16 +-
 3 files changed, 234 insertions(+), 10 deletions(-)

-- 
2.29.2

[RFC PATCH 1/3] usb: pd: DFP product types

[Heikki Krogerus]

USB Power Delivery Specification R3.0 introduced separate
field for the DFP product type to the ID Header VDO.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 include/linux/usb/pd_vdo.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/linux/usb/pd_vdo.h b/include/linux/usb/pd_vdo.h
index 68bdc4e2f5a90..704aaf826c5b4 100644
--- a/include/linux/usb/pd_vdo.h
+++ b/include/linux/usb/pd_vdo.h
@@ -103,17 +103,26 @@
  * --------------------
  * <31>     :: data capable as a USB host
  * <30>     :: data capable as a USB device
- * <29:27>  :: product type
+ * <29:27>  :: product type (UFP / Cable)
  * <26>     :: modal operation supported (1b == yes)
- * <25:16>  :: Reserved, Shall be set to zero
+ * <25:16>  :: product type (DFP)
  * <15:0>   :: USB-IF assigned VID for this cable vendor
  */
 #define IDH_PTYPE_UNDEF		0
 #define IDH_PTYPE_HUB		1
 #define IDH_PTYPE_PERIPH	2
+#define IDH_PTYPE_PSD		3
+#define IDH_PTYPE_AMA		5
+#define IDH_PTYPE_VPD		6
+
 #define IDH_PTYPE_PCABLE	3
 #define IDH_PTYPE_ACABLE	4
-#define IDH_PTYPE_AMA		5
+
+#define IDH_PTYPE_DFP_UNDEF	0
+#define IDH_PTYPE_DFP_HUB	1
+#define IDH_PTYPE_DFP_HOST	2
+#define IDH_PTYPE_DFP_PB	3
+#define IDH_PTYPE_DFP_AMC	4
 
 #define VDO_IDH(usbh, usbd, ptype, is_modal, vid)		\
 	((usbh) << 31 | (usbd) << 30 | ((ptype) & 0x7) << 27	\
@@ -122,6 +131,7 @@
 #define PD_IDH_PTYPE(vdo)	(((vdo) >> 27) & 0x7)
 #define PD_IDH_VID(vdo)		((vdo) & 0xffff)
 #define PD_IDH_MODAL_SUPP(vdo)	((vdo) & (1 << 26))
+#define PD_IDH_DFP_PTYPE(vdo)	(((vdo) >> 23) & 0x7)
 
 /*
  * Cert Stat VDO
-- 
2.29.2

[RFC PATCH 2/3] usb: typec: Add product_type sysfs attribute file for partners and cables

[Heikki Krogerus]

USB Power Delivery Specification defines a set of product
types for partners and cables. The product type is defined
in the ID Header VDO, which is the first object in the
response to the Discover Identity command.

This sysfs attribute file is only created for the partners
and cables if the product type is really known in the
driver. Some interfaces do not give access to the Discover
Identity response from the partner or cable, but they may
still supply the product type separately in some cases.

When the product type of the partner or cable is detected,
uevent is also raised with PRODUCT_TYPE set to show the
actual product type (for example PRODUCT_TYPE=host).

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-class-typec |  55 ++++++++
 drivers/usb/typec/class.c                   | 132 ++++++++++++++++++--
 2 files changed, 180 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
index b7794e02ad205..4c09e327c62be 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -139,6 +139,42 @@ Description:
 		Shows if the partner supports USB Power Delivery communication:
 		Valid values: yes, no
 
+What:		/sys/class/typec/<port>-partner/product_type
+Date:		December 2020
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:	USB Power Delivery Specification defines a set of product types
+		for the partner devices. This file will show the product type of
+		the partner if it is known. Dual-role capable partners will have
+		both UFP and DFP product types defined, but only one that
+		matches the current role will be active at the time. If the
+		product type of the partner is not visible to the device driver,
+		this file will not exist.
+
+		When the partner product type is detected, or changed with role
+		swap, uvevent is also raised that contains PRODUCT_TYPE=<product
+		type> (for example PRODUCT_TYPE=hub).
+
+		Valid values:
+
+		UFP / device role
+		========================  ==========================
+		undefined		  -
+		hub			  PDUSB Hub
+		peripheral		  PDUSB Peripheral
+		psd			  Power Bank
+		ama			  Alternate Mode Adapter
+		vpd			  VCONN Powered USB Device
+		========================  ==========================
+
+		DFP / host role
+		========================  ==========================
+		undefined		  -
+		hub			  PDUSB Hub
+		host			  PDUSB Host
+		power_brick		  Power Brick
+		amc			  Alternate Mode Controller
+		========================  ==========================
+
 What:		/sys/class/typec/<port>-partner>/identity/
 Date:		April 2017
 Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
@@ -202,6 +238,25 @@ Description:
 		- type-c
 		- captive
 
+What:		/sys/class/typec/<port>-cable/product_type
+Date:		December 2020
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:	USB Power Delivery Specification defines a set of product types
+		for the cables. This file will show the product type of the
+		cable if it is known. If the product type of the cable is not
+		visible to the device driver, this file will not exist.
+
+		When the cable product type is detected, uvevent is also raised
+		with PRODUCT_TYPE showing the product type of the cable.
+
+		Valid values:
+
+		========================  ==========================
+		undefined		  -
+		active			  Active Cable
+		passive			  Passive Cable
+		========================  ==========================
+
 What:		/sys/class/typec/<port>-cable/identity/
 Date:		April 2017
 Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 35eec707cb512..303f054181ff7 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -11,6 +11,7 @@
 #include <linux/mutex.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include <linux/usb/pd_vdo.h>
 
 #include "bus.h"
 
@@ -81,6 +82,30 @@ static const char * const typec_accessory_modes[] = {
 	[TYPEC_ACCESSORY_DEBUG]		= "debug",
 };
 
+/* Product types defined in USB PD Specification R3.0 V2.0 */
+static const char * const product_type_ufp[8] = {
+	[IDH_PTYPE_UNDEF]		= "undefined",
+	[IDH_PTYPE_HUB]			= "hub",
+	[IDH_PTYPE_PERIPH]		= "peripheral",
+	[IDH_PTYPE_PSD]			= "psd",
+	[IDH_PTYPE_AMA]			= "ama",
+	[IDH_PTYPE_VPD]			= "vpd",
+};
+
+static const char * const product_type_dfp[8] = {
+	[IDH_PTYPE_DFP_UNDEF]		= "undefined",
+	[IDH_PTYPE_DFP_HUB]		= "hub",
+	[IDH_PTYPE_DFP_HOST]		= "host",
+	[IDH_PTYPE_DFP_PB]		= "power_brick",
+	[IDH_PTYPE_DFP_AMC]		= "amc",
+};
+
+static const char * const product_type_cable[8] = {
+	[IDH_PTYPE_UNDEF]		= "undefined",
+	[IDH_PTYPE_PCABLE]		= "passive",
+	[IDH_PTYPE_ACABLE]		= "active",
+};
+
 static struct usb_pd_identity *get_pd_identity(struct device *dev)
 {
 	if (is_typec_partner(dev)) {
@@ -95,6 +120,24 @@ static struct usb_pd_identity *get_pd_identity(struct device *dev)
 	return NULL;
 }
 
+static const char *get_pd_product_type(struct device *dev)
+{
+	struct typec_port *port = to_typec_port(dev->parent);
+	struct usb_pd_identity *id = get_pd_identity(dev);
+	const char *ptype = NULL;
+
+	if (is_typec_partner(dev)) {
+		if (port->data_role == TYPEC_HOST)
+			ptype = product_type_ufp[PD_IDH_PTYPE(id->id_header)];
+		else
+			ptype = product_type_dfp[PD_IDH_DFP_PTYPE(id->id_header)];
+	} else if (is_typec_cable(dev)) {
+		ptype = product_type_cable[PD_IDH_PTYPE(id->id_header)];
+	}
+
+	return ptype;
+}
+
 static ssize_t id_header_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
@@ -139,11 +182,55 @@ static const struct attribute_group *usb_pd_id_groups[] = {
 	NULL,
 };
 
+static void typec_product_type_notify(struct device *dev)
+{
+	const char *ptype;
+	char *envp[2];
+
+	ptype = get_pd_product_type(dev);
+	if (!ptype)
+		return;
+
+	sysfs_notify(&dev->kobj, NULL, "product_type");
+
+	envp[0] = kasprintf(GFP_KERNEL, "PRODUCT_TYPE=%s", ptype);
+	if (!envp[0])
+		return;
+
+	envp[1] = NULL;
+	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
+	kfree(envp[0]);
+}
+
 static void typec_report_identity(struct device *dev)
 {
 	sysfs_notify(&dev->kobj, "identity", "id_header");
 	sysfs_notify(&dev->kobj, "identity", "cert_stat");
 	sysfs_notify(&dev->kobj, "identity", "product");
+	typec_product_type_notify(dev);
+}
+
+static ssize_t
+product_type_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	const char *ptype;
+
+	ptype = get_pd_product_type(dev);
+	if (!ptype)
+		return 0;
+
+	return sysfs_emit(buf, "%s\n", ptype);
+}
+static DEVICE_ATTR_RO(product_type);
+
+static umode_t typec_product_type_attr_is_visible(struct kobject *kobj,
+						  struct attribute *attr, int n)
+{
+	if (attr == &dev_attr_product_type.attr)
+		if (!get_pd_identity(kobj_to_dev(kobj)))
+			return 0;
+
+	return attr->mode;
 }
 
 /* ------------------------------------------------------------------------- */
@@ -534,10 +621,20 @@ static DEVICE_ATTR_RO(supports_usb_power_delivery);
 
 static struct attribute *typec_partner_attrs[] = {
 	&dev_attr_accessory_mode.attr,
+	&dev_attr_product_type.attr,
 	&dev_attr_supports_usb_power_delivery.attr,
 	NULL
 };
-ATTRIBUTE_GROUPS(typec_partner);
+
+static struct attribute_group typec_partner_group = {
+	.is_visible = typec_product_type_attr_is_visible,
+	.attrs = typec_partner_attrs,
+};
+
+static const struct attribute_group *typec_partner_groups[] = {
+	&typec_partner_group,
+	NULL
+};
 
 static void typec_partner_release(struct device *dev)
 {
@@ -773,9 +870,19 @@ static DEVICE_ATTR_RO(plug_type);
 static struct attribute *typec_cable_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_plug_type.attr,
+	&dev_attr_product_type.attr,
+	NULL
+};
+
+static struct attribute_group typec_cable_group = {
+	.is_visible = typec_product_type_attr_is_visible,
+	.attrs = typec_cable_attrs,
+};
+
+static const struct attribute_group *typec_cable_groups[] = {
+	&typec_cable_group,
 	NULL
 };
-ATTRIBUTE_GROUPS(typec_cable);
 
 static void typec_cable_release(struct device *dev)
 {
@@ -1352,6 +1459,11 @@ const struct device_type typec_port_dev_type = {
 /* --------------------------------------- */
 /* Driver callbacks to report role updates */
 
+static int partner_match(struct device *dev, void *data)
+{
+	return is_typec_partner(dev);
+}
+
 /**
  * typec_set_data_role - Report data role change
  * @port: The USB Type-C Port where the role was changed
@@ -1361,12 +1473,23 @@ const struct device_type typec_port_dev_type = {
  */
 void typec_set_data_role(struct typec_port *port, enum typec_data_role role)
 {
+	struct device *partner_dev;
+
 	if (port->data_role == role)
 		return;
 
 	port->data_role = role;
 	sysfs_notify(&port->dev.kobj, NULL, "data_role");
 	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
+
+	partner_dev = device_find_child(&port->dev, NULL, partner_match);
+	if (!partner_dev)
+		return;
+
+	if (to_typec_partner(partner_dev)->identity)
+		typec_product_type_notify(partner_dev);
+
+	put_device(partner_dev);
 }
 EXPORT_SYMBOL_GPL(typec_set_data_role);
 
@@ -1407,11 +1530,6 @@ void typec_set_vconn_role(struct typec_port *port, enum typec_role role)
 }
 EXPORT_SYMBOL_GPL(typec_set_vconn_role);
 
-static int partner_match(struct device *dev, void *data)
-{
-	return is_typec_partner(dev);
-}
-
 /**
  * typec_set_pwr_opmode - Report changed power operation mode
  * @port: The USB Type-C Port where the mode was changed
-- 
2.29.2

Re: [RFC PATCH 2/3] usb: typec: Add product_type sysfs attribute file for partners and cables

[Greg Kroah-Hartman]

On Wed, Nov 18, 2020 at 06:00:58PM +0300, Heikki Krogerus wrote:
> USB Power Delivery Specification defines a set of product
> types for partners and cables. The product type is defined
> in the ID Header VDO, which is the first object in the
> response to the Discover Identity command.
> 
> This sysfs attribute file is only created for the partners
> and cables if the product type is really known in the
> driver. Some interfaces do not give access to the Discover
> Identity response from the partner or cable, but they may
> still supply the product type separately in some cases.
> 
> When the product type of the partner or cable is detected,
> uevent is also raised with PRODUCT_TYPE set to show the
> actual product type (for example PRODUCT_TYPE=host).
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  55 ++++++++
>  drivers/usb/typec/class.c                   | 132 ++++++++++++++++++--
>  2 files changed, 180 insertions(+), 7 deletions(-)

Seems semi-sane, nice work :)

greg k-h

Re: [RFC PATCH 2/3] usb: typec: Add product_type sysfs attribute file for partners and cables

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 11427 bytes --]

Hi Heikki,

On Wed, Nov 18, 2020 at 06:00:58PM +0300, Heikki Krogerus wrote:
> USB Power Delivery Specification defines a set of product
> types for partners and cables. The product type is defined
> in the ID Header VDO, which is the first object in the
> response to the Discover Identity command.
> 
> This sysfs attribute file is only created for the partners
> and cables if the product type is really known in the
> driver. Some interfaces do not give access to the Discover
> Identity response from the partner or cable, but they may
> still supply the product type separately in some cases.
> 
> When the product type of the partner or cable is detected,
> uevent is also raised with PRODUCT_TYPE set to show the
> actual product type (for example PRODUCT_TYPE=host).
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  55 ++++++++
>  drivers/usb/typec/class.c                   | 132 ++++++++++++++++++--
>  2 files changed, 180 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index b7794e02ad205..4c09e327c62be 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -139,6 +139,42 @@ Description:
>  		Shows if the partner supports USB Power Delivery communication:
>  		Valid values: yes, no
>  
> +What:		/sys/class/typec/<port>-partner/product_type
> +Date:		December 2020
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:	USB Power Delivery Specification defines a set of product types
> +		for the partner devices. This file will show the product type of
> +		the partner if it is known. Dual-role capable partners will have
> +		both UFP and DFP product types defined, but only one that
> +		matches the current role will be active at the time. If the
> +		product type of the partner is not visible to the device driver,
> +		this file will not exist.
> +
> +		When the partner product type is detected, or changed with role
> +		swap, uvevent is also raised that contains PRODUCT_TYPE=<product
> +		type> (for example PRODUCT_TYPE=hub).
> +
> +		Valid values:
> +
> +		UFP / device role
> +		========================  ==========================
> +		undefined		  -
> +		hub			  PDUSB Hub
> +		peripheral		  PDUSB Peripheral
> +		psd			  Power Bank
> +		ama			  Alternate Mode Adapter
> +		vpd			  VCONN Powered USB Device

I have it on good authority that "vpd" is incorrectly categorized here,
and for future proofing, we'd better not introduce vpd as a product
type for UFP...

A vpd is actually more closely related to a "cable" than it is a "UFP."
A closer reading of the USB Type-C and USB PD specs will reveal that
VPDs can only ever appear as SOP' and not as SOP, so having its type
appear under UFP is a mistake.

In other words, the USB PD V3.0 R2.0 spec is wrong. A change has been
working its way through the spec committee to fix this, but it is not yet
published.

In order to reduce the amount of churn, I would recommend not
including vpd as a possible type until a new version of the spec (or the ECN)
is published.

Thanks,
Benson

> +		========================  ==========================
> +
> +		DFP / host role
> +		========================  ==========================
> +		undefined		  -
> +		hub			  PDUSB Hub
> +		host			  PDUSB Host
> +		power_brick		  Power Brick
> +		amc			  Alternate Mode Controller
> +		========================  ==========================
> +
>  What:		/sys/class/typec/<port>-partner>/identity/
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> @@ -202,6 +238,25 @@ Description:
>  		- type-c
>  		- captive
>  
> +What:		/sys/class/typec/<port>-cable/product_type
> +Date:		December 2020
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:	USB Power Delivery Specification defines a set of product types
> +		for the cables. This file will show the product type of the
> +		cable if it is known. If the product type of the cable is not
> +		visible to the device driver, this file will not exist.
> +
> +		When the cable product type is detected, uvevent is also raised
> +		with PRODUCT_TYPE showing the product type of the cable.
> +
> +		Valid values:
> +
> +		========================  ==========================
> +		undefined		  -
> +		active			  Active Cable
> +		passive			  Passive Cable
> +		========================  ==========================
> +
>  What:		/sys/class/typec/<port>-cable/identity/
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 35eec707cb512..303f054181ff7 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -11,6 +11,7 @@
>  #include <linux/mutex.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
> +#include <linux/usb/pd_vdo.h>
>  
>  #include "bus.h"
>  
> @@ -81,6 +82,30 @@ static const char * const typec_accessory_modes[] = {
>  	[TYPEC_ACCESSORY_DEBUG]		= "debug",
>  };
>  
> +/* Product types defined in USB PD Specification R3.0 V2.0 */
> +static const char * const product_type_ufp[8] = {
> +	[IDH_PTYPE_UNDEF]		= "undefined",
> +	[IDH_PTYPE_HUB]			= "hub",
> +	[IDH_PTYPE_PERIPH]		= "peripheral",
> +	[IDH_PTYPE_PSD]			= "psd",
> +	[IDH_PTYPE_AMA]			= "ama",
> +	[IDH_PTYPE_VPD]			= "vpd",
> +};
> +
> +static const char * const product_type_dfp[8] = {
> +	[IDH_PTYPE_DFP_UNDEF]		= "undefined",
> +	[IDH_PTYPE_DFP_HUB]		= "hub",
> +	[IDH_PTYPE_DFP_HOST]		= "host",
> +	[IDH_PTYPE_DFP_PB]		= "power_brick",
> +	[IDH_PTYPE_DFP_AMC]		= "amc",
> +};
> +
> +static const char * const product_type_cable[8] = {
> +	[IDH_PTYPE_UNDEF]		= "undefined",
> +	[IDH_PTYPE_PCABLE]		= "passive",
> +	[IDH_PTYPE_ACABLE]		= "active",
> +};
> +
>  static struct usb_pd_identity *get_pd_identity(struct device *dev)
>  {
>  	if (is_typec_partner(dev)) {
> @@ -95,6 +120,24 @@ static struct usb_pd_identity *get_pd_identity(struct device *dev)
>  	return NULL;
>  }
>  
> +static const char *get_pd_product_type(struct device *dev)
> +{
> +	struct typec_port *port = to_typec_port(dev->parent);
> +	struct usb_pd_identity *id = get_pd_identity(dev);
> +	const char *ptype = NULL;
> +
> +	if (is_typec_partner(dev)) {
> +		if (port->data_role == TYPEC_HOST)
> +			ptype = product_type_ufp[PD_IDH_PTYPE(id->id_header)];
> +		else
> +			ptype = product_type_dfp[PD_IDH_DFP_PTYPE(id->id_header)];
> +	} else if (is_typec_cable(dev)) {
> +		ptype = product_type_cable[PD_IDH_PTYPE(id->id_header)];
> +	}
> +
> +	return ptype;
> +}
> +
>  static ssize_t id_header_show(struct device *dev, struct device_attribute *attr,
>  			      char *buf)
>  {
> @@ -139,11 +182,55 @@ static const struct attribute_group *usb_pd_id_groups[] = {
>  	NULL,
>  };
>  
> +static void typec_product_type_notify(struct device *dev)
> +{
> +	const char *ptype;
> +	char *envp[2];
> +
> +	ptype = get_pd_product_type(dev);
> +	if (!ptype)
> +		return;
> +
> +	sysfs_notify(&dev->kobj, NULL, "product_type");
> +
> +	envp[0] = kasprintf(GFP_KERNEL, "PRODUCT_TYPE=%s", ptype);
> +	if (!envp[0])
> +		return;
> +
> +	envp[1] = NULL;
> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> +	kfree(envp[0]);
> +}
> +
>  static void typec_report_identity(struct device *dev)
>  {
>  	sysfs_notify(&dev->kobj, "identity", "id_header");
>  	sysfs_notify(&dev->kobj, "identity", "cert_stat");
>  	sysfs_notify(&dev->kobj, "identity", "product");
> +	typec_product_type_notify(dev);
> +}
> +
> +static ssize_t
> +product_type_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	const char *ptype;
> +
> +	ptype = get_pd_product_type(dev);
> +	if (!ptype)
> +		return 0;
> +
> +	return sysfs_emit(buf, "%s\n", ptype);
> +}
> +static DEVICE_ATTR_RO(product_type);
> +
> +static umode_t typec_product_type_attr_is_visible(struct kobject *kobj,
> +						  struct attribute *attr, int n)
> +{
> +	if (attr == &dev_attr_product_type.attr)
> +		if (!get_pd_identity(kobj_to_dev(kobj)))
> +			return 0;
> +
> +	return attr->mode;
>  }
>  
>  /* ------------------------------------------------------------------------- */
> @@ -534,10 +621,20 @@ static DEVICE_ATTR_RO(supports_usb_power_delivery);
>  
>  static struct attribute *typec_partner_attrs[] = {
>  	&dev_attr_accessory_mode.attr,
> +	&dev_attr_product_type.attr,
>  	&dev_attr_supports_usb_power_delivery.attr,
>  	NULL
>  };
> -ATTRIBUTE_GROUPS(typec_partner);
> +
> +static struct attribute_group typec_partner_group = {
> +	.is_visible = typec_product_type_attr_is_visible,
> +	.attrs = typec_partner_attrs,
> +};
> +
> +static const struct attribute_group *typec_partner_groups[] = {
> +	&typec_partner_group,
> +	NULL
> +};
>  
>  static void typec_partner_release(struct device *dev)
>  {
> @@ -773,9 +870,19 @@ static DEVICE_ATTR_RO(plug_type);
>  static struct attribute *typec_cable_attrs[] = {
>  	&dev_attr_type.attr,
>  	&dev_attr_plug_type.attr,
> +	&dev_attr_product_type.attr,
> +	NULL
> +};
> +
> +static struct attribute_group typec_cable_group = {
> +	.is_visible = typec_product_type_attr_is_visible,
> +	.attrs = typec_cable_attrs,
> +};
> +
> +static const struct attribute_group *typec_cable_groups[] = {
> +	&typec_cable_group,
>  	NULL
>  };
> -ATTRIBUTE_GROUPS(typec_cable);
>  
>  static void typec_cable_release(struct device *dev)
>  {
> @@ -1352,6 +1459,11 @@ const struct device_type typec_port_dev_type = {
>  /* --------------------------------------- */
>  /* Driver callbacks to report role updates */
>  
> +static int partner_match(struct device *dev, void *data)
> +{
> +	return is_typec_partner(dev);
> +}
> +
>  /**
>   * typec_set_data_role - Report data role change
>   * @port: The USB Type-C Port where the role was changed
> @@ -1361,12 +1473,23 @@ const struct device_type typec_port_dev_type = {
>   */
>  void typec_set_data_role(struct typec_port *port, enum typec_data_role role)
>  {
> +	struct device *partner_dev;
> +
>  	if (port->data_role == role)
>  		return;
>  
>  	port->data_role = role;
>  	sysfs_notify(&port->dev.kobj, NULL, "data_role");
>  	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> +
> +	partner_dev = device_find_child(&port->dev, NULL, partner_match);
> +	if (!partner_dev)
> +		return;
> +
> +	if (to_typec_partner(partner_dev)->identity)
> +		typec_product_type_notify(partner_dev);
> +
> +	put_device(partner_dev);
>  }
>  EXPORT_SYMBOL_GPL(typec_set_data_role);
>  
> @@ -1407,11 +1530,6 @@ void typec_set_vconn_role(struct typec_port *port, enum typec_role role)
>  }
>  EXPORT_SYMBOL_GPL(typec_set_vconn_role);
>  
> -static int partner_match(struct device *dev, void *data)
> -{
> -	return is_typec_partner(dev);
> -}
> -
>  /**
>   * typec_set_pwr_opmode - Report changed power operation mode
>   * @port: The USB Type-C Port where the mode was changed
> -- 
> 2.29.2
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 2/3] usb: typec: Add product_type sysfs attribute file for partners and cables

[Heikki Krogerus]

Hi Benson,

On Wed, Nov 18, 2020 at 09:48:21AM -0800, Benson Leung wrote:
> > +What:		/sys/class/typec/<port>-partner/product_type
> > +Date:		December 2020
> > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > +Description:	USB Power Delivery Specification defines a set of product types
> > +		for the partner devices. This file will show the product type of
> > +		the partner if it is known. Dual-role capable partners will have
> > +		both UFP and DFP product types defined, but only one that
> > +		matches the current role will be active at the time. If the
> > +		product type of the partner is not visible to the device driver,
> > +		this file will not exist.
> > +
> > +		When the partner product type is detected, or changed with role
> > +		swap, uvevent is also raised that contains PRODUCT_TYPE=<product
> > +		type> (for example PRODUCT_TYPE=hub).
> > +
> > +		Valid values:
> > +
> > +		UFP / device role
> > +		========================  ==========================
> > +		undefined		  -
> > +		hub			  PDUSB Hub
> > +		peripheral		  PDUSB Peripheral
> > +		psd			  Power Bank
> > +		ama			  Alternate Mode Adapter
> > +		vpd			  VCONN Powered USB Device
> 
> I have it on good authority that "vpd" is incorrectly categorized here,
> and for future proofing, we'd better not introduce vpd as a product
> type for UFP...
> 
> A vpd is actually more closely related to a "cable" than it is a "UFP."
> A closer reading of the USB Type-C and USB PD specs will reveal that
> VPDs can only ever appear as SOP' and not as SOP, so having its type
> appear under UFP is a mistake.
> 
> In other words, the USB PD V3.0 R2.0 spec is wrong. A change has been
> working its way through the spec committee to fix this, but it is not yet
> published.
> 
> In order to reduce the amount of churn, I would recommend not
> including vpd as a possible type until a new version of the spec (or the ECN)
> is published.

Thanks for the heads-up. I'll leave the vpd out then.

cheers,

-- 
heikki

Re: [RFC PATCH 2/3] usb: typec: Add product_type sysfs attribute file for partners and cables

[Prashant Malani]

Hi Heikki,

On Wed, Nov 18, 2020 at 06:00:58PM +0300, Heikki Krogerus wrote:
> USB Power Delivery Specification defines a set of product
> types for partners and cables. The product type is defined
> in the ID Header VDO, which is the first object in the
> response to the Discover Identity command.
> 
> This sysfs attribute file is only created for the partners
> and cables if the product type is really known in the
> driver. Some interfaces do not give access to the Discover
> Identity response from the partner or cable, but they may
> still supply the product type separately in some cases.
> 
> When the product type of the partner or cable is detected,
> uevent is also raised with PRODUCT_TYPE set to show the
> actual product type (for example PRODUCT_TYPE=host).
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

I tried this out with the following peripherals:
- Thunderbolt 3 active cable.
- Thunderbolt 3 passive cable.
- Dell WD19TB dock.
- Type C DisplayPort enabled monitor (which advertises as AMA).

For the above, the product_type seems to be getting parsed and displayed
correctly, so FWIW:

Tested-by: Prashant Malani <pmalani@chromium.org>

> ---
>  Documentation/ABI/testing/sysfs-class-typec |  55 ++++++++
>  drivers/usb/typec/class.c                   | 132 ++++++++++++++++++--
>  2 files changed, 180 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index b7794e02ad205..4c09e327c62be 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -139,6 +139,42 @@ Description:
>  		Shows if the partner supports USB Power Delivery communication:
>  		Valid values: yes, no
>  
> +What:		/sys/class/typec/<port>-partner/product_type
> +Date:		December 2020
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:	USB Power Delivery Specification defines a set of product types
> +		for the partner devices. This file will show the product type of
> +		the partner if it is known. Dual-role capable partners will have
> +		both UFP and DFP product types defined, but only one that
> +		matches the current role will be active at the time. If the
> +		product type of the partner is not visible to the device driver,
> +		this file will not exist.
> +
> +		When the partner product type is detected, or changed with role
> +		swap, uvevent is also raised that contains PRODUCT_TYPE=<product
> +		type> (for example PRODUCT_TYPE=hub).
> +
> +		Valid values:
> +
> +		UFP / device role
> +		========================  ==========================
> +		undefined		  -
> +		hub			  PDUSB Hub
> +		peripheral		  PDUSB Peripheral
> +		psd			  Power Bank
> +		ama			  Alternate Mode Adapter
> +		vpd			  VCONN Powered USB Device
> +		========================  ==========================
> +
> +		DFP / host role
> +		========================  ==========================
> +		undefined		  -
> +		hub			  PDUSB Hub
> +		host			  PDUSB Host
> +		power_brick		  Power Brick
> +		amc			  Alternate Mode Controller
> +		========================  ==========================
> +
>  What:		/sys/class/typec/<port>-partner>/identity/
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> @@ -202,6 +238,25 @@ Description:
>  		- type-c
>  		- captive
>  
> +What:		/sys/class/typec/<port>-cable/product_type
> +Date:		December 2020
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:	USB Power Delivery Specification defines a set of product types
> +		for the cables. This file will show the product type of the
> +		cable if it is known. If the product type of the cable is not
> +		visible to the device driver, this file will not exist.
> +
> +		When the cable product type is detected, uvevent is also raised
> +		with PRODUCT_TYPE showing the product type of the cable.
> +
> +		Valid values:
> +
> +		========================  ==========================
> +		undefined		  -
> +		active			  Active Cable
> +		passive			  Passive Cable
> +		========================  ==========================

There exists a /sys/class/typec/<port>-cable/type attribute (connected
to the "active" field in struct typec_cable [1]), which is supposed
to be populated by the Type C port driver. Won't the newly introduced
attribute duplicate the same information as "type"?

[1]
https://elixir.bootlin.com/linux/v5.10-rc4/source/include/linux/usb/typec.h#L170

Thanks,

-Prashant

Re: [RFC PATCH 2/3] usb: typec: Add product_type sysfs attribute file for partners and cables

[Heikki Krogerus]

On Wed, Nov 18, 2020 at 10:53:50AM -0800, Prashant Malani wrote:
> > +What:		/sys/class/typec/<port>-cable/product_type
> > +Date:		December 2020
> > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > +Description:	USB Power Delivery Specification defines a set of product types
> > +		for the cables. This file will show the product type of the
> > +		cable if it is known. If the product type of the cable is not
> > +		visible to the device driver, this file will not exist.
> > +
> > +		When the cable product type is detected, uvevent is also raised
> > +		with PRODUCT_TYPE showing the product type of the cable.
> > +
> > +		Valid values:
> > +
> > +		========================  ==========================
> > +		undefined		  -
> > +		active			  Active Cable
> > +		passive			  Passive Cable
> > +		========================  ==========================
> 
> There exists a /sys/class/typec/<port>-cable/type attribute (connected
> to the "active" field in struct typec_cable [1]), which is supposed
> to be populated by the Type C port driver. Won't the newly introduced
> attribute duplicate the same information as "type"?

True. So we don't need add this for the cable separately. I'll just
modify the code so that it considers also the response to Discover
Identity command if we have access to it.

Would it be OK if we name the file "type" instead of "product_type"
also with the partners?

thanks,

-- 
heikki

Re: [RFC PATCH 2/3] usb: typec: Add product_type sysfs attribute file for partners and cables

[Prashant Malani]

Hi Heikki,
 
On Thu, Nov 19, 2020 at 01:05:06PM +0200, Heikki Krogerus wrote:
> On Wed, Nov 18, 2020 at 10:53:50AM -0800, Prashant Malani wrote:
> > > +What:		/sys/class/typec/<port>-cable/product_type
> > > +Date:		December 2020
> > > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > +Description:	USB Power Delivery Specification defines a set of product types
> > > +		for the cables. This file will show the product type of the
> > > +		cable if it is known. If the product type of the cable is not
> > > +		visible to the device driver, this file will not exist.
> > > +
> > > +		When the cable product type is detected, uvevent is also raised
> > > +		with PRODUCT_TYPE showing the product type of the cable.
> > > +
> > > +		Valid values:
> > > +
> > > +		========================  ==========================
> > > +		undefined		  -
> > > +		active			  Active Cable
> > > +		passive			  Passive Cable
> > > +		========================  ==========================
> > 
> > There exists a /sys/class/typec/<port>-cable/type attribute (connected
> > to the "active" field in struct typec_cable [1]), which is supposed
> > to be populated by the Type C port driver. Won't the newly introduced
> > attribute duplicate the same information as "type"?
> 
> True. So we don't need add this for the cable separately. I'll just
> modify the code so that it considers also the response to Discover
> Identity command if we have access to it.
> 
> Would it be OK if we name the file "type" instead of "product_type"
> also with the partners?

That makes the naming consistent. Sounds good to me :)

Best regards,

-Prashant

Re: [RFC PATCH 2/3] usb: typec: Add product_type sysfs attribute file for partners and cables

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]

Hi Prashant,

On Thu, Nov 19, 2020 at 03:11:22AM -0800, Prashant Malani wrote:
> On Thu, Nov 19, 2020 at 01:05:06PM +0200, Heikki Krogerus wrote:
> > On Wed, Nov 18, 2020 at 10:53:50AM -0800, Prashant Malani wrote:
> > > > +What:		/sys/class/typec/<port>-cable/product_type
> > > > +Date:		December 2020
> > > > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > +Description:	USB Power Delivery Specification defines a set of product types
> > > > +		for the cables. This file will show the product type of the
> > > > +		cable if it is known. If the product type of the cable is not
> > > > +		visible to the device driver, this file will not exist.
> > > > +
> > > > +		When the cable product type is detected, uvevent is also raised
> > > > +		with PRODUCT_TYPE showing the product type of the cable.
> > > > +
> > > > +		Valid values:
> > > > +
> > > > +		========================  ==========================
> > > > +		undefined		  -
> > > > +		active			  Active Cable
> > > > +		passive			  Passive Cable
> > > > +		========================  ==========================
> > > 
> > > There exists a /sys/class/typec/<port>-cable/type attribute (connected
> > > to the "active" field in struct typec_cable [1]), which is supposed
> > > to be populated by the Type C port driver. Won't the newly introduced
> > > attribute duplicate the same information as "type"?
> > 
> > True. So we don't need add this for the cable separately. I'll just
> > modify the code so that it considers also the response to Discover
> > Identity command if we have access to it.
> > 
> > Would it be OK if we name the file "type" instead of "product_type"
> > also with the partners?
> 
> That makes the naming consistent. Sounds good to me :)

Cool. Could you test if the attached version works?

thanks,

-- 
heikki

[-- Attachment #2: 0001-usb-typec-Add-type-sysfs-attribute-file-for-partners.patch --]
[-- Type: text/plain, Size: 10307 bytes --]

From a3e8bbad905e6e3ac97fe99998ed4dbc56824de4 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Wed, 18 Nov 2020 17:27:57 +0300
Subject: [PATCH] usb: typec: Add type sysfs attribute file for partners

USB Power Delivery Specification defines a set of product
types for partners and cables. The product type can be read
from the ID Header VDO which is the first object in the
response to the Discover Identity command. This attribute
will display the product type of the partner. The cables
already have the attribute.

This sysfs attribute file is only created for the partners
and cables if the product type is really known in the
driver. Some interfaces do not give access to the Discover
Identity response from the partner or cable, but they may
still supply the product type separately in some cases.

When the product type of the partner or cable is detected,
uevent is also raised with PRODUCT_TYPE set to show the
actual product type (for example PRODUCT_TYPE=host).

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-class-typec |  53 ++++++-
 drivers/usb/typec/class.c                   | 147 +++++++++++++++++---
 2 files changed, 181 insertions(+), 19 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
index b7794e02ad205..6f97b85f55320 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -139,6 +139,41 @@ Description:
 		Shows if the partner supports USB Power Delivery communication:
 		Valid values: yes, no
 
+What:		/sys/class/typec/<port>-partner/type
+Date:		December 2020
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:	USB Power Delivery Specification defines a set of product types
+		for the partner devices. This file will show the product type of
+		the partner if it is known. Dual-role capable partners will have
+		both UFP and DFP product types defined, but only one that
+		matches the current role will be active at the time. If the
+		product type of the partner is not visible to the device driver,
+		this file will not exist.
+
+		When the partner product type is detected, or changed with role
+		swap, uvevent is also raised that contains PRODUCT_TYPE=<product
+		type> (for example PRODUCT_TYPE=hub).
+
+		Valid values:
+
+		UFP / device role
+		========================  ==========================
+		undefined		  -
+		hub			  PDUSB Hub
+		peripheral		  PDUSB Peripheral
+		ama			  Alternate Mode Adapter
+		vpd			  VCONN Powered USB Device
+		========================  ==========================
+
+		DFP / host role
+		========================  ==========================
+		undefined		  -
+		hub			  PDUSB Hub
+		host			  PDUSB Host
+		power_brick		  Power Brick
+		amc			  Alternate Mode Controller
+		========================  ==========================
+
 What:		/sys/class/typec/<port>-partner>/identity/
 Date:		April 2017
 Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
@@ -187,9 +222,21 @@ described in USB Type-C and USB Power Delivery specifications.
 What:		/sys/class/typec/<port>-cable/type
 Date:		April 2017
 Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
-Description:
-		Shows if the cable is active.
-		Valid values: active, passive
+Description:	USB Power Delivery Specification defines a set of product types
+		for the cables. This file will show the product type of the
+		cable if it is known. If the product type of the cable is not
+		visible to the device driver, this file will not exist.
+
+		When the cable product type is detected, uvevent is also raised
+		with PRODUCT_TYPE showing the product type of the cable.
+
+		Valid values:
+
+		========================  ==========================
+		undefined		  -
+		active			  Active Cable
+		passive			  Passive Cable
+		========================  ==========================
 
 What:		/sys/class/typec/<port>-cable/plug_type
 Date:		April 2017
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 35eec707cb512..1190148ad3ed5 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -11,6 +11,7 @@
 #include <linux/mutex.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include <linux/usb/pd_vdo.h>
 
 #include "bus.h"
 
@@ -81,6 +82,29 @@ static const char * const typec_accessory_modes[] = {
 	[TYPEC_ACCESSORY_DEBUG]		= "debug",
 };
 
+/* Product types defined in USB PD Specification R3.0 V2.0 */
+static const char * const product_type_ufp[8] = {
+	[IDH_PTYPE_UNDEF]		= "undefined",
+	[IDH_PTYPE_HUB]			= "hub",
+	[IDH_PTYPE_PERIPH]		= "peripheral",
+	[IDH_PTYPE_PSD]			= "psd",
+	[IDH_PTYPE_AMA]			= "ama",
+};
+
+static const char * const product_type_dfp[8] = {
+	[IDH_PTYPE_DFP_UNDEF]		= "undefined",
+	[IDH_PTYPE_DFP_HUB]		= "hub",
+	[IDH_PTYPE_DFP_HOST]		= "host",
+	[IDH_PTYPE_DFP_PB]		= "power_brick",
+	[IDH_PTYPE_DFP_AMC]		= "amc",
+};
+
+static const char * const product_type_cable[8] = {
+	[IDH_PTYPE_UNDEF]		= "undefined",
+	[IDH_PTYPE_PCABLE]		= "passive",
+	[IDH_PTYPE_ACABLE]		= "active",
+};
+
 static struct usb_pd_identity *get_pd_identity(struct device *dev)
 {
 	if (is_typec_partner(dev)) {
@@ -95,6 +119,32 @@ static struct usb_pd_identity *get_pd_identity(struct device *dev)
 	return NULL;
 }
 
+static const char *get_pd_product_type(struct device *dev)
+{
+	struct typec_port *port = to_typec_port(dev->parent);
+	struct usb_pd_identity *id = get_pd_identity(dev);
+	const char *ptype = NULL;
+
+	if (is_typec_partner(dev)) {
+		if (!id)
+			return NULL;
+
+		if (port->data_role == TYPEC_HOST)
+			ptype = product_type_ufp[PD_IDH_PTYPE(id->id_header)];
+		else
+			ptype = product_type_dfp[PD_IDH_DFP_PTYPE(id->id_header)];
+	} else if (is_typec_cable(dev)) {
+		if (id)
+			ptype = product_type_cable[PD_IDH_PTYPE(id->id_header)];
+		else
+			ptype = to_typec_cable(dev)->active ?
+				product_type_cable[IDH_PTYPE_ACABLE] :
+				product_type_cable[IDH_PTYPE_PCABLE];
+	}
+
+	return ptype;
+}
+
 static ssize_t id_header_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
@@ -139,11 +189,56 @@ static const struct attribute_group *usb_pd_id_groups[] = {
 	NULL,
 };
 
+static void typec_product_type_notify(struct device *dev)
+{
+	char *envp[2] = { };
+	const char *ptype;
+
+	ptype = get_pd_product_type(dev);
+	if (!ptype)
+		return;
+
+	sysfs_notify(&dev->kobj, NULL, "type");
+
+	envp[0] = kasprintf(GFP_KERNEL, "PRODUCT_TYPE=%s", ptype);
+	if (!envp[0])
+		return;
+
+	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
+	kfree(envp[0]);
+}
+
 static void typec_report_identity(struct device *dev)
 {
 	sysfs_notify(&dev->kobj, "identity", "id_header");
 	sysfs_notify(&dev->kobj, "identity", "cert_stat");
 	sysfs_notify(&dev->kobj, "identity", "product");
+	typec_product_type_notify(dev);
+}
+
+static ssize_t
+type_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	const char *ptype;
+
+	ptype = get_pd_product_type(dev);
+	if (!ptype)
+		return 0;
+
+	return sysfs_emit(buf, "%s\n", ptype);
+}
+static DEVICE_ATTR_RO(type);
+
+static umode_t typec_product_type_attr_is_visible(struct kobject *kobj,
+						  struct attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+
+	if (attr == &dev_attr_type.attr)
+		if (!get_pd_product_type(dev))
+			return 0;
+
+	return attr->mode;
 }
 
 /* ------------------------------------------------------------------------- */
@@ -535,9 +630,19 @@ static DEVICE_ATTR_RO(supports_usb_power_delivery);
 static struct attribute *typec_partner_attrs[] = {
 	&dev_attr_accessory_mode.attr,
 	&dev_attr_supports_usb_power_delivery.attr,
+	&dev_attr_type.attr,
+	NULL
+};
+
+static struct attribute_group typec_partner_group = {
+	.is_visible = typec_product_type_attr_is_visible,
+	.attrs = typec_partner_attrs,
+};
+
+static const struct attribute_group *typec_partner_groups[] = {
+	&typec_partner_group,
 	NULL
 };
-ATTRIBUTE_GROUPS(typec_partner);
 
 static void typec_partner_release(struct device *dev)
 {
@@ -744,15 +849,6 @@ EXPORT_SYMBOL_GPL(typec_unregister_plug);
 
 /* Type-C Cables */
 
-static ssize_t
-type_show(struct device *dev, struct device_attribute *attr, char *buf)
-{
-	struct typec_cable *cable = to_typec_cable(dev);
-
-	return sprintf(buf, "%s\n", cable->active ? "active" : "passive");
-}
-static DEVICE_ATTR_RO(type);
-
 static const char * const typec_plug_types[] = {
 	[USB_PLUG_NONE]		= "unknown",
 	[USB_PLUG_TYPE_A]	= "type-a",
@@ -775,7 +871,15 @@ static struct attribute *typec_cable_attrs[] = {
 	&dev_attr_plug_type.attr,
 	NULL
 };
-ATTRIBUTE_GROUPS(typec_cable);
+
+static struct attribute_group typec_cable_group = {
+	.attrs = typec_cable_attrs,
+};
+
+static const struct attribute_group *typec_cable_groups[] = {
+	&typec_cable_group,
+	NULL
+};
 
 static void typec_cable_release(struct device *dev)
 {
@@ -1352,6 +1456,11 @@ const struct device_type typec_port_dev_type = {
 /* --------------------------------------- */
 /* Driver callbacks to report role updates */
 
+static int partner_match(struct device *dev, void *data)
+{
+	return is_typec_partner(dev);
+}
+
 /**
  * typec_set_data_role - Report data role change
  * @port: The USB Type-C Port where the role was changed
@@ -1361,12 +1470,23 @@ const struct device_type typec_port_dev_type = {
  */
 void typec_set_data_role(struct typec_port *port, enum typec_data_role role)
 {
+	struct device *partner_dev;
+
 	if (port->data_role == role)
 		return;
 
 	port->data_role = role;
 	sysfs_notify(&port->dev.kobj, NULL, "data_role");
 	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
+
+	partner_dev = device_find_child(&port->dev, NULL, partner_match);
+	if (!partner_dev)
+		return;
+
+	if (to_typec_partner(partner_dev)->identity)
+		typec_product_type_notify(partner_dev);
+
+	put_device(partner_dev);
 }
 EXPORT_SYMBOL_GPL(typec_set_data_role);
 
@@ -1407,11 +1527,6 @@ void typec_set_vconn_role(struct typec_port *port, enum typec_role role)
 }
 EXPORT_SYMBOL_GPL(typec_set_vconn_role);
 
-static int partner_match(struct device *dev, void *data)
-{
-	return is_typec_partner(dev);
-}
-
 /**
  * typec_set_pwr_opmode - Report changed power operation mode
  * @port: The USB Type-C Port where the mode was changed
-- 
2.29.2

[RFC PATCH 3/3] usb: typec: Expose Product Type VDOs via sysfs

[Heikki Krogerus]

From: Prashant Malani <pmalani@chromium.org>

Interim. ABI doc missing.

A PD-capable device can return up to 3 Product Type VDOs as part of its
DiscoverIdentity Response (USB PD Spec, Rev 3.0, Version 2.0, Section
6.4.4.3.1). Add sysfs attribute to expose these to userspace.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
[ heikki: Only one instead of three attribute files ]
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>'
---
 drivers/usb/typec/class.c | 41 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 303f054181ff7..5e135678f5952 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -165,15 +165,55 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(product);
 
+static ssize_t
+product_type_vdo_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct usb_pd_identity *id = get_pd_identity(dev);
+	size_t len = 0;
+	int i;
+
+	for (i = 0; i < 3; i++) {
+		if (!id->vdo[i])
+			break;
+		len += sysfs_emit(buf, "%08x ", id->vdo[i]);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static struct device_attribute dev_attr_product_type_vdo = {
+	.attr = {
+		.name = "product_type",
+		.mode = 0444,
+	},
+	.show = product_type_vdo_show,
+};
+
+static umode_t
+typec_identity_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct usb_pd_identity *id = get_pd_identity(kobj_to_dev(kobj));
+
+	if (attr == &dev_attr_product_type_vdo.attr &&
+	    !id->vdo[0])
+		return 0;
+
+	return attr->mode;
+}
+
 static struct attribute *usb_pd_id_attrs[] = {
 	&dev_attr_id_header.attr,
 	&dev_attr_cert_stat.attr,
 	&dev_attr_product.attr,
+	&dev_attr_product_type_vdo.attr,
 	NULL
 };
 
 static const struct attribute_group usb_pd_id_group = {
 	.name = "identity",
+	.is_visible = typec_identity_attr_is_visible,
 	.attrs = usb_pd_id_attrs,
 };
 
@@ -191,6 +231,7 @@ static void typec_product_type_notify(struct device *dev)
 	if (!ptype)
 		return;
 
+	sysfs_notify(&dev->kobj, "identity", "product_type");
 	sysfs_notify(&dev->kobj, NULL, "product_type");
 
 	envp[0] = kasprintf(GFP_KERNEL, "PRODUCT_TYPE=%s", ptype);
-- 
2.29.2

Re: [RFC PATCH 3/3] usb: typec: Expose Product Type VDOs via sysfs

[Greg Kroah-Hartman]

On Wed, Nov 18, 2020 at 06:00:59PM +0300, Heikki Krogerus wrote:
> From: Prashant Malani <pmalani@chromium.org>
> 
> Interim. ABI doc missing.
> 
> A PD-capable device can return up to 3 Product Type VDOs as part of its
> DiscoverIdentity Response (USB PD Spec, Rev 3.0, Version 2.0, Section
> 6.4.4.3.1). Add sysfs attribute to expose these to userspace.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> [ heikki: Only one instead of three attribute files ]
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>'
> ---
>  drivers/usb/typec/class.c | 41 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 303f054181ff7..5e135678f5952 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -165,15 +165,55 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(product);
>  
> +static ssize_t
> +product_type_vdo_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct usb_pd_identity *id = get_pd_identity(dev);
> +	size_t len = 0;
> +	int i;
> +
> +	for (i = 0; i < 3; i++) {
> +		if (!id->vdo[i])
> +			break;
> +		len += sysfs_emit(buf, "%08x ", id->vdo[i]);
> +	}
> +
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}

I don't understand what you are trying to print out here, documentation
would be helpful :)

> +
> +static struct device_attribute dev_attr_product_type_vdo = {
> +	.attr = {
> +		.name = "product_type",
> +		.mode = 0444,
> +	},
> +	.show = product_type_vdo_show,
> +};

DEVICE_ATTR_RO(product_type_vdo)?

Why are you calling it "product_type" and not with the "vdo"?

And you have to name it this, there's always __ATTR_RO(), never put a
mode in "raw" numbers for a sysfs file if at all possible.

thanks,

greg k-h

Re: [RFC PATCH 3/3] usb: typec: Expose Product Type VDOs via sysfs

[Heikki Krogerus]

On Wed, Nov 18, 2020 at 04:56:57PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 18, 2020 at 06:00:59PM +0300, Heikki Krogerus wrote:
> > From: Prashant Malani <pmalani@chromium.org>
> > 
> > Interim. ABI doc missing.
> > 
> > A PD-capable device can return up to 3 Product Type VDOs as part of its
> > DiscoverIdentity Response (USB PD Spec, Rev 3.0, Version 2.0, Section
> > 6.4.4.3.1). Add sysfs attribute to expose these to userspace.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > [ heikki: Only one instead of three attribute files ]
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>'
> > ---
> >  drivers/usb/typec/class.c | 41 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 303f054181ff7..5e135678f5952 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -165,15 +165,55 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(product);
> >  
> > +static ssize_t
> > +product_type_vdo_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct usb_pd_identity *id = get_pd_identity(dev);
> > +	size_t len = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < 3; i++) {
> > +		if (!id->vdo[i])
> > +			break;
> > +		len += sysfs_emit(buf, "%08x ", id->vdo[i]);
> > +	}
> > +
> > +	buf[len - 1] = '\n';
> > +
> > +	return len;
> > +}
> 
> I don't understand what you are trying to print out here, documentation
> would be helpful :)
> 
> > +static struct device_attribute dev_attr_product_type_vdo = {
> > +	.attr = {
> > +		.name = "product_type",
> > +		.mode = 0444,
> > +	},
> > +	.show = product_type_vdo_show,
> > +};
> 
> DEVICE_ATTR_RO(product_type_vdo)?
> 
> Why are you calling it "product_type" and not with the "vdo"?
> 
> And you have to name it this, there's always __ATTR_RO(), never put a
> mode in "raw" numbers for a sysfs file if at all possible.

Sorry Greg. This is still work-in-progress.

I didn't use the _vdo ending in the file name because the other files
exposing the other parts (VDOs) of the response to the discover
identity don't have it either.


thanks,

-- 
heikki

Re: [RFC PATCH 0/3] usb: typec: Product Type time

[Prashant Malani]

Hi Heikki,

Thanks for developing these patches :)

On Wed, Nov 18, 2020 at 06:00:56PM +0300, Heikki Krogerus wrote:
> Hi Prashant,
> 
> The original discussion [1].
> 
> This proposal is in practice a compromise. I came to the conclusion
> that we probable should expose the product type separately after all.
> The reason for that is because we may in some cases actually know the
> product type even when we don't have access to the Discover Identity
> response. UCSI for example in practice gives us at least the cable
> product type even though it does not let us know the response to the
> Discover Identity command.
> 
> So my proposal here is that we add an attribute for the product type
> itself, showing the product type as a string. Then we also add the
> attribute for the product type specific VDOs which we place under the
> identity directory more or less the way you originally proposed.

Sounds good to me.

Best regards,

-Prashant