[PATCH v3 0/3] usb: typec: ucsi: Adding support for UCSI 3.0

[PATCH v3 0/3] usb: typec: ucsi: Adding support for UCSI 3.0

[Abhishek Pandit-Subedi]

Hi Heikki,

This series starts the work adding UCSI 3.0 support to the UCSI driver.

There's a couple of pieces to start here:
* Add version checks and limit read size on 1.2.
* Update Connector Status and Connector Capability structures.
* Expose Partner PD revision from Capability data.

These were tested against on a 6.6 kernel running a usermode PPM against
a Realtek Evaluation board.

One additional note: there are a lot more unaligned fields in UCSI now
and the struct definitions are getting a bit out of hand. We can discuss
alternate mechanisms for defining these structs in the patch that
changes these structures.

Thanks,
Abhishek

Changes in v3:
  - Change include to asm/unaligned.h and reorder include.

Changes in v2:
  - Changed log message to DEBUG
  - Formatting changes and update macro to use brackets.
  - Fix incorrect guard condition when checking connector capability.

Abhishek Pandit-Subedi (3):
  usb: typec: ucsi: Limit read size on v1.2
  usb: typec: ucsi: Update connector cap and status
  usb: typec: ucsi: Get PD revision for partner

 drivers/usb/typec/ucsi/ucsi.c | 49 +++++++++++++++++++++++++--
 drivers/usb/typec/ucsi/ucsi.h | 64 ++++++++++++++++++++++++++++++++---
 2 files changed, 107 insertions(+), 6 deletions(-)

-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 1/3] usb: typec: ucsi: Limit read size on v1.2

[Abhishek Pandit-Subedi]

Between UCSI 1.2 and UCSI 2.0, the size of the MESSAGE_IN region was
increased from 16 to 256. In order to avoid overflowing reads for older
systems, add a mechanism to use the read UCSI version to truncate read
sizes on UCSI v1.2.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Tested on 6.6 kernel. Dmesg output from this change:
[  105.058162] ucsi_um_test ucsi_um_test_device.0: Registered UCSI
interface with version 3.0.0


(no changes since v2)

Changes in v2:
  - Changed log message to DEBUG

 drivers/usb/typec/ucsi/ucsi.c | 26 ++++++++++++++++++++++++--
 drivers/usb/typec/ucsi/ucsi.h | 11 +++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 5392ec698959..a35056ee3e96 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -36,6 +36,19 @@
  */
 #define UCSI_SWAP_TIMEOUT_MS	5000
 
+static int ucsi_read_message_in(struct ucsi *ucsi, void *buf,
+					  size_t buf_size)
+{
+	/*
+	 * Below UCSI 2.0, MESSAGE_IN was limited to 16 bytes. Truncate the
+	 * reads here.
+	 */
+	if (ucsi->version <= UCSI_VERSION_1_2)
+		buf_size = min_t(size_t, 16, buf_size);
+
+	return ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, buf, buf_size);
+}
+
 static int ucsi_acknowledge_command(struct ucsi *ucsi)
 {
 	u64 ctrl;
@@ -72,7 +85,7 @@ static int ucsi_read_error(struct ucsi *ucsi)
 	if (ret < 0)
 		return ret;
 
-	ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, &error, sizeof(error));
+	ret = ucsi_read_message_in(ucsi, &error, sizeof(error));
 	if (ret)
 		return ret;
 
@@ -170,7 +183,7 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
 	length = ret;
 
 	if (data) {
-		ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, data, size);
+		ret = ucsi_read_message_in(ucsi, data, size);
 		if (ret)
 			goto out;
 	}
@@ -1556,6 +1569,15 @@ int ucsi_register(struct ucsi *ucsi)
 	if (!ucsi->version)
 		return -ENODEV;
 
+	/*
+	 * Version format is JJ.M.N (JJ = Major version, M = Minor version,
+	 * N = sub-minor version).
+	 */
+	dev_dbg(ucsi->dev, "Registered UCSI interface with version %x.%x.%x",
+		UCSI_BCD_GET_MAJOR(ucsi->version),
+		UCSI_BCD_GET_MINOR(ucsi->version),
+		UCSI_BCD_GET_SUBMINOR(ucsi->version));
+
 	queue_delayed_work(system_long_wq, &ucsi->work, 0);
 
 	ucsi_debugfs_register(ucsi);
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 6478016d5cb8..bec920fa6b8a 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -23,6 +23,17 @@ struct dentry;
 #define UCSI_CONTROL			8
 #define UCSI_MESSAGE_IN			16
 #define UCSI_MESSAGE_OUT		32
+#define UCSIv2_MESSAGE_OUT		272
+
+/* UCSI versions */
+#define UCSI_VERSION_1_2	0x0120
+#define UCSI_VERSION_2_0	0x0200
+#define UCSI_VERSION_2_1	0x0210
+#define UCSI_VERSION_3_0	0x0300
+
+#define UCSI_BCD_GET_MAJOR(_v_)		(((_v_) >> 8) & 0xFF)
+#define UCSI_BCD_GET_MINOR(_v_)		(((_v_) >> 4) & 0x0F)
+#define UCSI_BCD_GET_SUBMINOR(_v_)	((_v_) & 0x0F)
 
 /* Command Status and Connector Change Indication (CCI) bits */
 #define UCSI_CCI_CONNECTOR(_c_)		(((_c_) & GENMASK(7, 1)) >> 1)
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 2/3] usb: typec: ucsi: Update connector cap and status

