[PATCH 0/3] Add I2C support for Octeon SOCs.

[PATCH 0/3] Add I2C support for Octeon SOCs.

[David Daney]

This patch set adds I2C driver support for Cavium Networks' Octeon
processor family.  The Octeon is a multi-core MIPS64 based SOC.

The first patch adds platform devices for the I2C devices.  The second
patch is the main driver.  Finally the third patch registers some
devices so we have something to control with the fancy new driver.

I will reply with the three patches.

David Daney (2):
   MIPS: Octeon: Add I2C platform driver.
   MIPS: Octeon: Register some devices on the I2C bus.

Rade Bozic (1):
   I2C: Add driver for Cavium OCTEON I2C ports.

  arch/mips/cavium-octeon/octeon-platform.c |   85 +++++
  arch/mips/include/asm/octeon/octeon.h     |    5 +
  drivers/i2c/busses/Kconfig                |   10 +
  drivers/i2c/busses/Makefile               |    1 +
  drivers/i2c/busses/i2c-octeon.c           |  579 
+++++++++++++++++++++++++++++
  5 files changed, 680 insertions(+), 0 deletions(-)
  create mode 100644 drivers/i2c/busses/i2c-octeon.c

[PATCH 1/3] MIPS: Octeon: Add I2C platform driver.

[David Daney]

Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
CC: Rade Bozic <rade.bozic.ext-OYasijW0DpE@public.gmane.org>
---
 arch/mips/cavium-octeon/octeon-platform.c |   72 +++++++++++++++++++++++++++++
 arch/mips/include/asm/octeon/octeon.h     |    5 ++
 2 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/arch/mips/cavium-octeon/octeon-platform.c b/arch/mips/cavium-octeon/octeon-platform.c
index 20698a6..f2c0602 100644
--- a/arch/mips/cavium-octeon/octeon-platform.c
+++ b/arch/mips/cavium-octeon/octeon-platform.c
@@ -165,6 +165,78 @@ out:
 }
 device_initcall(octeon_rng_device_init);
 
