[PATCHv2] mc13xxx: Add i2c support for mc13xxx-core

[PATCHv2] mc13xxx: Add i2c support for mc13xxx-core

[Marc Reilly]

Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
---
 drivers/mfd/mc13xxx-core.c |  331 +++++++++++++++++++++++++++++++-------------
 1 files changed, 236 insertions(+), 95 deletions(-)

diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index a2ac2ed..d355433 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -9,7 +9,6 @@
  * 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>
@@ -18,12 +17,28 @@
 #include <linux/spi/spi.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/mc13xxx.h>
+#include <linux/i2c.h>
+
+enum mc13xxx_id {
+	MC13XXX_ID_MC13783,
+	MC13XXX_ID_MC13892,
+	MC13XXX_ID_INVALID,
+};
 
 struct mc13xxx {
-	struct spi_device *spidev;
+	union {
+		struct spi_device *spidev;
+		struct i2c_client *i2cclient;
+	};
+	struct device *pdev;
+	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];
 };
@@ -150,36 +165,47 @@ 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->pdev, "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->pdev, "%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->pdev, "%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;
 
+	ret = mc13xxx->read_dev(mc13xxx, offset, val);
+	dev_vdbg(mc13xxx->pdev, "[0x%02x] -> 0x%06x\n", offset, *val);
+
+	return ret;
+}
+EXPORT_SYMBOL(mc13xxx_reg_read);
+
+#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));
@@ -201,26 +227,42 @@ 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_i2c_reg_read(struct mc13xxx *mc13xxx, unsigned int offset,
+		u32 *val)
 {
-	u32 buf;
-	struct spi_transfer t;
-	struct spi_message m;
 	int ret;
+	unsigned char buff[3] = {0, 0, 0};
 
-	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
+	ret = i2c_smbus_read_i2c_block_data(mc13xxx->i2cclient,
+			offset, 3, buff);
+	*val = buff[0] << 16 | buff[1] << 8 | buff[2];
 
-	dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] <- 0x%06x\n", offset, val);
+	return ret == 3 ? 0 : ret;
+}
+
+int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val)
+{
+	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
+	dev_vdbg(mc13xxx->pdev, "[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);
+
+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));
@@ -241,7 +283,22 @@ int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val)
 
 	return 0;
 }
-EXPORT_SYMBOL(mc13xxx_reg_write);
+
+static int mc13xxx_i2c_reg_write(struct mc13xxx *mc13xxx, unsigned int offset,
+		u32 val)
+{
+	int ret;
+	unsigned char buff[3];
+
+	buff[0] = (val >> 16) & 0xff;
+	buff[1] = (val >> 8) & 0xff;
+	buff[2] = val & 0xff;
+
+	ret = i2c_smbus_write_i2c_block_data(mc13xxx->i2cclient,
+			offset, 3, buff);
+
+	return ret;
+}
 
 int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset,
 		u32 mask, u32 val)
@@ -445,7 +502,7 @@ static int mc13xxx_irq_handle(struct mc13xxx *mc13xxx,
 			if (handled == IRQ_HANDLED)
 				num_handled++;
 		} else {
-			dev_err(&mc13xxx->spidev->dev,
+			dev_err(mc13xxx->pdev,
 					"BUG: irq %u but no handler\n",
 					baseirq + irq);
 
@@ -481,11 +538,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 +545,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 +563,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->pdev, "%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 +587,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;
 }
 
 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 +600,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->pdev);
 
 	return pdata->flags;
 }
@@ -601,7 +638,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->pdev, "%s\n", __func__);
 
 	mc13xxx_lock(mc13xxx);
 
@@ -643,7 +680,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.pdev, "%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 +738,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->pdev, -1, &cell, 1, NULL, 0);
 }
 
 static int mc13xxx_add_subdevice(struct mc13xxx *mc13xxx, const char *format)
