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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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