[PATCH v2 0/3] usb: typec: ucsi: Add support for SET_PDOS command

[PATCH v2 0/3] usb: typec: ucsi: Add support for SET_PDOS command

[Pooja Katiyar]

This series implements support for UCSI SET_PDOS command. It provides
interface to send message out data structure and update source or sink
capabilities PDOs on a connector over debugfs interface.

Pooja Katiyar (3):
  usb: typec: ucsi: Add support for message out data structure
  usb: typec: ucsi: Enable debugfs for message_out data structure
  usb: typec: ucsi: Add support for SET_PDOS command

 drivers/usb/typec/ucsi/cros_ec_ucsi.c |  4 +-
 drivers/usb/typec/ucsi/debugfs.c      | 33 +++++++++++++-
 drivers/usb/typec/ucsi/displayport.c  |  6 +--
 drivers/usb/typec/ucsi/ucsi.c         | 64 ++++++++++++++++-----------
 drivers/usb/typec/ucsi/ucsi.h         | 14 ++++--
 drivers/usb/typec/ucsi/ucsi_acpi.c    | 20 ++++++++-
 drivers/usb/typec/ucsi/ucsi_ccg.c     |  4 +-
 7 files changed, 105 insertions(+), 40 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Pooja Katiyar]

Add support for updating message out data structure for UCSI ACPI
interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
LPM Firmware Update.

Additionally, update ucsi_send_command to accept message_out data
and .sync_control function to pass message_out data to
write_message_out function if the command is UCSI_SET_PDOS.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
---
Changelog v2:
- Moved write_message_out operation to .sync_control.
- Updated ucsi_send_command to accept message_out data.

 drivers/usb/typec/ucsi/cros_ec_ucsi.c |  4 +-
 drivers/usb/typec/ucsi/debugfs.c      |  4 +-
 drivers/usb/typec/ucsi/displayport.c  |  6 +--
 drivers/usb/typec/ucsi/ucsi.c         | 64 ++++++++++++++++-----------
 drivers/usb/typec/ucsi/ucsi.h         | 10 +++--
 drivers/usb/typec/ucsi/ucsi_acpi.c    | 20 ++++++++-
 drivers/usb/typec/ucsi/ucsi_ccg.c     |  4 +-
 7 files changed, 72 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
index 4ec1c6d22310..53caa354461b 100644
--- a/drivers/usb/typec/ucsi/cros_ec_ucsi.c
+++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
@@ -106,12 +106,12 @@ static int cros_ucsi_async_control(struct ucsi *ucsi, u64 cmd)
 }
 
 static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd, u32 *cci,
-				  void *data, size_t size)
+				  void *data, size_t size, void *message_out)
 {
 	struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
 	int ret;
 
-	ret = ucsi_sync_control_common(ucsi, cmd, cci, data, size);
+	ret = ucsi_sync_control_common(ucsi, cmd, cci, data, size, message_out);
 	switch (ret) {
 	case -EBUSY:
 		/* EC may return -EBUSY if CCI.busy is set.
diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c
index 92ebf1a2defd..c3c349e712d0 100644
--- a/drivers/usb/typec/ucsi/debugfs.c
+++ b/drivers/usb/typec/ucsi/debugfs.c
@@ -35,7 +35,7 @@ static int ucsi_cmd(void *data, u64 val)
 	case UCSI_SET_SINK_PATH:
 	case UCSI_SET_NEW_CAM:
 	case UCSI_SET_USB:
-		ret = ucsi_send_command(ucsi, val, NULL, 0);
+		ret = ucsi_send_command(ucsi, val, NULL, 0, NULL);
 		break;
 	case UCSI_GET_CAPABILITY:
 	case UCSI_GET_CONNECTOR_CAPABILITY:
@@ -52,7 +52,7 @@ static int ucsi_cmd(void *data, u64 val)
 	case UCSI_GET_LPM_PPM_INFO:
 		ret = ucsi_send_command(ucsi, val,
 					&ucsi->debugfs->response,
-					sizeof(ucsi->debugfs->response));
+					sizeof(ucsi->debugfs->response), NULL);
 		break;
 	default:
 		ret = -EOPNOTSUPP;
diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index 8aae80b457d7..93912719d915 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -67,7 +67,7 @@ static int ucsi_displayport_enter(struct typec_altmode *alt, u32 *vdo)
 	}
 
 	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(dp->con->num);
-	ret = ucsi_send_command(ucsi, command, &cur, sizeof(cur));
+	ret = ucsi_send_command(ucsi, command, &cur, sizeof(cur), NULL);
 	if (ret < 0) {
 		if (ucsi->version > 0x0100)
 			goto err_unlock;
@@ -126,7 +126,7 @@ static int ucsi_displayport_exit(struct typec_altmode *alt)
 	}
 
 	command = UCSI_CMD_SET_NEW_CAM(dp->con->num, 0, dp->offset, 0);
-	ret = ucsi_send_command(dp->con->ucsi, command, NULL, 0);
+	ret = ucsi_send_command(dp->con->ucsi, command, NULL, 0, NULL);
 	if (ret < 0)
 		goto out_unlock;
 
@@ -193,7 +193,7 @@ static int ucsi_displayport_configure(struct ucsi_dp *dp)
 
 	command = UCSI_CMD_SET_NEW_CAM(dp->con->num, 1, dp->offset, pins);
 
-	return ucsi_send_command(dp->con->ucsi, command, NULL, 0);
+	return ucsi_send_command(dp->con->ucsi, command, NULL, 0, NULL);
 }
 
 static int ucsi_displayport_vdm(struct typec_altmode *alt,
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 01ce858a1a2b..c8410c88b45b 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -56,7 +56,7 @@ void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
 EXPORT_SYMBOL_GPL(ucsi_notify_common);
 
 int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
-			     void *data, size_t size)
+			     void *data, size_t size, void *message_out)
 {
 	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
 	int ret;
@@ -68,6 +68,18 @@ int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
 
 	reinit_completion(&ucsi->complete);
 
+	if (message_out) {
+		if (!ucsi->ops->write_message_out) {
+			ret = -EOPNOTSUPP;
+			goto out_clear_bit;
+		}
+
+		ret = ucsi->ops->write_message_out(ucsi, message_out,
+						   UCSI_COMMAND_DATA_LEN(command));
+		if (ret)
+			goto out_clear_bit;
+	}
+
 	ret = ucsi->ops->async_control(ucsi, command);
 	if (ret)
 		goto out_clear_bit;
@@ -103,11 +115,11 @@ static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
 		ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
 	}
 
-	return ucsi->ops->sync_control(ucsi, ctrl, NULL, NULL, 0);
+	return ucsi->ops->sync_control(ucsi, ctrl, NULL, NULL, 0, NULL);
 }
 
 static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
-			    void *data, size_t size, bool conn_ack)
+			    void *data, size_t size, bool conn_ack, void *message_out)
 {
 	int ret, err;
 
@@ -116,10 +128,10 @@ static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
 	if (size > UCSI_MAX_DATA_LENGTH(ucsi))
 		return -EINVAL;
 
-	ret = ucsi->ops->sync_control(ucsi, command, cci, data, size);
+	ret = ucsi->ops->sync_control(ucsi, command, cci, data, size, message_out);
 
 	if (*cci & UCSI_CCI_BUSY)
-		return ucsi_run_command(ucsi, UCSI_CANCEL, cci, NULL, 0, false) ?: -EBUSY;
+		return ucsi_run_command(ucsi, UCSI_CANCEL, cci, NULL, 0, false, NULL) ?: -EBUSY;
 	if (ret)
 		return ret;
 
@@ -151,7 +163,7 @@ static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
 	int ret;
 
 	command = UCSI_GET_ERROR_STATUS | UCSI_CONNECTOR_NUMBER(connector_num);
-	ret = ucsi_run_command(ucsi, command, &cci, &error, sizeof(error), false);
+	ret = ucsi_run_command(ucsi, command, &cci, &error, sizeof(error), false, NULL);
 	if (ret < 0)
 		return ret;
 
@@ -201,7 +213,7 @@ static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
 }
 
 static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
-				    void *data, size_t size, bool conn_ack)
+				    void *data, size_t size, bool conn_ack, void *message_out)
 {
 	u8 connector_num;
 	u32 cci;
@@ -229,7 +241,7 @@ static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
 
 	mutex_lock(&ucsi->ppm_lock);
 
-	ret = ucsi_run_command(ucsi, cmd, &cci, data, size, conn_ack);
+	ret = ucsi_run_command(ucsi, cmd, &cci, data, size, conn_ack, message_out);
 
 	if (cci & UCSI_CCI_ERROR)
 		ret = ucsi_read_error(ucsi, connector_num);
@@ -239,9 +251,9 @@ static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
 }
 
 int ucsi_send_command(struct ucsi *ucsi, u64 command,
-		      void *data, size_t size)
+		      void *data, size_t size, void *message_out)
 {
-	return ucsi_send_command_common(ucsi, command, data, size, false);
+	return ucsi_send_command_common(ucsi, command, data, size, false, message_out);
 }
 EXPORT_SYMBOL_GPL(ucsi_send_command);
 
@@ -319,7 +331,7 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
 	int i;
 
 	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(con->ucsi, command, &cur, sizeof(cur));
+	ret = ucsi_send_command(con->ucsi, command, &cur, sizeof(cur), NULL);
 	if (ret < 0) {
 		if (con->ucsi->version > 0x0100) {
 			dev_err(con->ucsi->dev,
@@ -510,7 +522,7 @@ ucsi_register_altmodes_nvidia(struct ucsi_connector *con, u8 recipient)
 		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
 		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
 		command |= UCSI_GET_ALTMODE_OFFSET(i);
-		len = ucsi_send_command(con->ucsi, command, &alt, sizeof(alt));
+		len = ucsi_send_command(con->ucsi, command, &alt, sizeof(alt), NULL);
 		/*
 		 * We are collecting all altmodes first and then registering.
 		 * Some type-C device will return zero length data beyond last
@@ -587,7 +599,7 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
 		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
 		command |= UCSI_GET_ALTMODE_OFFSET(i);
-		len = ucsi_send_command(con->ucsi, command, alt, sizeof(alt));
+		len = ucsi_send_command(con->ucsi, command, alt, sizeof(alt), NULL);
 		if (len == -EBUSY)
 			continue;
 		if (len <= 0)
@@ -660,7 +672,7 @@ static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack)
 			  UCSI_MAX_DATA_LENGTH(con->ucsi));
 	int ret;
 
-	ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack);
+	ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack, NULL);
 
 	return ret < 0 ? ret : 0;
 }
@@ -684,7 +696,7 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
 	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
 	command |= is_source(role) ? UCSI_GET_PDOS_SRC_PDOS : 0;
 	ret = ucsi_send_command(ucsi, command, pdos + offset,
-				num_pdos * sizeof(u32));
+				num_pdos * sizeof(u32), NULL);
 	if (ret < 0 && ret != -ETIMEDOUT)
 		dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
 
@@ -771,7 +783,7 @@ static int ucsi_get_pd_message(struct ucsi_connector *con, u8 recipient,
 		command |= UCSI_GET_PD_MESSAGE_BYTES(len);
 		command |= UCSI_GET_PD_MESSAGE_TYPE(type);
 
-		ret = ucsi_send_command(con->ucsi, command, data + offset, len);
+		ret = ucsi_send_command(con->ucsi, command, data + offset, len, NULL);
 		if (ret < 0)
 			return ret;
 	}
@@ -936,7 +948,7 @@ static int ucsi_register_cable(struct ucsi_connector *con)
 	int ret;
 
 	command = UCSI_GET_CABLE_PROPERTY | UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(con->ucsi, command, &cable_prop, sizeof(cable_prop));
+	ret = ucsi_send_command(con->ucsi, command, &cable_prop, sizeof(cable_prop), NULL);
 	if (ret < 0) {
 		dev_err(con->ucsi->dev, "GET_CABLE_PROPERTY failed (%d)\n", ret);
 		return ret;
@@ -997,7 +1009,7 @@ static int ucsi_check_connector_capability(struct ucsi_connector *con)
 		return 0;
 
 	command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap));
+	ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap), NULL);
 	if (ret < 0) {
 		dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret);
 		return ret;
@@ -1337,7 +1349,7 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard)
 	else if (con->ucsi->version >= UCSI_VERSION_2_0)
 		command |= hard ? 0 : UCSI_CONNECTOR_RESET_DATA_VER_2_0;
 
-	return ucsi_send_command(con->ucsi, command, NULL, 0);
+	return ucsi_send_command(con->ucsi, command, NULL, 0, NULL);
 }
 
 static int ucsi_reset_ppm(struct ucsi *ucsi)
@@ -1418,7 +1430,7 @@ static int ucsi_role_cmd(struct ucsi_connector *con, u64 command)
 {
 	int ret;
 
-	ret = ucsi_send_command(con->ucsi, command, NULL, 0);
+	ret = ucsi_send_command(con->ucsi, command, NULL, 0, NULL);
 	if (ret == -ETIMEDOUT) {
 		u64 c;
 
@@ -1426,7 +1438,7 @@ static int ucsi_role_cmd(struct ucsi_connector *con, u64 command)
 		ucsi_reset_ppm(con->ucsi);
 
 		c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy;
-		ucsi_send_command(con->ucsi, c, NULL, 0);
+		ucsi_send_command(con->ucsi, c, NULL, 0, NULL);
 
 		ucsi_reset_connector(con, true);
 	}
@@ -1579,7 +1591,7 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
 	/* Get connector capability */
 	command = UCSI_GET_CONNECTOR_CAPABILITY;
 	command |= UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(ucsi, command, &con->cap, sizeof(con->cap));
+	ret = ucsi_send_command(ucsi, command, &con->cap, sizeof(con->cap), NULL);
 	if (ret < 0)
 		goto out_unlock;
 
@@ -1775,14 +1787,14 @@ static int ucsi_init(struct ucsi *ucsi)
 	/* Enable basic notifications */
 	ntfy = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
 	command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
-	ret = ucsi_send_command(ucsi, command, NULL, 0);
+	ret = ucsi_send_command(ucsi, command, NULL, 0, NULL);
 	if (ret < 0)
 		goto err_reset;
 
 	/* Get PPM capabilities */
 	command = UCSI_GET_CAPABILITY;
 	ret = ucsi_send_command(ucsi, command, &ucsi->cap,
-				BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE));
+				BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE), NULL);
 	if (ret < 0)
 		goto err_reset;
 
