Hi Andy:

Thanks for your continued review of the patch.

On Thu, Mar 5, 2026 at 9:52 PM Andy Shevchenko
[off-list ref] wrote:

On Thu, Mar 5, 2026 at 2:57 PM Binbin Zhou [off-list ref] wrote:

quoted

This I2C module is integrated into the Loongson-2K0300 SoCs.

It provides multi-master functionality and controls all I2C bus-specific
timing, protocols, arbitration, and timing. It supports both standard
and fast modes.

...

quoted

+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>

+ time.h // for USEC_PER_* et alia.

quoted

+#include <linux/types.h>
+#include <linux/units.h>

...

quoted

+/**
+ * struct loongson2_i2c_priv - private data of the controller
+ * @adapter: I2C adapter for this controller
+ * @complete: completion of I2C message
+ * @clk: hw i2c clock
+ * @regmap: regmap of the I2C device
+ * @parent_rate: I2C clock parent rate in MHz
+ * @msg: I2C transfer information
+ */
+struct loongson2_i2c_priv {
+       struct i2c_adapter              adapter;
+       struct completion               complete;
+       struct clk                      *clk;
+       struct regmap                   *regmap;
+       unsigned long                   parent_rate;

it's better to keep units in the variable name

       unsigned long                   parent_rate_MHz;

// yes, it's fine to spell the unit suffix as it's in physics.

quoted

+       struct loongson2_i2c_msg        msg;
+};

...

quoted

+static void loongson2_i2c_handle_rx_done(struct loongson2_i2c_priv *priv)
+{
+       struct loongson2_i2c_msg *msg = &priv->msg;

quoted

+       u32 i;

Not needed to be here (see below how).

quoted

+       switch (msg->count) {
+       case 2:
+               /*
+                * The STOP/START bit has to be set before reading the last two bytes.
+                * After that, we could read the last two bytes.
+                */
+               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_OP_MASK,
+                                  msg->stop ? LOONGSON2_I2C_CR1_STOP : LOONGSON2_I2C_CR1_START);

quoted

+               for (i = 2; i > 0; i--)
+                       loongson2_i2c_read_msg(priv);

First of all, in another case the type of iterator is unsigned int.
Second, this iterator is not used outside of the for-loop, third, the
upper limit is already known, no need to use magic, hence

               for (unsigned int i = 0; i < msg->count; i++)

Here, we read the last two bytes. In `loongson2_i2c_read_msg()`,
`msg->count` is decremented, so the loop condition here should be:

                     for (unsigned int i = msg->count; i > 0; i--)

Also, I will address all other comments in the next version.

quoted

+               loongson2_i2c_disable_irq(priv);
+
+               complete(&priv->complete);
+               break;
+       case 3:
+               /*
+                * In order to generate the NACK after the last received data byte, enable NACK
+                * before reading N-2 data
+                */
+               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_ACK, 0);
+               loongson2_i2c_read_msg(priv);
+               break;
+       default:
+               loongson2_i2c_read_msg(priv);
+               break;
+       }
+}

...

quoted

+static int loongson2_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[], int num)
+{
+       struct loongson2_i2c_priv *priv = i2c_get_adapdata(i2c_adap);
+       struct device *dev = priv->adapter.dev.parent;
+       unsigned int i, status;
+       int ret;
+
+       /* Wait I2C bus free */
+       ret = regmap_read_poll_timeout(priv->regmap, LOONGSON2_I2C_SR2, status,
+                                      !(status & LOONGSON2_I2C_SR2_BUSY),
+                                      LOONGSON2_I2C_FREE_SLEEP_US,
+                                      LOONGSON2_I2C_FREE_TIMEOUT_US);
+       if (ret) {
+               dev_dbg(dev, "The I2C bus is busy now.\n");
+               return ret;
+       }
+
+       /* Start generation */
+       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_START,
+                          LOONGSON2_I2C_CR1_START);

quoted

+       for (i = 0; i < num; i++) {

       for (unsigned int i = 0; i < num; i++) {

quoted

+               ret = loongson2_i2c_xfer_msg(priv, &msgs[i], i == num - 1);
+               if (ret < 0)
+                       return ret;
+       }
+
+       return num;
+}

...

quoted

+static int loongson2_i2c_adjust_bus_speed(struct loongson2_i2c_priv *priv)
+{
+       struct device *dev = priv->adapter.dev.parent;
+       struct i2c_timings i2c_t;
+       u32 val, freq_mhz, ccr;

freq_MHz
(or at least make the parent_rate and this one consistent with the
suffixes in use)

quoted

+       i2c_parse_fw_timings(dev, &i2c_t, true);
+       priv->parent_rate = clk_get_rate(priv->clk);
+
+       if (i2c_t.bus_freq_hz == I2C_MAX_STANDARD_MODE_FREQ) {
+                /* Select Standard mode */
+               ccr = 0;
+               val = DIV_ROUND_UP(priv->parent_rate, i2c_t.bus_freq_hz * 2);
+       } else if (i2c_t.bus_freq_hz == I2C_MAX_FAST_MODE_FREQ) {
+               /* Select Fast mode */
+               ccr = LOONGSON2_I2C_CCR_FS;
+               val = DIV_ROUND_UP(priv->parent_rate, i2c_t.bus_freq_hz * 3);
+       } else {

quoted

+               dev_err(dev, "Unsupported speed (%uhz)\n", i2c_t.bus_freq_hz);

hz --> Hz (in the message)

quoted

+               return -EINVAL;
+       }

It seems part of the probe phase, so why not

  return dev_err_probe(...);

?

quoted

+       FIELD_MODIFY(LOONGSON2_I2C_CCR_CCR, &ccr, val);
+       regmap_write(priv->regmap, LOONGSON2_I2C_CCR, ccr);
+
+       freq_mhz = DIV_ROUND_UP(priv->parent_rate, HZ_PER_MHZ);
+       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_FREQ,
+                          FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq_mhz));
+
+       regmap_update_bits(priv->regmap, LOONGSON2_I2C_TRISE, LOONGSON2_I2C_TRISE_SCL,
+                          LOONGSON2_I2C_TRISE_SCL);
+
+       /* Enable I2C */
+       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_PE,
+                          LOONGSON2_I2C_CR1_PE);
+
+       return 0;
+}

--
With Best Regards,
Andy Shevchenko

-- 
Thanks.
Binbin