mc13xxx core support for i2c

mc13xxx core support for i2c

[Marc Reilly]

These patches add i2c support for the mc13xxx-core driver. This v3 improves the v2
(http://lists.arm.linux.org.uk/lurker/message/20101215.214755.b6bae366.en.html) by splitting
the driver up into core, spi and i2c files so that it can support one subsystem in the absence
of the other.

[PATCHv3 1/4] mc13xxx-core: Consolidate common code to prepare for separate i2c and spi

[Marc Reilly]

This patch moves common mc13xxx definitions to the header file in
preparation for separate i2c and spi driver modules.
spi specific functions are also removed.

Changes to the mc13xxx struct are:
removing the redundant irq member,
adding driver read/write function ptrs,
adding ictype

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/mc13xxx-core.c  |  207 +++++++-----------------------------------
 include/linux/mfd/mc13xxx.h |   77 +++++++++++-----
 2 files changed, 87 insertions(+), 197 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index a2ac2ed..3367a40 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -9,24 +9,14 @@
  * 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>
 
-struct mc13xxx {
-	struct spi_device *spidev;
-	struct mutex lock;
-	int irq;
-
-	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
-	void *irqdata[MC13XXX_NUM_IRQ];
-};
 
 struct mc13783 {
 	struct mc13xxx mc13xxx;
@@ -150,99 +140,53 @@ EXPORT_SYMBOL(mc13783_to_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)
 {
-	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));
-
-	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);
+	ret = mc13xxx->read_dev(mc13xxx, offset, val);
+	dev_vdbg(mc13xxx->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
 
-	if (ret)
-		return ret;
-
-	*val &= 0xffffff;
-
-	dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
-
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(mc13xxx_reg_read);
 
+
 int mc13xxx_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);
+	dev_vdbg(mc13xxx->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));
-
-	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;
+	return mc13xxx->write_dev(mc13xxx, offset, val);
 }
 EXPORT_SYMBOL(mc13xxx_reg_write);
 
+
 int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset,
 		u32 mask, u32 val)
 {
@@ -445,7 +389,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);
 
@@ -481,11 +425,6 @@ 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,
-};
 
 const char *mc13xxx_chipname[] = {
 	[MC13XXX_ID_MC13783] = "mc13783",
@@ -493,13 +432,16 @@ const char *mc13xxx_chipname[] = {
 };
 
 #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;
@@ -508,26 +450,22 @@ 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),
@@ -536,26 +474,12 @@ 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];
 }
 
 #include <linux/mfd/mc13783.h>
@@ -563,7 +487,7 @@ static const char *mc13xxx_get_chipname(struct mc13xxx *mc13xxx)
 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;
 }
@@ -601,7 +525,7 @@ int mc13783_adc_do_conversion(struct mc13783 *mc13783, 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);
 
@@ -643,7 +567,7 @@ int mc13783_adc_do_conversion(struct mc13783 *mc13783, unsigned int mode,
 		return -EINVAL;
 	}
 
-	dev_dbg(&mc13783->mc13xxx.spidev->dev, "%s: request irq\n", __func__);
+	dev_dbg(mc13783->mc13xxx.dev, "%s: request irq\n", __func__);
 	mc13xxx_irq_request(mc13xxx, MC13783_IRQ_ADCDONE,
 			mc13783_handler_adcdone, __func__, &adcdone_data);
 	mc13xxx_irq_ack(mc13xxx, MC13783_IRQ_ADCDONE);
@@ -701,7 +625,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)
@@ -709,29 +633,16 @@ 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)
+int mc13xxx_common_init(struct mc13xxx *mc13xxx,
+			struct mc13xxx_platform_data *pdata, int irq)
 {
-	struct mc13xxx *mc13xxx;
-	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
-	enum mc13xxx_id id;
 	int ret;
 
-	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->spidev = spi;
-
 	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 */
@@ -743,14 +654,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:
 		mutex_unlock(&mc13xxx->lock);
-		dev_set_drvdata(&spi->dev, NULL);
 		kfree(mc13xxx);
 		return ret;
 	}
@@ -786,54 +696,7 @@ err_revision:
 
 	return 0;
 }
-
-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 */
-	}
-};
-
-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);
+EXPORT_SYMBOL_GPL(mc13xxx_common_init);
 
 MODULE_DESCRIPTION("Core driver for Freescale MC13XXX PMIC");
 MODULE_AUTHOR("Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>");
diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
index a1d391b..d7c18d7 100644
--- a/include/linux/mfd/mc13xxx.h
+++ b/include/linux/mfd/mc13xxx.h
@@ -11,7 +11,53 @@
 
 #include <linux/interrupt.h>
 