@@ -1809,7 +1821,7 @@ static int ucsi_init(struct ucsi *ucsi)
 	/* Enable all supported notifications */
 	ntfy = ucsi_get_supported_notifications(ucsi);
 	command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
-	ret = ucsi_send_command(ucsi, command, NULL, 0);
+	ret = ucsi_send_command(ucsi, command, NULL, 0, NULL);
 	if (ret < 0)
 		goto err_unregister;
 
@@ -1860,7 +1872,7 @@ static void ucsi_resume_work(struct work_struct *work)
 
 	/* Restore UCSI notification enable mask after system resume */
 	command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
-	ret = ucsi_send_command(ucsi, command, NULL, 0);
+	ret = ucsi_send_command(ucsi, command, NULL, 0, NULL);
 	if (ret < 0) {
 		dev_err(ucsi->dev, "failed to re-enable notifications (%d)\n", ret);
 		return;
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 5a8f947fcece..dbc7bcb07bc0 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -79,8 +79,9 @@ struct ucsi_operations {
 	int (*read_cci)(struct ucsi *ucsi, u32 *cci);
 	int (*poll_cci)(struct ucsi *ucsi, u32 *cci);
 	int (*read_message_in)(struct ucsi *ucsi, void *val, size_t val_len);
+	int (*write_message_out)(struct ucsi *ucsi, void *data, size_t data_len);
 	int (*sync_control)(struct ucsi *ucsi, u64 command, u32 *cci,
-			    void *data, size_t size);
+			    void *data, size_t size, void *message_out);
 	int (*async_control)(struct ucsi *ucsi, u64 command);
 	bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
 				struct ucsi_altmode *updated);
@@ -205,6 +206,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
 #define   UCSI_GET_PD_MESSAGE_TYPE_IDENTITY	4
 #define   UCSI_GET_PD_MESSAGE_TYPE_REVISION	5
 
+/* Data length bits */
+#define UCSI_COMMAND_DATA_LEN(_cmd_)		(((_cmd_) >> 8) & GENMASK(7, 0))
+
 /* -------------------------------------------------------------------------- */
 
 /* Error information returned by PPM in response to GET_ERROR_STATUS command. */
@@ -532,14 +536,14 @@ struct ucsi_connector {
 };
 
 int ucsi_send_command(struct ucsi *ucsi, u64 command,
-		      void *retval, size_t size);
+		      void *retval, size_t size, void *message_out);
 
 void ucsi_altmode_update_active(struct ucsi_connector *con);
 int ucsi_resume(struct ucsi *ucsi);
 
 void ucsi_notify_common(struct ucsi *ucsi, u32 cci);
 int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
-			     void *data, size_t size);
+			     void *data, size_t size, void *message_out);
 
 #if IS_ENABLED(CONFIG_POWER_SUPPLY)
 int ucsi_register_port_psy(struct ucsi_connector *con);
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 6b92f296e985..c0426bb1e81b 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -86,6 +86,21 @@ static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_le
 	return 0;
 }
 
+static int ucsi_acpi_write_message_out(struct ucsi *ucsi, void *data, size_t data_len)
+{
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+
+	if (!data || !data_len)
+		return -EINVAL;
+
+	if (ucsi->version <= UCSI_VERSION_1_2)
+		memcpy(ua->base + UCSI_MESSAGE_OUT, data, data_len);
+	else
+		memcpy(ua->base + UCSIv2_MESSAGE_OUT, data, data_len);
+
+	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_WRITE);
+}
+
 static int ucsi_acpi_async_control(struct ucsi *ucsi, u64 command)
 {
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
@@ -101,19 +116,20 @@ static const struct ucsi_operations ucsi_acpi_ops = {
 	.read_cci = ucsi_acpi_read_cci,
 	.poll_cci = ucsi_acpi_poll_cci,
 	.read_message_in = ucsi_acpi_read_message_in,
+	.write_message_out = ucsi_acpi_write_message_out,
 	.sync_control = ucsi_sync_control_common,
 	.async_control = ucsi_acpi_async_control
 };
 
 static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
-				  void *val, size_t len)
+				  void *val, size_t len, void *message_out)
 {
 	u16 bogus_change = UCSI_CONSTAT_POWER_LEVEL_CHANGE |
 			   UCSI_CONSTAT_PDOS_CHANGE;
 	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
 	int ret;
 
-	ret = ucsi_sync_control_common(ucsi, command, cci, val, len);
+	ret = ucsi_sync_control_common(ucsi, command, cci, val, len, message_out);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index e9a9df1431af..7757493b658e 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -603,7 +603,7 @@ static int ucsi_ccg_async_control(struct ucsi *ucsi, u64 command)
 }
 
 static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