@@ -709,11 +746,44 @@ 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)
+static int mc13xxx_common_probe(struct mc13xxx *mc13xxx,
+				struct mc13xxx_platform_data *pdata)
+{
+	if (pdata->flags & MC13XXX_USE_ADC)
+		mc13xxx_add_subdevice(mc13xxx, "%s-adc");
+
+	if (pdata->flags & MC13XXX_USE_CODEC)
+		mc13xxx_add_subdevice(mc13xxx, "%s-codec");
+
+	if (pdata->flags & MC13XXX_USE_REGULATOR) {
+		struct mc13xxx_regulator_platform_data regulator_pdata = {
+			.num_regulators = pdata->num_regulators,
+			.regulators = pdata->regulators,
+		};
+
+		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-regulator",
+				&regulator_pdata, sizeof(regulator_pdata));
+	}
+
+	if (pdata->flags & MC13XXX_USE_RTC)
+		mc13xxx_add_subdevice(mc13xxx, "%s-rtc");
+
+	if (pdata->flags & MC13XXX_USE_TOUCHSCREEN)
+		mc13xxx_add_subdevice(mc13xxx, "%s-ts");
+
+	if (pdata->flags & MC13XXX_USE_LED) {
+		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led",
+					pdata->leds, sizeof(*pdata->leds));
+	}
+
+	return 0;
+}
+
+static int mc13xxx_spi_probe(struct spi_device *spi)
 {
 	struct mc13xxx *mc13xxx;
 	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
-	enum mc13xxx_id id;
+	const struct spi_device_id *devid;
 	int ret;
 
 	mc13xxx = kzalloc(sizeof(*mc13xxx), GFP_KERNEL);
@@ -725,15 +795,24 @@ static int mc13xxx_probe(struct spi_device *spi)
 	spi->bits_per_word = 32;
 	spi_setup(spi);
 
+	mc13xxx->pdev = &spi->dev;
 	mc13xxx->spidev = spi;
+	mc13xxx->read_dev = mc13xxx_spi_reg_read;
+	mc13xxx->write_dev = mc13xxx_spi_reg_write;
+	mc13xxx->irq = spi->irq;
 
 	mutex_init(&mc13xxx->lock);
 	mc13xxx_lock(mc13xxx);
 
-	ret = mc13xxx_identify(mc13xxx, &id);
-	if (ret || id == MC13XXX_ID_INVALID)
+	ret = mc13xxx_identify(mc13xxx);
+	if (ret || (mc13xxx->ictype == MC13XXX_ID_INVALID))
 		goto err_revision;
 
+	devid = spi_get_device_id(mc13xxx->spidev);
+	if (!devid || devid->driver_data != mc13xxx->ictype)
+		dev_warn(mc13xxx->pdev, "device id doesn't "
+					"match auto detection!\n");
+
 	/* mask all irqs */
 	ret = mc13xxx_reg_write(mc13xxx, MC13XXX_IRQMASK0, 0x00ffffff);
 	if (ret)
@@ -743,7 +822,7 @@ 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(mc13xxx->irq, NULL, mc13xxx_irq_thread,
 			IRQF_ONESHOT | IRQF_TRIGGER_HIGH, "mc13xxx", mc13xxx);
 
 	if (ret) {
@@ -757,41 +836,15 @@ err_revision:
 
 	mc13xxx_unlock(mc13xxx);
 
-	if (pdata->flags & MC13XXX_USE_ADC)
-		mc13xxx_add_subdevice(mc13xxx, "%s-adc");
-
-	if (pdata->flags & MC13XXX_USE_CODEC)
-		mc13xxx_add_subdevice(mc13xxx, "%s-codec");
-
-	if (pdata->flags & MC13XXX_USE_REGULATOR) {
-		struct mc13xxx_regulator_platform_data regulator_pdata = {
-			.num_regulators = pdata->num_regulators,
-			.regulators = pdata->regulators,
-		};
-
-		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-regulator",
-				&regulator_pdata, sizeof(regulator_pdata));
-	}
-
-	if (pdata->flags & MC13XXX_USE_RTC)
-		mc13xxx_add_subdevice(mc13xxx, "%s-rtc");
-
-	if (pdata->flags & MC13XXX_USE_TOUCHSCREEN)
-		mc13xxx_add_subdevice(mc13xxx, "%s-ts");
-
-	if (pdata->flags & MC13XXX_USE_LED) {
-		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led",
-					pdata->leds, sizeof(*pdata->leds));
-	}
-
-	return 0;
+	return mc13xxx_common_probe(mc13xxx, pdata);
 }
 
