[PATCH] i2c/busses: Add support for Aeroflex Gaisler I2CMST controller

[PATCH] i2c/busses: Add support for Aeroflex Gaisler I2CMST controller

[Jan Andersson]

This patch adds support for the I2CMST core found on LEON/GRLIB SoCs.

Signed-off-by: Jan Andersson <jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>
---
The I2CMST core is basically the OpenCores I2C master with an AMBA APB
interface. This driver re-uses much of i2c-ocores.c. It is submitted as
a separate driver since the register interfaces differ sligthly. Also the
two IP cores are maintained separately so they may diverge further in
the future.

The driver is identical in terms of transfer handling and HW control.
The original module author string has been kept.

 drivers/i2c/busses/Kconfig         |   10 +
 drivers/i2c/busses/Makefile        |    1 +
 drivers/i2c/busses/i2c-gr-i2cmst.c |  371 ++++++++++++++++++++++++++++++++++++
 3 files changed, 382 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-gr-i2cmst.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 113505a..06a3150 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -646,6 +646,16 @@ config I2C_EG20T
           is an IOH(Input/Output Hub) for x86 embedded processor.
           This driver can access PCH I2C bus device.
 
+config I2C_GR_I2CMST
+	tristate "Aeroflex Gaisler I2CMST I2C Controller"
+	depends on SPARC_LEON
+	help
+	  If you say yes to this option, support will be included for the
+	  I2CMST I2C controller present on some LEON/GRLIB SoCs.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-gr-i2cmst.
+
 comment "External I2C/SMBus adapter drivers"
 
 config I2C_PARPORT
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 9d2d0ec..1e16e66 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
 obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
 obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
 obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
+obj-$(CONFIG_I2C_GR_I2CMST)	+= i2c-gr-i2cmst.o
 
 # External I2C/SMBus adapter drivers
 obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