+
+#define OCTEON_I2C_IO_BASE 0x1180000001000ull
+#define OCTEON_I2C_IO_UNIT_OFFSET 0x200
+
+static struct octeon_i2c_data octeon_i2c_data[2];
+
+static int __init octeon_i2c_device_init(void)
+{
+	struct platform_device *pd;
+	int ret = 0;
+	int port, num_ports;
+
+	struct resource i2c_resources[] = {
+		{
+			.flags	= IORESOURCE_MEM,
+		}, {
+			.flags	= IORESOURCE_IRQ,
+		}
+	};
+
+	if (OCTEON_IS_MODEL(OCTEON_CN56XX) || OCTEON_IS_MODEL(OCTEON_CN52XX))
+		num_ports = 2;
+	else
+		num_ports = 1;
+
+	for (port = 0; port < num_ports; port++) {
+		octeon_i2c_data[port].sys_freq = octeon_get_clock_rate();
+		/*FIXME: should be examined. At the moment is set for 100Khz */
+		octeon_i2c_data[port].i2c_freq = 100000;
+
+		pd = platform_device_alloc("i2c-octeon", port);
+		if (!pd) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		pd->dev.platform_data = octeon_i2c_data + port;
+
+		i2c_resources[0].start =
+			OCTEON_I2C_IO_BASE + (port * OCTEON_I2C_IO_UNIT_OFFSET);
+		i2c_resources[0].end = i2c_resources[0].start + 0x20;
+		switch (port) {
+		case 0:
+			i2c_resources[1].start = OCTEON_IRQ_TWSI;
+			i2c_resources[1].end = OCTEON_IRQ_TWSI;
+			break;
+		case 1:
+			i2c_resources[1].start = OCTEON_IRQ_TWSI2;
+			i2c_resources[1].end = OCTEON_IRQ_TWSI2;
+			break;
+		default:
+			BUG();
+		}
+
+		ret = platform_device_add_resources(pd,
+						    i2c_resources,
+						    ARRAY_SIZE(i2c_resources));
+		if (ret)
+			goto fail;
+
+		ret = platform_device_add(pd);
+		if (ret)
+			goto fail;
+	}
+	return ret;
+fail:
+	platform_device_put(pd);
+out:
+	return ret;
+}
+device_initcall(octeon_i2c_device_init);
+
 /* Octeon SMI/MDIO interface.  */
 static int __init octeon_mdiobus_device_init(void)
 {
diff --git a/arch/mips/include/asm/octeon/octeon.h b/arch/mips/include/asm/octeon/octeon.h
index 705fef2..a0cf129 100644
--- a/arch/mips/include/asm/octeon/octeon.h
+++ b/arch/mips/include/asm/octeon/octeon.h
@@ -213,6 +213,11 @@ struct octeon_cf_data {
 	int		dma_engine;	/* -1 for no DMA */
 };
 
+struct octeon_i2c_data {
+	unsigned int	sys_freq;
+	unsigned int	i2c_freq;
+};
+
 extern void octeon_write_lcd(const char *s);
 extern void octeon_check_cpu_bist(void);
 extern int octeon_get_boot_uart(void);
-- 
1.6.0.6

Re: [PATCH 1/3] MIPS: Octeon: Add I2C platform driver.

[Sergei Shtylyov]

Hello.

David Daney wrote:

> Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
> CC: Rade Bozic <rade.bozic.ext-OYasijW0DpE@public.gmane.org>
>   
[...]
> diff --git a/arch/mips/cavium-octeon/octeon-platform.c b/arch/mips/cavium-octeon/octeon-platform.c
> index 20698a6..f2c0602 100644
> --- a/arch/mips/cavium-octeon/octeon-platform.c
> +++ b/arch/mips/cavium-octeon/octeon-platform.c
> @@ -165,6 +165,78 @@ out:
>  }
>  device_initcall(octeon_rng_device_init);
>  
> +
> +#define OCTEON_I2C_IO_BASE 0x1180000001000ull
> +#define OCTEON_I2C_IO_UNIT_OFFSET 0x200
> +
> +static struct octeon_i2c_data octeon_i2c_data[2];
> +
> +static int __init octeon_i2c_device_init(void)
> +{
> +	struct platform_device *pd;
> +	int ret = 0;
> +	int port, num_ports;
> +
> +	struct resource i2c_resources[] = {
> +		{
> +			.flags	= IORESOURCE_MEM,
> +		}, {
> +			.flags	= IORESOURCE_IRQ,
> +		}
> +	};
> +
> +	if (OCTEON_IS_MODEL(OCTEON_CN56XX) || OCTEON_IS_MODEL(OCTEON_CN52XX))
> +		num_ports = 2;
> +	else
> +		num_ports = 1;
> +
> +	for (port = 0; port < num_ports; port++) {
> +		octeon_i2c_data[port].sys_freq = octeon_get_clock_rate();
> +		/*FIXME: should be examined. At the moment is set for 100Khz */
> +		octeon_i2c_data[port].i2c_freq = 100000;
> +
> +		pd = platform_device_alloc("i2c-octeon", port);
> +		if (!pd) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		pd->dev.platform_data = octeon_i2c_data + port;
> +
> +		i2c_resources[0].start =
> +			OCTEON_I2C_IO_BASE + (port * OCTEON_I2C_IO_UNIT_OFFSET);
> +		i2c_resources[0].end = i2c_resources[0].start + 0x20;
>   

   Not 0x1F?

WBR, Sergei

Re: [PATCH 1/3] MIPS: Octeon: Add I2C platform driver.

[David Daney]

Sergei Shtylyov wrote:
> Hello.
> 
> David Daney wrote:
> 
>> Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
>> CC: Rade Bozic <rade.bozic.ext-OYasijW0DpE@public.gmane.org>
>>   
> [...]
>> diff --git a/arch/mips/cavium-octeon/octeon-platform.c 
>> b/arch/mips/cavium-octeon/octeon-platform.c
>> index 20698a6..f2c0602 100644
>> --- a/arch/mips/cavium-octeon/octeon-platform.c
>> +++ b/arch/mips/cavium-octeon/octeon-platform.c
>> @@ -165,6 +165,78 @@ out:
>>  }
>>  device_initcall(octeon_rng_device_init);
>>  
>> +
>> +#define OCTEON_I2C_IO_BASE 0x1180000001000ull
>> +#define OCTEON_I2C_IO_UNIT_OFFSET 0x200
>> +
>> +static struct octeon_i2c_data octeon_i2c_data[2];
>> +
>> +static int __init octeon_i2c_device_init(void)
>> +{
>> +    struct platform_device *pd;
>> +    int ret = 0;
>> +    int port, num_ports;
>> +
>> +    struct resource i2c_resources[] = {
>> +        {
>> +            .flags    = IORESOURCE_MEM,
>> +        }, {
>> +            .flags    = IORESOURCE_IRQ,
>> +        }
>> +    };
>> +
>> +    if (OCTEON_IS_MODEL(OCTEON_CN56XX) || 
>> OCTEON_IS_MODEL(OCTEON_CN52XX))
>> +        num_ports = 2;
>> +    else
>> +        num_ports = 1;
>> +
>> +    for (port = 0; port < num_ports; port++) {
>> +        octeon_i2c_data[port].sys_freq = octeon_get_clock_rate();
>> +        /*FIXME: should be examined. At the moment is set for 100Khz */
>> +        octeon_i2c_data[port].i2c_freq = 100000;
>> +
>> +        pd = platform_device_alloc("i2c-octeon", port);
>> +        if (!pd) {
>> +            ret = -ENOMEM;
>> +            goto out;
>> +        }
>> +
>> +        pd->dev.platform_data = octeon_i2c_data + port;
>> +
>> +        i2c_resources[0].start =
>> +            OCTEON_I2C_IO_BASE + (port * OCTEON_I2C_IO_UNIT_OFFSET);
>> +        i2c_resources[0].end = i2c_resources[0].start + 0x20;
>>   
> 
>   Not 0x1F?
> 

You are correct.  I will fix it.

David Daney

[PATCH 1/3] MIPS: Octeon: Add I2C platform device.

[David Daney]

Signed-off-by: David Daney <ddaney@caviumnetworks.com>
CC: Rade Bozic <rade.bozic.ext@nsn.com>
---

This supersedes the original '[PATCH 1/3] MIPS: Octeon: Add I2C
platform driver.' and corrects the resource end value as noted by
Sergei Shtylyov.

 arch/mips/cavium-octeon/octeon-platform.c |   72 +++++++++++++++++++++++++++++
 arch/mips/include/asm/octeon/octeon.h     |    5 ++
 2 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/arch/mips/cavium-octeon/octeon-platform.c b/arch/mips/cavium-octeon/octeon-platform.c
index 20698a6..b4a0b5f 100644
--- a/arch/mips/cavium-octeon/octeon-platform.c
+++ b/arch/mips/cavium-octeon/octeon-platform.c
@@ -165,6 +165,78 @@ out:
 }
 device_initcall(octeon_rng_device_init);
 
+
+#define OCTEON_I2C_IO_BASE 0x1180000001000ull
+#define OCTEON_I2C_IO_UNIT_OFFSET 0x200
+
+static struct octeon_i2c_data octeon_i2c_data[2];
+
+static int __init octeon_i2c_device_init(void)
+{
+	struct platform_device *pd;
+	int ret = 0;
+	int port, num_ports;
+
+	struct resource i2c_resources[] = {
+		{
+			.flags	= IORESOURCE_MEM,
+		}, {
+			.flags	= IORESOURCE_IRQ,
+		}
+	};
+
+	if (OCTEON_IS_MODEL(OCTEON_CN56XX) || OCTEON_IS_MODEL(OCTEON_CN52XX))
+		num_ports = 2;
+	else
+		num_ports = 1;
+
+	for (port = 0; port < num_ports; port++) {
+		octeon_i2c_data[port].sys_freq = octeon_get_clock_rate();
+		/*FIXME: should be examined. At the moment is set for 100Khz */
+		octeon_i2c_data[port].i2c_freq = 100000;
+
+		pd = platform_device_alloc("i2c-octeon", port);
+		if (!pd) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		pd->dev.platform_data = octeon_i2c_data + port;
+
+		i2c_resources[0].start =
+			OCTEON_I2C_IO_BASE + (port * OCTEON_I2C_IO_UNIT_OFFSET);
+		i2c_resources[0].end = i2c_resources[0].start + 0x1f;
+		switch (port) {
+		case 0:
+			i2c_resources[1].start = OCTEON_IRQ_TWSI;
+			i2c_resources[1].end = OCTEON_IRQ_TWSI;
+			break;
+		case 1:
+			i2c_resources[1].start = OCTEON_IRQ_TWSI2;
+			i2c_resources[1].end = OCTEON_IRQ_TWSI2;
+			break;
+		default:
+			BUG();
+		}
+
+		ret = platform_device_add_resources(pd,
+						    i2c_resources,
+						    ARRAY_SIZE(i2c_resources));
+		if (ret)
+			goto fail;
+
+		ret = platform_device_add(pd);
+		if (ret)
+			goto fail;
+	}
+	return ret;
+fail:
+	platform_device_put(pd);
+out:
+	return ret;
+}
+device_initcall(octeon_i2c_device_init);
+
 /* Octeon SMI/MDIO interface.  */
 static int __init octeon_mdiobus_device_init(void)
 {
diff --git a/arch/mips/include/asm/octeon/octeon.h b/arch/mips/include/asm/octeon/octeon.h
index 705fef2..a0cf129 100644
--- a/arch/mips/include/asm/octeon/octeon.h
+++ b/arch/mips/include/asm/octeon/octeon.h
@@ -213,6 +213,11 @@ struct octeon_cf_data {
 	int		dma_engine;	/* -1 for no DMA */
 };
 
+struct octeon_i2c_data {
+	unsigned int	sys_freq;
+	unsigned int	i2c_freq;
+};
+
 extern void octeon_write_lcd(const char *s);
 extern void octeon_check_cpu_bist(void);
 extern int octeon_get_boot_uart(void);
-- 
1.6.0.6

[PATCH 2/3] I2C: Add driver for Cavium OCTEON I2C ports.

[David Daney]

From: Rade Bozic <rade.bozic.ext-OYasijW0DpE@public.gmane.org>

Signed-off-by: Rade Bozic <rade.bozic.ext-OYasijW0DpE@public.gmane.org>
Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
---
 drivers/i2c/busses/Kconfig      |   10 +
 drivers/i2c/busses/Makefile     |    1 +
 drivers/i2c/busses/i2c-octeon.c |  579 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 590 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-octeon.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 5f318ce..737f052 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -564,6 +564,16 @@ config I2C_VERSATILE
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-versatile.
 
+config I2C_OCTEON
+	tristate "Cavium OCTEON I2C bus support"
+	depends on CPU_CAVIUM_OCTEON
+	help
+	  Say yes if you want to support the I2C serial bus on Cavium
+	  OCTEON SOC.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-octeon.
+
 comment "External I2C/SMBus adapter drivers"
 
 config I2C_PARPORT
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 302c551..c2c4ea1 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
 obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
+obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
 
 # External I2C/SMBus adapter drivers
 obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
new file mode 100644
index 0000000..57455ac
--- /dev/null
+++ b/drivers/i2c/busses/i2c-octeon.c
@@ -0,0 +1,579 @@
+/*
+ * (C) Copyright 2009-2010
+ * Nokia Siemens Networks, michael.lawnick.ext-OYasijW0DpE@public.gmane.org
+ *
+ * This is a driver for the i2c adapter in Cavium Networks' OCTEON processors.
+ *
+ * Release 0.1
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/init.h>
+
+#include <linux/io.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+
+#include <asm/octeon/octeon.h>
+
+#define DRV_NAME "i2c-octeon"
+#define DRV_VERSION	"0.1"
+
+/* register offsets */
+#define SW_TWSI	 0x00
+#define TWSI_INT 0x10
+
+/* Controller command patterns */
+#define SW_TWSI_V               0x8000000000000000ull
+#define SW_TWSI_EOP_TWSI_DATA   0x0C00000100000000ull
+#define SW_TWSI_EOP_TWSI_CTL    0x0C00000200000000ull
+#define SW_TWSI_EOP_TWSI_CLKCTL 0x0C00000300000000ull
+#define SW_TWSI_EOP_TWSI_STAT   0x0C00000300000000ull
+#define SW_TWSI_EOP_TWSI_RST    0x0C00000700000000ull
+#define SW_TWSI_OP_TWSI_CLK     0x0800000000000000ull
+#define SW_TWSI_R               0x0100000000000000ull
+
+/* Controller command and status bits */
+#define TWSI_CTL_CE   0x80
+#define TWSI_CTL_ENAB 0x40
+#define TWSI_CTL_STA  0x20
+#define TWSI_CTL_STP  0x10
+#define TWSI_CTL_IFLG 0x08
+#define TWSI_CTL_AAK  0x04
+
+/* Some status values */
+#define STAT_START      0x08
+#define STAT_RSTART     0x10
+#define STAT_TXADDR_ACK 0x18
+#define STAT_TXDATA_ACK 0x28
+#define STAT_RXADDR_ACK 0x40
+#define STAT_RXDATA_ACK 0x50
+#define STAT_IDLE       0xF8
+
+#ifndef NO_IRQ
+#define NO_IRQ (-1)
+#endif
+
+struct octeon_i2c {
+	wait_queue_head_t queue;
+	struct i2c_adapter adap;
+	int irq;
+	int twsi_freq;
+	int sys_freq;
+	uint8_t twsi_ctl;
+	resource_size_t twsi_phys;
+	void __iomem *twsi_base;
+	resource_size_t regsize;
+	struct device *dev;
+};
+
+/* Writes need to be flushed by a read. */
+static void octeon_i2c_write_sw(struct octeon_i2c *i2c,
+				uint64_t eop_reg,
+				uint8_t data)
+{
+	uint64_t tmp;
+
+	__raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base + SW_TWSI);
+	tmp = __raw_readq(i2c->twsi_base + SW_TWSI);
+}
+
+static uint8_t octeon_i2c_read_sw(struct octeon_i2c *i2c, uint64_t eop_reg)
+{
+	uint64_t tmp;
+
+	__raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base + SW_TWSI);
+	tmp = __raw_readq(i2c->twsi_base + SW_TWSI);
+
+	return __raw_readq(i2c->twsi_base + SW_TWSI) & 0xFF;
+}
+
+static void octeon_i2c_write_int(struct octeon_i2c *i2c, uint64_t data)
+{
+	uint64_t tmp;
+
+	__raw_writeq(data, i2c->twsi_base + TWSI_INT);
+	tmp = __raw_readq(i2c->twsi_base + TWSI_INT);
+}
+
+static void octeon_i2c_int_enable(struct octeon_i2c *i2c)
+{
+	octeon_i2c_write_int(i2c, 0x40);
+}
+
+static void octeon_i2c_int_disable(struct octeon_i2c *i2c)
+{
+	octeon_i2c_write_int(i2c, 0);
+}
+
+/* If there was a reset while a device was driving 0 to bus,
+   bus is blocked. We toggle it free manually by some clock
+   cycles and send a stop. */
+static void octeon_i2c_unblock(struct octeon_i2c *i2c)
+{
+	int i;
+
+	dev_dbg(i2c->dev, "%s\n", __func__);
+	for (i = 0; i < 9; i++) {
+		octeon_i2c_write_int(i2c, 0x0);
+		udelay(5);
+		octeon_i2c_write_int(i2c, 0x200);
+		udelay(5);
+	}
+	octeon_i2c_write_int(i2c, 0x300);
+	udelay(5);
+	octeon_i2c_write_int(i2c, 0x100);
+	udelay(5);
+	octeon_i2c_write_int(i2c, 0x0);
+}
+
+static irqreturn_t octeon_i2c_isr(int irq, void *dev_id)
+{
+	struct octeon_i2c *i2c = dev_id;
+
+	octeon_i2c_int_disable(i2c);
+	i2c->twsi_ctl = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_CTL);
+	wake_up_interruptible(&i2c->queue);
+
+	return IRQ_HANDLED;
+}
+
+static int octeon_i2c_wait(struct octeon_i2c *i2c)
+{
+	unsigned long orig_jiffies = jiffies;
+	uint8_t ctl;
+	int result = 0;
+
+	if (i2c->irq == NO_IRQ) {
+		/* polling */
+		do {
+			ctl = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_CTL);
+			if (ctl & TWSI_CTL_IFLG)
+				break;
+		} while (!time_after(jiffies,
+					orig_jiffies + i2c->adap.timeout));
+
+		if (!(ctl & TWSI_CTL_IFLG)) {
+			dev_dbg(i2c->dev, "%s: timeout\n", __func__);
+			result = -ETIMEDOUT;
+		}
+	} else {
+		i2c->twsi_ctl = 0;
+		/* interrupt mode */
+		octeon_i2c_int_enable(i2c);
+
+		result = wait_event_interruptible_timeout(i2c->queue,
+				(i2c->twsi_ctl & TWSI_CTL_IFLG),
+				i2c->adap.timeout);
+
+		if (unlikely(result < 0)) {
+			dev_dbg(i2c->dev, "%s: wait interrupted\n", __func__);
+		} else if (unlikely(!(i2c->twsi_ctl & TWSI_CTL_IFLG))) {
+			dev_dbg(i2c->dev, "%s: timeout\n", __func__);
+			result = -ETIMEDOUT;
+		}
+	}
+
+	return result < 0 ? result : 0;
+}
+
+static int octeon_i2c_start(struct octeon_i2c *i2c)
+{
+	uint8_t data;
+	int result;
+
+	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL,
+				TWSI_CTL_ENAB | TWSI_CTL_STA);
+
+	result = octeon_i2c_wait(i2c);
+	if (unlikely(result < 0) &&
+	    octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT) == STAT_IDLE) {
+		/*
+		 * Controller refused to send start flag May be a
+		 * client is holding SDA low - let's try to free it.
+		 */
+		octeon_i2c_unblock(i2c);
+		octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL,
+				    TWSI_CTL_ENAB | TWSI_CTL_STA);
+
+		result = octeon_i2c_wait(i2c);
+		if (result < 0)
+			return result;
+	}
+
+	data = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
+	if ((data != STAT_START) && (data != STAT_RSTART)) {
+		dev_dbg(i2c->dev, "%s: bad status (0x%x)\n", __func__, data);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int octeon_i2c_stop(struct octeon_i2c *i2c)
+{
+	unsigned long orig_jiffies = jiffies;
+	uint8_t data;
+
+	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL,
+				TWSI_CTL_ENAB | TWSI_CTL_STP);
+
+	/*
+	 * Controller will change to idle, but that does not raise an
+	 * interrupt, so we have to poll.
+	 */
+	do {
+		data = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
+		if (data == STAT_IDLE)
+			break;
+		udelay(1);
+	} while (!time_after(jiffies, orig_jiffies + 2));
+
+	if (data != STAT_IDLE) {
+		dev_dbg(i2c->dev, "%s: bad status(0x%x)\n", __func__, data);
+		return -1;
+	}
+	return 0;
+}
+
+static int octeon_i2c_write(struct octeon_i2c *i2c, int target,
+			    const uint8_t *data, int length)
+{
+	int i, result;
+	uint8_t tmp;
+
+	result = octeon_i2c_start(i2c);
+	if (unlikely(result < 0))
+		return result;
+
+	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_DATA, target << 1);
+	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL, TWSI_CTL_ENAB);
+
+	result = octeon_i2c_wait(i2c);
+	if (unlikely(result < 0))
+		return result;
+
+	for (i = 0; i < length; i++) {
+		tmp = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
+		if ((tmp != STAT_TXADDR_ACK) && (tmp != STAT_TXDATA_ACK)) {
+			dev_dbg(i2c->dev,
+				"%s: bad status before write (0x%x)\n",
+				__func__, tmp);
+			return -(int)tmp;
+		}
+
+		octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_DATA, data[i]);
+		octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL, TWSI_CTL_ENAB);
+
+		result = octeon_i2c_wait(i2c);
+		if (unlikely(result < 0))
+			return result;
+	}
+
+	return 0;
+}
+
+static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
+			   uint8_t *data, int length)
+{
+	int i, result;
+	uint8_t tmp;
+
+	if (length < 1)
+		return -EINVAL;
+
+	result = octeon_i2c_start(i2c);
+	if (result)
+		return result;
+
+	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_DATA, (target<<1) | 1);
+	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL, TWSI_CTL_ENAB);
+
+	result = octeon_i2c_wait(i2c);
+	if (unlikely(result < 0))
+		return result;
+
+	for (i = 0; i < length; i++) {
+		tmp = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
+		if ((tmp != STAT_RXDATA_ACK) && (tmp != STAT_RXADDR_ACK)) {
+			dev_dbg(i2c->dev,
+				"%s: bad status before read (0x%x)\n",
+				__func__, tmp);
+			return -(int)tmp;
+		}
+
+		if (i+1 < length)
+			octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL,
+						TWSI_CTL_ENAB | TWSI_CTL_AAK);
+		else
+			octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL,
+						TWSI_CTL_ENAB);
+
+		result = octeon_i2c_wait(i2c);
+		if (unlikely(result < 0))
+			return result;
+
+		data[i] = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_DATA);
+	}
+	return length;
+}
+
+static int octeon_i2c_xfer(struct i2c_adapter *adap,
+			   struct i2c_msg *msgs,
+			   int num)
+{
+	struct i2c_msg *pmsg;
+	int i;
+	int ret = 0;
+	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
+
+	for (i = 0; ret >= 0 && i < num; i++) {
+		pmsg = &msgs[i];
+		dev_dbg(i2c->dev,
+			"%s: Doing %s %d byte(s) to/from 0x%02x - "
+			"%d of %d messages\n",
+			 __func__,
+			 pmsg->flags & I2C_M_RD ? "read" : "write",
+			 pmsg->len, pmsg->addr, i + 1, num);
+		if (pmsg->flags & I2C_M_RD)
+			ret = octeon_i2c_read(i2c, pmsg->addr, pmsg->buf,
+						pmsg->len);
+		else
+			ret = octeon_i2c_write(i2c, pmsg->addr, pmsg->buf,
+						pmsg->len);
+	}
+	octeon_i2c_stop(i2c);
+
+	return (ret < 0) ? ret : num;
+}
+
+static u32 octeon_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm octeon_i2c_algo = {
+	.master_xfer = octeon_i2c_xfer,
+	.functionality = octeon_i2c_functionality,
+};
+
+static struct i2c_adapter octeon_i2c_ops = {
+	.owner = THIS_MODULE,
+	.name = "OCTEON adapter",
+	.algo = &octeon_i2c_algo,
+	.timeout = 2,
+};
+
+static int __init octeon_i2c_setclock(struct octeon_i2c *i2c)
+{
+	int tclk, thp_base, inc, thp_idx, mdiv_idx, ndiv_idx, foscl, diff;
+	int thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = 1000000;
+
+	for (ndiv_idx = 0; ndiv_idx < 8 && delta_hz != 0; ndiv_idx++) {
+		/*
+		 * An mdiv value of less than 2 seems to not work well
+		 * with ds1337 RTCs, so we constrain it to larger
+		 * values.
+		 */
+		for (mdiv_idx = 15; mdiv_idx >= 2 && delta_hz != 0; mdiv_idx--) {
+			/*
+			 * For given ndiv and mdiv values check the
+			 * two closest thp values.
+			 */
+			tclk = i2c->twsi_freq * (mdiv_idx + 1) * 10;
+			tclk *= (1 << ndiv_idx);
+			thp_base = (i2c->sys_freq / (tclk * 2)) - 1;
+			for (inc = 0; inc <= 1; inc++) {
+				thp_idx = thp_base + inc;
+				if (thp_idx < 5 || thp_idx > 0xff)
+					continue;
+
+				foscl = i2c->sys_freq / (2 * (thp_idx + 1));
+				foscl = foscl / (1 << ndiv_idx);
+				foscl = foscl / (mdiv_idx + 1) / 10;
+				diff = abs(foscl - i2c->twsi_freq);
+				if (diff < delta_hz) {
+					delta_hz = diff;
+					thp = thp_idx;
+					mdiv = mdiv_idx;
+					ndiv = ndiv_idx;
+				}
+			}
+		}
+	}
+	octeon_i2c_write_sw(i2c, SW_TWSI_OP_TWSI_CLK, thp);
+	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CLKCTL, (mdiv << 3) | ndiv);
+
+	return 0;
+}
+
+static int __init octeon_i2c_initlowlevel(struct octeon_i2c *i2c)
+{
+	uint8_t status;
+	int tries;
+
+	/* disable high level controller, enable bus access */
+	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL, TWSI_CTL_ENAB);
+
+	/* reset controller */
+	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_RST, 0);
+
+	for (tries = 10; tries; tries--) {
+		udelay(1);
+		status = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
+		if (status == STAT_IDLE)
+			return 0;
+	}
+	dev_dbg(i2c->dev, "%s: TWSI_RST failed! (0x%x)\n", __func__, status);
+	return -1;
+}
+
+static int __init octeon_i2c_probe(struct platform_device *pdev)
+{
+	int irq, result = 0;
+	struct octeon_i2c *i2c;
+	struct octeon_i2c_data *i2c_data;
+	struct resource *res_mem;
+
+	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+	if (!i2c) {
+		printk(KERN_ERR "%s i2c-cavium - kzalloc failed\n", __func__);
+		return -ENOMEM;
+	}
+	i2c->dev = &pdev->dev;
+	i2c_data = pdev->dev.platform_data;
+
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq = platform_get_irq(pdev, 0);
+
+	if (res_mem == NULL) {
+		dev_dbg(i2c->dev, "%s: found no memory resource\n", __func__);
+		kfree(i2c);
+		return -ENODEV;
+	}
+
+	if (i2c_data == NULL) {
+		dev_dbg(i2c->dev, "%s: no I2C frequence data\n", __func__);
+		kfree(i2c);
+		return -ENODEV;
+	}
+
+	i2c->twsi_phys = res_mem->start;
+	i2c->regsize = resource_size(res_mem);
+	i2c->twsi_freq = i2c_data->i2c_freq;
+	i2c->sys_freq = i2c_data->sys_freq;
+
+	if (!request_mem_region(i2c->twsi_phys, i2c->regsize, res_mem->name)) {
+		dev_dbg(i2c->dev,
+			"%s i2c-cavium - request_mem_region failed\n",
+			__func__);
+		goto fail_region;
+	}
+	i2c->twsi_base = ioremap(i2c->twsi_phys, i2c->regsize);
+
+	init_waitqueue_head(&i2c->queue);
+
+	i2c->irq = irq;
+	if (i2c->irq != NO_IRQ) {
+		/* i2c->irq = NO_IRQ implies polling */
+		result = request_irq(i2c->irq, octeon_i2c_isr, 0, DRV_NAME, i2c);
+		if (result < 0) {
+			dev_dbg(i2c->dev,
+				"%s: - failed to attach interrupt\n",
+				__func__);
+			goto fail_irq;
+		}
+	}
+
+	result = octeon_i2c_initlowlevel(i2c);
+	if (result) {
+		dev_dbg(i2c->dev, "%s: init low level failed\n", __func__);
+		goto  fail_add;
+	}
+
+	result = octeon_i2c_setclock(i2c);
+	if (result) {
+		dev_dbg(i2c->dev, "%s: clock init failed\n", __func__);
+		goto  fail_add;
+	}
+
+	i2c->adap = octeon_i2c_ops;
+	i2c->adap.dev.parent = &pdev->dev;
+	i2c->adap.nr = pdev->id >= 0 ? pdev->id : 0;
+	i2c_set_adapdata(&i2c->adap, i2c);
+	platform_set_drvdata(pdev, i2c);
+
+	result = i2c_add_numbered_adapter(&i2c->adap);
+	if (result < 0) {
+		dev_dbg(i2c->dev, "%s: failed to add adapter\n", __func__);
+		goto fail_add;
+	}
+	return result;
+
+fail_add:
+	platform_set_drvdata(pdev, NULL);
+	if (i2c->irq != NO_IRQ)
+		free_irq(i2c->irq, i2c);
+fail_irq:
+	iounmap(i2c->twsi_base);
+	release_mem_region(i2c->twsi_phys, i2c->regsize);
+fail_region:
+	kfree(i2c);
+	return result;
+};
+
+static int __exit octeon_i2c_remove(struct platform_device *pdev)
+{
+	struct octeon_i2c *i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c->adap);
+	platform_set_drvdata(pdev, NULL);
+	if (i2c->irq != NO_IRQ)
+		free_irq(i2c->irq, i2c);
+	iounmap(i2c->twsi_base);
+	release_mem_region(i2c->twsi_phys, i2c->regsize);
+	kfree(i2c);
+	return 0;
+};
+
+static struct platform_driver octeon_i2c_driver = {
+	.probe		= octeon_i2c_probe,
+	.remove		= __exit_p(octeon_i2c_remove),
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= DRV_NAME,
+	},
+};
+
+static int __init octeon_i2c_init(void)
+{
+	int rv;
+
+	rv = platform_driver_register(&octeon_i2c_driver);
+	printk(KERN_INFO "driver %s is loaded\n", DRV_NAME);
+	return rv;
+}
+
+static void __exit octeon_i2c_exit(void)
+{
+	platform_driver_unregister(&octeon_i2c_driver);
+	printk(KERN_INFO "driver %s unloaded\n", DRV_NAME);
+}
+
+MODULE_AUTHOR("Michael Lawnick <michael.lawnick.ext-OYasijW0DpE@public.gmane.org>");
+MODULE_DESCRIPTION("I2C-Bus adapter for Cavium OCTEON processors");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(DRV_VERSION);
+MODULE_ALIAS("platform:" DRV_NAME);
+
+module_init(octeon_i2c_init);
+module_exit(octeon_i2c_exit);
+
-- 
1.6.0.6

