I2C support for CE4100, v3

The Sodaville I2C controller is almost the same as found on PXA2xx. The difference:
- the register are at a different spot

- no slave support

The PCI probe code adds three platform devices which are probed then by the platform code.
The X86 part also adds dummy clock defines because we don't have HW clock support.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

Signed-off-by Dirk Brandewie <[email protected]>

---
 drivers/i2c/busses/Kconfig       |    7 +-
 drivers/i2c/busses/Makefile      |    1 +
 drivers/i2c/busses/i2c-pxa-pci.c |  173 ++++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-pxa.c     |   27 +++++-
 4 files changed, 203 insertions(+), 5 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-pxa-pci.c

drivers/i2c/busses/Kconfig +5 -2

index 3a6321c..9ee3e60 100644
@@ -525,15 +525,18 @@ config I2C_PNX
 
 config I2C_PXA
 	tristate "Intel PXA2XX I2C adapter"
-	depends on ARCH_PXA || ARCH_MMP
+	depends on ARCH_PXA || ARCH_MMP || (X86_32 && PCI && OF)
 	help
 	  If you have devices in the PXA I2C bus, say yes to this option.
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-pxa.
 
+config I2C_PXA_PCI
+	def_bool I2C_PXA && X86_32 && PCI && OF
+
 config I2C_PXA_SLAVE
 	bool "Intel PXA2XX I2C Slave comms support"
-	depends on I2C_PXA
+	depends on I2C_PXA && !X86_32
 	help
 	  Support I2C slave mode communications on the PXA I2C bus.  This
 	  is necessary for systems where the PXA may be a target on the

drivers/i2c/busses/Makefile +1 -0

index 84cb16a..78db2e3 100644
@@ -52,6 +52,7 @@ obj-$(CONFIG_I2C_PCA_PLATFORM)	+= i2c-pca-platform.o
 obj-$(CONFIG_I2C_PMCMSP)	+= i2c-pmcmsp.o
 obj-$(CONFIG_I2C_PNX)		+= i2c-pnx.o
 obj-$(CONFIG_I2C_PXA)		+= i2c-pxa.o
+obj-$(CONFIG_I2C_PXA_PCI)	+= i2c-pxa-pci.o
 obj-$(CONFIG_I2C_S3C2410)	+= i2c-s3c2410.o
 obj-$(CONFIG_I2C_S6000)		+= i2c-s6000.o
 obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o

drivers/i2c/busses/i2c-pxa-pci.c new file +19 -0

new file mode 100644
index 0000000..f8709d3
@@ -0,0 +1,173 @@
+/*
+ * The CE4100's I2C device is more or less the same one as found on PXA.
+ * It does not support slave mode, the register slightly moved. This PCI
+ * device provides three bars, every contains a single I2C controller.
+ */
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/i2c/pxa-i2c.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+
+#define CE4100_PCI_I2C_DEVS	3
+
+struct ce4100_i2c_device {
+	struct platform_device pdev;
+	struct resource res[2];
+	struct i2c_pxa_platform_data pdata;
+};

drivers/i2c/busses/i2c-pxa-pci.c new file +11 -0

+	of_device_node_put(&sd->pdev.dev);
+}
+static int add_i2c_device(struct pci_dev *dev, int bar,
+		struct ce4100_i2c_device *sd)
+{
+	struct platform_device *pdev = &sd->pdev;
+	struct i2c_pxa_platform_data *pdata = &sd->pdata;
+	struct device_node *child;
+	int found = 0;
+	static int devnum;
+

drivers/i2c/busses/i2c-pxa-pci.c new file +25 -0

+	sd->res[1].flags = IORESOURCE_IRQ;
+	sd->res[1].start = dev->irq;
+	sd->res[1].end = dev->irq;
+	pdev->num_resources = 2;
+
+	for_each_child_of_node(dev->dev.of_node, child) {
+		const void *prop;
+		struct resource r;
+		int ret;
+
+		ret = of_address_to_resource(child, 0, &r);
+		if (ret < 0)
+			continue;
+		if (r.start != sd->res[0].start)
+			continue;
+		if (r.end != sd->res[0].end)
+			continue;
+		if (r.flags != sd->res[0].flags)
+			continue;
+
+		pdev->dev.of_node = child;
+		prop = of_get_property(child, "fast-mode", NULL);
+		if (prop)
+			pdata->fast_mode = 1;
+

drivers/i2c/busses/i2c-pxa-pci.c new file +75 -0

+	return -EINVAL;
+}
+
+static int __devinit ce4100_i2c_probe(struct pci_dev *dev,
+		const struct pci_device_id *ent)
+{
+	int ret;
+	int i;
+	struct ce4100_devices *sds;
+
+	ret = pci_enable_device_mem(dev);
+	if (ret)
+		return ret;
+
+	if (!dev->dev.of_node) {
+		dev_err(&dev->dev, "Missing device tree node.\n");
+		return -EINVAL;
+	}
+	sds = kzalloc(sizeof(*sds), GFP_KERNEL);
+	if (!sds)
+		goto err_mem;
+
+	pci_set_drvdata(dev, sds);
+
+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++) {
+		ret = add_i2c_device(dev, i, &sds->sd[i]);
+		if (ret) {
+			while (--i >= 0)
+				platform_device_unregister(&sds->sd[i].pdev);
+			goto err_dev_add;
+		}
+	}
+	return 0;
+
+err_dev_add:
+	pci_set_drvdata(dev, NULL);
+	kfree(sds);
+err_mem:
+	pci_disable_device(dev);
+	return ret;
+}
+
+static void __devexit ce4100_i2c_remove(struct pci_dev *dev)
+{
+	struct ce4100_devices *sds;
+	unsigned int i;
+
+	sds = pci_get_drvdata(dev);
+	pci_set_drvdata(dev, NULL);
+
+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++)
+		platform_device_unregister(&sds->sd[i].pdev);
+
+	pci_disable_device(dev);
+	kfree(sds);
+}
+
+static struct pci_device_id ce4100_i2c_devices[] __devinitdata = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x2e68)},
+	{ },
+};
+MODULE_DEVICE_TABLE(pci, ce4100_i2c_devices);
+
+static struct pci_driver ce4100_i2c_driver = {
+	.name           = "ce4100_i2c",
+	.id_table       = ce4100_i2c_devices,
+	.probe          = ce4100_i2c_probe,
+	.remove         = __devexit_p(ce4100_i2c_remove),
+};
+
+static int __init ce4100_i2c_init(void)
+{
+	return pci_register_driver(&ce4100_i2c_driver);
+}
+module_init(ce4100_i2c_init);

