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

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

On Wed, Nov 30, 2022 at 01:56:20PM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.

Looks like some of my comments were not addressed. Are you sure
you have read the previous reviews carefully?

...

> +static int ls2x_i2c_xfer(struct i2c_adapter *adap,
> +			struct i2c_msg *msgs, int num)
> +{
> +	int ret, retry;
> +	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
> +
> +	for (retry = 0; retry < adap->retries; retry++) {
> +		ret = ls2x_i2c_doxfer(adap, msgs, num);
> +		if (ret != -EAGAIN)
> +			return ret;
> +
> +		dev_dbg(priv->dev, "Retrying transmission (%d)\n", retry);

> +		udelay(100);

Why atomic? This long (esp. atomic) delay must be explained.

> +	}
> +
> +	return -EREMOTEIO;
> +}

...

> +static void ls2x_i2c_reginit(struct ls2x_i2c_priv *priv)
> +{
> +	u8 data;
> +
> +	/* Enable operations about frequency divider register */
> +	data = readb(priv->base + I2C_LS2X_CTR);
> +	writeb(data & ~0x80, priv->base + I2C_LS2X_CTR);

Magic number.

> +	ls2x_i2c_adjust_bus_speed(priv);
> +
> +	/* Set to normal I2C operating mode and enable interrupts */
> +	data = readb(priv->base + I2C_LS2X_CTR);
> +	writeb(data | 0xe0, priv->base + I2C_LS2X_CTR);

Ditto.

> +}

...

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

Indentation.

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

...

> +	adap->dev.of_node = pdev->dev.of_node;

Why is this needed?

> +	device_set_node(&adap->dev, dev_fwnode(dev));

...

> +	/* i2c device drivers may be active on return from add_adapter() */
> +	r = i2c_add_adapter(adap);

Why not use devm_ variant of this?

> +	if (r)
> +		return dev_err_probe(dev, r, "Failure adding adapter\n");

...

> +static int ls2x_i2c_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);

Can't you use dev_get_drvdata() directly? Why?

> +
> +	priv->suspended = 1;
> +
> +	return 0;
> +}
> +
> +static int ls2x_i2c_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);

Ditto.

> +	priv->suspended = 0;
> +	ls2x_i2c_reginit(priv);
> +
> +	return 0;
> +}

...

> +static const struct dev_pm_ops ls2x_i2c_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend, ls2x_i2c_resume)
> +};

Use corresponding DEFINE_ macro.

-- 
With Best Regards,
Andy Shevchenko