-				 void *data, size_t size)
+				 void *data, size_t size, void *message_out)
 {
 	struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
 	struct ucsi_connector *con;
@@ -625,7 +625,7 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command, u32 *cci,
 		ucsi_ccg_update_set_new_cam_cmd(uc, con, &command);
 	}
 
-	ret = ucsi_sync_control_common(ucsi, command, cci, data, size);
+	ret = ucsi_sync_control_common(ucsi, command, cci, data, size, message_out);
 
 	switch (UCSI_COMMAND(command)) {
 	case UCSI_GET_CURRENT_CAM:
-- 
2.34.1

Re: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Dmitry Baryshkov]

On 27/06/2025 21:10, Pooja Katiyar wrote:
> Add support for updating message out data structure for UCSI ACPI
> interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
> LPM Firmware Update.
> 
> Additionally, update ucsi_send_command to accept message_out data
> and .sync_control function to pass message_out data to
> write_message_out function if the command is UCSI_SET_PDOS.
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
> ---
> Changelog v2:
> - Moved write_message_out operation to .sync_control.
> - Updated ucsi_send_command to accept message_out data.
> 
>   drivers/usb/typec/ucsi/cros_ec_ucsi.c |  4 +-
>   drivers/usb/typec/ucsi/debugfs.c      |  4 +-
>   drivers/usb/typec/ucsi/displayport.c  |  6 +--
>   drivers/usb/typec/ucsi/ucsi.c         | 64 ++++++++++++++++-----------
>   drivers/usb/typec/ucsi/ucsi.h         | 10 +++--
>   drivers/usb/typec/ucsi/ucsi_acpi.c    | 20 ++++++++-
>   drivers/usb/typec/ucsi/ucsi_ccg.c     |  4 +-
>   7 files changed, 72 insertions(+), 40 deletions(-)
> 

[...]

> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 01ce858a1a2b..c8410c88b45b 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -56,7 +56,7 @@ void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
>   EXPORT_SYMBOL_GPL(ucsi_notify_common);
>   
>   int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
> -			     void *data, size_t size)
> +			     void *data, size_t size, void *message_out)

Missing size for message_out. Also logically it should be:

int ucsi_sync_control_common(struct ucsi *ucsi, u64 command,
                              void *message_out, size_t message_out_size,
                              u32 *cci,
			     void *data, size_t size)

This way in params are separated from out.

>   {
>   	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
>   	int ret;
> @@ -68,6 +68,18 @@ int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
>   
>   	reinit_completion(&ucsi->complete);
>   
> +	if (message_out) {
> +		if (!ucsi->ops->write_message_out) {
> +			ret = -EOPNOTSUPP;
> +			goto out_clear_bit;
> +		}
> +
> +		ret = ucsi->ops->write_message_out(ucsi, message_out,
> +						   UCSI_COMMAND_DATA_LEN(command));

I think it should be other way around. write message_out, then set the 
DataLength field of the command.

> +		if (ret)
> +			goto out_clear_bit;
> +	}
> +
>   	ret = ucsi->ops->async_control(ucsi, command);
>   	if (ret)
>   		goto out_clear_bit;

[...]

> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 6b92f296e985..c0426bb1e81b 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -86,6 +86,21 @@ static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_le
>   	return 0;
>   }
>   
> +static int ucsi_acpi_write_message_out(struct ucsi *ucsi, void *data, size_t data_len)
> +{
> +	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> +
> +	if (!data || !data_len)
> +		return -EINVAL;
> +
> +	if (ucsi->version <= UCSI_VERSION_1_2)
> +		memcpy(ua->base + UCSI_MESSAGE_OUT, data, data_len);
> +	else
> +		memcpy(ua->base + UCSIv2_MESSAGE_OUT, data, data_len);
> +
> +	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_WRITE);

Why do we need extra WRITE? Isn't the one performed by 
ucsi_acpi_async_control() not enough?

> +}
> +
>   static int ucsi_acpi_async_control(struct ucsi *ucsi, u64 command)
>   {
>   	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Heikki Krogerus]

Hi Dmitry,

Please check the comment from Greg. I think we need to revert
back to the original. Please let us know if you still disagree.

But I tend to agree with Greg. There are too many parameters for the
functions like this. Let's just supply a separate function that can be
used to fill the message_out like Pooja originally suggested.

> > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> > index 6b92f296e985..c0426bb1e81b 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> > @@ -86,6 +86,21 @@ static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_le
> >   	return 0;
> >   }
> > +static int ucsi_acpi_write_message_out(struct ucsi *ucsi, void *data, size_t data_len)
> > +{
> > +	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> > +
> > +	if (!data || !data_len)
> > +		return -EINVAL;
> > +
> > +	if (ucsi->version <= UCSI_VERSION_1_2)
> > +		memcpy(ua->base + UCSI_MESSAGE_OUT, data, data_len);
> > +	else
> > +		memcpy(ua->base + UCSIv2_MESSAGE_OUT, data, data_len);
> > +
> > +	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_WRITE);
> 
> Why do we need extra WRITE? Isn't the one performed by
> ucsi_acpi_async_control() not enough?

This is actually a good point!

Syncing the mailbox before there is a command will confuse at least
some of the EC firmwares. But more importantly, the previous command
may still be in the control field, no? So that write may actually
cause the previous command to be executed again.

So Pooja, please drop that write.

thanks,

-- 
heikki

Re: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Greg KH]

On Fri, Jun 27, 2025 at 11:10:10AM -0700, Pooja Katiyar wrote:
> Add support for updating message out data structure for UCSI ACPI
> interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
> LPM Firmware Update.
> 
> Additionally, update ucsi_send_command to accept message_out data
> and .sync_control function to pass message_out data to
> write_message_out function if the command is UCSI_SET_PDOS.

Normally when you say "additionally" that implies that the patch should
be split up into pieces.  Why not do that here?

And do you _really_ need to add a new parameter to all of these
functions?  It's now getting even worse, look at this:

>  		ret = ucsi_send_command(ucsi, val,
>  					&ucsi->debugfs->response,
> -					sizeof(ucsi->debugfs->response));
> +					sizeof(ucsi->debugfs->response), NULL);

You can kind of guess what the parameters mean before the NULL change,
but now you have to go look up "what is the last pointer for"
everywhere.

This feels very fragile and horrible to maintain over time, please
reconsider this type of api change.