drivers/i2c/busses/i2c-pxa.c +18 -1

index fc2a90e..225e9a5 100644
@@ -38,6 +38,13 @@
 
 #include <asm/irq.h>
 
+#ifdef CONFIG_X86
+#define clk_get(dev, id)	NULL
+#define clk_put(clk)		do { } while (0)
+#define clk_disable(clk)	do { } while (0)
+#define clk_enable(clk)		do { } while (0)
+#endif
+
 struct pxa_reg_layout {
 	u32 ibmr;
 	u32 idbr;

@@ -49,6 +56,7 @@ struct pxa_reg_layout {
 enum pxa_i2c_types {
 	REGS_PXA2XX,
 	REGS_PXA3XX,
+	REGS_CE4100,
 };
 
 /*

@@ -69,11 +77,19 @@ static struct pxa_reg_layout pxa_reg_layout[] = {
 		.isr =	0x18,
 		.isar =	0x20,
 	},
+	[REGS_CE4100] = {
+		.ibmr =	0x14,
+		.idbr =	0x0c,
+		.icr =	0x00,
+		.isr =	0x04,
+		/* no isar register */
+	},
 };
 
 static const struct platform_device_id i2c_pxa_id_table[] = {
 	{ "pxa2xx-i2c",		REGS_PXA2XX },
 	{ "pxa3xx-pwri2c",	REGS_PXA3XX },
+	{ "ce4100-i2c",		REGS_CE4100 },
 	{ },
 };
 MODULE_DEVICE_TABLE(platform, i2c_pxa_id_table);

@@ -442,7 +458,8 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
 	writel(I2C_ISR_INIT, _ISR(i2c));
 	writel(readl(_ICR(i2c)) & ~ICR_UR, _ICR(i2c));
 
-	writel(i2c->slave_addr, _ISAR(i2c));
+	if (i2c->reg_isar)
+		writel(i2c->slave_addr, _ISAR(i2c));
 
 	/* set control register values */
 	writel(I2C_ICR_INIT | (i2c->fast_mode ? ICR_FM : 0), _ICR(i2c));

drivers/i2c/busses/i2c-pxa.c +4 -1

 	i2c->iobase = res->start;
 	i2c->iosize = resource_size(res);

@@ -1113,7 +1131,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	i2c->adap.algo_data = i2c;
 	i2c->adap.dev.parent = &dev->dev;
 
