<div dir="ltr"><div>Hi Andy,</div><div><br></div><div>Thanks for your review,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 3 Mar 2022 at 16:11, Andy Shevchenko <<a href="mailto:andriy.shevchenko@linux.intel.com" target="_blank">andriy.shevchenko@linux.intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Mar 03, 2022 at 02:35:58PM +0200, Tali Perry wrote:<br>
> > On Thu, Mar 3, 2022 at 12:45 PM Andy Shevchenko <<a href="mailto:andriy.shevchenko@linux.intel.com" target="_blank">andriy.shevchenko@linux.intel.com</a>> wrote:<br>
> > > On Thu, Mar 03, 2022 at 04:31:41PM +0800, Tyrone Ting wrote:<br>
<br>
...<br>
<br>
> > > > -     left_in_fifo = FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,<br>
> > > > -                              ioread8(bus->reg + NPCM_I2CTXF_STS));<br>
> > > > +     left_in_fifo = (bus->data->txf_sts_tx_bytes &<br>
> > > > +                     ioread8(bus->reg + NPCM_I2CTXF_STS));<br>
> > ><br>
> > > Besides too many parentheses, this is an interesting change. So, in different<br>
> > > versions of IP the field is on different bits? Perhaps it means that you need<br>
> > > something like internal ops structure for all these, where you will have been<br>
> > > using the statically defined masks?<br>
> > ><br>
> <br>
> Those are two very similar modules. The first generation had a 16 bytes HW FIFO<br>
> and the second generation has 32 bytes.<br>
> In V1 of this patchset the masks were defined under<br>
> CONFIG but we were asked to change the approach:<br>
> <br>
> the entire discussion can be found here:<br>
> <br>
> <a href="https://www.spinics.net/lists/linux-i2c/msg55566.html" rel="noreferrer" target="_blank">https://www.spinics.net/lists/linux-i2c/msg55566.html</a><br>
> <br>
> Did we understand the request change right?<br>
<br>
Not really. If you have not simply "one (MSB) bit more" for FIFO size, then<br>
I proposed to create a specific operations structure and use callbacks (see<br>
drivers/dma/dw/ case for iDMA 32-bit vs. DesignWare).<br>
<br>
But hold on and read set of questions below.<br>
<br>
Previously it was a fixed field with the NPCM_I2CTXF_STS_TX_BYTES mask applied,<br>
right? From above I have got that FIFO is growing twice. Is it correct?<br></blockquote><div> </div><div>What do you mean by growing twice? TX and RX?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Does the LSB stay at the same offset? What is the meaning of the MSB in 32 byte<br>
case? If it's reserved then why not to always use 32 byte approach?<br></blockquote><div> </div><div>Yes, the LSB stays in the same place, and bit 5 is reserved in the NPCM7XX SoC.</div><div>Unfortunately, the I2C test failed when we tried to use the 32 bytes approach at NPCM7XX Soc, this is why we added NPCM_I2CTXF_STS_TX_BYTES and NPCM_I2C_STSRXF_RX_BYTES to the data structure.</div><div><br></div><div>The device tree data struct<font face="arial, sans-serif">ure <span style="color:rgb(51,51,51);font-variant-ligatures:none">pass data for each specific device, so </span></font>I don't understand why not use device tree data for supporting the I2C specific device?<font face="arial, sans-serif"><span style="color:rgb(51,51,51);font-variant-ligatures:none"> this is not the case here?</span></font></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-- <br>
With Best Regards,<br>
Andy Shevchenko<br>
<br>
<br></blockquote><div><br></div><div>Best Regards,</div><div><br></div><div>Tomer Maimon </div></div></div>