-static int __devexit mc13xxx_remove(struct spi_device *spi)
+
+static int __devexit mc13xxx_spi_remove(struct spi_device *spi)
 {
 	struct mc13xxx *mc13xxx = dev_get_drvdata(&spi->dev);
 
-	free_irq(mc13xxx->spidev->irq, mc13xxx);
+	free_irq(mc13xxx->irq, mc13xxx);
 
 	mfd_remove_devices(&spi->dev);
 
@@ -819,18 +872,106 @@ static struct spi_driver mc13xxx_driver = {
 		.bus = &spi_bus_type,
 		.owner = THIS_MODULE,
 	},
-	.probe = mc13xxx_probe,
-	.remove = __devexit_p(mc13xxx_remove),
+	.probe = mc13xxx_spi_probe,
+	.remove = __devexit_p(mc13xxx_spi_remove),
+};
+
+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->pdev = &client->dev;
+	mc13xxx->i2cclient = client;
+	mc13xxx->read_dev = mc13xxx_i2c_reg_read;
+	mc13xxx->write_dev = mc13xxx_i2c_reg_write;
+	mc13xxx->irq = client->irq;
+
+	mutex_init(&mc13xxx->lock);
+	mc13xxx_lock(mc13xxx);
+
+	ret = mc13xxx_identify(mc13xxx);
+	if (ret || (mc13xxx->ictype == MC13XXX_ID_INVALID))
+		goto err_revision;
+
+	if (id->driver_data != mc13xxx->ictype)
+		dev_warn(mc13xxx->pdev, "device id doesn't "
+					"match auto detection!\n");
+
+	/* mask all irqs */
+	ret = mc13xxx_reg_write(mc13xxx, MC13XXX_IRQMASK0, 0x00ffffff);
+	if (ret)
+		goto err_mask;
+
+	ret = mc13xxx_reg_write(mc13xxx, MC13XXX_IRQMASK1, 0x00ffffff);
+	if (ret)
+		goto err_mask;
+
+	ret = request_threaded_irq(mc13xxx->irq, NULL, mc13xxx_irq_thread,
+			IRQF_ONESHOT | IRQF_TRIGGER_HIGH, "mc13xxx", mc13xxx);
+
+	if (ret) {
+err_mask:
+err_revision:
+		mutex_unlock(&mc13xxx->lock);
+		kfree(mc13xxx);
+		return ret;
+	}
+
+	mc13xxx_unlock(mc13xxx);
+
+	return mc13xxx_common_probe(mc13xxx, pdata);
+}
+
+static int __devexit mc13xxx_i2c_remove(struct i2c_client *client)
+{
+	struct mc13xxx *mc13xxx = dev_get_drvdata(&client->dev);
+
+	free_irq(mc13xxx->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_init(void)
 {
-	return spi_register_driver(&mc13xxx_driver);
+	int i2cret;
+	int spiret;
+	i2cret = i2c_add_driver(&mc13xxx_i2c_driver);
+	spiret = spi_register_driver(&mc13xxx_driver);
+
+	return (i2cret == 0) && (spiret == 0);
 }
 subsys_initcall(mc13xxx_init);
 
 static void __exit mc13xxx_exit(void)
 {
+	i2c_del_driver(&mc13xxx_i2c_driver);
 	spi_unregister_driver(&mc13xxx_driver);
 }
 module_exit(mc13xxx_exit);
-- 
1.7.1

[PATCHv2] mc13xxx: Add i2c support for mc13xxx-core

[Uwe Kleine-König]

Hello Marc,

On Thu, Dec 16, 2010 at 08:47:55AM +1100, Marc Reilly wrote:
> Signed-off-by: Marc Reilly <marc@cpdesign.com.au>
> ---
>  drivers/mfd/mc13xxx-core.c |  331 +++++++++++++++++++++++++++++++-------------
>  1 files changed, 236 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index a2ac2ed..d355433 100644
> --- a/drivers/mfd/mc13xxx-core.c
> +++ b/drivers/mfd/mc13xxx-core.c
> @@ -9,7 +9,6 @@
>   * 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>
> @@ -18,12 +17,28 @@
>  #include <linux/spi/spi.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/mc13xxx.h>
> +#include <linux/i2c.h>
> +
> +enum mc13xxx_id {
> +	MC13XXX_ID_MC13783,
> +	MC13XXX_ID_MC13892,
> +	MC13XXX_ID_INVALID,
> +};
>  
>  struct mc13xxx {
> -	struct spi_device *spidev;
> +	union {
> +		struct spi_device *spidev;
> +		struct i2c_client *i2cclient;
> +	};
> +	struct device *pdev;
pdev is a usual name for 'struct platform_device's, so can you please
use 'dev'?

> +	enum mc13xxx_id ictype;
> +
>  	struct mutex lock;
>  	int irq;
Before your patch this member was unused.  And AFAICT you don't really
need it.  (Whenever you need the number contained in it, you are in an
spi or i2c specific function so you can use spidev->irq or
i2cclient->irq.)  So can you please just drop this member?

>  
> +	int (*read_dev)(struct mc13xxx*, unsigned int, u32*);
> +	int (*write_dev)(struct mc13xxx*, unsigned int, u32);
space between type and asterisk?

> +
>  	irq_handler_t irqhandler[MC13XXX_NUM_IRQ];
>  	void *irqdata[MC13XXX_NUM_IRQ];
>  };
> @@ -150,36 +165,47 @@ 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->pdev, "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->pdev, "%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->pdev, "%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;
>  
> +	ret = mc13xxx->read_dev(mc13xxx, offset, val);
> +	dev_vdbg(mc13xxx->pdev, "[0x%02x] -> 0x%06x\n", offset, *val);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(mc13xxx_reg_read);
> +
> +#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));
> @@ -201,26 +227,42 @@ 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_i2c_reg_read(struct mc13xxx *mc13xxx, unsigned int offset,
> +		u32 *val)
>  {
> -	u32 buf;
> -	struct spi_transfer t;
> -	struct spi_message m;
>  	int ret;
> +	unsigned char buff[3] = {0, 0, 0};
buf is more usual I think

>  
> -	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
> +	ret = i2c_smbus_read_i2c_block_data(mc13xxx->i2cclient,
> +			offset, 3, buff);
> +	*val = buff[0] << 16 | buff[1] << 8 | buff[2];
>  
> -	dev_vdbg(&mc13xxx->spidev->dev, "[0x%02x] <- 0x%06x\n", offset, val);
> +	return ret == 3 ? 0 : ret;
> +}
> +
> +int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val)
> +{
> +	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
> +	dev_vdbg(mc13xxx->pdev, "[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);
> +
> +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));
> @@ -241,7 +283,22 @@ int mc13xxx_reg_write(struct mc13xxx *mc13xxx, unsigned int offset, u32 val)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL(mc13xxx_reg_write);
> +
> +static int mc13xxx_i2c_reg_write(struct mc13xxx *mc13xxx, unsigned int offset,
> +		u32 val)
> +{
> +	int ret;
> +	unsigned char buff[3];
> +
> +	buff[0] = (val >> 16) & 0xff;
> +	buff[1] = (val >> 8) & 0xff;
> +	buff[2] = val & 0xff;
ditto

> +
> +	ret = i2c_smbus_write_i2c_block_data(mc13xxx->i2cclient,
> +			offset, 3, buff);
> +
> +	return ret;
> +}
>  
>  int mc13xxx_reg_rmw(struct mc13xxx *mc13xxx, unsigned int offset,
>  		u32 mask, u32 val)
> @@ -445,7 +502,7 @@ static int mc13xxx_irq_handle(struct mc13xxx *mc13xxx,
>  			if (handled == IRQ_HANDLED)
>  				num_handled++;
>  		} else {
> -			dev_err(&mc13xxx->spidev->dev,
> +			dev_err(mc13xxx->pdev,
>  					"BUG: irq %u but no handler\n",
>  					baseirq + irq);
>  
> @@ -481,11 +538,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 +545,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.
> +	 * */
please put /* and */ on lines for themself, see chapter 8 in
Documentation/CodingStyle

>  	ret = mc13xxx_reg_read(mc13xxx, 46, &icid);
>  	if (ret)
>  		return ret;
> @@ -508,26 +563,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->pdev, "%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 +587,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;
>  }
>  
>  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 +600,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->pdev);
>  
>  	return pdata->flags;
>  }
> @@ -601,7 +638,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->pdev, "%s\n", __func__);
>  
>  	mc13xxx_lock(mc13xxx);
>  
> @@ -643,7 +680,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.pdev, "%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 +738,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->pdev, -1, &cell, 1, NULL, 0);
>  }
>  
>  static int mc13xxx_add_subdevice(struct mc13xxx *mc13xxx, const char *format)
> @@ -709,11 +746,44 @@ 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)
> +static int mc13xxx_common_probe(struct mc13xxx *mc13xxx,
> +				struct mc13xxx_platform_data *pdata)
> +{
> +	if (pdata->flags & MC13XXX_USE_ADC)
> +		mc13xxx_add_subdevice(mc13xxx, "%s-adc");
> +
> +	if (pdata->flags & MC13XXX_USE_CODEC)
> +		mc13xxx_add_subdevice(mc13xxx, "%s-codec");
> +
> +	if (pdata->flags & MC13XXX_USE_REGULATOR) {
> +		struct mc13xxx_regulator_platform_data regulator_pdata = {
> +			.num_regulators = pdata->num_regulators,
> +			.regulators = pdata->regulators,
> +		};
> +
> +		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-regulator",
> +				&regulator_pdata, sizeof(regulator_pdata));
> +	}
> +
> +	if (pdata->flags & MC13XXX_USE_RTC)
> +		mc13xxx_add_subdevice(mc13xxx, "%s-rtc");
> +
> +	if (pdata->flags & MC13XXX_USE_TOUCHSCREEN)
> +		mc13xxx_add_subdevice(mc13xxx, "%s-ts");
> +
> +	if (pdata->flags & MC13XXX_USE_LED) {
> +		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led",
> +					pdata->leds, sizeof(*pdata->leds));
> +	}
> +
> +	return 0;
> +}
> +
> +static int mc13xxx_spi_probe(struct spi_device *spi)
>  {
>  	struct mc13xxx *mc13xxx;
>  	struct mc13xxx_platform_data *pdata = dev_get_platdata(&spi->dev);
> -	enum mc13xxx_id id;
> +	const struct spi_device_id *devid;
>  	int ret;
>  
>  	mc13xxx = kzalloc(sizeof(*mc13xxx), GFP_KERNEL);
> @@ -725,15 +795,24 @@ static int mc13xxx_probe(struct spi_device *spi)
>  	spi->bits_per_word = 32;
>  	spi_setup(spi);
>  
> +	mc13xxx->pdev = &spi->dev;
>  	mc13xxx->spidev = spi;
> +	mc13xxx->read_dev = mc13xxx_spi_reg_read;
> +	mc13xxx->write_dev = mc13xxx_spi_reg_write;
> +	mc13xxx->irq = spi->irq;
>  
>  	mutex_init(&mc13xxx->lock);
>  	mc13xxx_lock(mc13xxx);
>  
> -	ret = mc13xxx_identify(mc13xxx, &id);
> -	if (ret || id == MC13XXX_ID_INVALID)
> +	ret = mc13xxx_identify(mc13xxx);
> +	if (ret || (mc13xxx->ictype == MC13XXX_ID_INVALID))
>  		goto err_revision;
>  
> +	devid = spi_get_device_id(mc13xxx->spidev);
> +	if (!devid || devid->driver_data != mc13xxx->ictype)
> +		dev_warn(mc13xxx->pdev, "device id doesn't "
> +					"match auto detection!\n");
Don't linebreak error strings please.

> +
>  	/* mask all irqs */
>  	ret = mc13xxx_reg_write(mc13xxx, MC13XXX_IRQMASK0, 0x00ffffff);
>  	if (ret)
> @@ -743,7 +822,7 @@ 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(mc13xxx->irq, NULL, mc13xxx_irq_thread,
>  			IRQF_ONESHOT | IRQF_TRIGGER_HIGH, "mc13xxx", mc13xxx);
>  
>  	if (ret) {
> @@ -757,41 +836,15 @@ err_revision:
>  
>  	mc13xxx_unlock(mc13xxx);
>  
> -	if (pdata->flags & MC13XXX_USE_ADC)
> -		mc13xxx_add_subdevice(mc13xxx, "%s-adc");
> -
> -	if (pdata->flags & MC13XXX_USE_CODEC)
> -		mc13xxx_add_subdevice(mc13xxx, "%s-codec");
> -
> -	if (pdata->flags & MC13XXX_USE_REGULATOR) {
> -		struct mc13xxx_regulator_platform_data regulator_pdata = {
> -			.num_regulators = pdata->num_regulators,
> -			.regulators = pdata->regulators,
> -		};
> -
> -		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-regulator",
> -				&regulator_pdata, sizeof(regulator_pdata));
> -	}
> -
> -	if (pdata->flags & MC13XXX_USE_RTC)
> -		mc13xxx_add_subdevice(mc13xxx, "%s-rtc");
> -
> -	if (pdata->flags & MC13XXX_USE_TOUCHSCREEN)
> -		mc13xxx_add_subdevice(mc13xxx, "%s-ts");
> -
> -	if (pdata->flags & MC13XXX_USE_LED) {
> -		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led",
> -					pdata->leds, sizeof(*pdata->leds));
> -	}
> -
> -	return 0;
> +	return mc13xxx_common_probe(mc13xxx, pdata);
>  }
>  
> -static int __devexit mc13xxx_remove(struct spi_device *spi)
> +
> +static int __devexit mc13xxx_spi_remove(struct spi_device *spi)
>  {
>  	struct mc13xxx *mc13xxx = dev_get_drvdata(&spi->dev);
>  
> -	free_irq(mc13xxx->spidev->irq, mc13xxx);
> +	free_irq(mc13xxx->irq, mc13xxx);
>  
>  	mfd_remove_devices(&spi->dev);
>  
> @@ -819,18 +872,106 @@ static struct spi_driver mc13xxx_driver = {
>  		.bus = &spi_bus_type,
>  		.owner = THIS_MODULE,
>  	},
> -	.probe = mc13xxx_probe,
> -	.remove = __devexit_p(mc13xxx_remove),
> +	.probe = mc13xxx_spi_probe,
> +	.remove = __devexit_p(mc13xxx_spi_remove),
> +};
> +
> +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->pdev = &client->dev;
> +	mc13xxx->i2cclient = client;
> +	mc13xxx->read_dev = mc13xxx_i2c_reg_read;
> +	mc13xxx->write_dev = mc13xxx_i2c_reg_write;
> +	mc13xxx->irq = client->irq;
I think that starting here the rest of the function can go to
common_probe, doesn't it?  (OK, apart from the irq number, that would
then go as a parameter to common_probe.)

