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
