mc13xxx: add I2C support

mc13xxx: add I2C support

[Marc Reilly]

Hi,

This series (based on 3.2) adds I2C support for the mc13xxx family PMICs.
The patches are similar, but not identical to my previous patches for this [1], the
main difference being the way the config items are done in patch 2.

I don't have the hardware to retest on SPI hardware, I'd appreciate if someone could 
double check it still works.

I've also got a stack of patches for the mc13xxx to follow, but should get this in
first.

Cheers,
Marc

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/thread.html#36936

[PATCH 1/3] mc13xxx-core: Prepare for separate spi and i2c backends.

[Marc Reilly]

This patch abstracts the bus specific operations from the driver core.
Read and write handlers are introduced and generic initialization is
consolidated into mc13xxx_comon_init. The irq member is removed because
it is unused.

Signed-off-by: Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org>
---
 drivers/mfd/mc13xxx-core.c  |  162 +++++++++++++++++++++++++-----------------
 include/linux/mfd/mc13xxx.h |    5 ++
 2 files changed, 101 insertions(+), 66 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index e9619ac..3c3079f 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -19,10 +19,22 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/mc13xxx.h>
 
+enum mc13xxx_id {
+	MC13XXX_ID_MC13783,
+	MC13XXX_ID_MC13892,
+	MC13XXX_ID_INVALID,
+};
+
 struct mc13xxx {
 	struct spi_device *spidev;
+
+	struct device *dev;
+	enum mc13xxx_id ictype;
+
 	struct mutex lock;
-	int irq;
+
+	int (*read_dev)(struct mc13xxx *, unsigned int, u32 *);
+	int (*write_dev)(struct mc13xxx *, unsigned int, u32);
 
 	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
 	void *irqdata[MC13XXX_NUM_IRQ];
@@ -140,36 +152,32 @@ struct mc13xxx {
 void mc13xxx_lock(struct mc13xxx *mc13xxx)
 {
 	if (!mutex_trylock(&mc13xxx->lock)) {
-		dev_dbg(&mc13xxx->spidev->dev, "wait for %s from %pf\n",
+		dev_dbg(mc13xxx->dev, "wait for %s from %pf\n",
 				__func__, __builtin_return_address(0));
 
 		mutex_lock(&mc13xxx->lock);
 	}
-	dev_dbg(&mc13xxx->spidev->dev, "%s from %pf\n",
+	dev_dbg(mc13xxx->dev, "%s from %pf\n",
 			__func__, __builtin_return_address(0));
 }
 EXPORT_SYMBOL(mc13xxx_lock);
 
 void mc13xxx_unlock(struct mc13xxx *mc13xxx)
 {
-	dev_dbg(&mc13xxx->spidev->dev, "%s from %pf\n",
+	dev_dbg(mc13xxx->dev, "%s from %pf\n",
 			__func__, __builtin_return_address(0));
 	mutex_unlock(&mc13xxx->lock);
 }
 EXPORT_SYMBOL(mc13xxx_unlock);
 
 #define MC13XXX_REGOFFSET_SHIFT 25
-int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val)
+static int mc13xxx_spi_reg_read(struct mc13xxx *mc13xxx,
+				unsigned int offset, u32 *val)
 {
 	struct spi_transfer t;
 	struct spi_message m;
 	int ret;
 
-	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
-
-	if (offset > MC13XXX_NUMREGS)
-		return -EINVAL;
-
 	*val = offset << MC13XXX_REGOFFSET_SHIFT;
 
 	memset(&t, 0, sizeof(t));
@@ -191,26 +199,17 @@ int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val)
 
 	*val &= 0xffffff;
 
-	dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
-
 	return 0;
 }
-EXPORT_SYMBOL(mc13xxx_reg_read);
 
-int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val)
+static int mc13xxx_spi_reg_write(struct mc13xxx *mc13xxx, unsigned int offset,
+		u32 val)
 {
 	u32 buf;
 	struct spi_transfer t;
 	struct spi_message m;
 	int ret;
 
-	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
-
-	dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] <- 0x%06x\n", offset, val);
-
-	if (offset > MC13XXX_NUMREGS || val > 0xffffff)
-		return -EINVAL;
-
 	buf = 1 << 31 | offset << MC13XXX_REGOFFSET_SHIFT | val;
 
 	memset(&t, 0, sizeof(t));
@@ -231,6 +230,34 @@ int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val)
 
 	return 0;
 }
+
+int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val)
+{
+	int ret;
+
+	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
+
+	if (offset > MC13XXX_NUMREGS)
+		return -EINVAL;
+
+	ret = mc13xxx->read_dev(mc13xxx, offset, val);
+	dev_vdbg(mc13xxx->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
+
+	return ret;
+}
+EXPORT_SYMBOL(mc13xxx_reg_read);
+
+int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val)
+{
+	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
+
+	dev_vdbg(mc13xxx->dev, "[0x%02x] <- 0x%06x\n", offset, val);
+
+	if (offset > MC13XXX_NUMREGS || val > 0xffffff)
+		return -EINVAL;
+
+	return mc13xxx->write_dev(mc13xxx, offset, val);
+}
 EXPORT_SYMBOL(mc13xxx_reg_write);
 
 int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset,
