Hi Andy:
On Tue, Dec 13, 2022 at 10:58 PM Andy Shevchenko <andy@xxxxxxxxxx> wrote:
>
> On Tue, Dec 13, 2022 at 04:26:57PM +0800, Binbin Zhou wrote:
> > This I2C module is integrated into the Loongson-2K SoCs and Loongson
> > LS7A bridge chip.
>
> ...
>
> > +// SPDX-License-Identifier: GPL-2.0
>
> GPL-2.0-only
>
> > +/*
> > + * Loongson-2K/Loongson LS7A I2C master mode driver
> > + *
> > + * Copyright (C) 2013 Loongson Technology Corporation Limited.
> > + * Copyright (C) 2014-2017 Lemote, Inc.
> > + * Copyright (C) 2018-2022 Loongson Technology Corporation Limited.
> > + *
> > + * Originally written by liushaozong
>
> > + *
>
> No need to have this blank line.
>
> > + * Rewritten for mainline by Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> > + */
>
> ...
>
> > +#include <linux/bits.h>
> > +#include <linux/completion.h>
> > +#include <linux/device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/platform_device.h>
>
> + property.h at least for dev_fwnode().
>
> > +#include <linux/units.h>
>
> ...
>
> > +#define I2C_LS2X_PRER_LO 0x0 /* Freq Division Low Byte Register */
> > +#define I2C_LS2X_PRER_HI 0x1 /* Freq Division High Byte Register */
>
> _HI is not used, can we just drop the suffix?
I will drop it.
>
> ...
>
> > +struct ls2x_i2c_priv {
>
> > + struct i2c_adapter adapter;
>
> > + struct device *dev;
>
> In some cases you are using adapter.dev, in some this one.
> Also isn't it the dev is the same as adapter.dev.parent?
>
> Hence, why do you need this one?
I checked the use of 'dev' and it can be completely replaced by
'adapter.dev.parent'. I will drop it.
>
> > + void __iomem *base;
> > + struct i2c_timings i2c_t;
> > + struct completion cmd_complete;
> > +};
>
> ...
>
> > + return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
>
> Too many parentheses.
>
> ...
>
> > +static int ls2x_i2c_xfer_one(struct i2c_adapter *adap,
> > + struct i2c_msg *msg, bool stop)
> > +{
> > + int ret;
> > + bool is_read = msg->flags & I2C_M_RD;
> > +
> > + /* Contains steps to send start condition and address */
> > + ret = ls2x_i2c_start(adap, msg);
> > + if (ret)
> > + return ret;
> > +
> > + if (is_read)
> > + ret = ls2x_i2c_rx(adap, msg->buf, msg->len);
> > + else
> > + ret = ls2x_i2c_tx(adap, msg->buf, msg->len);
> > +
> > + /* could not acquire bus. bail out without STOP */
> > + if (ret == -EAGAIN)
> > + return ret;
>
> So, if ret is *not* 0 and *not* --EAGAIN, why don't we bail out here? It needs
> at least a comment.
After more tests, these judgment conditions may be incomplete.
The following judgment may make more sense:
1. The return value of ls2x_i2c_start() should also be judged.
2. When ret = -ENXIO (no ack), we should stop the bus; for other
errors, such as "arbitration lost", we should reinitialize the i2c
bus.
As follows:
@@ -162,23 +222,24 @@ static int ls2x_i2c_xfer_one(struct i2c_adapter *adap,
{
int ret;
bool is_read = msg->flags & I2C_M_RD;
+ struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
/* Contains steps to send start condition and address */
ret = ls2x_i2c_start(adap, msg);
- if (ret)
- return ret;
-
- if (is_read)
- ret = ls2x_i2c_rx(adap, msg->buf, msg->len);
- else
- ret = ls2x_i2c_tx(adap, msg->buf, msg->len);
-
- /* could not acquire bus. bail out without STOP */
- if (ret == -EAGAIN)
- return ret;
+ if (!ret) {
+ if (is_read)
+ ret = ls2x_i2c_rx(adap, msg->buf, msg->len);
+ else
+ ret = ls2x_i2c_tx(adap, msg->buf, msg->len);
+
+ if (!ret && stop)
+ ret = ls2x_i2c_stop(adap);
+ }
- if (stop)
- ret = ls2x_i2c_stop(adap);
+ if (ret == -ENXIO)
+ ls2x_i2c_stop(adap);
+ else if (ret < 0)
+ ls2x_i2c_reginit(priv);
return ret;
}
>
> > + if (stop)
> > + ret = ls2x_i2c_stop(adap);
> > +
> > + return ret;
> > +}
>
> ...
>
> > +static int ls2x_i2c_master_xfer(struct i2c_adapter *adap,
> > + struct i2c_msg *msgs, int num)
> > +{
> > + int ret;
> > + struct i2c_msg *msg, *emsg = msgs + num;
> > +
> > + for (msg = msgs; msg < emsg; msg++) {
> > + /* Emit STOP if it is the last message or I2C_M_STOP is set */
>
> > + bool stop = (msg + 1 == emsg) || (msg->flags & I2C_M_STOP);
>
> I'm wondering if we can always set the _STOP bit in the flags of the last
> message before entering this loop. In such case you can reduce to one part
> and supply as a parameter directly.
Here, I think the I2C_M_STOP flag can be ignored as long as the last
msg is checked.
As follows:
@@ -190,10 +190,7 @@ static int ls2x_i2c_master_xfer(struct i2c_adapter *adap,
struct i2c_msg *msg, *emsg = msgs + num;
for (msg = msgs; msg < emsg; msg++) {
- /* Emit STOP if it is the last message or I2C_M_STOP is set */
- bool stop = (msg + 1 == emsg) || (msg->flags & I2C_M_STOP);
-
- ret = ls2x_i2c_xfer_one(adap, msg, stop);
+ ret = ls2x_i2c_xfer_one(adap, msg, msg == (emsg - 1));
if (ret)
return ret;
}
>
> > +
> > + ret = ls2x_i2c_xfer_one(adap, msg, stop);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return num;
> > +}
>
> ...
>
> > + /* Calculate and set LS2X I2C frequency */
> > + writel(LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1,
> > + priv->base + I2C_LS2X_PRER_LO);
>
> writel()?! Shouldn't be writew()?
Sorry, it's my fault, it should be writew().
>
> ...
>
> > + r = devm_request_irq(dev, irq, ls2x_i2c_isr,
> > + IRQF_SHARED, "ls2x-i2c", priv);
>
> There is a room on the previous line for at least one more argument.
>
> > + if (r < 0)
> > + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
>
> ...
>
> > +subsys_initcall(ls2x_i2c_init_driver);
>
> Non-standard init calls should be commented.
On the LS7A bridge chip, the display subsystem depends on it. The i2c
channel to read the EEID is fixed and the i2c driver should be
initialized earlier than it, otherwise it will cause conflicts.
I will comment this.
Thanks.
Binbin
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