Re: [PATCH 2/3] I2C: Add driver for Cavium OCTEON I2C ports.

[Ben Dooks]

On Thu, Jan 07, 2010 at 11:54:20AM -0800, David Daney wrote:
> From: Rade Bozic <rade.bozic.ext-OYasijW0DpE@public.gmane.org>
> 
> Signed-off-by: Rade Bozic <rade.bozic.ext-OYasijW0DpE@public.gmane.org>
> Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig      |   10 +
>  drivers/i2c/busses/Makefile     |    1 +
>  drivers/i2c/busses/i2c-octeon.c |  579 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 590 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-octeon.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 5f318ce..737f052 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -564,6 +564,16 @@ config I2C_VERSATILE
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-versatile.
>  
> +config I2C_OCTEON
> +	tristate "Cavium OCTEON I2C bus support"
> +	depends on CPU_CAVIUM_OCTEON
> +	help
> +	  Say yes if you want to support the I2C serial bus on Cavium
> +	  OCTEON SOC.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-octeon.
> +
>  comment "External I2C/SMBus adapter drivers"
>  
>  config I2C_PARPORT
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 302c551..c2c4ea1 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
>  obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
>  obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
>  obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
> +obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
>  
>  # External I2C/SMBus adapter drivers
>  obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
> diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
> new file mode 100644
> index 0000000..57455ac
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-octeon.c
> @@ -0,0 +1,579 @@
> +/*
> + * (C) Copyright 2009-2010
> + * Nokia Siemens Networks, michael.lawnick.ext-OYasijW0DpE@public.gmane.org
> + *
> + * This is a driver for the i2c adapter in Cavium Networks' OCTEON processors.
> + *
> + * Release 0.1
> + *
> + * 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/init.h>
> +
> +#include <linux/io.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/octeon/octeon.h>
> +
> +#define DRV_NAME "i2c-octeon"
> +#define DRV_VERSION	"0.1"
> +
> +/* register offsets */
> +#define SW_TWSI	 0x00
> +#define TWSI_INT 0x10
> +
> +/* Controller command patterns */
> +#define SW_TWSI_V               0x8000000000000000ull
> +#define SW_TWSI_EOP_TWSI_DATA   0x0C00000100000000ull
> +#define SW_TWSI_EOP_TWSI_CTL    0x0C00000200000000ull
> +#define SW_TWSI_EOP_TWSI_CLKCTL 0x0C00000300000000ull
> +#define SW_TWSI_EOP_TWSI_STAT   0x0C00000300000000ull
> +#define SW_TWSI_EOP_TWSI_RST    0x0C00000700000000ull
> +#define SW_TWSI_OP_TWSI_CLK     0x0800000000000000ull
> +#define SW_TWSI_R               0x0100000000000000ull
> +
> +/* Controller command and status bits */
> +#define TWSI_CTL_CE   0x80
> +#define TWSI_CTL_ENAB 0x40
> +#define TWSI_CTL_STA  0x20
> +#define TWSI_CTL_STP  0x10
> +#define TWSI_CTL_IFLG 0x08
> +#define TWSI_CTL_AAK  0x04
> +
> +/* Some status values */
> +#define STAT_START      0x08
> +#define STAT_RSTART     0x10
> +#define STAT_TXADDR_ACK 0x18
> +#define STAT_TXDATA_ACK 0x28
> +#define STAT_RXADDR_ACK 0x40
> +#define STAT_RXDATA_ACK 0x50
> +#define STAT_IDLE       0xF8
> +
> +#ifndef NO_IRQ
> +#define NO_IRQ (-1)
> +#endif

