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

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

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