diff --git a/drivers/i2c/busses/i2c-gr-i2cmst.c b/drivers/i2c/busses/i2c-gr-i2cmst.c
new file mode 100644
index 0000000..90b23a8
--- /dev/null
+++ b/drivers/i2c/busses/i2c-gr-i2cmst.c
@@ -0,0 +1,371 @@
+/*
+ * i2c-gr-i2cmst.c I2C bus driver for GRLIB I2CMST core
+ *
+ * Main parts of this driver re-used from:
+ *
+ * i2c-ocores.c: I2C bus driver for OpenCores I2C controller
+ * by Peter Korsgaard <jacmet-OfajU3CKLf1/SzgSGea1oA@public.gmane.org>
+ *
+ * Adaption to GRLIB/I2CMST by Jan Andersson <jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2.  This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/wait.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+
+/* I2CMST APB registers */
+struct i2cmst_regs {
+	u32 prescaler;
+	u32 control;
+	u32 data;
+	u32 cmdstat; /* Command (write), Status (read) */
+};
+
+struct gr_i2cmst_i2c {
+	struct i2cmst_regs *regs;
+	wait_queue_head_t wait;
+	struct i2c_adapter adap;
+	struct i2c_msg *msg;
+	int pos;
+	int nmsgs;
+	int state; /* see STATE_ */
+	int clock_khz;
+};
+
+/* register fields and commands */
+#define I2CMST_CTRL_IEN		0x40
+#define I2CMST_CTRL_EN		0x80
+
+#define I2CMST_CMD_START	0x91
+#define I2CMST_CMD_STOP		0x41
+#define I2CMST_CMD_READ		0x21
+#define I2CMST_CMD_WRITE	0x11
+#define I2CMST_CMD_READ_ACK	0x21
+#define I2CMST_CMD_READ_NACK	0x29
+#define I2CMST_CMD_IACK		0x01
+
+#define I2CMST_STAT_IF		0x01
+#define I2CMST_STAT_TIP		0x02
+#define I2CMST_STAT_ARBLOST	0x20
+#define I2CMST_STAT_BUSY	0x40
+#define I2CMST_STAT_NACK	0x80
+
+#define STATE_DONE		0
+#define STATE_START		1
+#define STATE_WRITE		2
+#define STATE_READ		3
+#define STATE_ERROR		4
+
+static inline void i2cmst_setreg(void __iomem *reg, u32 value)
+{
+	__raw_writel(cpu_to_be32(value), reg);
+}
+
+static inline u32 i2cmst_getreg(void __iomem *reg)
+{
+	return be32_to_cpu(__raw_readl(reg));
+}
+
+static void gr_i2cmst_process(struct gr_i2cmst_i2c *i2c)
+{
+	struct i2c_msg *msg = i2c->msg;
+	u32 stat = i2cmst_getreg(&i2c->regs->cmdstat);
+
+	if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) {
+		/* stop has been sent */
+		i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_IACK);
+		wake_up(&i2c->wait);
+		return;
+	}
+
+	/* error? */
+	if (stat & I2CMST_STAT_ARBLOST) {
+		i2c->state = STATE_ERROR;
+		i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP);
+		return;
+	}
+
+	if ((i2c->state == STATE_START) || (i2c->state == STATE_WRITE)) {
+		i2c->state =
+			(msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE;
+
+		if (stat & I2CMST_STAT_NACK) {
+			i2c->state = STATE_ERROR;
+			i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP);
+			return;
+		}
+	} else
+		msg->buf[i2c->pos++] = i2cmst_getreg(&i2c->regs->data);
+
+	/* end of msg? */
+	if (i2c->pos == msg->len) {
+		i2c->nmsgs--;
+		i2c->msg++;
+		i2c->pos = 0;
+		msg = i2c->msg;
+
+		if (i2c->nmsgs) {	/* end? */
+			/* send start? */
+			if (!(msg->flags & I2C_M_NOSTART)) {
+				u8 addr = (msg->addr << 1);
+
+				if (msg->flags & I2C_M_RD)
+					addr |= 1;
+
+				i2c->state = STATE_START;
+
+				i2cmst_setreg(&i2c->regs->data, addr);
+				i2cmst_setreg(&i2c->regs->cmdstat,
+					I2CMST_CMD_START);
+				return;
+			} else
+				i2c->state = (msg->flags & I2C_M_RD)
+					? STATE_READ : STATE_WRITE;
+		} else {
+			i2c->state = STATE_DONE;
+			i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP);
+			return;
+		}
+	}
+
+	if (i2c->state == STATE_READ) {
+		i2cmst_setreg(&i2c->regs->cmdstat, i2c->pos == (msg->len-1) ?
+			I2CMST_CMD_READ_NACK : I2CMST_CMD_READ_ACK);
+	} else {
+		i2cmst_setreg(&i2c->regs->data, msg->buf[i2c->pos++]);
+		i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_WRITE);
+	}
+}
+
+static irqreturn_t gr_i2cmst_isr(int irq, void *dev_id)
+{
+	struct gr_i2cmst_i2c *i2c = dev_id;
+
+	gr_i2cmst_process(i2c);
+
+	return IRQ_HANDLED;
+}
+
+static int gr_i2cmst_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct gr_i2cmst_i2c *i2c = i2c_get_adapdata(adap);
+
+	i2c->msg = msgs;
+	i2c->pos = 0;
+	i2c->nmsgs = num;
+	i2c->state = STATE_START;
+
+	i2cmst_setreg(&i2c->regs->data, (i2c->msg->addr << 1) |
+			((i2c->msg->flags & I2C_M_RD) ? 1 : 0));
+
+	i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_START);
+
+	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
+			       (i2c->state == STATE_DONE), HZ))
+		return (i2c->state == STATE_DONE) ? num : -EIO;
+	else
+		return -ETIMEDOUT;
+}
+
+static void gr_i2cmst_init(struct gr_i2cmst_i2c *i2c)
+{
+	int prescale;
+	u32 ctrl = i2cmst_getreg(&i2c->regs->control);
+
+	/* make sure the device is disabled */
+	i2cmst_setreg(&i2c->regs->control,
+			ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN));
+
+	/* Required prescaler = (AMBAfreq)/(5 * SCLfreq)-1 */
+	prescale = (i2c->clock_khz / (5*100)) - 1;
+	/* Minimum value of precaler is 3 due to HW sync */
+	prescale = prescale < 3 ? 3 : prescale;
+	i2cmst_setreg(&i2c->regs->prescaler, prescale);
+
+	/* Init the device */
+	i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_IACK);
+	ctrl |= I2CMST_CTRL_IEN | I2CMST_CTRL_EN;
+	i2cmst_setreg(&i2c->regs->control, ctrl);
+}
+
+
+static u32 gr_i2cmst_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm gr_i2cmst_algorithm = {
+	.master_xfer	= gr_i2cmst_xfer,
+	.functionality	= gr_i2cmst_func,
+};
+
+static struct i2c_adapter gr_i2cmst_adapter = {
+	.owner		= THIS_MODULE,
+	.name		= "i2c-gr-i2cmst",
+	.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
+	.algo		= &gr_i2cmst_algorithm,
+};
+
+
+static int __devinit gr_i2cmst_of_probe(struct platform_device *ofdev, const struct of_device_id *match)
+{
+	struct gr_i2cmst_i2c *i2c;
+	const unsigned int *busfreq;
+	int ret;
+
+	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	/* Get frequency of APB bus that core is attached to */
+	busfreq = of_get_property(ofdev->dev.of_node, "freq", NULL);
+	if (!busfreq) {
+		dev_err(&ofdev->dev, "Missing parameter 'freq'\n");
+		return -ENODEV;
+	}
+	i2c->clock_khz = *busfreq/1000;
+
+	i2c->regs = of_ioremap(&ofdev->resource[0], 0,
+			resource_size(&ofdev->resource[0]),
+			"grlib-i2cmst regs");
+	if (!i2c->regs) {
+		dev_err(&ofdev->dev, "Unable to map registers\n");
+		ret = -EIO;
+		goto map_failed;
+	}
+
+	gr_i2cmst_init(i2c);
+
+	init_waitqueue_head(&i2c->wait);
+	ret = request_irq(ofdev->archdata.irqs[0], gr_i2cmst_isr, 0,
+			ofdev->name, i2c);
+	if (ret) {
+		dev_err(&ofdev->dev, "Cannot claim IRQ\n");
+		goto request_irq_failed;
+	}
+
+	/* hook up driver to tree */
+	dev_set_drvdata(&ofdev->dev, i2c);
+	i2c->adap = gr_i2cmst_adapter;
+	i2c_set_adapdata(&i2c->adap, i2c);
+	i2c->adap.dev.parent = &ofdev->dev;
+	i2c->adap.dev.of_node = ofdev->dev.of_node;
+
+	/* add i2c adapter to i2c tree */
+	ret = i2c_add_adapter(&i2c->adap);
+	if (ret) {
+		dev_err(&ofdev->dev, "Failed to add adapter\n");
+		goto add_adapter_failed;
+	}
+
+	return 0;
+
+add_adapter_failed:
+	free_irq(ofdev->archdata.irqs[0], i2c);
+request_irq_failed:
+	of_iounmap(&ofdev->resource[0], i2c->regs,
+		resource_size(&ofdev->resource[0]));
+map_failed:
+	kfree(i2c);
+
+	return ret;
+}
+
+static int __devexit gr_i2cmst_of_remove(struct platform_device *ofdev)
+{
+	struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev);
+	u32 ctrl = i2cmst_getreg(&i2c->regs->control);
+
+	/* disable i2c logic */
+	i2cmst_setreg(&i2c->regs->control,
+		ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN));
+
+	/* remove adapter & data */
+	i2c_del_adapter(&i2c->adap);
+	dev_set_drvdata(&ofdev->dev, NULL);
+
+	free_irq(ofdev->archdata.irqs[0], i2c);
+
+	of_iounmap(&ofdev->resource[0], i2c->regs,
+		resource_size(&ofdev->resource[0]));
+
+	kfree(i2c);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int gr_i2cmst_suspend(struct platform_device *ofdev, pm_message_t state)
+{
+	struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev);
+	u32 ctrl = gr_i2cmst_getreg(&i2c->regs->control);
+
+	/* make sure the device is disabled */
+	gr_i2cmst_setreg(&i2c->regs->control,
+			ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN));
+
+	return 0;
+}
+
+static int gr_i2cmst_resume(struct platform_device *ofdev)
+{
+	struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev);
+
+	gr_i2cmst_init(i2c);
+
+	return 0;
+}
+#else
+#define gr_i2cmst_suspend	NULL
+#define gr_i2cmst_resume	NULL
+#endif
+
+static struct of_device_id gr_i2cmst_of_match[] = {
+	{ .name = "GAISLER_I2CMST", },
+	{ .name = "01_028", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, gr_i2cmst_of_match);
+
+static struct of_platform_driver gr_i2cmst_of_driver = {
+	.driver = {
+		.name = "grlib-i2cmst",
+		.owner = THIS_MODULE,
+		.of_match_table = gr_i2cmst_of_match,
+	},
+	.probe = gr_i2cmst_of_probe,
+	.remove = __devexit_p(gr_i2cmst_of_remove),
+	.suspend = gr_i2cmst_suspend,
+	.resume  = gr_i2cmst_resume,
+};
+
+
+static int __init i2cmst_init(void)
+{
+	return of_register_platform_driver(&gr_i2cmst_of_driver);
+}
+
+static void __exit i2cmst_exit(void)
+{
+	of_unregister_platform_driver(&gr_i2cmst_of_driver);
+}
+
+module_init(i2cmst_init);
+module_exit(i2cmst_exit);
+
+MODULE_AUTHOR("Peter Korsgaard <jacmet-OfajU3CKLf1/SzgSGea1oA@public.gmane.org>");
+MODULE_DESCRIPTION("GRLIB I2CMST I2C bus driver");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

Re: [PATCH] i2c/busses: Add support for Aeroflex Gaisler I2CMST controller

[Wolfram Sang]

Hi Jan,

On Wed, Feb 16, 2011 at 01:30:48PM +0100, Jan Andersson wrote:
> This patch adds support for the I2CMST core found on LEON/GRLIB SoCs.
> 
> Signed-off-by: Jan Andersson <jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>
> ---
> The I2CMST core is basically the OpenCores I2C master with an AMBA APB
> interface. This driver re-uses much of i2c-ocores.c. It is submitted as
> a separate driver since the register interfaces differ sligthly. Also the
> two IP cores are maintained separately so they may diverge further in
> the future.
> 
> The driver is identical in terms of transfer handling and HW control.
> The original module author string has been kept.

Just judging from this message, it seems that algo could be extracted and used
by both drivers? Or would this make no sense?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] i2c/busses: Add support for Aeroflex Gaisler I2CMST controller