-struct mc13xxx;
+#define MC13XXX_IRQ_ADCDONE	0
+#define MC13XXX_IRQ_ADCBISDONE	1
+#define MC13XXX_IRQ_TS		2
+#define MC13XXX_IRQ_CHGDET	6
+#define MC13XXX_IRQ_CHGREV	8
+#define MC13XXX_IRQ_CHGSHORT	9
+#define MC13XXX_IRQ_CCCV	10
+#define MC13XXX_IRQ_CHGCURR	11
+#define MC13XXX_IRQ_BPON	12
+#define MC13XXX_IRQ_LOBATL	13
+#define MC13XXX_IRQ_LOBATH	14
+#define MC13XXX_IRQ_1HZ		24
+#define MC13XXX_IRQ_TODA	25
+#define MC13XXX_IRQ_SYSRST	30
+#define MC13XXX_IRQ_RTCRST	31
+#define MC13XXX_IRQ_PC		32
+#define MC13XXX_IRQ_WARM	33
+#define MC13XXX_IRQ_MEMHLD	34
+#define MC13XXX_IRQ_THWARNL	36
+#define MC13XXX_IRQ_THWARNH	37
+#define MC13XXX_IRQ_CLK		38
+
+#define MC13XXX_NUM_IRQ		46
+
+
+enum mc13xxx_id {
+	MC13XXX_ID_MC13783,
+	MC13XXX_ID_MC13892,
+	MC13XXX_ID_INVALID,
+};
+
+struct mc13xxx {
+	union {
+		struct spi_device *spidev;
+		struct i2c_client *i2cclient;
+	};
+	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];
+};
 
 void mc13xxx_lock(struct mc13xxx *mc13xxx);
 void mc13xxx_unlock(struct mc13xxx *mc13xxx);
@@ -21,6 +67,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,
@@ -37,30 +88,6 @@ int mc13xxx_irq_ack(struct mc13xxx *mc13xxx, int irq);
 
 int mc13xxx_get_flags(struct mc13xxx *mc13xxx);
 
-#define MC13XXX_IRQ_ADCDONE	0
-#define MC13XXX_IRQ_ADCBISDONE	1
-#define MC13XXX_IRQ_TS		2
-#define MC13XXX_IRQ_CHGDET	6
-#define MC13XXX_IRQ_CHGREV	8
-#define MC13XXX_IRQ_CHGSHORT	9
-#define MC13XXX_IRQ_CCCV	10
-#define MC13XXX_IRQ_CHGCURR	11
-#define MC13XXX_IRQ_BPON	12
-#define MC13XXX_IRQ_LOBATL	13
-#define MC13XXX_IRQ_LOBATH	14
-#define MC13XXX_IRQ_1HZ		24
-#define MC13XXX_IRQ_TODA	25
-#define MC13XXX_IRQ_SYSRST	30
-#define MC13XXX_IRQ_RTCRST	31
-#define MC13XXX_IRQ_PC		32
-#define MC13XXX_IRQ_WARM	33
-#define MC13XXX_IRQ_MEMHLD	34
-#define MC13XXX_IRQ_THWARNL	36
-#define MC13XXX_IRQ_THWARNH	37
-#define MC13XXX_IRQ_CLK		38
-
-#define MC13XXX_NUM_IRQ		46
-
 struct regulator_init_data;
 
 struct mc13xxx_regulator_init_data {
-- 
1.7.1

[PATCHv3 1/4] mc13xxx-core: Consolidate common code to prepare for separate i2c and spi

[Uwe Kleine-König]

On Mon, Dec 20, 2010 at 02:50:52PM +1100, Marc Reilly wrote:
> This patch moves common mc13xxx definitions to the header file in
> preparation for separate i2c and spi driver modules.
> spi specific functions are also removed.
> 
> Changes to the mc13xxx struct are:
> removing the redundant irq member,
> adding driver read/write function ptrs,
> adding ictype
This list isn't complete, but see below.

I don't like that after this patch the driver isn't functional, because
you removed the spi functionality which breaks bisection.

> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/mfd/mc13xxx-core.c  |  207 +++++++-----------------------------------
>  include/linux/mfd/mc13xxx.h |   77 +++++++++++-----
>  2 files changed, 87 insertions(+), 197 deletions(-)
> 
> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index a2ac2ed..3367a40 100644
> --- a/drivers/mfd/mc13xxx-core.c
> +++ b/drivers/mfd/mc13xxx-core.c
> @@ -9,24 +9,14 @@
>   * the terms of the GNU General Public License version 2 as published by the
>   * Free Software Foundation.
>   */
> -
(hmm.  I think there is no style guide for that, but I know people who
think that this nl should be there.  So IMHO don't touch this.)

>  #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>
>  
> -struct mc13xxx {
> -	struct spi_device *spidev;
> -	struct mutex lock;
> -	int irq;
> -
> -	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
> -	void *irqdata[MC13XXX_NUM_IRQ];
> -};
>  
>  struct mc13783 {
>  	struct mc13xxx mc13xxx;
> @@ -150,99 +140,53 @@ EXPORT_SYMBOL(mc13783_to_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)
>  {
> -	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));
> -
> -	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);
> +	ret = mc13xxx->read_dev(mc13xxx, offset, val);
> +	dev_vdbg(mc13xxx->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
>  
> -	if (ret)
> -		return ret;
> -
> -	*val &= 0xffffff;
> -
> -	dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
> -
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL(mc13xxx_reg_read);
>  
> +
no please

>  int mc13xxx_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);
> +	dev_vdbg(mc13xxx->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));
> -
> -	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;
> +	return mc13xxx->write_dev(mc13xxx, offset, val);
>  }
>  EXPORT_SYMBOL(mc13xxx_reg_write);
>  
> +
no please