[Abhishek Pandit-Subedi]

Update the data structures for ucsi_connector_capability and
ucsi_connector_status to UCSIv3.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Connector status has several unaligned bitfields (16-bit) that result in
difficult to maintain macros. It may be better if we simply re-define
these structs as u8[] and add bit range macros to access and cast these
values.

i.e.
struct ucsi_connector_status {
  u8 raw_data[18];

...
\#define UCSI_CONSTAT_CONNECTOR_STATUS          FIELD(u16, 15, 0)
\#define UCSI_CONSTAT_BCD_PD_VER_OPER_MODE      FIELD(u16, 85, 70)
}

GET_UCSI_FIELD(con->status, UCSI_CONSTAT_CONNECTOR_STATUS);
SET_UCSI_FIELD(con->status, UCSI_CONSTAT_CONNECTOR_STATUS, 0);

I didn't find a clear example of an existing mechanism to do this. Would
love some pointers here if it already exists and some feedback from the
maintainer if this is a direction you want to go.


Changes in v3:
  - Change include to asm/unaligned.h and reorder include.

 drivers/usb/typec/ucsi/ucsi.h | 50 ++++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index bec920fa6b8a..1bae4cf8ecdc 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -10,6 +10,7 @@
 #include <linux/usb/typec.h>
 #include <linux/usb/pd.h>
 #include <linux/usb/role.h>
+#include <asm/unaligned.h>
 
 /* -------------------------------------------------------------------------- */
 
@@ -214,9 +215,29 @@ struct ucsi_connector_capability {
 #define UCSI_CONCAP_OPMODE_USB2			BIT(5)
 #define UCSI_CONCAP_OPMODE_USB3			BIT(6)
 #define UCSI_CONCAP_OPMODE_ALT_MODE		BIT(7)
-	u8 flags;
+	u32 flags;
 #define UCSI_CONCAP_FLAG_PROVIDER		BIT(0)
 #define UCSI_CONCAP_FLAG_CONSUMER		BIT(1)
+#define UCSI_CONCAP_FLAG_SWAP_TO_DFP		BIT(2)
+#define UCSI_CONCAP_FLAG_SWAP_TO_UFP		BIT(3)
+#define UCSI_CONCAP_FLAG_SWAP_TO_SRC		BIT(4)
+#define UCSI_CONCAP_FLAG_SWAP_TO_SINK		BIT(5)
+#define UCSI_CONCAP_FLAG_EX_OP_MODE(_f_) \
+	(((_f_) & GENMASK(13, 6)) >> 6)
+#define   UCSI_CONCAP_EX_OP_MODE_USB4_GEN2	BIT(0)
+#define   UCSI_CONCAP_EX_OP_MODE_EPR_SRC	BIT(1)
+#define   UCSI_CONCAP_EX_OP_MODE_EPR_SINK	BIT(2)
+#define   UCSI_CONCAP_EX_OP_MODE_USB4_GEN3	BIT(3)
+#define   UCSI_CONCAP_EX_OP_MODE_USB4_GEN4	BIT(4)
+#define UCSI_CONCAP_FLAG_MISC_CAPS(_f_) \
+	(((_f_) & GENMASK(17, 14)) >> 14)
+#define   UCSI_CONCAP_MISC_CAP_FW_UPDATE	BIT(0)
+#define   UCSI_CONCAP_MISC_CAP_SECURITY		BIT(1)
+#define UCSI_CONCAP_FLAG_REV_CURR_PROT_SUPPORT	BIT(18)
+#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) \
+	(((_f_) & GENMASK(20, 19)) >> 19)
+#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(_f_) \
+	(UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) << 8)
 } __packed;
 
 struct ucsi_altmode {
@@ -276,15 +297,36 @@ struct ucsi_connector_status {
 #define   UCSI_CONSTAT_PARTNER_TYPE_DEBUG	5
 #define   UCSI_CONSTAT_PARTNER_TYPE_AUDIO	6
 	u32 request_data_obj;
-	u8 pwr_status;
-#define UCSI_CONSTAT_BC_STATUS(_p_)		((_p_) & GENMASK(2, 0))
+
+	u8 pwr_status[3];
+#define UCSI_CONSTAT_BC_STATUS(_p_)		((_p_[0]) & GENMASK(1, 0))
 #define   UCSI_CONSTAT_BC_NOT_CHARGING		0
 #define   UCSI_CONSTAT_BC_NOMINAL_CHARGING	1
 #define   UCSI_CONSTAT_BC_SLOW_CHARGING		2
 #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING	3
-#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)	(((_p_) & GENMASK(6, 3)) >> 3)
+#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)	(((_p_[0]) & GENMASK(5, 2)) >> 2)
 #define   UCSI_CONSTAT_CAP_PWR_LOWERED		0
 #define   UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT	1
+#define UCSI_CONSTAT_PROVIDER_PD_VERSION_OPER_MODE(_p_)	\
+	((get_unaligned_le32(_p_) & GENMASK(21, 6)) >> 6)
+#define UCSI_CONSTAT_ORIENTATION(_p_)		(((_p_[2]) & GENMASK(6, 6)) >> 6)
+#define   UCSI_CONSTAT_ORIENTATION_DIRECT	0
+#define   UCSI_CONSTAT_ORIENTATION_FLIPPED	1
+#define UCSI_CONSTAT_SINK_PATH_STATUS(_p_)	(((_p_[2]) & GENMASK(7, 7)) >> 7)
+#define   UCSI_CONSTAT_SINK_PATH_DISABLED	0
+#define   UCSI_CONSTAT_SINK_PATH_ENABLED	1
+	u8 pwr_readings[9];
+#define UCSI_CONSTAT_REV_CURR_PROT_STATUS(_p_)	((_p_[0]) & 0x1)
+#define UCSI_CONSTAT_PWR_READING_VALID(_p_)	(((_p_[0]) & GENMASK(1, 1)) >> 1)
+#define UCSI_CONSTAT_CURRENT_SCALE(_p_)		(((_p_[0]) & GENMASK(4, 2)) >> 2)
+#define UCSI_CONSTAT_PEAK_CURRENT(_p_) \
+	((get_unaligned_le32(_p_) & GENMASK(20, 5)) >> 5)
+#define UCSI_CONSTAT_AVG_CURRENT(_p_) \
+	((get_unaligned_le32(&(_p_)[2]) & GENMASK(20, 5)) >> 5)
+#define UCSI_CONSTAT_VOLTAGE_SCALE(_p_) \
+	((get_unaligned_le16(&(_p_)[4]) & GENMASK(8, 5)) >> 5)
+#define UCSI_CONSTAT_VOLTAGE_READING(_p_) \
+	((get_unaligned_le32(&(_p_)[5]) & GENMASK(16, 1)) >> 1)
 } __packed;
 
 /* -------------------------------------------------------------------------- */
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v3 3/3] usb: typec: ucsi: Get PD revision for partner

