All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation
@ 2020-10-23 21:43 Prashant Malani
  2020-10-23 21:43 ` [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs Prashant Malani
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Prashant Malani @ 2020-10-23 21:43 UTC (permalink / raw)
  To: linux-kernel, linux-usb, gregkh; +Cc: Prashant Malani, Heikki Krogerus

Both partner and cable have identity VDOs. These are listed separately
in the Documentation/ABI/testing/sysfs-class-typec. Factor these out
into a common location to avoid the duplication.

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

Patch first introduced in v3.

 Documentation/ABI/testing/sysfs-class-typec | 59 ++++++---------------
 1 file changed, 17 insertions(+), 42 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
index b834671522d6..0f839fd022f1 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -134,42 +134,6 @@ Description:
 		Shows if the partner supports USB Power Delivery communication:
 		Valid values: yes, no
 
-What:		/sys/class/typec/<port>-partner>/identity/
-Date:		April 2017
-Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
-Description:
-		This directory appears only if the port device driver is capable
-		of showing the result of Discover Identity USB power delivery
-		command. That will not always be possible even when USB power
-		delivery is supported, for example when USB power delivery
-		communication for the port is mostly handled in firmware. If the
-		directory exists, it will have an attribute file for every VDO
-		in Discover Identity command result.
-
-What:		/sys/class/typec/<port>-partner/identity/id_header
-Date:		April 2017
-Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
-Description:
-		ID Header VDO part of Discover Identity command result. The
-		value will show 0 until Discover Identity command result becomes
-		available. The value can be polled.
-
-What:		/sys/class/typec/<port>-partner/identity/cert_stat
-Date:		April 2017
-Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
-Description:
-		Cert Stat VDO part of Discover Identity command result. The
-		value will show 0 until Discover Identity command result becomes
-		available. The value can be polled.
-
-What:		/sys/class/typec/<port>-partner/identity/product
-Date:		April 2017
-Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
-Description:
-		Product VDO part of Discover Identity command result. The value
-		will show 0 until Discover Identity command result becomes
-		available. The value can be polled.
-
 
 USB Type-C cable devices (eg. /sys/class/typec/port0-cable/)
 
@@ -196,17 +160,28 @@ Description:
 		- type-c
 		- captive
 
-What:		/sys/class/typec/<port>-cable/identity/
+
+USB Type-C partner/cable Power Delivery Identity objects
+
+NOTE: The following attributes will be applicable to both
+partner (e.g /sys/class/typec/port0-partner/) and
+cable (e.g /sys/class/typec/port0-cable/) devices. Consequently, the example file
+paths below are prefixed with "/sys/class/typec/<port>-{partner|cable}/" to
+reflect this.
+
+What:		/sys/class/typec/<port>-{partner|cable}/identity/
 Date:		April 2017
 Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
 Description:
 		This directory appears only if the port device driver is capable
 		of showing the result of Discover Identity USB power delivery
 		command. That will not always be possible even when USB power
-		delivery is supported. If the directory exists, it will have an
-		attribute for every VDO returned by Discover Identity command.
+		delivery is supported, for example when USB power delivery
+		communication for the port is mostly handled in firmware. If the
+		directory exists, it will have an attribute file for every VDO
+		in Discover Identity command result.
 
-What:		/sys/class/typec/<port>-cable/identity/id_header
+What:		/sys/class/typec/<port>-{partner|cable}/identity/id_header
 Date:		April 2017
 Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
 Description:
@@ -214,7 +189,7 @@ Description:
 		value will show 0 until Discover Identity command result becomes
 		available. The value can be polled.
 
-What:		/sys/class/typec/<port>-cable/identity/cert_stat
+What:		/sys/class/typec/<port>-{partner|cable}/identity/cert_stat
 Date:		April 2017
 Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
 Description:
@@ -222,7 +197,7 @@ Description:
 		value will show 0 until Discover Identity command result becomes
 		available. The value can be polled.
 
-What:		/sys/class/typec/<port>-cable/identity/product
+What:		/sys/class/typec/<port>-{partner|cable}/identity/product
 Date:		April 2017
 Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
 Description:
-- 
2.29.0.rc1.297.gfa9743e501-goog


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

* [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-10-23 21:43 [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation Prashant Malani
@ 2020-10-23 21:43 ` Prashant Malani
  2020-11-04 17:49   ` Prashant Malani
                     ` (2 more replies)
  2020-11-10 10:52 ` [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation Heikki Krogerus
  2020-11-24 13:23 ` Heikki Krogerus
  2 siblings, 3 replies; 15+ messages in thread
From: Prashant Malani @ 2020-10-23 21:43 UTC (permalink / raw)
  To: linux-kernel, linux-usb, gregkh
  Cc: Prashant Malani, Benson Leung, Heikki Krogerus

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 attributes to expose these to userspace.

Cc: Benson Leung <bleung@chromium.org>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v3:
- Split each product type VDO into a separate attribute.
- Changed sprintf() to sysfs_emit().
- Changed ABI documentation based on consolidation of identity VDO
  descriptions in the previous patch (1/2).

Changes in v2:
- Added sysfs_notify() call for the attribute.
- Added description for the attribute in
  Documentation/ABI/testing/sysfs-class-typec.

 Documentation/ABI/testing/sysfs-class-typec | 24 +++++++++++++++
 drivers/usb/typec/class.c                   | 33 +++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
index 0f839fd022f1..0ac144bc5927 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -205,6 +205,30 @@ Description:
 		will show 0 until Discover Identity command result becomes
 		available. The value can be polled.
 
+What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo1
+Date:		October 2020
+Contact:	Prashant Malani <pmalani@chromium.org>
+Description:
+		1st Product Type VDO of Discover Identity command result.
+		The value will show 0 until Discover Identity command result becomes
+		available and a valid Product Type VDO is returned.
+
+What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo2
+Date:		October 2020
+Contact:	Prashant Malani <pmalani@chromium.org>
+Description:
+		2nd Product Type VDO of Discover Identity command result.
+		The value will show 0 until Discover Identity command result becomes
+		available and a valid Product Type VDO is returned.
+
+What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo3
+Date:		October 2020
+Contact:	Prashant Malani <pmalani@chromium.org>
+Description:
+		3rd Product Type VDO of Discover Identity command result.
+		The value will show 0 until Discover Identity command result becomes
+		available and a valid Product Type VDO is returned.
+
 
 USB Type-C port alternate mode devices.
 
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 35eec707cb51..a2c88594b044 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -122,10 +122,40 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(product);
 
