Messages in this thread | | | Date | Mon, 28 Nov 2022 21:13:02 +0800 | Subject | Re: [PATCH v1 1/2] i2c: loongson: add bus driver for the loongson i2c controller | From | 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.
>
| |