[Abhishek Pandit-Subedi]

PD major revision for the port partner is described in
GET_CONNECTOR_CAPABILITY and is only valid on UCSI 2.0 and newer. Update
the pd_revision on the partner if the UCSI version is 2.0 or newer.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
$ cat /sys/class/typec/port2-partner/usb_power_delivery_revision
3.0

(no changes since v2)

Changes in v2:
  - Formatting changes and update macro to use brackets.
  - Fix incorrect guard condition when checking connector capability.

 drivers/usb/typec/ucsi/ucsi.c | 23 +++++++++++++++++++++++
 drivers/usb/typec/ucsi/ucsi.h |  3 +++
 2 files changed, 26 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index a35056ee3e96..2b7983d2fdae 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -782,6 +782,7 @@ static int ucsi_register_partner(struct ucsi_connector *con)
 	}
 
 	desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD;
+	desc.pd_revision = UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags);
 
 	partner = typec_register_partner(con->port, &desc);
 	if (IS_ERR(partner)) {
@@ -856,6 +857,27 @@ static void ucsi_partner_change(struct ucsi_connector *con)
 			con->num, u_role);
 }
 
+static int ucsi_check_connector_capability(struct ucsi_connector *con)
+{
+	u64 command;
+	int ret;
+
+	if (!con->partner || !IS_MIN_VERSION_2_0(con->ucsi))
+		return 0;
+
+	command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num);
+	ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap));
+	if (ret < 0) {
+		dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret);
+		return ret;
+	}
+
+	typec_partner_set_pd_revision(con->partner,
+		UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags));
+
+	return ret;
+}
+
 static int ucsi_check_connection(struct ucsi_connector *con)
 {
 	u8 prev_flags = con->status.flags;
@@ -925,6 +947,7 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 		if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
 			ucsi_register_partner(con);
 			ucsi_partner_task(con, ucsi_check_connection, 1, HZ);
+			ucsi_partner_task(con, ucsi_check_connector_capability, 1, HZ);
 
 			if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) ==
 			    UCSI_CONSTAT_PWR_OPMODE_PD)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 1bae4cf8ecdc..d1d0e11b0704 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -36,6 +36,9 @@ struct dentry;
 #define UCSI_BCD_GET_MINOR(_v_)		(((_v_) >> 4) & 0x0F)
 #define UCSI_BCD_GET_SUBMINOR(_v_)	((_v_) & 0x0F)
 
+#define IS_MIN_VERSION(ucsi, min_ver)	((ucsi)->version >= (min_ver))
+#define IS_MIN_VERSION_2_0(ucsi)	IS_MIN_VERSION(ucsi, UCSI_VERSION_2_0)
+
 /* Command Status and Connector Change Indication (CCI) bits */
 #define UCSI_CCI_CONNECTOR(_c_)		(((_c_) & GENMASK(7, 1)) >> 1)
 #define UCSI_CCI_LENGTH(_c_)		(((_c_) & GENMASK(15, 8)) >> 8)
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH v3 1/3] usb: typec: ucsi: Limit read size on v1.2

[Prashant Malani]

Hi Abhishek,

On Fri, Jan 26, 2024 at 10:39 AM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Between UCSI 1.2 and UCSI 2.0, the size of the MESSAGE_IN region was
> increased from 16 to 256. In order to avoid overflowing reads for older
> systems, add a mechanism to use the read UCSI version to truncate read
> sizes on UCSI v1.2.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Please pick up review tags that you've received for previous versions
(unless the patch
has changed drastically).

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

Re: [PATCH v3 2/3] usb: typec: ucsi: Update connector cap and status

[Prashant Malani]