+static ssize_t product_type_vdo1_show(struct device *dev, struct device_attribute *attr,
+				      char *buf)
+{
+	struct usb_pd_identity *id = get_pd_identity(dev);
+
+	return sysfs_emit(buf, "0x%08x\n", id->vdo[0]);
+}
+static DEVICE_ATTR_RO(product_type_vdo1);
+
+static ssize_t product_type_vdo2_show(struct device *dev, struct device_attribute *attr,
+				      char *buf)
+{
+	struct usb_pd_identity *id = get_pd_identity(dev);
+
+	return sysfs_emit(buf, "0x%08x\n", id->vdo[1]);
+}
+static DEVICE_ATTR_RO(product_type_vdo2);
+
+static ssize_t product_type_vdo3_show(struct device *dev, struct device_attribute *attr,
+				      char *buf)
+{
+	struct usb_pd_identity *id = get_pd_identity(dev);
+
+	return sysfs_emit(buf, "0x%08x\n", id->vdo[2]);
+}
+static DEVICE_ATTR_RO(product_type_vdo3);
+
 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_vdo1.attr,
+	&dev_attr_product_type_vdo2.attr,
+	&dev_attr_product_type_vdo3.attr,
 	NULL
 };
 
@@ -144,6 +174,9 @@ 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");
+	sysfs_notify(&dev->kobj, "identity", "product_type_vdo1");
+	sysfs_notify(&dev->kobj, "identity", "product_type_vdo2");
+	sysfs_notify(&dev->kobj, "identity", "product_type_vdo3");
 }
 
 /* ------------------------------------------------------------------------- */
-- 
2.29.0.rc1.297.gfa9743e501-goog


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

* Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-10-23 21:43 ` [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs Prashant Malani
@ 2020-11-04 17:49   ` Prashant Malani
  2020-11-06  7:36     ` Heikki Krogerus
  2020-11-10 11:54   ` Heikki Krogerus
  2020-11-24 13:28   ` Heikki Krogerus
  2 siblings, 1 reply; 15+ messages in thread
From: Prashant Malani @ 2020-11-04 17:49 UTC (permalink / raw)
  To: linux-kernel, linux-usb, gregkh; +Cc: Benson Leung, Heikki Krogerus

Hi All,

Was wondering if there were any comments on v3 of this series?

Best regards,

-Prashant
On Fri, Oct 23, 2020 at 02:43:28PM -0700, Prashant Malani wrote:
> 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 attributes to expose these to userspace.
> 
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---

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

* Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-11-04 17:49   ` Prashant Malani
@ 2020-11-06  7:36     ` Heikki Krogerus
  0 siblings, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2020-11-06  7:36 UTC (permalink / raw)
  To: Prashant Malani; +Cc: linux-kernel, linux-usb, gregkh, Benson Leung

On Wed, Nov 04, 2020 at 09:49:17AM -0800, Prashant Malani wrote:
> Hi All,
> 
> Was wondering if there were any comments on v3 of this series?

Sorry to keep you waiting with this...

I'm still wondering if the patch 2/2 is what we should use. Right now
I don't have really any better ideas. But give the rest of the week to
think about this.

thanks,

-- 
heikki

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

* Re: [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation
  2020-10-23 21:43 [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation Prashant Malani
  2020-10-23 21:43 ` [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs Prashant Malani
@ 2020-11-10 10:52 ` Heikki Krogerus
  2020-11-24 13:23 ` Heikki Krogerus
  2 siblings, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2020-11-10 10:52 UTC (permalink / raw)
  To: Prashant Malani; +Cc: linux-kernel, linux-usb, gregkh

On Fri, Oct 23, 2020 at 02:43:26PM -0700, Prashant Malani wrote:
> Both partner and cable have identity VDOs. These are listed separately
> in the Documentation/ABI/testing/sysfs-class-typec. Factor these out
> into a common location to avoid the duplication.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

I don't have a problem with this change. FWIW:

Acked-by: Heikki Krogerus <heikki.krogeurus@linux.intel.com>

> ---
> 
> Patch first introduced in v3.
> 
>  Documentation/ABI/testing/sysfs-class-typec | 59 ++++++---------------
>  1 file changed, 17 insertions(+), 42 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index b834671522d6..0f839fd022f1 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -134,42 +134,6 @@ Description:
>  		Shows if the partner supports USB Power Delivery communication:
>  		Valid values: yes, no
>  
> -What:		/sys/class/typec/<port>-partner>/identity/
> -Date:		April 2017
> -Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> -Description:
> -		This directory appears only if the port device driver is capable
> -		of showing the result of Discover Identity USB power delivery
> -		command. That will not always be possible even when USB power
> -		delivery is supported, for example when USB power delivery
> -		communication for the port is mostly handled in firmware. If the
> -		directory exists, it will have an attribute file for every VDO
> -		in Discover Identity command result.
> -
> -What:		/sys/class/typec/<port>-partner/identity/id_header
> -Date:		April 2017
> -Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> -Description:
> -		ID Header VDO part of Discover Identity command result. The
> -		value will show 0 until Discover Identity command result becomes
> -		available. The value can be polled.
> -
> -What:		/sys/class/typec/<port>-partner/identity/cert_stat
> -Date:		April 2017
> -Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> -Description:
> -		Cert Stat VDO part of Discover Identity command result. The
> -		value will show 0 until Discover Identity command result becomes
> -		available. The value can be polled.
> -
> -What:		/sys/class/typec/<port>-partner/identity/product
> -Date:		April 2017
> -Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> -Description:
> -		Product VDO part of Discover Identity command result. The value
> -		will show 0 until Discover Identity command result becomes
> -		available. The value can be polled.
> -
>  
>  USB Type-C cable devices (eg. /sys/class/typec/port0-cable/)
>  
> @@ -196,17 +160,28 @@ Description:
>  		- type-c
>  		- captive
>  
> -What:		/sys/class/typec/<port>-cable/identity/
> +
> +USB Type-C partner/cable Power Delivery Identity objects
> +
> +NOTE: The following attributes will be applicable to both
> +partner (e.g /sys/class/typec/port0-partner/) and
> +cable (e.g /sys/class/typec/port0-cable/) devices. Consequently, the example file
> +paths below are prefixed with "/sys/class/typec/<port>-{partner|cable}/" to
> +reflect this.
> +
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>  Description:
>  		This directory appears only if the port device driver is capable
>  		of showing the result of Discover Identity USB power delivery
>  		command. That will not always be possible even when USB power
> -		delivery is supported. If the directory exists, it will have an
> -		attribute for every VDO returned by Discover Identity command.
> +		delivery is supported, for example when USB power delivery
> +		communication for the port is mostly handled in firmware. If the
> +		directory exists, it will have an attribute file for every VDO
> +		in Discover Identity command result.
>  
> -What:		/sys/class/typec/<port>-cable/identity/id_header
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/id_header
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>  Description:
> @@ -214,7 +189,7 @@ Description:
>  		value will show 0 until Discover Identity command result becomes
>  		available. The value can be polled.
>  
> -What:		/sys/class/typec/<port>-cable/identity/cert_stat
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/cert_stat
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>  Description:
> @@ -222,7 +197,7 @@ Description:
>  		value will show 0 until Discover Identity command result becomes
>  		available. The value can be polled.
>  
> -What:		/sys/class/typec/<port>-cable/identity/product
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/product
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>  Description:
> -- 
> 2.29.0.rc1.297.gfa9743e501-goog

thanks,

-- 
heikki

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

* Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-10-23 21:43 ` [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs Prashant Malani
  2020-11-04 17:49   ` Prashant Malani
@ 2020-11-10 11:54   ` Heikki Krogerus
  2020-11-10 12:22     ` Greg KH
  2020-11-12  2:40     ` Prashant Malani
  2020-11-24 13:28   ` Heikki Krogerus
  2 siblings, 2 replies; 15+ messages in thread
From: Heikki Krogerus @ 2020-11-10 11:54 UTC (permalink / raw)
  To: Prashant Malani; +Cc: linux-kernel, linux-usb, gregkh, Benson Leung

On Fri, Oct 23, 2020 at 02:43:28PM -0700, Prashant Malani wrote:
> 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 attributes to expose these to userspace.
> 
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes in v3:
> - Split each product type VDO into a separate attribute.
> - Changed sprintf() to sysfs_emit().
> - Changed ABI documentation based on consolidation of identity VDO
>   descriptions in the previous patch (1/2).
> 
> Changes in v2:
> - Added sysfs_notify() call for the attribute.
> - Added description for the attribute in
>   Documentation/ABI/testing/sysfs-class-typec.
> 
>  Documentation/ABI/testing/sysfs-class-typec | 24 +++++++++++++++
>  drivers/usb/typec/class.c                   | 33 +++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index 0f839fd022f1..0ac144bc5927 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -205,6 +205,30 @@ Description:
>  		will show 0 until Discover Identity command result becomes
>  		available. The value can be polled.
>  
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo1
> +Date:		October 2020
> +Contact:	Prashant Malani <pmalani@chromium.org>
> +Description:
> +		1st Product Type VDO of Discover Identity command result.
> +		The value will show 0 until Discover Identity command result becomes
> +		available and a valid Product Type VDO is returned.
> +
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo2
> +Date:		October 2020
> +Contact:	Prashant Malani <pmalani@chromium.org>
> +Description:
> +		2nd Product Type VDO of Discover Identity command result.
> +		The value will show 0 until Discover Identity command result becomes
> +		available and a valid Product Type VDO is returned.
> +
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo3
> +Date:		October 2020
> +Contact:	Prashant Malani <pmalani@chromium.org>
> +Description:
> +		3rd Product Type VDO of Discover Identity command result.
> +		The value will show 0 until Discover Identity command result becomes
> +		available and a valid Product Type VDO is returned.
> +
>  
>  USB Type-C port alternate mode devices.
>  
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 35eec707cb51..a2c88594b044 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -122,10 +122,40 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(product);
>  
> +static ssize_t product_type_vdo1_show(struct device *dev, struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct usb_pd_identity *id = get_pd_identity(dev);
> +
> +	return sysfs_emit(buf, "0x%08x\n", id->vdo[0]);
> +}
> +static DEVICE_ATTR_RO(product_type_vdo1);
> +
> +static ssize_t product_type_vdo2_show(struct device *dev, struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct usb_pd_identity *id = get_pd_identity(dev);
> +
> +	return sysfs_emit(buf, "0x%08x\n", id->vdo[1]);
> +}
> +static DEVICE_ATTR_RO(product_type_vdo2);
> +
> +static ssize_t product_type_vdo3_show(struct device *dev, struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct usb_pd_identity *id = get_pd_identity(dev);
> +
> +	return sysfs_emit(buf, "0x%08x\n", id->vdo[2]);
> +}
> +static DEVICE_ATTR_RO(product_type_vdo3);
> +
>  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_vdo1.attr,
> +	&dev_attr_product_type_vdo2.attr,
> +	&dev_attr_product_type_vdo3.attr,
>  	NULL
>  };
>  
> @@ -144,6 +174,9 @@ 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");
> +	sysfs_notify(&dev->kobj, "identity", "product_type_vdo1");
> +	sysfs_notify(&dev->kobj, "identity", "product_type_vdo2");
> +	sysfs_notify(&dev->kobj, "identity", "product_type_vdo3");
>  }

I've now come to the conclusion that this is not the correct approach.
Instead, the whole identity, all six VDOs, should be supplied
separately with a "raw" sysfs attribute file after all.

The three attribute files that we already have - so id_header,
cert_stat and product - can always supply the actual VDO as is,
regardless of the product type, so they are fine. But these new
attribute files, product_type_vdoX, would behave differently as they
supply different information depending on the product type. That just
does not feel right to me.

So lets just add the "raw" sysfs attribute file. We can think about
extracting some other details from the product type VDOs once the
specification has settled down a bit and we can be quite certain that
those details will always be available.

Would this be OK to you? I think we should be able to dump the data to
the "raw" sysfs attribute file with something like hex_dump_to_buffer().

thanks,

-- 
heikki

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

* Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-11-10 11:54   ` Heikki Krogerus
@ 2020-11-10 12:22     ` Greg KH
  2020-11-11  9:21       ` Heikki Krogerus
  2020-11-12  2:40     ` Prashant Malani
  1 sibling, 1 reply; 15+ messages in thread
From: Greg KH @ 2020-11-10 12:22 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: Prashant Malani, linux-kernel, linux-usb, Benson Leung

On Tue, Nov 10, 2020 at 01:54:53PM +0200, Heikki Krogerus wrote:
> On Fri, Oct 23, 2020 at 02:43:28PM -0700, Prashant Malani wrote:
> > 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 attributes to expose these to userspace.
> > 
> > Cc: Benson Leung <bleung@chromium.org>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> > 
> > Changes in v3:
> > - Split each product type VDO into a separate attribute.
> > - Changed sprintf() to sysfs_emit().
> > - Changed ABI documentation based on consolidation of identity VDO
> >   descriptions in the previous patch (1/2).
> > 
> > Changes in v2:
> > - Added sysfs_notify() call for the attribute.
> > - Added description for the attribute in
> >   Documentation/ABI/testing/sysfs-class-typec.
> > 
> >  Documentation/ABI/testing/sysfs-class-typec | 24 +++++++++++++++
> >  drivers/usb/typec/class.c                   | 33 +++++++++++++++++++++
> >  2 files changed, 57 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > index 0f839fd022f1..0ac144bc5927 100644
> > --- a/Documentation/ABI/testing/sysfs-class-typec
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -205,6 +205,30 @@ Description:
> >  		will show 0 until Discover Identity command result becomes
> >  		available. The value can be polled.
> >  
> > +What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo1
> > +Date:		October 2020
> > +Contact:	Prashant Malani <pmalani@chromium.org>
> > +Description:
> > +		1st Product Type VDO of Discover Identity command result.
> > +		The value will show 0 until Discover Identity command result becomes
> > +		available and a valid Product Type VDO is returned.
> > +
> > +What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo2
> > +Date:		October 2020
> > +Contact:	Prashant Malani <pmalani@chromium.org>
> > +Description:
> > +		2nd Product Type VDO of Discover Identity command result.
> > +		The value will show 0 until Discover Identity command result becomes
> > +		available and a valid Product Type VDO is returned.
> > +
> > +What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo3
> > +Date:		October 2020
> > +Contact:	Prashant Malani <pmalani@chromium.org>
> > +Description:
> > +		3rd Product Type VDO of Discover Identity command result.
> > +		The value will show 0 until Discover Identity command result becomes
> > +		available and a valid Product Type VDO is returned.
> > +
> >  
> >  USB Type-C port alternate mode devices.
> >  
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 35eec707cb51..a2c88594b044 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -122,10 +122,40 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(product);
> >  
> > +static ssize_t product_type_vdo1_show(struct device *dev, struct device_attribute *attr,
> > +				      char *buf)
> > +{
> > +	struct usb_pd_identity *id = get_pd_identity(dev);
> > +
> > +	return sysfs_emit(buf, "0x%08x\n", id->vdo[0]);
> > +}
> > +static DEVICE_ATTR_RO(product_type_vdo1);
> > +
> > +static ssize_t product_type_vdo2_show(struct device *dev, struct device_attribute *attr,
> > +				      char *buf)
> > +{
> > +	struct usb_pd_identity *id = get_pd_identity(dev);
> > +
> > +	return sysfs_emit(buf, "0x%08x\n", id->vdo[1]);
> > +}
> > +static DEVICE_ATTR_RO(product_type_vdo2);
> > +
> > +static ssize_t product_type_vdo3_show(struct device *dev, struct device_attribute *attr,
> > +				      char *buf)
> > +{
> > +	struct usb_pd_identity *id = get_pd_identity(dev);
> > +
> > +	return sysfs_emit(buf, "0x%08x\n", id->vdo[2]);
> > +}
> > +static DEVICE_ATTR_RO(product_type_vdo3);
> > +
> >  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_vdo1.attr,
> > +	&dev_attr_product_type_vdo2.attr,
> > +	&dev_attr_product_type_vdo3.attr,
> >  	NULL
> >  };
> >  
> > @@ -144,6 +174,9 @@ 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");
> > +	sysfs_notify(&dev->kobj, "identity", "product_type_vdo1");
> > +	sysfs_notify(&dev->kobj, "identity", "product_type_vdo2");
> > +	sysfs_notify(&dev->kobj, "identity", "product_type_vdo3");
> >  }
> 
> I've now come to the conclusion that this is not the correct approach.
> Instead, the whole identity, all six VDOs, should be supplied
> separately with a "raw" sysfs attribute file after all.
> 
> The three attribute files that we already have - so id_header,
> cert_stat and product - can always supply the actual VDO as is,
> regardless of the product type, so they are fine. But these new
> attribute files, product_type_vdoX, would behave differently as they
> supply different information depending on the product type. That just
> does not feel right to me.
> 
> So lets just add the "raw" sysfs attribute file. We can think about
> extracting some other details from the product type VDOs once the
> specification has settled down a bit and we can be quite certain that
> those details will always be available.
> 
> Would this be OK to you? I think we should be able to dump the data to
> the "raw" sysfs attribute file with something like hex_dump_to_buffer().

Does this mean that the value of the attributes depends on something
external to the device?  If so, how is userspace going to know how to
parse this any differently than the kernel could today?

And I say this as the maintainer of 'lsusb' which probably should start
getting support for the typec attributes that are being exposed here :)

thanks,

greg k-h

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

* Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-11-10 12:22     ` Greg KH
@ 2020-11-11  9:21       ` Heikki Krogerus
  0 siblings, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2020-11-11  9:21 UTC (permalink / raw)
  To: Greg KH; +Cc: Prashant Malani, linux-kernel, linux-usb, Benson Leung

Hi Greg,

On Tue, Nov 10, 2020 at 01:22:06PM +0100, Greg KH wrote:
> > I've now come to the conclusion that this is not the correct approach.
> > Instead, the whole identity, all six VDOs, should be supplied
> > separately with a "raw" sysfs attribute file after all.
> > 
> > The three attribute files that we already have - so id_header,
> > cert_stat and product - can always supply the actual VDO as is,
> > regardless of the product type, so they are fine. But these new
> > attribute files, product_type_vdoX, would behave differently as they
> > supply different information depending on the product type. That just
> > does not feel right to me.
> > 
> > So lets just add the "raw" sysfs attribute file. We can think about
> > extracting some other details from the product type VDOs once the
> > specification has settled down a bit and we can be quite certain that
> > those details will always be available.
> > 
> > Would this be OK to you? I think we should be able to dump the data to
> > the "raw" sysfs attribute file with something like hex_dump_to_buffer().
> 
> Does this mean that the value of the attributes depends on something
> external to the device?  If so, how is userspace going to know how to
> parse this any differently than the kernel could today?
> 
> And I say this as the maintainer of 'lsusb' which probably should start
> getting support for the typec attributes that are being exposed here :)