> +
> +	mutex_init(&mc13xxx->lock);
> +	mc13xxx_lock(mc13xxx);
> +
> +	ret = mc13xxx_identify(mc13xxx);
> +	if (ret || (mc13xxx->ictype == MC13XXX_ID_INVALID))
> +		goto err_revision;
> +
> +	if (id->driver_data != mc13xxx->ictype)
> +		dev_warn(mc13xxx->pdev, "device id doesn't "
> +					"match auto detection!\n");
linebroken error string

> +
> +	/* mask all irqs */
> +	ret = mc13xxx_reg_write(mc13xxx, MC13XXX_IRQMASK0, 0x00ffffff);
> +	if (ret)
> +		goto err_mask;
> +
> +	ret = mc13xxx_reg_write(mc13xxx, MC13XXX_IRQMASK1, 0x00ffffff);
> +	if (ret)
> +		goto err_mask;
> +
> +	ret = request_threaded_irq(mc13xxx->irq, NULL, mc13xxx_irq_thread,
> +			IRQF_ONESHOT | IRQF_TRIGGER_HIGH, "mc13xxx", mc13xxx);
> +
> +	if (ret) {
> +err_mask:
> +err_revision:
> +		mutex_unlock(&mc13xxx->lock);
> +		kfree(mc13xxx);
> +		return ret;
> +	}
> +
> +	mc13xxx_unlock(mc13xxx);
> +
> +	return mc13xxx_common_probe(mc13xxx, pdata);
> +}
> +
> +static int __devexit mc13xxx_i2c_remove(struct i2c_client *client)
> +{
> +	struct mc13xxx *mc13xxx = dev_get_drvdata(&client->dev);
> +
> +	free_irq(mc13xxx->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_init(void)
>  {
> -	return spi_register_driver(&mc13xxx_driver);
> +	int i2cret;
> +	int spiret;
> +	i2cret = i2c_add_driver(&mc13xxx_i2c_driver);
> +	spiret = spi_register_driver(&mc13xxx_driver);
> +
> +	return (i2cret == 0) && (spiret == 0);
If i2c_add_driver succeeds but spi_register_driver you return
-ESOMETHING, and the module is discarded while the i2c core keeps a
reference to &mc13xxx_i2c_driver.  This is bad.
I think you need to do:

	ret = i2c_add_driver(..)
	if (ret)
		return ret;

	ret = spi_register_driver(...)
	if (ret)
		i2c_del_driver(...)

	return ret;

Best regards
Uwe

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

[PATCHv2] mc13xxx: Add i2c support for mc13xxx-core

[Marc Reilly]

Hi Uwe,

Thanks for your comments. I have a couple of questions, but agree with all 
your comments on the whole.

> > 
> > -	struct spi_device *spidev;
> > +	union {
> > +		struct spi_device *spidev;
> > +		struct i2c_client *i2cclient;
> > +	};
> > +	struct device *pdev;
> 
> pdev is a usual name for 'struct platform_device's, so can you please
> use 'dev'?

no worries. 
(My old habit of putting a 'p' in front of every pointer name. But let's leave 
that there.. :)

> 
> > +	enum mc13xxx_id ictype;
> > +
> > 
> >  	struct mutex lock;
> >  	int irq;
> 
> Before your patch this member was unused.  And AFAICT you don't really
> need it.  (Whenever you need the number contained in it, you are in an
> spi or i2c specific function so you can use spidev->irq or
> i2cclient->irq.)  So can you please just drop this member?

no worries. (Last round of comments you suggested to do in a different patch).

> 
> > +	int (*read_dev)(struct mc13xxx*, unsigned int, u32*);
> > +	int (*write_dev)(struct mc13xxx*, unsigned int, u32);
> 
> space between type and asterisk?

Not sure which asterisk you're talking about. (Checkpatch.pl resulted in no 
errors/warnings, so I'm ignorant past that)


> > 
> > +	unsigned char buff[3] = {0, 0, 0};
> 
> buf is more usual I think

no worries.


> > 
> > +	/* 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.
> > +	 * */
> 
> please put /* and */ on lines for themself, see chapter 8 in
> Documentation/CodingStyle

no worries. (have read, just forget :)



> > +	devid = spi_get_device_id(mc13xxx->spidev);
> > +	if (!devid || devid->driver_data != mc13xxx->ictype)
> > +		dev_warn(mc13xxx->pdev, "device id doesn't "
> > +					"match auto detection!\n");
> 
> Don't linebreak error strings please.

no worries.


> > +
> > +	dev_set_drvdata(&client->dev, mc13xxx);
> > +	mc13xxx->pdev = &client->dev;
> > +	mc13xxx->i2cclient = client;
> > +	mc13xxx->read_dev = mc13xxx_i2c_reg_read;
> > +	mc13xxx->write_dev = mc13xxx_i2c_reg_write;
> > +	mc13xxx->irq = client->irq;
> 
> I think that starting here the rest of the function can go to
> common_probe, doesn't it?  (OK, apart from the irq number, that would
> then go as a parameter to common_probe.)

agree.


> 
> > +
> > +	mutex_init(&mc13xxx->lock);
> > +	mc13xxx_lock(mc13xxx);
> > +
> > +	ret = mc13xxx_identify(mc13xxx);
> > +	if (ret || (mc13xxx->ictype == MC13XXX_ID_INVALID))
> > +		goto err_revision;
> > +
> > +	if (id->driver_data != mc13xxx->ictype)
> > +		dev_warn(mc13xxx->pdev, "device id doesn't "
> > +					"match auto detection!\n");
> 
> linebroken error string

no worries.


> >  static int __init mc13xxx_init(void)
> >  {
> > 
> > -	return spi_register_driver(&mc13xxx_driver);
> > +	int i2cret;
> > +	int spiret;
> > +	i2cret = i2c_add_driver(&mc13xxx_i2c_driver);
> > +	spiret = spi_register_driver(&mc13xxx_driver);
> > +
> > +	return (i2cret == 0) && (spiret == 0);
> 
> If i2c_add_driver succeeds but spi_register_driver you return
> -ESOMETHING, and the module is discarded while the i2c core keeps a
> reference to &mc13xxx_i2c_driver.  This is bad.
> I think you need to do:
> 
> 	ret = i2c_add_driver(..)
> 	if (ret)
> 		return ret;
> 
> 	ret = spi_register_driver(...)
> 	if (ret)
> 		i2c_del_driver(...)
> 
> 	return ret;
> 

This was my biggest reservation with this code too. I think it should try to 
register both, but should clean appropriately if one fails. (What to return 
from mc13xxx_init() in that case? Or maybe should it be split into separate 
files?).

Cheers
Marc

[PATCHv2] mc13xxx: Add i2c support for mc13xxx-core

[Uwe Kleine-König]

Hello Marc,

> > pdev is a usual name for 'struct platform_device's, so can you please
> > use 'dev'?
> 
> no worries. 
> (My old habit of putting a 'p' in front of every pointer name. But let's leave 
> that there.. :)
You're not working for Microsoft, are you?

> > > +	enum mc13xxx_id ictype;
> > > +
> > > 
> > >  	struct mutex lock;
> > >  	int irq;
> > 
> > Before your patch this member was unused.  And AFAICT you don't really
> > need it.  (Whenever you need the number contained in it, you are in an
> > spi or i2c specific function so you can use spidev->irq or
> > i2cclient->irq.)  So can you please just drop this member?
> 
> no worries. (Last round of comments you suggested to do in a different patch).
Both is fine IMHO.  If you do it in one patch you should just note it in
the commit log.

> > > +	int (*read_dev)(struct mc13xxx*, unsigned int, u32*);
> > > +	int (*write_dev)(struct mc13xxx*, unsigned int, u32);
> > 
> > space between type and asterisk?
> 
> Not sure which asterisk you're talking about. (Checkpatch.pl resulted in no 
> errors/warnings, so I'm ignorant past that)
I want
int (*read_dev)(struct mc13xxx *, unsigned int, u32 *);

> > >  static int __init mc13xxx_init(void)
> > >  {
> > > 
> > > -	return spi_register_driver(&mc13xxx_driver);
> > > +	int i2cret;
> > > +	int spiret;
> > > +	i2cret = i2c_add_driver(&mc13xxx_i2c_driver);
> > > +	spiret = spi_register_driver(&mc13xxx_driver);
> > > +
> > > +	return (i2cret == 0) && (spiret == 0);
> > 
> > If i2c_add_driver succeeds but spi_register_driver you return
> > -ESOMETHING, and the module is discarded while the i2c core keeps a
> > reference to &mc13xxx_i2c_driver.  This is bad.
> > I think you need to do:
> > 
> > 	ret = i2c_add_driver(..)
> > 	if (ret)
> > 		return ret;
> > 
> > 	ret = spi_register_driver(...)
> > 	if (ret)
> > 		i2c_del_driver(...)
> > 
> > 	return ret;
> > 
> 
> This was my biggest reservation with this code too. I think it should try to 
> register both, but should clean appropriately if one fails. (What to return 
> from mc13xxx_init() in that case? Or maybe should it be split into separate 
> files?).
Don't split, in general your approach is fine.  And there are two
possibilities to handle spi vs. i2c:

  a) (as suggested above) do both, if at least one fails, do none
     This is easier and probably OK.
  b) You could still succeed if at least one registration doesn't fail.
     Then you need to remember which was successful and only clean this
     one up in the remove callback.  You could then even depend on I2C
     || SPI.
     (BTW, did you fix the dependencies?  Currently you need to depend
     on I2C && SPI.  (Not sure these are the correct names, you may want
     to double check that.))
     This is more flexible but IMHO not worth the effort.

Best regards
Uwe

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

[PATCHv2] mc13xxx: Add i2c support for mc13xxx-core

[Wolfram Sang]

Hi guys,

>      (BTW, did you fix the dependencies?  Currently you need to depend
>      on I2C && SPI.  (Not sure these are the correct names, you may want
>      to double check that.))

Be careful, this is a subtle issue. You will get unresolved symbols if your
chip is connected via SPI, and <driver>=y, SPI=y, together with I2C=m. Been
there.

>      This is more flexible but IMHO not worth the effort.

I had to do it back then :( I wondered if there couldn't be a generic way to
deal with SPI+I2C-drivers, never got to investigate it, though.

Also, be sure to CC i2c- and spi-lists when you do the next version.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101216/46476a3c/attachment-0001.sig>

[PATCHv2] mc13xxx: Add i2c support for mc13xxx-core

[Marc Reilly]

Hi,

> >      (BTW, did you fix the dependencies?  Currently you need to depend
> >      on I2C && SPI.  (Not sure these are the correct names, you may want
> >      to double check that.))
> 
> Be careful, this is a subtle issue. You will get unresolved symbols if your
> chip is connected via SPI, and <driver>=y, SPI=y, together with I2C=m. Been
> there.
> 
How quickly a "simple" patch gets complicated. :)

Am I right in thinking that in the above scenario (assuming it was coerced 
into compiling) the mc13xxx i2c driver would never get registered even after 
the i2c module is insmod'ed?

Sounds like the only way to handle this is by splitting the spi and i2c... 

> >      This is more flexible but IMHO not worth the effort.
> 
> I had to do it back then :( I wondered if there couldn't be a generic way
> to deal with SPI+I2C-drivers, never got to investigate it, though.
> 

Looking at some of the other mfd drivers, it seems that's the way they've gone 
too.

> Also, be sure to CC i2c- and spi-lists when you do the next version.

Will do.

Cheers,
Marc