-	ret = i2c_add_numbered_adapter(&i2c->adap);
+	if (i2c_type == REGS_CE4100)
+		ret = i2c_add_adapter(&i2c->adap);
+	else
+		ret = i2c_add_numbered_adapter(&i2c->adap);
 	if (ret < 0) {
 		printk(KERN_INFO "I2C: Failed to add bus\n");
 		goto eadapt;

Signature · 1 line

1.7.3.2

Quoted 3 lines · Sebastian Andrzej Siewior

The Sodaville I2C controller is almost the same as found on PXA2xx. The difference:
- the register are at a different spot

maybe 'offset' is a better word than 'spot' here.

Quoted 30 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +23 -0

new file mode 100644
index 0000000..f8709d3
@@ -0,0 +1,173 @@
+/*
+ * The CE4100's I2C device is more or less the same one as found on PXA.
+ * It does not support slave mode, the register slightly moved. This PCI
+ * device provides three bars, every contains a single I2C controller.
+ */
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/i2c/pxa-i2c.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+
+#define CE4100_PCI_I2C_DEVS	3
+
+struct ce4100_i2c_device {
+	struct platform_device pdev;
+	struct resource res[2];
+	struct i2c_pxa_platform_data pdata;
+};
+
+struct ce4100_devices {
+	struct ce4100_i2c_device sd[CE4100_PCI_I2C_DEVS];
+};

drivers/i2c/busses/i2c-pxa-pci.c line 29

Quoted 6 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+
+static void plat_dev_release(struct device *dev)
+{
+	struct ce4100_i2c_device *sd = container_of(dev,
+			struct ce4100_i2c_device, pdev.dev);
+

maybe add a little to_ce_dev() and use it?

Quoted 15 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +15 -0

+	of_device_node_put(&sd->pdev.dev);
+}
+static int add_i2c_device(struct pci_dev *dev, int bar,
+		struct ce4100_i2c_device *sd)
+{
+	struct platform_device *pdev = &sd->pdev;
+	struct i2c_pxa_platform_data *pdata = &sd->pdata;
+	struct device_node *child;
+	int found = 0;
+	static int devnum;
+
+	pdev->name = "ce4100-i2c";
+	pdev->dev.release = plat_dev_release;
+	pdev->dev.parent = &dev->dev;
+

drivers/i2c/busses/i2c-pxa-pci.c line 50

Quoted 6 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+	pdev->dev.platform_data = pdata;
+	pdev->resource = sd->res;
+
+	sd->res[0].flags = IORESOURCE_MEM;
+	sd->res[0].start = pci_resource_start(dev, bar);
+	sd->res[0].end = pci_resource_end(dev, bar);

hmm, could you copy the original resource to this?

Quoted 30 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +30 -0

+	sd->res[1].flags = IORESOURCE_IRQ;
+	sd->res[1].start = dev->irq;
+	sd->res[1].end = dev->irq;
+	pdev->num_resources = 2;
+
+	for_each_child_of_node(dev->dev.of_node, child) {
+		const void *prop;
+		struct resource r;
+		int ret;
+
+		ret = of_address_to_resource(child, 0, &r);
+		if (ret < 0)
+			continue;
+		if (r.start != sd->res[0].start)
+			continue;
+		if (r.end != sd->res[0].end)
+			continue;
+		if (r.flags != sd->res[0].flags)
+			continue;
+
+		pdev->dev.of_node = child;
+		prop = of_get_property(child, "fast-mode", NULL);
+		if (prop)
+			pdata->fast_mode = 1;
+
+		pdev->id = devnum++;
+		found = 1;
+		break;
+	}
+

drivers/i2c/busses/i2c-pxa-pci.c line 86

Quoted 5 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +5 -0

+	if (found)
+		return platform_device_register(pdev);
+
+	dev_err(&dev->dev, "Missing a DT node at %s for controller bar %d.\n",
+			dev->dev.of_node->full_name, bar);

Hmm, do you really need to print dev->dev.of_node->full_name here, or is it missing from the dev_err() print?

drivers/i2c/busses/i2c-pxa-pci.c line 88

Quoted 2 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +2 -0

+	dev_err(&dev->dev, "Its memory space is 0x%08x - 0x%08x.\n",
+			sd->res[0].start, sd->res[0].end);

No need for Its in this message. Also, why not print IRQ number?

Quoted 35 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +2 -0

+	return -EINVAL;
+}

drivers/i2c/busses/i2c-pxa.c +10 -1

index fc2a90e..225e9a5 100644
@@ -69,11 +77,19 @@ static struct pxa_reg_layout pxa_reg_layout[] = {
 		.isr =	0x18,
 		.isar =	0x20,
 	},
+	[REGS_CE4100] = {
+		.ibmr =	0x14,
+		.idbr =	0x0c,
+		.icr =	0x00,
+		.isr =	0x04,
+		/* no isar register */
+	},
 };
 
 static const struct platform_device_id i2c_pxa_id_table[] = {
 	{ "pxa2xx-i2c",		REGS_PXA2XX },
 	{ "pxa3xx-pwri2c",	REGS_PXA3XX },
+	{ "ce4100-i2c",		REGS_CE4100 },
 	{ },
 };
 MODULE_DEVICE_TABLE(platform, i2c_pxa_id_table);

@@ -442,7 +458,8 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
 	writel(I2C_ISR_INIT, _ISR(i2c));
 	writel(readl(_ICR(i2c)) & ~ICR_UR, _ICR(i2c));
 
-	writel(i2c->slave_addr, _ISAR(i2c));
+	if (i2c->reg_isar)
+		writel(i2c->slave_addr, _ISAR(i2c));
 

drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1095

Quoted 9 lines

drivers/i2c/busses/i2c-pxa.c +2 -1

 	/* set control register values */
 	writel(I2C_ICR_INIT | (i2c->fast_mode ? ICR_FM : 0), _ICR(i2c));

@@ -1074,7 +1091,8 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
 	i2c->reg_icr = i2c->reg_base + pxa_reg_layout[i2c_type].icr;
 	i2c->reg_isr = i2c->reg_base + pxa_reg_layout[i2c_type].isr;