[Jan Andersson]

On 02/16/2011 03:27 PM, Wolfram Sang wrote:
> Hi Jan,
>
> On Wed, Feb 16, 2011 at 01:30:48PM +0100, Jan Andersson wrote:
>> This patch adds support for the I2CMST core found on LEON/GRLIB SoCs.
>>
>> Signed-off-by: Jan Andersson<jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>
>> ---
>> The I2CMST core is basically the OpenCores I2C master with an AMBA APB
>> interface. This driver re-uses much of i2c-ocores.c. It is submitted as
>> a separate driver since the register interfaces differ sligthly. Also the
>> two IP cores are maintained separately so they may diverge further in
>> the future.
>>
>> The driver is identical in terms of transfer handling and HW control.
>> The original module author string has been kept.
>
> Just judging from this message, it seems that algo could be extracted and used
> by both drivers? Or would this make no sense?
>

Hi Wolfram,

The control parts of the *_process, *_isr and *_xfer functions could be 
shared by ripping it out and changing the calls used to read/set registers.

I considered that change more complex and did not think it to be worth 
the trouble for saving ~50 LOC (probably less with added glue). Of 
course, it could be beneficial to have the algo part shared if there are 
future updates to it. At the same time I suppose that it could be bad to 
have the algo part shared if the updates are only valid for one of the 
cores.

Best regards,
   Jan

Re: [PATCH] i2c/busses: Add support for Aeroflex Gaisler I2CMST controller

[Wolfram Sang]

Jan,

> I considered that change more complex and did not think it to be
> worth the trouble for saving ~50 LOC (probably less with added
> glue). Of course, it could be beneficial to have the algo part
> shared if there are future updates to it. At the same time I suppose

Or another core pops up using the same algorithm.

> that it could be bad to have the algo part shared if the updates are
> only valid for one of the cores.

Well, for now it really seems it won't matter much. Plus, we could rip it out
later if needed. OK, thanks for answering.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] i2c/busses: Add support for Aeroflex Gaisler I2CMST controller

[Ben Dooks]

On Wed, Feb 16, 2011 at 01:30:48PM +0100, Jan Andersson wrote:
> This patch adds support for the I2CMST core found on LEON/GRLIB SoCs.
> 
> Signed-off-by: Jan Andersson <jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>
> ---
> The I2CMST core is basically the OpenCores I2C master with an AMBA APB
> interface. This driver re-uses much of i2c-ocores.c. It is submitted as
> a separate driver since the register interfaces differ sligthly. Also the
> two IP cores are maintained separately so they may diverge further in
> the future.

Hmm, some more of that should go above the "---" line.

Going to do a quick review and then get back to this once I've had some
time to think on it.
 
> The driver is identical in terms of transfer handling and HW control.
> The original module author string has been kept.
> 
>  drivers/i2c/busses/Kconfig         |   10 +
>  drivers/i2c/busses/Makefile        |    1 +
>  drivers/i2c/busses/i2c-gr-i2cmst.c |  371 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 382 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-gr-i2cmst.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 113505a..06a3150 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -646,6 +646,16 @@ config I2C_EG20T
>            is an IOH(Input/Output Hub) for x86 embedded processor.
>            This driver can access PCH I2C bus device.
>  
> +config I2C_GR_I2CMST
> +	tristate "Aeroflex Gaisler I2CMST I2C Controller"
> +	depends on SPARC_LEON
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  I2CMST I2C controller present on some LEON/GRLIB SoCs.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-gr-i2cmst.
> +
>  comment "External I2C/SMBus adapter drivers"
>  
>  config I2C_PARPORT
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 9d2d0ec..1e16e66 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
>  obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
>  obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
>  obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
> +obj-$(CONFIG_I2C_GR_I2CMST)	+= i2c-gr-i2cmst.o
>  
>  # External I2C/SMBus adapter drivers
>  obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
> diff --git a/drivers/i2c/busses/i2c-gr-i2cmst.c b/drivers/i2c/busses/i2c-gr-i2cmst.c
> new file mode 100644
> index 0000000..90b23a8
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-gr-i2cmst.c
> @@ -0,0 +1,371 @@
> +/*
> + * i2c-gr-i2cmst.c I2C bus driver for GRLIB I2CMST core
> + *
> + * Main parts of this driver re-used from:
> + *
> + * i2c-ocores.c: I2C bus driver for OpenCores I2C controller
> + * by Peter Korsgaard <jacmet-OfajU3CKLf1/SzgSGea1oA@public.gmane.org>
> + *
> + * Adaption to GRLIB/I2CMST by Jan Andersson <jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2.  This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + */
> +
> +#include <linux/of_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/wait.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +
> +/* I2CMST APB registers */
> +struct i2cmst_regs {
> +	u32 prescaler;
> +	u32 control;
> +	u32 data;
> +	u32 cmdstat; /* Command (write), Status (read) */
> +};

using structures for memory mapped registers makes me want to go
EURGH. the compiler is free to pack and order as it feels.

