On Mon, Dec 19, 2022 at 08:28:33PM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.
Almost there, see my comments below (note, you have ~1w of time before this
can be applied anyway, so take you time for carefully addressing comments,
(re-)testing, etc.).
...
> @@ -888,6 +888,17 @@ config I2C_OWL
> Say Y here if you want to use the I2C bus controller on
> the Actions Semiconductor Owl SoC's.
>
> +config I2C_LS2X
I believe in Latin alphabet L goes before O...
> + tristate "Loongson LS2X I2C adapter"
> + depends on MACH_LOONGSON64 || COMPILE_TEST
> + help
> + If you say yes to this option, support will be included for the
> + I2C interface on the Loongson-2K SoCs and Loongson LS7A bridge
> + chip.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-ls2x.
> +
> config I2C_PASEMI
> tristate "PA Semi SMBus interface"
> depends on PPC_PASEMI && PCI
...
> obj-$(CONFIG_I2C_MXS) += i2c-mxs.o
> obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o
> obj-$(CONFIG_I2C_NPCM) += i2c-npcm7xx.o
> +obj-$(CONFIG_I2C_LS2X) += i2c-ls2x.o
...and even before n and m.
> obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o
> obj-$(CONFIG_I2C_OMAP) += i2c-omap.o
> obj-$(CONFIG_I2C_OWL) += i2c-owl.o
...
> +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;
> + 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) {
> + 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);
So, we will send stop here...
> + }
> + if (ret == -ENXIO)
> + ls2x_i2c_stop(adap);
...and if it fails, we send it again here. Is it okay?
> + else if (ret < 0)
> + ls2x_i2c_init(priv);
> +
> + return ret;
> +}
...
> + ret = ls2x_i2c_xfer_one(adap, msg, msg == (emsg - 1));
Too many parentheses, isn't it?
> + if (ret)
> + return ret;
...
> + r = devm_request_irq(dev, irq, ls2x_i2c_isr, IRQF_SHARED, "ls2x-i2c",
> + priv);
Everywhere else you use 'ret', why is 'r' here?
> + if (r < 0)
> + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
--
With Best Regards,
Andy Shevchenko
