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

[Bjorn Andersson]

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

Thanks,
Bjorn