this does not fill me with a warm joyous feeling... 

> +struct octeon_i2c {
> +	wait_queue_head_t queue;
> +	struct i2c_adapter adap;
> +	int irq;
> +	int twsi_freq;
> +	int sys_freq;
> +	uint8_t twsi_ctl;
> +	resource_size_t twsi_phys;
> +	void __iomem *twsi_base;
> +	resource_size_t regsize;
> +	struct device *dev;
> +};

kerneldoc or any documentation at-all would be nice.

> +/* Writes need to be flushed by a read. */
> +static void octeon_i2c_write_sw(struct octeon_i2c *i2c,
> +				uint64_t eop_reg,
> +				uint8_t data)
> +{
> +	uint64_t tmp;
> +
> +	__raw_writeq(SW_TWSI_V | eop_reg | data, i2c->twsi_base + SW_TWSI);
> +	tmp = __raw_readq(i2c->twsi_base + SW_TWSI);
> +}
> +
> +static uint8_t octeon_i2c_read_sw(struct octeon_i2c *i2c, uint64_t eop_reg)
> +{
> +	uint64_t tmp;
> +
> +	__raw_writeq(SW_TWSI_V | eop_reg | SW_TWSI_R, i2c->twsi_base + SW_TWSI);
> +	tmp = __raw_readq(i2c->twsi_base + SW_TWSI);
> +
> +	return __raw_readq(i2c->twsi_base + SW_TWSI) & 0xFF;
> +}
> +
> +static void octeon_i2c_write_int(struct octeon_i2c *i2c, uint64_t data)
> +{
> +	uint64_t tmp;
> +
> +	__raw_writeq(data, i2c->twsi_base + TWSI_INT);
> +	tmp = __raw_readq(i2c->twsi_base + TWSI_INT);
> +}
> +
> +static void octeon_i2c_int_enable(struct octeon_i2c *i2c)
> +{
> +	octeon_i2c_write_int(i2c, 0x40);
> +}
> +
> +static void octeon_i2c_int_disable(struct octeon_i2c *i2c)
> +{
> +	octeon_i2c_write_int(i2c, 0);
> +}
> +
> +/* If there was a reset while a device was driving 0 to bus,
> +   bus is blocked. We toggle it free manually by some clock
> +   cycles and send a stop. */
> +static void octeon_i2c_unblock(struct octeon_i2c *i2c)
> +{
> +	int i;
> +
> +	dev_dbg(i2c->dev, "%s\n", __func__);
> +	for (i = 0; i < 9; i++) {
> +		octeon_i2c_write_int(i2c, 0x0);
> +		udelay(5);
> +		octeon_i2c_write_int(i2c, 0x200);
> +		udelay(5);
> +	}
> +	octeon_i2c_write_int(i2c, 0x300);
> +	udelay(5);
> +	octeon_i2c_write_int(i2c, 0x100);
> +	udelay(5);
> +	octeon_i2c_write_int(i2c, 0x0);
> +}
> +
> +static irqreturn_t octeon_i2c_isr(int irq, void *dev_id)
> +{
> +	struct octeon_i2c *i2c = dev_id;
> +
> +	octeon_i2c_int_disable(i2c);
> +	i2c->twsi_ctl = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_CTL);
> +	wake_up_interruptible(&i2c->queue);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int octeon_i2c_wait(struct octeon_i2c *i2c)
> +{
> +	unsigned long orig_jiffies = jiffies;
> +	uint8_t ctl;
> +	int result = 0;
> +
> +	if (i2c->irq == NO_IRQ) {
> +		/* polling */
> +		do {
> +			ctl = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_CTL);
> +			if (ctl & TWSI_CTL_IFLG)
> +				break;
> +		} while (!time_after(jiffies,
> +					orig_jiffies + i2c->adap.timeout));
> +
> +		if (!(ctl & TWSI_CTL_IFLG)) {
> +			dev_dbg(i2c->dev, "%s: timeout\n", __func__);
> +			result = -ETIMEDOUT;
> +		}
> +	} else {
> +		i2c->twsi_ctl = 0;
> +		/* interrupt mode */
> +		octeon_i2c_int_enable(i2c);
> +
> +		result = wait_event_interruptible_timeout(i2c->queue,
> +				(i2c->twsi_ctl & TWSI_CTL_IFLG),
> +				i2c->adap.timeout);
> +
> +		if (unlikely(result < 0)) {
> +			dev_dbg(i2c->dev, "%s: wait interrupted\n", __func__);
> +		} else if (unlikely(!(i2c->twsi_ctl & TWSI_CTL_IFLG))) {
> +			dev_dbg(i2c->dev, "%s: timeout\n", __func__);
> +			result = -ETIMEDOUT;
> +		}
> +	}
> +
> +	return result < 0 ? result : 0;
> +}
> +
> +static int octeon_i2c_start(struct octeon_i2c *i2c)
> +{
> +	uint8_t data;
> +	int result;
> +
> +	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL,
> +				TWSI_CTL_ENAB | TWSI_CTL_STA);
> +
> +	result = octeon_i2c_wait(i2c);
> +	if (unlikely(result < 0) &&
> +	    octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT) == STAT_IDLE) {
> +		/*
> +		 * Controller refused to send start flag May be a
> +		 * client is holding SDA low - let's try to free it.
> +		 */
> +		octeon_i2c_unblock(i2c);
> +		octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL,
> +				    TWSI_CTL_ENAB | TWSI_CTL_STA);
> +
> +		result = octeon_i2c_wait(i2c);
> +		if (result < 0)
> +			return result;
> +	}
> +
> +	data = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
> +	if ((data != STAT_START) && (data != STAT_RSTART)) {
> +		dev_dbg(i2c->dev, "%s: bad status (0x%x)\n", __func__, data);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int octeon_i2c_stop(struct octeon_i2c *i2c)
> +{
> +	unsigned long orig_jiffies = jiffies;
> +	uint8_t data;
> +
> +	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL,
> +				TWSI_CTL_ENAB | TWSI_CTL_STP);
> +
> +	/*
> +	 * Controller will change to idle, but that does not raise an
> +	 * interrupt, so we have to poll.
> +	 */
> +	do {
> +		data = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
> +		if (data == STAT_IDLE)
> +			break;
> +		udelay(1);
> +	} while (!time_after(jiffies, orig_jiffies + 2));

you sure you want to busy wait like this?

> +	if (data != STAT_IDLE) {
> +		dev_dbg(i2c->dev, "%s: bad status(0x%x)\n", __func__, data);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static int octeon_i2c_write(struct octeon_i2c *i2c, int target,
> +			    const uint8_t *data, int length)
> +{
> +	int i, result;
> +	uint8_t tmp;
> +
> +	result = octeon_i2c_start(i2c);
> +	if (unlikely(result < 0))
> +		return result;
> +
> +	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_DATA, target << 1);
> +	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL, TWSI_CTL_ENAB);
> +
> +	result = octeon_i2c_wait(i2c);
> +	if (unlikely(result < 0))
> +		return result;
> +
> +	for (i = 0; i < length; i++) {
> +		tmp = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
> +		if ((tmp != STAT_TXADDR_ACK) && (tmp != STAT_TXDATA_ACK)) {
> +			dev_dbg(i2c->dev,
> +				"%s: bad status before write (0x%x)\n",
> +				__func__, tmp);
> +			return -(int)tmp;
> +		}
> +
> +		octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_DATA, data[i]);
> +		octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL, TWSI_CTL_ENAB);
> +
> +		result = octeon_i2c_wait(i2c);
> +		if (unlikely(result < 0))
> +			return result;
> +	}
> +
> +	return 0;
> +}
> +
> +static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
> +			   uint8_t *data, int length)
> +{
> +	int i, result;
> +	uint8_t tmp;
> +
> +	if (length < 1)
> +		return -EINVAL;
> +
> +	result = octeon_i2c_start(i2c);
> +	if (result)
> +		return result;
> +
> +	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_DATA, (target<<1) | 1);
> +	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL, TWSI_CTL_ENAB);
> +
> +	result = octeon_i2c_wait(i2c);
> +	if (unlikely(result < 0))
> +		return result;
> +
> +	for (i = 0; i < length; i++) {
> +		tmp = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
> +		if ((tmp != STAT_RXDATA_ACK) && (tmp != STAT_RXADDR_ACK)) {
> +			dev_dbg(i2c->dev,
> +				"%s: bad status before read (0x%x)\n",
> +				__func__, tmp);
> +			return -(int)tmp;
> +		}
> +
> +		if (i+1 < length)
> +			octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL,
> +						TWSI_CTL_ENAB | TWSI_CTL_AAK);
> +		else
> +			octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL,
> +						TWSI_CTL_ENAB);
> +
> +		result = octeon_i2c_wait(i2c);
> +		if (unlikely(result < 0))
> +			return result;
> +
> +		data[i] = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_DATA);
> +	}
> +	return length;
> +}
> +
> +static int octeon_i2c_xfer(struct i2c_adapter *adap,
> +			   struct i2c_msg *msgs,
> +			   int num)
> +{
> +	struct i2c_msg *pmsg;
> +	int i;
> +	int ret = 0;
> +	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
> +
> +	for (i = 0; ret >= 0 && i < num; i++) {
> +		pmsg = &msgs[i];
> +		dev_dbg(i2c->dev,
> +			"%s: Doing %s %d byte(s) to/from 0x%02x - "
> +			"%d of %d messages\n",

i'd rather not see text lines broken over like this, it is one
case where >80 character lines are allowed. You could always shorten
it by removing the "to/from" and having "%d/%d msgs" at the end
and possible removing the "Doing" as well.

> +			 __func__,
> +			 pmsg->flags & I2C_M_RD ? "read" : "write",
> +			 pmsg->len, pmsg->addr, i + 1, num);
> +		if (pmsg->flags & I2C_M_RD)
> +			ret = octeon_i2c_read(i2c, pmsg->addr, pmsg->buf,
> +						pmsg->len);
> +		else
> +			ret = octeon_i2c_write(i2c, pmsg->addr, pmsg->buf,
> +						pmsg->len);
> +	}
> +	octeon_i2c_stop(i2c);
> +
> +	return (ret < 0) ? ret : num;
> +}
> +
> +static u32 octeon_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm octeon_i2c_algo = {
> +	.master_xfer = octeon_i2c_xfer,
> +	.functionality = octeon_i2c_functionality,
> +};
> +
> +static struct i2c_adapter octeon_i2c_ops = {
> +	.owner = THIS_MODULE,
> +	.name = "OCTEON adapter",
> +	.algo = &octeon_i2c_algo,
> +	.timeout = 2,
> +};
> +
> +static int __init octeon_i2c_setclock(struct octeon_i2c *i2c)
> +{
> +	int tclk, thp_base, inc, thp_idx, mdiv_idx, ndiv_idx, foscl, diff;
> +	int thp = 0x18, mdiv = 2, ndiv = 0, delta_hz = 1000000;
> +
> +	for (ndiv_idx = 0; ndiv_idx < 8 && delta_hz != 0; ndiv_idx++) {
> +		/*
> +		 * An mdiv value of less than 2 seems to not work well
> +		 * with ds1337 RTCs, so we constrain it to larger
> +		 * values.
> +		 */
> +		for (mdiv_idx = 15; mdiv_idx >= 2 && delta_hz != 0; mdiv_idx--) {
> +			/*
> +			 * For given ndiv and mdiv values check the
> +			 * two closest thp values.
> +			 */
> +			tclk = i2c->twsi_freq * (mdiv_idx + 1) * 10;
> +			tclk *= (1 << ndiv_idx);
> +			thp_base = (i2c->sys_freq / (tclk * 2)) - 1;
> +			for (inc = 0; inc <= 1; inc++) {
> +				thp_idx = thp_base + inc;
> +				if (thp_idx < 5 || thp_idx > 0xff)
> +					continue;
> +
> +				foscl = i2c->sys_freq / (2 * (thp_idx + 1));
> +				foscl = foscl / (1 << ndiv_idx);
> +				foscl = foscl / (mdiv_idx + 1) / 10;
> +				diff = abs(foscl - i2c->twsi_freq);
> +				if (diff < delta_hz) {
> +					delta_hz = diff;
> +					thp = thp_idx;
> +					mdiv = mdiv_idx;
> +					ndiv = ndiv_idx;
> +				}
> +			}
> +		}
> +	}
> +	octeon_i2c_write_sw(i2c, SW_TWSI_OP_TWSI_CLK, thp);
> +	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CLKCTL, (mdiv << 3) | ndiv);
> +
> +	return 0;
> +}
> +
> +static int __init octeon_i2c_initlowlevel(struct octeon_i2c *i2c)
> +{
> +	uint8_t status;
> +	int tries;
> +
> +	/* disable high level controller, enable bus access */
> +	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_CTL, TWSI_CTL_ENAB);
> +
> +	/* reset controller */
> +	octeon_i2c_write_sw(i2c, SW_TWSI_EOP_TWSI_RST, 0);
> +
> +	for (tries = 10; tries; tries--) {
> +		udelay(1);
> +		status = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
> +		if (status == STAT_IDLE)
> +			return 0;
> +	}
> +	dev_dbg(i2c->dev, "%s: TWSI_RST failed! (0x%x)\n", __func__, status);
> +	return -1;
> +}
> +
> +static int __init octeon_i2c_probe(struct platform_device *pdev)
> +{
> +	int irq, result = 0;
> +	struct octeon_i2c *i2c;
> +	struct octeon_i2c_data *i2c_data;
> +	struct resource *res_mem;
> +
> +	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c) {
> +		printk(KERN_ERR "%s i2c-cavium - kzalloc failed\n", __func__);
> +		return -ENOMEM;
> +	}
> +	i2c->dev = &pdev->dev;
> +	i2c_data = pdev->dev.platform_data;
> +
> +	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	irq = platform_get_irq(pdev, 0);
> +
> +	if (res_mem == NULL) {
> +		dev_dbg(i2c->dev, "%s: found no memory resource\n", __func__);
> +		kfree(i2c);
> +		return -ENODEV;
> +	}
> +
> +	if (i2c_data == NULL) {
> +		dev_dbg(i2c->dev, "%s: no I2C frequence data\n", __func__);
> +		kfree(i2c);
> +		return -ENODEV;
> +	}

