Re: [PATCH V3 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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,
+	},
+};




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux