Hi Andy:

Thanks for your detailed review.

On Tue, Jan 27, 2026 at 4:18 PM Andy Shevchenko
[off-list ref] wrote:

On Tue, Jan 27, 2026 at 10:47:57AM +0800, Binbin Zhou 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.

Thanks for the update, but either v1 was not reviewed properly (if reviewed
at all), or this missed comments from there. This driver needs much more work.
Since it misses v6.20 anyway, you have about 2 month to put this into a shape.

...

quoted

+/*
+ * Loongson-2K fast I2C controller driver
+ *
+ * Copyright (C) 2025-2026 Loongson Technology Corporation Limited

quoted

+ *

Redundant blank line.

ok..

quoted

+ */

...

quoted

+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>

quoted

+#include <linux/device.h>

Is this being used?

quoted

+#include <linux/iopoll.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>

quoted

+#include <linux/kernel.h>

No way you should include this one.

quoted

+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>

Please, follow IWYU principle, exempli gratia here the types.h is missing.

I will check the included header files.

quoted

+#include <linux/units.h>

...

quoted

+#define LOONGSON2_I2C_CR2_IRQ_MASK   (LOONGSON2_I2C_CR2_ITBUFEN | \
+                                      LOONGSON2_I2C_CR2_ITEVTEN | \
+                                      LOONGSON2_I2C_CR2_ITERREN)

Better to indent differently:

#define LOONGSON2_I2C_CR2_IRQ_MASK      \
        (LOONGSON2_I2C_CR2_ITBUFEN | LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN)

ok. I will check.

...

quoted

+#define LOONGSON2_I2C_SR1_ITEVTEN_MASK       (LOONGSON2_I2C_SR1_BTF | \
+                                      LOONGSON2_I2C_SR1_ADDR | \
+                                      LOONGSON2_I2C_SR1_SB)
+#define LOONGSON2_I2C_SR1_ITBUFEN_MASK       (LOONGSON2_I2C_SR1_TXE | LOONGSON2_I2C_SR1_RXNE)
+#define LOONGSON2_I2C_SR1_ITERREN_MASK       (LOONGSON2_I2C_SR1_AF | \
+                                      LOONGSON2_I2C_SR1_ARLO | \
+                                      LOONGSON2_I2C_SR1_BERR)

As per above.

...

quoted

+#define LOONGSON2_I2C_FREE_SLEEP_US  1000
+#define LOONGSON2_I2C_FREE_TIMEOUT_US        5000

1 * USEC_PER_MSEC
5 * USEC_PER_MSEC

ok...

...

quoted

+#define LOONGSON2_I2C_MIN_STD_FREQ   2U
+#define LOONGSON2_I2C_MIN_FAST_FREQ  6U
+#define LOONGSON2_I2C_MAX_FREQ               46U

The three are unused, what are they for?

I will drop it.

quoted

+#define HZ_TO_MHZ                    1000000

Dup. Use units.h.

...

quoted

+enum loongson2_i2c_speed {
+     LOONGSON2_I2C_SPEED_STANDARD, /* 100 kHz */
+     LOONGSON2_I2C_SPEED_FAST, /* 400 kHz */
+     LOONGSON2_I2C_SPEED_FAST_PLUS, /* 1 MHz */

Use existing speed definitions and drop this enum.

ok....

quoted

+     LOONGSON2_I2C_SPEED_END,

Besides comma in the terminator, that has not to be there, this is unused.

quoted

+};

...

quoted

+/*

It's not marked as kernel-doc, but looks very much like that. Why?
Same Q for *all* cases like this.

I didn't intend to mark it in kernel-doc; I just wanted to comment on
the variable.
Is removing the `@` symbol sufficient?

quoted

+ * struct loongson2_i2c_msg - client specific data
+ * @addr: 8-bit slave addr, including r/w bit
+ * @count: number of bytes to be transferred
+ * @buf: data buffer
+ * @stop: last I2C msg to be sent, i.e. STOP to be generated
+ * @result: result of the transfer
+ */
+struct loongson2_i2c_msg {
+     u8 addr;
+     u32 count;
+     u8 *buf;
+     bool stop;
+     int result;

Run `pahole` and amend *all* data types accordingly.

 Moving 'stop' from after 'buf' to after 'addr'.

quoted

+};

