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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




在 2022/12/2 16:29, Andy Shevchenko 写道:
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?


As we expected, the driver is geared towards Loongosn-2K and Loongson LS7A bridge chips, some of the older hardware still in use has scan failure issues and we expect to keep that part as compatible.

"Atomic" may not be necessary, It is expected to try a few more times when the scan device fails. Also, I think the corresponding part of the s3c2410 may have been referenced in the beginning.

I will try to drop the loop part to do the relevant i2c read/write tests.


Thanks.

Binbin

+	}
...

+	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()?






[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux