Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 15 Jun 2023 09:45:07 -0700
From:   Bjorn Andersson <andersson@kernel.org>
To:     Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Cc:     Bjorn Andersson <quic_bjorande@quicinc.com>,
        Sarannya S <quic_sarannya@quicinc.com>, swboyd@chromium.org,
        quic_clew@quicinc.com, mathieu.poirier@linaro.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-remoteproc@vger.kernel.org,
        Deepak Kumar Singh <quic_deesin@quicinc.com>
Subject: Re: [PATCH V7 1/3] rpmsg: core: Add signal API support
Message-ID: <20230615164507.mu7fd22poamjth7p@ripper>
References: <1682160127-18103-1-git-send-email-quic_sarannya@quicinc.com>
 <1682160127-18103-2-git-send-email-quic_sarannya@quicinc.com>
 <20230614155453.dywcrntfjddxojfv@ripper>
 <6e51d6d8-cd3a-b0f2-c044-6282749aae89@foss.st.com>
 <20230615145039.GA3256591@hu-bjorande-lv.qualcomm.com>
 <4d89950d-0376-e355-c70b-d054776e83d4@foss.st.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4d89950d-0376-e355-c70b-d054776e83d4@foss.st.com>
Precedence: bulk

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@quicinc.com>
> >>>>
> >>>> 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@quicinc.com>
> >>>> Signed-off-by: Sarannya S <quic_sarannya@quicinc.com>
> >>>> ---
> >>>>  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