i2c: new bus driver for efm32

This was tested on a EFM32GG-DK3750 devboard that has a temperature sensor and an eeprom on its i2c bus.

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
Hello,

there are two places marked with a triple-X where I don't know if I did it right. The one is "Do I need to protect from .master_xfer being called again before the previous call returned?" The other is what to return in the .functionality callback.

Also note there is a matching entry in MAINTAINERS for "ARM/ENERGY MICRO (SILICON LABS) EFM32 SUPPORT".

Best regards
Uwe

 drivers/i2c/busses/Kconfig              |   7 +
 drivers/i2c/busses/Makefile             |   1 +
 drivers/i2c/busses/i2c-efm32.c          | 527 ++++++++++++++++++++++++++++++++
 include/linux/platform_data/efm32-i2c.h |  15 +
 4 files changed, 550 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-efm32.c
 create mode 100644 include/linux/platform_data/efm32-i2c.h

drivers/i2c/busses/Kconfig +7 -0

index f5ed03164d86..7322ff0c4c60 100644
@@ -432,6 +432,13 @@ config I2C_DESIGNWARE_PCI
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-designware-pci.
 
+config I2C_EFM32
+	tristate "EFM32 I2C controller"
+	depends on OF && (ARCH_EFM32 || COMPILE_TEST)
+	help
+	  This driver supports the i2c block found in Energy Micro's EFM32
+	  SoCs.
+
 config I2C_EG20T
 	tristate "Intel EG20T PCH/LAPIS Semicon IOH(ML7213/ML7223/ML7831) I2C"
 	depends on PCI

drivers/i2c/busses/Makefile +1 -0

index a08931fe73e1..2a56ab181851 100644
@@ -41,6 +41,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-platform.o
 i2c-designware-platform-objs := i2c-designware-platdrv.o
 obj-$(CONFIG_I2C_DESIGNWARE_PCI)	+= i2c-designware-pci.o
 i2c-designware-pci-objs := i2c-designware-pcidrv.o
+obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
 obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
 obj-$(CONFIG_I2C_EXYNOS5)	+= i2c-exynos5.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o

drivers/i2c/busses/i2c-efm32.c new file +527 -0

