Re: [PATCH v1 1/2] i2c: loongson: add bus driver for the loongson i2c controller

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

On Thu, Nov 17, 2022 at 03:59:37PM +0800, Yinbo Zhu wrote:
> This bus driver supports the Loongson i2c hardware controller in the
> Loongson platforms and supports to use DTS and ACPI framework to
> register i2c adapter device resources.
> 
> The Loongson i2c controller supports operating frequencty is 50MHZ
> and supports the maximum transmission rate is 400kbps.

...

> +#include <linux/acpi.h>

Besides missed of.h (but do not add it) this one has no users (see below how).
What you _might_ need is to have property.h to be included (seems no need).

> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>

Can you keep it ordered?

...

> +#define CTR_EN				0x80
> +#define CTR_IEN				0x40

BIT() ?
(don't forget to add bits.h for that)

...

> +#define CR_START			0x81
> +#define CR_STOP				0x41
> +#define CR_READ				0x21
> +#define CR_WRITE			0x11

Sounds to me like a BIT() + one specific bit to be set which should be defined
separately.

> +#define CR_ACK				0x8
> +#define CR_IACK				0x1
> +
> +#define SR_NOACK			0x80
> +#define SR_BUSY				0x40
> +#define SR_AL				0x20
> +#define SR_SLAVE_ADDRESSED		0x10
> +#define SR_SLAVE_RW			0x8
> +#define SR_TIP				0x2
> +#define SR_IF				0x1

All above seems like candidates for BIT()

...

> +#define i2c_readb(addr)			readb(dev->base + addr)
> +#define i2c_writeb(val, addr)		writeb(val, dev->base + addr)

These are rather bad macros than useful.
Instead, you have to pass a dev parameter and even better to have them
as static inline helpers.

Also you may consider using regmap MMIO APIs.

...

> +static bool repeated_start = 1;
> +module_param(repeated_start, bool, 0600);
> +MODULE_PARM_DESC(repeated_start,
> +		"Compatible with devices that support repeated start");

We discourage people to have new module parameters in the new code. Why this
can't be altered at run-time?

...

> +struct loongson_i2c_dev {
> +	spinlock_t		lock;

Haven't checkpatch complained that lock is not documented?

> +	unsigned int		suspended:1;
> +	struct device		*dev;
> +	void __iomem		*base;
> +	int			irq;
> +	u32			speed_hz;
> +	struct completion	cmd_complete;
> +	struct resource		*ioarea;

> +	struct i2c_adapter	adapter;

Also consider to check what is better to have as the first field in this data
structure. Depending on performance and code size you may choose which one can
go with no-op pointer arithmetics.

From code size perspective you can check with bloat-o-meter.

> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	struct i2c_client	*slave;
> +	enum loongson_i2c_slave_state	slave_state;
> +#endif
> +};

...

> +static int loongson_i2c_start(
> +		struct loongson_i2c_dev *dev, int dev_addr, int flags)

Broken indentation.

> +{
> +	unsigned long time_left;
> +	int retry = 5;

> +	unsigned char addr = (dev_addr & 0x7f) << 1;

Don't we have specific macro or helper for this?

> +	addr |= (flags & I2C_M_RD) ? 1 : 0;
> +
> +start:

> +	mdelay(1);

Why?

> +	i2c_writeb(addr, LOONGSON_I2C_TXR_REG);
> +	i2c_writeb((CR_START | CR_WRITE), LOONGSON_I2C_CR_REG);

> +	time_left = wait_for_completion_timeout(
> +		&dev->cmd_complete,
> +		(&dev->adapter)->timeout);

Broken indentation, too many parentheses (use . when it's appropriate).
Check your entire code for these.

> +	if (!time_left)
> +		return -ETIMEDOUT;
> +
> +	if (i2c_readb(LOONGSON_I2C_SR_REG) & SR_NOACK) {
> +		if (loongson_i2c_stop(dev) < 0)
> +			return -1;
> +
> +		while (retry--)

> +			goto start;

These labels here and there make code hard to understand. Try to refactor them,
so they will be easier to follow.

> +		return 0;
> +	}

> +	return 1;

What does this mean? Don't you need a specific definition, since it's not an
error code?

> +}

...

> +	i2c_writeb(i2c_readb(LOONGSON_I2C_CR_REG) |
> +					0x01, LOONGSON_I2C_CR_REG);
> +	i2c_writeb(i2c_readb(LOONGSON_I2C_CTR_REG) & ~0x80,
> +					LOONGSON_I2C_CTR_REG);
> +	i2c_writeb(prer_val & 0xFF, LOONGSON_I2C_PRER_LO_REG);
> +	i2c_writeb((prer_val & 0xFF00) >> 8, LOONGSON_I2C_PRER_HI_REG);
> +	i2c_writeb(i2c_readb(LOONGSON_I2C_CTR_REG) |
> +					0xe0, LOONGSON_I2C_CTR_REG);

Try to avoid magic numbers. Utilize GENMASK() when it's appropriate
(here it seems you have redundant '& 0xff{00}' stuff).

...

It's already a lot of remarks and comments. This patch needs more work.
I stopped here, only a couple additional remarks below as promised above.

...

> +	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));

device_set_node()

...

> +err_iounmap:
> +	iounmap(dev->base);
> +err_request_irq:
> +err_free_mem:
> +	platform_set_drvdata(pdev, NULL);
> +	kfree(dev);
> +err_release_region:
> +	release_mem_region(mem->start, resource_size(mem));
> +
> +	return r;

Can you utilize devm_*() / pcim_*() APIs? Why not?

...

> +		.of_match_table = of_match_ptr(loongson_i2c_id_table),
> +		.acpi_match_table = ACPI_PTR(loongson_i2c_acpi_match),

No of_match_ptr(), no ACPI_PTR(). You probably haven't compiled your code in
different configuration with `make W=1 ...`.

-- 
With Best Regards,
Andy Shevchenko