On Fri, Nov 25, 2022 at 04:55:20PM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.
...
Missing bits.h.
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
There is no user of this header.
Why?
> +#include <linux/platform_device.h>
...
> +/* LS2X I2C clock frequency 50M */
> +#define HZ_PER_MHZ (50 * 1000000)
units.h ?
...
> +struct ls2x_i2c_dev {
> + struct device *dev;
> + void __iomem *base;
> + int irq;
> + u32 bus_clk_rate;
> + struct completion cmd_complete;
> + struct i2c_adapter adapter;
Check if moving this to be the first field makes code generation better
(bloat-o-meter is your friend).
> + unsigned int suspended:1;
> +};
> + return ls2x_i2c_send_byte(adap, LS2X_CR_STOP);
> +}
...
> +static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
> +{
> + struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
> + unsigned char addr = i2c_8bit_addr_from_msg(msgs);
> +
> + reinit_completion(&dev->cmd_complete);
> + addr |= (msgs->flags & I2C_M_RD) ? 1 : 0;
Why is this needed?
> + writeb(addr, dev->base + I2C_LS2X_TXR);
> +
> + return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
> +}
...
> + while (len--) {
> + if (len == 0)
> + cmd |= LS2X_CR_ACK;
> +
> + writeb(cmd, dev->base + I2C_LS2X_CR);
Can be written as
writeb(cmd | (len ? 0 : LS2X_CR_ACK), dev->base + I2C_LS2X_CR);
but it's up to you.
> + time_left = wait_for_completion_timeout(&dev->cmd_complete,
> + adap->timeout);
> + if (unlikely(!time_left)) {
> + dev_err(dev->dev, "transaction timeout\n");
> + return -ETIMEDOUT;
> + }
> +
> + *buf++ = readb(dev->base + I2C_LS2X_RXR);
> + }
...
> + for (retry = 0; retry < adap->retries; retry++) {
> +
Unneeded blank line.
> + ret = ls2x_i2c_doxfer(adap, msgs, num);
> + if (ret != -EAGAIN)
> + return ret;
> +
> + dev_dbg(dev->dev, "Retrying transmission (%d)\n", retry);
> + udelay(100);
> + }
Can something from iopoll.h be utilized here?
...
> + if (iflag & LS2X_SR_IF) {
> + writeb(LS2X_CR_IACK, dev->base + I2C_LS2X_CR);
> + complete(&dev->cmd_complete);
> + } else
> + return IRQ_NONE;
Use usual pattern: checking for error condition first.
if (!(iflag & LS2X_SR_IF))
return IRQ_NONE;
writeb(LS2X_CR_IACK, dev->base + I2C_LS2X_CR);
complete(&dev->cmd_complete);
> + return IRQ_HANDLED;
...
> + writeb((val & 0xff00) >> 8, dev->base + I2C_LS2X_PRER_HI);
What ' & 0xff00' part is for?
...
> + dev = devm_kzalloc(&pdev->dev,
> + sizeof(struct ls2x_i2c_dev), GFP_KERNEL);
sizeof(*dev) and make it one line.
> + if (unlikely(!dev))
Why unlikely()?
> + return -ENOMEM;
...
> + dev->irq = platform_get_irq(pdev, 0);
> + if (unlikely(dev->irq <= 0))
Why 'unlikely()'? Why == 0 is here?
> + return -ENODEV;
...
> + r = devm_request_irq(&pdev->dev, dev->irq, ls2x_i2c_isr,
> + IRQF_SHARED, "ls2x-i2c", dev);
> + if (unlikely(r)) {
Why 'unlikely()'? You must explain all likely() / unlikely() use in the code.
> + dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
> + return r;
return dev_err_probe(...);
> + }
...
> + /*
> + * The I2C controller has a fixed I2C bus frequency by default, but to
> + * be compatible with more client devices, we can obtain the set I2C
> + * bus frequency from ACPI or FDT.
> + */
> + dev->bus_clk_rate = i2c_acpi_find_bus_speed(&pdev->dev);
> + if (!dev->bus_clk_rate)
> + device_property_read_u32(&pdev->dev, "clock-frequency",
> + &dev->bus_clk_rate);
This should be done via
i2c_parse_fw_timings(&pdev->dev, ...);
no?
...
> + adap->dev.of_node = pdev->dev.of_node;
> + ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
device_set_node()
...
> + /* i2c device drivers may be active on return from add_adapter() */
> + r = i2c_add_adapter(adap);
> + if (r) {
> + dev_err(dev->dev, "failure adding adapter\n");
> + return r;
return dev_err_probe(...);
> + }
...
> +static int __maybe_unused ls2x_i2c_suspend_noirq(struct device *dev)
No __maybe_unused, use proper PM macros and definitions.
(look for pm_ptr() / pm_sleep_ptr() and corresponding defines)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ls2x_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> + i2c_dev->suspended = 1;
> +
> + return 0;
> +}
> +
> +static int __maybe_unused ls2x_i2c_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ls2x_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> + i2c_dev->suspended = 0;
> + ls2x_i2c_reginit(i2c_dev);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ls2x_i2c_dev_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend_noirq, ls2x_i2c_resume)
> +};
As per above.
...
> +static const struct of_device_id ls2x_i2c_id_table[] = {
> + { .compatible = "loongson,ls2k-i2c" },
> + { .compatible = "loongson,ls7a-i2c" },
> + { /* sentinel */ },
No comma for terminator entry.
> +};
...
> + { "LOON0004", 0 },
', 0' is redundant.
...
> +static struct platform_driver ls2x_i2c_driver = {
> + .probe = ls2x_i2c_probe,
> + .remove = ls2x_i2c_remove,
> + .driver = {
> + .name = "ls2x-i2c",
> + .owner = THIS_MODULE,
Why?
> + .pm = &ls2x_i2c_dev_pm_ops,
> + .of_match_table = ls2x_i2c_id_table,
> + .acpi_match_table = ls2x_i2c_acpi_match,
> + },
> +};
--
With Best Regards,
Andy Shevchenko
