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

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

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