returning -ENODEV isn't a good idea, the device layer won't print an
error on seeing -ENODEV as it thinks this is a probe for a device that
was never there.

> +	i2c->twsi_phys = res_mem->start;
> +	i2c->regsize = resource_size(res_mem);
> +	i2c->twsi_freq = i2c_data->i2c_freq;
> +	i2c->sys_freq = i2c_data->sys_freq;
> +
> +	if (!request_mem_region(i2c->twsi_phys, i2c->regsize, res_mem->name)) {
> +		dev_dbg(i2c->dev,
> +			"%s i2c-cavium - request_mem_region failed\n",
> +			__func__);
> +		goto fail_region;
> +	}
> +	i2c->twsi_base = ioremap(i2c->twsi_phys, i2c->regsize);
> +
> +	init_waitqueue_head(&i2c->queue);
> +
> +	i2c->irq = irq;
> +	if (i2c->irq != NO_IRQ) {

platform_get_irq() returns a negative error cdoe if no irq is found,
so you need to do (irq < 0) here otherwise it'll never trip. Should
also mean yo ucan get rid of NO_IRQ define above.

> +		/* i2c->irq = NO_IRQ implies polling */
> +		result = request_irq(i2c->irq, octeon_i2c_isr, 0, DRV_NAME, i2c);
> +		if (result < 0) {
> +			dev_dbg(i2c->dev,
> +				"%s: - failed to attach interrupt\n",
> +				__func__);
> +			goto fail_irq;
> +		}
> +	}
> +
> +	result = octeon_i2c_initlowlevel(i2c);
> +	if (result) {
> +		dev_dbg(i2c->dev, "%s: init low level failed\n", __func__);
> +		goto  fail_add;
> +	}
> +
> +	result = octeon_i2c_setclock(i2c);
> +	if (result) {
> +		dev_dbg(i2c->dev, "%s: clock init failed\n", __func__);
> +		goto  fail_add;
> +	}
> +
> +	i2c->adap = octeon_i2c_ops;
> +	i2c->adap.dev.parent = &pdev->dev;
> +	i2c->adap.nr = pdev->id >= 0 ? pdev->id : 0;
> +	i2c_set_adapdata(&i2c->adap, i2c);
> +	platform_set_drvdata(pdev, i2c);
> +
> +	result = i2c_add_numbered_adapter(&i2c->adap);
> +	if (result < 0) {
> +		dev_dbg(i2c->dev, "%s: failed to add adapter\n", __func__);
> +		goto fail_add;
> +	}
> +	return result;
> +
> +fail_add:
> +	platform_set_drvdata(pdev, NULL);
> +	if (i2c->irq != NO_IRQ)
> +		free_irq(i2c->irq, i2c);
> +fail_irq:
> +	iounmap(i2c->twsi_base);
> +	release_mem_region(i2c->twsi_phys, i2c->regsize);
> +fail_region:
> +	kfree(i2c);
> +	return result;
> +};
> +
> +static int __exit octeon_i2c_remove(struct platform_device *pdev)
> +{
> +	struct octeon_i2c *i2c = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&i2c->adap);
> +	platform_set_drvdata(pdev, NULL);
> +	if (i2c->irq != NO_IRQ)
> +		free_irq(i2c->irq, i2c);