OK, this is great! I really want you opinion on this. Let me try to
explain the situation.

So USB Power Delivery specification defines a set of these product
types, as I'm sure you already know. You have your hub, alternate mode
adapter, and so on. Then there are also a couple of cable types.
Originally I though that a product type simply equals a device type
(and that still feels correct to me). Each product type can have its
own "ABI" in form of the attribute files, that really should not be a
problem, right?

The problem with that was that we do not always know the product type.
For example UCSI does not supply the operating system the response to
the Discover Identity request from the partner device that contains
this information. It would mean that depending on your system, we will
claim that, let's say a hub really is a hub, or just some kind of a
default partner device. There was discussion about this back in the
day on this mailing list, and it was considered to be at least a
little bit confusing for the user.

In the end I did not propose the separate device types for each USB PD
product type (yet). Instead we only expose the header part of the
response to the Discover Identity (when we have it) because that part
is the same for all product types. So basically we have just the
"default" partner device type for everything for now.

But since now we clearly need all the identity details, not just the
header, it's good to talk about this again. Maybe even the "raw"
attribute file is not that useful in the end, and we really should
finally introduce the separate device types for each product types?

Or maybe there is a third option that I have not even thought of?

thanks,

-- 
heikki

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

* Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-11-10 11:54   ` Heikki Krogerus
  2020-11-10 12:22     ` Greg KH
@ 2020-11-12  2:40     ` Prashant Malani
  2020-11-12 12:43       ` Heikki Krogerus
  1 sibling, 1 reply; 15+ messages in thread
From: Prashant Malani @ 2020-11-12  2:40 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: linux-kernel, linux-usb, gregkh, Benson Leung

Hi Heikki,

On Tue, Nov 10, 2020 at 01:54:53PM +0200, Heikki Krogerus wrote:
> On Fri, Oct 23, 2020 at 02:43:28PM -0700, Prashant Malani wrote:
> 
> I've now come to the conclusion that this is not the correct approach.
> Instead, the whole identity, all six VDOs, should be supplied
> separately with a "raw" sysfs attribute file after all.
> 
> The three attribute files that we already have - so id_header,
> cert_stat and product - can always supply the actual VDO as is,
> regardless of the product type, so they are fine. But these new
> attribute files, product_type_vdoX, would behave differently as they
> supply different information depending on the product type. That just
> does not feel right to me.

OOI: I'd like to understand the reservations around this approach. Can't
userspace just read these and then interpret them appropriately according
to the id_header as well as PD revision (and version number) if that's exposed?
The only thing I see changing is how we name those product_type_vdoX
sysfs files, i.e product_type_vdo0 == passive_cable_vdo OR active_cable_vdo1
depending on the product type.

That said, perhaps I'm missing some aspect of this.

> 
> So lets just add the "raw" sysfs attribute file. We can think about
> extracting some other details from the product type VDOs once the
> specification has settled down a bit and we can be quite certain that
> those details will always be available.
> 
> Would this be OK to you? I think we should be able to dump the data to
> the "raw" sysfs attribute file with something like hex_dump_to_buffer().

FWIW, "raw" option SGTM (the product type VDOs can be parsed from the
buffer since the format is fixed).

> 
> thanks,
> 
> -- 
> heikki

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

* Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-11-12  2:40     ` Prashant Malani
@ 2020-11-12 12:43       ` Heikki Krogerus
  2020-11-12 16:50         ` Prashant Malani
  0 siblings, 1 reply; 15+ messages in thread
From: Heikki Krogerus @ 2020-11-12 12:43 UTC (permalink / raw)
  To: Prashant Malani; +Cc: linux-kernel, linux-usb, gregkh, Benson Leung

On Wed, Nov 11, 2020 at 06:40:55PM -0800, Prashant Malani wrote:
> Hi Heikki,
> 
> On Tue, Nov 10, 2020 at 01:54:53PM +0200, Heikki Krogerus wrote:
> > On Fri, Oct 23, 2020 at 02:43:28PM -0700, Prashant Malani wrote:
> > 
> > I've now come to the conclusion that this is not the correct approach.
> > Instead, the whole identity, all six VDOs, should be supplied
> > separately with a "raw" sysfs attribute file after all.
> > 
> > The three attribute files that we already have - so id_header,
> > cert_stat and product - can always supply the actual VDO as is,
> > regardless of the product type, so they are fine. But these new
> > attribute files, product_type_vdoX, would behave differently as they
> > supply different information depending on the product type. That just
> > does not feel right to me.
> 
> OOI: I'd like to understand the reservations around this approach. Can't
> userspace just read these and then interpret them appropriately according
> to the id_header as well as PD revision (and version number) if that's exposed?
> The only thing I see changing is how we name those product_type_vdoX
> sysfs files, i.e product_type_vdo0 == passive_cable_vdo OR active_cable_vdo1
> depending on the product type.
> 
> That said, perhaps I'm missing some aspect of this.

I don't think the userspace should have to interpret any of these
VDOs. If the userspace has to interpret the information, then the
userspace should interpret everything for the sake of consistency (so
the "raw" attribute file).

But I still think that defining separate device types for every
product type would be the best way to handle the identity. We could
then have sysfs attribute files that are specific for each product
type. It does not even matter that some of the product types are going
to be removed. We will have to handle all of them in any case,
including the ones that were removed. This way things would be much
more clear for the userspace.

The only problem IMO with the separate device types for each product
type is that we don't always have access to the Discover Identity
result. It means depending on your system we will claim the
partner device type is "default" (no identity information) or the
actual product type. That is also a bit inconsistent, but is is
acceptable? I would really like to here what Greg thinks about all
this.

> > So lets just add the "raw" sysfs attribute file. We can think about
> > extracting some other details from the product type VDOs once the
> > specification has settled down a bit and we can be quite certain that
> > those details will always be available.
> > 
> > Would this be OK to you? I think we should be able to dump the data to
> > the "raw" sysfs attribute file with something like hex_dump_to_buffer().
> 
> FWIW, "raw" option SGTM (the product type VDOs can be parsed from the
> buffer since the format is fixed).

Well, I'm starting to think that what if we just prepare patches where
we propose separate device type for every product type? Of course, if
they are OK to you?


thanks,

-- 
heikki

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

* Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-11-12 12:43       ` Heikki Krogerus
@ 2020-11-12 16:50         ` Prashant Malani
  2020-11-13 14:34           ` Heikki Krogerus
  0 siblings, 1 reply; 15+ messages in thread