> +struct gr_i2cmst_i2c {
> +	struct i2cmst_regs *regs;
> +	wait_queue_head_t wait;
> +	struct i2c_adapter adap;
> +	struct i2c_msg *msg;
> +	int pos;
> +	int nmsgs;
> +	int state; /* see STATE_ */
> +	int clock_khz;
> +};
> +
> +/* register fields and commands */
> +#define I2CMST_CTRL_IEN		0x40
> +#define I2CMST_CTRL_EN		0x80
> +
> +#define I2CMST_CMD_START	0x91
> +#define I2CMST_CMD_STOP		0x41
> +#define I2CMST_CMD_READ		0x21
> +#define I2CMST_CMD_WRITE	0x11
> +#define I2CMST_CMD_READ_ACK	0x21
> +#define I2CMST_CMD_READ_NACK	0x29
> +#define I2CMST_CMD_IACK		0x01
> +
> +#define I2CMST_STAT_IF		0x01
> +#define I2CMST_STAT_TIP		0x02
> +#define I2CMST_STAT_ARBLOST	0x20
> +#define I2CMST_STAT_BUSY	0x40
> +#define I2CMST_STAT_NACK	0x80
> +
> +#define STATE_DONE		0
> +#define STATE_START		1
> +#define STATE_WRITE		2
> +#define STATE_READ		3
> +#define STATE_ERROR		4
> +
> +static inline void i2cmst_setreg(void __iomem *reg, u32 value)
> +{
> +	__raw_writel(cpu_to_be32(value), reg);
> +}
> +
> +static inline u32 i2cmst_getreg(void __iomem *reg)
> +{
> +	return be32_to_cpu(__raw_readl(reg));
> +}

do you really need to use the __raw versions of thees calls?

> +static void gr_i2cmst_process(struct gr_i2cmst_i2c *i2c)
> +{
> +	struct i2c_msg *msg = i2c->msg;
> +	u32 stat = i2cmst_getreg(&i2c->regs->cmdstat);
> +
> +	if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) {
> +		/* stop has been sent */
> +		i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_IACK);
> +		wake_up(&i2c->wait);
> +		return;
> +	}
> +
> +	/* error? */
> +	if (stat & I2CMST_STAT_ARBLOST) {
> +		i2c->state = STATE_ERROR;
> +		i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP);
> +		return;
> +	}
> +
> +	if ((i2c->state == STATE_START) || (i2c->state == STATE_WRITE)) {
> +		i2c->state =
> +			(msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE;
> +
> +		if (stat & I2CMST_STAT_NACK) {
> +			i2c->state = STATE_ERROR;
> +			i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP);
> +			return;
> +		}
> +	} else
> +		msg->buf[i2c->pos++] = i2cmst_getreg(&i2c->regs->data);
> +
> +	/* end of msg? */
> +	if (i2c->pos == msg->len) {
> +		i2c->nmsgs--;
> +		i2c->msg++;
> +		i2c->pos = 0;
> +		msg = i2c->msg;
> +
> +		if (i2c->nmsgs) {	/* end? */
> +			/* send start? */
> +			if (!(msg->flags & I2C_M_NOSTART)) {
> +				u8 addr = (msg->addr << 1);
> +
> +				if (msg->flags & I2C_M_RD)
> +					addr |= 1;
> +
> +				i2c->state = STATE_START;
> +
> +				i2cmst_setreg(&i2c->regs->data, addr);
> +				i2cmst_setreg(&i2c->regs->cmdstat,
> +					I2CMST_CMD_START);
> +				return;
> +			} else
> +				i2c->state = (msg->flags & I2C_M_RD)
> +					? STATE_READ : STATE_WRITE;
> +		} else {
> +			i2c->state = STATE_DONE;
> +			i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP);
> +			return;
> +		}
> +	}
> +
> +	if (i2c->state == STATE_READ) {
> +		i2cmst_setreg(&i2c->regs->cmdstat, i2c->pos == (msg->len-1) ?
> +			I2CMST_CMD_READ_NACK : I2CMST_CMD_READ_ACK);
> +	} else {
> +		i2cmst_setreg(&i2c->regs->data, msg->buf[i2c->pos++]);
> +		i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_WRITE);
> +	}
> +}
> +
> +static irqreturn_t gr_i2cmst_isr(int irq, void *dev_id)
> +{
> +	struct gr_i2cmst_i2c *i2c = dev_id;
> +
> +	gr_i2cmst_process(i2c);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int gr_i2cmst_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct gr_i2cmst_i2c *i2c = i2c_get_adapdata(adap);
> +
> +	i2c->msg = msgs;
> +	i2c->pos = 0;
> +	i2c->nmsgs = num;
> +	i2c->state = STATE_START;
> +
> +	i2cmst_setreg(&i2c->regs->data, (i2c->msg->addr << 1) |
> +			((i2c->msg->flags & I2C_M_RD) ? 1 : 0));
> +
> +	i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_START);
> +
> +	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> +			       (i2c->state == STATE_DONE), HZ))
> +		return (i2c->state == STATE_DONE) ? num : -EIO;
> +	else
> +		return -ETIMEDOUT;
> +}
> +
> +static void gr_i2cmst_init(struct gr_i2cmst_i2c *i2c)
> +{
> +	int prescale;
> +	u32 ctrl = i2cmst_getreg(&i2c->regs->control);
> +
> +	/* make sure the device is disabled */
> +	i2cmst_setreg(&i2c->regs->control,
> +			ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN));
> +
> +	/* Required prescaler = (AMBAfreq)/(5 * SCLfreq)-1 */
> +	prescale = (i2c->clock_khz / (5*100)) - 1;
> +	/* Minimum value of precaler is 3 due to HW sync */
> +	prescale = prescale < 3 ? 3 : prescale;
> +	i2cmst_setreg(&i2c->regs->prescaler, prescale);
> +
> +	/* Init the device */
> +	i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_IACK);
> +	ctrl |= I2CMST_CTRL_IEN | I2CMST_CTRL_EN;
> +	i2cmst_setreg(&i2c->regs->control, ctrl);
> +}
> +
> +
> +static u32 gr_i2cmst_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm gr_i2cmst_algorithm = {
> +	.master_xfer	= gr_i2cmst_xfer,
> +	.functionality	= gr_i2cmst_func,
> +};
> +
> +static struct i2c_adapter gr_i2cmst_adapter = {
> +	.owner		= THIS_MODULE,
> +	.name		= "i2c-gr-i2cmst",
> +	.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
> +	.algo		= &gr_i2cmst_algorithm,
> +};
> +
> +
> +static int __devinit gr_i2cmst_of_probe(struct platform_device *ofdev, const struct of_device_id *match)

please avoid line-wrap like this.