-	i2c->reg_isar = i2c->reg_base + pxa_reg_layout[i2c_type].isar;
+	if (i2c_type != REGS_CE4100)
+		i2c->reg_isar = i2c->reg_base + pxa_reg_layout[i2c_type].isar;

do you really need to bother to checking i2c_type here?

* Ben Dooks | 2011-01-05 20:21:36 [+0000]:

[cut]

I fixed up all your comments (unless I forgot something by accident). Is it possible for this to go in this merge window or do I have to wait for the next one?

Quoted 1 line · * Sebastian Andrzej Siewior | 2011-01-14 15:31:07 [+0100]:

* Ben Dooks | 2011-01-05 20:21:36 [+0000]:

Hi Ben,

Quoted 2 lines

[cut]

Quoted 3 lines

I fixed up all your comments (unless I forgot something by accident). Is it possible for this to go in this merge window or do I have to wait for the next one?

I've rebased the whole series on top of v2.6.38-rc3. I fixed the conflict and considered three new boards (colibri-evalboard, saarb and pxa95).

The series is available at
git://git.linutronix.de/users/bigeasy/soda.git i2c_v3

Sebastian Andrzej Siewior (6):
i2c-pxa2xx: use dynamic register layout
ARM: pxa2xx: reorganize I2C files
i2c-pxa2xx: Add PCI support for PXA I2C controller
i2c-pxa2xx: add support for shared IRQ handler
i2c-pxa2xx: check timeout correctly
i2c-pxa2xx: pass of_node from platform driver to adapter and publish

 arch/arm/mach-mmp/include/mach/mmp2.h              |    2 +-
 arch/arm/mach-mmp/include/mach/pxa168.h            |    2 +-
 arch/arm/mach-mmp/include/mach/pxa910.h            |    2 +-
 arch/arm/mach-pxa/balloon3.c                       |    3 +-
 arch/arm/mach-pxa/cm-x300.c                        |    2 +-
 arch/arm/mach-pxa/colibri-evalboard.c              |    3 +-
 arch/arm/mach-pxa/colibri-pxa270-income.c          |    3 +-
 arch/arm/mach-pxa/corgi.c                          |    2 +-
 arch/arm/mach-pxa/csb726.c                         |    2 +-
 arch/arm/mach-pxa/devices.c                        |    2 +-
 arch/arm/mach-pxa/em-x270.c                        |    2 +-
 arch/arm/mach-pxa/ezx.c                            |    2 +-
 arch/arm/mach-pxa/hx4700.c                         |    2 +-
 arch/arm/mach-pxa/littleton.c                      |    2 +-
 arch/arm/mach-pxa/magician.c                       |    2 +-
 arch/arm/mach-pxa/mainstone.c                      |    2 +-
 arch/arm/mach-pxa/mioa701.c                        |    2 +-
 arch/arm/mach-pxa/mxm8x10.c                        |    2 +-
 arch/arm/mach-pxa/palm27x.c                        |    3 +-
 arch/arm/mach-pxa/pcm990-baseboard.c               |    2 +-
 arch/arm/mach-pxa/poodle.c                         |    2 +-
 arch/arm/mach-pxa/pxa27x.c                         |    3 +-
 arch/arm/mach-pxa/pxa3xx.c                         |    2 +-
 arch/arm/mach-pxa/pxa95x.c                         |    2 +-
 arch/arm/mach-pxa/raumfeld.c                       |    2 +-
 arch/arm/mach-pxa/saar.c                           |    2 +-
 arch/arm/mach-pxa/saarb.c                          |    3 +-
 arch/arm/mach-pxa/spitz.c                          |    3 +-
 arch/arm/mach-pxa/stargate2.c                      |    2 +-
 arch/arm/mach-pxa/tavorevb3.c                      |    3 +-
 arch/arm/mach-pxa/tosa.c                           |    2 +-
 arch/arm/mach-pxa/trizeps4.c                       |    2 +-
 arch/arm/mach-pxa/viper.c                          |    2 +-
 arch/arm/mach-pxa/vpac270.c                        |    3 +-
 arch/arm/mach-pxa/xcep.c                           |    3 +-
 arch/arm/mach-pxa/z2.c                             |    3 +-
 arch/arm/mach-pxa/zeus.c                           |    3 +-
 arch/arm/mach-pxa/zylonite_pxa300.c                |    2 +-
 drivers/i2c/busses/Kconfig                         |    7 +-
 drivers/i2c/busses/Makefile                        |    1 +
 drivers/i2c/busses/i2c-pxa-pci.c                   |  176 ++++++++++++++++++++
 drivers/i2c/busses/i2c-pxa.c                       |  116 ++++++++++---
 .../plat/i2c.h => include/linux/i2c/pxa-i2c.h      |    0
 43 files changed, 310 insertions(+), 78 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-pxa-pci.c
 rename arch/arm/plat-pxa/include/plat/i2c.h => include/linux/i2c/pxa-i2c.h (100%)

Sebastian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

drivers/i2c/busses/i2c-pxa-pci.c new file +12 -0

