Message-ID: <f401b35d-f775-9a06-bec6-983d33ddb249@loongson.cn>
Date:   Mon, 28 Nov 2022 21:13:02 +0800
From:   Yinbo Zhu <zhuyinbo@...ngson.cn>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Wolfram Sang <wsa@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Jean Delvare <jdelvare@...e.de>,
        William Zhang <william.zhang@...adcom.com>,
        Conor Dooley <conor.dooley@...rochip.com>,
        Jan Dabros <jsd@...ihalf.com>,
        Tharun Kumar P <tharunkumar.pasumarthi@...rochip.com>,
        Phil Edworthy <phil.edworthy@...esas.com>,
        Sam Protsenko <semen.protsenko@...aro.org>,
        Tyrone Ting <kfting@...oton.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] i2c: loongson: add bus driver for the loongson i2c
 controller


在 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.

>

Powered by blists - more mailing lists