Re: [PATCH v3 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Feb 25, 2026 at 03:34:44PM +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.

...

> +struct loongson2_i2c_priv {
> +	struct i2c_adapter		adapter;
> +	struct completion		complete;
> +	struct clk			*clk;
> +	struct regmap			*regmap;

> +	int				speed;
> +	int				parent_rate;

May any of these two be negative? The kernel doc says nothing about that.

> +	struct loongson2_i2c_msg	msg;
> +};

...

> +static void loongson2_i2c_handle_read(struct loongson2_i2c_priv *priv, int flag)
> +{
> +	struct loongson2_i2c_msg *msg = &priv->msg;
> +	unsigned int i;
> +	bool changed;
> +
> +	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 = msg->count; i > 0; i--)
> +			loongson2_i2c_read_msg(priv);
> +
> +		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;

Instead better to spell it explicitly:

		loongson2_i2c_read_msg(priv);
		break;

> +	default:
> +		loongson2_i2c_read_msg(priv);

Missing break.

> +	}
> +}

This also make me think for the possible optimisations.
if regmap_updates_bit() in case 2 can be run after the completion,
the case 1 with a parameter may be split to a helper.

Also would be nice to have a comment why in case 3 we only read a single
message.

...

> +static void loongson2_i2c_handle_write(struct loongson2_i2c_priv *priv)
> +{
> +	struct loongson2_i2c_msg *msg = &priv->msg;
> +
> +	if (msg->count) {
> +		loongson2_i2c_write_msg(priv, *msg->buf++);

> +		msg->count--;
> +		if (!msg->count)

Can be

		if (!--msg->count)

but someone may find it difficult to correctly parse.

> +			regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
> +					   LOONGSON2_I2C_CR2_ITBUFEN, 0);
> +	} else {
> +		loongson2_i2c_terminate_xfer(priv);
> +	}
> +}

...

> +static int loongson2_i2c_adjust_bus_speed(struct loongson2_i2c_priv *priv)
> +{
> +	u32 val, freq_mhz, ccr;
> +
> +	priv->parent_rate = clk_get_rate(priv->clk);

> +	freq_mhz = DIV_ROUND_UP(priv->parent_rate, HZ_PER_MHZ);

Usually the rule of thumb to put assignments close to its first user...

> +	if (priv->speed == I2C_MAX_STANDARD_MODE_FREQ) {
> +		 /* Select Standard mode */
> +		ccr = 0;
> +		val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_STANDARD_MODE_FREQ * 2);
> +	} else {
> +		/* Select Fast mode */
> +		ccr = LOONGSON2_I2C_CCR_FS;
> +		val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_FAST_MODE_FREQ * 3);
> +	}
> +
> +	FIELD_MODIFY(LOONGSON2_I2C_CCR_CCR, &ccr, val);
> +	regmap_write(priv->regmap, LOONGSON2_I2C_CCR, ccr);

...somewhere here.

> +	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;
> +}

...

> +	priv->speed = I2C_MAX_STANDARD_MODE_FREQ;
> +	ret = device_property_read_u32(dev, "clock-frequency", &clk_rate);
> +	if (!ret && clk_rate >= I2C_MAX_FAST_MODE_FREQ)
> +		priv->speed = I2C_MAX_FAST_MODE_FREQ;

Please, use i2c_parse_fw_timings() instead of custom approach.

-- 
With Best Regards,
Andy Shevchenko