Hi Andy:
Firstly, thanks for your careful review.
在 2022/11/25 18:41, Andy Shevchenko 写道:
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.
Is it needed? I found it already included in I2c.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 ?
I misunderstood your previous comment, and the HZ_PER_MHZ in units.h
will be used.
...
+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).
vmlinux.old: original order
vmlinux: adapter to be the first field
[zhoubinbin@kernelserver github]$ scripts/bloat-o-meter vmlinux.old vmlinux
add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-8 (-8)
Function old new delta
ls2x_i2c_remove 36 32 -4
ls2x_i2c_probe 424 420 -4
Total: Before=19302026, After=19302018, chg -0.00%
+ 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?
In the ls2x I2C controller, the bit 0 of TXR indicates the read/write
status when transferring the address.
+ 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?
I think udelay() should be suitable because it is just the time interval
between two retry.
...
+ 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?
Emm... I'll use writel(val, priv->base + I2C_LS2X_PRER_LO); instead.
...
+ 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.
These 'unlikely()' may not be quite right, at that time I just thought
these anomalies were infrequent.
+ 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?
Yes, I get it，and the i2c_ls2x_adjust_bus_speed() function will be
introduced to calculate i2c bus_freq_hz.
...
+ 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.
The pm_sleep_ptr(&ls2x_i2c_dev_pm_ops) will be used in ls2x_i2c_driver
and drop __maybe_unused.
Binbin
...
+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,
+ },
+};
