Re: [PATCH v2 3/7] firmware: stratix10-svc: Add initial support for asynchronous communication with Stratix 10 service channel

[Dinh Nguyen <dinguyen@kernel.org>]

On 5/22/25 05:33, Mahesh Rao wrote:
> 
> 
> On 22-05-2025 01:48 am, Dinh Nguyen wrote:
>> On 5/21/25 03:42, Mahesh Rao wrote:
>>>
>>>
>>> On 19-05-2025 05:28 pm, Dinh Nguyen wrote:
>>>> On 5/12/25 06:39, Mahesh Rao via B4 Relay wrote:
>>>>> From: Mahesh Rao <mahesh.rao@altera.com>
>>>>>
>>
>> <snip>
>>
>>>>> +
>>>>> +/**
>>>>> + * stratix10_svc_async_prepare_response - Prepare the response 
>>>>> data for an asynchronous transaction.
>>>>> + * @chan: Pointer to the service channel structure.
>>>>> + * @handle: Pointer to the asynchronous handler structure.
>>>>> + * @data: Pointer to the callback data structure.
>>>>> + *
>>>>> + * This function prepares the response data for an asynchronous 
>>>>> transaction. It
>>>>> + * extracts the response data from the SMC response structure and 
>>>>> stores it in
>>>>> + * the callback data structure. The function also logs the 
>>>>> completion of the
>>>>> + * asynchronous transaction.
>>>>> + *
>>>>> + * Return: 0 on success, -ENOENT if the command is invalid
>>>>> + */
>>>>> +static int stratix10_svc_async_prepare_response(struct 
>>>>> stratix10_svc_chan *chan,
>>>>> +                        struct stratix10_svc_async_handler *handle,
>>>>> +                        struct stratix10_svc_cb_data *data)
>>>>> +{
>>>>> +    struct stratix10_svc_client_msg *p_msg =
>>>>> +        (struct stratix10_svc_client_msg *)handle->msg;
>>>>> +    struct stratix10_svc_controller *ctrl = chan->ctrl;
>>>>> +
>>>>> +    data->status = STRATIX10_GET_SDM_STATUS_CODE(handle->res.a1);
>>>>> +
>>>>> +    switch (p_msg->command) {
>>>>> +    default:
>>>>> +        dev_alert(ctrl->dev, "Invalid command\n ,%d", 
>>>>> p_msg->command);
>>>>> +        return -ENOENT;
>>>>> +    }
>>>>
>>>> What is the above code doing?
>>>
>>> This function prepares the response  for clients after retriving the 
>>> response from the Arm Trusted Firmware using the polling call. 
>>> Currently only the negative scenario is shown for incvalid command, 
>>> the last patch in series adds command for hwmon for the positive 
>>> scenario.
>>>
>>
>> Okay, but this is confusing. Please just add this to the last patch then.
>>
> 
> Shall I combine the last patch for adding hwmon commands here so that 
> the framework has some command usage?.
> 

I like the idea of splitting up the patches to be small, but I don't 
think combining them is the right thing to do either. Each patch should 
be able to stand on it's own as well. The HWMON commands are fine in 
their own patch. You don't need to put the switch statements in this 
patch, because nothing is being done.