>  int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset,
>  		u32 mask, u32 val)
>  {
> @@ -445,7 +389,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);
>  
> @@ -481,11 +425,6 @@ 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,
> -};
>  
>  const char *mc13xxx_chipname[] = {
>  	[MC13XXX_ID_MC13783] = "mc13783",
> @@ -493,13 +432,16 @@ const char *mc13xxx_chipname[] = {
>  };
>  
>  #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
/* should be on a seperate line

> +	 * 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;
> @@ -508,26 +450,22 @@ 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),
> @@ -536,26 +474,12 @@ 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];
>  }
>  
>  #include <linux/mfd/mc13783.h>
> @@ -563,7 +487,7 @@ static const char *mc13xxx_get_chipname(struct mc13xxx *mc13xxx)
>  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;
>  }
> @@ -601,7 +525,7 @@ int mc13783_adc_do_conversion(struct mc13783 *mc13783, 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);
>  
> @@ -643,7 +567,7 @@ int mc13783_adc_do_conversion(struct mc13783 *mc13783, unsigned int mode,
>  		return -EINVAL;
>  	}
>  
> -	dev_dbg(&mc13783->mc13xxx.spidev->dev, "%s: request irq\n", __func__);
> +	dev_dbg(mc13783->mc13xxx.dev, "%s: request irq\n", __func__);
>  	mc13xxx_irq_request(mc13xxx, MC13783_IRQ_ADCDONE,
>  			mc13783_handler_adcdone, __func__, &adcdone_data);
>  	mc13xxx_irq_ack(mc13xxx, MC13783_IRQ_ADCDONE);
> @@ -701,7 +625,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)
> @@ -709,29 +633,16 @@ 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)
> +int mc13xxx_common_init(struct mc13xxx *mc13xxx,
> +			struct mc13xxx_platform_data *pdata, int irq)
>  {
> -	struct mc13xxx *mc13xxx;
> -	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
> -	enum mc13xxx_id id;
>  	int ret;
>  
> -	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->spidev = spi;
> -
>  	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 */
> @@ -743,14 +654,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:
>  		mutex_unlock(&mc13xxx->lock);
> -		dev_set_drvdata(&spi->dev, NULL);
>  		kfree(mc13xxx);
>  		return ret;
>  	}
> @@ -786,54 +696,7 @@ err_revision:
>  
>  	return 0;
>  }
> -
> -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 */
> -	}
> -};
> -
> -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);
> +EXPORT_SYMBOL_GPL(mc13xxx_common_init);
>  
>  MODULE_DESCRIPTION("Core driver for Freescale MC13XXX PMIC");
>  MODULE_AUTHOR("Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>");
> diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
> index a1d391b..d7c18d7 100644
> --- a/include/linux/mfd/mc13xxx.h
> +++ b/include/linux/mfd/mc13xxx.h
> @@ -11,7 +11,53 @@
>  
>  #include <linux/interrupt.h>
>  
> -struct mc13xxx;
> +#define MC13XXX_IRQ_ADCDONE	0
> +#define MC13XXX_IRQ_ADCBISDONE	1
> +#define MC13XXX_IRQ_TS		2
> +#define MC13XXX_IRQ_CHGDET	6
> +#define MC13XXX_IRQ_CHGREV	8
> +#define MC13XXX_IRQ_CHGSHORT	9
> +#define MC13XXX_IRQ_CCCV	10
> +#define MC13XXX_IRQ_CHGCURR	11
> +#define MC13XXX_IRQ_BPON	12
> +#define MC13XXX_IRQ_LOBATL	13
> +#define MC13XXX_IRQ_LOBATH	14
> +#define MC13XXX_IRQ_1HZ		24
> +#define MC13XXX_IRQ_TODA	25
> +#define MC13XXX_IRQ_SYSRST	30
> +#define MC13XXX_IRQ_RTCRST	31
> +#define MC13XXX_IRQ_PC		32
> +#define MC13XXX_IRQ_WARM	33
> +#define MC13XXX_IRQ_MEMHLD	34
> +#define MC13XXX_IRQ_THWARNL	36
> +#define MC13XXX_IRQ_THWARNH	37
> +#define MC13XXX_IRQ_CLK		38
> +
> +#define MC13XXX_NUM_IRQ		46
> +
> +
> +enum mc13xxx_id {
> +	MC13XXX_ID_MC13783,
> +	MC13XXX_ID_MC13892,
> +	MC13XXX_ID_INVALID,
> +};
> +
> +struct mc13xxx {
> +	union {
> +		struct spi_device *spidev;
> +		struct i2c_client *i2cclient;
> +	};
I'd do this in a later patch.  IMHO the first patch should only shuffle
code around without changing any behaviour.

> +	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];
> +};
>  
>  void mc13xxx_lock(struct mc13xxx *mc13xxx);
>  void mc13xxx_unlock(struct mc13xxx *mc13xxx);
> @@ -21,6 +67,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,
> @@ -37,30 +88,6 @@ int mc13xxx_irq_ack(struct mc13xxx *mc13xxx, int irq);
>  
>  int mc13xxx_get_flags(struct mc13xxx *mc13xxx);
>  
> -#define MC13XXX_IRQ_ADCDONE	0
> -#define MC13XXX_IRQ_ADCBISDONE	1
> -#define MC13XXX_IRQ_TS		2
> -#define MC13XXX_IRQ_CHGDET	6
> -#define MC13XXX_IRQ_CHGREV	8
> -#define MC13XXX_IRQ_CHGSHORT	9
> -#define MC13XXX_IRQ_CCCV	10
> -#define MC13XXX_IRQ_CHGCURR	11
> -#define MC13XXX_IRQ_BPON	12
> -#define MC13XXX_IRQ_LOBATL	13
> -#define MC13XXX_IRQ_LOBATH	14
> -#define MC13XXX_IRQ_1HZ		24
> -#define MC13XXX_IRQ_TODA	25
> -#define MC13XXX_IRQ_SYSRST	30
> -#define MC13XXX_IRQ_RTCRST	31
> -#define MC13XXX_IRQ_PC		32
> -#define MC13XXX_IRQ_WARM	33
> -#define MC13XXX_IRQ_MEMHLD	34
> -#define MC13XXX_IRQ_THWARNL	36
> -#define MC13XXX_IRQ_THWARNH	37
> -#define MC13XXX_IRQ_CLK		38
> -
> -#define MC13XXX_NUM_IRQ		46
> -
why don't you keep these at their original place?

