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

[Andy Shevchenko <andy@kernel.org>]

On Mon, Dec 19, 2022 at 08:28:33PM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.

Almost there, see my comments below (note, you have ~1w of time before this
can be applied anyway, so take you time for carefully addressing comments,
(re-)testing, etc.).

...

> @@ -888,6 +888,17 @@ config I2C_OWL
>  	  Say Y here if you want to use the I2C bus controller on
>  	  the Actions Semiconductor Owl SoC's.
>  
> +config I2C_LS2X

I believe in Latin alphabet L goes before O...

> +	tristate "Loongson LS2X I2C adapter"
> +	depends on MACH_LOONGSON64 || COMPILE_TEST
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  I2C interface on the Loongson-2K SoCs and Loongson LS7A bridge
> +	  chip.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called i2c-ls2x.
> +
>  config I2C_PASEMI
>  	tristate "PA Semi SMBus interface"
>  	depends on PPC_PASEMI && PCI

...

>  obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
>  obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
>  obj-$(CONFIG_I2C_NPCM)		+= i2c-npcm7xx.o
> +obj-$(CONFIG_I2C_LS2X)		+= i2c-ls2x.o

...and even before n and m.

>  obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
>  obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
>  obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o

...

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

> +		if (!ret && stop)
> +			ret = ls2x_i2c_stop(adap);

So, we will send stop here...

> +	}

> +	if (ret == -ENXIO)
> +		ls2x_i2c_stop(adap);

...and if it fails, we send it again here. Is it okay?

> +	else if (ret < 0)
> +		ls2x_i2c_init(priv);
> +
> +	return ret;
> +}

...

> +		ret = ls2x_i2c_xfer_one(adap, msg, msg == (emsg - 1));

Too many parentheses, isn't it?

> +		if (ret)
> +			return ret;

...

> +	r = devm_request_irq(dev, irq, ls2x_i2c_isr, IRQF_SHARED, "ls2x-i2c",
> +			     priv);

Everywhere else you use 'ret', why is 'r' here?

> +	if (r < 0)
> +		return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);

-- 
With Best Regards,
Andy Shevchenko