Hi Abhishek,

On Fri, Jan 26, 2024 at 10:39 AM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Update the data structures for ucsi_connector_capability and
> ucsi_connector_status to UCSIv3.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Same comment as Patch 2/3: Please collect review tags that you've
received on previous
versions, unless the patchset is significantly different (which it
isn't in this case).
Or, if you're choosing to drop the tags, please explain why in the changelog.

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

Re: [PATCH v3 3/3] usb: typec: ucsi: Get PD revision for partner

[Prashant Malani]

Hi Abhishek,

On Fri, Jan 26, 2024 at 10:39 AM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> PD major revision for the port partner is described in
> GET_CONNECTOR_CAPABILITY and is only valid on UCSI 2.0 and newer. Update
> the pd_revision on the partner if the UCSI version is 2.0 or newer.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> $ cat /sys/class/typec/port2-partner/usb_power_delivery_revision
> 3.0
>
> (no changes since v2)
>
> Changes in v2:
>   - Formatting changes and update macro to use brackets.
>   - Fix incorrect guard condition when checking connector capability.
>
>  drivers/usb/typec/ucsi/ucsi.c | 23 +++++++++++++++++++++++
>  drivers/usb/typec/ucsi/ucsi.h |  3 +++
>  2 files changed, 26 insertions(+)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index a35056ee3e96..2b7983d2fdae 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -782,6 +782,7 @@ static int ucsi_register_partner(struct ucsi_connector *con)
>         }
>
>         desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD;
> +       desc.pd_revision = UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags);
>
>         partner = typec_register_partner(con->port, &desc);
>         if (IS_ERR(partner)) {
> @@ -856,6 +857,27 @@ static void ucsi_partner_change(struct ucsi_connector *con)
>                         con->num, u_role);
>  }
>
> +static int ucsi_check_connector_capability(struct ucsi_connector *con)
> +{
> +       u64 command;
> +       int ret;
> +
> +       if (!con->partner || !IS_MIN_VERSION_2_0(con->ucsi))

I'll reiterate my comment from a previous version, since this series
has been revv-ed a few
times since and it may have gotten lost; no need to respond to it if
you don't want to,
since I believe we left it to the maintainer(s) to decide [1]:

This macro is unnecessary. Since the version is in BCD format and we
already have the
macros for versions, just a simple comparison is enough:
         if (!con-partner || con->ucsi->version < UCSI_VERSION_2_0)
                 return 0;

I'll add that Patch 1 of this series [2] is also using the same style
for comparing version numbers.

> +               return 0;
> +
> +       command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num);
> +       ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap));
> +       if (ret < 0) {
> +               dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret);

nit: I know this is being done elsewhere in this file, but we should
avoid putting error
numbers in parentheses [3]. Perhaps something for a separate cleanup patch.

[1] https://lore.kernel.org/linux-usb/CANFp7mXP=aN8bQi4akKKcoMZE8RaCBuFnwTa5hbp0MZvZe0hYQ@mail.gmail.com/
[2] https://lore.kernel.org/linux-usb/20240126103859.v3.1.Iacf5570a66b82b73ef03daa6557e2fc0db10266a@changeid/
[3] https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages

Re: [PATCH v3 0/3] usb: typec: ucsi: Adding support for UCSI 3.0

[Mario Limonciello]

On 1/26/2024 12:39, Abhishek Pandit-Subedi wrote:
> 
> Hi Heikki,
> 
> This series starts the work adding UCSI 3.0 support to the UCSI driver.
> 
> There's a couple of pieces to start here:
> * Add version checks and limit read size on 1.2.
> * Update Connector Status and Connector Capability structures.
> * Expose Partner PD revision from Capability data.
> 
> These were tested against on a 6.6 kernel running a usermode PPM against
> a Realtek Evaluation board.
> 
> One additional note: there are a lot more unaligned fields in UCSI now
> and the struct definitions are getting a bit out of hand. We can discuss
> alternate mechanisms for defining these structs in the patch that
> changes these structures.

On the Windows side I notice that Microsoft explicitly checks the UCSI 
version to decide what data structures to use.

https://learn.microsoft.com/en-us/windows-hardware/drivers/usbcon/ucsi#ucm-ucsi-acpi-device-for-ucsi-20-and-greater

Perhaps doing something similar makes sense in Linux?

> 
> Thanks,
> Abhishek
> 
> Changes in v3:
>    - Change include to asm/unaligned.h and reorder include.
> 
> Changes in v2:
>    - Changed log message to DEBUG
>    - Formatting changes and update macro to use brackets.
>    - Fix incorrect guard condition when checking connector capability.
> 
> Abhishek Pandit-Subedi (3):
>    usb: typec: ucsi: Limit read size on v1.2
>    usb: typec: ucsi: Update connector cap and status
>    usb: typec: ucsi: Get PD revision for partner
> 
>   drivers/usb/typec/ucsi/ucsi.c | 49 +++++++++++++++++++++++++--
>   drivers/usb/typec/ucsi/ucsi.h | 64 ++++++++++++++++++++++++++++++++---
>   2 files changed, 107 insertions(+), 6 deletions(-)
> 

Re: [PATCH v3 1/3] usb: typec: ucsi: Limit read size on v1.2

[Heikki Krogerus]