From: Prashant Malani @ 2020-11-12 16:50 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Linux Kernel Mailing List, open list:USB NETWORKING DRIVERS,
	Greg Kroah-Hartman, Benson Leung

Hi Heikki,

On Thu, Nov 12, 2020 at 4:43 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Nov 11, 2020 at 06:40:55PM -0800, Prashant Malani wrote:
> > Hi Heikki,
> >
> > On Tue, Nov 10, 2020 at 01:54:53PM +0200, Heikki Krogerus wrote:
> > > On Fri, Oct 23, 2020 at 02:43:28PM -0700, Prashant Malani wrote:
> > >
> > > I've now come to the conclusion that this is not the correct approach.
> > > Instead, the whole identity, all six VDOs, should be supplied
> > > separately with a "raw" sysfs attribute file after all.
> > >
> > > The three attribute files that we already have - so id_header,
> > > cert_stat and product - can always supply the actual VDO as is,
> > > regardless of the product type, so they are fine. But these new
> > > attribute files, product_type_vdoX, would behave differently as they
> > > supply different information depending on the product type. That just
> > > does not feel right to me.
> >
> > OOI: I'd like to understand the reservations around this approach. Can't
> > userspace just read these and then interpret them appropriately according
> > to the id_header as well as PD revision (and version number) if that's exposed?
> > The only thing I see changing is how we name those product_type_vdoX
> > sysfs files, i.e product_type_vdo0 == passive_cable_vdo OR active_cable_vdo1
> > depending on the product type.
> >
> > That said, perhaps I'm missing some aspect of this.
>
> I don't think the userspace should have to interpret any of these
> VDOs. If the userspace has to interpret the information, then the
> userspace should interpret everything for the sake of consistency (so
> the "raw" attribute file).
>
> But I still think that defining separate device types for every
> product type would be the best way to handle the identity. We could
> then have sysfs attribute files that are specific for each product
> type. It does not even matter that some of the product types are going
> to be removed. We will have to handle all of them in any case,
> including the ones that were removed. This way things would be much
> more clear for the userspace.
>
> The only problem IMO with the separate device types for each product
> type is that we don't always have access to the Discover Identity
> result. It means depending on your system we will claim the
> partner device type is "default" (no identity information) or the
> actual product type. That is also a bit inconsistent, but is is
> acceptable? I would really like to here what Greg thinks about all
> this.

Thanks for explaining the rationale.
Of course, I defer to Greg & your decision on this :)

I'm yet unable to grasp what benefit userspace gets from having the kernel parse
and present this data in appropriately named sysfs files when the userspace has
enough info to do so itself.

For that reason and also because the "raw" approach is IMO a bit more
resilient to the changes
we talk about (some product type VDOs being dropped off across PD spec
uprevs [1] etc)
the "raw" proposal sounded appealing to me.

>
> > > So lets just add the "raw" sysfs attribute file. We can think about
> > > extracting some other details from the product type VDOs once the
> > > specification has settled down a bit and we can be quite certain that
> > > those details will always be available.
> > >
> > > Would this be OK to you? I think we should be able to dump the data to
> > > the "raw" sysfs attribute file with something like hex_dump_to_buffer().
> >
> > FWIW, "raw" option SGTM (the product type VDOs can be parsed from the
> > buffer since the format is fixed).
>
> Well, I'm starting to think that what if we just prepare patches where
> we propose separate device type for every product type? Of course, if
> they are OK to you?
>
SG. To clarify, will you prepare these patches?

Thanks & best regards,

-Prashant

[1]: https://lore.kernel.org/linux-usb/CANLzEkskrWXWLC+csObYwB+JUFdH+p6V6giMHtsKY-L61cTG9g@mail.gmail.com/

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

* Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-11-12 16:50         ` Prashant Malani
@ 2020-11-13 14:34           ` Heikki Krogerus
  0 siblings, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2020-11-13 14:34 UTC (permalink / raw)
  To: Prashant Malani
  Cc: Linux Kernel Mailing List, open list:USB NETWORKING DRIVERS,
	Greg Kroah-Hartman, Benson Leung

On Thu, Nov 12, 2020 at 08:50:55AM -0800, Prashant Malani wrote:
> Hi Heikki,
> 
> On Thu, Nov 12, 2020 at 4:43 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Wed, Nov 11, 2020 at 06:40:55PM -0800, Prashant Malani wrote:
> > > Hi Heikki,
> > >
> > > On Tue, Nov 10, 2020 at 01:54:53PM +0200, Heikki Krogerus wrote:
> > > > On Fri, Oct 23, 2020 at 02:43:28PM -0700, Prashant Malani wrote:
> > > >
> > > > I've now come to the conclusion that this is not the correct approach.
> > > > Instead, the whole identity, all six VDOs, should be supplied
> > > > separately with a "raw" sysfs attribute file after all.
> > > >
> > > > The three attribute files that we already have - so id_header,
> > > > cert_stat and product - can always supply the actual VDO as is,
> > > > regardless of the product type, so they are fine. But these new
> > > > attribute files, product_type_vdoX, would behave differently as they
> > > > supply different information depending on the product type. That just
> > > > does not feel right to me.
> > >
> > > OOI: I'd like to understand the reservations around this approach. Can't
> > > userspace just read these and then interpret them appropriately according
> > > to the id_header as well as PD revision (and version number) if that's exposed?
> > > The only thing I see changing is how we name those product_type_vdoX
> > > sysfs files, i.e product_type_vdo0 == passive_cable_vdo OR active_cable_vdo1
> > > depending on the product type.
> > >
> > > That said, perhaps I'm missing some aspect of this.
> >
> > I don't think the userspace should have to interpret any of these
> > VDOs. If the userspace has to interpret the information, then the
> > userspace should interpret everything for the sake of consistency (so
> > the "raw" attribute file).
> >
> > But I still think that defining separate device types for every
> > product type would be the best way to handle the identity. We could
> > then have sysfs attribute files that are specific for each product
> > type. It does not even matter that some of the product types are going
> > to be removed. We will have to handle all of them in any case,
> > including the ones that were removed. This way things would be much
> > more clear for the userspace.
> >
> > The only problem IMO with the separate device types for each product
> > type is that we don't always have access to the Discover Identity
> > result. It means depending on your system we will claim the
> > partner device type is "default" (no identity information) or the
> > actual product type. That is also a bit inconsistent, but is is
> > acceptable? I would really like to here what Greg thinks about all
> > this.
> 
> Thanks for explaining the rationale.
> Of course, I defer to Greg & your decision on this :)
> 
> I'm yet unable to grasp what benefit userspace gets from having the kernel parse
> and present this data in appropriately named sysfs files when the userspace has
> enough info to do so itself.
> 
> For that reason and also because the "raw" approach is IMO a bit more
> resilient to the changes
> we talk about (some product type VDOs being dropped off across PD spec
> uprevs [1] etc)
> the "raw" proposal sounded appealing to me.