@@ -435,7 +462,7 @@ static int mc13xxx_irq_handle(struct mc13xxx *mc13xxx,
 			if (handled == IRQ_HANDLED)
 				num_handled++;
 		} else {
-			dev_err(&mc13xxx->spidev->dev,
+			dev_err(mc13xxx->dev,
 					"BUG: irq %u but no handler\n",
 					baseirq + irq);
 
@@ -471,25 +498,23 @@ static irqreturn_t mc13xxx_irq_thread(int irq, void *data)
 	return IRQ_RETVAL(handled);
 }
 
-enum mc13xxx_id {
-	MC13XXX_ID_MC13783,
-	MC13XXX_ID_MC13892,
-	MC13XXX_ID_INVALID,
-};
-
 static const char *mc13xxx_chipname[] = {
 	[MC13XXX_ID_MC13783] = "mc13783",
 	[MC13XXX_ID_MC13892] = "mc13892",
 };
 
 #define maskval(reg, mask)	(((reg) & (mask)) >> __ffs(mask))
-static int mc13xxx_identify(struct mc13xxx *mc13xxx, enum mc13xxx_id *id)
+static int mc13xxx_identify(struct mc13xxx *mc13xxx)
 {
 	u32 icid;
 	u32 revision;
-	const char *name;
 	int ret;
 
+	/*
+	 * Get the generation ID from register 46, as apparently some older
+	 * IC revisions only have this info at this location. Newer ICs seem to
+	 * have both.
+	 */
 	ret = mc13xxx_reg_read(mc13xxx, 46, &icid);
 	if (ret)
 		return ret;
@@ -498,26 +523,23 @@ static int mc13xxx_identify(struct mc13xxx *mc13xxx, enum mc13xxx_id *id)
 
 	switch (icid) {
 	case 2:
-		*id = MC13XXX_ID_MC13783;
-		name = "mc13783";
+		mc13xxx->ictype = MC13XXX_ID_MC13783;
 		break;
 	case 7:
-		*id = MC13XXX_ID_MC13892;
-		name = "mc13892";
+		mc13xxx->ictype = MC13XXX_ID_MC13892;
 		break;
 	default:
-		*id = MC13XXX_ID_INVALID;
+		mc13xxx->ictype = MC13XXX_ID_INVALID;
 		break;
 	}
 
-	if (*id == MC13XXX_ID_MC13783 || *id == MC13XXX_ID_MC13892) {
+	if (mc13xxx->ictype == MC13XXX_ID_MC13783 ||
+			mc13xxx->ictype == MC13XXX_ID_MC13892) {
 		ret = mc13xxx_reg_read(mc13xxx, MC13XXX_REVISION, &revision);
-		if (ret)
-			return ret;
 
-		dev_info(&mc13xxx->spidev->dev, "%s: rev: %d.%d, "
+		dev_info(mc13xxx->dev, "%s: rev: %d.%d, "
 				"fin: %d, fab: %d, icid: %d/%d\n",
-				mc13xxx_chipname[*id],
+				mc13xxx_chipname[mc13xxx->ictype],
 				maskval(revision, MC13XXX_REVISION_REVFULL),
 				maskval(revision, MC13XXX_REVISION_REVMETAL),
 				maskval(revision, MC13XXX_REVISION_FIN),
@@ -526,32 +548,18 @@ static int mc13xxx_identify(struct mc13xxx *mc13xxx, enum mc13xxx_id *id)
 				maskval(revision, MC13XXX_REVISION_ICIDCODE));
 	}
 
-	if (*id != MC13XXX_ID_INVALID) {
-		const struct spi_device_id *devid =
-			spi_get_device_id(mc13xxx->spidev);
-		if (!devid || devid->driver_data != *id)
-			dev_warn(&mc13xxx->spidev->dev, "device id doesn't "
-					"match auto detection!\n");
-	}
-
-	return 0;
+	return (mc13xxx->ictype == MC13XXX_ID_INVALID) ? -ENODEV : 0;
 }
 
 static const char *mc13xxx_get_chipname(struct mc13xxx *mc13xxx)
 {
-	const struct spi_device_id *devid =
-		spi_get_device_id(mc13xxx->spidev);
-
-	if (!devid)
-		return NULL;
-
-	return mc13xxx_chipname[devid->driver_data];
+	return mc13xxx_chipname[mc13xxx->ictype];
 }
 
 int mc13xxx_get_flags(struct mc13xxx *mc13xxx)
 {
 	struct mc13xxx_platform_data *pdata =
-		dev_get_platdata(&mc13xxx->spidev->dev);
+		dev_get_platdata(mc13xxx->dev);
 
 	return pdata->flags;
 }
@@ -588,7 +596,7 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
 	};
 	init_completion(&adcdone_data.done);
 
-	dev_dbg(&mc13xxx->spidev->dev, "%s\n", __func__);
+	dev_dbg(mc13xxx->dev, "%s\n", __func__);
 
 	mc13xxx_lock(mc13xxx);
 
@@ -630,7 +638,7 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
 		return -EINVAL;
 	}
 
-	dev_dbg(&mc13xxx->spidev->dev, "%s: request irq\n", __func__);
+	dev_dbg(mc13xxx->dev, "%s: request irq\n", __func__);
 	mc13xxx_irq_request(mc13xxx, MC13XXX_IRQ_ADCDONE,
 			mc13xxx_handler_adcdone, __func__, &adcdone_data);
 	mc13xxx_irq_ack(mc13xxx, MC13XXX_IRQ_ADCDONE);
@@ -688,7 +696,7 @@ static int mc13xxx_add_subdevice_pdata(struct mc13xxx *mc13xxx,
 	if (!cell.name)
 		return -ENOMEM;
 
-	return mfd_add_devices(&mc13xxx->spidev->dev, -1, &cell, 1, NULL, 0);
+	return mfd_add_devices(mc13xxx->dev, -1, &cell, 1, NULL, 0);
 }
 
 static int mc13xxx_add_subdevice(struct mc13xxx *mc13xxx, const char *format)
@@ -700,7 +708,6 @@ static int mc13xxx_probe(struct spi_device *spi)
 {
 	struct mc13xxx *mc13xxx;
 	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
-	enum mc13xxx_id id;
 	int ret;
 
 	if (!pdata) {
@@ -717,13 +724,36 @@ static int mc13xxx_probe(struct spi_device *spi)
 	spi->bits_per_word = 32;
 	spi_setup(spi);
 
+	mc13xxx->dev = &spi->dev;
 	mc13xxx->spidev = spi;
+	mc13xxx->read_dev = mc13xxx_spi_reg_read;
+	mc13xxx->write_dev = mc13xxx_spi_reg_write;
+
+	ret = mc13xxx_common_init(mc13xxx, pdata, spi->irq);
+
+	if (ret) {
+		dev_set_drvdata(&spi->dev, NULL);
+	} else {
+		const struct spi_device_id *devid =
+			spi_get_device_id(mc13xxx->spidev);
+		if (!devid || devid->driver_data != mc13xxx->ictype)
+			dev_warn(mc13xxx->dev,
+				"device id doesn't match auto detection!\n");
+	}
+
+	return ret;
+}
+
+int mc13xxx_common_init(struct mc13xxx *mc13xxx,
+		struct mc13xxx_platform_data *pdata, int irq)
+{
+	int ret;
 
 	mutex_init(&mc13xxx->lock);
 	mc13xxx_lock(mc13xxx);
 
-	ret = mc13xxx_identify(mc13xxx, &id);
-	if (ret || id == MC13XXX_ID_INVALID)
+	ret = mc13xxx_identify(mc13xxx);
+	if (ret)
 		goto err_revision;
 
 	/* mask all irqs */
@@ -735,14 +765,13 @@ static int mc13xxx_probe(struct spi_device *spi)
 	if (ret)
 		goto err_mask;
 
-	ret = request_threaded_irq(spi->irq, NULL, mc13xxx_irq_thread,
+	ret = request_threaded_irq(irq, NULL, mc13xxx_irq_thread,
 			IRQF_ONESHOT | IRQF_TRIGGER_HIGH, "mc13xxx", mc13xxx);
 
 	if (ret) {
 err_mask:
 err_revision:
-		mc13xxx_unlock(mc13xxx);
-		dev_set_drvdata(&spi->dev, NULL);
+		mutex_unlock(&mc13xxx->lock);
 		kfree(mc13xxx);
 		return ret;
 	}
@@ -774,6 +803,7 @@ err_revision:
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mc13xxx_common_init);
 
 static int __devexit mc13xxx_remove(struct spi_device *spi)
 {
diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
index 3816c2f..3e38f72 100644
--- a/include/linux/mfd/mc13xxx.h
+++ b/include/linux/mfd/mc13xxx.h
@@ -21,6 +21,11 @@ int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val);
 int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset,
 		u32 mask, u32 val);
 
+struct mc13xxx_platform_data;
+
+int mc13xxx_common_init(struct mc13xxx *mc13xxx,
+		struct mc13xxx_platform_data *pdata, int irq);
+
 int mc13xxx_get_flags(struct mc13xxx *mc13xxx);
 
 int mc13xxx_irq_request(struct mc13xxx *mc13xxx, int irq,
-- 
1.7.3.4

[PATCH 2/3] mfd: mc13xxx-core: Move spi specific code into separate module.

[Marc Reilly]

All spi specific code is moved into a new module. The mc13xxx
struct moves to the include file by necessity.

A new config choice selects the bus type with SPI as the first item
(default selection) to remain compatible with existing configs.

Signed-off-by: Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org>
---
 drivers/mfd/Kconfig         |   23 +++++-
 drivers/mfd/Makefile        |    1 +
 drivers/mfd/mc13xxx-core.c  |  174 -------------------------------------------
 drivers/mfd/mc13xxx-spi.c   |  173 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/mc13xxx.h |   23 ++++++
 5 files changed, 216 insertions(+), 178 deletions(-)
 create mode 100644 drivers/mfd/mc13xxx-spi.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f1391c2..6cf84c8 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -514,17 +514,32 @@ config MFD_MC13783
 	tristate
 
 config MFD_MC13XXX
-	tristate "Support Freescale MC13783 and MC13892"
+	tristate "Support Freescale MC13783 or MC13892"
 	depends on SPI_MASTER
 	select MFD_CORE
 	select MFD_MC13783
 	help
-	  Support for the Freescale (Atlas) PMIC and audio CODECs
-	  MC13783 and MC13892.
-	  This driver provides common support for accessing  the device,
+	  Enable support for the Freescale MC13783 and MC13892 PMICs.
+	  This driver provides common support for accessing the device,
 	  additional drivers must be enabled in order to use the
 	  functionality of the device.
 
+choice
+	tristate "MC13XXX Bus interface type"
+	depends on MFD_MC13XXX
+	default MFD_MC13XXX_SPI
+	help
+	  The MC13XXX family can be connected over an SPI or I2C bus.
+	  Select the appropriate option for your hardware configuration.
+
+config MFD_MC13XXX_SPI
+	tristate "SPI interface"
+	depends on SPI_MASTER
+	help
+	  Select this if your MC13xxx is connected via an SPI bus.
+
+endchoice
+
 config ABX500_CORE
 	bool "ST-Ericsson ABX500 Mixed Signal Circuit register functions"
 	default y if ARCH_U300 || ARCH_U8500
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b2292eb..3856820 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_TWL6030_PWM)	+= twl6030-pwm.o
 obj-$(CONFIG_TWL6040_CORE)	+= twl6040-core.o twl6040-irq.o
 
 obj-$(CONFIG_MFD_MC13XXX)	+= mc13xxx-core.o
+obj-$(CONFIG_MFD_MC13XXX_SPI)	+= mc13xxx-spi.o
 
 obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
 
diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index 3c3079f..53732c0 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -15,33 +15,9 @@
 #include <linux/platform_device.h>
 #include <linux/mutex.h>
 #include <linux/interrupt.h>
-#include <linux/spi/spi.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/mc13xxx.h>
 
-enum mc13xxx_id {
-	MC13XXX_ID_MC13783,
-	MC13XXX_ID_MC13892,
-	MC13XXX_ID_INVALID,
-};
-
-struct mc13xxx {
-	struct spi_device *spidev;
-
-	struct device *dev;
-	enum mc13xxx_id ictype;
-
-	struct mutex lock;
-
-	int (*read_dev)(struct mc13xxx *, unsigned int, u32 *);
-	int (*write_dev)(struct mc13xxx *, unsigned int, u32);
-
-	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
-	void *irqdata[MC13XXX_NUM_IRQ];
-
-	int adcflags;
-};
-
 #define MC13XXX_IRQSTAT0	0
 #define MC13XXX_IRQSTAT0_ADCDONEI	(1 << 0)
 #define MC13XXX_IRQSTAT0_ADCBISDONEI	(1 << 1)
@@ -170,67 +146,6 @@ void mc13xxx_unlock(struct mc13xxx *mc13xxx)
 }
 EXPORT_SYMBOL(mc13xxx_unlock);
 
-#define MC13XXX_REGOFFSET_SHIFT 25
-static int mc13xxx_spi_reg_read(struct mc13xxx *mc13xxx,
-				unsigned int offset, u32 *val)
-{
-	struct spi_transfer t;
-	struct spi_message m;
-	int ret;
-
-	*val = offset << MC13XXX_REGOFFSET_SHIFT;
-
-	memset(&t, 0, sizeof(t));
-
-	t.tx_buf = val;
-	t.rx_buf = val;
-	t.len = sizeof(u32);
-
-	spi_message_init(&m);
-	spi_message_add_tail(&t, &m);
-
-	ret = spi_sync(mc13xxx->spidev, &m);
-
-	/* error in message.status implies error return from spi_sync */
-	BUG_ON(!ret && m.status);
-
-	if (ret)
-		return ret;
-
-	*val &= 0xffffff;
-
-	return 0;
-}
-
-static int mc13xxx_spi_reg_write(struct mc13xxx *mc13xxx, unsigned int offset,
-		u32 val)
-{
-	u32 buf;
-	struct spi_transfer t;
-	struct spi_message m;
-	int ret;
-
-	buf = 1 << 31 | offset << MC13XXX_REGOFFSET_SHIFT | val;
-
-	memset(&t, 0, sizeof(t));
-
-	t.tx_buf = &buf;
-	t.rx_buf = &buf;
-	t.len = sizeof(u32);
-
-	spi_message_init(&m);
-	spi_message_add_tail(&t, &m);
-
-	ret = spi_sync(mc13xxx->spidev, &m);
-
-	BUG_ON(!ret && m.status);
-
-	if (ret)
-		return ret;
-
-	return 0;
-}
-
 int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val)
 {
 	int ret;
@@ -704,46 +619,6 @@ static int mc13xxx_add_subdevice(struct mc13xxx *mc13xxx, const char *format)
 	return mc13xxx_add_subdevice_pdata(mc13xxx, format, NULL, 0);
 }
 