see above comment on NO_IRQ

> +	iounmap(i2c->twsi_base);
> +	release_mem_region(i2c->twsi_phys, i2c->regsize);
> +	kfree(i2c);
> +	return 0;
> +};
> +
> +static struct platform_driver octeon_i2c_driver = {
> +	.probe		= octeon_i2c_probe,
> +	.remove		= __exit_p(octeon_i2c_remove),
__devexit_p() here, probably should change __exit to __devexit on the
remove function.
> +	.driver		= {
> +		.owner	= THIS_MODULE,
> +		.name	= DRV_NAME,
> +	},
> +};
> +
> +static int __init octeon_i2c_init(void)
> +{
> +	int rv;
> +
> +	rv = platform_driver_register(&octeon_i2c_driver);
> +	printk(KERN_INFO "driver %s is loaded\n", DRV_NAME);
> +	return rv;
> +}

I'd rather not see 'driver loaded' messages if possible.


> +static void __exit octeon_i2c_exit(void)
> +{
> +	platform_driver_unregister(&octeon_i2c_driver);
> +	printk(KERN_INFO "driver %s unloaded\n", DRV_NAME);
> +}

I'd say this is even less useful that the one in the init code.

> +MODULE_AUTHOR("Michael Lawnick <michael.lawnick.ext-OYasijW0DpE@public.gmane.org>");
> +MODULE_DESCRIPTION("I2C-Bus adapter for Cavium OCTEON processors");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_ALIAS("platform:" DRV_NAME);
> +
> +module_init(octeon_i2c_init);
> +module_exit(octeon_i2c_exit);

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

  'a smiley only costs 4 bytes'