On Fri, Jan 26, 2024 at 10:39:07AM -0800, Abhishek Pandit-Subedi wrote:
> Between UCSI 1.2 and UCSI 2.0, the size of the MESSAGE_IN region was
> increased from 16 to 256. In order to avoid overflowing reads for older
> systems, add a mechanism to use the read UCSI version to truncate read
> sizes on UCSI v1.2.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

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

> ---
> Tested on 6.6 kernel. Dmesg output from this change:
> [  105.058162] ucsi_um_test ucsi_um_test_device.0: Registered UCSI
> interface with version 3.0.0
> 
> 
> (no changes since v2)
> 
> Changes in v2:
>   - Changed log message to DEBUG
> 
>  drivers/usb/typec/ucsi/ucsi.c | 26 ++++++++++++++++++++++++--
>  drivers/usb/typec/ucsi/ucsi.h | 11 +++++++++++
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 5392ec698959..a35056ee3e96 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -36,6 +36,19 @@
>   */
>  #define UCSI_SWAP_TIMEOUT_MS	5000
>  
> +static int ucsi_read_message_in(struct ucsi *ucsi, void *buf,
> +					  size_t buf_size)
> +{
> +	/*
> +	 * Below UCSI 2.0, MESSAGE_IN was limited to 16 bytes. Truncate the
> +	 * reads here.
> +	 */
> +	if (ucsi->version <= UCSI_VERSION_1_2)
> +		buf_size = min_t(size_t, 16, buf_size);
> +
> +	return ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, buf, buf_size);
> +}
> +
>  static int ucsi_acknowledge_command(struct ucsi *ucsi)
>  {
>  	u64 ctrl;
> @@ -72,7 +85,7 @@ static int ucsi_read_error(struct ucsi *ucsi)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, &error, sizeof(error));
> +	ret = ucsi_read_message_in(ucsi, &error, sizeof(error));
>  	if (ret)
>  		return ret;
>  
> @@ -170,7 +183,7 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
>  	length = ret;
>  
>  	if (data) {
> -		ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, data, size);
> +		ret = ucsi_read_message_in(ucsi, data, size);
>  		if (ret)
>  			goto out;
>  	}
> @@ -1556,6 +1569,15 @@ int ucsi_register(struct ucsi *ucsi)
>  	if (!ucsi->version)
>  		return -ENODEV;
>  
> +	/*
> +	 * Version format is JJ.M.N (JJ = Major version, M = Minor version,
> +	 * N = sub-minor version).
> +	 */
> +	dev_dbg(ucsi->dev, "Registered UCSI interface with version %x.%x.%x",
> +		UCSI_BCD_GET_MAJOR(ucsi->version),
> +		UCSI_BCD_GET_MINOR(ucsi->version),
> +		UCSI_BCD_GET_SUBMINOR(ucsi->version));
> +
>  	queue_delayed_work(system_long_wq, &ucsi->work, 0);
>  
>  	ucsi_debugfs_register(ucsi);
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 6478016d5cb8..bec920fa6b8a 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -23,6 +23,17 @@ struct dentry;
>  #define UCSI_CONTROL			8
>  #define UCSI_MESSAGE_IN			16
>  #define UCSI_MESSAGE_OUT		32
> +#define UCSIv2_MESSAGE_OUT		272
> +
> +/* UCSI versions */
> +#define UCSI_VERSION_1_2	0x0120
> +#define UCSI_VERSION_2_0	0x0200
> +#define UCSI_VERSION_2_1	0x0210
> +#define UCSI_VERSION_3_0	0x0300
> +
> +#define UCSI_BCD_GET_MAJOR(_v_)		(((_v_) >> 8) & 0xFF)
> +#define UCSI_BCD_GET_MINOR(_v_)		(((_v_) >> 4) & 0x0F)
> +#define UCSI_BCD_GET_SUBMINOR(_v_)	((_v_) & 0x0F)
>  
>  /* Command Status and Connector Change Indication (CCI) bits */
>  #define UCSI_CCI_CONNECTOR(_c_)		(((_c_) & GENMASK(7, 1)) >> 1)
> -- 
> 2.43.0.429.g432eaa2c6b-goog

-- 
heikki

Re: [PATCH v3 2/3] usb: typec: ucsi: Update connector cap and status

[Heikki Krogerus]

On Fri, Jan 26, 2024 at 10:39:08AM -0800, Abhishek Pandit-Subedi wrote:
> Update the data structures for ucsi_connector_capability and
> ucsi_connector_status to UCSIv3.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

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