...

quoted

+struct loongson2_i2c_priv {
+     struct i2c_adapter adapter;

quoted

+     struct device *dev;

Isn't this a dup? Can't it be derived from regmap and/or adapter?

 it can be converted using `struct device *dev = priv->adapter.dev.parent;`

quoted

+     struct clk *clk;
+     struct completion complete;
+     struct regmap *regmap;
+     int speed;
+     int parent_rate;
+     struct loongson2_i2c_msg msg;
+};

...

After run `pahole`, the structs are reorganized as follow:

pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_priv
i2c-ls2x-v2.o
struct loongson2_i2c_priv {
        struct i2c_adapter         adapter
__attribute__((__aligned__(8))); /*     0  1064 */
        /* --- cacheline 16 boundary (1024 bytes) was 40 bytes ago ---
*/
        struct clk *               clk;                  /*  1064
8 */
        struct completion          complete;             /*  1072
32 */
        /* --- cacheline 17 boundary (1088 bytes) was 16 bytes ago ---
*/
        struct regmap *            regmap;               /*  1104
8 */
        int                        speed;                /*  1112
4 */
        int                        parent_rate;          /*  1116
4 */
        struct loongson2_i2c_msg   msg;                  /*  1120
24 */

        /* XXX last struct has 4 bytes of padding */

        /* size: 1144, cachelines: 18, members: 7 */
        /* paddings: 1, sum paddings: 4 */
        /* forced alignments: 1 */
        /* last cacheline: 56 bytes */
} __attribute__((__aligned__(8)));

pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_msg
i2c-ls2x-v2.o
struct loongson2_i2c_msg {
        u8                         addr;                 /*     0
1 */
        bool                       stop;                 /*     1
1 */

        /* XXX 2 bytes hole, try to pack */

        u32                        count;                /*     4
4 */
        u8 *                       buf;                  /*     8
8 */
        int                        result;               /*    16
4 */

        /* size: 24, cachelines: 1, members: 5 */
        /* sum members: 18, holes: 1, sum holes: 2 */
        /* padding: 4 */
        /* last cacheline: 24 bytes */
};

quoted

+static int loongson2_i2c_wait_free_bus(struct loongson2_i2c_priv *priv)
+{
+     u32 status;
+     int ret;
+
+     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(priv->dev, "I2C bus free failed.\n");

quoted

+             ret = -EBUSY;

Why?! What's wrong with the error code returned in ret?

I want to indicate the bus busy state if it times out.

quoted

+     }
+
+     return ret;
+}

...

quoted

+static void loongson2_i2c_handle_read(struct loongson2_i2c_priv *priv, int flag)
+{
+     struct loongson2_i2c_msg *msg = &priv->msg;
+     bool changed;

quoted

+     int i;

Why signed?

unsigned int i;

quoted

+     switch (msg->count) {
+     case 1:
+             /* only transmit 1 bytes condition */
+             loongson2_i2c_disable_irq(priv);
+             loongson2_i2c_read_msg(priv);
+             complete(&priv->complete);
+             break;
+     case 2:
+             if (flag != 1) {
+                     /* ensure only transmit 2 bytes condition */
+                     regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
+                                        LOONGSON2_I2C_CR2_ITBUFEN, 0);
+                     break;
+             }
+             regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_OP_MASK,
+                                msg->stop ? LOONGSON2_I2C_CR1_STOP : LOONGSON2_I2C_CR1_START);
+
+             loongson2_i2c_disable_irq(priv);

quoted

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

Just unroll the loop and put a comment on top explaining the magic 2. But I
think it's just a msg->count.

Yes it's  msg->count.

quoted