RE: [PATCH 2/3] I2C: Add driver for Cavium OCTEON I2C ports.

[Bozic, Rade (EXT-Other - DE/Ulm)]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]

Hello David,

Yesterday Ben Dooks wrote a email with some new issues ( Message-ID: <20100124160017.GF28675@fluff.org.uk>).

New changes:

1. Some return values was Changed
2. Changed __exit_p to __devexit_p and __exit into __devexit
3. New handling of NO_IRQ


In the appendix is a new I2C patch.
Maybe we can put the patch also in the Kernel.



Regards
Rade

[-- Attachment #2: i2c-octeon_25.12.2010.patch --]
[-- Type: application/octet-stream, Size: 3244 bytes --]

Signed-off-by: Rade Bozic <rade.bozic.ext@nsn.com>
Signed-off-by: David Daney <ddaney@caviumnetworks.com>

---> 
diff -purN linux-2.6_org/drivers/i2c/busses/i2c-octeon.c linux-2.6_i2c/drivers/i2c/busses/i2c-octeon.c
--- linux-2.6_org/drivers/i2c/busses/i2c-octeon.c	2010-01-22 16:33:23.000000000 +0100
+++ linux-2.6_i2c/drivers/i2c/busses/i2c-octeon.c	2010-01-25 15:33:23.000000000 +0100
@@ -10,7 +10,6 @@
  * License version 2. This program is licensed "as is" without any
  * warranty of any kind, whether express or implied.
  */
-
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/sched.h>
@@ -352,7 +351,7 @@ static int octeon_i2c_xfer(struct i2c_ad
 	}
 	octeon_i2c_stop(i2c);
 
-	return (ret < 0) ? ret : num;
+	return (ret < 0) ? EIO : num;
 }
 
 static u32 octeon_i2c_functionality(struct i2c_adapter *adap)
