Hi Krzysztof,
On 2023/6/6 16:34, Krzysztof Kozlowski wrote:
On 02/06/2023 07:01, Hans Hu wrote:
Add Zhaoxin I2C controller driver. It provides the access to the i2c
busses, which connects to the touchpad, eeprom, etc.
Zhaoxin I2C controller has two separate busses, so may accommodate up
to two I2C adapters. Those adapters are listed in the ACPI namespace
with the "IIC1D17" HID, and probed by a platform driver.
The driver works with IRQ mode, and supports basic I2C features. Flags
I2C_AQ_NO_ZERO_LEN and I2C_AQ_COMB_WRITE_THEN_READ are used to limit
the unsupported access.
Change since v2:
* fixed some code style issues
* check if "timeout == 0" first, then check if event $condition is true
* add warnning message for "timeout == 1"
Link: https://lore.kernel.org/all/20230531110058.n7ubjp2kzlx7tuoc@intel.intel/
Change since v1:
* remove some useless include files and sort the rest.
* use mmio bar distinguish host index.
* use pci-dev's name and i2c-dev's name rename adapter's name.
* remove some debug code, fix some code style issue.
Link: https://lore.kernel.org/all/20230504060043.13155-1-hanshu-oc@xxxxxxxxxxx/
Signed-off-by: Hans Hu <hanshu-oc@xxxxxxxxxxx>
---
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-zhaoxin.c | 645 +++++++++++++++++++++++++++++++
3 files changed, 656 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-zhaoxin.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 87600b4aacb3..4c2d302184aa 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -333,6 +333,16 @@ config I2C_VIAPRO
This driver can also be built as a module. If so, the module
will be called i2c-viapro.
+config I2C_ZHAOXIN
+ tristate "ZHAOXIN I2C Interface"
+ depends on PCI && ACPI
Why this cannot be compile tested?
will add COMPILE_TEST in patch v4.
+ help
+ If you say yes to this option, support will be included for the
+ ZHAOXIN I2C interface
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-zhaoxin.
+
if ACPI
comment "ACPI drivers"
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index af56fe2c75c0..cc470ce470ca 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_I2C_SIS630) += i2c-sis630.o
obj-$(CONFIG_I2C_SIS96X) += i2c-sis96x.o
obj-$(CONFIG_I2C_VIA) += i2c-via.o
obj-$(CONFIG_I2C_VIAPRO) += i2c-viapro.o
+obj-$(CONFIG_I2C_ZHAOXIN) += i2c-zhaoxin.o
# Mac SMBus host controller drivers
obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
diff --git a/drivers/i2c/busses/i2c-zhaoxin.c b/drivers/i2c/busses/i2c-zhaoxin.c
new file mode 100644
index 000000000000..61807da9c170
--- /dev/null
+++ b/drivers/i2c/busses/i2c-zhaoxin.c
@@ -0,0 +1,645 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * i2c-zhaoxin-i2c.c - Zhaoxin I2C controller driver
+ *
+ * Copyright(c) 2021 Shanghai Zhaoxin Semiconductor Corporation.
+ * All rights reserved.
+ *
+ * CONTACTS:
+ *
+ * Hans Hu hanshu@xxxxxxxxxxx
+ * Cobe Chen cobechen@xxxxxxxxxxx
+ */
+
+#define DRIVER_VERSION "1.4.0"
+
Drop. Linux already has a version.
will drop.
+
+static int zxi2c_parse_resources(struct zxi2c *i2c)
+{
+ struct platform_device *pdev = to_platform_device(i2c->dev);
+ struct resource *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ i2c->regs = devm_ioremap_resource(&pdev->dev, res);
There is a helper for these two.
I want to use the lower 8 bits of the physical address to
distinguish the controller's index, to set reset_bitmask.
+ if (IS_ERR(i2c->regs))
+ return dev_err_probe(&pdev->dev, PTR_ERR(i2c->regs),
+ "get base addr failed\n");
+
+ if (res->start & 0x20)
+ i2c->reset_bitmask = BIT(4);
+ else
+ i2c->reset_bitmask = BIT(5);
+
+ i2c->irq = platform_get_irq(pdev, 0);
+ if (i2c->irq < 0)
+ return dev_err_probe(&pdev->dev, i2c->irq,
+ "get irq failed\n");
+
+ zxi2c_get_bus_speed(i2c);
+
+ i2c->hrv = get_reversion(i2c->regs);
+
+ i2c->dynamic = get_dynamic_clock(i2c->regs);
+ set_dynamic_clock(i2c->regs, i2c->dynamic);
+
+ i2c->fstp = get_fstp_value(i2c->regs);
+ zxi2c_set_bus_speed(i2c);
+
+ return 0;
+}
+
+static int zxi2c_probe(struct platform_device *pdev)
+{
+ int error;
+ struct zxi2c *i2c;
+ struct pci_dev *pci;
+ struct device *dev;
+
+ /* make sure this is zhaoxin platform */
Why? How can you match to different platform?
In case a platform device named "IIC1D17" is also reported on
non-zhaoxin platform, this driver may also be registered successfully,
even if it shouldn't. In theory.
+ dev = pdev->dev.parent;
+ if (dev && dev_is_pci(dev)) {
+ pci = to_pci_dev(dev);
+ if (pci->vendor != 0x1d17 || pci->device != 0x1001)
+ return -ENODEV;
+ } else {
+ return -ENODEV;
+ }
+
+ i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+ if (IS_ERR(i2c)) {
+ return dev_err_probe(&pdev->dev, -ENOMEM,
+ "devm_kzalloc failed\n");
Don't print ENOMEM.
will change to just return -ENOMEM;
Run:
1. Checkpatch
2. Coccinelle
to fix trivial issues.
I used the checkpatch in different versions of the kernel, and there were
no errors, just a warning about the modify MAINTAINERS, will modify it.
+ }
+
+ i2c->pci = pci;
+ i2c->dev = &pdev->dev;
+ error = zxi2c_parse_resources(i2c);
+ if (error)
+ return error;
+
+ platform_set_drvdata(pdev, (void *)i2c);
+
+ error = devm_request_irq(&pdev->dev, i2c->irq, zxi2c_irq_handle,
+ IRQF_SHARED, pdev->name, i2c);
+ if (error < 0)
+ return dev_err_probe(i2c->dev, error,
+ "IRQ%d request failed\n",
+ i2c->irq);
+
+ init_waitqueue_head(&i2c->waitq);
+
+ i2c->adap.owner = THIS_MODULE;
+ i2c->adap.algo = &zxi2c_algorithm;
+ i2c->adap.retries = 2;
+ i2c->adap.quirks = &zxi2c_quirks;
+ i2c->adap.dev.parent = &pdev->dev;
+ ACPI_COMPANION_SET(&i2c->adap.dev, ACPI_COMPANION(&pdev->dev));
+ snprintf(i2c->adap.name, sizeof(i2c->adap.name),
+ "%s-%s", dev_name(&pci->dev), dev_name(i2c->dev));
+ i2c_set_adapdata(&i2c->adap, i2c);
+
+ error = i2c_add_adapter(&i2c->adap);
+ if (unlikely(error))
+ return dev_err_probe(i2c->dev, error,
+ "adapter registration failed\n");
+
+ dev_info(i2c->dev, "adapter /dev/i2c-%d registered. hw version %x\n",
+ i2c->adap.nr, i2c->hrv);
+
+ return 0;
+}
+
+static int zxi2c_remove(struct platform_device *pdev)
+{
+ struct zxi2c *i2c = platform_get_drvdata(pdev);
+
+ i2c_lock_bus(&i2c->adap, I2C_LOCK_ROOT_ADAPTER);
+
+ zxi2c_module_reset(i2c);
+ master_regs_reset(i2c->regs);
+
+ devm_free_irq(&pdev->dev, i2c->irq, i2c);
+
+ i2c_unlock_bus(&i2c->adap, I2C_LOCK_ROOT_ADAPTER);
+
+ i2c_del_adapter(&i2c->adap);
+
+ platform_set_drvdata(pdev, NULL);
+ devm_kfree(&pdev->dev, i2c);
Why do you do it? Its devm.
will drop it, and replace i2c_add_adapter with devm_i2c_add_adapter.
+
+ dev_info(&pdev->dev, "adapter unregistered\n");
Drop simple probe or function success statuses. Kernel has other ways to
get this information.
will drop it.
...
+ .acpi_match_table = ACPI_PTR(zxi2c_acpi_match),
Drop ACPI_PTR. This will cause warnings.
will drop it.
Thank you,
Hans
Best regards,
Krzysztof