+             regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_POS, 0);
+             complete(&priv->complete);
+             break;
+     case 3:
+             regmap_update_bits_check(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_ITBUFEN,
+                                      0, &changed);
+             if (changed)
+                     break;
+             regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_ACK, 0);
+             fallthrough;
+     default:
+             loongson2_i2c_read_msg(priv);
+     }
+}

...

quoted

+static irqreturn_t loongson2_i2c_isr_error(u32 status, void *data)
+{
+     struct loongson2_i2c_priv *priv = data;
+     struct loongson2_i2c_msg *msg = &priv->msg;
+
+     /* Arbitration lost */
+     if (status & LOONGSON2_I2C_SR1_ARLO) {
+             regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_ARLO, 0);
+             msg->result = -EAGAIN;
+     }
+
+     /*
+      * Acknowledge failure:
+      * In master transmitter mode a Stop must be generated by software
+      */
+     if (status & LOONGSON2_I2C_SR1_AF) {
+             regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_STOP,
+                                LOONGSON2_I2C_CR1_STOP);
+             regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_AF, 0);
+             msg->result = -EIO;
+     }
+
+     /* Bus error */
+     if (status & LOONGSON2_I2C_SR1_BERR) {
+             regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_BERR, 0);
+             msg->result = -EIO;
+     }

Even if the interrupt is spurious you still Ack it, why?

Ah, it seems it's a helper. Please, return the error code from it as int and
not irqreturn_t, this will make things clearer.

How about just define it as void:

static void loongson2_i2c_isr_error(u32 status, void *data)
{
.........
        if (status & LOONGSON2_I2C_SR1_ARLO) {
         ........
                msg->result = -EAGAIN;
                goto out;
        }
        if (status & LOONGSON2_I2C_SR1_AF) {
           .......
                msg->result = -EIO;
                goto out;
        }
        if (status & LOONGSON2_I2C_SR1_BERR) {
         .........
                msg->result = -EIO;
                goto out;
        }

out:
        loongson2_i2c_disable_irq(priv);
        complete(&priv->complete);
}

and in loongson2_i2c_isr_event(), I reference it as:

        if (status & LOONGSON2_I2C_SR1_ITERREN_MASK) {
                loongson2_i2c_isr_error(status, data);
                return IRQ_NONE;
        }

Its return value is meaningless for loongson2_i2c_isr_event().

quoted

+     loongson2_i2c_disable_irq(priv);
+     complete(&priv->complete);
+
+     return IRQ_HANDLED;
+}

...

quoted

+static irqreturn_t loongson2_i2c_isr_event(int irq, void *data)
+{
+     u32 possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;

Split assignment...

quoted

+     struct loongson2_i2c_priv *priv = data;
+     struct loongson2_i2c_msg *msg = &priv->msg;
+     u32 status, ien, event, cr2;
+
+     regmap_read(priv->regmap, LOONGSON2_I2C_SR1, &status);
+     if (status & LOONGSON2_I2C_SR1_ITERREN_MASK)
+             return loongson2_i2c_isr_error(status, data);
+
+     regmap_read(priv->regmap, LOONGSON2_I2C_CR2, &cr2);
+     ien = cr2 & LOONGSON2_I2C_CR2_IRQ_MASK;

...to be here, which improves readability (no need to go somewhere up in
the code to see what this is about.

ok, I will put  `possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;` here.

quoted

+     /* Update possible_status if buffer interrupt is enabled */
+     if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
+             possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;
+
+     event = status & possible_status;
+     if (!event) {
+             dev_dbg(priv->dev, "spurious evt irq (status=0x%08x, ien=0x%08x)\n", status, ien);
+             return IRQ_NONE;
+     }
+
+     /* Start condition generated */
+     if (event & LOONGSON2_I2C_SR1_SB)
+             loongson2_i2c_write_msg(priv, msg->addr);
+
+     /* I2C Address sent */
+     if (event & LOONGSON2_I2C_SR1_ADDR) {
+             if (msg->addr & I2C_M_RD)
+                     loongson2_i2c_handle_rx_addr(priv);
+             /* Clear ADDR flag */
+             regmap_read(priv->regmap, LOONGSON2_I2C_SR2, &status);
+             /* Enable buffer interrupts for RX/TX not empty events */
+             regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_ITBUFEN,
+                                LOONGSON2_I2C_CR2_ITBUFEN);
+     }
+
+     if (msg->addr & I2C_M_RD) {
+             /* RX not empty */
+             if (event & LOONGSON2_I2C_SR1_RXNE)
+                     loongson2_i2c_handle_read(priv, 0);
+
+             if (event & LOONGSON2_I2C_SR1_BTF)
+                     loongson2_i2c_handle_read(priv, 1);
+     } else {
+             /* TX empty */
+             if (event & LOONGSON2_I2C_SR1_TXE)
+                     loongson2_i2c_handle_write(priv);
+
+             if (event & LOONGSON2_I2C_SR1_BTF)
+                     loongson2_i2c_handle_write(priv);
+     }
+
+     return IRQ_HANDLED;
+}