>  		break;
>  	default:
>  		ret = -EOPNOTSUPP;
> diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
> index 8aae80b457d7..93912719d915 100644
> --- a/drivers/usb/typec/ucsi/displayport.c
> +++ b/drivers/usb/typec/ucsi/displayport.c
> @@ -67,7 +67,7 @@ static int ucsi_displayport_enter(struct typec_altmode *alt, u32 *vdo)
>  	}
>  
>  	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(dp->con->num);
> -	ret = ucsi_send_command(ucsi, command, &cur, sizeof(cur));
> +	ret = ucsi_send_command(ucsi, command, &cur, sizeof(cur), NULL);

See, why NULL?  What does it have to do with a command?  And why can't
whatever is in that pointer place be part of that command to start with?

>  int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
> -			     void *data, size_t size)
> +			     void *data, size_t size, void *message_out)

So what is the difference between command, cci, data, and message_out?

Again, I think you need to reconsider this...

greg k-h

Re: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Heikki Krogerus]

Hi,

On Sat, Jun 28, 2025 at 04:51:56PM +0200, Greg KH wrote:
> On Fri, Jun 27, 2025 at 11:10:10AM -0700, Pooja Katiyar wrote:
> > Add support for updating message out data structure for UCSI ACPI
> > interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
> > LPM Firmware Update.
> > 
> > Additionally, update ucsi_send_command to accept message_out data
> > and .sync_control function to pass message_out data to
> > write_message_out function if the command is UCSI_SET_PDOS.
> 
> Normally when you say "additionally" that implies that the patch should
> be split up into pieces.  Why not do that here?
> 
> And do you _really_ need to add a new parameter to all of these
> functions?  It's now getting even worse, look at this:
> 
> >  		ret = ucsi_send_command(ucsi, val,
> >  					&ucsi->debugfs->response,
> > -					sizeof(ucsi->debugfs->response));
> > +					sizeof(ucsi->debugfs->response), NULL);
> 
> You can kind of guess what the parameters mean before the NULL change,
> but now you have to go look up "what is the last pointer for"
> everywhere.
> 
> This feels very fragile and horrible to maintain over time, please
> reconsider this type of api change.

So I think what Pooja was proposing in the first version of this
series, where you had a dedicated function for filling the
message_out, was better after all.

Pooja, I'm really sorry about this, but can you revert back to that,
and send it as v3? Let's start over.

thanks,

-- 
heikki

Re: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Dmitry Baryshkov]

On 01/07/2025 11:46, Heikki Krogerus wrote:
> Hi,
> 
> On Sat, Jun 28, 2025 at 04:51:56PM +0200, Greg KH wrote:
>> On Fri, Jun 27, 2025 at 11:10:10AM -0700, Pooja Katiyar wrote:
>>> Add support for updating message out data structure for UCSI ACPI
>>> interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
>>> LPM Firmware Update.
>>>
>>> Additionally, update ucsi_send_command to accept message_out data
>>> and .sync_control function to pass message_out data to
>>> write_message_out function if the command is UCSI_SET_PDOS.
>>
>> Normally when you say "additionally" that implies that the patch should
>> be split up into pieces.  Why not do that here?
>>
>> And do you _really_ need to add a new parameter to all of these
>> functions?  It's now getting even worse, look at this:
>>
>>>   		ret = ucsi_send_command(ucsi, val,
>>>   					&ucsi->debugfs->response,
>>> -					sizeof(ucsi->debugfs->response));
>>> +					sizeof(ucsi->debugfs->response), NULL);
>>
>> You can kind of guess what the parameters mean before the NULL change,
>> but now you have to go look up "what is the last pointer for"
>> everywhere.
>>
>> This feels very fragile and horrible to maintain over time, please
>> reconsider this type of api change.
> 
> So I think what Pooja was proposing in the first version of this
> series, where you had a dedicated function for filling the
> message_out, was better after all.
> 
> Pooja, I'm really sorry about this, but can you revert back to that,
> and send it as v3? Let's start over.

But that breaks the sync_control logic - currently it is possible to 
handle the command in .sync_control completely. If for any reason we 
need to implement workarounds for commands using MESSAGE_OUT field, we'd 
have to introduce additional logic (which we just got rid of).

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Heikki Krogerus]

On Tue, Jul 01, 2025 at 11:50:21AM +0300, Dmitry Baryshkov wrote:
> On 01/07/2025 11:46, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Sat, Jun 28, 2025 at 04:51:56PM +0200, Greg KH wrote:
> > > On Fri, Jun 27, 2025 at 11:10:10AM -0700, Pooja Katiyar wrote:
> > > > Add support for updating message out data structure for UCSI ACPI
> > > > interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
> > > > LPM Firmware Update.
> > > > 
> > > > Additionally, update ucsi_send_command to accept message_out data
> > > > and .sync_control function to pass message_out data to
> > > > write_message_out function if the command is UCSI_SET_PDOS.
> > > 
> > > Normally when you say "additionally" that implies that the patch should
> > > be split up into pieces.  Why not do that here?
> > > 
> > > And do you _really_ need to add a new parameter to all of these
> > > functions?  It's now getting even worse, look at this:
> > > 
> > > >   		ret = ucsi_send_command(ucsi, val,
> > > >   					&ucsi->debugfs->response,
> > > > -					sizeof(ucsi->debugfs->response));
> > > > +					sizeof(ucsi->debugfs->response), NULL);
> > > 
> > > You can kind of guess what the parameters mean before the NULL change,
> > > but now you have to go look up "what is the last pointer for"
> > > everywhere.
> > > 
> > > This feels very fragile and horrible to maintain over time, please
> > > reconsider this type of api change.
> > 
> > So I think what Pooja was proposing in the first version of this
> > series, where you had a dedicated function for filling the
> > message_out, was better after all.
> > 
> > Pooja, I'm really sorry about this, but can you revert back to that,
> > and send it as v3? Let's start over.
> 
> But that breaks the sync_control logic - currently it is possible to handle
> the command in .sync_control completely. If for any reason we need to
> implement workarounds for commands using MESSAGE_OUT field, we'd have to
> introduce additional logic (which we just got rid of).

Okay. So how about a data structure for the entire mailbox that we can
pass to these functions?

struct ucsi_mailbox {
        u32 cci;
        u64 control;
        void *message_in;
        size_t message_in_size;
        void *message_out;
        size_t message_out_size;
};

thanks,

-- 
heikki

Re: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Dmitry Baryshkov]