> +{
> +	struct gr_i2cmst_i2c *i2c;
> +	const unsigned int *busfreq;
> +	int ret;
> +
> +	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;

would be nice to have an error print here.

> +	/* Get frequency of APB bus that core is attached to */
> +	busfreq = of_get_property(ofdev->dev.of_node, "freq", NULL);
> +	if (!busfreq) {
> +		dev_err(&ofdev->dev, "Missing parameter 'freq'\n");
> +		return -ENODEV;
> +	}
> +	i2c->clock_khz = *busfreq/1000;
> +
> +	i2c->regs = of_ioremap(&ofdev->resource[0], 0,
> +			resource_size(&ofdev->resource[0]),
> +			"grlib-i2cmst regs");
> +	if (!i2c->regs) {
> +		dev_err(&ofdev->dev, "Unable to map registers\n");
> +		ret = -EIO;
> +		goto map_failed;
> +	}
> +
> +	gr_i2cmst_init(i2c);
> +
> +	init_waitqueue_head(&i2c->wait);
> +	ret = request_irq(ofdev->archdata.irqs[0], gr_i2cmst_isr, 0,
> +			ofdev->name, i2c);
> +	if (ret) {
> +		dev_err(&ofdev->dev, "Cannot claim IRQ\n");
> +		goto request_irq_failed;
> +	}
> +
> +	/* hook up driver to tree */
> +	dev_set_drvdata(&ofdev->dev, i2c);
> +	i2c->adap = gr_i2cmst_adapter;
> +	i2c_set_adapdata(&i2c->adap, i2c);
> +	i2c->adap.dev.parent = &ofdev->dev;
> +	i2c->adap.dev.of_node = ofdev->dev.of_node;
> +
> +	/* add i2c adapter to i2c tree */
> +	ret = i2c_add_adapter(&i2c->adap);
> +	if (ret) {
> +		dev_err(&ofdev->dev, "Failed to add adapter\n");
> +		goto add_adapter_failed;
> +	}
> +
> +	return 0;
> +
> +add_adapter_failed:
> +	free_irq(ofdev->archdata.irqs[0], i2c);
> +request_irq_failed:
> +	of_iounmap(&ofdev->resource[0], i2c->regs,
> +		resource_size(&ofdev->resource[0]));
> +map_failed:
> +	kfree(i2c);
> +
> +	return ret;
> +}
> +
> +static int __devexit gr_i2cmst_of_remove(struct platform_device *ofdev)
> +{
> +	struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev);
> +	u32 ctrl = i2cmst_getreg(&i2c->regs->control);
> +
> +	/* disable i2c logic */
> +	i2cmst_setreg(&i2c->regs->control,
> +		ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN));
> +
> +	/* remove adapter & data */
> +	i2c_del_adapter(&i2c->adap);
> +	dev_set_drvdata(&ofdev->dev, NULL);
> +
> +	free_irq(ofdev->archdata.irqs[0], i2c);
> +
> +	of_iounmap(&ofdev->resource[0], i2c->regs,
> +		resource_size(&ofdev->resource[0]));
> +
> +	kfree(i2c);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int gr_i2cmst_suspend(struct platform_device *ofdev, pm_message_t state)
> +{
> +	struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev);
> +	u32 ctrl = gr_i2cmst_getreg(&i2c->regs->control);
> +
> +	/* make sure the device is disabled */
> +	gr_i2cmst_setreg(&i2c->regs->control,
> +			ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN));
> +
> +	return 0;
> +}
> +
> +static int gr_i2cmst_resume(struct platform_device *ofdev)
> +{
> +	struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev);
> +
> +	gr_i2cmst_init(i2c);
> +
> +	return 0;
> +}
> +#else
> +#define gr_i2cmst_suspend	NULL
> +#define gr_i2cmst_resume	NULL
> +#endif
> +
> +static struct of_device_id gr_i2cmst_of_match[] = {
> +	{ .name = "GAISLER_I2CMST", },
> +	{ .name = "01_028", },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, gr_i2cmst_of_match);
> +
> +static struct of_platform_driver gr_i2cmst_of_driver = {
> +	.driver = {
> +		.name = "grlib-i2cmst",
> +		.owner = THIS_MODULE,
> +		.of_match_table = gr_i2cmst_of_match,
> +	},
> +	.probe = gr_i2cmst_of_probe,
> +	.remove = __devexit_p(gr_i2cmst_of_remove),
> +	.suspend = gr_i2cmst_suspend,
> +	.resume  = gr_i2cmst_resume,
> +};
> +
> +
> +static int __init i2cmst_init(void)
> +{
> +	return of_register_platform_driver(&gr_i2cmst_of_driver);
> +}
> +
> +static void __exit i2cmst_exit(void)
> +{
> +	of_unregister_platform_driver(&gr_i2cmst_of_driver);
> +}
> +
> +module_init(i2cmst_init);
> +module_exit(i2cmst_exit);
> +
> +MODULE_AUTHOR("Peter Korsgaard <jacmet-OfajU3CKLf1/SzgSGea1oA@public.gmane.org>");
> +MODULE_DESCRIPTION("GRLIB I2CMST I2C bus driver");
> +MODULE_LICENSE("GPL");

-- 
Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH] i2c/busses: Add support for Aeroflex Gaisler I2CMST controller

[Ben Dooks]

On Wed, Feb 16, 2011 at 03:51:11PM +0100, Jan Andersson wrote:
> On 02/16/2011 03:27 PM, Wolfram Sang wrote:
>> Hi Jan,
>>
>> On Wed, Feb 16, 2011 at 01:30:48PM +0100, Jan Andersson wrote:
>>> This patch adds support for the I2CMST core found on LEON/GRLIB SoCs.
>>>
>>> Signed-off-by: Jan Andersson<jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>
>>> ---
>>> The I2CMST core is basically the OpenCores I2C master with an AMBA APB
>>> interface. This driver re-uses much of i2c-ocores.c. It is submitted as
>>> a separate driver since the register interfaces differ sligthly. Also the
>>> two IP cores are maintained separately so they may diverge further in
>>> the future.
>>>
>>> The driver is identical in terms of transfer handling and HW control.
>>> The original module author string has been kept.
>>
>> Just judging from this message, it seems that algo could be extracted and used
>> by both drivers? Or would this make no sense?
>>
>
> Hi Wolfram,
>
> The control parts of the *_process, *_isr and *_xfer functions could be  
> shared by ripping it out and changing the calls used to read/set 
> registers.
>
> I considered that change more complex and did not think it to be worth  
> the trouble for saving ~50 LOC (probably less with added glue). Of  
> course, it could be beneficial to have the algo part shared if there are  
> future updates to it. At the same time I suppose that it could be bad to  
> have the algo part shared if the updates are only valid for one of the  
> cores.

could at-least the contstants be shared?

>
> Best regards,
>   Jan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH] i2c/busses: Add support for Aeroflex Gaisler I2CMST controller