...

quoted

+static int loongson2_i2c_xfer_msg(struct loongson2_i2c_priv *priv, struct i2c_msg *msg,
+                               bool is_stop)
+{
+     struct loongson2_i2c_msg *l_msg = &priv->msg;
+     unsigned long timeout;

quoted

+     int ret;

Seems unneeded.

emm, I will drop it.

quoted

+
+     l_msg->addr   = i2c_8bit_addr_from_msg(msg);
+     l_msg->buf    = msg->buf;
+     l_msg->count  = msg->len;
+     l_msg->stop   = is_stop;
+     l_msg->result = 0;
+
+     reinit_completion(&priv->complete);
+
+     /* Enable events and errors interrupts */
+     regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
+                        LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN,
+                        LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN);

quoted

+     timeout = wait_for_completion_timeout(&priv->complete, priv->adapter.timeout);
+     ret = l_msg->result;
+
+     if (!timeout)
+             ret = -ETIMEDOUT;
+
+     return ret;

        timeout = wait_for_completion_timeout(&priv->complete, priv->adapter.timeout);
        if (!timeout)
                return -ETIMEDOUT;

        return l_msg->result;

quoted

+}

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);
+     int ret = 0, i;

Why is 'i' signed?
And redundant assignment for 'ret'.

unsigned int i;
int ret;

quoted

+     ret = loongson2_i2c_wait_free_bus(priv);
+     if (ret)
+             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 && !ret; i++)
+             ret = loongson2_i2c_xfer_msg(priv, &msgs[i], i == num - 1);
+
+     return (ret < 0) ? ret : num;

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

        return num;

Yes, this way will be more readable.

quoted

+}

...

quoted

+static int loongson2_i2c_adjust_bus_speed(struct loongson2_i2c_priv *priv)
+{
+     u32 val, freq, ccr = 0, cr2 = 0;

Redundant assignment(s), see below why.

quoted

+     priv->parent_rate = clk_get_rate(priv->clk);
+     freq = DIV_ROUND_UP(priv->parent_rate, HZ_TO_MHZ);

Use unit suffix:

        freq_mhz = ...

(if I am not mistaken).

Yes, `freq_mhz` is more appropriate.

quoted

+     cr2 |= FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq);

        cr2 = FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq);

ok..

quoted

+     regmap_write(priv->regmap, LOONGSON2_I2C_CR2, cr2);
+
+     if (priv->speed == LOONGSON2_I2C_SPEED_STANDARD) {
+             val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_STANDARD_MODE_FREQ * 2);

                /* Select Standard mode */
                ccr = 0;

quoted

+     } else {
+             val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_FAST_MODE_FREQ * 3);
+
+             /* Select Fast mode */
+             ccr |= LOONGSON2_I2C_CCR_FS;

                ccr = LOONGSON2_I2C_CCR_FS;

quoted

+     }

quoted

+     ccr |= FIELD_GET(LOONGSON2_I2C_CCR_CCR, val);

FIELD_MODIFY()?