Uwe

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

[PATCHv3 1/4] mc13xxx-core: Consolidate common code to prepare for separate i2c and spi

[Marc Reilly]

Hi Uwe,

Thanks again for your comments, and your patience :)

On Monday, December 20, 2010 07:31:20 pm Uwe Kleine-K?nig wrote:
> On Mon, Dec 20, 2010 at 02:50:52PM +1100, Marc Reilly wrote:
> > This patch moves common mc13xxx definitions to the header file in
> > preparation for separate i2c and spi driver modules.
> > spi specific functions are also removed.
> > 
> > Changes to the mc13xxx struct are:
> > removing the redundant irq member,
> > adding driver read/write function ptrs,
> > adding ictype
> 
> This list isn't complete, but see below.

Do you mean the list above is incomplete? I should have said "Changes to the 
struct _include_". 
I'm ignorant of "patch summary etiquette" (or what people otherwise like to 
see) but I had just assumed that the patch comments were more like a 
highlights reel - the real info is in the patch itself.

Or did you just mean that your list of critiques is incomplete? :)

> 
> I don't like that after this patch the driver isn't functional, because
> you removed the spi functionality which breaks bisection.

OK. I should introduce things more slowly, like adding the read/write function 
ptrs and splitting out the common code. Then move the spi code, then add the 
i2c driver...  

> 
> > Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> > ---
> > 
> >  drivers/mfd/mc13xxx-core.c  |  207
> >  +++++++----------------------------------- include/linux/mfd/mc13xxx.h
> >  |   77 +++++++++++-----
> >  2 files changed, 87 insertions(+), 197 deletions(-)
> > 
> > diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> > index a2ac2ed..3367a40 100644
> > --- a/drivers/mfd/mc13xxx-core.c
> > +++ b/drivers/mfd/mc13xxx-core.c
> > @@ -9,24 +9,14 @@
> > 
> >   * the terms of the GNU General Public License version 2 as published by
> >   the * Free Software Foundation.
> >   */
> > 
> > -
> 
> (hmm.  I think there is no style guide for that, but I know people who
> think that this nl should be there.  So IMHO don't touch this.)

Follow over from the first time around. I don't notice it because I don't 
really care - but I understand why people might. 
And because I don't care, it can go back in without further hassle :)

> 
> >  #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>
> > 
> > -struct mc13xxx {
> > -	struct spi_device *spidev;
> > -	struct mutex lock;
> > -	int irq;
> > -
> > -	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
> > -	void *irqdata[MC13XXX_NUM_IRQ];
> > -};
> > 
> >  struct mc13783 {
> >  
> >  	struct mc13xxx mc13xxx;
> > 
> > @@ -150,99 +140,53 @@ EXPORT_SYMBOL(mc13783_to_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) {
> > 
> > -	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));
> > -
> > -	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);
> > +	ret = mc13xxx->read_dev(mc13xxx, offset, val);
> > +	dev_vdbg(mc13xxx->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
> > 
> > -	if (ret)
> > -		return ret;
> > -
> > -	*val &= 0xffffff;
> > -
> > -	dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] -> 0x%06x\n", offset, *val);
> > -
> > -	return 0;
> > +	return ret;
> > 
> >  }
> >  EXPORT_SYMBOL(mc13xxx_reg_read);
> > 
> > +
> 
> no please
(whoops x2)

