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

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

On Tue, Dec 06, 2022 at 11:16:56AM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.

Much better, thanks!

...

> +/*
> + * The I2C controller has a fixed I2C bus frequency by default, but to
> + * be compatible with more client devices, we can obtain the set I2C
> + * bus frequency from ACPI or FDT.
> + */
> +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
> +{
> +	u16 val = 0x12c; /* Default value of I2C divider latch register */

Besides comment better to be placed on top of the commented line, the value
is better to have its own definition where you place the comment and elaborate
what it means in practice (The clock frequency is changed?  Bus speed is
different?)

> +	struct i2c_timings *t = &priv->i2c_t;
> +	u32 acpi_speed = i2c_acpi_find_bus_speed(priv->dev);
> +
> +	i2c_parse_fw_timings(priv->dev, t, false);
> +
> +	if (acpi_speed || t->bus_freq_hz)
> +		val = 10 * HZ_PER_MHZ / max(t->bus_freq_hz, acpi_speed) - 1;
> +
> +	/* Set LS2X I2C frequency */
> +	writel(val, priv->base + I2C_LS2X_PRER_LO);
> +}

...

> +	writeb(data | LS2X_CTR_EN | LS2X_CTR_IEN | LS2X_CTR_MST,
> +			priv->base + I2C_LS2X_CTR);

Wrong indentation.

...

> +	r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler,
> +			     IRQF_SHARED, "ls2x-i2c", priv);
> +	if (r < 0)
> +		return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);

You requested IRQ without filling all data structures. Is it fine? Have you
checked that with CONFIG_DEBUG_SHIRQ being enabled?

...

> +	r = devm_i2c_add_adapter(dev, adap);
> +	if (r)
> +		return dev_err_probe(dev, r, "Failure adding adapter\n");
> +
> +	return 0;
> +}

Looking at the above...

> +static int ls2x_i2c_remove(struct platform_device *pdev)
> +{
> +	struct ls2x_i2c_priv *priv = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&priv->adapter);

...are you sure this is correct?

> +	return 0;
> +}

...

> +static int ls2x_i2c_suspend(struct device *dev)
> +{
> +	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);

> +	priv->suspended = 1;

No protection needed?

> +	return 0;
> +}
> +
> +static int ls2x_i2c_resume(struct device *dev)
> +{
> +	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);

> +	priv->suspended = 0;

Ditto.

> +	ls2x_i2c_reginit(priv);
> +	return 0;
> +}

...

> +MODULE_ALIAS("platform:ls2x-i2c");

Why is this required?

-- 
With Best Regards,
Andy Shevchenko