Re: [PATCH v2 i2c-master] i2c: microchip: pci1xxxx: Add driver for I2C host controller in multifunction endpoint of pci1xxxx switch

[<Tharunkumar.Pasumarthi@microchip.com>]

On Fri, 2022-09-02 at 17:08 +0300, Andy Shevchenko wrote:
> > > > +             if (buf)
> > > > +                     memcpy_toio((i2c->i2c_base + SMBUS_MST_BUF), buf,
> > > > transferlen);
> > > 
> > > Why do you need buf checks? Is your code can shoot itself in the foot?
> > 
> > Yes, buf will be passed as NULL in some cases. So, this check is required.
> 
> Can you show an excerpt of the caller which passes NULL?

During I2C read, for writing just the client address without passing any other
data, the buf pointer is set as NULL in pci1xxxx_i2c_buffer_write API in the
"pci1xxxx_i2c_read" function

> > > Each long sleep (20 ms is quite long) has to be explained. But this entire
> > > loop
> > > looks like a band-aid of lack of IRQ or so. Why do you need to poll?
> > 
> > This handling takes care of special case when system is put to suspend when
> > i2c
> > transfer is progress in driver. We will wait until transfer completes.
> 
> This should be at least a comment in the code.

Okay, I will add comment.
> > 

> > > You can't mix devm_ and non-devm_ in such manner. It's asking for troubles
> > > at
> > > ->remove() or unbind stages.
> > 
> > I am not getting this comment. Can you kindly explain more.
> > 
> > > > +             return ret;
> 
> Explanations [1] & [2]. Example how to workaround [3].
> 
> [1]: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1949091.html
> [2]: https://lore.kernel.org/all/YXktrG1LhK5tj2uF@smile.fi.intel.com/
> [3]: https://www.spinics.net/lists/kernel/msg4433985.html

Okay. I will update code as per this logic in the upcoming version of the patch.

> > > After fixing above, convert the error messages to use
> > > 
> > >         return dev_err_probe(...);
> > > 
> > > pattern.
> > 
> > Okay.
> 
> Will be result of above fix.

Okay

> > > 
> > > This will be gone.
> > 
> > I am not getting this comment also.
> 
> See above.

Okay




Thanks,
Tharun Kumar P