> 
> >  int mc13xxx_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);
> > +	dev_vdbg(mc13xxx->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));
> > -
> > -	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;
> > +	return mc13xxx->write_dev(mc13xxx, offset, val);
> > 
> >  }
> >  EXPORT_SYMBOL(mc13xxx_reg_write);
> > 
> > +
> 
> no please
> 
> >  int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset,
> >  
> >  		u32 mask, u32 val)
> >  
> >  {
> > 
> > @@ -445,7 +389,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);
> > 
> > @@ -481,11 +425,6 @@ 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,
> > -};
> > 
> >  const char *mc13xxx_chipname[] = {
> >  
> >  	[MC13XXX_ID_MC13783] = "mc13783",
> > 
> > @@ -493,13 +432,16 @@ const char *mc13xxx_chipname[] = {
> > 
> >  };
> >  
> >  #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
> 
> /* should be on a seperate line

Sorry. You asked me to read the coding style docs for precisely this. I did, 
but then I saw it done this way in several other files and inadvertently 
copied that.

> 
> > +	 * 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;
> > 
> > @@ -508,26 +450,22 @@ 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),
> > 
> > @@ -536,26 +474,12 @@ 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];
> > 
> >  }
> >  
> >  #include <linux/mfd/mc13783.h>
> > 
> > @@ -563,7 +487,7 @@ static const char *mc13xxx_get_chipname(struct
> > mc13xxx *mc13xxx)
> > 
> >  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;
> >  
> >  }
> > 
> > @@ -601,7 +525,7 @@ int mc13783_adc_do_conversion(struct mc13783
> > *mc13783, 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);
> > 
> > @@ -643,7 +567,7 @@ int mc13783_adc_do_conversion(struct mc13783
> > *mc13783, unsigned int mode,
> > 
> >  		return -EINVAL;
> >  	
> >  	}
> > 
> > -	dev_dbg(&mc13783->mc13xxx.spidev->dev, "%s: request irq\n", __func__);
> > +	dev_dbg(mc13783->mc13xxx.dev, "%s: request irq\n", __func__);
> > 
> >  	mc13xxx_irq_request(mc13xxx, MC13783_IRQ_ADCDONE,
> >  	
> >  			mc13783_handler_adcdone, __func__, &adcdone_data);
> >  	
> >  	mc13xxx_irq_ack(mc13xxx, MC13783_IRQ_ADCDONE);
> > 
> > @@ -701,7 +625,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)
> > 
> > @@ -709,29 +633,16 @@ 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)
> > +int mc13xxx_common_init(struct mc13xxx *mc13xxx,
> > +			struct mc13xxx_platform_data *pdata, int irq)
> > 
> >  {
> > 
> > -	struct mc13xxx *mc13xxx;
> > -	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
> > -	enum mc13xxx_id id;
> > 
> >  	int ret;
> > 
> > -	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->spidev = spi;
> > -
> > 
> >  	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 */
> > 
> > @@ -743,14 +654,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:
> >  		mutex_unlock(&mc13xxx->lock);
> > 
> > -		dev_set_drvdata(&spi->dev, NULL);
> > 
> >  		kfree(mc13xxx);
> >  		return ret;
> >  	
> >  	}
> > 
> > @@ -786,54 +696,7 @@ err_revision:
> >  	return 0;
> >  
> >  }
> > 
> > -
> > -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 */
> > -	}
> > -};
> > -
> > -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);
> > +EXPORT_SYMBOL_GPL(mc13xxx_common_init);
> > 
> >  MODULE_DESCRIPTION("Core driver for Freescale MC13XXX PMIC");
> >  MODULE_AUTHOR("Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>");
> > 
> > diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
> > index a1d391b..d7c18d7 100644
> > --- a/include/linux/mfd/mc13xxx.h
> > +++ b/include/linux/mfd/mc13xxx.h
> > @@ -11,7 +11,53 @@
> > 
> >  #include <linux/interrupt.h>
> > 
> > -struct mc13xxx;
> > +#define MC13XXX_IRQ_ADCDONE	0
> > +#define MC13XXX_IRQ_ADCBISDONE	1
> > +#define MC13XXX_IRQ_TS		2
> > +#define MC13XXX_IRQ_CHGDET	6
> > +#define MC13XXX_IRQ_CHGREV	8
> > +#define MC13XXX_IRQ_CHGSHORT	9
> > +#define MC13XXX_IRQ_CCCV	10
> > +#define MC13XXX_IRQ_CHGCURR	11
> > +#define MC13XXX_IRQ_BPON	12
> > +#define MC13XXX_IRQ_LOBATL	13
> > +#define MC13XXX_IRQ_LOBATH	14
> > +#define MC13XXX_IRQ_1HZ		24
> > +#define MC13XXX_IRQ_TODA	25
> > +#define MC13XXX_IRQ_SYSRST	30
> > +#define MC13XXX_IRQ_RTCRST	31
> > +#define MC13XXX_IRQ_PC		32
> > +#define MC13XXX_IRQ_WARM	33
> > +#define MC13XXX_IRQ_MEMHLD	34
> > +#define MC13XXX_IRQ_THWARNL	36
> > +#define MC13XXX_IRQ_THWARNH	37
> > +#define MC13XXX_IRQ_CLK		38
> > +
> > +#define MC13XXX_NUM_IRQ		46
> > +
> > +
> > +enum mc13xxx_id {
> > +	MC13XXX_ID_MC13783,
> > +	MC13XXX_ID_MC13892,
> > +	MC13XXX_ID_INVALID,
> > +};
> > +
> > +struct mc13xxx {
> > +	union {
> > +		struct spi_device *spidev;
> > +		struct i2c_client *i2cclient;
> > +	};
> 
> I'd do this in a later patch.  IMHO the first patch should only shuffle
> code around without changing any behaviour.

OK. I think i understand.

> 
> > +	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];
> > +};
> > 
> >  void mc13xxx_lock(struct mc13xxx *mc13xxx);
> >  void mc13xxx_unlock(struct mc13xxx *mc13xxx);
> > 
> > @@ -21,6 +67,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,
> > 
> > @@ -37,30 +88,6 @@ int mc13xxx_irq_ack(struct mc13xxx *mc13xxx, int irq);
> > 
> >  int mc13xxx_get_flags(struct mc13xxx *mc13xxx);
> > 
> > -#define MC13XXX_IRQ_ADCDONE	0
> > -#define MC13XXX_IRQ_ADCBISDONE	1
> > -#define MC13XXX_IRQ_TS		2
> > -#define MC13XXX_IRQ_CHGDET	6
> > -#define MC13XXX_IRQ_CHGREV	8
> > -#define MC13XXX_IRQ_CHGSHORT	9
> > -#define MC13XXX_IRQ_CCCV	10
> > -#define MC13XXX_IRQ_CHGCURR	11
> > -#define MC13XXX_IRQ_BPON	12
> > -#define MC13XXX_IRQ_LOBATL	13
> > -#define MC13XXX_IRQ_LOBATH	14
> > -#define MC13XXX_IRQ_1HZ		24
> > -#define MC13XXX_IRQ_TODA	25
> > -#define MC13XXX_IRQ_SYSRST	30
> > -#define MC13XXX_IRQ_RTCRST	31
> > -#define MC13XXX_IRQ_PC		32
> > -#define MC13XXX_IRQ_WARM	33
> > -#define MC13XXX_IRQ_MEMHLD	34
> > -#define MC13XXX_IRQ_THWARNL	36
> > -#define MC13XXX_IRQ_THWARNH	37
> > -#define MC13XXX_IRQ_CLK		38
> > -
> > -#define MC13XXX_NUM_IRQ		46
> > -
> 
> why don't you keep these at their original place?