[Jan Andersson]

On 02/23/2011 02:06 AM, Ben Dooks wrote:
> On Wed, Feb 16, 2011 at 01:30:48PM +0100, Jan Andersson wrote:
>> This patch adds support for the I2CMST core found on LEON/GRLIB SoCs.
>>
>> Signed-off-by: Jan Andersson <jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>
>> ---
>> The I2CMST core is basically the OpenCores I2C master with an AMBA APB
>> interface. This driver re-uses much of i2c-ocores.c. It is submitted as
>> a separate driver since the register interfaces differ sligthly. Also the
>> two IP cores are maintained separately so they may diverge further in
>> the future.
> 
> Hmm, some more of that should go above the "---" line.
> 
> Going to do a quick review and then get back to this once I've had some
> time to think on it.
>  

Thanks, I will wait a while for further comments and then send a V2.

>> The driver is identical in terms of transfer handling and HW control.
>> The original module author string has been kept.
>>
>>  drivers/i2c/busses/Kconfig         |   10 +
>>  drivers/i2c/busses/Makefile        |    1 +
>>  drivers/i2c/busses/i2c-gr-i2cmst.c |  371 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 382 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/i2c/busses/i2c-gr-i2cmst.c
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 113505a..06a3150 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -646,6 +646,16 @@ config I2C_EG20T
>>            is an IOH(Input/Output Hub) for x86 embedded processor.
>>            This driver can access PCH I2C bus device.
>>  
>> +config I2C_GR_I2CMST
>> +	tristate "Aeroflex Gaisler I2CMST I2C Controller"
>> +	depends on SPARC_LEON
>> +	help
>> +	  If you say yes to this option, support will be included for the
>> +	  I2CMST I2C controller present on some LEON/GRLIB SoCs.
>> +
>> +	  This driver can also be built as a module.  If so, the module
>> +	  will be called i2c-gr-i2cmst.
>> +
>>  comment "External I2C/SMBus adapter drivers"
>>  
>>  config I2C_PARPORT
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 9d2d0ec..1e16e66 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -62,6 +62,7 @@ obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
>>  obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
>>  obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
>>  obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
>> +obj-$(CONFIG_I2C_GR_I2CMST)	+= i2c-gr-i2cmst.o
>>  
>>  # External I2C/SMBus adapter drivers
>>  obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
>> diff --git a/drivers/i2c/busses/i2c-gr-i2cmst.c b/drivers/i2c/busses/i2c-gr-i2cmst.c
>> new file mode 100644
>> index 0000000..90b23a8
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-gr-i2cmst.c
>> @@ -0,0 +1,371 @@
>> +/*
>> + * i2c-gr-i2cmst.c I2C bus driver for GRLIB I2CMST core
>> + *
>> + * Main parts of this driver re-used from:
>> + *
>> + * i2c-ocores.c: I2C bus driver for OpenCores I2C controller
>> + * by Peter Korsgaard <jacmet-OfajU3CKLf1/SzgSGea1oA@public.gmane.org>
>> + *
>> + * Adaption to GRLIB/I2CMST by Jan Andersson <jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>
>> + *
>> + * This file is licensed under the terms of the GNU General Public License
>> + * version 2.  This program is licensed "as is" without any warranty of any
>> + * kind, whether express or implied.
>> + */
>> +
>> +#include <linux/of_platform.h>
>> +#include <linux/of_device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/errno.h>
>> +#include <linux/i2c.h>
>> +#include <linux/wait.h>
>> +#include <linux/slab.h>
>> +#include <linux/io.h>
>> +
>> +/* I2CMST APB registers */
>> +struct i2cmst_regs {
>> +	u32 prescaler;
>> +	u32 control;
>> +	u32 data;
>> +	u32 cmdstat; /* Command (write), Status (read) */
>> +};
> 
> using structures for memory mapped registers makes me want to go
> EURGH. the compiler is free to pack and order as it feels.
> 

Ah, perhaps I should have added a __attribute__ ((packed)); after
that(?). For V2 I can go back to using a pointer to the base address to
be consistent with many of the other i2c drivers.

>> +struct gr_i2cmst_i2c {
>> +	struct i2cmst_regs *regs;
>> +	wait_queue_head_t wait;
>> +	struct i2c_adapter adap;
>> +	struct i2c_msg *msg;
>> +	int pos;
>> +	int nmsgs;
>> +	int state; /* see STATE_ */
>> +	int clock_khz;
>> +};
>> +
>> +/* register fields and commands */
>> +#define I2CMST_CTRL_IEN		0x40
>> +#define I2CMST_CTRL_EN		0x80
>> +
>> +#define I2CMST_CMD_START	0x91
>> +#define I2CMST_CMD_STOP		0x41
>> +#define I2CMST_CMD_READ		0x21
>> +#define I2CMST_CMD_WRITE	0x11
>> +#define I2CMST_CMD_READ_ACK	0x21
>> +#define I2CMST_CMD_READ_NACK	0x29
>> +#define I2CMST_CMD_IACK		0x01
>> +
>> +#define I2CMST_STAT_IF		0x01
>> +#define I2CMST_STAT_TIP		0x02
>> +#define I2CMST_STAT_ARBLOST	0x20
>> +#define I2CMST_STAT_BUSY	0x40
>> +#define I2CMST_STAT_NACK	0x80
>> +
>> +#define STATE_DONE		0
>> +#define STATE_START		1
>> +#define STATE_WRITE		2
>> +#define STATE_READ		3
>> +#define STATE_ERROR		4
>> +
>> +static inline void i2cmst_setreg(void __iomem *reg, u32 value)
>> +{
>> +	__raw_writel(cpu_to_be32(value), reg);
>> +}
>> +
>> +static inline u32 i2cmst_getreg(void __iomem *reg)
>> +{
>> +	return be32_to_cpu(__raw_readl(reg));
>> +}
> 
> do you really need to use the __raw versions of thees calls?

With just readl() the register value would be byte swapped on my
platform. After looking a bit closer at this it seems like it could be
appropriate to replace the whole line with a call to ioread32be(..).