On 01/07/2025 13:05, Heikki Krogerus wrote:
> On Tue, Jul 01, 2025 at 11:50:21AM +0300, Dmitry Baryshkov wrote:
>> On 01/07/2025 11:46, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> On Sat, Jun 28, 2025 at 04:51:56PM +0200, Greg KH wrote:
>>>> On Fri, Jun 27, 2025 at 11:10:10AM -0700, Pooja Katiyar wrote:
>>>>> Add support for updating message out data structure for UCSI ACPI
>>>>> interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
>>>>> LPM Firmware Update.
>>>>>
>>>>> Additionally, update ucsi_send_command to accept message_out data
>>>>> and .sync_control function to pass message_out data to
>>>>> write_message_out function if the command is UCSI_SET_PDOS.
>>>>
>>>> Normally when you say "additionally" that implies that the patch should
>>>> be split up into pieces.  Why not do that here?
>>>>
>>>> And do you _really_ need to add a new parameter to all of these
>>>> functions?  It's now getting even worse, look at this:
>>>>
>>>>>    		ret = ucsi_send_command(ucsi, val,
>>>>>    					&ucsi->debugfs->response,
>>>>> -					sizeof(ucsi->debugfs->response));
>>>>> +					sizeof(ucsi->debugfs->response), NULL);
>>>>
>>>> You can kind of guess what the parameters mean before the NULL change,
>>>> but now you have to go look up "what is the last pointer for"
>>>> everywhere.
>>>>
>>>> This feels very fragile and horrible to maintain over time, please
>>>> reconsider this type of api change.
>>>
>>> So I think what Pooja was proposing in the first version of this
>>> series, where you had a dedicated function for filling the
>>> message_out, was better after all.
>>>
>>> Pooja, I'm really sorry about this, but can you revert back to that,
>>> and send it as v3? Let's start over.
>>
>> But that breaks the sync_control logic - currently it is possible to handle
>> the command in .sync_control completely. If for any reason we need to
>> implement workarounds for commands using MESSAGE_OUT field, we'd have to
>> introduce additional logic (which we just got rid of).
> 
> Okay. So how about a data structure for the entire mailbox that we can
> pass to these functions?
> 
> struct ucsi_mailbox {
>          u32 cci;
>          u64 control;
>          void *message_in;
>          size_t message_in_size;
>          void *message_out;
>          size_t message_out_size;
> };

What about a slightly different proposal (following ucsi_ccg design):


struct ucsi {
    // .....
    u32 cci;
    u8 message_in[UCSI_MAX_MESSAGE_IN];
    u8 message_out[UCSI_MAX_MESSAGE_OUT];
};

The caller will fill ucsi->message_out, call sync_control with a proper 
length specified, then read UCSI_CCI_LENGTH(ucsi->cci) bytes from 
ucsi->message_in.

Note: I'm positive that we can handle message buffers in this way. I'm 
not so sure about the CCI, it might be too dynamic.

> 
> thanks,
> 


-- 
With best wishes
Dmitry

Re: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Heikki Krogerus]

On Tue, Jul 01, 2025 at 01:11:24PM +0300, Dmitry Baryshkov wrote:
> On 01/07/2025 13:05, Heikki Krogerus wrote:
> > On Tue, Jul 01, 2025 at 11:50:21AM +0300, Dmitry Baryshkov wrote:
> > > On 01/07/2025 11:46, Heikki Krogerus wrote:
> > > > Hi,
> > > > 
> > > > On Sat, Jun 28, 2025 at 04:51:56PM +0200, Greg KH wrote:
> > > > > On Fri, Jun 27, 2025 at 11:10:10AM -0700, Pooja Katiyar wrote:
> > > > > > Add support for updating message out data structure for UCSI ACPI
> > > > > > interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
> > > > > > LPM Firmware Update.
> > > > > > 
> > > > > > Additionally, update ucsi_send_command to accept message_out data
> > > > > > and .sync_control function to pass message_out data to
> > > > > > write_message_out function if the command is UCSI_SET_PDOS.
> > > > > 
> > > > > Normally when you say "additionally" that implies that the patch should
> > > > > be split up into pieces.  Why not do that here?
> > > > > 
> > > > > And do you _really_ need to add a new parameter to all of these
> > > > > functions?  It's now getting even worse, look at this:
> > > > > 
> > > > > >    		ret = ucsi_send_command(ucsi, val,
> > > > > >    					&ucsi->debugfs->response,
> > > > > > -					sizeof(ucsi->debugfs->response));
> > > > > > +					sizeof(ucsi->debugfs->response), NULL);
> > > > > 
> > > > > You can kind of guess what the parameters mean before the NULL change,
> > > > > but now you have to go look up "what is the last pointer for"
> > > > > everywhere.
> > > > > 
> > > > > This feels very fragile and horrible to maintain over time, please
> > > > > reconsider this type of api change.
> > > > 
> > > > So I think what Pooja was proposing in the first version of this
> > > > series, where you had a dedicated function for filling the
> > > > message_out, was better after all.
> > > > 
> > > > Pooja, I'm really sorry about this, but can you revert back to that,
> > > > and send it as v3? Let's start over.
> > > 
> > > But that breaks the sync_control logic - currently it is possible to handle
> > > the command in .sync_control completely. If for any reason we need to
> > > implement workarounds for commands using MESSAGE_OUT field, we'd have to
> > > introduce additional logic (which we just got rid of).
> > 
> > Okay. So how about a data structure for the entire mailbox that we can
> > pass to these functions?
> > 
> > struct ucsi_mailbox {
> >          u32 cci;
> >          u64 control;
> >          void *message_in;
> >          size_t message_in_size;
> >          void *message_out;
> >          size_t message_out_size;
> > };
> 
> What about a slightly different proposal (following ucsi_ccg design):
> 
> 
> struct ucsi {
>    // .....
>    u32 cci;
>    u8 message_in[UCSI_MAX_MESSAGE_IN];
>    u8 message_out[UCSI_MAX_MESSAGE_OUT];
> };
> 
> The caller will fill ucsi->message_out, call sync_control with a proper
> length specified, then read UCSI_CCI_LENGTH(ucsi->cci) bytes from
> ucsi->message_in.

Looks better indeed.

> Note: I'm positive that we can handle message buffers in this way. I'm not
> so sure about the CCI, it might be too dynamic.

Pooja, I'm sorry about the extra work needed. Can you check this?

P.S. I'm already on vacation so there will be delays with my
responses.

-- 
heikki

RE: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Katiyar, Pooja]

Hello Greg, Heikki and Dmitry,