The MC13XXX_NUM_IRQ is needed for the irqhandler and irqdata arrays in the 
mc13xxx struct. Agree, the actual IRQ defines should have stayed where they 
were.


> 
> Uwe

Thanks again,

Cheers
Marc

[PATCHv3 1/4] mc13xxx-core: Consolidate common code to prepare for separate i2c and spi

[Mark Brown]

On Mon, Dec 20, 2010 at 09:00:29PM +1100, Marc Reilly wrote:
> On Monday, December 20, 2010 07:31:20 pm Uwe Kleine-K?nig wrote:

> > This list isn't complete, but see below.

> Do you mean the list above is incomplete? I should have said "Changes to the 
> struct _include_". 
> I'm ignorant of "patch summary etiquette" (or what people otherwise like to 
> see) but I had just assumed that the patch comments were more like a 
> highlights reel - the real info is in the patch itself.

No, the patch summary should generally be complete - if your patch is
changing so many things that it's getting tedious to list them all then
that's usually a sign that it should be split up into a series of
changes.

[PATCHv3 1/4] mc13xxx-core: Consolidate common code to prepare for separate i2c and spi

[Grant Likely]

Hi Marc and Uwe,

On Mon, Dec 20, 2010 at 09:31:20AM +0100, Uwe Kleine-K?nig wrote:
> On Mon, Dec 20, 2010 at 02:50:52PM +1100, Marc Reilly wrote:
> > This patch moves common mc13xxx definitions to the header file in
> > preparation for separate i2c and spi driver modules.
> > spi specific functions are also removed.
> > 
> > Changes to the mc13xxx struct are:
> > removing the redundant irq member,
> > adding driver read/write function ptrs,
> > adding ictype
> This list isn't complete, but see below.
> 
> I don't like that after this patch the driver isn't functional, because
> you removed the spi functionality which breaks bisection.

Ugh, yes.  Breaking bisection is absolutely a no-no.  I'll wait for
the next version of this series before I do a full review.