> 
>> +static void gr_i2cmst_process(struct gr_i2cmst_i2c *i2c)
>> +{
>> +	struct i2c_msg *msg = i2c->msg;
>> +	u32 stat = i2cmst_getreg(&i2c->regs->cmdstat);
>> +
>> +	if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) {
>> +		/* stop has been sent */
>> +		i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_IACK);
>> +		wake_up(&i2c->wait);
>> +		return;
>> +	}
>> +
>> +	/* error? */
>> +	if (stat & I2CMST_STAT_ARBLOST) {
>> +		i2c->state = STATE_ERROR;
>> +		i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP);
>> +		return;
>> +	}
>> +
>> +	if ((i2c->state == STATE_START) || (i2c->state == STATE_WRITE)) {
>> +		i2c->state =
>> +			(msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE;
>> +
>> +		if (stat & I2CMST_STAT_NACK) {
>> +			i2c->state = STATE_ERROR;
>> +			i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP);
>> +			return;
>> +		}
>> +	} else
>> +		msg->buf[i2c->pos++] = i2cmst_getreg(&i2c->regs->data);
>> +
>> +	/* end of msg? */
>> +	if (i2c->pos == msg->len) {
>> +		i2c->nmsgs--;
>> +		i2c->msg++;
>> +		i2c->pos = 0;
>> +		msg = i2c->msg;
>> +
>> +		if (i2c->nmsgs) {	/* end? */
>> +			/* send start? */
>> +			if (!(msg->flags & I2C_M_NOSTART)) {
>> +				u8 addr = (msg->addr << 1);
>> +
>> +				if (msg->flags & I2C_M_RD)
>> +					addr |= 1;
>> +
>> +				i2c->state = STATE_START;
>> +
>> +				i2cmst_setreg(&i2c->regs->data, addr);
>> +				i2cmst_setreg(&i2c->regs->cmdstat,
>> +					I2CMST_CMD_START);
>> +				return;
>> +			} else
>> +				i2c->state = (msg->flags & I2C_M_RD)
>> +					? STATE_READ : STATE_WRITE;
>> +		} else {
>> +			i2c->state = STATE_DONE;
>> +			i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP);
>> +			return;
>> +		}
>> +	}
>> +
>> +	if (i2c->state == STATE_READ) {
>> +		i2cmst_setreg(&i2c->regs->cmdstat, i2c->pos == (msg->len-1) ?
>> +			I2CMST_CMD_READ_NACK : I2CMST_CMD_READ_ACK);
>> +	} else {
>> +		i2cmst_setreg(&i2c->regs->data, msg->buf[i2c->pos++]);
>> +		i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_WRITE);
>> +	}
>> +}
>> +
>> +static irqreturn_t gr_i2cmst_isr(int irq, void *dev_id)
>> +{
>> +	struct gr_i2cmst_i2c *i2c = dev_id;
>> +
>> +	gr_i2cmst_process(i2c);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int gr_i2cmst_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>> +{
>> +	struct gr_i2cmst_i2c *i2c = i2c_get_adapdata(adap);
>> +
>> +	i2c->msg = msgs;
>> +	i2c->pos = 0;
>> +	i2c->nmsgs = num;
>> +	i2c->state = STATE_START;
>> +
>> +	i2cmst_setreg(&i2c->regs->data, (i2c->msg->addr << 1) |
>> +			((i2c->msg->flags & I2C_M_RD) ? 1 : 0));
>> +
>> +	i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_START);
>> +
>> +	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
>> +			       (i2c->state == STATE_DONE), HZ))
>> +		return (i2c->state == STATE_DONE) ? num : -EIO;
>> +	else
>> +		return -ETIMEDOUT;
>> +}
>> +
>> +static void gr_i2cmst_init(struct gr_i2cmst_i2c *i2c)
>> +{
>> +	int prescale;
>> +	u32 ctrl = i2cmst_getreg(&i2c->regs->control);
>> +
>> +	/* make sure the device is disabled */
>> +	i2cmst_setreg(&i2c->regs->control,
>> +			ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN));
>> +
>> +	/* Required prescaler = (AMBAfreq)/(5 * SCLfreq)-1 */
>> +	prescale = (i2c->clock_khz / (5*100)) - 1;
>> +	/* Minimum value of precaler is 3 due to HW sync */
>> +	prescale = prescale < 3 ? 3 : prescale;
>> +	i2cmst_setreg(&i2c->regs->prescaler, prescale);
>> +
>> +	/* Init the device */
>> +	i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_IACK);
>> +	ctrl |= I2CMST_CTRL_IEN | I2CMST_CTRL_EN;
>> +	i2cmst_setreg(&i2c->regs->control, ctrl);
>> +}
>> +
>> +
>> +static u32 gr_i2cmst_func(struct i2c_adapter *adap)
>> +{
>> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>> +}
>> +
>> +static const struct i2c_algorithm gr_i2cmst_algorithm = {
>> +	.master_xfer	= gr_i2cmst_xfer,
>> +	.functionality	= gr_i2cmst_func,
>> +};
>> +
>> +static struct i2c_adapter gr_i2cmst_adapter = {
>> +	.owner		= THIS_MODULE,
>> +	.name		= "i2c-gr-i2cmst",
>> +	.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD,
>> +	.algo		= &gr_i2cmst_algorithm,
>> +};
>> +
>> +
>> +static int __devinit gr_i2cmst_of_probe(struct platform_device *ofdev, const struct of_device_id *match)
> 
> please avoid line-wrap like this.

OK