-static int mc13xxx_probe(struct spi_device *spi)
-{
-	struct mc13xxx *mc13xxx;
-	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
-	int ret;
-
-	if (!pdata) {
-		dev_err(&spi->dev, "invalid platform data\n");
-		return -EINVAL;
-	}
-
-	mc13xxx = kzalloc(sizeof(*mc13xxx), GFP_KERNEL);
-	if (!mc13xxx)
-		return -ENOMEM;
-
-	dev_set_drvdata(&spi->dev, mc13xxx);
-	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
-	spi->bits_per_word = 32;
-	spi_setup(spi);
-
-	mc13xxx->dev = &spi->dev;
-	mc13xxx->spidev = spi;
-	mc13xxx->read_dev = mc13xxx_spi_reg_read;
-	mc13xxx->write_dev = mc13xxx_spi_reg_write;
-
-	ret = mc13xxx_common_init(mc13xxx, pdata, spi->irq);
-
-	if (ret) {
-		dev_set_drvdata(&spi->dev, NULL);
-	} else {
-		const struct spi_device_id *devid =
-			spi_get_device_id(mc13xxx->spidev);
-		if (!devid || devid->driver_data != mc13xxx->ictype)
-			dev_warn(mc13xxx->dev,
-				"device id doesn't match auto detection!\n");
-	}
-
-	return ret;
-}
-
 int mc13xxx_common_init(struct mc13xxx *mc13xxx,
 		struct mc13xxx_platform_data *pdata, int irq)
 {
@@ -805,55 +680,6 @@ err_revision:
 }
 EXPORT_SYMBOL_GPL(mc13xxx_common_init);
 