FIELD_MODIFY(LOONGSON2_I2C_CCR_CCR, &ccr, val);  instead.

quoted

+     regmap_write(priv->regmap, LOONGSON2_I2C_CCR, ccr);
+
+     /* reference clock determination the configure val(0x3f) */
+     regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_FREQ,
+                        LOONGSON2_I2C_CR2_FREQ);
+     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;
+}

...

quoted

+static int loongson2_i2c_probe(struct platform_device *pdev)
+{
+     struct device *dev = &pdev->dev;
+     struct loongson2_i2c_priv *priv;
+     struct i2c_adapter *adap;
+     void __iomem *base;
+     u32 clk_rate;
+     int irq, ret;
+
+     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+     if (!priv)
+             return -ENOMEM;
+
+     base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(base))

quoted

+             return dev_err_probe(dev, PTR_ERR(base),
+                                  "Failed to ioremap resource.\n");

You utilise 100, put this on a single line.

ok. I will check the issue again.

But, this is a dup, devm_platform_*() call above takes care already about error
message.

emm. I will use `return PTR_ERR(base);` instead.

quoted

+
+     priv->regmap = devm_regmap_init_mmio(dev, base,
+                                          &loongson2_i2c_regmap_config);

Single line.

quoted

+     if (IS_ERR(priv->regmap))
+             return dev_err_probe(dev, PTR_ERR(priv->regmap),
+                                  "Failed to init regmap.\n");

Single line.

quoted

+     priv->clk = devm_clk_get_enabled(dev, NULL);
+     if (IS_ERR(priv->clk))
+             return dev_err_probe(dev, PTR_ERR(priv->clk),
+                                  "Failed  to enable clock.\n");

Single line.

quoted

+     irq = platform_get_irq(pdev, 0);
+     if (irq < 0)
+             return -EINVAL;

Why?! What's wrong with the error code in 'irq'?

I will use `return irq` instead.

quoted

+     priv->dev = dev;
+
+     adap = &priv->adapter;
+     adap->retries = 5;
+     adap->nr = pdev->id;
+     adap->dev.parent = dev;
+     adap->owner = THIS_MODULE;
+     adap->algo = &loongson2_i2c_algo;
+     adap->timeout = 2 * HZ;
+     device_set_node(&adap->dev, dev_fwnode(dev));
+     i2c_set_adapdata(adap, priv);

quoted

+     strscpy(adap->name, pdev->name, sizeof(adap->name));

2-arguments version is even better.

Sorry, I'm not quite sure what you mean by `2-arguments version.`

quoted

+     init_completion(&priv->complete);
+     platform_set_drvdata(pdev, priv);
+
+     priv->speed = LOONGSON2_I2C_SPEED_STANDARD;

quoted

+     ret = of_property_read_u32(dev->of_node, "clock-frequency", &clk_rate);

device_property_read_u32()

ok...

quoted

+     if (!ret && clk_rate >= I2C_MAX_FAST_MODE_FREQ)
+             priv->speed = LOONGSON2_I2C_SPEED_FAST;
+
+     ret = loongson2_i2c_adjust_bus_speed(priv);
+     if (ret)
+             return ret;
+
+     ret = devm_request_irq(dev, irq, loongson2_i2c_isr_event, IRQF_SHARED, pdev->name, priv);
+     if (ret)

quoted

+             return dev_err_probe(dev, ret, "Unable to request irq %d\n", irq);

Dup message.

ok...

                return ret;

quoted

+     return devm_i2c_add_adapter(dev, adap);
+}

...

quoted

+static struct platform_driver loongson2_i2c_driver = {
+     .driver = {
+             .name = "loongson2-i2c-v2",
+             .of_match_table = loongson2_i2c_id_table,
+     },
+     .probe = loongson2_i2c_probe,
+};

quoted

+

Remove this blank line.

ok...

quoted

+module_platform_driver(loongson2_i2c_driver);

--
With Best Regards,
Andy Shevchenko

--
Thanks.
Binbin