Re: [PATCH V7 1/3] rpmsg: core: Add signal API support

[Arnaud POULIQUEN]

On 6/15/23 18:45, Bjorn Andersson wrote:
> On Thu, Jun 15, 2023 at 06:19:37PM +0200, Arnaud POULIQUEN wrote:
>>
>>
>> On 6/15/23 16:50, Bjorn Andersson wrote:
>>> On Thu, Jun 15, 2023 at 11:01:14AM +0200, Arnaud POULIQUEN wrote:
>>>>
>>>>
>>>> On 6/14/23 17:54, Bjorn Andersson wrote:
>>>>> On Sat, Apr 22, 2023 at 04:12:05PM +0530, Sarannya S wrote:
>>>>>> From: Deepak Kumar Singh <quic_deesin@xxxxxxxxxxx>
>>>>>>
>>>>>> Some transports like Glink support the state notifications between
>>>>>> clients using flow control signals similar to serial protocol signals.
>>>>>> Local glink client drivers can send and receive flow control status
>>>>>> to glink clients running on remote processors.
>>>>>>
>>>>>> Add APIs to support sending and receiving of flow control status by
>>>>>> rpmsg clients.
>>>>>>
>>>>>> Signed-off-by: Deepak Kumar Singh <quic_deesin@xxxxxxxxxxx>
>>>>>> Signed-off-by: Sarannya S <quic_sarannya@xxxxxxxxxxx>
>>>>>> ---
>>>>>>  drivers/rpmsg/rpmsg_core.c     | 21 +++++++++++++++++++++
>>>>>>  drivers/rpmsg/rpmsg_internal.h |  2 ++
>>>>>>  include/linux/rpmsg.h          | 15 +++++++++++++++
>>>>>>  3 files changed, 38 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>>>>>> index a2207c0..e8bbe05 100644
>>>>>> --- a/drivers/rpmsg/rpmsg_core.c
>>>>>> +++ b/drivers/rpmsg/rpmsg_core.c
>>>>>> @@ -331,6 +331,25 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>>>>>>  EXPORT_SYMBOL(rpmsg_trysend_offchannel);
>>>>>>  
>>>>>>  /**
>>>>>> + * rpmsg_set_flow_control() - sets/clears serial flow control signals
>>>>>> + * @ept:	the rpmsg endpoint
>>>>>> + * @enable:	pause/resume incoming data flow	
>>>>>
>>>>> As shown in the discussion, it's still not clear what true/false means.
>>>>> Also, let's try to clarify that it's a request for the other side to do
>>>>> something:
>>>>>
>>>>> * rpmsg_set_flow_control() - request remote to pause/resume transmission
>>>>> * ...
>>>>> * @enable: flow restricted
>>>>> * ...
>>>>>
>>>>>
>>>>> PS. There's a stray space at the end of the line.
>>>>
>>>> The notion of flow restricted seems to me also ambiguous. It does
>>>> not specify if the stream is limited in term of bandwidth or stopped.
>>>>
>>>> What about using XON/XOFF as specified in software flow control[1]
>>>>
>>>> XOFF	Pause transmission
>>>> XON	Resume transmission
>>>>
>>>> or simply pause/resume definitions
>>>>
>>>
>>> I agree, that's still ambiguous.
>>>
>>> I was concerned about expressing it such that the reader would assume
>>> that calling this means there will be no more data coming, but there
>>> might be things in the queues etc. Expressing it in terms of the state
>>> of transmission is clearer.
>>>
>>>
>>> /*
>>>  * rpmsg_set_flow_control() - request remote to pause/resume transmission
>>>  ...
>>>  * @enable: Pause transmission
>>>  ...
>>>  */
>>>
>>> Does that sound okay and clear to you?
>>
>> Much better! I still have a nitpicking point :)
>> What about replacing @enable variable by @pause to align the variable with the
>> usage?
>>  /*
>>   * rpmsg_set_flow_control() - request remote to pause/resume transmission
>>   ...
>>   * @pause: set to 1 to pause transmission, to 0 to resume the transmission
> 
> It's a boolean, so I think with your name change suggestion, together
> with the function description, it should be clear enough what the two
> states (true/false) means.
> 
> * @pause: Pause transmission

Ok for me

Thanks,
Arnaud

> 
> Thanks,
> Bjorn