+static void plat_dev_release(struct device *dev)
+{
+	struct ce4100_i2c_device *sd = container_of(dev,
+			struct ce4100_i2c_device, pdev.dev);
+
+	of_device_node_put(&sd->pdev.dev);
+}
+static int add_i2c_device(struct pci_dev *dev, int bar,
+		struct ce4100_i2c_device *sd)
+{
+	struct platform_device *pdev = &sd->pdev;
+	struct i2c_pxa_platform_data *pdata = &sd->pdata;

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+	pdev->name = "ce4100-i2c";
+	pdev->dev.release = plat_dev_release;
+	pdev->dev.parent = &dev->dev;
+
+	pdev->dev.platform_data = pdata;
+	pdev->resource = sd->res;

drivers/i2c/busses/i2c-pxa-pci.c new file +14 -0

+static int __devinit ce4100_i2c_probe(struct pci_dev *dev,
+		const struct pci_device_id *ent)
+{
+	sds = kzalloc(sizeof(*sds), GFP_KERNEL);
+	if (!sds)
+		goto err_mem;
+
+	pci_set_drvdata(dev, sds);
+
+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++) {
+		ret = add_i2c_device(dev, i, &sds->sd[i]);
+		if (ret) {
+			while (--i >= 0)
+				platform_device_unregister(&sds->sd[i].pdev);

+static void __devexit ce4100_i2c_remove(struct pci_dev *dev)

Quoted 12 lines · Sebastian Andrzej Siewior

drivers/i2c/busses/i2c-pxa-pci.c new file +12 -0

+static void plat_dev_release(struct device *dev)
+{
+	struct ce4100_i2c_device *sd = container_of(dev,
+			struct ce4100_i2c_device, pdev.dev);
+
+	of_device_node_put(&sd->pdev.dev);
+}
+static int add_i2c_device(struct pci_dev *dev, int bar,
+		struct ce4100_i2c_device *sd)
+{
+	struct platform_device *pdev = &sd->pdev;
+	struct i2c_pxa_platform_data *pdata = &sd->pdata;

...

Quoted 6 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+	pdev->name = "ce4100-i2c";
+	pdev->dev.release = plat_dev_release;
+	pdev->dev.parent = &dev->dev;
+
+	pdev->dev.platform_data = pdata;
+	pdev->resource = sd->res;

...

Quoted 14 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +14 -0

+static int __devinit ce4100_i2c_probe(struct pci_dev *dev,
+		const struct pci_device_id *ent)
+{
+	sds = kzalloc(sizeof(*sds), GFP_KERNEL);
+	if (!sds)
+		goto err_mem;
+
+	pci_set_drvdata(dev, sds);
+
+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++) {
+		ret = add_i2c_device(dev, i, &sds->sd[i]);
+		if (ret) {
+			while (--i >= 0)
+				platform_device_unregister(&sds->sd[i].pdev);

...

Quoted 1 line

+static void __devexit ce4100_i2c_remove(struct pci_dev *dev)

...

drivers/i2c/busses/i2c-pxa-pci.c line 144

Quoted 6 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++)
+		platform_device_unregister(&sds->sd[i].pdev);
+
+	pci_disable_device(dev);
+	kfree(sds);
+}

I see we're still repeating the same mistakes with lifetime rules of sysfs objects.

Any struct device which has been registered into the device model can remain indefinitely live after it's been unregistered (by, eg, if userspace holds a reference to it via sysfs.)

Only once the release method has been called is it safe to give up the memory backing it - and that also goes for the code comprising the function implementing the release.

This effectively prevents modules having release functions in them - while you can put module use count manipulation in to prevent unloading, it effectively prevents the module from being unloaded until you've unbound the device.

I think you should be trying to use the platform_device_alloc() interfaces here, rather than trying to reinvent them. The only issue I see with that is the of_device_node_put() call. Maybe OF/DT/device model people can provide some pointers? Adding Greg for the device model maintainer view.

Quoted 48 lines · Russell King - ARM Linux

Quoted 12 lines · Sebastian Andrzej Siewior

drivers/i2c/busses/i2c-pxa-pci.c new file +12 -0

+static void plat_dev_release(struct device *dev)
+{
+	struct ce4100_i2c_device *sd = container_of(dev,
+			struct ce4100_i2c_device, pdev.dev);
+
+	of_device_node_put(&sd->pdev.dev);
+}
+static int add_i2c_device(struct pci_dev *dev, int bar,
+		struct ce4100_i2c_device *sd)
+{
+	struct platform_device *pdev = &sd->pdev;
+	struct i2c_pxa_platform_data *pdata = &sd->pdata;

...

Quoted 6 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+	pdev->name = "ce4100-i2c";
+	pdev->dev.release = plat_dev_release;
+	pdev->dev.parent = &dev->dev;
+
+	pdev->dev.platform_data = pdata;
+	pdev->resource = sd->res;

...

Quoted 14 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +14 -0

+static int __devinit ce4100_i2c_probe(struct pci_dev *dev,
+		const struct pci_device_id *ent)
+{
+	sds = kzalloc(sizeof(*sds), GFP_KERNEL);
+	if (!sds)
+		goto err_mem;
+
+	pci_set_drvdata(dev, sds);
+
+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++) {
+		ret = add_i2c_device(dev, i, &sds->sd[i]);
+		if (ret) {
+			while (--i >= 0)
+				platform_device_unregister(&sds->sd[i].pdev);

...

Quoted 1 line

+static void __devexit ce4100_i2c_remove(struct pci_dev *dev)

...

drivers/i2c/busses/i2c-pxa-pci.c line 144

Quoted 6 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++)
+		platform_device_unregister(&sds->sd[i].pdev);
+
+	pci_disable_device(dev);
+	kfree(sds);
+}

I see we're still repeating the same mistakes with lifetime rules of sysfs objects.

Quoted 3 lines

Any struct device which has been registered into the device model can remain indefinitely live after it's been unregistered (by, eg, if userspace holds a reference to it via sysfs.)

Actually this race is almost not possible these days with the rework that Tejun did on sysfs, so it's not easy to test for this anymore.

Quoted 49 lines · Greg KH

Quoted 48 lines · Russell King - ARM Linux

Quoted 12 lines · Sebastian Andrzej Siewior

drivers/i2c/busses/i2c-pxa-pci.c new file +12 -0

+static void plat_dev_release(struct device *dev)
+{
+	struct ce4100_i2c_device *sd = container_of(dev,
+			struct ce4100_i2c_device, pdev.dev);
+
+	of_device_node_put(&sd->pdev.dev);
+}
+static int add_i2c_device(struct pci_dev *dev, int bar,
+		struct ce4100_i2c_device *sd)
+{
+	struct platform_device *pdev = &sd->pdev;
+	struct i2c_pxa_platform_data *pdata = &sd->pdata;

...

Quoted 6 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+	pdev->name = "ce4100-i2c";
+	pdev->dev.release = plat_dev_release;
+	pdev->dev.parent = &dev->dev;
+
+	pdev->dev.platform_data = pdata;
+	pdev->resource = sd->res;

...

Quoted 14 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +14 -0

+static int __devinit ce4100_i2c_probe(struct pci_dev *dev,
+		const struct pci_device_id *ent)
+{
+	sds = kzalloc(sizeof(*sds), GFP_KERNEL);
+	if (!sds)
+		goto err_mem;
+
+	pci_set_drvdata(dev, sds);
+
+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++) {
+		ret = add_i2c_device(dev, i, &sds->sd[i]);
+		if (ret) {
+			while (--i >= 0)
+				platform_device_unregister(&sds->sd[i].pdev);

...

Quoted 1 line

+static void __devexit ce4100_i2c_remove(struct pci_dev *dev)

...

drivers/i2c/busses/i2c-pxa-pci.c line 144

Quoted 6 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++)
+		platform_device_unregister(&sds->sd[i].pdev);
+
+	pci_disable_device(dev);
+	kfree(sds);
+}

I see we're still repeating the same mistakes with lifetime rules of sysfs objects.

Quoted 6 lines

Quoted 3 lines

Any struct device which has been registered into the device model can remain indefinitely live after it's been unregistered (by, eg, if userspace holds a reference to it via sysfs.)

Actually this race is almost not possible these days with the rework that Tejun did on sysfs, so it's not easy to test for this anymore.

Is it wise to make such a problematical bug harder to trigger without completely preventing it triggering?

A different approach was taken with IRQ handling where people were registering handlers before the driver was ready for it to be called - request_irq() would explicitly call the handler as soon as it was registered to provoke bugs.

Surely for these lifetime violations a similar approach should be taken. Make the kernel more likely to oops should a violation occur before the developer can get the code out the door. One way I can think of doing that is when devices are unregistered but not yet released, place them on a list which is periodically scanned, and not only is the device dereferenced by also the release function.

When the use count drops to zero, don't immediately release, but wait a number of polls.

If either goes away before the device has been released, then we predictably oops, and the developer gets to know about his violation of the rules immediately.

Quoted 38 lines · Russell King - ARM Linux

Quoted 12 lines · Sebastian Andrzej Siewior

drivers/i2c/busses/i2c-pxa-pci.c new file +12 -0

+static void plat_dev_release(struct device *dev)
+{
+	struct ce4100_i2c_device *sd = container_of(dev,
+			struct ce4100_i2c_device, pdev.dev);
+
+	of_device_node_put(&sd->pdev.dev);
+}
+static int add_i2c_device(struct pci_dev *dev, int bar,
+		struct ce4100_i2c_device *sd)
+{
+	struct platform_device *pdev = &sd->pdev;
+	struct i2c_pxa_platform_data *pdata = &sd->pdata;

...

Quoted 6 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+	pdev->name = "ce4100-i2c";
+	pdev->dev.release = plat_dev_release;
+	pdev->dev.parent = &dev->dev;
+
+	pdev->dev.platform_data = pdata;
+	pdev->resource = sd->res;

...

Quoted 14 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +14 -0

+static int __devinit ce4100_i2c_probe(struct pci_dev *dev,
+		const struct pci_device_id *ent)
+{
+	sds = kzalloc(sizeof(*sds), GFP_KERNEL);
+	if (!sds)
+		goto err_mem;
+
+	pci_set_drvdata(dev, sds);
+
+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++) {
+		ret = add_i2c_device(dev, i, &sds->sd[i]);
+		if (ret) {
+			while (--i >= 0)
+				platform_device_unregister(&sds->sd[i].pdev);

...

Quoted 1 line

+static void __devexit ce4100_i2c_remove(struct pci_dev *dev)

...

Quoted 13 lines

drivers/i2c/busses/i2c-pxa-pci.c line 144

Quoted 6 lines

drivers/i2c/busses/i2c-pxa-pci.c new file +6 -0

+	for (i = 0; i < ARRAY_SIZE(sds->sd); i++)
+		platform_device_unregister(&sds->sd[i].pdev);
+
+	pci_disable_device(dev);
+	kfree(sds);
+}

I think you should be trying to use the platform_device_alloc() interfaces here, rather than trying to reinvent them. The only issue I see with that is the of_device_node_put() call. Maybe OF/DT/device model people can provide some pointers? Adding Greg for the device model maintainer view.

Don't reinvent functions that the core already provides, that's not a good idea.

Sebastian, why didn't those functions work for you?

thanks,

greg k-h

which functions are you talking about? I allocated one piece of memory which contained a platform device, ressources, platform data. It looked better than allocating multiple small chunks. I registered it with platform_device_register() and platform_device_unregister() complained about a missing ->release function.

Please see my email where I described the lifetime rules. Let's say your module is removed. In doing so, you call platform_device_unregister() followed by kfree(). After this, your module is removed from the system and the memory backing the module code is unmapped.

If something holds a reference to the platform device, then that's just waiting for an OOPS to happen. When those references are finally given up, your release function will be called - but the code has been unmapped.

Quoted 8 lines · * Russell King - ARM Linux | 2011-01-06 11:12:31 [+0000]:

Please see my email where I described the lifetime rules. ?Let's say your module is removed. ?In doing so, you call platform_device_unregister() followed by kfree(). ?After this, your module is removed from the system and the memory backing the module code is unmapped.

If something holds a reference to the platform device, then that's just waiting for an OOPS to happen. ?When those references are finally given up, your release function will be called - but the code has been unmapped.

I got that. That is why I'm going go to use allready provided functions (platform_device_register_resndata() in this case) so I'm no longer allocating any memory. So I'm only holding a struct which is pointing to the platform_device. Everything else is unmapped by the driver core.

So I just have just to of_device_node_put() myself. Hmmm.

Grant, who is calling of_device_node_put() on nodes assigned in of_register_spi_devices() & of_i2c_register_devices()? I can't find any reference. The spi part is not putting the node or removing the irq mapping in error case so I guess there must be some magic triggered somewhere.

Also adap->dev.of_node and master->dev.of_node has to be non-null for the device assignment to work. Who is putting these nodes back? I see drivers (i2c-cpm & i2c-mpc) taking refs but I fail to find a place where it is put back.

the of_node will auto-publish devices which are added to the device tree.

Commit 925bb9c6 aka ("of/i2c: Fix module load order issue caused by of_i2c.c) moved the of_i2c_register_devices() function from the i2c core back to the drivers. This patch does the same thing for the pxa driver.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

Signed-off-by Dirk Brandewie <[email protected]>

---
?drivers/i2c/busses/i2c-pxa.c | ? ?5 +++++
?1 files changed, 5 insertions(+), 0 deletions(-)

drivers/i2c/busses/i2c-pxa.c +0 -0

index bfd115d..a90739b 100644
@@ -29,6 +29,7 @@

?#include <linux/errno.h>
?#include <linux/interrupt.h>
?#include <linux/i2c-pxa.h>
+#include <linux/of_i2c.h>
?#include <linux/platform_device.h>
?#include <linux/err.h>
?#include <linux/clk.h>

drivers/i2c/busses/i2c-pxa.c +4 -1

@@ -1138,6 +1139,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
 
 ? ? ? ?i2c->adap.algo_data = i2c;
 ? ? ? ?i2c->adap.dev.parent = &dev->dev;
+#ifdef CONFIG_OF
+ ? ? ? i2c->adap.dev.of_node = dev->dev.of_node;
+#endif
 
 ? ? ? ?if (i2c_type == REGS_CE4100)
 ? ? ? ? ? ? ? ?ret = i2c_add_adapter(&i2c->adap);

@@ -1147,6 +1151,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
 ? ? ? ? ? ? ? ?printk(KERN_INFO "I2C: Failed to add bus\n");
 ? ? ? ? ? ? ? ?goto eadapt;
 ? ? ? ?}
+ ? ? ? of_i2c_register_devices(&i2c->adap);
 
 ? ? ? ?platform_set_drvdata(dev, i2c);
 
--

1.7.3.2

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The I2C core inside CE4100 is very much the same as in PXA25x. I Cc the ARM folks because patch 2 reorganizes some files so that they can be accessed from x86.
The I2C device is behind a PCI bus. The PCI probe code simply creates three platform devices so we don't have much changes to the platform driver. There is no clock framework on x86 and the peripherals don't support changing their clocks or to enable/disable them. So I provided dummy function which keep the driver happy.
This series indirectly depends on the SPI series because both modify include files and spi & i2c includes are sometimes too close to each other.

History:
v1: Initial post
v2: - ISAR is still touched if not in SLAVE mode except on X86 where it
is not touched at all.
- There are no files created in include/asm
- register defines are in common code
- the PCI controller now requires a device tree. It is used to obtain an id (which is used as device & i2c bus id).
- the PCI part now uses ARRAY_SIZE(). As it turns out pci_select_bars() is not useable.
- the patch which resetted the chip if the i2c bus was not available has been dropped.
v3: - introduced dynamic register mapping. With this change I can avoid
accessing the unavailable ISAR register without an #ifdef around it.
- modified the DT part of the pci driver to address latest DT review comments.

The patch series has been created with the -M option so file moves can be easily noticed. The whole series (including the spi rename) is also available at
git://git.linutronix.de/users/bigeasy/soda.git spi-i2c

Sebastian Andrzej Siewior (6):
i2c/pxa: use dynamic register layout
arm/pxa2xx: reorganize I2C files
i2c/pxa2xx: Add PCI support for PXA I2C controller i2c/pxa2xx: add support for shared IRQ handler i2c/pxa2xx: check timeout correctly
i2c/pxa2xx: pass of_node from platform driver to adapter and publish

 arch/arm/mach-mmp/include/mach/mmp2.h              |    2 +-
 arch/arm/mach-mmp/include/mach/pxa168.h            |    2 +-
 arch/arm/mach-mmp/include/mach/pxa910.h            |    2 +-
 arch/arm/mach-pxa/balloon3.c                       |    3 +-
 arch/arm/mach-pxa/cm-x300.c                        |    2 +-
 arch/arm/mach-pxa/colibri-pxa270-income.c          |    3 +-
 arch/arm/mach-pxa/corgi.c                          |    2 +-
 arch/arm/mach-pxa/csb726.c                         |    2 +-
 arch/arm/mach-pxa/devices.c                        |    2 +-
 arch/arm/mach-pxa/em-x270.c                        |    2 +-
 arch/arm/mach-pxa/ezx.c                            |    2 +-
 arch/arm/mach-pxa/hx4700.c                         |    2 +-
 arch/arm/mach-pxa/littleton.c                      |    2 +-
 arch/arm/mach-pxa/magician.c                       |    2 +-
 arch/arm/mach-pxa/mainstone.c                      |    2 +-
 arch/arm/mach-pxa/mioa701.c                        |    2 +-
 arch/arm/mach-pxa/mxm8x10.c                        |    2 +-
 arch/arm/mach-pxa/palm27x.c                        |    3 +-
 arch/arm/mach-pxa/pcm990-baseboard.c               |    2 +-
 arch/arm/mach-pxa/poodle.c                         |    2 +-
 arch/arm/mach-pxa/pxa27x.c                         |    2 +-
 arch/arm/mach-pxa/pxa3xx.c                         |    2 +-
 arch/arm/mach-pxa/raumfeld.c                       |    2 +-
 arch/arm/mach-pxa/saar.c                           |    2 +-
 arch/arm/mach-pxa/spitz.c                          |    3 +-
 arch/arm/mach-pxa/stargate2.c                      |    2 +-
 arch/arm/mach-pxa/tavorevb3.c                      |    3 +-
 arch/arm/mach-pxa/tosa.c                           |    2 +-
 arch/arm/mach-pxa/trizeps4.c                       |    2 +-
 arch/arm/mach-pxa/viper.c                          |    2 +-
 arch/arm/mach-pxa/vpac270.c                        |    3 +-
 arch/arm/mach-pxa/xcep.c                           |    3 +-
 arch/arm/mach-pxa/z2.c                             |    3 +-
 arch/arm/mach-pxa/zeus.c                           |    3 +-
 arch/arm/mach-pxa/zylonite_pxa300.c                |    2 +-
 drivers/i2c/busses/Kconfig                         |    7 +-
 drivers/i2c/busses/Makefile                        |    1 +
 drivers/i2c/busses/i2c-pxa-pci.c                   |  173 ++++++++++++++++++++
 drivers/i2c/busses/i2c-pxa.c                       |  116 ++++++++++---
 .../plat/i2c.h => include/linux/i2c/pxa-i2c.h      |    0
 40 files changed, 304 insertions(+), 72 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-pxa-pci.c
 rename arch/arm/plat-pxa/include/plat/i2c.h => include/linux/i2c/pxa-i2c.h (100%)

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines

Sebastian Andrzej Siewior (6):
i2c/pxa: use dynamic register layout
arm/pxa2xx: reorganize I2C files
i2c/pxa2xx: Add PCI support for PXA I2C controller i2c/pxa2xx: add support for shared IRQ handler i2c/pxa2xx: check timeout correctly
i2c/pxa2xx: pass of_node from platform driver to adapter and publish

please rename these i2c-pxa2xx
and for the ARM, use ARM: pxa2xx as the prefix