-static int __devexit mc13xxx_remove(struct spi_device *spi)
-{
-	struct mc13xxx *mc13xxx = dev_get_drvdata(&spi->dev);
-
-	free_irq(mc13xxx->spidev->irq, mc13xxx);
-
-	mfd_remove_devices(&spi->dev);
-
-	kfree(mc13xxx);
-
-	return 0;
-}
-
-static const struct spi_device_id mc13xxx_device_id[] = {
-	{
-		.name = "mc13783",
-		.driver_data = MC13XXX_ID_MC13783,
-	}, {
-		.name = "mc13892",
-		.driver_data = MC13XXX_ID_MC13892,
-	}, {
-		/* sentinel */
-	}
-};
-MODULE_DEVICE_TABLE(spi, mc13xxx_device_id);
-
-static struct spi_driver mc13xxx_driver = {
-	.id_table = mc13xxx_device_id,
-	.driver = {
-		.name = "mc13xxx",
-		.bus = &spi_bus_type,
-		.owner = THIS_MODULE,
-	},
-	.probe = mc13xxx_probe,
-	.remove = __devexit_p(mc13xxx_remove),
-};
-
-static int __init mc13xxx_init(void)
-{
-	return spi_register_driver(&mc13xxx_driver);
-}
-subsys_initcall(mc13xxx_init);
-
-static void __exit mc13xxx_exit(void)
-{
-	spi_unregister_driver(&mc13xxx_driver);
-}
-module_exit(mc13xxx_exit);
-
 MODULE_DESCRIPTION("Core driver for Freescale MC13XXX PMIC");
 MODULE_AUTHOR("Uwe Kleine-Koenig <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
new file mode 100644
index 0000000..8958b44
--- /dev/null
+++ b/drivers/mfd/mc13xxx-spi.c
@@ -0,0 +1,173 @@
+/*
+ * Copyright 2009-2010 Pengutronix
+ * Uwe Kleine-Koenig <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
+ *
+ * loosely based on an earlier driver that has
+ * Copyright 2009 Pengutronix, Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
+ *
+ * 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/slab.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mutex.h>
+#include <linux/interrupt.h>
+#include <linux/spi/spi.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/mc13xxx.h>
+
+#define MC13XXX_REGOFFSET_SHIFT 25
+static int mc13xxx_spi_reg_read(struct mc13xxx *mc13xxx,
+				unsigned int offset, u32 *val)
+{
+	struct spi_transfer t;
+	struct spi_message m;
+	int ret;
+
+	*val = offset << MC13XXX_REGOFFSET_SHIFT;
+
+	memset(&t, 0, sizeof(t));
+
+	t.tx_buf = val;
+	t.rx_buf = val;
+	t.len = sizeof(u32);
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+
+	ret = spi_sync(mc13xxx->spidev, &m);
+
+	/* error in message.status implies error return from spi_sync */
+	BUG_ON(!ret && m.status);
+
+	if (ret)
+		return ret;
+
+	*val &= 0xffffff;
+
+	return 0;
+}
+
+static int mc13xxx_spi_reg_write(struct mc13xxx *mc13xxx, unsigned int offset,
+		u32 val)
+{
+	u32 buf;
+	struct spi_transfer t;
+	struct spi_message m;
+	int ret;
+
+	buf = 1 << 31 | offset << MC13XXX_REGOFFSET_SHIFT | val;
+
+	memset(&t, 0, sizeof(t));
+
+	t.tx_buf = &buf;
+	t.rx_buf = &buf;
+	t.len = sizeof(u32);
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+
+	ret = spi_sync(mc13xxx->spidev, &m);
+
+	BUG_ON(!ret && m.status);
+
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mc13xxx_spi_probe(struct spi_device *spi)
+{
+	struct mc13xxx *mc13xxx;
+	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
+	int ret;
+
+	if (!pdata) {
+		dev_err(&spi->dev, "invalid platform data\n");
+		return -EINVAL;
+	}
+
+	mc13xxx = kzalloc(sizeof(*mc13xxx), GFP_KERNEL);
+	if (!mc13xxx)
+		return -ENOMEM;
+
+	dev_set_drvdata(&spi->dev, mc13xxx);
+	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
+	spi->bits_per_word = 32;
+	spi_setup(spi);
+
+	mc13xxx->dev = &spi->dev;
+	mc13xxx->spidev = spi;
+	mc13xxx->read_dev = mc13xxx_spi_reg_read;
+	mc13xxx->write_dev = mc13xxx_spi_reg_write;
+
+	ret = mc13xxx_common_init(mc13xxx, pdata, spi->irq);
+
+	if (ret) {
+		dev_set_drvdata(&spi->dev, NULL);
+	} else {
+		const struct spi_device_id *devid =
+			spi_get_device_id(mc13xxx->spidev);
+		if (!devid || devid->driver_data != mc13xxx->ictype)
+			dev_warn(mc13xxx->dev,
+				"device id doesn't match auto detection!\n");
+	}
+
+	return ret;
+}
+
+static int __devexit mc13xxx_spi_remove(struct spi_device *spi)
+{
+	struct mc13xxx *mc13xxx = dev_get_drvdata(&spi->dev);
+
+	free_irq(mc13xxx->spidev->irq, mc13xxx);
+
+	mfd_remove_devices(&spi->dev);
+
+	kfree(mc13xxx);
+
+	return 0;
+}
+
+static const struct spi_device_id mc13xxx_device_id[] = {
+	{
+		.name = "mc13783",
+		.driver_data = MC13XXX_ID_MC13783,
+	}, {
+		.name = "mc13892",
+		.driver_data = MC13XXX_ID_MC13892,
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct spi_driver mc13xxx_spi_driver = {
+	.id_table = mc13xxx_device_id,
+	.driver = {
+		.name = "mc13xxx",
+		.bus = &spi_bus_type,
+		.owner = THIS_MODULE,
+	},
+	.probe = mc13xxx_spi_probe,
+	.remove = __devexit_p(mc13xxx_spi_remove),
+};
+
+static int __init mc13xxx_spi_init(void)
+{
+	return spi_register_driver(&mc13xxx_spi_driver);
+}
+subsys_initcall(mc13xxx_spi_init);
+
+static void __exit mc13xxx_spi_exit(void)
+{
+	spi_unregister_driver(&mc13xxx_spi_driver);
+}
+module_exit(mc13xxx_spi_exit);
+
+MODULE_DESCRIPTION("SPI driver for Freescale MC13XXX PMIC");
+MODULE_AUTHOR("Uwe Kleine-Koenig <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
index 3e38f72..abf77d3 100644
--- a/include/linux/mfd/mc13xxx.h
+++ b/include/linux/mfd/mc13xxx.h
@@ -69,6 +69,29 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx,
 
 #define MC13XXX_NUM_IRQ		46
 
+enum mc13xxx_id {
+	MC13XXX_ID_MC13783,
+	MC13XXX_ID_MC13892,
+	MC13XXX_ID_INVALID,
+};
+
+struct mc13xxx {
+	struct spi_device *spidev;
+
+	struct device *dev;
+	enum mc13xxx_id ictype;
+
+	struct mutex lock;
+
+	int (*read_dev)(struct mc13xxx *, unsigned int, u32 *);
+	int (*write_dev)(struct mc13xxx *, unsigned int, u32);
+
+	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
+	void *irqdata[MC13XXX_NUM_IRQ];
+
+	int adcflags;
+};
+
 struct regulator_init_data;
 
 struct mc13xxx_regulator_init_data {
-- 
1.7.3.4

[PATCH 3/3] mfd: mc13xxx-core: Add i2c driver

[Marc Reilly]

Adds support for mc13xxx family ICs connected via i2c.

Signed-off-by: Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org>
---
 drivers/mfd/Kconfig         |    8 +++-
 drivers/mfd/Makefile        |    1 +
 drivers/mfd/mc13xxx-i2c.c   |  120 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/mc13xxx.h |    5 ++-
 4 files changed, 132 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mfd/mc13xxx-i2c.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6cf84c8..08afc84 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -515,7 +515,7 @@ config MFD_MC13783
 
 config MFD_MC13XXX
 	tristate "Support Freescale MC13783 or MC13892"
-	depends on SPI_MASTER
+	depends on SPI_MASTER || I2C
 	select MFD_CORE
 	select MFD_MC13783
 	help
@@ -538,6 +538,12 @@ config MFD_MC13XXX_SPI
 	help
 	  Select this if your MC13xxx is connected via an SPI bus.
 
+config MFD_MC13XXX_I2C
+	tristate "I2C interface"
+	depends on I2C
+	help
+	  Select this if your MC13xxx is connected via an I2C bus.
+
 endchoice
 
 config ABX500_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 3856820..5db8b76 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_TWL6040_CORE)	+= twl6040-core.o twl6040-irq.o
 
 obj-$(CONFIG_MFD_MC13XXX)	+= mc13xxx-core.o
 obj-$(CONFIG_MFD_MC13XXX_SPI)	+= mc13xxx-spi.o
+obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
 
 obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
 
diff --git a/drivers/mfd/mc13xxx-i2c.c b/drivers/mfd/mc13xxx-i2c.c
new file mode 100644
index 0000000..9965481
--- /dev/null
+++ b/drivers/mfd/mc13xxx-i2c.c
@@ -0,0 +1,120 @@
+/*
+ * Copyright 2009-2010 Creative Product Design
+ * Marc Reilly marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org
+ *
+ * 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/slab.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mutex.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/mc13xxx.h>
+#include <linux/i2c.h>
+
+static int mc13xxx_i2c_reg_read(struct mc13xxx *mc13xxx, unsigned int offset,
+		u32 *val)
+{
+	int ret;
+	unsigned char buf[3] = {0, 0, 0};
+
+	ret = i2c_smbus_read_i2c_block_data(mc13xxx->i2cclient,
+			offset, 3, buf);
+	*val = buf[0] << 16 | buf[1] << 8 | buf[2];
+
+	return ret == 3 ? 0 : ret;
+}
+
+static int mc13xxx_i2c_reg_write(struct mc13xxx *mc13xxx, unsigned int offset,
+		u32 val)
+{
+	int ret;
+	unsigned char buf[3];
+
+	buf[0] = (val >> 16) & 0xff;
+	buf[1] = (val >> 8) & 0xff;
+	buf[2] = val & 0xff;
+
+	ret = i2c_smbus_write_i2c_block_data(mc13xxx->i2cclient,
+			offset, 3, buf);
+
+	return ret;
+}
+
+static int mc13xxx_i2c_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	struct mc13xxx *mc13xxx;
+	struct mc13xxx_platform_data *pdata = dev_get_platdata(&client->dev);
+	int ret;
+
+	if (!pdata) {
+		dev_err(&client->dev, "invalid platform data\n");
+		return -EINVAL;
+	}
+
+	mc13xxx = kzalloc(sizeof(*mc13xxx), GFP_KERNEL);
+	if (!mc13xxx)
+		return -ENOMEM;
+
+	dev_set_drvdata(&client->dev, mc13xxx);
+	mc13xxx->dev = &client->dev;
+	mc13xxx->i2cclient = client;
+	mc13xxx->read_dev = mc13xxx_i2c_reg_read;
+	mc13xxx->write_dev = mc13xxx_i2c_reg_write;
+
+	ret = mc13xxx_common_init(mc13xxx, pdata, client->irq);
+
+	if (ret == 0 && (id->driver_data != mc13xxx->ictype))
+		dev_warn(mc13xxx->dev,
+				"device id doesn't match auto detection!\n");
+
+	return ret;
+}
+
+static int __devexit mc13xxx_i2c_remove(struct i2c_client *client)
+{
+	struct mc13xxx *mc13xxx = dev_get_drvdata(&client->dev);
+
+	free_irq(client->irq, mc13xxx);
+
+	mfd_remove_devices(&client->dev);
+
+	kfree(mc13xxx);
+
+	return 0;
+}
+
+static const struct i2c_device_id mc13xxx_i2c_idtable[] = {
+	{"mc13892", MC13XXX_ID_MC13892},
+	{ }
+};
+
+static struct i2c_driver mc13xxx_i2c_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "mc13xxx-i2c"
+	},
+	.id_table = mc13xxx_i2c_idtable,
+	.probe = mc13xxx_i2c_probe,
+	.remove = __devexit_p(mc13xxx_i2c_remove),
+};
+
+static int __init mc13xxx_i2c_init(void)
+{
+	return i2c_add_driver(&mc13xxx_i2c_driver);
+}
+subsys_initcall(mc13xxx_i2c_init);
+
+static void __exit mc13xxx_i2c_exit(void)
+{
+	i2c_del_driver(&mc13xxx_i2c_driver);
+}
+module_exit(mc13xxx_i2c_exit);
+
+MODULE_DESCRIPTION("i2c driver for Freescale MC13XXX PMIC");
+MODULE_AUTHOR("Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
index abf77d3..c17fc60 100644
--- a/include/linux/mfd/mc13xxx.h
+++ b/include/linux/mfd/mc13xxx.h
@@ -76,7 +76,10 @@ enum mc13xxx_id {
 };
 
 struct mc13xxx {
-	struct spi_device *spidev;
+	union {
+		struct spi_device *spidev;
+		struct i2c_client *i2cclient;
+	};
 
 	struct device *dev;
 	enum mc13xxx_id ictype;
-- 
1.7.3.4

Re: mc13xxx: add I2C support

[Shawn Guo]

Hi Marc,

On Thu, Jan 19, 2012 at 12:01:31PM +1100, Marc Reilly wrote:
> Hi,
> 
> This series (based on 3.2) adds I2C support for the mc13xxx family PMICs.
> The patches are similar, but not identical to my previous patches for this [1], the
> main difference being the way the config items are done in patch 2.
> 
The series looks nice.

> I don't have the hardware to retest on SPI hardware, I'd appreciate if someone could 
> double check it still works.
> 
I just gave it a spin on my imx51 babbage board where mc13892 is
connected on spi.  The spi access works fine as before, since the part
still gets recognized correctly.

BTW, I see the following compile error.

  CC      drivers/regulator/mc13892-regulator.o
In file included from include/linux/mfd/mc13892.h:12:0,
                 from drivers/regulator/mc13892-regulator.c:13:
include/linux/mfd/mc13xxx.h:87:15: error: field ‘lock’ has incomplete type

And I fixed it as below.

---8<----
diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
index c17fc60..4ca8439 100644
--- a/include/linux/mfd/mc13xxx.h
+++ b/include/linux/mfd/mc13xxx.h
@@ -10,6 +10,7 @@
 #define __LINUX_MFD_MC13XXX_H

 #include <linux/interrupt.h>
+#include <linux/mutex.h>

 struct mc13xxx;

---->8---

-- 
Regards,
Shawn

Re: [PATCH 1/3] mc13xxx-core: Prepare for separate spi and i2c backends.

[Shawn Guo]

Forgot to mention in my last reply that I recently added device tree
probe support for mc13xxx-core which has hit the mainline, so you may
need to rebase this series, or you will be asked by Samuel to anyway,
I guess :)

One nit on the subject of this patch: you may want to have prefix
"mfd: mc13xxx-core: ..." to align with the other two patches in the
series.

-- 
Regards,
Shawn

On Thu, Jan 19, 2012 at 12:01:32PM +1100, Marc Reilly wrote:
> This patch abstracts the bus specific operations from the driver core.
> Read and write handlers are introduced and generic initialization is
> consolidated into mc13xxx_comon_init. The irq member is removed because
> it is unused.
> 
> Signed-off-by: Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org>

Re: [PATCH 1/3] mc13xxx-core: Prepare for separate spi and i2c backends.

[Uwe Kleine-König]

Hello Marc,

I'm glad you respin the series. Some comments below.

On Thu, Jan 19, 2012 at 12:01:32PM +1100, Marc Reilly wrote:
> This patch abstracts the bus specific operations from the driver core.
> Read and write handlers are introduced and generic initialization is
> consolidated into mc13xxx_comon_init. The irq member is removed because
> it is unused.
> 
> Signed-off-by: Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org>
> ---
>  drivers/mfd/mc13xxx-core.c  |  162 +++++++++++++++++++++++++-----------------
>  include/linux/mfd/mc13xxx.h |    5 ++
>  2 files changed, 101 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index e9619ac..3c3079f 100644
> --- a/drivers/mfd/mc13xxx-core.c
> +++ b/drivers/mfd/mc13xxx-core.c
> @@ -19,10 +19,22 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/mc13xxx.h>
>  
> +enum mc13xxx_id {
> +	MC13XXX_ID_MC13783,
> +	MC13XXX_ID_MC13892,
> +	MC13XXX_ID_INVALID,
> +};
> +
>  struct mc13xxx {
>  	struct spi_device *spidev;
> +
> +	struct device *dev;
> +	enum mc13xxx_id ictype;
> +
>  	struct mutex lock;
> -	int irq;
> +
> +	int (*read_dev)(struct mc13xxx *, unsigned int, u32 *);
> +	int (*write_dev)(struct mc13xxx *, unsigned int, u32);
>  
>  	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
>  	void *irqdata[MC13XXX_NUM_IRQ];
> @@ -140,36 +152,32 @@ struct mc13xxx {
>  void mc13xxx_lock(struct mc13xxx *mc13xxx)
>  {
>  	if (!mutex_trylock(&mc13xxx->lock)) {
> -		dev_dbg(&mc13xxx->spidev->dev, "wait for %s from %pf\n",
> +		dev_dbg(mc13xxx->dev, "wait for %s from %pf\n",
>  				__func__, __builtin_return_address(0));
>  
>  		mutex_lock(&mc13xxx->lock);
>  	}
> -	dev_dbg(&mc13xxx->spidev->dev, "%s from %pf\n",
> +	dev_dbg(mc13xxx->dev, "%s from %pf\n",
>  			__func__, __builtin_return_address(0));
>  }
>  EXPORT_SYMBOL(mc13xxx_lock);
>  
>  void mc13xxx_unlock(struct mc13xxx *mc13xxx)
>  {
> -	dev_dbg(&mc13xxx->spidev->dev, "%s from %pf\n",
> +	dev_dbg(mc13xxx->dev, "%s from %pf\n",
>  			__func__, __builtin_return_address(0));
>  	mutex_unlock(&mc13xxx->lock);
>  }
>  EXPORT_SYMBOL(mc13xxx_unlock);
>  
>  #define MC13XXX_REGOFFSET_SHIFT 25
> -int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val)
> +static int mc13xxx_spi_reg_read(struct mc13xxx *mc13xxx,
> +				unsigned int offset, u32 *val)
>  {
>  	struct spi_transfer t;
>  	struct spi_message m;
>  	int ret;
>  
> -	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
> -
> -	if (offset > MC13XXX_NUMREGS)
> -		return -EINVAL;
> -
>  	*val = offset << MC13XXX_REGOFFSET_SHIFT;
>  
>  	memset(&t, 0, sizeof(t));
> @@ -191,26 +199,17 @@ int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val)
>  
>  	*val &= 0xffffff;
>  
> -	dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
> -
>  	return 0;
>  }
> -EXPORT_SYMBOL(mc13xxx_reg_read);
>  
> -int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val)
> +static int mc13xxx_spi_reg_write(struct mc13xxx *mc13xxx, unsigned int offset,
> +		u32 val)
>  {
>  	u32 buf;
>  	struct spi_transfer t;
>  	struct spi_message m;
>  	int ret;
>  
> -	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
> -
> -	dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] <- 0x%06x\n", offset, val);
> -
> -	if (offset > MC13XXX_NUMREGS || val > 0xffffff)
> -		return -EINVAL;
> -
>  	buf = 1 << 31 | offset << MC13XXX_REGOFFSET_SHIFT | val;
>  
>  	memset(&t, 0, sizeof(t));
> @@ -231,6 +230,34 @@ int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val)
>  
>  	return 0;
>  }
> +
> +int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val)
> +{
> +	int ret;
> +
> +	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
> +
> +	if (offset > MC13XXX_NUMREGS)
> +		return -EINVAL;
> +
> +	ret = mc13xxx->read_dev(mc13xxx, offset, val);
> +	dev_vdbg(mc13xxx->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(mc13xxx_reg_read);
> +
> +int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val)
> +{
> +	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
> +
> +	dev_vdbg(mc13xxx->dev, "[0x%02x] <- 0x%06x\n", offset, val);
> +
> +	if (offset > MC13XXX_NUMREGS || val > 0xffffff)
> +		return -EINVAL;
> +
> +	return mc13xxx->write_dev(mc13xxx, offset, val);
> +}
>  EXPORT_SYMBOL(mc13xxx_reg_write);
>  
>  int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset,
> @@ -435,7 +462,7 @@ static int mc13xxx_irq_handle(struct mc13xxx *mc13xxx,
>  			if (handled == IRQ_HANDLED)
>  				num_handled++;
>  		} else {
> -			dev_err(&mc13xxx->spidev->dev,
> +			dev_err(mc13xxx->dev,
>  					"BUG: irq %u but no handler\n",
>  					baseirq + irq);
>  
> @@ -471,25 +498,23 @@ static irqreturn_t mc13xxx_irq_thread(int irq, void *data)
>  	return IRQ_RETVAL(handled);
>  }
>  
> -enum mc13xxx_id {
> -	MC13XXX_ID_MC13783,
> -	MC13XXX_ID_MC13892,
> -	MC13XXX_ID_INVALID,
> -};
> -
>  static const char *mc13xxx_chipname[] = {
>  	[MC13XXX_ID_MC13783] = "mc13783",
>  	[MC13XXX_ID_MC13892] = "mc13892",
>  };
>  
>  #define maskval(reg, mask)	(((reg) & (mask)) >> __ffs(mask))
> -static int mc13xxx_identify(struct mc13xxx *mc13xxx, enum mc13xxx_id *id)
> +static int mc13xxx_identify(struct mc13xxx *mc13xxx)
>  {
>  	u32 icid;
>  	u32 revision;
> -	const char *name;
>  	int ret;
>  
> +	/*
> +	 * Get the generation ID from register 46, as apparently some older
> +	 * IC revisions only have this info at this location. Newer ICs seem to
> +	 * have both.
> +	 */
>  	ret = mc13xxx_reg_read(mc13xxx, 46, &icid);
>  	if (ret)
>  		return ret;
> @@ -498,26 +523,23 @@ static int mc13xxx_identify(struct mc13xxx *mc13xxx, enum mc13xxx_id *id)
>  
>  	switch (icid) {
>  	case 2:
> -		*id = MC13XXX_ID_MC13783;
> -		name = "mc13783";
> +		mc13xxx->ictype = MC13XXX_ID_MC13783;
>  		break;
>  	case 7:
> -		*id = MC13XXX_ID_MC13892;
> -		name = "mc13892";
> +		mc13xxx->ictype = MC13XXX_ID_MC13892;
>  		break;
>  	default:
> -		*id = MC13XXX_ID_INVALID;
> +		mc13xxx->ictype = MC13XXX_ID_INVALID;
>  		break;
>  	}
>  
> -	if (*id == MC13XXX_ID_MC13783 || *id == MC13XXX_ID_MC13892) {
> +	if (mc13xxx->ictype == MC13XXX_ID_MC13783 ||
> +			mc13xxx->ictype == MC13XXX_ID_MC13892) {
>  		ret = mc13xxx_reg_read(mc13xxx, MC13XXX_REVISION, &revision);
> -		if (ret)
> -			return ret;
>  
> -		dev_info(&mc13xxx->spidev->dev, "%s: rev: %d.%d, "
> +		dev_info(mc13xxx->dev, "%s: rev: %d.%d, "
>  				"fin: %d, fab: %d, icid: %d/%d\n",
> -				mc13xxx_chipname[*id],
> +				mc13xxx_chipname[mc13xxx->ictype],
>  				maskval(revision, MC13XXX_REVISION_REVFULL),
>  				maskval(revision, MC13XXX_REVISION_REVMETAL),
>  				maskval(revision, MC13XXX_REVISION_FIN),
> @@ -526,32 +548,18 @@ static int mc13xxx_identify(struct mc13xxx *mc13xxx, enum mc13xxx_id *id)
>  				maskval(revision, MC13XXX_REVISION_ICIDCODE));
>  	}
>  
> -	if (*id != MC13XXX_ID_INVALID) {
> -		const struct spi_device_id *devid =
> -			spi_get_device_id(mc13xxx->spidev);
> -		if (!devid || devid->driver_data != *id)
> -			dev_warn(&mc13xxx->spidev->dev, "device id doesn't "
> -					"match auto detection!\n");
> -	}
> -
> -	return 0;
> +	return (mc13xxx->ictype == MC13XXX_ID_INVALID) ? -ENODEV : 0;
>  }
>  
>  static const char *mc13xxx_get_chipname(struct mc13xxx *mc13xxx)
>  {
> -	const struct spi_device_id *devid =
> -		spi_get_device_id(mc13xxx->spidev);
> -
> -	if (!devid)
> -		return NULL;
> -
> -	return mc13xxx_chipname[devid->driver_data];
> +	return mc13xxx_chipname[mc13xxx->ictype];
>  }
>  
>  int mc13xxx_get_flags(struct mc13xxx *mc13xxx)
>  {
>  	struct mc13xxx_platform_data *pdata =
> -		dev_get_platdata(&mc13xxx->spidev->dev);
> +		dev_get_platdata(mc13xxx->dev);
>  
>  	return pdata->flags;
>  }
> @@ -588,7 +596,7 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
>  	};
>  	init_completion(&adcdone_data.done);
>  
> -	dev_dbg(&mc13xxx->spidev->dev, "%s\n", __func__);
> +	dev_dbg(mc13xxx->dev, "%s\n", __func__);
>  
>  	mc13xxx_lock(mc13xxx);
>  
> @@ -630,7 +638,7 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode,
>  		return -EINVAL;
>  	}
>  
> -	dev_dbg(&mc13xxx->spidev->dev, "%s: request irq\n", __func__);
> +	dev_dbg(mc13xxx->dev, "%s: request irq\n", __func__);
>  	mc13xxx_irq_request(mc13xxx, MC13XXX_IRQ_ADCDONE,
>  			mc13xxx_handler_adcdone, __func__, &adcdone_data);
>  	mc13xxx_irq_ack(mc13xxx, MC13XXX_IRQ_ADCDONE);
> @@ -688,7 +696,7 @@ static int mc13xxx_add_subdevice_pdata(struct mc13xxx *mc13xxx,
>  	if (!cell.name)
>  		return -ENOMEM;
>  
> -	return mfd_add_devices(&mc13xxx->spidev->dev, -1, &cell, 1, NULL, 0);
> +	return mfd_add_devices(mc13xxx->dev, -1, &cell, 1, NULL, 0);
>  }
>  
>  static int mc13xxx_add_subdevice(struct mc13xxx *mc13xxx, const char *format)
> @@ -700,7 +708,6 @@ static int mc13xxx_probe(struct spi_device *spi)
>  {
>  	struct mc13xxx *mc13xxx;
>  	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
> -	enum mc13xxx_id id;
>  	int ret;
>  
>  	if (!pdata) {
> @@ -717,13 +724,36 @@ static int mc13xxx_probe(struct spi_device *spi)
>  	spi->bits_per_word = 32;
>  	spi_setup(spi);
>  
> +	mc13xxx->dev = &spi->dev;
>  	mc13xxx->spidev = spi;
> +	mc13xxx->read_dev = mc13xxx_spi_reg_read;
> +	mc13xxx->write_dev = mc13xxx_spi_reg_write;
> +
> +	ret = mc13xxx_common_init(mc13xxx, pdata, spi->irq);
small nitpick: IMHO declaring mc13xxx_common_init globally isn't needed.
For example drivers/mfd/mc13xxx.h would be enough. And this only in
patch 2 if you rearrage it to be defined before mc13xxx_probe.
EXPORT_SYMBOL can then come in patch 2, too.

> +
> +	if (ret) {
> +		dev_set_drvdata(&spi->dev, NULL);
> +	} else {
> +		const struct spi_device_id *devid =
> +			spi_get_device_id(mc13xxx->spidev);
> +		if (!devid || devid->driver_data != mc13xxx->ictype)
> +			dev_warn(mc13xxx->dev,
> +				"device id doesn't match auto detection!\n");
> +	}
> +
> +	return ret;
> +}
> +
> +int mc13xxx_common_init(struct mc13xxx *mc13xxx,
> +		struct mc13xxx_platform_data *pdata, int irq)
> +{
> +	int ret;
>  
>  	mutex_init(&mc13xxx->lock);
>  	mc13xxx_lock(mc13xxx);
>  
> -	ret = mc13xxx_identify(mc13xxx, &id);
> -	if (ret || id == MC13XXX_ID_INVALID)
> +	ret = mc13xxx_identify(mc13xxx);
> +	if (ret)
>  		goto err_revision;
>  
>  	/* mask all irqs */
> @@ -735,14 +765,13 @@ static int mc13xxx_probe(struct spi_device *spi)
>  	if (ret)
>  		goto err_mask;
>  
> -	ret = request_threaded_irq(spi->irq, NULL, mc13xxx_irq_thread,
> +	ret = request_threaded_irq(irq, NULL, mc13xxx_irq_thread,
>  			IRQF_ONESHOT | IRQF_TRIGGER_HIGH, "mc13xxx", mc13xxx);
>  
>  	if (ret) {
>  err_mask:
>  err_revision:
> -		mc13xxx_unlock(mc13xxx);
> -		dev_set_drvdata(&spi->dev, NULL);
> +		mutex_unlock(&mc13xxx->lock);
What is the reason to switch back to an explicit mutex_unlock?
(My guess is a wrong conflict resolution during rebase in the presence
of commit e1b88eb0e08335d2f6c.)

Other than that I'm happy with the patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/3] mfd: mc13xxx-core: Move spi specific code into separate module.

[Uwe Kleine-König]

Hello,

On Thu, Jan 19, 2012 at 12:01:33PM +1100, Marc Reilly wrote:
> All spi specific code is moved into a new module. The mc13xxx
> struct moves to the include file by necessity.
> 
> A new config choice selects the bus type with SPI as the first item
> (default selection) to remain compatible with existing configs.
> 
> Signed-off-by: Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org>
> ---
>  drivers/mfd/Kconfig         |   23 +++++-
>  drivers/mfd/Makefile        |    1 +
>  drivers/mfd/mc13xxx-core.c  |  174 -------------------------------------------
>  drivers/mfd/mc13xxx-spi.c   |  173 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/mc13xxx.h |   23 ++++++
>  5 files changed, 216 insertions(+), 178 deletions(-)
>  create mode 100644 drivers/mfd/mc13xxx-spi.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index f1391c2..6cf84c8 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -514,17 +514,32 @@ config MFD_MC13783
>  	tristate
>  
>  config MFD_MC13XXX
> -	tristate "Support Freescale MC13783 and MC13892"
> +	tristate "Support Freescale MC13783 or MC13892"
>  	depends on SPI_MASTER
>  	select MFD_CORE
>  	select MFD_MC13783
>  	help
> -	  Support for the Freescale (Atlas) PMIC and audio CODECs
> -	  MC13783 and MC13892.
> -	  This driver provides common support for accessing  the device,
> +	  Enable support for the Freescale MC13783 and MC13892 PMICs.
> +	  This driver provides common support for accessing the device,
>  	  additional drivers must be enabled in order to use the
>  	  functionality of the device.
>  
> +choice
> +	tristate "MC13XXX Bus interface type"
s/tristate/prompt/ and I would prefer MC13xxx but not care much.

> +	depends on MFD_MC13XXX
> +	default MFD_MC13XXX_SPI
> +	help
> +	  The MC13XXX family can be connected over an SPI or I2C bus.
> +	  Select the appropriate option for your hardware configuration.
> +
> +config MFD_MC13XXX_SPI
> +	tristate "SPI interface"
> +	depends on SPI_MASTER
> +	help
> +	  Select this if your MC13xxx is connected via an SPI bus.
> +
> +endchoice
> +
Hmm, you cannot have both, spi and i2c support?

What do you think about:

menuconfig MFD_MC13XXX
	...

if MFD_MC13XXX

config MFD_MC13XXX_SPI
	bool "MC13xxx spi bus interface" if SPI_MASTER && I2C
	default SPI_MASTER
	...

config MFD_MC13XXX_I2C
	bool "MC13xxx i2c bus interface" if SPI_MASTER && I2C
	default I2C
	...

endif # MFD_MC13XXX

This way the bus type is even selected automatically when only one of
spi and i2c support is enabled. (I havn't tested that though.)

	
>  config ABX500_CORE
>  	bool "ST-Ericsson ABX500 Mixed Signal Circuit register functions"
>  	default y if ARCH_U300 || ARCH_U8500
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index b2292eb..3856820 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_TWL6030_PWM)	+= twl6030-pwm.o
>  obj-$(CONFIG_TWL6040_CORE)	+= twl6040-core.o twl6040-irq.o
>  
>  obj-$(CONFIG_MFD_MC13XXX)	+= mc13xxx-core.o
> +obj-$(CONFIG_MFD_MC13XXX_SPI)	+= mc13xxx-spi.o
>  
>  obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
>  
> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index 3c3079f..53732c0 100644
> --- a/drivers/mfd/mc13xxx-core.c
> +++ b/drivers/mfd/mc13xxx-core.c
> @@ -15,33 +15,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/mutex.h>
>  #include <linux/interrupt.h>
> -#include <linux/spi/spi.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/mc13xxx.h>
>  
> -enum mc13xxx_id {
> -	MC13XXX_ID_MC13783,
> -	MC13XXX_ID_MC13892,
> -	MC13XXX_ID_INVALID,
> -};
> -
> -struct mc13xxx {
> -	struct spi_device *spidev;
> -
> -	struct device *dev;
> -	enum mc13xxx_id ictype;
> -
> -	struct mutex lock;
> -
> -	int (*read_dev)(struct mc13xxx *, unsigned int, u32 *);
> -	int (*write_dev)(struct mc13xxx *, unsigned int, u32);
> -
> -	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
> -	void *irqdata[MC13XXX_NUM_IRQ];
> -
> -	int adcflags;
> -};
> -
>  #define MC13XXX_IRQSTAT0	0
>  #define MC13XXX_IRQSTAT0_ADCDONEI	(1 << 0)
>  #define MC13XXX_IRQSTAT0_ADCBISDONEI	(1 << 1)
> @@ -170,67 +146,6 @@ void mc13xxx_unlock(struct mc13xxx *mc13xxx)
>  }
>  EXPORT_SYMBOL(mc13xxx_unlock);
>  
> -#define MC13XXX_REGOFFSET_SHIFT 25
> -static int mc13xxx_spi_reg_read(struct mc13xxx *mc13xxx,
> -				unsigned int offset, u32 *val)
> -{
> -	struct spi_transfer t;
> -	struct spi_message m;
> -	int ret;
> -
> -	*val = offset << MC13XXX_REGOFFSET_SHIFT;
> -
> -	memset(&t, 0, sizeof(t));
> -
> -	t.tx_buf = val;
> -	t.rx_buf = val;
> -	t.len = sizeof(u32);
> -
> -	spi_message_init(&m);
> -	spi_message_add_tail(&t, &m);
> -
> -	ret = spi_sync(mc13xxx->spidev, &m);
> -
> -	/* error in message.status implies error return from spi_sync */
> -	BUG_ON(!ret && m.status);
> -
> -	if (ret)
> -		return ret;
> -
> -	*val &= 0xffffff;
> -
> -	return 0;
> -}
> -
> -static int mc13xxx_spi_reg_write(struct mc13xxx *mc13xxx, unsigned int offset,
> -		u32 val)
> -{
> -	u32 buf;
> -	struct spi_transfer t;
> -	struct spi_message m;
> -	int ret;
> -
> -	buf = 1 << 31 | offset << MC13XXX_REGOFFSET_SHIFT | val;
> -
> -	memset(&t, 0, sizeof(t));
> -
> -	t.tx_buf = &buf;
> -	t.rx_buf = &buf;
> -	t.len = sizeof(u32);
> -
> -	spi_message_init(&m);
> -	spi_message_add_tail(&t, &m);
> -
> -	ret = spi_sync(mc13xxx->spidev, &m);
> -
> -	BUG_ON(!ret && m.status);
> -
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> -}
> -
>  int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val)
>  {
>  	int ret;
> @@ -704,46 +619,6 @@ static int mc13xxx_add_subdevice(struct mc13xxx *mc13xxx, const char *format)
>  	return mc13xxx_add_subdevice_pdata(mc13xxx, format, NULL, 0);
>  }
>  
> -static int mc13xxx_probe(struct spi_device *spi)
> -{
> -	struct mc13xxx *mc13xxx;
> -	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
> -	int ret;
> -
> -	if (!pdata) {
> -		dev_err(&spi->dev, "invalid platform data\n");
> -		return -EINVAL;
> -	}
> -
> -	mc13xxx = kzalloc(sizeof(*mc13xxx), GFP_KERNEL);
> -	if (!mc13xxx)
> -		return -ENOMEM;
> -
> -	dev_set_drvdata(&spi->dev, mc13xxx);
> -	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
> -	spi->bits_per_word = 32;
> -	spi_setup(spi);
> -
> -	mc13xxx->dev = &spi->dev;
> -	mc13xxx->spidev = spi;
> -	mc13xxx->read_dev = mc13xxx_spi_reg_read;
> -	mc13xxx->write_dev = mc13xxx_spi_reg_write;
> -
> -	ret = mc13xxx_common_init(mc13xxx, pdata, spi->irq);
> -
> -	if (ret) {
> -		dev_set_drvdata(&spi->dev, NULL);
> -	} else {
> -		const struct spi_device_id *devid =
> -			spi_get_device_id(mc13xxx->spidev);
> -		if (!devid || devid->driver_data != mc13xxx->ictype)
> -			dev_warn(mc13xxx->dev,
> -				"device id doesn't match auto detection!\n");
> -	}
> -
> -	return ret;
> -}
> -
>  int mc13xxx_common_init(struct mc13xxx *mc13xxx,
>  		struct mc13xxx_platform_data *pdata, int irq)
>  {
> @@ -805,55 +680,6 @@ err_revision:
>  }
>  EXPORT_SYMBOL_GPL(mc13xxx_common_init);
>  
> -static int __devexit mc13xxx_remove(struct spi_device *spi)
> -{
> -	struct mc13xxx *mc13xxx = dev_get_drvdata(&spi->dev);
> -
> -	free_irq(mc13xxx->spidev->irq, mc13xxx);
> -
> -	mfd_remove_devices(&spi->dev);
> -
> -	kfree(mc13xxx);
> -
> -	return 0;
> -}
> -
> -static const struct spi_device_id mc13xxx_device_id[] = {
> -	{
> -		.name = "mc13783",
> -		.driver_data = MC13XXX_ID_MC13783,
> -	}, {
> -		.name = "mc13892",
> -		.driver_data = MC13XXX_ID_MC13892,
> -	}, {
> -		/* sentinel */
> -	}
> -};
> -MODULE_DEVICE_TABLE(spi, mc13xxx_device_id);
> -
> -static struct spi_driver mc13xxx_driver = {
> -	.id_table = mc13xxx_device_id,
> -	.driver = {
> -		.name = "mc13xxx",
> -		.bus = &spi_bus_type,
> -		.owner = THIS_MODULE,
> -	},
> -	.probe = mc13xxx_probe,
> -	.remove = __devexit_p(mc13xxx_remove),
> -};
> -
> -static int __init mc13xxx_init(void)
> -{
> -	return spi_register_driver(&mc13xxx_driver);
> -}
> -subsys_initcall(mc13xxx_init);
> -
> -static void __exit mc13xxx_exit(void)
> -{
> -	spi_unregister_driver(&mc13xxx_driver);
> -}
> -module_exit(mc13xxx_exit);
> -
>  MODULE_DESCRIPTION("Core driver for Freescale MC13XXX PMIC");
>  MODULE_AUTHOR("Uwe Kleine-Koenig <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
> new file mode 100644
> index 0000000..8958b44
> --- /dev/null
> +++ b/drivers/mfd/mc13xxx-spi.c
> @@ -0,0 +1,173 @@
> +/*
> + * Copyright 2009-2010 Pengutronix
> + * Uwe Kleine-Koenig <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + *
> + * loosely based on an earlier driver that has
> + * Copyright 2009 Pengutronix, Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + *
> + * 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/slab.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +#include <linux/interrupt.h>
> +#include <linux/spi/spi.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/mc13xxx.h>
> +
> +#define MC13XXX_REGOFFSET_SHIFT 25
> +static int mc13xxx_spi_reg_read(struct mc13xxx *mc13xxx,
> +				unsigned int offset, u32 *val)
> +{
> +	struct spi_transfer t;
> +	struct spi_message m;
> +	int ret;
> +
> +	*val = offset << MC13XXX_REGOFFSET_SHIFT;
> +
> +	memset(&t, 0, sizeof(t));
> +
> +	t.tx_buf = val;
> +	t.rx_buf = val;
> +	t.len = sizeof(u32);
> +
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t, &m);
> +
> +	ret = spi_sync(mc13xxx->spidev, &m);
> +
> +	/* error in message.status implies error return from spi_sync */
> +	BUG_ON(!ret && m.status);
> +
> +	if (ret)
> +		return ret;
> +
> +	*val &= 0xffffff;
> +
> +	return 0;
> +}
> +
> +static int mc13xxx_spi_reg_write(struct mc13xxx *mc13xxx, unsigned int offset,
> +		u32 val)
> +{
> +	u32 buf;
> +	struct spi_transfer t;
> +	struct spi_message m;
> +	int ret;
> +
> +	buf = 1 << 31 | offset << MC13XXX_REGOFFSET_SHIFT | val;
> +
> +	memset(&t, 0, sizeof(t));
> +
> +	t.tx_buf = &buf;
> +	t.rx_buf = &buf;
> +	t.len = sizeof(u32);
> +
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t, &m);
> +
> +	ret = spi_sync(mc13xxx->spidev, &m);
> +
> +	BUG_ON(!ret && m.status);
> +
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int mc13xxx_spi_probe(struct spi_device *spi)
> +{
> +	struct mc13xxx *mc13xxx;
> +	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
> +	int ret;
> +
> +	if (!pdata) {
> +		dev_err(&spi->dev, "invalid platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	mc13xxx = kzalloc(sizeof(*mc13xxx), GFP_KERNEL);
> +	if (!mc13xxx)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&spi->dev, mc13xxx);
> +	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
> +	spi->bits_per_word = 32;
> +	spi_setup(spi);
> +
> +	mc13xxx->dev = &spi->dev;
> +	mc13xxx->spidev = spi;
> +	mc13xxx->read_dev = mc13xxx_spi_reg_read;
> +	mc13xxx->write_dev = mc13xxx_spi_reg_write;
> +
> +	ret = mc13xxx_common_init(mc13xxx, pdata, spi->irq);
> +
> +	if (ret) {
> +		dev_set_drvdata(&spi->dev, NULL);
> +	} else {
> +		const struct spi_device_id *devid =
> +			spi_get_device_id(mc13xxx->spidev);
> +		if (!devid || devid->driver_data != mc13xxx->ictype)
> +			dev_warn(mc13xxx->dev,
> +				"device id doesn't match auto detection!\n");
> +	}
> +
> +	return ret;
> +}
> +
> +static int __devexit mc13xxx_spi_remove(struct spi_device *spi)
> +{
> +	struct mc13xxx *mc13xxx = dev_get_drvdata(&spi->dev);
> +
> +	free_irq(mc13xxx->spidev->irq, mc13xxx);
> +
> +	mfd_remove_devices(&spi->dev);
> +
> +	kfree(mc13xxx);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id mc13xxx_device_id[] = {
> +	{
> +		.name = "mc13783",
> +		.driver_data = MC13XXX_ID_MC13783,
> +	}, {
> +		.name = "mc13892",
> +		.driver_data = MC13XXX_ID_MC13892,
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +
> +static struct spi_driver mc13xxx_spi_driver = {
> +	.id_table = mc13xxx_device_id,
> +	.driver = {
> +		.name = "mc13xxx",
> +		.bus = &spi_bus_type,
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = mc13xxx_spi_probe,
> +	.remove = __devexit_p(mc13xxx_spi_remove),
> +};
> +
> +static int __init mc13xxx_spi_init(void)
> +{
> +	return spi_register_driver(&mc13xxx_spi_driver);
> +}
> +subsys_initcall(mc13xxx_spi_init);
> +
> +static void __exit mc13xxx_spi_exit(void)
> +{
> +	spi_unregister_driver(&mc13xxx_spi_driver);
> +}
> +module_exit(mc13xxx_spi_exit);
> +
> +MODULE_DESCRIPTION("SPI driver for Freescale MC13XXX PMIC");
> +MODULE_AUTHOR("Uwe Kleine-Koenig <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
> index 3e38f72..abf77d3 100644
> --- a/include/linux/mfd/mc13xxx.h
> +++ b/include/linux/mfd/mc13xxx.h
> @@ -69,6 +69,29 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx,
>  
>  #define MC13XXX_NUM_IRQ		46
>  
> +enum mc13xxx_id {
> +	MC13XXX_ID_MC13783,
> +	MC13XXX_ID_MC13892,
> +	MC13XXX_ID_INVALID,
> +};
> +
> +struct mc13xxx {
> +	struct spi_device *spidev;
> +
> +	struct device *dev;
> +	enum mc13xxx_id ictype;
> +
> +	struct mutex lock;
> +
> +	int (*read_dev)(struct mc13xxx *, unsigned int, u32 *);
> +	int (*write_dev)(struct mc13xxx *, unsigned int, u32);
> +
> +	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
> +	void *irqdata[MC13XXX_NUM_IRQ];
> +
> +	int adcflags;
> +};
> +
this should go to drivers/mfd/mc13xxx.h, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: mc13xxx: add I2C support

[Arnaud Patard]

Marc Reilly <marc@cpdesign.com.au> writes:

> Hi,

Hi,

>
> This series (based on 3.2) adds I2C support for the mc13xxx family PMICs.
> The patches are similar, but not identical to my previous patches for this [1], the
> main difference being the way the config items are done in patch 2.
>
> I don't have the hardware to retest on SPI hardware, I'd appreciate if someone could 
> double check it still works.
>
> I've also got a stack of patches for the mc13xxx to follow, but should get this in
> first.

I've never looked at regmap deeply but can't it be done with regmap or is it
just a bad idea ?

Arnaud

Re: mc13xxx: add I2C support

[Mark Brown]

On Thu, Jan 19, 2012 at 12:12:31PM +0100, Arnaud Patard wrote:

> I've never looked at regmap deeply but can't it be done with regmap or is it
> just a bad idea ?

Glancing quickly at the existing code it should map on reasonably well,
though a new format definition may be required for the 25 bit shift that
would be trivial.

Re: [PATCH 2/3] mfd: mc13xxx-core: Move spi specific code into separate module.

[Grant Likely]

On Thu, Jan 19, 2012 at 09:08:43AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Jan 19, 2012 at 12:01:33PM +1100, Marc Reilly wrote:
> > All spi specific code is moved into a new module. The mc13xxx
> > struct moves to the include file by necessity.
> > 
> > A new config choice selects the bus type with SPI as the first item
> > (default selection) to remain compatible with existing configs.
> > 
> > Signed-off-by: Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org>
> > ---
> > +choice
> > +	tristate "MC13XXX Bus interface type"
> > +	depends on MFD_MC13XXX
> > +	default MFD_MC13XXX_SPI
> > +	help
> > +	  The MC13XXX family can be connected over an SPI or I2C bus.
> > +	  Select the appropriate option for your hardware configuration.
> > +
> > +config MFD_MC13XXX_SPI
> > +	tristate "SPI interface"
> > +	depends on SPI_MASTER
> > +	help
> > +	  Select this if your MC13xxx is connected via an SPI bus.
> > +
> > +endchoice
> > +
> Hmm, you cannot have both, spi and i2c support?

+1 on Uwe's comment.  We're going multiplatform.  I2C and SPI support
should not be mutually exclusive.

g.

Re: mc13xxx: add I2C support

[Marc Reilly]

Hi,

Thankyou all for your feedback and comments. I'll use them for a V2 but 
Samuel, I'd like to know if you'd like me to base them on a specific branch 
before I do.

Shawn, thanks for testing!

On Thursday, January 19, 2012 10:29:41 PM Mark Brown wrote:
> On Thu, Jan 19, 2012 at 12:12:31PM +0100, Arnaud Patard wrote:
> > I've never looked at regmap deeply but can't it be done with regmap or is
> > it just a bad idea ?
> 
> Glancing quickly at the existing code it should map on reasonably well,
> though a new format definition may be required for the 25 bit shift that
> would be trivial.

I'm sadly unfamiliar with regmap, is it a far superior solution? does it need 
to used now? (ie. I'm relucant to totally rework this now. Please convince me 
I need to if required.)

Cheers,
Marc
------------------------------------------------------------------------------
Keep Your Developer Skills Current with LearnDevNow!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-d2d

Re: mc13xxx: add I2C support

[Arnaud Patard]

Marc Reilly <marc-DtE7ei5U7Kg0n/F98K4Iww@public.gmane.org> writes:

> Hi,

Hi,

>
> Thankyou all for your feedback and comments. I'll use them for a V2 but 
> Samuel, I'd like to know if you'd like me to base them on a specific branch 
> before I do.
>
> Shawn, thanks for testing!
>
> On Thursday, January 19, 2012 10:29:41 PM Mark Brown wrote:
>> On Thu, Jan 19, 2012 at 12:12:31PM +0100, Arnaud Patard wrote:
>> > I've never looked at regmap deeply but can't it be done with regmap or is
>> > it just a bad idea ?
>> 
>> Glancing quickly at the existing code it should map on reasonably well,
>> though a new format definition may be required for the 25 bit shift that
>> would be trivial.
>
> I'm sadly unfamiliar with regmap, is it a far superior solution? does it need 

The aim of regmap is to provide a solution for chip running either on
i2c or spi, that's why I thought it would be interesting to use it
instead of rewriting similar code.

> to used now? (ie. I'm relucant to totally rework this now. Please convince me 
> I need to if required.)

I don't know. I've no strong opinion about it. Moreover, nothing
prevents you to try to merge this version and then convert to regmap,
say once the 25 bit shift problem noticed by Mark is fixed.

Arnaud

Re: mc13xxx: add I2C support

[Mark Brown]

On Fri, Jan 20, 2012 at 06:55:35PM +1100, Marc Reilly wrote:
> On Thursday, January 19, 2012 10:29:41 PM Mark Brown wrote:
> > On Thu, Jan 19, 2012 at 12:12:31PM +0100, Arnaud Patard wrote:

> > > I've never looked at regmap deeply but can't it be done with regmap or is
> > > it just a bad idea ?

> > Glancing quickly at the existing code it should map on reasonably well,
> > though a new format definition may be required for the 25 bit shift that
> > would be trivial.

> I'm sadly unfamiliar with regmap, is it a far superior solution? does it need 
> to used now? (ie. I'm relucant to totally rework this now. Please convince me 
> I need to if required.)

It shouldn't be a total rework - the conversion is usually just a case
of replacing the body of the current read/write functions (and ideally
also update_bits() if you've got a one of those) with the equivalent
regmap calls and adding the calls to initialise and free to the probe
functions.  This should be pretty quick and easy.

The benefit of doing the simple conversion is mostly just code sharing,
though you do also get some trace infrastructure for logging register
writes for free.  With a tiny bit more work (saying what the highest
register number is) you'd also get debugfs support for dumping the
register map which is handy for development.  With another small bit of
work (turning on a flag when registering the device and adding a
function which says which registers the chip can change itself) you can
get a register cache which gives a nice performance improvement on
read/modify/write updates as it cuts out the read.  This is useful for
things like DVFS where the regulator voltage change should be as quick
as possible.  None of this is earth shattering but it's very easy to
deploy once the infrastructure is shared.

I doubt Samuel would insist on doing this immediately, though given that
you're adding new bus support it'd be nice.

Re: mc13xxx: add I2C support

[Samuel Ortiz]

Hi Marc,

On Fri, Jan 20, 2012 at 06:55:35PM +1100, Marc Reilly wrote:
> Hi,
> 
> Thankyou all for your feedback and comments. I'll use them for a V2 but 
> Samuel, I'd like to know if you'd like me to base them on a specific branch 
> before I do.
Please rebase it against my for-next branch.


> Shawn, thanks for testing!
> 
> On Thursday, January 19, 2012 10:29:41 PM Mark Brown wrote:
> > On Thu, Jan 19, 2012 at 12:12:31PM +0100, Arnaud Patard wrote:
> > > I've never looked at regmap deeply but can't it be done with regmap or is
> > > it just a bad idea ?
> > 
> > Glancing quickly at the existing code it should map on reasonably well,
> > though a new format definition may be required for the 25 bit shift that
> > would be trivial.
> 
> I'm sadly unfamiliar with regmap, is it a far superior solution? does it need 
> to used now? (ie. I'm relucant to totally rework this now. Please convince me 
> I need to if required.)
I think Mark exposed the advantages of using regmap. It's not a mandatory API
usage at that point, but since we still have about 4 weeks before the next
merge window opens, and since the changes wouldn't be a huge piece of work,
I'd recommend switching to it now.

Cheers,
Samuel. 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/