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

[Greg KH <gregkh@linuxfoundation.org>]

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