I'm not saying no to the "raw" proposal. I would prefer that we parse
the data for the userspace in this case, but if you guys think that it
is better to just let the userspace take care of that, then I'm fine
with that too.

> > > > So lets just add the "raw" sysfs attribute file. We can think about
> > > > extracting some other details from the product type VDOs once the
> > > > specification has settled down a bit and we can be quite certain that
> > > > those details will always be available.
> > > >
> > > > Would this be OK to you? I think we should be able to dump the data to
> > > > the "raw" sysfs attribute file with something like hex_dump_to_buffer().
> > >
> > > FWIW, "raw" option SGTM (the product type VDOs can be parsed from the
> > > buffer since the format is fixed).
> >
> > Well, I'm starting to think that what if we just prepare patches where
> > we propose separate device type for every product type? Of course, if
> > they are OK to you?
> >
> SG. To clarify, will you prepare these patches?

I'll take a look at this. I can prepare something. Let's start with
the raw dump approach.


thanks,

-- 
heikki

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

* Re: [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation
  2020-10-23 21:43 [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation Prashant Malani
  2020-10-23 21:43 ` [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs Prashant Malani
  2020-11-10 10:52 ` [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation Heikki Krogerus
@ 2020-11-24 13:23 ` Heikki Krogerus
  2020-11-24 20:13   ` Prashant Malani
  2 siblings, 1 reply; 15+ messages in thread
From: Heikki Krogerus @ 2020-11-24 13:23 UTC (permalink / raw)
  To: Prashant Malani; +Cc: linux-kernel, linux-usb, gregkh

On Fri, Oct 23, 2020 at 02:43:26PM -0700, Prashant Malani wrote:
> Both partner and cable have identity VDOs. These are listed separately
> in the Documentation/ABI/testing/sysfs-class-typec. Factor these out
> into a common location to avoid the duplication.

This does not apply any more. Cany you resend these.

thanks,

> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Patch first introduced in v3.
> 
>  Documentation/ABI/testing/sysfs-class-typec | 59 ++++++---------------
>  1 file changed, 17 insertions(+), 42 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index b834671522d6..0f839fd022f1 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -134,42 +134,6 @@ Description:
>  		Shows if the partner supports USB Power Delivery communication:
>  		Valid values: yes, no
>  
> -What:		/sys/class/typec/<port>-partner>/identity/
> -Date:		April 2017
> -Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> -Description:
> -		This directory appears only if the port device driver is capable
> -		of showing the result of Discover Identity USB power delivery
> -		command. That will not always be possible even when USB power
> -		delivery is supported, for example when USB power delivery
> -		communication for the port is mostly handled in firmware. If the
> -		directory exists, it will have an attribute file for every VDO
> -		in Discover Identity command result.
> -
> -What:		/sys/class/typec/<port>-partner/identity/id_header
> -Date:		April 2017
> -Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> -Description:
> -		ID Header VDO part of Discover Identity command result. The
> -		value will show 0 until Discover Identity command result becomes
> -		available. The value can be polled.
> -
> -What:		/sys/class/typec/<port>-partner/identity/cert_stat
> -Date:		April 2017
> -Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> -Description:
> -		Cert Stat VDO part of Discover Identity command result. The
> -		value will show 0 until Discover Identity command result becomes
> -		available. The value can be polled.
> -
> -What:		/sys/class/typec/<port>-partner/identity/product
> -Date:		April 2017
> -Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> -Description:
> -		Product VDO part of Discover Identity command result. The value
> -		will show 0 until Discover Identity command result becomes
> -		available. The value can be polled.
> -
>  
>  USB Type-C cable devices (eg. /sys/class/typec/port0-cable/)
>  
> @@ -196,17 +160,28 @@ Description:
>  		- type-c
>  		- captive
>  
> -What:		/sys/class/typec/<port>-cable/identity/
> +
> +USB Type-C partner/cable Power Delivery Identity objects
> +
> +NOTE: The following attributes will be applicable to both
> +partner (e.g /sys/class/typec/port0-partner/) and
> +cable (e.g /sys/class/typec/port0-cable/) devices. Consequently, the example file
> +paths below are prefixed with "/sys/class/typec/<port>-{partner|cable}/" to
> +reflect this.
> +
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>  Description:
>  		This directory appears only if the port device driver is capable
>  		of showing the result of Discover Identity USB power delivery
>  		command. That will not always be possible even when USB power
> -		delivery is supported. If the directory exists, it will have an
> -		attribute for every VDO returned by Discover Identity command.
> +		delivery is supported, for example when USB power delivery
> +		communication for the port is mostly handled in firmware. If the
> +		directory exists, it will have an attribute file for every VDO
> +		in Discover Identity command result.
>  
> -What:		/sys/class/typec/<port>-cable/identity/id_header
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/id_header
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>  Description:
> @@ -214,7 +189,7 @@ Description:
>  		value will show 0 until Discover Identity command result becomes
>  		available. The value can be polled.
>  
> -What:		/sys/class/typec/<port>-cable/identity/cert_stat
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/cert_stat
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>  Description:
> @@ -222,7 +197,7 @@ Description:
>  		value will show 0 until Discover Identity command result becomes
>  		available. The value can be polled.
>  
> -What:		/sys/class/typec/<port>-cable/identity/product
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/product
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>  Description:
> -- 
> 2.29.0.rc1.297.gfa9743e501-goog

-- 
heikki

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

* Re: [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs
  2020-10-23 21:43 ` [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs Prashant Malani
  2020-11-04 17:49   ` Prashant Malani
  2020-11-10 11:54   ` Heikki Krogerus
@ 2020-11-24 13:28   ` Heikki Krogerus
  2 siblings, 0 replies; 15+ messages in thread
From: Heikki Krogerus @ 2020-11-24 13:28 UTC (permalink / raw)
  To: Prashant Malani; +Cc: linux-kernel, linux-usb, gregkh, Benson Leung

On Fri, Oct 23, 2020 at 02:43:28PM -0700, Prashant Malani wrote:
> 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 attributes to expose these to userspace.
> 
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

I'm now fine with this as it is, no more objections. I'll send the
"type2 attribute patch separately.

I think you need to rebase this too, but FWIW:

Reviewed-by Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes in v3:
> - Split each product type VDO into a separate attribute.
> - Changed sprintf() to sysfs_emit().
> - Changed ABI documentation based on consolidation of identity VDO
>   descriptions in the previous patch (1/2).
> 
> Changes in v2:
> - Added sysfs_notify() call for the attribute.
> - Added description for the attribute in
>   Documentation/ABI/testing/sysfs-class-typec.
> 
>  Documentation/ABI/testing/sysfs-class-typec | 24 +++++++++++++++
>  drivers/usb/typec/class.c                   | 33 +++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index 0f839fd022f1..0ac144bc5927 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -205,6 +205,30 @@ Description:
>  		will show 0 until Discover Identity command result becomes
>  		available. The value can be polled.
>  
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo1
> +Date:		October 2020
> +Contact:	Prashant Malani <pmalani@chromium.org>
> +Description:
> +		1st Product Type VDO of Discover Identity command result.
> +		The value will show 0 until Discover Identity command result becomes
> +		available and a valid Product Type VDO is returned.
> +
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo2
> +Date:		October 2020
> +Contact:	Prashant Malani <pmalani@chromium.org>
> +Description:
> +		2nd Product Type VDO of Discover Identity command result.
> +		The value will show 0 until Discover Identity command result becomes
> +		available and a valid Product Type VDO is returned.
> +
> +What:		/sys/class/typec/<port>-{partner|cable}/identity/product_type_vdo3
> +Date:		October 2020
> +Contact:	Prashant Malani <pmalani@chromium.org>
> +Description:
> +		3rd Product Type VDO of Discover Identity command result.
> +		The value will show 0 until Discover Identity command result becomes
> +		available and a valid Product Type VDO is returned.
> +
>  
>  USB Type-C port alternate mode devices.
>  
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 35eec707cb51..a2c88594b044 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -122,10 +122,40 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(product);
>  
> +static ssize_t product_type_vdo1_show(struct device *dev, struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct usb_pd_identity *id = get_pd_identity(dev);
> +
> +	return sysfs_emit(buf, "0x%08x\n", id->vdo[0]);
> +}
> +static DEVICE_ATTR_RO(product_type_vdo1);
> +
> +static ssize_t product_type_vdo2_show(struct device *dev, struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct usb_pd_identity *id = get_pd_identity(dev);
> +
> +	return sysfs_emit(buf, "0x%08x\n", id->vdo[1]);
> +}
> +static DEVICE_ATTR_RO(product_type_vdo2);
> +
> +static ssize_t product_type_vdo3_show(struct device *dev, struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct usb_pd_identity *id = get_pd_identity(dev);
> +
> +	return sysfs_emit(buf, "0x%08x\n", id->vdo[2]);
> +}
> +static DEVICE_ATTR_RO(product_type_vdo3);
> +
>  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_vdo1.attr,
> +	&dev_attr_product_type_vdo2.attr,
> +	&dev_attr_product_type_vdo3.attr,
>  	NULL
>  };
>  
> @@ -144,6 +174,9 @@ 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");
> +	sysfs_notify(&dev->kobj, "identity", "product_type_vdo1");
> +	sysfs_notify(&dev->kobj, "identity", "product_type_vdo2");
> +	sysfs_notify(&dev->kobj, "identity", "product_type_vdo3");
>  }
>  
>  /* ------------------------------------------------------------------------- */
> -- 
> 2.29.0.rc1.297.gfa9743e501-goog

thanks,

-- 
heikki

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

* Re: [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation
  2020-11-24 13:23 ` Heikki Krogerus
@ 2020-11-24 20:13   ` Prashant Malani
  0 siblings, 0 replies; 15+ messages in thread
From: Prashant Malani @ 2020-11-24 20:13 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Linux Kernel Mailing List, open list:USB NETWORKING DRIVERS,
	Greg Kroah-Hartman

Hi Heikki,

On Tue, Nov 24, 2020 at 5:23 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Fri, Oct 23, 2020 at 02:43:26PM -0700, Prashant Malani wrote:
> > Both partner and cable have identity VDOs. These are listed separately
> > in the Documentation/ABI/testing/sysfs-class-typec. Factor these out
> > into a common location to avoid the duplication.
>
> This does not apply any more. Cany you resend these.
Thanks for the heads up. Resent here [1]

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

BR,

-Prashant

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

end of thread, other threads:[~2020-11-24 20:14 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-23 21:43 [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation Prashant Malani
2020-10-23 21:43 ` [PATCH v3 2/2] usb: typec: Expose Product Type VDOs via sysfs Prashant Malani
2020-11-04 17:49   ` Prashant Malani
2020-11-06  7:36     ` Heikki Krogerus
2020-11-10 11:54   ` Heikki Krogerus
2020-11-10 12:22     ` Greg KH
2020-11-11  9:21       ` Heikki Krogerus
2020-11-12  2:40     ` Prashant Malani
2020-11-12 12:43       ` Heikki Krogerus
2020-11-12 16:50         ` Prashant Malani
2020-11-13 14:34           ` Heikki Krogerus
2020-11-24 13:28   ` Heikki Krogerus
2020-11-10 10:52 ` [PATCH v3 1/2] usb: typec: Consolidate syfs ABI documentation Heikki Krogerus
2020-11-24 13:23 ` Heikki Krogerus
2020-11-24 20:13   ` Prashant Malani

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.