@@ -457,13 +456,13 @@ static int __init octeon_i2c_probe(struc
 	if (res_mem == NULL) {
 		dev_dbg(i2c->dev, "%s: found no memory resource\n", __func__);
 		kfree(i2c);
-		return -ENODEV;
+		return -EINVAL;
 	}
 
 	if (i2c_data == NULL) {
 		dev_dbg(i2c->dev, "%s: no I2C frequence data\n", __func__);
 		kfree(i2c);
-		return -ENODEV;
+		return -EINVAL;
 	}
 
 	i2c->twsi_phys = res_mem->start;
@@ -475,6 +474,7 @@ static int __init octeon_i2c_probe(struc
 		dev_dbg(i2c->dev,
 			"%s i2c-cavium - request_mem_region failed\n",
 			__func__);
+		result = -ENOMEM;
 		goto fail_region;
 	}
 	i2c->twsi_base = ioremap(i2c->twsi_phys, i2c->regsize);
@@ -482,27 +482,37 @@ static int __init octeon_i2c_probe(struc
 	init_waitqueue_head(&i2c->queue);
 
 	i2c->irq = irq;
+
+	/* i2c->irq == NO_IRQ implies polling */
 	if (i2c->irq != NO_IRQ) {
-		/* i2c->irq = NO_IRQ implies polling */
-		result = request_irq(i2c->irq, octeon_i2c_isr, 0, DRV_NAME, i2c);
-		if (result < 0) {
+		if (i2c->irq < 0) {
 			dev_dbg(i2c->dev,
-				"%s: - failed to attach interrupt\n",
+				"%s: - bad interrupt number\n",
 				__func__);
+			result = -EINVAL;
 			goto fail_irq;
+		} else {
+			result = request_irq(i2c->irq, octeon_i2c_isr, 0,
+								DRV_NAME, i2c);
+			if (result < 0) {
+				dev_dbg(i2c->dev,
+					"%s: - failed to attach interrupt\n",
+					__func__);
+				goto fail_irq;
+			}
 		}
 	}
 
 	result = octeon_i2c_initlowlevel(i2c);
 	if (result) {
 		dev_dbg(i2c->dev, "%s: init low level failed\n", __func__);
-		goto  fail_add;
+		goto fail_add;
 	}
 
 	result = octeon_i2c_setclock(i2c);
 	if (result) {
 		dev_dbg(i2c->dev, "%s: clock init failed\n", __func__);
-		goto  fail_add;
+		goto fail_add;
 	}
 
 	i2c->adap = octeon_i2c_ops;
@@ -530,7 +540,7 @@ fail_region:
 	return result;
 };
 
-static int __exit octeon_i2c_remove(struct platform_device *pdev)
+static int __devexit octeon_i2c_remove(struct platform_device *pdev)
 {
 	struct octeon_i2c *i2c = platform_get_drvdata(pdev);
 
@@ -546,7 +556,7 @@ static int __exit octeon_i2c_remove(stru
 
 static struct platform_driver octeon_i2c_driver = {
 	.probe		= octeon_i2c_probe,
-	.remove		= __exit_p(octeon_i2c_remove),
+	.remove		= __devexit_p(octeon_i2c_remove),
 	.driver		= {
 		.owner	= THIS_MODULE,
 		.name	= DRV_NAME,
@@ -576,4 +586,3 @@ MODULE_ALIAS("platform:" DRV_NAME);
 
 module_init(octeon_i2c_init);
 module_exit(octeon_i2c_exit);
-

Re: [PATCH 2/3] I2C: Add driver for Cavium OCTEON I2C ports.

[Michael Lawnick]

Hi,

this is the author.

Ben Dooks said the following:
> On Thu, Jan 07, 2010 at 11:54:20AM -0800, David Daney wrote:
<snip>
>> +#ifndef NO_IRQ
>> +#define NO_IRQ (-1)
>> +#endif
> 
> this does not fill me with a warm joyous feeling... 

see near end of reply.

> 
>> +struct octeon_i2c {
>> +	wait_queue_head_t queue;
>> +	struct i2c_adapter adap;
>> +	int irq;
>> +	int twsi_freq;
>> +	int sys_freq;
>> +	uint8_t twsi_ctl;
>> +	resource_size_t twsi_phys;
>> +	void __iomem *twsi_base;
>> +	resource_size_t regsize;
>> +	struct device *dev;
>> +};
> 
> kerneldoc or any documentation at-all would be nice.

This is driver private data, used within this file only. Usage should be
rather obvious?!

<snip>

>> +	do {
>> +		data = octeon_i2c_read_sw(i2c, SW_TWSI_EOP_TWSI_STAT);
>> +		if (data == STAT_IDLE)
>> +			break;
>> +		udelay(1);
>> +	} while (!time_after(jiffies, orig_jiffies + 2));
> 
> you sure you want to busy wait like this?
> 
Hmm, yes?
Its a busy wait of typically 10us, but not guaranteed.
Timeout is 1 to 2 jiffies.
Any other suggestion?

<snip>

>> +	i2c_data = pdev->dev.platform_data;
>> +
>> +	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	irq = platform_get_irq(pdev, 0);
>> +
>> +	if (res_mem == NULL) {
>> +		dev_dbg(i2c->dev, "%s: found no memory resource\n", __func__);
>> +		kfree(i2c);
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (i2c_data == NULL) {
>> +		dev_dbg(i2c->dev, "%s: no I2C frequence data\n", __func__);
>> +		kfree(i2c);
>> +		return -ENODEV;
>> +	}
> 
> returning -ENODEV isn't a good idea, the device layer won't print an
> error on seeing -ENODEV as it thinks this is a probe for a device that
> was never there.
> 
ACK, couldn't find something really appropriate. Will change to EINVAL.

<snip>

>> +	i2c->twsi_phys = res_mem->start;
>> +	i2c->regsize = resource_size(res_mem);
>> +	i2c->twsi_freq = i2c_data->i2c_freq;
>> +	i2c->sys_freq = i2c_data->sys_freq;
>> +
>> +	if (!request_mem_region(i2c->twsi_phys, i2c->regsize, res_mem->name)) {
>> +		dev_dbg(i2c->dev,
>> +			"%s i2c-cavium - request_mem_region failed\n",
>> +			__func__);
>> +		goto fail_region;
>> +	}
>> +	i2c->twsi_base = ioremap(i2c->twsi_phys, i2c->regsize);
>> +
>> +	init_waitqueue_head(&i2c->queue);
>> +
>> +	i2c->irq = irq;
>> +	if (i2c->irq != NO_IRQ) {
> 
> platform_get_irq() returns a negative error cdoe if no irq is found,
> so you need to do (irq < 0) here otherwise it'll never trip. Should
> also mean yo ucan get rid of NO_IRQ define above.

NO_IRQ is thought of as polling.
The code assumes that request_irq() denies irq < 0
After reviewing kernel code I think this should not be assumed.
I will have to contemplate about this.

<snip>

>> +static struct platform_driver octeon_i2c_driver = {
>> +	.probe		= octeon_i2c_probe,
>> +	.remove		= __exit_p(octeon_i2c_remove),
> __devexit_p() here, probably should change __exit to __devexit on the
> remove function.

I thought I had read something about abuse of __devexit (to be used only
for hotplugable devices), but I can't find it anymore.
I'll change.

Currently I'm unsure about using __devinit for probe().
It will be called twice (2 devices), but I can't absolutely tell that
second call will be in system initialization phase.
Your 2 Cents?

>> +	.driver		= {
>> +		.owner	= THIS_MODULE,
>> +		.name	= DRV_NAME,
>> +	},
>> +};
>> +
>> +static int __init octeon_i2c_init(void)
>> +{
>> +	int rv;
>> +
>> +	rv = platform_driver_register(&octeon_i2c_driver);
>> +	printk(KERN_INFO "driver %s is loaded\n", DRV_NAME);
>> +	return rv;
>> +}
> 
> I'd rather not see 'driver loaded' messages if possible.

Whats about adjusting your message filter? ;-)
Would KERN_DEBUG be more appropriate?
We love such messages in our projects. You can filter for the 'loaded
phrases and have an fast overview this way.
-- 
Kind Regards,
Michael

Re: [PATCH 2/3] I2C: Add driver for Cavium OCTEON I2C ports.

[David Daney]

Ben Dooks wrote:
> On Thu, Jan 07, 2010 at 11:54:20AM -0800, David Daney wrote:
>> From: Rade Bozic <rade.bozic.ext-OYasijW0DpE@public.gmane.org>
>>
>> Signed-off-by: Rade Bozic <rade.bozic.ext-OYasijW0DpE@public.gmane.org>
>> Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
>> ---
>>  drivers/i2c/busses/Kconfig      |   10 +
>>  drivers/i2c/busses/Makefile     |    1 +
>>  drivers/i2c/busses/i2c-octeon.c |  579 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 590 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/i2c/busses/i2c-octeon.c
[...]
>> +
>> +#ifndef NO_IRQ
>> +#define NO_IRQ (-1)
>> +#endif
> 
> this does not fill me with a warm joyous feeling... 
> 

Indeed.

[ many other helpful comments deleted for brevity.]

We will improve the driver and resubmit.

Thanks for looking at it.

David Daney

[PATCH 3/3] MIPS: Octeon: Register some devices on the I2C bus.

[David Daney]

Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
CC: Rade Bozic <rade.bozic.ext-OYasijW0DpE@public.gmane.org>
---
 arch/mips/cavium-octeon/octeon-platform.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/arch/mips/cavium-octeon/octeon-platform.c b/arch/mips/cavium-octeon/octeon-platform.c
index f2c0602..d8f8129 100644
--- a/arch/mips/cavium-octeon/octeon-platform.c
+++ b/arch/mips/cavium-octeon/octeon-platform.c
@@ -10,6 +10,7 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/irq.h>
+#include <linux/i2c.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -165,6 +166,18 @@ out:
 }
 device_initcall(octeon_rng_device_init);
 
+static struct i2c_board_info __initdata octeon_i2c_devices[] = {
+	{
+		I2C_BOARD_INFO("ds1337", 0x68),
+	},
+};
+
+static int __init octeon_i2c_devices_init(void)
+{
+	return i2c_register_board_info(0, octeon_i2c_devices,
+				       ARRAY_SIZE(octeon_i2c_devices));
+}
+arch_initcall(octeon_i2c_devices_init);
 
 #define OCTEON_I2C_IO_BASE 0x1180000001000ull
 #define OCTEON_I2C_IO_UNIT_OFFSET 0x200
-- 
1.6.0.6

Re: [PATCH 0/3] Add I2C support for Octeon SOCs.

[David Daney]

Bah...  I forgot to include Rade Bozic on the original message.  Now 
included.

David Daney wrote:
> This patch set adds I2C driver support for Cavium Networks' Octeon
> processor family.  The Octeon is a multi-core MIPS64 based SOC.
> 
> The first patch adds platform devices for the I2C devices.  The second
> patch is the main driver.  Finally the third patch registers some
> devices so we have something to control with the fancy new driver.
> 
> I will reply with the three patches.
> 
> David Daney (2):
>   MIPS: Octeon: Add I2C platform driver.
>   MIPS: Octeon: Register some devices on the I2C bus.
> 
> Rade Bozic (1):
>   I2C: Add driver for Cavium OCTEON I2C ports.
> 
>  arch/mips/cavium-octeon/octeon-platform.c |   85 +++++
>  arch/mips/include/asm/octeon/octeon.h     |    5 +
>  drivers/i2c/busses/Kconfig                |   10 +
>  drivers/i2c/busses/Makefile               |    1 +
>  drivers/i2c/busses/i2c-octeon.c           |  579 
> +++++++++++++++++++++++++++++
>  5 files changed, 680 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-octeon.c
> 
> 

Re: [PATCH 0/3] Add I2C support for Octeon SOCs.

[Ralf Baechle]

On Thu, Jan 07, 2010 at 11:56:33AM -0800, David Daney wrote:

> David Daney wrote:
> >This patch set adds I2C driver support for Cavium Networks' Octeon
> >processor family.  The Octeon is a multi-core MIPS64 based SOC.
> >
> >The first patch adds platform devices for the I2C devices.  The second
> >patch is the main driver.  Finally the third patch registers some
> >devices so we have something to control with the fancy new driver.
> >
> >I will reply with the three patches.
> >
> >David Daney (2):
> >  MIPS: Octeon: Add I2C platform driver.
> >  MIPS: Octeon: Register some devices on the I2C bus.
> >
> >Rade Bozic (1):
> >  I2C: Add driver for Cavium OCTEON I2C ports.

Do you want to merge this series through the MIPS tree?

  Ralf

Re: [PATCH 0/3] Add I2C support for Octeon SOCs.

[David Daney]

Ralf Baechle wrote:
> On Thu, Jan 07, 2010 at 11:56:33AM -0800, David Daney wrote:
> 
>> David Daney wrote:
>>> This patch set adds I2C driver support for Cavium Networks' Octeon
>>> processor family.  The Octeon is a multi-core MIPS64 based SOC.
>>>
>>> The first patch adds platform devices for the I2C devices.  The second
>>> patch is the main driver.  Finally the third patch registers some
>>> devices so we have something to control with the fancy new driver.
>>>
>>> I will reply with the three patches.
>>>
>>> David Daney (2):
>>>  MIPS: Octeon: Add I2C platform driver.
>>>  MIPS: Octeon: Register some devices on the I2C bus.
>>>
>>> Rade Bozic (1):
>>>  I2C: Add driver for Cavium OCTEON I2C ports.
> 
> Do you want to merge this series through the MIPS tree?
> 

Two of the patches touch only arch/mips/cavium-octeon, so it might make 
sense.  But the I2C maintainers may have other desires, so I would defer 
to them.

David Daney

Re: [PATCH 0/3] Add I2C support for Octeon SOCs.

[Jean Delvare]

On Mon, 11 Jan 2010 09:16:03 -0800, David Daney wrote:
> Ralf Baechle wrote:
> > On Thu, Jan 07, 2010 at 11:56:33AM -0800, David Daney wrote:
> > 
> >> David Daney wrote:
> >>> This patch set adds I2C driver support for Cavium Networks' Octeon
> >>> processor family.  The Octeon is a multi-core MIPS64 based SOC.
> >>>
> >>> The first patch adds platform devices for the I2C devices.  The second
> >>> patch is the main driver.  Finally the third patch registers some
> >>> devices so we have something to control with the fancy new driver.
> >>>
> >>> I will reply with the three patches.
> >>>
> >>> David Daney (2):
> >>>  MIPS: Octeon: Add I2C platform driver.
> >>>  MIPS: Octeon: Register some devices on the I2C bus.
> >>>
> >>> Rade Bozic (1):
> >>>  I2C: Add driver for Cavium OCTEON I2C ports.
> > 
> > Do you want to merge this series through the MIPS tree?
> 
> Two of the patches touch only arch/mips/cavium-octeon, so it might make 
> sense.  But the I2C maintainers may have other desires, so I would defer 
> to them.

I won't take care of them, so I have no objection if the go through the
MIPS tree. For a platform-specific driver I think it makes a lot of
sense.

-- 
Jean Delvare

Re: [PATCH 0/3] Add I2C support for Octeon SOCs.

[Markus Gothe]

Methinks this goes to I2C...

//Markus
On 11 Jan 2010, at 18:16, David Daney wrote:

> Ralf Baechle wrote:
>> On Thu, Jan 07, 2010 at 11:56:33AM -0800, David Daney wrote:
>>> David Daney wrote:
>>>> This patch set adds I2C driver support for Cavium Networks' Octeon
>>>> processor family.  The Octeon is a multi-core MIPS64 based SOC.
>>>>
>>>> The first patch adds platform devices for the I2C devices.  The  
>>>> second
>>>> patch is the main driver.  Finally the third patch registers some
>>>> devices so we have something to control with the fancy new driver.
>>>>
>>>> I will reply with the three patches.
>>>>
>>>> David Daney (2):
>>>> MIPS: Octeon: Add I2C platform driver.
>>>> MIPS: Octeon: Register some devices on the I2C bus.
>>>>
>>>> Rade Bozic (1):
>>>> I2C: Add driver for Cavium OCTEON I2C ports.
>> Do you want to merge this series through the MIPS tree?
>
> Two of the patches touch only arch/mips/cavium-octeon, so it might  
> make sense.  But the I2C maintainers may have other desires, so I  
> would defer to them.
>
> David Daney
>

Re: [PATCH 0/3] Add I2C support for Octeon SOCs.

[Jean Delvare]

On Wed, 13 Jan 2010 01:49:08 +0100, Markus Gothe wrote:
> Methinks this goes to I2C...

But given that you are not maintaining any part of the kernel, what you
think on this matter doesn't have much value, methinks. You do not
seriously intend to tell maintainers how they should work together, do
you?

-- 
Jean Delvare