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

[Yinbo Zhu]

在 2022/11/17 16:44, Andy Shevchenko 写道:
> 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).

The i2c.h had included of.h/acpi.h/property.h, and remove acpi.h

 head file.

> > +#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?
This is a workaroud.
> > +	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 ...`.

Hi Andy,


Thanks your advice,

I had adopt it in v2 patch.

>