> ---
> Connector status has several unaligned bitfields (16-bit) that result in
> difficult to maintain macros. It may be better if we simply re-define
> these structs as u8[] and add bit range macros to access and cast these
> values.
> 
> i.e.
> struct ucsi_connector_status {
>   u8 raw_data[18];
> 
> ...
> \#define UCSI_CONSTAT_CONNECTOR_STATUS          FIELD(u16, 15, 0)
> \#define UCSI_CONSTAT_BCD_PD_VER_OPER_MODE      FIELD(u16, 85, 70)
> }
> 
> GET_UCSI_FIELD(con->status, UCSI_CONSTAT_CONNECTOR_STATUS);
> SET_UCSI_FIELD(con->status, UCSI_CONSTAT_CONNECTOR_STATUS, 0);
> 
> I didn't find a clear example of an existing mechanism to do this. Would
> love some pointers here if it already exists and some feedback from the
> maintainer if this is a direction you want to go.
> 
> 
> Changes in v3:
>   - Change include to asm/unaligned.h and reorder include.
> 
>  drivers/usb/typec/ucsi/ucsi.h | 50 ++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index bec920fa6b8a..1bae4cf8ecdc 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -10,6 +10,7 @@
>  #include <linux/usb/typec.h>
>  #include <linux/usb/pd.h>
>  #include <linux/usb/role.h>
> +#include <asm/unaligned.h>
>  
>  /* -------------------------------------------------------------------------- */
>  
> @@ -214,9 +215,29 @@ struct ucsi_connector_capability {
>  #define UCSI_CONCAP_OPMODE_USB2			BIT(5)
>  #define UCSI_CONCAP_OPMODE_USB3			BIT(6)
>  #define UCSI_CONCAP_OPMODE_ALT_MODE		BIT(7)
> -	u8 flags;
> +	u32 flags;
>  #define UCSI_CONCAP_FLAG_PROVIDER		BIT(0)
>  #define UCSI_CONCAP_FLAG_CONSUMER		BIT(1)
> +#define UCSI_CONCAP_FLAG_SWAP_TO_DFP		BIT(2)
> +#define UCSI_CONCAP_FLAG_SWAP_TO_UFP		BIT(3)
> +#define UCSI_CONCAP_FLAG_SWAP_TO_SRC		BIT(4)
> +#define UCSI_CONCAP_FLAG_SWAP_TO_SINK		BIT(5)
> +#define UCSI_CONCAP_FLAG_EX_OP_MODE(_f_) \
> +	(((_f_) & GENMASK(13, 6)) >> 6)
> +#define   UCSI_CONCAP_EX_OP_MODE_USB4_GEN2	BIT(0)
> +#define   UCSI_CONCAP_EX_OP_MODE_EPR_SRC	BIT(1)
> +#define   UCSI_CONCAP_EX_OP_MODE_EPR_SINK	BIT(2)
> +#define   UCSI_CONCAP_EX_OP_MODE_USB4_GEN3	BIT(3)
> +#define   UCSI_CONCAP_EX_OP_MODE_USB4_GEN4	BIT(4)
> +#define UCSI_CONCAP_FLAG_MISC_CAPS(_f_) \
> +	(((_f_) & GENMASK(17, 14)) >> 14)
> +#define   UCSI_CONCAP_MISC_CAP_FW_UPDATE	BIT(0)
> +#define   UCSI_CONCAP_MISC_CAP_SECURITY		BIT(1)
> +#define UCSI_CONCAP_FLAG_REV_CURR_PROT_SUPPORT	BIT(18)
> +#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) \
> +	(((_f_) & GENMASK(20, 19)) >> 19)
> +#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(_f_) \
> +	(UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) << 8)
>  } __packed;
>  
>  struct ucsi_altmode {
> @@ -276,15 +297,36 @@ struct ucsi_connector_status {
>  #define   UCSI_CONSTAT_PARTNER_TYPE_DEBUG	5
>  #define   UCSI_CONSTAT_PARTNER_TYPE_AUDIO	6
>  	u32 request_data_obj;
> -	u8 pwr_status;
> -#define UCSI_CONSTAT_BC_STATUS(_p_)		((_p_) & GENMASK(2, 0))
> +
> +	u8 pwr_status[3];
> +#define UCSI_CONSTAT_BC_STATUS(_p_)		((_p_[0]) & GENMASK(1, 0))
>  #define   UCSI_CONSTAT_BC_NOT_CHARGING		0
>  #define   UCSI_CONSTAT_BC_NOMINAL_CHARGING	1
>  #define   UCSI_CONSTAT_BC_SLOW_CHARGING		2
>  #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING	3
> -#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)	(((_p_) & GENMASK(6, 3)) >> 3)
> +#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)	(((_p_[0]) & GENMASK(5, 2)) >> 2)
>  #define   UCSI_CONSTAT_CAP_PWR_LOWERED		0
>  #define   UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT	1
> +#define UCSI_CONSTAT_PROVIDER_PD_VERSION_OPER_MODE(_p_)	\
> +	((get_unaligned_le32(_p_) & GENMASK(21, 6)) >> 6)
> +#define UCSI_CONSTAT_ORIENTATION(_p_)		(((_p_[2]) & GENMASK(6, 6)) >> 6)
> +#define   UCSI_CONSTAT_ORIENTATION_DIRECT	0
> +#define   UCSI_CONSTAT_ORIENTATION_FLIPPED	1
> +#define UCSI_CONSTAT_SINK_PATH_STATUS(_p_)	(((_p_[2]) & GENMASK(7, 7)) >> 7)
> +#define   UCSI_CONSTAT_SINK_PATH_DISABLED	0
> +#define   UCSI_CONSTAT_SINK_PATH_ENABLED	1
> +	u8 pwr_readings[9];
> +#define UCSI_CONSTAT_REV_CURR_PROT_STATUS(_p_)	((_p_[0]) & 0x1)
> +#define UCSI_CONSTAT_PWR_READING_VALID(_p_)	(((_p_[0]) & GENMASK(1, 1)) >> 1)
> +#define UCSI_CONSTAT_CURRENT_SCALE(_p_)		(((_p_[0]) & GENMASK(4, 2)) >> 2)
> +#define UCSI_CONSTAT_PEAK_CURRENT(_p_) \
> +	((get_unaligned_le32(_p_) & GENMASK(20, 5)) >> 5)
> +#define UCSI_CONSTAT_AVG_CURRENT(_p_) \
> +	((get_unaligned_le32(&(_p_)[2]) & GENMASK(20, 5)) >> 5)
> +#define UCSI_CONSTAT_VOLTAGE_SCALE(_p_) \
> +	((get_unaligned_le16(&(_p_)[4]) & GENMASK(8, 5)) >> 5)
> +#define UCSI_CONSTAT_VOLTAGE_READING(_p_) \
> +	((get_unaligned_le32(&(_p_)[5]) & GENMASK(16, 1)) >> 1)
>  } __packed;
>  
>  /* -------------------------------------------------------------------------- */
> -- 
> 2.43.0.429.g432eaa2c6b-goog

-- 
heikki

Re: [PATCH v3 0/3] usb: typec: ucsi: Adding support for UCSI 3.0

[Abhishek Pandit-Subedi]

On Sun, Jan 28, 2024 at 8:06 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 1/26/2024 12:39, Abhishek Pandit-Subedi wrote:
> >
> > Hi Heikki,
> >
> > This series starts the work adding UCSI 3.0 support to the UCSI driver.
> >
> > There's a couple of pieces to start here:
> > * Add version checks and limit read size on 1.2.
> > * Update Connector Status and Connector Capability structures.
> > * Expose Partner PD revision from Capability data.
> >
> > These were tested against on a 6.6 kernel running a usermode PPM against
> > a Realtek Evaluation board.
> >
> > One additional note: there are a lot more unaligned fields in UCSI now
> > and the struct definitions are getting a bit out of hand. We can discuss
> > alternate mechanisms for defining these structs in the patch that
> > changes these structures.
>
> On the Windows side I notice that Microsoft explicitly checks the UCSI
> version to decide what data structures to use.
>
> https://learn.microsoft.com/en-us/windows-hardware/drivers/usbcon/ucsi#ucm-ucsi-acpi-device-for-ucsi-20-and-greater
>
> Perhaps doing something similar makes sense in Linux?

That probably belongs in `ucsi_acpi` and would be good to add there. I
don't have a Windows device reporting a UCSI version 2.0 but wanting
1.2 so I can't add or test this patch.

>
> >
> > Thanks,
> > Abhishek
> >
> > Changes in v3:
> >    - Change include to asm/unaligned.h and reorder include.
> >
> > Changes in v2:
> >    - Changed log message to DEBUG
> >    - Formatting changes and update macro to use brackets.
> >    - Fix incorrect guard condition when checking connector capability.
> >
> > Abhishek Pandit-Subedi (3):
> >    usb: typec: ucsi: Limit read size on v1.2
> >    usb: typec: ucsi: Update connector cap and status
> >    usb: typec: ucsi: Get PD revision for partner
> >
> >   drivers/usb/typec/ucsi/ucsi.c | 49 +++++++++++++++++++++++++--
> >   drivers/usb/typec/ucsi/ucsi.h | 64 ++++++++++++++++++++++++++++++++---
> >   2 files changed, 107 insertions(+), 6 deletions(-)
> >
>

Re: [PATCH v3 3/3] usb: typec: ucsi: Get PD revision for partner

[Abhishek Pandit-Subedi]

Hi Heikki,

Friendly ping to review this patch (I see you added Reviewed-by to the
other two in this series).

Thanks,
Abhishek

On Fri, Jan 26, 2024 at 12:25 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Abhishek,
>
> On Fri, Jan 26, 2024 at 10:39 AM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > PD major revision for the port partner is described in
> > GET_CONNECTOR_CAPABILITY and is only valid on UCSI 2.0 and newer. Update
> > the pd_revision on the partner if the UCSI version is 2.0 or newer.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> > $ cat /sys/class/typec/port2-partner/usb_power_delivery_revision
> > 3.0
> >
> > (no changes since v2)
> >
> > Changes in v2:
> >   - Formatting changes and update macro to use brackets.
> >   - Fix incorrect guard condition when checking connector capability.
> >
> >  drivers/usb/typec/ucsi/ucsi.c | 23 +++++++++++++++++++++++
> >  drivers/usb/typec/ucsi/ucsi.h |  3 +++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index a35056ee3e96..2b7983d2fdae 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -782,6 +782,7 @@ static int ucsi_register_partner(struct ucsi_connector *con)
> >         }
> >
> >         desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD;
> > +       desc.pd_revision = UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags);
> >
> >         partner = typec_register_partner(con->port, &desc);
> >         if (IS_ERR(partner)) {
> > @@ -856,6 +857,27 @@ static void ucsi_partner_change(struct ucsi_connector *con)
> >                         con->num, u_role);
> >  }
> >
> > +static int ucsi_check_connector_capability(struct ucsi_connector *con)
> > +{
> > +       u64 command;
> > +       int ret;
> > +
> > +       if (!con->partner || !IS_MIN_VERSION_2_0(con->ucsi))
>
> I'll reiterate my comment from a previous version, since this series
> has been revv-ed a few
> times since and it may have gotten lost; no need to respond to it if
> you don't want to,
> since I believe we left it to the maintainer(s) to decide [1]:
>
> This macro is unnecessary. Since the version is in BCD format and we
> already have the
> macros for versions, just a simple comparison is enough:
>          if (!con-partner || con->ucsi->version < UCSI_VERSION_2_0)
>                  return 0;
>
> I'll add that Patch 1 of this series [2] is also using the same style
> for comparing version numbers.
>
> > +               return 0;
> > +
> > +       command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num);
> > +       ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap));
> > +       if (ret < 0) {
> > +               dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret);
>
> nit: I know this is being done elsewhere in this file, but we should
> avoid putting error
> numbers in parentheses [3]. Perhaps something for a separate cleanup patch.
>
> [1] https://lore.kernel.org/linux-usb/CANFp7mXP=aN8bQi4akKKcoMZE8RaCBuFnwTa5hbp0MZvZe0hYQ@mail.gmail.com/
> [2] https://lore.kernel.org/linux-usb/20240126103859.v3.1.Iacf5570a66b82b73ef03daa6557e2fc0db10266a@changeid/
> [3] https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages

Re: [PATCH v3 3/3] usb: typec: ucsi: Get PD revision for partner

[Heikki Krogerus]

Hi Abhishek,

On Mon, Feb 05, 2024 at 02:05:38PM -0800, Abhishek Pandit-Subedi wrote:
> Hi Heikki,
> 
> Friendly ping to review this patch (I see you added Reviewed-by to the
> other two in this series).

I think Prashant said that he prefers macros with those version checks,
and I kinda agree. But I'll leave this to you to decide. I think
that's also something that can be improved later.

> On Fri, Jan 26, 2024 at 12:25 PM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Hi Abhishek,
> >
> > On Fri, Jan 26, 2024 at 10:39 AM Abhishek Pandit-Subedi
> > <abhishekpandit@chromium.org> wrote:
> > >
> > > PD major revision for the port partner is described in
> > > GET_CONNECTOR_CAPABILITY and is only valid on UCSI 2.0 and newer. Update
> > > the pd_revision on the partner if the UCSI version is 2.0 or newer.
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

So this okay by me:

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

> > > ---
> > > $ cat /sys/class/typec/port2-partner/usb_power_delivery_revision
> > > 3.0
> > >
> > > (no changes since v2)
> > >
> > > Changes in v2:
> > >   - Formatting changes and update macro to use brackets.
> > >   - Fix incorrect guard condition when checking connector capability.
> > >
> > >  drivers/usb/typec/ucsi/ucsi.c | 23 +++++++++++++++++++++++
> > >  drivers/usb/typec/ucsi/ucsi.h |  3 +++
> > >  2 files changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > index a35056ee3e96..2b7983d2fdae 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > @@ -782,6 +782,7 @@ static int ucsi_register_partner(struct ucsi_connector *con)
> > >         }
> > >
> > >         desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD;
> > > +       desc.pd_revision = UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags);
> > >
> > >         partner = typec_register_partner(con->port, &desc);
> > >         if (IS_ERR(partner)) {
> > > @@ -856,6 +857,27 @@ static void ucsi_partner_change(struct ucsi_connector *con)
> > >                         con->num, u_role);
> > >  }
> > >
> > > +static int ucsi_check_connector_capability(struct ucsi_connector *con)
> > > +{
> > > +       u64 command;
> > > +       int ret;
> > > +
> > > +       if (!con->partner || !IS_MIN_VERSION_2_0(con->ucsi))
> >
> > I'll reiterate my comment from a previous version, since this series
> > has been revv-ed a few
> > times since and it may have gotten lost; no need to respond to it if
> > you don't want to,
> > since I believe we left it to the maintainer(s) to decide [1]:
> >
> > This macro is unnecessary. Since the version is in BCD format and we
> > already have the
> > macros for versions, just a simple comparison is enough:
> >          if (!con-partner || con->ucsi->version < UCSI_VERSION_2_0)
> >                  return 0;
> >
> > I'll add that Patch 1 of this series [2] is also using the same style
> > for comparing version numbers.
> >
> > > +               return 0;
> > > +
> > > +       command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num);
> > > +       ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap));
> > > +       if (ret < 0) {
> > > +               dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret);
> >
> > nit: I know this is being done elsewhere in this file, but we should
> > avoid putting error
> > numbers in parentheses [3]. Perhaps something for a separate cleanup patch.
> > 
> > [1] https://lore.kernel.org/linux-usb/CANFp7mXP=aN8bQi4akKKcoMZE8RaCBuFnwTa5hbp0MZvZe0hYQ@mail.gmail.com/
> > [2] https://lore.kernel.org/linux-usb/20240126103859.v3.1.Iacf5570a66b82b73ef03daa6557e2fc0db10266a@changeid/
> > [3] https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages

thanks,

-- 
heikki

Re: [PATCH v3 3/3] usb: typec: ucsi: Get PD revision for partner

[Prashant Malani]

On Tue, Feb 6, 2024 at 2:18 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Abhishek,
>
> On Mon, Feb 05, 2024 at 02:05:38PM -0800, Abhishek Pandit-Subedi wrote:
> > Hi Heikki,
> >
> > Friendly ping to review this patch (I see you added Reviewed-by to the
> > other two in this series).
>
> I think Prashant said that he prefers macros with those version checks,
> and I kinda agree. But I'll leave this to you to decide. I think
> that's also something that can be improved later.

Yeah, the macro strikes me as unnecessary here. Anyhow, for the rest of it:

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