Re: [PATCH V8 3/4] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Andy Shevchenko <andy@kernel.org>]

On Fri, Dec 23, 2022 at 05:00:51PM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.

...

> +static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv,
> +			     struct i2c_msg *msg, bool stop)
> +{
> +	int ret;
> +	bool is_read = msg->flags & I2C_M_RD;
> +
> +	/* Contains steps to send start condition and address */
> +	ret = ls2x_i2c_start(priv, msg);
> +	if (!ret) {
> +		if (is_read)
> +			ret = ls2x_i2c_rx(priv, msg->buf, msg->len);
> +		else
> +			ret = ls2x_i2c_tx(priv, msg->buf, msg->len);
> +
> +		if (!ret && stop)
> +			ret = ls2x_i2c_stop(priv);
> +	}
> +
> +	if (ret == -ENXIO)
> +		ls2x_i2c_stop(priv);
> +	else if (ret < 0)
> +		ls2x_i2c_init(priv);
> +
> +	return ret;
> +}

Still this code is odd from reader's perspective. It's in particular not clear
if the stop can be called twice in a row. I recommend to split it to two
functions and then do something like

_read_one()
{
	ret = start();
	if (ret)
		goto _stop; // Do we really need this?

		ret = rx();
		if (ret)
			goto _stop; // Do we need this?

		/* By setting this call the stop */
		if (stop)
			ret = -ENXIO;

	out_send_stop:
		if (ret == ...)
			return _stop();
		// I don't like above, so this error checking/setting parts
		// also can be rethought and refactored accordingly

		return ret;
}


	if (is_read)
		ret = _read_one();
	else
		ret = _write_one();

	if (ret)
		_init();

	return ret;


-- 
With Best Regards,
Andy Shevchenko