new file mode 100644
index 000000000000..9e860abcab63
@@ -0,0 +1,527 @@
+/*
+ * Copyright (C) 2014 Uwe Kleine-Koenig for Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License version 2 as published by the
+ * Free Software Foundation.
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/platform_data/efm32-i2c.h>
+
+#define DRIVER_NAME "efm32-i2c"
+
+#define MASK_VAL(mask, val)		((val << __ffs(mask)) & mask)
+
+#define REG_CTRL		0x00
+#define REG_CTRL_EN			0x00001
+#define REG_CTRL_SLAVE			0x00002
+#define REG_CTRL_AUTOACK		0x00004
+#define REG_CTRL_AUTOSE			0x00008
+#define REG_CTRL_AUTOSN			0x00010
+#define REG_CTRL_ARBDIS			0x00020
+#define REG_CTRL_GCAMEN			0x00040
+#define REG_CTRL_CLHR__MASK		0x00300
+#define REG_CTRL_BITO__MASK		0x03000
+#define REG_CTRL_BITO_OFF		0x00000
+#define REG_CTRL_BITO_40PCC		0x01000
+#define REG_CTRL_BITO_80PCC		0x02000
+#define REG_CTRL_BITO_160PCC		0x03000
+#define REG_CTRL_GIBITO			0x08000
+#define REG_CTRL_CLTO__MASK		0x70000
+#define REG_CTRL_CLTO_OFF		0x00000
+
+#define REG_CMD			0x04
+#define REG_CMD_START			0x00001
+#define REG_CMD_STOP			0x00002
+#define REG_CMD_ACK			0x00004
+#define REG_CMD_NACK			0x00008
+#define REG_CMD_CONT			0x00010
+#define REG_CMD_ABORT			0x00020
+#define REG_CMD_CLEARTX			0x00040
+#define REG_CMD_CLEARPC			0x00080
+
+#define REG_STATE		0x08
+#define REG_STATE_BUSY			0x00001
+#define REG_STATE_MASTER		0x00002
+#define REG_STATE_TRANSMITTER		0x00004
+#define REG_STATE_NACKED		0x00008
+#define REG_STATE_BUSHOLD		0x00010
+#define REG_STATE_STATE__MASK		0x000e0
+#define REG_STATE_STATE_IDLE		0x00000
+#define REG_STATE_STATE_WAIT		0x00020
+#define REG_STATE_STATE_START		0x00040
+#define REG_STATE_STATE_ADDR		0x00060
+#define REG_STATE_STATE_ADDRACK		0x00080
+#define REG_STATE_STATE_DATA		0x000a0
+#define REG_STATE_STATE_DATAACK		0x000c0
+
+#define REG_STATUS		0x0c
+#define REG_STATUS_PSTART		0x00001
+#define REG_STATUS_PSTOP		0x00002
+#define REG_STATUS_PACK			0x00004
+#define REG_STATUS_PNACK		0x00008
+#define REG_STATUS_PCONT		0x00010
+#define REG_STATUS_PABORT		0x00020
+#define REG_STATUS_TXC			0x00040
+#define REG_STATUS_TXBL			0x00080
+#define REG_STATUS_RXDATAV		0x00100
+
+#define REG_CLKDIV		0x10
+#define REG_CLKDIV_DIV__MASK		0x001ff
+#define REG_CLKDIV_DIV(div)		MASK_VAL(REG_CLKDIV_DIV__MASK, (div))
+
+#define REG_SADDR		0x14
+#define REG_SADDRMASK		0x18
+#define REG_RXDATA		0x1c
+#define REG_RXDATAP		0x20
+#define REG_TXDATA		0x24
+#define REG_IF			0x28
+#define REG_IF_START			0x00001
+#define REG_IF_RSTART			0x00002
+#define REG_IF_ADDR			0x00004
+#define REG_IF_TXC			0x00008
+#define REG_IF_TXBL			0x00010
+#define REG_IF_RXDATAV			0x00020
+#define REG_IF_ACK			0x00040
+#define REG_IF_NACK			0x00080
+#define REG_IF_MSTOP			0x00100
+#define REG_IF_ARBLOST			0x00200
+#define REG_IF_BUSERR			0x00400
+#define REG_IF_BUSHOLD			0x00800
+#define REG_IF_TXOF			0x01000
+#define REG_IF_RXUF			0x02000
+#define REG_IF_BITO			0x04000
+#define REG_IF_CLTO			0x08000
+#define REG_IF_SSTOP			0x10000
+
+#define REG_IFS			0x2c
+#define REG_IFC			0x30
+#define REG_IFC__MASK			0x1ffcf
+
+#define REG_IEN			0x34
+
+#define REG_ROUTE		0x38
+#define REG_ROUTE_SDAPEN		0x00001
+#define REG_ROUTE_SCLPEN		0x00002
+#define REG_ROUTE_LOCATION__MASK	0x00700
+#define REG_ROUTE_LOCATION(n)		MASK_VAL(REG_ROUTE_LOCATION__MASK, (n))
+
+struct efm32_i2c_ddata {
+	struct i2c_adapter adapter;
+	spinlock_t lock;
+
+	struct clk *clk;
+	void __iomem *base;
+	unsigned int irq;
+	struct efm32_i2c_pdata pdata;
+
+	/* transfer data */
+	struct completion done;
+	struct i2c_msg *msgs;
+	size_t num_msgs;
+	size_t current_word, current_msg;
+};
+
+static u32 efm32_i2c_read32(struct efm32_i2c_ddata *ddata, unsigned offset)
+{
+	return readl(ddata->base + offset);
+}
+
+static void efm32_i2c_write32(struct efm32_i2c_ddata *ddata,
+		unsigned offset, u32 value)
+{
+	writel(value, ddata->base + offset);
+}
+
+static void efm32_i2c_send_next_msg(struct efm32_i2c_ddata *ddata)
+{
+	struct i2c_msg *cur_msg = &ddata->msgs[ddata->current_msg];
+
+	dev_dbg(&ddata->adapter.dev, "send msg %zu/%zu (addr = %x, flags = %x, if = %08x)\n",
+			ddata->current_msg, ddata->num_msgs, cur_msg->addr,
+			cur_msg->flags, efm32_i2c_read32(ddata, REG_IF));
+	efm32_i2c_write32(ddata, REG_CMD, REG_CMD_START);
+	efm32_i2c_write32(ddata, REG_TXDATA, cur_msg->addr << 1 |
+			(cur_msg->flags & I2C_M_RD ? 1 : 0));
+}
+
+static void efm32_i2c_send_next_byte(struct efm32_i2c_ddata *ddata)
+{
+	struct i2c_msg *cur_msg = &ddata->msgs[ddata->current_msg];
+	dev_dbg(&ddata->adapter.dev, "%s, %zu %zu\n",
+			__func__, ddata->current_word, cur_msg->len);
+	if (ddata->current_word >= cur_msg->len) {
+		/* cur_msg completely transferred */
+		ddata->current_word = 0;
+		ddata->current_msg += 1;
+
+		if (ddata->current_msg >= ddata->num_msgs) {
+			dev_dbg(&ddata->adapter.dev, "Stop\n");
+			efm32_i2c_write32(ddata, REG_CMD, REG_CMD_STOP);
+			complete(&ddata->done);
+		} else {
+			efm32_i2c_send_next_msg(ddata);
+		}
+	} else {
+		dev_dbg(&ddata->adapter.dev, "send byte %zu/%zu\n",
+				ddata->current_word, cur_msg->len);
+		efm32_i2c_write32(ddata, REG_TXDATA,
+				cur_msg->buf[ddata->current_word++]);
+	}
+}
+
+static void efm32_i2c_recv_next_byte(struct efm32_i2c_ddata *ddata)
+{
+	struct i2c_msg *cur_msg = &ddata->msgs[ddata->current_msg];
+
+	cur_msg->buf[ddata->current_word] = efm32_i2c_read32(ddata, REG_RXDATA);
+	dev_dbg(&ddata->adapter.dev, "recv byte %zu/%zu: 0x%02hhx\n",
+			ddata->current_word, cur_msg->len, cur_msg->buf[ddata->current_word]);
+	ddata->current_word += 1;
+	if (ddata->current_word >= cur_msg->len) {
+		/* cur_msg completely transferred */
+		ddata->current_word = 0;
+		ddata->current_msg += 1;
+
+		efm32_i2c_write32(ddata, REG_CMD, REG_CMD_NACK);
+
+		if (ddata->current_msg >= ddata->num_msgs) {
+			efm32_i2c_write32(ddata, REG_CMD, REG_CMD_STOP);
+			complete(&ddata->done);
+		} else {
+			efm32_i2c_send_next_msg(ddata);
+		}
+	} else {
+		efm32_i2c_write32(ddata, REG_CMD, REG_CMD_ACK);
+	}
+}
+
+static irqreturn_t efm32_i2c_irq(int irq, void *dev_id)
+{
+	struct efm32_i2c_ddata *ddata = dev_id;
+	struct i2c_msg *cur_msg = &ddata->msgs[ddata->current_msg];
+	u32 irqflag = efm32_i2c_read32(ddata, REG_IF);
+	u32 state = efm32_i2c_read32(ddata, REG_STATE);
+
+	dev_dbg(&ddata->adapter.dev, "irq: if: %08x, state: %08x, status: %08x\n",
+			irqflag, state, efm32_i2c_read32(ddata, REG_STATUS));
+	efm32_i2c_write32(ddata, REG_IFC, irqflag & REG_IFC__MASK);
+
+	switch (state & REG_STATE_STATE__MASK) {
+	case REG_STATE_STATE_IDLE:
+		/* arbitration lost? */
+		complete(&ddata->done);
+		break;
+	case REG_STATE_STATE_WAIT:
+		/* huh, this shouldn't happen */
+		BUG();
+		break;
+	case REG_STATE_STATE_START:
+		/* "caller" is expected to send an address */
+		break;
+	case REG_STATE_STATE_ADDR:
+		/* wait for Ack or NAck of slave */
+		break;
+	case REG_STATE_STATE_ADDRACK:
+		if (state & REG_STATE_NACKED) {
+			efm32_i2c_write32(ddata, REG_CMD, REG_CMD_STOP);
+			complete(&ddata->done);
+		} else if (cur_msg->flags & I2C_M_RD) {
+			/* wait for slave to send first data byte */
+		} else {
+			efm32_i2c_send_next_byte(ddata);
+		}
+		break;
+	case REG_STATE_STATE_DATA:
+		if (cur_msg->flags & I2C_M_RD) {
+			efm32_i2c_recv_next_byte(ddata);
+		} else {
+			/* wait for Ack or Nack of slave */
+		}
+		break;
+	case REG_STATE_STATE_DATAACK:
+		if (state & REG_STATE_NACKED) {
+			efm32_i2c_write32(ddata, REG_CMD, REG_CMD_STOP);
+			complete(&ddata->done);
+		} else {
+			efm32_i2c_send_next_byte(ddata);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int efm32_i2c_master_xfer(struct i2c_adapter *adap,
+		struct i2c_msg *msgs, int num)
+{
+	struct efm32_i2c_ddata *ddata = i2c_get_adapdata(adap);
+	int ret = -EBUSY;
+
+	spin_lock_irq(&ddata->lock);
+
+	if (ddata->msgs)
+		/*
+		 * XXX: can .master_xfer be called when the previous is still
+		 * running?
+		 */
+		goto out_unlock;
+
+	ddata->msgs = msgs;
+	ddata->num_msgs = num;
+	ddata->current_word = 0;
+	ddata->current_msg = 0;
+
+	init_completion(&ddata->done);
+
+	dev_dbg(&ddata->adapter.dev, "state: %08x, status: %08x\n",
+			efm32_i2c_read32(ddata, REG_STATE),
+			efm32_i2c_read32(ddata, REG_STATUS));
+
+	efm32_i2c_send_next_msg(ddata);
+
+	spin_unlock_irq(&ddata->lock);
+
+	wait_for_completion(&ddata->done);
+
+	spin_lock_irq(&ddata->lock);
+
+	if (ddata->current_msg >= ddata->num_msgs)
+		ret = ddata->num_msgs;
+	else
+		ret = -EIO;
+
+	ddata->msgs = NULL;
+
+out_unlock:
+	spin_unlock_irq(&ddata->lock);
+	return ret;
+}
+
+static u32 efm32_i2c_functionality(struct i2c_adapter *adap)
+{
+	/* XXX: some more? */
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm efm32_i2c_algo = {
+	.master_xfer = efm32_i2c_master_xfer,
+	.functionality = efm32_i2c_functionality,
+};
+
+static u32 efm32_i2c_get_configured_location(struct efm32_i2c_ddata *ddata)
+{
+	u32 reg = efm32_i2c_read32(ddata, REG_ROUTE);
+
+	return (reg & REG_ROUTE_LOCATION__MASK) >>
+		__ffs(REG_ROUTE_LOCATION__MASK);
+}
+
+static int efm32_i2c_probe_dt(struct platform_device *pdev,
+		struct efm32_i2c_ddata *ddata)
+{
+	struct device_node *np = pdev->dev.of_node;
+	u32 location, frequency;
+	int ret;
+
+	if (!np)
+		return 1;
+
+	ret = of_property_read_u32(np, "location", &location);
+	if (!ret) {
+		dev_dbg(&pdev->dev, "using location %u\n", location);
+	} else {
+		/* default to location configured in hardware */
+		location = efm32_i2c_get_configured_location(ddata);
+
+		dev_info(&pdev->dev, "fall back to location %u\n", location);
+	}
+
+	ddata->pdata.location = location;
+
+	ret = of_property_read_u32(np, "clock-frequency", &frequency);
+	if (!ret) {
+		dev_dbg(&pdev->dev, "using frequency %u\n", frequency);
+	} else {
+		frequency = 100000;
+		dev_info(&pdev->dev, "defaulting to 100 kHz\n");
+	}
+	ddata->pdata.frequency = frequency;
+
+	/* i2c core takes care about bus numbering using an alias */
+	ddata->adapter.nr = -1;
+
+	return 0;
+}
+
+static int efm32_i2c_probe(struct platform_device *pdev)
+{
+	struct efm32_i2c_ddata *ddata;
+	struct resource *res;
+	unsigned long rate;
+	int ret;
+	u32 clkdiv;
+
+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata) {
+		dev_dbg(&pdev->dev, "failed to allocate private data\n");
+		return -ENOMEM;
+	}
+	platform_set_drvdata(pdev, ddata);
+
+	strlcpy(ddata->adapter.name, pdev->name, sizeof(ddata->adapter.name));
+	ddata->adapter.owner = THIS_MODULE;
+	ddata->adapter.algo = &efm32_i2c_algo;
+	ddata->adapter.dev.parent = &pdev->dev;
+	ddata->adapter.dev.of_node = pdev->dev.of_node;
+	i2c_set_adapdata(&ddata->adapter, ddata);
+
+	spin_lock_init(&ddata->lock);
+
+	ddata->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ddata->clk)) {
+		ret = PTR_ERR(ddata->clk);
+		dev_err(&pdev->dev, "failed to get clock: %d\n", ret);
+		return ret;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to determine base address\n");
+		return -ENODEV;
+	}
+
+	if (resource_size(res) < 0x42) {
+		dev_err(&pdev->dev, "memory resource too small\n");
+		return -EINVAL;
+	}
+
+	ddata->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ddata->base))
+		return PTR_ERR(ddata->base);
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret <= 0) {
+		dev_err(&pdev->dev, "failed to get irq (%d)\n", ret);
+		if (!ret)
+			ret = -EINVAL;
+		return ret;
+	}
+
+	ddata->irq = ret;
+
+	ret = clk_prepare_enable(ddata->clk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to enable clock (%d)\n", ret);
+		return ret;
+	}
+
+	ret = efm32_i2c_probe_dt(pdev, ddata);
+	if (ret > 0) {
+		/* not created by device tree */
+		const struct efm32_i2c_pdata *pdata =
+			dev_get_platdata(&pdev->dev);
+
+		if (pdata)
+			ddata->pdata = *pdata;
+		else {
+			ddata->pdata.location =
+				efm32_i2c_get_configured_location(ddata);
+			ddata->pdata.frequency = 100000;
+		}
+
+		ddata->adapter.nr = pdev->id;
+	} else if (ret < 0) {
+		goto err_disable_clk;
+	}
+
+	rate = clk_get_rate(ddata->clk);
+	if (!rate) {
+		dev_err(&pdev->dev, "there is no input clock available\n");
+		ret = -EIO;
+		goto err_disable_clk;
+	}
+	clkdiv = DIV_ROUND_UP(rate, 8 * ddata->pdata.frequency) - 1;
+	if (clkdiv >= 0x200) {
+		dev_err(&pdev->dev,
+				"input clock too fast (%lu) to divide down to bus freq (%lu)",
+				rate, ddata->pdata.frequency);
+		ret = -EIO;
+		goto err_disable_clk;
+	}
+
+	dev_dbg(&pdev->dev, "input clock = %lu, bus freq = %lu, clkdiv = %lu\n",
+			rate, ddata->pdata.frequency, (unsigned long)clkdiv);
+	efm32_i2c_write32(ddata, REG_CLKDIV, REG_CLKDIV_DIV(clkdiv));
+
+	efm32_i2c_write32(ddata, REG_ROUTE, REG_ROUTE_SDAPEN |
+			REG_ROUTE_SCLPEN |
+			REG_ROUTE_LOCATION(ddata->pdata.location));
+
+	efm32_i2c_write32(ddata, REG_CTRL, REG_CTRL_EN |
+			REG_CTRL_BITO_160PCC | 0 * REG_CTRL_GIBITO);
+
+	efm32_i2c_write32(ddata, REG_IFC, REG_IFC__MASK);
+	efm32_i2c_write32(ddata, REG_IEN, REG_IF_TXC | REG_IF_ACK | REG_IF_NACK
+			| REG_IF_ARBLOST | REG_IF_BUSERR | REG_IF_RXDATAV);
+
+	/* to make bus idle */
+	efm32_i2c_write32(ddata, REG_CMD, REG_CMD_ABORT);
+
+	ret = request_irq(ddata->irq, efm32_i2c_irq, 0, DRIVER_NAME, ddata);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request irq (%d)\n", ret);
+		return ret;
+	}
+
+	ret = i2c_add_numbered_adapter(&ddata->adapter);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add i2c adapter (%d)\n", ret);
+		free_irq(ddata->irq, ddata);
+
+err_disable_clk:
+		clk_disable_unprepare(ddata->clk);
+	}
+	return ret;
+}
+
+static int efm32_i2c_remove(struct platform_device *pdev)
+{
+	struct efm32_i2c_ddata *ddata = platform_get_drvdata(pdev);
+
+	free_irq(ddata->irq, ddata);
+	clk_disable_unprepare(ddata->clk);
+
+	return 0;
+}
+
+static const struct of_device_id efm32_i2c_dt_ids[] = {
+	{
+		.compatible = "efm32,i2c",
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, efm32_i2c_dt_ids);
+
+static struct platform_driver efm32_i2c_driver = {
+	.probe = efm32_i2c_probe,
+	.remove = efm32_i2c_remove,
+
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = efm32_i2c_dt_ids,
+	},
+};
+module_platform_driver(efm32_i2c_driver);
+
+MODULE_AUTHOR("Uwe Kleine-Koenig <[email protected]>");
+MODULE_DESCRIPTION("EFM32 i2c driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRIVER_NAME);

include/linux/platform_data/efm32-i2c.h new file +15 -0

new file mode 100644
index 000000000000..5e175db68bb6
@@ -0,0 +1,15 @@
+#ifndef __LINUX_PLATFORM_DATA_EFM32_I2C_H__
+#define __LINUX_PLATFORM_DATA_EFM32_I2C_H__
+
+#include <linux/types.h>
+
+/**
+ * struct efm32_i2c_pdata
+ * @location: pinmux location for the I/O pins (to be written to the ROUTE
+ * 	register)
+ */
+struct efm32_i2c_pdata {
+	u8 location;
+	unsigned long frequency; /* in Hz */
+};
+#endif /* ifndef __LINUX_PLATFORM_DATA_EFM32_I2C_H__ */

Signature · 1 line

1.8.5.2

This was tested on a EFM32GG-DK3750 devboard that has a temperature sensor and an eeprom on its i2c bus.

Signed-off-by Uwe Kleine-K?nig <[email protected]>

+#include <linux/platform_data/efm32-i2c.h>

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

+	struct clk *clk;
+	void __iomem *base;
+	unsigned int irq;
+	struct efm32_i2c_pdata pdata;
+
+	/* transfer data */
+	struct completion done;
+	struct i2c_msg *msgs;
+	size_t num_msgs;
+	size_t current_word, current_msg;
+};
+

