From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Binbin Zhou <zhoubinbin@loongson.cn>
Cc: Binbin Zhou <zhoubb.aaron@gmail.com>,
Huacai Chen <chenhuacai@loongson.cn>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Andi Shyti <andi.shyti@kernel.org>,
Wolfram Sang <wsa+renesas@sang-engineering.com>,
Andy Shevchenko <andy@kernel.org>,
linux-i2c@vger.kernel.org, Huacai Chen <chenhuacai@kernel.org>,
Xuerui Wang <kernel@xen0n.name>,
loongarch@lists.linux.dev, devicetree@vger.kernel.org
Subject: Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller
Date: Tue, 27 Jan 2026 10:18:27 +0200 [thread overview]
Message-ID: <aXh000kBfaqd1G9H@smile.fi.intel.com> (raw)
In-Reply-To: <402121da829497fc97f1461c8aaa3a44252c3f06.1769476820.git.zhoubinbin@loongson.cn>
On Tue, Jan 27, 2026 at 10:47:57AM +0800, Binbin Zhou wrote:
> 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.
...
> +/*
> + * Loongson-2K fast I2C controller driver
> + *
> + * Copyright (C) 2025-2026 Loongson Technology Corporation Limited
> + *
Redundant blank line.
> + */
...
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
Is this being used?
> +#include <linux/iopoll.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
No way you should include this one.
> +#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.
> +#include <linux/units.h>
...
> +#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)
...
> +#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.
...
> +#define LOONGSON2_I2C_FREE_SLEEP_US 1000
> +#define LOONGSON2_I2C_FREE_TIMEOUT_US 5000
1 * USEC_PER_MSEC
5 * USEC_PER_MSEC
...
> +#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?
> +#define HZ_TO_MHZ 1000000
Dup. Use units.h.
...
> +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.
> + LOONGSON2_I2C_SPEED_END,
Besides comma in the terminator, that has not to be there, this is unused.
> +};
...
> +/*
It's not marked as kernel-doc, but looks very much like that. Why?
Same Q for *all* cases like this.
> + * 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.
> +};
...
> +struct loongson2_i2c_priv {
> + struct i2c_adapter adapter;
> + struct device *dev;
Isn't this a dup? Can't it be derived from regmap and/or adapter?
> + struct clk *clk;
> + struct completion complete;
> + struct regmap *regmap;
> + int speed;
> + int parent_rate;
> + struct loongson2_i2c_msg msg;
> +};
...
> +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");
> + ret = -EBUSY;
Why?! What's wrong with the error code returned in ret?
> + }
> +
> + return ret;
> +}
...
> +static void loongson2_i2c_handle_read(struct loongson2_i2c_priv *priv, int flag)
> +{
> + struct loongson2_i2c_msg *msg = &priv->msg;
> + bool changed;
> + int i;
Why signed?
> + 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);
> + 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.
> + 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);
> + }
> +}
...
> +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.
> + loongson2_i2c_disable_irq(priv);
> + complete(&priv->complete);
> +
> + return IRQ_HANDLED;
> +}
...
> +static irqreturn_t loongson2_i2c_isr_event(int irq, void *data)
> +{
> + u32 possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;
Split assignment...
> + 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.
> + /* 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;
> +}
...
> +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;
> + int ret;
Seems unneeded.
> +
> + 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);
> + 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;
> +}
> +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'.
> + 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);
> + 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;
> +}
...
> +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.
> + 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).
> + cr2 |= FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq);
cr2 = FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq);
> + 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;
> + } 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;
> + }
> + ccr |= FIELD_GET(LOONGSON2_I2C_CCR_CCR, val);
FIELD_MODIFY()?
> + 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;
> +}
...
> +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))
> + return dev_err_probe(dev, PTR_ERR(base),
> + "Failed to ioremap resource.\n");
You utilise 100, put this on a single line.
But, this is a dup, devm_platform_*() call above takes care already about error
message.
> +
> + priv->regmap = devm_regmap_init_mmio(dev, base,
> + &loongson2_i2c_regmap_config);
Single line.
> + if (IS_ERR(priv->regmap))
> + return dev_err_probe(dev, PTR_ERR(priv->regmap),
> + "Failed to init regmap.\n");
Single line.
> + 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.
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return -EINVAL;
Why?! What's wrong with the error code in 'irq'?
> + 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);
> + strscpy(adap->name, pdev->name, sizeof(adap->name));
2-arguments version is even better.
> + init_completion(&priv->complete);
> + platform_set_drvdata(pdev, priv);
> +
> + priv->speed = LOONGSON2_I2C_SPEED_STANDARD;
> + ret = of_property_read_u32(dev->of_node, "clock-frequency", &clk_rate);
device_property_read_u32()
> + 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)
> + return dev_err_probe(dev, ret, "Unable to request irq %d\n", irq);
Dup message.
return ret;
> + return devm_i2c_add_adapter(dev, adap);
> +}
...
> +static struct platform_driver loongson2_i2c_driver = {
> + .driver = {
> + .name = "loongson2-i2c-v2",
> + .of_match_table = loongson2_i2c_id_table,
> + },
> + .probe = loongson2_i2c_probe,
> +};
> +
Remove this blank line.
> +module_platform_driver(loongson2_i2c_driver);
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2026-01-27 8:18 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-27 2:47 [PATCH v2 0/2] i2c: Add Loongson-2K0300 I2C controller support Binbin Zhou
2026-01-27 2:47 ` [PATCH v2 1/2] dt-bindings: i2c: loongson,ls2x: Add ls2k0300-i2c compatible Binbin Zhou
2026-01-27 2:47 ` [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller Binbin Zhou
2026-01-27 8:18 ` Andy Shevchenko [this message]
2026-01-29 8:07 ` Binbin Zhou
2026-01-29 14:38 ` Andy Shevchenko
2026-02-02 6:39 ` Binbin Zhou
2026-02-03 9:53 ` Andy Shevchenko
2026-01-28 4:15 ` Huacai Chen
2026-01-28 9:08 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aXh000kBfaqd1G9H@smile.fi.intel.com \
--to=andriy.shevchenko@intel.com \
--cc=andi.shyti@kernel.org \
--cc=andy@kernel.org \
--cc=chenhuacai@kernel.org \
--cc=chenhuacai@loongson.cn \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=kernel@xen0n.name \
--cc=krzk+dt@kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=loongarch@lists.linux.dev \
--cc=robh@kernel.org \
--cc=wsa+renesas@sang-engineering.com \
--cc=zhoubb.aaron@gmail.com \
--cc=zhoubinbin@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox