Hi Andy:
在 2022/12/1 04:41, Andy Shevchenko 写道:
On Wed, Nov 30, 2022 at 01:56:20PM +0800, Binbin Zhou wrote:
This I2C module is integrated into the Loongson-2K SoCs and Loongson
LS7A bridge chip.
Looks like some of my comments were not addressed. Are you sure
you have read the previous reviews carefully?
...
+static int ls2x_i2c_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ int ret, retry;
+ struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+ 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?
+ }
+
+ return -EREMOTEIO;
+}
...
+static void ls2x_i2c_reginit(struct ls2x_i2c_priv *priv)
+{
+ u8 data;
+
+ /* Enable operations about frequency divider register */
+ data = readb(priv->base + I2C_LS2X_CTR);
+ writeb(data & ~0x80, priv->base + I2C_LS2X_CTR);
Magic number.
+ ls2x_i2c_adjust_bus_speed(priv);
+
+ /* Set to normal I2C operating mode and enable interrupts */
+ data = readb(priv->base + I2C_LS2X_CTR);
+ writeb(data | 0xe0, priv->base + I2C_LS2X_CTR);
Ditto.
OK.. I will use the macro to define the corresponding bit.
@@ -48,6 +48,11 @@
#define LS2X_SR_TIP BIT(1)
#define LS2X_SR_IF BIT(0)
+/* Control Register Bit */
+#define LS2X_CTR_EN BIT(7) /* 0: Frequency setting operation */
+#define LS2X_CTR_IEN BIT(6) /* Enable i2c interrupt */
+#define LS2X_CTR_MST BIT(5) /* 0 = Slave 1 = Master */
+
#define LS2X_I2C_MAX_RETRIES 5
struct ls2x_i2c_priv {
@@ -266,13 +271,14 @@ static void ls2x_i2c_reginit(struct ls2x_i2c_priv
*priv)
/* Enable operations about frequency divider register */
data = readb(priv->base + I2C_LS2X_CTR);
- writeb(data & ~0x80, priv->base + I2C_LS2X_CTR);
+ writeb(data & ~LS2X_CTR_EN, priv->base + I2C_LS2X_CTR);
ls2x_i2c_adjust_bus_speed(priv);
/* Set to normal I2C operating mode and enable interrupts */
data = readb(priv->base + I2C_LS2X_CTR);
- writeb(data | 0xe0, priv->base + I2C_LS2X_CTR);
+ writeb(data | LS2X_CTR_EN | LS2X_CTR_IEN | LS2X_CTR_MST,
+ priv->base + I2C_LS2X_CTR);
+}
...
+ 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" ?
+ if (r < 0)
+ return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
...
+ adap->dev.of_node = pdev->dev.of_node;
Why is this needed?
+ device_set_node(&adap->dev, dev_fwnode(dev));
...
+ /* i2c device drivers may be active on return from add_adapter() */
+ r = i2c_add_adapter(adap);
Why not use devm_ variant of this?
+ if (r)
+ return dev_err_probe(dev, r, "Failure adding adapter\n");
...
+static int ls2x_i2c_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
Can't you use dev_get_drvdata() directly? Why?
+
+ priv->suspended = 1;
+
+ return 0;
+}
+
+static int ls2x_i2c_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
Ditto.
+ priv->suspended = 0;
+ ls2x_i2c_reginit(priv);
+
+ return 0;
+}
...
+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),"
Thanks.
Binbin