drivers/i2c/busses/i2c-efm32.c new file +7 -0

+
+	dev_dbg(&ddata->adapter.dev, "state: %08x, status: %08x\n",
+			efm32_i2c_read32(ddata, REG_STATE),
+			efm32_i2c_read32(ddata, REG_STATUS));
+
+	efm32_i2c_send_next_msg(ddata);
+

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

+	if (!ret) {
+		dev_dbg(&pdev->dev, "using location %u\n", location);
+	} else {
+		/* default to location configured in hardware */
+		location = efm32_i2c_get_configured_location(ddata);
+
+		dev_info(&pdev->dev, "fall back to location %u\n", location);
+	}
+
+	ddata->pdata.location = location;
+
+	ret = of_property_read_u32(np, "clock-frequency", &frequency);

drivers/i2c/busses/i2c-efm32.c new file +4 -0

+	rate = clk_get_rate(ddata->clk);
+	if (!rate) {
+		dev_err(&pdev->dev, "there is no input clock available\n");
+		ret = -EIO;

Quoted 1 line

+#include <linux/platform_data/efm32-i2c.h>

Shouldn't a new platform like efm32 be DT only?

drivers/i2c/busses/i2c-efm32.c line 117

Quoted 4 lines

drivers/i2c/busses/i2c-efm32.c new file +4 -0

+
+struct efm32_i2c_ddata {
+	struct i2c_adapter adapter;
+	spinlock_t lock;

No need, see later.

Quoted 5 lines

...

drivers/i2c/busses/i2c-efm32.c new file +4 -0

+static u32 efm32_i2c_read32(struct efm32_i2c_ddata *ddata, unsigned offset)
+{
+	return readl(ddata->base + offset);
+}

drivers/i2c/busses/i2c-efm32.c line 140

Quoted 6 lines

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

+
+static void efm32_i2c_write32(struct efm32_i2c_ddata *ddata,
+		unsigned offset, u32 value)
+{
+	writel(value, ddata->base + offset);
+}

Do those two really help readability?

drivers/i2c/busses/i2c-efm32.c line 148

Quoted 7 lines

drivers/i2c/busses/i2c-efm32.c new file +7 -0

+static void efm32_i2c_send_next_msg(struct efm32_i2c_ddata *ddata)
+{
+	struct i2c_msg *cur_msg = &ddata->msgs[ddata->current_msg];
+
+	dev_dbg(&ddata->adapter.dev, "send msg %zu/%zu (addr = %x, flags = %x, if = %08x)\n",
+			ddata->current_msg, ddata->num_msgs, cur_msg->addr,
+			cur_msg->flags, efm32_i2c_read32(ddata, REG_IF));

Remove. We have stuff like this in the core and will soon get tracing functionality.

drivers/i2c/busses/i2c-efm32.c line 219

Quoted 4 lines

drivers/i2c/busses/i2c-efm32.c new file +4 -0

+	switch (state & REG_STATE_STATE__MASK) {
+	case REG_STATE_STATE_IDLE:
+		/* arbitration lost? */
+		complete(&ddata->done);

If arbitration is lost, you should return -EAGAIN.

drivers/i2c/busses/i2c-efm32.c line 223

Quoted 4 lines

drivers/i2c/busses/i2c-efm32.c new file +4 -0

+		break;
+	case REG_STATE_STATE_WAIT:
+		/* huh, this shouldn't happen */
+		BUG();

Is this really a reason to halt the kernel?

Quoted 7 lines

drivers/i2c/busses/i2c-efm32.c new file +7 -0

+static int efm32_i2c_master_xfer(struct i2c_adapter *adap,
+		struct i2c_msg *msgs, int num)
+{
+	struct efm32_i2c_ddata *ddata = i2c_get_adapdata(adap);
+	int ret = -EBUSY;
+
+	spin_lock_irq(&ddata->lock);

drivers/i2c/busses/i2c-efm32.c line 272

Quoted 6 lines

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

+
+	if (ddata->msgs)
+		/*
+		 * XXX: can .master_xfer be called when the previous is still
+		 * running?
+		 */

Check the core. It has per adapter locks. So the lock can go away.

drivers/i2c/busses/i2c-efm32.c line 280

Quoted 8 lines

drivers/i2c/busses/i2c-efm32.c new file +8 -0

+		goto out_unlock;
+
+	ddata->msgs = msgs;
+	ddata->num_msgs = num;
+	ddata->current_word = 0;
+	ddata->current_msg = 0;
+
+	init_completion(&ddata->done);

reinit_completion() here and init_completion() in probe.

Quoted 11 lines

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

+
+	dev_dbg(&ddata->adapter.dev, "state: %08x, status: %08x\n",
+			efm32_i2c_read32(ddata, REG_STATE),
+			efm32_i2c_read32(ddata, REG_STATUS));
+
+	efm32_i2c_send_next_msg(ddata);
+
+	spin_unlock_irq(&ddata->lock);
+
+	wait_for_completion(&ddata->done);
+

drivers/i2c/busses/i2c-efm32.c line 297

Quoted 6 lines

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

+	spin_lock_irq(&ddata->lock);
+
+	if (ddata->current_msg >= ddata->num_msgs)
+		ret = ddata->num_msgs;
+	else
+		ret = -EIO;

Check Documentation/i2c/fault-codes for more fine grained responses.

Quoted 7 lines

drivers/i2c/busses/i2c-efm32.c new file +7 -0

+
+	ddata->msgs = NULL;
+
+out_unlock:
+	spin_unlock_irq(&ddata->lock);
+	return ret;
+}

drivers/i2c/busses/i2c-efm32.c line 309

Quoted 5 lines

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

+
+static u32 efm32_i2c_functionality(struct i2c_adapter *adap)
+{
+	/* XXX: some more? */
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

That is usually enough. Make sure you checked SMBUS_QUICK, though (i2cdetect -q ...).

drivers/i2c/busses/i2c-efm32.c line 335

Quoted 1 line

drivers/i2c/busses/i2c-efm32.c new file +1 -0

+	ret = of_property_read_u32(np, "location", &location);

Huh? Is this an accepted binding? Doesn't look like it because of a generic name and IMO a specific use-case. BTW the binding documentation for this driver is missing.

drivers/i2c/busses/i2c-efm32.c line 357

Quoted 3 lines

...

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

+	/* i2c core takes care about bus numbering using an alias */
+	ddata->adapter.nr = -1;

In case of DT only, drop this and simply use i2c_add_adapter.

drivers/i2c/busses/i2c-efm32.c line 395

Quoted 8 lines

drivers/i2c/busses/i2c-efm32.c new file +7 -0

+
+	return 0;
+}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to determine base address\n");

devm_ioremap_resource() checks for a valid resource. Drop this.

drivers/i2c/busses/i2c-efm32.c line 402

Quoted 7 lines

drivers/i2c/busses/i2c-efm32.c new file +7 -0

+		return -ENODEV;
+	}
+
+	if (resource_size(res) < 0x42) {
+		dev_err(&pdev->dev, "memory resource too small\n");
+		return -EINVAL;
+	}

I'd drop this check since, but I won't force you to.

Quoted 8 lines

drivers/i2c/busses/i2c-efm32.c new file +8 -0

+	rate = clk_get_rate(ddata->clk);
+	if (!rate) {
+		dev_err(&pdev->dev, "there is no input clock available\n");
+		ret = -EIO;
+		goto err_disable_clk;
+	}
+	clkdiv = DIV_ROUND_UP(rate, 8 * ddata->pdata.frequency) - 1;
+	if (clkdiv >= 0x200) {

drivers/i2c/busses/i2c-efm32.c line 456

Quoted 6 lines

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

+		dev_err(&pdev->dev,
+				"input clock too fast (%lu) to divide down to bus freq (%lu)",
+				rate, ddata->pdata.frequency);
+		ret = -EIO;
+		goto err_disable_clk;
+	}

-EIO for clocks errors? Is this common?

drivers/i2c/busses/i2c-efm32.c line 498

Quoted 6 lines

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

+static int efm32_i2c_remove(struct platform_device *pdev)
+{
+	struct efm32_i2c_ddata *ddata = platform_get_drvdata(pdev);
+
+	free_irq(ddata->irq, ddata);
+	clk_disable_unprepare(ddata->clk);

No i2c_del_adapter()?

it is, at least in mainline. My (not very strong) POV is that it's not much effort/code size to support both. I dropped the non-DT part, it's easily readded if need should arise.

Quoted 6 lines

drivers/i2c/busses/i2c-efm32.c line 223

Quoted 4 lines

drivers/i2c/busses/i2c-efm32.c new file +4 -0

+		break;
+	case REG_STATE_STATE_WAIT:
+		/* huh, this shouldn't happen */
+		BUG();

Is this really a reason to halt the kernel?

No, probably not. What do you suggest? Reinit the hardware, report and return an error?

Quoted 1 line

Check the core. It has per adapter locks. So the lock can go away.

ok. So I can also drop the "if (ddata->msgs)" check, right?

Quoted 1 line

Check Documentation/i2c/fault-codes for more fine grained responses.

ok, I have EAGAIN for arbitration lost, ENXIO for NAck in address phase and EIO for NAck in data phase now. Sounds good?

Quoted 2 lines

That is usually enough. Make sure you checked SMBUS_QUICK, though (i2cdetect -q ...).

Both -q and -r seem to do the right thing.

Quoted 3 lines

Huh? Is this an accepted binding? Doesn't look like it because of a generic name and IMO a specific use-case. BTW the binding documentation for this driver is missing.

Regarding the generic name: I don't care much, but I don't have a problem with it. IMHO it's implicitly name-spaced by the compatible string which starts with "efm32," and so is fine. I'd like to have the same property name for all efm32 device drivers and "location" matches the hardware reference manual (apart from capitalization).

Quoted 5 lines

drivers/i2c/busses/i2c-efm32.c line 395

Quoted 3 lines

drivers/i2c/busses/i2c-efm32.c new file +3 -0

+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to determine base address\n");

devm_ioremap_resource() checks for a valid resource. Drop this.

But resource_size doesn't ...

Quoted 10 lines

drivers/i2c/busses/i2c-efm32.c line 402

Quoted 8 lines

drivers/i2c/busses/i2c-efm32.c new file +7 -0

+		return -ENODEV;
+	}
+
+	if (resource_size(res) < 0x42) {
+		dev_err(&pdev->dev, "memory resource too small\n");
+		return -EINVAL;
+	}

I'd drop this check since, but I won't force you to.

I'd understand your sentence with s/since//, not sure about it as is. Anyhow, I like this check.

Quoted 10 lines

drivers/i2c/busses/i2c-efm32.c line 456

Quoted 8 lines

drivers/i2c/busses/i2c-efm32.c new file +8 -0

+	clkdiv = DIV_ROUND_UP(rate, 8 * ddata->pdata.frequency) - 1;
+	if (clkdiv >= 0x200) {
+		dev_err(&pdev->dev,
+				"input clock too fast (%lu) to divide down to bus freq (%lu)",
+				rate, ddata->pdata.frequency);
+		ret = -EIO;
+		goto err_disable_clk;
+	}

-EIO for clocks errors? Is this common?

Changed to ENODEV. Ok?

Quoted 8 lines

Quoted 3 lines

Huh? Is this an accepted binding? Doesn't look like it because of a generic name and IMO a specific use-case. BTW the binding documentation for this driver is missing.

Regarding the generic name: I don't care much, but I don't have a problem with it. IMHO it's implicitly name-spaced by the compatible string which starts with "efm32," and so is fine. I'd like to have the same property name for all efm32 device drivers and "location" matches the hardware reference manual (apart from capitalization).

I would in deed have expected a binding like "efm32,location" to emphasize this is an efm32 specific thing. I know vendor-specific "setup" bindings from elsewhere. Since it has been accepted already in other places, we should keep it likes this.

Quoted 6 lines

Quoted 6 lines

drivers/i2c/busses/i2c-efm32.c line 402

Quoted 4 lines

drivers/i2c/busses/i2c-efm32.c new file +4 -0

+	if (resource_size(res) < 0x42) {
+		dev_err(&pdev->dev, "memory resource too small\n");
+		return -EINVAL;
+	}

I'd drop this check since, but I won't force you to.

Quoted 2 lines

I'd understand your sentence with s/since//, not sure about it as is. Anyhow, I like this check.

A leftover. I was about to write "since the check is somewhat heuristic and does not proof much". But then I decided it is not worth spending too much discussion on it :)

Quoted 10 lines

Quoted 10 lines

drivers/i2c/busses/i2c-efm32.c line 456

Quoted 8 lines

drivers/i2c/busses/i2c-efm32.c new file +8 -0

+	clkdiv = DIV_ROUND_UP(rate, 8 * ddata->pdata.frequency) - 1;
+	if (clkdiv >= 0x200) {
+		dev_err(&pdev->dev,
+				"input clock too fast (%lu) to divide down to bus freq (%lu)",
+				rate, ddata->pdata.frequency);
+		ret = -EIO;
+		goto err_disable_clk;
+	}

-EIO for clocks errors? Is this common?

Quoted 1 line

Changed to ENODEV. Ok?

Nope, then the driver core will silent drop the error. -EINVAL?