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
