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
