Re: [PATCH v5 2/4] dmaengine: gpi: Add Lock and Unlock TRE support to access I2C exclusively

[Vinod Koul <vkoul@kernel.org>]

On 18-12-24, 18:04, Mukesh Kumar Savaliya wrote:
> Hi Vinod, Thanks !  I just saw your comments now as somehow it was going in
> some other folder and didn't realize.
> 
> On 12/4/2024 5:51 PM, Vinod Koul wrote:
> > On 02-12-24, 16:13, Mukesh Kumar Savaliya wrote:
> > > Thanks for the review comments Vinod !
> > > 
> > > On 12/2/2024 12:17 PM, Vinod Koul wrote:
> > > > On 29-11-24, 20:13, Mukesh Kumar Savaliya wrote:
> > > > > GSI DMA provides specific TREs(Transfer ring element) namely Lock and
> > > > > Unlock TRE. It provides mutually exclusive access to I2C controller from
> > > > > any of the processor(Apps,ADSP). Lock prevents other subsystems from
> > > > > concurrently performing DMA transfers and avoids disturbance to data path.
> > > > > Basically for shared I2C usecase, lock the SE(Serial Engine) for one of
> > > > > the processor, complete the transfer, unlock the SE.
> > > > > 
> > > > > Apply Lock TRE for the first transfer of shared SE and Apply Unlock
> > > > > TRE for the last transfer.
> > > > > 
> > > > > Also change MAX_TRE macro to 5 from 3 because of the two additional TREs.
> > > > > 
> > > > 
> > > > ...
> > > > 
> > > > > @@ -65,6 +65,9 @@ enum i2c_op {
> > > > >     * @rx_len: receive length for buffer
> > > > >     * @op: i2c cmd
> > > > >     * @muli-msg: is part of multi i2c r-w msgs
> > > > > + * @shared_se: bus is shared between subsystems
> > > > > + * @bool first_msg: use it for tracking multimessage xfer
> > > > > + * @bool last_msg: use it for tracking multimessage xfer
> > > > >     */
> > > > >    struct gpi_i2c_config {
> > > > >    	u8 set_config;
> > > > > @@ -78,6 +81,9 @@ struct gpi_i2c_config {
> > > > >    	u32 rx_len;
> > > > >    	enum i2c_op op;
> > > > >    	bool multi_msg;
> > > > > +	bool shared_se;
> > > > 
> > > > Looking at this why do you need this field? It can be internal to your
> > > > i2c driver... Why not just set an enum for lock and use the values as
> > > > lock/unlock/dont care and make the interface simpler. I see no reason to
> > > > use three variables to communicate the info which can be handled in
> > > > simpler way..?
> > > > 
> > > Below was earlier reply to [PATCH V3, 2/4], please let me know if you have
> > > any additional comment and need further clarifications.
> > 
> > Looks like you misunderstood, the question is why do you need three
> > variables to convey this info..? Use a single variable please
> Yes, I think so. Please let me clarify.
> First variable is a feature flag and it's required to be explicitly
> mentioned by client (i2c/spi/etc) to GSI driver.
> 
> Second and third, can be optimized to boolean so either first or last can be
> passed.
> 
> Please correct me or add simple change where you would like to make, i can
> add that.

I though we could do with a single and derive

Also, please see 20241212041639.4109039-3-quic_mdalam@quicinc.com, folks
from same company should talk together on same solutions, please
converge and come up with a single proposal which works for both drivers

-- 
~Vinod