> 
> > Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> > ---
> >  drivers/mfd/mc13xxx-core.c  |  207 +++++++-----------------------------------
> >  include/linux/mfd/mc13xxx.h |   77 +++++++++++-----
> >  2 files changed, 87 insertions(+), 197 deletions(-)
> > 
> > diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> > index a2ac2ed..3367a40 100644
> > --- a/drivers/mfd/mc13xxx-core.c
> > +++ b/drivers/mfd/mc13xxx-core.c
> > @@ -9,24 +9,14 @@
> >   * the terms of the GNU General Public License version 2 as published by the
> >   * Free Software Foundation.
> >   */
> > -
> (hmm.  I think there is no style guide for that, but I know people who
> think that this nl should be there.  So IMHO don't touch this.)

A better comment is: don't make unrelated, non-functional whitespace
changes in patches for something else.  They add noise that make it
harder to review.

Cheers,
g.

[PATCHv3 2/4] mc13xxx: Add spi driver

[Marc Reilly]

Adds spi specific code for mc13xxx.
Most code extracted from the previous mc13xxx-core.c

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/mc13xxx-spi.c |  168 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 168 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/mc13xxx-spi.c

diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
new file mode 100644
index 0000000..de832ea
--- /dev/null
+++ b/drivers/mfd/mc13xxx-spi.c
@@ -0,0 +1,168 @@
+/*
+ * Copyright 2009-2010 Pengutronix
+ * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
+ *
+ * loosely based on an earlier driver that has
+ * Copyright 2009 Pengutronix, Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * 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;
+
+	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(spi->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_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_driver);
+}
+subsys_initcall(mc13xxx_spi_init);
+
+static void __exit mc13xxx_spi_exit(void)
+{
+	spi_unregister_driver(&mc13xxx_driver);
+}
+module_exit(mc13xxx_spi_exit);
+
+MODULE_DESCRIPTION("spi driver for Freescale MC13XXX PMIC");
+MODULE_AUTHOR("Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.1

[PATCHv3 3/4] mc13xxx: Add i2c driver

[Marc Reilly]

Adds i2c specific code for mc13xxx.

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/mc13xxx-i2c.c |  115 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 115 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/mc13xxx-i2c.c

diff --git a/drivers/mfd/mc13xxx-i2c.c b/drivers/mfd/mc13xxx-i2c.c
new file mode 100644
index 0000000..f7ceed8
--- /dev/null
+++ b/drivers/mfd/mc13xxx-i2c.c
@@ -0,0 +1,115 @@
+/*
+ * Copyright 2009-2010 Creative Product Design
+ * Marc Reilly marc at cpdesign.com.au
+ *
+ *
+ * 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;
+
+	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@cpdesign.com.au");
+MODULE_LICENSE("GPL v2");
-- 
1.7.1

[PATCHv3 4/4] mc13xxx: Add i2c and spi drivers to Kconfig and Makefile

[Marc Reilly]

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/Kconfig  |   22 ++++++++++++++--------
 drivers/mfd/Makefile |    2 ++
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3a1493b..6980cf2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -424,20 +424,26 @@ config MFD_PCF50633
 	  facilities, and registers devices for the various functions
 	  so that function-specific drivers can bind to them.
 
-config MFD_MC13783
-	tristate
-
 config MFD_MC13XXX
-	tristate "Support Freescale MC13783 and MC13892"
-	depends on SPI_MASTER
+	tristate "Support Freescale MC13XXX"
+	depends on SPI_MASTER || I2C
 	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,
+	  This driver provides common support for accessing the device,
 	  additional drivers must be enabled in order to use the
-	  functionality of the device.
+	  functionality of these devices.
+
+config MFD_MC13XXX_SPI
+	tristate "Support for MC13XXX via SPI"
+	depends on SPI_MASTER
+	select MFD_MC13XXX
+
+config MFD_MC13XXX_I2C
+	tristate "Support for MC13XXX via I2C"
+	depends on I2C
+	select MFD_MC13XXX
 
 config PCF50633_ADC
 	tristate "Support for NXP PCF50633 ADC"
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index f54b365..b7d774f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -42,6 +42,8 @@ obj-$(CONFIG_TWL4030_CODEC)	+= twl4030-codec.o
 obj-$(CONFIG_TWL6030_PWM)	+= twl6030-pwm.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
 
-- 
1.7.1

[PATCHv3 4/4] mc13xxx: Add i2c and spi drivers to Kconfig and Makefile

[Uwe Kleine-König]

On Mon, Dec 20, 2010 at 02:50:55PM +1100, Marc Reilly wrote:
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/mfd/Kconfig  |   22 ++++++++++++++--------
>  drivers/mfd/Makefile |    2 ++
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3a1493b..6980cf2 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -424,20 +424,26 @@ config MFD_PCF50633
>  	  facilities, and registers devices for the various functions
>  	  so that function-specific drivers can bind to them.
>  
> -config MFD_MC13783
> -	tristate
> -
this needs at least a note in the commit log.  And $(git grep
MFD_MC13783 drivers) shows that you don't want to remove that yet.

>  config MFD_MC13XXX
> -	tristate "Support Freescale MC13783 and MC13892"
> -	depends on SPI_MASTER
> +	tristate "Support Freescale MC13XXX"
Hmm, I'd prefer to have the supported numbers.  Consider FSL releasing
an MC13991.  Then it's unclear if MFD_MC13XXX supports it.

> +	depends on SPI_MASTER || I2C
>  	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,
> +	  This driver provides common support for accessing the device,
>  	  additional drivers must be enabled in order to use the
> -	  functionality of the device.
> +	  functionality of these devices.
> +
> +config MFD_MC13XXX_SPI
> +	tristate "Support for MC13XXX via SPI"
> +	depends on SPI_MASTER
> +	select MFD_MC13XXX
> +
> +config MFD_MC13XXX_I2C
> +	tristate "Support for MC13XXX via I2C"
and only mc13892 can do i2c, so writing MC13XXX doesn't make sense here.

> +	depends on I2C
> +	select MFD_MC13XXX
Hmm, that means that MFD_MC13XXX alone doesn't do anything useful,
right?  IMHO either do:

	config MFD_MC13XXX_SPI
		tristate

	config MFD_MC13XXX_I2C
		tristate

	config MFD_MC13XXX
		tristate "..."
		select MFD_MC13XXX_SPI if SPI_MASTER
		select MFD_MC13XXX_I2C if I2C
		...

or

	config MFD_MC13XXX
		tristate
		...

	config MFD_MC13XXX_SPI
		tristate "..."
		select MFD_MC13XXX
		...
	config MFD_MC13XXX_I2C
		tristate "..."
		select MFD_MC13XXX
		...

>  config PCF50633_ADC
>  	tristate "Support for NXP PCF50633 ADC"
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f54b365..b7d774f 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -42,6 +42,8 @@ obj-$(CONFIG_TWL4030_CODEC)	+= twl4030-codec.o
>  obj-$(CONFIG_TWL6030_PWM)	+= twl6030-pwm.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

Best regards
Uwe

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

[PATCHv3 4/4] mc13xxx: Add i2c and spi drivers to Kconfig and Makefile

[Marc Reilly]

Hi,

On Monday, December 20, 2010 07:38:39 pm Uwe Kleine-K?nig wrote:
> On Mon, Dec 20, 2010 at 02:50:55PM +1100, Marc Reilly wrote:
> > Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> > ---
> > 
> >  drivers/mfd/Kconfig  |   22 ++++++++++++++--------
> >  drivers/mfd/Makefile |    2 ++
> >  2 files changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 3a1493b..6980cf2 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -424,20 +424,26 @@ config MFD_PCF50633
> > 
> >  	  facilities, and registers devices for the various functions
> >  	  so that function-specific drivers can bind to them.
> > 
> > -config MFD_MC13783
> > -	tristate
> > -
> 
> this needs at least a note in the commit log.  And $(git grep
> MFD_MC13783 drivers) shows that you don't want to remove that yet.

I didn't get anything... which makes me think I'm doing it wrong.

> 
> >  config MFD_MC13XXX
> > 
> > -	tristate "Support Freescale MC13783 and MC13892"
> > -	depends on SPI_MASTER
> > +	tristate "Support Freescale MC13XXX"
> 
> Hmm, I'd prefer to have the supported numbers.  Consider FSL releasing
> an MC13991.  Then it's unclear if MFD_MC13XXX supports it.

I started with similar thinking, but then thought that if a new IC is released 
in the family and it's not compatible then it can be changed (the file names 
themselves would also be misleading). Until then, all the family so far come 
under the mc13xxx umbrella.
The help file also explains which are supported.

> 
> > +	depends on SPI_MASTER || I2C
> > 
> >  	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,
> > +	  This driver provides common support for accessing the device,
> > 
> >  	  additional drivers must be enabled in order to use the
> > 
> > -	  functionality of the device.
> > +	  functionality of these devices.
> > +
> > +config MFD_MC13XXX_SPI
> > +	tristate "Support for MC13XXX via SPI"
> > +	depends on SPI_MASTER
> > +	select MFD_MC13XXX
> > +
> > +config MFD_MC13XXX_I2C
> > +	tristate "Support for MC13XXX via I2C"
> 
> and only mc13892 can do i2c, so writing MC13XXX doesn't make sense here.
> 
> > +	depends on I2C
> > +	select MFD_MC13XXX
> 
> Hmm, that means that MFD_MC13XXX alone doesn't do anything useful,
> right?  IMHO either do:
> 
> 	config MFD_MC13XXX_SPI
> 		tristate
> 
> 	config MFD_MC13XXX_I2C
> 		tristate
> 
> 	config MFD_MC13XXX
> 		tristate "..."
> 		select MFD_MC13XXX_SPI if SPI_MASTER
> 		select MFD_MC13XXX_I2C if I2C
> 		...
> 
> or
> 
> 	config MFD_MC13XXX
> 		tristate
> 		...
> 
> 	config MFD_MC13XXX_SPI
> 		tristate "..."
> 		select MFD_MC13XXX
> 		...
> 	config MFD_MC13XXX_I2C
> 		tristate "..."
> 		select MFD_MC13XXX
> 		...

It would need to be the second one, but OK.

> 
> >  config PCF50633_ADC
> >  
> >  	tristate "Support for NXP PCF50633 ADC"
> > 
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index f54b365..b7d774f 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -42,6 +42,8 @@ obj-$(CONFIG_TWL4030_CODEC)	+= twl4030-codec.o
> > 
> >  obj-$(CONFIG_TWL6030_PWM)	+= twl6030-pwm.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
> 
> Best regards
> Uwe

Thanks again.

Cheers
Marc

mc13xxx core support for i2c

[Ben Dooks]

On 20/12/10 03:50, Marc Reilly wrote:
> These patches add i2c support for the mc13xxx-core driver. This v3 improves the v2
> (http://lists.arm.linux.org.uk/lurker/message/20101215.214755.b6bae366.en.html) by splitting
> the driver up into core, spi and i2c files so that it can support one subsystem in the absence
> of the other.

As noted by others, this needs to be re-worked so that the driver can
always been built and used across bisections.

A suggested approach would be to move items out into the header file
first, and then maybe split the spi driver out of the core, and then
add the I2c driver. It may also make the patch set easier to follow.