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

[Vinod Koul <vkoul@kernel.org>]

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

> --
> > Looking at the usage in following patches, why cant this be handled
> > internally as part of prep call?
> >
> As per design, i2c driver iterates over each message and submits to GPI
> where it creates TRE. Since it's per transfer, we need to create Lock and
> Unlock TRE based on first or last message.
> --
> > > +	bool first_msg;
> > > +	bool last_msg;
> > 

-- 
~Vinod