Re: [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Dec 02, 2022 at 11:22:19AM +0800, Binbin Zhou wrote:
> 在 2022/12/1 04:41, Andy Shevchenko 写道:
> > On Wed, Nov 30, 2022 at 01:56:20PM +0800, Binbin Zhou wrote:

...

> > > +	for (retry = 0; retry < adap->retries; retry++) {
> > > +		ret = ls2x_i2c_doxfer(adap, msgs, num);
> > > +		if (ret != -EAGAIN)
> > > +			return ret;
> > > +
> > > +		dev_dbg(priv->dev, "Retrying transmission (%d)\n", retry);
> > > +		udelay(100);
> > Why atomic? This long (esp. atomic) delay must be explained.
> 
> The modification records for this part of the source code are no longer
> traceable.
> 
> Communicating with colleagues offline, I learned that this part of the code
> first appeared on Linux 2.6.36, which was done to circumvent the problem of
> probable failure to scan the device for i2c devices on some boards.
> 
> How about I add a comment here to explain the reason for this?

Yes, that's what we want, and not what you said above. I.o.w. the comment like
"reason is unknown" is not accepted.

Can you be more specific about the boards and why do you still need this delay?

And also why is it atomic?

> > > +	}

...

> > > +	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
> > > +				IRQF_SHARED, "ls2x-i2c", priv);
> > Indentation.
> 
> Do you mean  "IRQF_SHARE"  should be aligned to "dev"  ?

Yes.

...

> > > +static const struct dev_pm_ops ls2x_i2c_pm_ops = {
> > > +	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend, ls2x_i2c_resume)
> > > +};
> > Use corresponding DEFINE_ macro.
> 
> ok.
> 
> I will use
> 
> "static DEFINE_SIMPLE_DEV_PM_OPS(ls2x_i2c_pm_ops, ls2x_i2c_suspend,
> ls2x_i2c_resume);"  corresponding to  ".pm     = pm_ptr(&ls2x_i2c_pm_ops),"

Shouldn't be pm_sleep_ptr()?


-- 
With Best Regards,
Andy Shevchenko