> 
>> +{
>> +	struct gr_i2cmst_i2c *i2c;
>> +	const unsigned int *busfreq;
>> +	int ret;
>> +
>> +	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
>> +	if (!i2c)
>> +		return -ENOMEM;
> 
> would be nice to have an error print here.

OK

> 
>> +	/* Get frequency of APB bus that core is attached to */
>> +	busfreq = of_get_property(ofdev->dev.of_node, "freq", NULL);
>> +	if (!busfreq) {
>> +		dev_err(&ofdev->dev, "Missing parameter 'freq'\n");
>> +		return -ENODEV;
>> +	}
>> +	i2c->clock_khz = *busfreq/1000;
>> +
>> +	i2c->regs = of_ioremap(&ofdev->resource[0], 0,
>> +			resource_size(&ofdev->resource[0]),
>> +			"grlib-i2cmst regs");
>> +	if (!i2c->regs) {
>> +		dev_err(&ofdev->dev, "Unable to map registers\n");
>> +		ret = -EIO;
>> +		goto map_failed;
>> +	}
>> +
>> +	gr_i2cmst_init(i2c);
>> +
>> +	init_waitqueue_head(&i2c->wait);
>> +	ret = request_irq(ofdev->archdata.irqs[0], gr_i2cmst_isr, 0,
>> +			ofdev->name, i2c);
>> +	if (ret) {
>> +		dev_err(&ofdev->dev, "Cannot claim IRQ\n");
>> +		goto request_irq_failed;
>> +	}
>> +
>> +	/* hook up driver to tree */
>> +	dev_set_drvdata(&ofdev->dev, i2c);
>> +	i2c->adap = gr_i2cmst_adapter;
>> +	i2c_set_adapdata(&i2c->adap, i2c);
>> +	i2c->adap.dev.parent = &ofdev->dev;
>> +	i2c->adap.dev.of_node = ofdev->dev.of_node;
>> +
>> +	/* add i2c adapter to i2c tree */
>> +	ret = i2c_add_adapter(&i2c->adap);
>> +	if (ret) {
>> +		dev_err(&ofdev->dev, "Failed to add adapter\n");
>> +		goto add_adapter_failed;
>> +	}
>> +
>> +	return 0;
>> +
>> +add_adapter_failed:
>> +	free_irq(ofdev->archdata.irqs[0], i2c);
>> +request_irq_failed:
>> +	of_iounmap(&ofdev->resource[0], i2c->regs,
>> +		resource_size(&ofdev->resource[0]));
>> +map_failed:
>> +	kfree(i2c);
>> +
>> +	return ret;
>> +}
>> +
>> +static int __devexit gr_i2cmst_of_remove(struct platform_device *ofdev)
>> +{
>> +	struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev);
>> +	u32 ctrl = i2cmst_getreg(&i2c->regs->control);
>> +
>> +	/* disable i2c logic */
>> +	i2cmst_setreg(&i2c->regs->control,
>> +		ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN));
>> +
>> +	/* remove adapter & data */
>> +	i2c_del_adapter(&i2c->adap);
>> +	dev_set_drvdata(&ofdev->dev, NULL);
>> +
>> +	free_irq(ofdev->archdata.irqs[0], i2c);
>> +
>> +	of_iounmap(&ofdev->resource[0], i2c->regs,
>> +		resource_size(&ofdev->resource[0]));
>> +
>> +	kfree(i2c);
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int gr_i2cmst_suspend(struct platform_device *ofdev, pm_message_t state)
>> +{
>> +	struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev);
>> +	u32 ctrl = gr_i2cmst_getreg(&i2c->regs->control);
>> +
>> +	/* make sure the device is disabled */
>> +	gr_i2cmst_setreg(&i2c->regs->control,
>> +			ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN));
>> +
>> +	return 0;
>> +}
>> +
>> +static int gr_i2cmst_resume(struct platform_device *ofdev)
>> +{
>> +	struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev);
>> +
>> +	gr_i2cmst_init(i2c);
>> +
>> +	return 0;
>> +}
>> +#else
>> +#define gr_i2cmst_suspend	NULL
>> +#define gr_i2cmst_resume	NULL
>> +#endif
>> +
>> +static struct of_device_id gr_i2cmst_of_match[] = {
>> +	{ .name = "GAISLER_I2CMST", },
>> +	{ .name = "01_028", },
>> +	{},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, gr_i2cmst_of_match);
>> +
>> +static struct of_platform_driver gr_i2cmst_of_driver = {
>> +	.driver = {
>> +		.name = "grlib-i2cmst",
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = gr_i2cmst_of_match,
>> +	},
>> +	.probe = gr_i2cmst_of_probe,
>> +	.remove = __devexit_p(gr_i2cmst_of_remove),
>> +	.suspend = gr_i2cmst_suspend,
>> +	.resume  = gr_i2cmst_resume,
>> +};
>> +
>> +
>> +static int __init i2cmst_init(void)
>> +{
>> +	return of_register_platform_driver(&gr_i2cmst_of_driver);
>> +}
>> +
>> +static void __exit i2cmst_exit(void)
>> +{
>> +	of_unregister_platform_driver(&gr_i2cmst_of_driver);
>> +}
>> +
>> +module_init(i2cmst_init);
>> +module_exit(i2cmst_exit);
>> +
>> +MODULE_AUTHOR("Peter Korsgaard <jacmet-OfajU3CKLf1/SzgSGea1oA@public.gmane.org>");
>> +MODULE_DESCRIPTION("GRLIB I2CMST I2C bus driver");
>> +MODULE_LICENSE("GPL");
> 

Thanks for reviewing!

Best regards,
  Jan

Re: [PATCH] i2c/busses: Add support for Aeroflex Gaisler I2CMST controller

[Jan Andersson]

On 02/23/2011 02:07 AM, Ben Dooks wrote:
> On Wed, Feb 16, 2011 at 03:51:11PM +0100, Jan Andersson wrote:
>> On 02/16/2011 03:27 PM, Wolfram Sang wrote:
>>> Hi Jan,
>>>
>>> On Wed, Feb 16, 2011 at 01:30:48PM +0100, Jan Andersson wrote:
>>>> This patch adds support for the I2CMST core found on LEON/GRLIB SoCs.
>>>>
>>>> Signed-off-by: Jan Andersson<jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>
>>>> ---
>>>> The I2CMST core is basically the OpenCores I2C master with an AMBA APB
>>>> interface. This driver re-uses much of i2c-ocores.c. It is submitted as
>>>> a separate driver since the register interfaces differ sligthly. Also the
>>>> two IP cores are maintained separately so they may diverge further in
>>>> the future.
>>>>
>>>> The driver is identical in terms of transfer handling and HW control.
>>>> The original module author string has been kept.
>>>
>>> Just judging from this message, it seems that algo could be extracted and used
>>> by both drivers? Or would this make no sense?
>>>
>>
>> Hi Wolfram,
>>
>> The control parts of the *_process, *_isr and *_xfer functions could be  
>> shared by ripping it out and changing the calls used to read/set 
>> registers.
>>
>> I considered that change more complex and did not think it to be worth  
>> the trouble for saving ~50 LOC (probably less with added glue). Of  
>> course, it could be beneficial to have the algo part shared if there are  
>> future updates to it. At the same time I suppose that it could be bad to  
>> have the algo part shared if the updates are only valid for one of the  
>> cores.
> 
> could at-least the contstants be shared?
> 

The register field definitions can currently be shared. The register
offsets (steps) are different since the two prescaler registers have
been merged into one register on the i2cmst core.

For V2 I will move the register field defines from i2c-ocores.c to
i2c-ocores.h and include that file in the new driver - unless anyone
objects.

Thanks to you both for reviewing!

Best regards,
  Jan

Re: [PATCH] i2c/busses: Add support for Aeroflex Gaisler I2CMST controller

[Peter Korsgaard]

>>>>> "Jan" == Jan Andersson <jan-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org> writes:

Hi,

 Jan> The register field definitions can currently be shared. The register
 Jan> offsets (steps) are different since the two prescaler registers have
 Jan> been merged into one register on the i2cmst core.

 Jan> For V2 I will move the register field defines from i2c-ocores.c to
 Jan> i2c-ocores.h and include that file in the new driver - unless anyone
 Jan> objects.

Ben, do you really think that's worthwhile? It's just 6 registers with a
few bits in 3 of them.

-- 
Bye, Peter Korsgaard