On Tue, Jul 1, 2025 05:51:12AM -0700 , Heikki Krogerus wrote:
> On Tue, Jul 01, 2025 at 01:11:24PM +0300, Dmitry Baryshkov wrote:
> > On 01/07/2025 13:05, Heikki Krogerus wrote:
> > > On Tue, Jul 01, 2025 at 11:50:21AM +0300, Dmitry Baryshkov wrote:
> > > > On 01/07/2025 11:46, Heikki Krogerus wrote:
> > > > > Hi,
> > > > >
> > > > > On Sat, Jun 28, 2025 at 04:51:56PM +0200, Greg KH wrote:
> > > > > > On Fri, Jun 27, 2025 at 11:10:10AM -0700, Pooja Katiyar wrote:
> > > > > > > Add support for updating message out data structure for UCSI ACPI
> > > > > > > interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
> > > > > > > LPM Firmware Update.

...

> > > Okay. So how about a data structure for the entire mailbox that we can
> > > pass to these functions?
> > >
> > > struct ucsi_mailbox {
> > >          u32 cci;
> > >          u64 control;
> > >          void *message_in;
> > >          size_t message_in_size;
> > >          void *message_out;
> > >          size_t message_out_size;
> > > };
> >
> > What about a slightly different proposal (following ucsi_ccg design):
> >
> >
> > struct ucsi {
> >    // .....
> >    u32 cci;
> >    u8 message_in[UCSI_MAX_MESSAGE_IN];
> >    u8 message_out[UCSI_MAX_MESSAGE_OUT];
> > };
> >
> > The caller will fill ucsi->message_out, call sync_control with a proper
> > length specified, then read UCSI_CCI_LENGTH(ucsi->cci) bytes from
> > ucsi->message_in.
> 
> Looks better indeed.
> 
> > Note: I'm positive that we can handle message buffers in this way. I'm not
> > so sure about the CCI, it might be too dynamic.
> 
> Pooja, I'm sorry about the extra work needed. Can you check this?
> 
> P.S. I'm already on vacation so there will be delays with my
> responses.
> 

Thank you for the feedback and suggestions.

I can start working on the changes. Before diving into implementation,
I would like to clarify a few points.

1. What is the preference with respect to the lengths of message in and message out,
should the length be included as part of the ucsi structure or should be passed as 
separate parameters to relevant functions?

2. As Dmitriy pointed out, cci changes might be too dynamic and hence we believe
it's better to include message_in and message_out as part of the ucsi struct and
exclude cci. What are your thoughts on this?

3. Implementing these changes will affect functions across different interfaces such as 
ACPI, ucsi_ccg, cros_ec_ucsi etc. At this time, we can only validate these changes on the 
ACPI interface. Unfortunately, we don't have the means to test other interfaces. What
is your suggestion to proceed with implementation? should owners of other interfaces
be involved for validation?

Thanks,
Pooja

Re: [PATCH v2 1/3] usb: typec: ucsi: Add support for message out data structure

[Dmitry Baryshkov]

On Thu, 3 Jul 2025 at 07:29, Katiyar, Pooja <pooja.katiyar@intel.com> wrote:
>
> Hello Greg, Heikki and Dmitry,
>
> On Tue, Jul 1, 2025 05:51:12AM -0700 , Heikki Krogerus wrote:
> > On Tue, Jul 01, 2025 at 01:11:24PM +0300, Dmitry Baryshkov wrote:
> > > On 01/07/2025 13:05, Heikki Krogerus wrote:
> > > > On Tue, Jul 01, 2025 at 11:50:21AM +0300, Dmitry Baryshkov wrote:
> > > > > On 01/07/2025 11:46, Heikki Krogerus wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Sat, Jun 28, 2025 at 04:51:56PM +0200, Greg KH wrote:
> > > > > > > On Fri, Jun 27, 2025 at 11:10:10AM -0700, Pooja Katiyar wrote:
> > > > > > > > Add support for updating message out data structure for UCSI ACPI
> > > > > > > > interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and
> > > > > > > > LPM Firmware Update.
>
> ...
>
> > > > Okay. So how about a data structure for the entire mailbox that we can
> > > > pass to these functions?
> > > >
> > > > struct ucsi_mailbox {
> > > >          u32 cci;
> > > >          u64 control;
> > > >          void *message_in;
> > > >          size_t message_in_size;
> > > >          void *message_out;
> > > >          size_t message_out_size;
> > > > };
> > >
> > > What about a slightly different proposal (following ucsi_ccg design):
> > >
> > >
> > > struct ucsi {
> > >    // .....
> > >    u32 cci;
> > >    u8 message_in[UCSI_MAX_MESSAGE_IN];
> > >    u8 message_out[UCSI_MAX_MESSAGE_OUT];
> > > };
> > >
> > > The caller will fill ucsi->message_out, call sync_control with a proper
> > > length specified, then read UCSI_CCI_LENGTH(ucsi->cci) bytes from
> > > ucsi->message_in.
> >
> > Looks better indeed.
> >
> > > Note: I'm positive that we can handle message buffers in this way. I'm not
> > > so sure about the CCI, it might be too dynamic.
> >
> > Pooja, I'm sorry about the extra work needed. Can you check this?
> >
> > P.S. I'm already on vacation so there will be delays with my
> > responses.
> >
>
> Thank you for the feedback and suggestions.
>
> I can start working on the changes. Before diving into implementation,
> I would like to clarify a few points.
>
> 1. What is the preference with respect to the lengths of message in and message out,
> should the length be included as part of the ucsi structure or should be passed as
> separate parameters to relevant functions?

Store the lengths next to the buffer arrays.

>
> 2. As Dmitriy pointed out, cci changes might be too dynamic and hence we believe
> it's better to include message_in and message_out as part of the ucsi struct and
> exclude cci. What are your thoughts on this?
>
> 3. Implementing these changes will affect functions across different interfaces such as
> ACPI, ucsi_ccg, cros_ec_ucsi etc. At this time, we can only validate these changes on the
> ACPI interface. Unfortunately, we don't have the means to test other interfaces. What
> is your suggestion to proceed with implementation? should owners of other interfaces
> be involved for validation?

Change all the implementations at once and then collect Tested-by
tags, that's the usual procedure.

-- 
With best wishes
Dmitry

[PATCH v2 2/3] usb: typec: ucsi: Enable debugfs for message_out data structure

[Pooja Katiyar]

Add debugfs entry for writing message_out data structure to handle
UCSI 2.1 and 3.0 commands through debugfs interface.

Users writing to the message_out debugfs file should ensure the input
data adheres to the following format:
1. Input must be a non-empty valid hexadecimal string.
2. Input length of hexadecimal string must not exceed 256 bytes of
   length to be in alignment with the message out data structure size
   as per the UCSI specification v2.1.
3. If the input string length is odd, then user needs to prepend a
   '0' to the first character for proper hex conversion.

Below are examples of valid hex strings. Note that these values are
just examples. The exact values depend on specific command use case.

	#echo 1A2B3C4D > message_out
	#echo 01234567 > message_out

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
---
 drivers/usb/typec/ucsi/debugfs.c | 26 ++++++++++++++++++++++++++
 drivers/usb/typec/ucsi/ucsi.h    |  3 +++
 2 files changed, 29 insertions(+)

diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c
index c3c349e712d0..6058986ea7fd 100644
--- a/drivers/usb/typec/ucsi/debugfs.c
+++ b/drivers/usb/typec/ucsi/debugfs.c
@@ -80,6 +80,30 @@ static int ucsi_resp_show(struct seq_file *s, void *not_used)
 }
 DEFINE_SHOW_ATTRIBUTE(ucsi_resp);
 
+static ssize_t ucsi_message_out_write(struct file *file,
+				      const char __user *data, size_t count, loff_t *ppos)
+{
+	struct ucsi *ucsi = file->private_data;
+	int ret;
+
+	char *buf __free(kfree) = memdup_user_nul(data, count);
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	ret = hex2bin(ucsi->debugfs->message_out, buf,
+		      min(count / 2, sizeof(ucsi->debugfs->message_out)));
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static const struct file_operations ucsi_message_out_fops = {
+	.open = simple_open,
+	.write = ucsi_message_out_write,
+	.llseek = generic_file_llseek,
+};
+
 void ucsi_debugfs_register(struct ucsi *ucsi)
 {
 	ucsi->debugfs = kzalloc(sizeof(*ucsi->debugfs), GFP_KERNEL);
@@ -89,6 +113,8 @@ void ucsi_debugfs_register(struct ucsi *ucsi)
 	ucsi->debugfs->dentry = debugfs_create_dir(dev_name(ucsi->dev), ucsi_debugfs_root);
 	debugfs_create_file("command", 0200, ucsi->debugfs->dentry, ucsi, &ucsi_cmd_fops);
 	debugfs_create_file("response", 0400, ucsi->debugfs->dentry, ucsi, &ucsi_resp_fops);
+	debugfs_create_file("message_out", 0200, ucsi->debugfs->dentry, ucsi,
+			    &ucsi_message_out_fops);
 }
 
 void ucsi_debugfs_unregister(struct ucsi *ucsi)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index dbc7bcb07bc0..0d1c39cfe8a9 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -434,6 +434,8 @@ struct ucsi_bitfield {
 
 /* -------------------------------------------------------------------------- */
 
+#define MESSAGE_OUT_MAX_LEN 256
+
 struct ucsi_debugfs_entry {
 	u64 command;
 	struct ucsi_data {
@@ -441,6 +443,7 @@ struct ucsi_debugfs_entry {
 		u64 high;
 	} response;
 	int status;
+	u8 message_out[MESSAGE_OUT_MAX_LEN];
 	struct dentry *dentry;
 };
 
-- 
2.34.1

Re: [PATCH v2 2/3] usb: typec: ucsi: Enable debugfs for message_out data structure

[Dmitry Baryshkov]

On Fri, Jun 27, 2025 at 11:10:11AM -0700, Pooja Katiyar wrote:
> Add debugfs entry for writing message_out data structure to handle
> UCSI 2.1 and 3.0 commands through debugfs interface.
> 
> Users writing to the message_out debugfs file should ensure the input
> data adheres to the following format:
> 1. Input must be a non-empty valid hexadecimal string.
> 2. Input length of hexadecimal string must not exceed 256 bytes of
>    length to be in alignment with the message out data structure size
>    as per the UCSI specification v2.1.
> 3. If the input string length is odd, then user needs to prepend a
>    '0' to the first character for proper hex conversion.
> 
> Below are examples of valid hex strings. Note that these values are
> just examples. The exact values depend on specific command use case.
> 
> 	#echo 1A2B3C4D > message_out
> 	#echo 01234567 > message_out
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
> ---
>  drivers/usb/typec/ucsi/debugfs.c | 26 ++++++++++++++++++++++++++
>  drivers/usb/typec/ucsi/ucsi.h    |  3 +++
>  2 files changed, 29 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

[PATCH v2 3/3] usb: typec: ucsi: Add support for SET_PDOS command

[Pooja Katiyar]

Add support for UCSI SET_PDOS command as per UCSI specification v2.1 and
above to debugfs.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
---
Changelog v2:
- Send message_out data as part of ucsi_send_command.

 drivers/usb/typec/ucsi/debugfs.c | 3 +++
 drivers/usb/typec/ucsi/ucsi.h    | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c
index 6058986ea7fd..e6bea273fbd9 100644
--- a/drivers/usb/typec/ucsi/debugfs.c
+++ b/drivers/usb/typec/ucsi/debugfs.c
@@ -37,6 +37,9 @@ static int ucsi_cmd(void *data, u64 val)
 	case UCSI_SET_USB:
 		ret = ucsi_send_command(ucsi, val, NULL, 0, NULL);
 		break;
+	case UCSI_SET_PDOS:
+		ret = ucsi_send_command(ucsi, val, NULL, 0, ucsi->debugfs->message_out);
+		break;
 	case UCSI_GET_CAPABILITY:
 	case UCSI_GET_CONNECTOR_CAPABILITY:
 	case UCSI_GET_ALTERNATE_MODES:
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 0d1c39cfe8a9..2504041b7432 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -130,6 +130,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
 #define UCSI_GET_PD_MESSAGE			0x15
 #define UCSI_GET_CAM_CS			0x18
 #define UCSI_SET_SINK_PATH			0x1c
+#define UCSI_SET_PDOS				0x1d
 #define UCSI_SET_USB				0x21
 #define UCSI_GET_LPM_PPM_INFO			0x22
 
-- 
2.34.1

Re: [PATCH v2 3/3] usb: typec: ucsi: Add support for SET_PDOS command

[Dmitry Baryshkov]

On Fri, Jun 27, 2025 at 11:10:12AM -0700, Pooja Katiyar wrote:
> Add support for UCSI SET_PDOS command as per UCSI specification v2.1 and
> above to debugfs.
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
> ---
> Changelog v2:
> - Send message_out data as part of ucsi_send_command.
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry