[PATCH 1/2] sfc: Use kernel I2C system and i2c-algo-bit driver

[PATCH 1/2] sfc: Use kernel I2C system and i2c-algo-bit driver

[Ben Hutchings]

Remove our own implementation of I2C bit-banging.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sfc/Kconfig      |    2 +
 drivers/net/sfc/Makefile     |    2 +-
 drivers/net/sfc/boards.c     |    2 +-
 drivers/net/sfc/boards.h     |    3 +-
 drivers/net/sfc/efx.c        |    2 +
 drivers/net/sfc/falcon.c     |   72 ++++++---
 drivers/net/sfc/i2c-direct.c |  381 ------------------------------------------
 drivers/net/sfc/i2c-direct.h |   91 ----------
 drivers/net/sfc/net_driver.h |   11 +-
 drivers/net/sfc/sfe4001.c    |  126 ++++++++-------
 10 files changed, 133 insertions(+), 559 deletions(-)
 delete mode 100644 drivers/net/sfc/i2c-direct.c
 delete mode 100644 drivers/net/sfc/i2c-direct.h

diff --git a/drivers/net/sfc/Kconfig b/drivers/net/sfc/Kconfig
index dbad95c..3be13b5 100644
--- a/drivers/net/sfc/Kconfig
+++ b/drivers/net/sfc/Kconfig
@@ -4,6 +4,8 @@ config SFC
 	select MII
 	select INET_LRO
 	select CRC32
+	select I2C
+	select I2C_ALGOBIT
 	help
 	  This driver supports 10-gigabit Ethernet cards based on
 	  the Solarflare Communications Solarstorm SFC4000 controller.
diff --git a/drivers/net/sfc/Makefile b/drivers/net/sfc/Makefile
index 1d2daee..c8f5704 100644
--- a/drivers/net/sfc/Makefile
+++ b/drivers/net/sfc/Makefile
@@ -1,5 +1,5 @@
 sfc-y			+= efx.o falcon.o tx.o rx.o falcon_xmac.o \
-			   i2c-direct.o selftest.o ethtool.o xfp_phy.o \
+			   selftest.o ethtool.o xfp_phy.o \
 			   mdio_10g.o tenxpress.o boards.o sfe4001.o
 
 obj-$(CONFIG_SFC)	+= sfc.o
diff --git a/drivers/net/sfc/boards.c b/drivers/net/sfc/boards.c
index 7fc0328..d3d3dd0 100644
--- a/drivers/net/sfc/boards.c
+++ b/drivers/net/sfc/boards.c
@@ -109,7 +109,7 @@ static struct efx_board_data board_data[] = {
 	[EFX_BOARD_INVALID] =
 	{NULL,	    NULL,                  dummy_init},
 	[EFX_BOARD_SFE4001] =
-	{"SFE4001", "10GBASE-T adapter",   sfe4001_poweron},
+	{"SFE4001", "10GBASE-T adapter",   sfe4001_init},
 	[EFX_BOARD_SFE4002] =
 	{"SFE4002", "XFP adapter",         sfe4002_init},
 };
diff --git a/drivers/net/sfc/boards.h b/drivers/net/sfc/boards.h
index 695764d..e5e8443 100644
--- a/drivers/net/sfc/boards.h
+++ b/drivers/net/sfc/boards.h
@@ -20,8 +20,7 @@ enum efx_board_type {
 };
 
 extern int efx_set_board_info(struct efx_nic *efx, u16 revision_info);
-extern int sfe4001_poweron(struct efx_nic *efx);
-extern void sfe4001_poweroff(struct efx_nic *efx);
+extern int sfe4001_init(struct efx_nic *efx);
 /* Are we putting the PHY into flash config mode */
 extern unsigned int sfe4001_phy_flash_cfg;
 
diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 4497606..74265d8 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -1815,6 +1815,7 @@ static struct efx_board efx_dummy_board_info = {
 	.init    = efx_nic_dummy_op_int,
 	.init_leds = efx_port_dummy_op_int,
 	.set_fault_led = efx_port_dummy_op_blink,
+	.fini	= efx_port_dummy_op_void,
 };
 
 /**************************************************************************
@@ -1941,6 +1942,7 @@ static void efx_pci_remove_main(struct efx_nic *efx)
 	efx_fini_port(efx);
 
 	/* Shutdown the board, then the NIC and board state */
+	efx->board_info.fini(efx);
 	falcon_fini_interrupt(efx);
 
 	efx_fini_napi(efx);
diff --git a/drivers/net/sfc/falcon.c b/drivers/net/sfc/falcon.c
index d3f749c..8acd53d 100644
--- a/drivers/net/sfc/falcon.c
+++ b/drivers/net/sfc/falcon.c
@@ -13,6 +13,8 @@
 #include <linux/pci.h>
 #include <linux/module.h>
 #include <linux/seq_file.h>
+#include <linux/i2c.h>
+#include <linux/i2c-algo-bit.h>
 #include "net_driver.h"
 #include "bitfield.h"
 #include "efx.h"
@@ -36,10 +38,12 @@
  * struct falcon_nic_data - Falcon NIC state
  * @next_buffer_table: First available buffer table id
  * @pci_dev2: The secondary PCI device if present
+ * @i2c_data: Operations and state for I2C bit-bashing algorithm
  */
 struct falcon_nic_data {
 	unsigned next_buffer_table;
 	struct pci_dev *pci_dev2;
+	struct i2c_algo_bit_data i2c_data;
 };
 
 /**************************************************************************
@@ -175,39 +179,57 @@ static inline int falcon_event_present(efx_qword_t *event)
  *
  **************************************************************************
  */
-static void falcon_setsdascl(struct efx_i2c_interface *i2c)
+static void falcon_setsda(void *data, int state)
 {
+	struct efx_nic *efx = (struct efx_nic *)data;
 	efx_oword_t reg;
 
-	falcon_read(i2c->efx, &reg, GPIO_CTL_REG_KER);
-	EFX_SET_OWORD_FIELD(reg, GPIO0_OEN, (i2c->scl ? 0 : 1));
-	EFX_SET_OWORD_FIELD(reg, GPIO3_OEN, (i2c->sda ? 0 : 1));
-	falcon_write(i2c->efx, &reg, GPIO_CTL_REG_KER);
+	falcon_read(efx, &reg, GPIO_CTL_REG_KER);
+	EFX_SET_OWORD_FIELD(reg, GPIO3_OEN, !state);
+	falcon_write(efx, &reg, GPIO_CTL_REG_KER);
 }
 
-static int falcon_getsda(struct efx_i2c_interface *i2c)
+static void falcon_setscl(void *data, int state)
 {
+	struct efx_nic *efx = (struct efx_nic *)data;
 	efx_oword_t reg;
 
-	falcon_read(i2c->efx, &reg, GPIO_CTL_REG_KER);
+	falcon_read(efx, &reg, GPIO_CTL_REG_KER);
+	EFX_SET_OWORD_FIELD(reg, GPIO0_OEN, !state);
+	falcon_write(efx, &reg, GPIO_CTL_REG_KER);
+}
+
+static int falcon_getsda(void *data)
+{
+	struct efx_nic *efx = (struct efx_nic *)data;
+	efx_oword_t reg;
+
+	falcon_read(efx, &reg, GPIO_CTL_REG_KER);
 	return EFX_OWORD_FIELD(reg, GPIO3_IN);
 }
 
-static int falcon_getscl(struct efx_i2c_interface *i2c)
+static int falcon_getscl(void *data)
 {
+	struct efx_nic *efx = (struct efx_nic *)data;
 	efx_oword_t reg;
 
-	falcon_read(i2c->efx, &reg, GPIO_CTL_REG_KER);
-	return EFX_DWORD_FIELD(reg, GPIO0_IN);
+	falcon_read(efx, &reg, GPIO_CTL_REG_KER);
+	return EFX_OWORD_FIELD(reg, GPIO0_IN);
 }
 
-static struct efx_i2c_bit_operations falcon_i2c_bit_operations = {
-	.setsda		= falcon_setsdascl,
-	.setscl		= falcon_setsdascl,
+static struct i2c_algo_bit_data falcon_i2c_bit_operations = {
+	.setsda		= falcon_setsda,
+	.setscl		= falcon_setscl,
 	.getsda		= falcon_getsda,
 	.getscl		= falcon_getscl,
 	.udelay		= 100,
-	.mdelay		= 10,
+	/*
+	 * This is the number of system clock ticks after which
+	 * i2c-algo-bit gives up waiting for SCL to become high.
+	 * It must be at least 2 since the first tick can happen
+	 * immediately after it starts waiting.
+	 */
+	.timeout	= 2,
 };
 
 /**************************************************************************
@@ -2403,12 +2425,6 @@ int falcon_probe_nic(struct efx_nic *efx)
 	struct falcon_nic_data *nic_data;
 	int rc;
 
-	/* Initialise I2C interface state */
-	efx->i2c.efx = efx;
-	efx->i2c.op = &falcon_i2c_bit_operations;
-	efx->i2c.sda = 1;
-	efx->i2c.scl = 1;
-
 	/* Allocate storage for hardware specific data */
 	nic_data = kzalloc(sizeof(*nic_data), GFP_KERNEL);
 	efx->nic_data = nic_data;
@@ -2459,6 +2475,18 @@ int falcon_probe_nic(struct efx_nic *efx)
 	if (rc)
 		goto fail5;
 
+	/* Initialise I2C adapter */
+ 	efx->i2c_adap.owner = THIS_MODULE;
+ 	efx->i2c_adap.class = I2C_CLASS_HWMON;
+	nic_data->i2c_data = falcon_i2c_bit_operations;
+	nic_data->i2c_data.data = efx;
+ 	efx->i2c_adap.algo_data = &nic_data->i2c_data;
+	efx->i2c_adap.dev.parent = &efx->pci_dev->dev;
+	strcpy(efx->i2c_adap.name, "SFC4000 GPIO");
+	rc = i2c_bit_add_bus(&efx->i2c_adap);
+	if (rc)
+		goto fail5;
+
 	return 0;
 
  fail5:
@@ -2633,6 +2661,10 @@ int falcon_init_nic(struct efx_nic *efx)
 void falcon_remove_nic(struct efx_nic *efx)
 {
 	struct falcon_nic_data *nic_data = efx->nic_data;
+	int rc;
+
+	rc = i2c_del_adapter(&efx->i2c_adap);
+	BUG_ON(rc);
 
 	falcon_free_buffer(efx, &efx->irq_status);
 
diff --git a/drivers/net/sfc/i2c-direct.c b/drivers/net/sfc/i2c-direct.c
deleted file mode 100644
index b6c62d0..0000000
--- a/drivers/net/sfc/i2c-direct.c
+++ /dev/null
@@ -1,381 +0,0 @@
-/****************************************************************************
- * Driver for Solarflare Solarstorm network controllers and boards
- * Copyright 2005 Fen Systems Ltd.
- * Copyright 2006-2008 Solarflare Communications Inc.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License version 2 as published
- * by the Free Software Foundation, incorporated herein by reference.
- */
-
-#include <linux/delay.h>
-#include "net_driver.h"
-#include "i2c-direct.h"
-
-/*
- * I2C data (SDA) and clock (SCL) line read/writes with appropriate
- * delays.
- */
-
-static inline void setsda(struct efx_i2c_interface *i2c, int state)
-{
-	udelay(i2c->op->udelay);
-	i2c->sda = state;
-	i2c->op->setsda(i2c);
-	udelay(i2c->op->udelay);
-}
-
-static inline void setscl(struct efx_i2c_interface *i2c, int state)
-{
-	udelay(i2c->op->udelay);
-	i2c->scl = state;
-	i2c->op->setscl(i2c);
-	udelay(i2c->op->udelay);
-}
-
-static inline int getsda(struct efx_i2c_interface *i2c)
-{
-	int sda;
-
-	udelay(i2c->op->udelay);
-	sda = i2c->op->getsda(i2c);
-	udelay(i2c->op->udelay);
-	return sda;
-}
-
-static inline int getscl(struct efx_i2c_interface *i2c)
-{
-	int scl;
-
-	udelay(i2c->op->udelay);
-	scl = i2c->op->getscl(i2c);
-	udelay(i2c->op->udelay);
-	return scl;
-}
-
-/*
- * I2C low-level protocol operations
- *
- */
-
-static inline void i2c_release(struct efx_i2c_interface *i2c)
-{
-	EFX_WARN_ON_PARANOID(!i2c->scl);
-	EFX_WARN_ON_PARANOID(!i2c->sda);
-	/* Devices may time out if operations do not end */
-	setscl(i2c, 1);
-	setsda(i2c, 1);
-	EFX_BUG_ON_PARANOID(getsda(i2c) != 1);
-	EFX_BUG_ON_PARANOID(getscl(i2c) != 1);
-}
-
-static inline void i2c_start(struct efx_i2c_interface *i2c)
-{
-	/* We may be restarting immediately after a {send,recv}_bit,
-	 * so SCL will not necessarily already be high.
-	 */
-	EFX_WARN_ON_PARANOID(!i2c->sda);
-	setscl(i2c, 1);
-	setsda(i2c, 0);
-	setscl(i2c, 0);
-	setsda(i2c, 1);
-}
-
-static inline void i2c_send_bit(struct efx_i2c_interface *i2c, int bit)
-{
-	EFX_WARN_ON_PARANOID(i2c->scl != 0);
-	setsda(i2c, bit);
-	setscl(i2c, 1);
-	setscl(i2c, 0);
-	setsda(i2c, 1);
-}
-
-static inline int i2c_recv_bit(struct efx_i2c_interface *i2c)
-{
-	int bit;
-
-	EFX_WARN_ON_PARANOID(i2c->scl != 0);
-	EFX_WARN_ON_PARANOID(!i2c->sda);
-	setscl(i2c, 1);
-	bit = getsda(i2c);
-	setscl(i2c, 0);
-	return bit;
-}
-
-static inline void i2c_stop(struct efx_i2c_interface *i2c)
-{
-	EFX_WARN_ON_PARANOID(i2c->scl != 0);
-	setsda(i2c, 0);
-	setscl(i2c, 1);
-	setsda(i2c, 1);
-}
-
-/*
- * I2C mid-level protocol operations
- *
- */
-
-/* Sends a byte via the I2C bus and checks for an acknowledgement from
- * the slave device.
- */
-static int i2c_send_byte(struct efx_i2c_interface *i2c, u8 byte)
-{
-	int i;
-
-	/* Send byte */
-	for (i = 0; i < 8; i++) {
-		i2c_send_bit(i2c, !!(byte & 0x80));
-		byte <<= 1;
-	}
-
-	/* Check for acknowledgement from slave */
-	return (i2c_recv_bit(i2c) == 0 ? 0 : -EIO);
-}
-
-/* Receives a byte via the I2C bus and sends ACK/NACK to the slave device. */
-static u8 i2c_recv_byte(struct efx_i2c_interface *i2c, int ack)
-{
-	u8 value = 0;
-	int i;
-
-	/* Receive byte */
-	for (i = 0; i < 8; i++)
-		value = (value << 1) | i2c_recv_bit(i2c);
-
-	/* Send ACK/NACK */
-	i2c_send_bit(i2c, (ack ? 0 : 1));
-
-	return value;
-}
-
-/* Calculate command byte for a read operation */
-static inline u8 i2c_read_cmd(u8 device_id)
-{
-	return ((device_id << 1) | 1);
-}
-
-/* Calculate command byte for a write operation */
-static inline u8 i2c_write_cmd(u8 device_id)
-{
-	return ((device_id << 1) | 0);
-}
-
-int efx_i2c_check_presence(struct efx_i2c_interface *i2c, u8 device_id)
-{
-	int rc;
-
-	/* If someone is driving the bus low we just give up. */
-	if (getsda(i2c) == 0 || getscl(i2c) == 0) {
-		EFX_ERR(i2c->efx, "%s someone is holding the I2C bus low."
-			" Giving up.\n", __func__);
-		return -EFAULT;
-	}
-
-	/* Pretend to initiate a device write */
-	i2c_start(i2c);
-	rc = i2c_send_byte(i2c, i2c_write_cmd(device_id));
-	if (rc)
-		goto out;
-
- out:
-	i2c_stop(i2c);
-	i2c_release(i2c);
-
-	return rc;
-}
-
-/* This performs a fast read of one or more consecutive bytes from an
- * I2C device.  Not all devices support consecutive reads of more than
- * one byte; for these devices use efx_i2c_read() instead.
- */
-int efx_i2c_fast_read(struct efx_i2c_interface *i2c,
-		      u8 device_id, u8 offset, u8 *data, unsigned int len)
-{
-	int i;
-	int rc;
-
-	EFX_WARN_ON_PARANOID(getsda(i2c) != 1);
-	EFX_WARN_ON_PARANOID(getscl(i2c) != 1);
-	EFX_WARN_ON_PARANOID(data == NULL);
-	EFX_WARN_ON_PARANOID(len < 1);
-
-	/* Select device and starting offset */
-	i2c_start(i2c);
-	rc = i2c_send_byte(i2c, i2c_write_cmd(device_id));
-	if (rc)
-		goto out;
-	rc = i2c_send_byte(i2c, offset);
-	if (rc)
-		goto out;
-
-	/* Read data from device */
-	i2c_start(i2c);
-	rc = i2c_send_byte(i2c, i2c_read_cmd(device_id));
-	if (rc)
-		goto out;
-	for (i = 0; i < (len - 1); i++)
-		/* Read and acknowledge all but the last byte */
-		data[i] = i2c_recv_byte(i2c, 1);
-	/* Read last byte with no acknowledgement */
-	data[i] = i2c_recv_byte(i2c, 0);
-
- out:
-	i2c_stop(i2c);
-	i2c_release(i2c);
-
-	return rc;
-}
-
-/* This performs a fast write of one or more consecutive bytes to an
- * I2C device.  Not all devices support consecutive writes of more
- * than one byte; for these devices use efx_i2c_write() instead.
- */
-int efx_i2c_fast_write(struct efx_i2c_interface *i2c,
-		       u8 device_id, u8 offset,
-		       const u8 *data, unsigned int len)
-{
-	int i;
-	int rc;
-
-	EFX_WARN_ON_PARANOID(getsda(i2c) != 1);
-	EFX_WARN_ON_PARANOID(getscl(i2c) != 1);
-	EFX_WARN_ON_PARANOID(len < 1);
-
-	/* Select device and starting offset */
-	i2c_start(i2c);
-	rc = i2c_send_byte(i2c, i2c_write_cmd(device_id));
-	if (rc)
-		goto out;
-	rc = i2c_send_byte(i2c, offset);
-	if (rc)
-		goto out;
-
-	/* Write data to device */
-	for (i = 0; i < len; i++) {
-		rc = i2c_send_byte(i2c, data[i]);
-		if (rc)
-			goto out;
-	}
-
- out:
-	i2c_stop(i2c);
-	i2c_release(i2c);
-
-	return rc;
-}
-
-/* I2C byte-by-byte read */
-int efx_i2c_read(struct efx_i2c_interface *i2c,
-		 u8 device_id, u8 offset, u8 *data, unsigned int len)
-{
-	int rc;
-
-	/* i2c_fast_read with length 1 is a single byte read */
-	for (; len > 0; offset++, data++, len--) {
-		rc = efx_i2c_fast_read(i2c, device_id, offset, data, 1);
-		if (rc)
-			return rc;
-	}
-
-	return 0;
-}
-
-/* I2C byte-by-byte write */
-int efx_i2c_write(struct efx_i2c_interface *i2c,
-		  u8 device_id, u8 offset, const u8 *data, unsigned int len)
-{
-	int rc;
-
-	/* i2c_fast_write with length 1 is a single byte write */
-	for (; len > 0; offset++, data++, len--) {
-		rc = efx_i2c_fast_write(i2c, device_id, offset, data, 1);
-		if (rc)
-			return rc;
-		mdelay(i2c->op->mdelay);
-	}
-
-	return 0;
-}
-
-
-/* This is just a slightly neater wrapper round efx_i2c_fast_write
- * in the case where the target doesn't take an offset
- */
-int efx_i2c_send_bytes(struct efx_i2c_interface *i2c,
-		       u8 device_id, const u8 *data, unsigned int len)
-{
-	return efx_i2c_fast_write(i2c, device_id, data[0], data + 1, len - 1);
-}
-
-/* I2C receiving of bytes - does not send an offset byte */
-int efx_i2c_recv_bytes(struct efx_i2c_interface *i2c, u8 device_id,
-		       u8 *bytes, unsigned int len)
-{
-	int i;
-	int rc;
-
-	EFX_WARN_ON_PARANOID(getsda(i2c) != 1);
-	EFX_WARN_ON_PARANOID(getscl(i2c) != 1);
-	EFX_WARN_ON_PARANOID(len < 1);
-
-	/* Select device */
-	i2c_start(i2c);
-
-	/* Read data from device */
-	rc = i2c_send_byte(i2c, i2c_read_cmd(device_id));
-	if (rc)
-		goto out;
-
-	for (i = 0; i < (len - 1); i++)
-		/* Read and acknowledge all but the last byte */
-		bytes[i] = i2c_recv_byte(i2c, 1);
-	/* Read last byte with no acknowledgement */
-	bytes[i] = i2c_recv_byte(i2c, 0);
-
- out:
-	i2c_stop(i2c);
-	i2c_release(i2c);
-
-	return rc;
-}
-
-/* SMBus and some I2C devices will time out if the I2C clock is
- * held low for too long. This is most likely to happen in virtualised
- * systems (when the entire domain is descheduled) but could in
- * principle happen due to preemption on any busy system (and given the
- * potential length of an I2C operation turning preemption off is not
- * a sensible option). The following functions deal with the failure by
- * retrying up to a fixed number of times.
-  */
-
-#define I2C_MAX_RETRIES	(10)
-
-/* The timeout problem will result in -EIO. If the wrapped function
- * returns any other error, pass this up and do not retry. */
-#define RETRY_WRAPPER(_f) \
-	int retries = I2C_MAX_RETRIES; \
-	int rc; \
-	while (retries) { \
-		rc = _f; \
-		if (rc != -EIO) \
-			return rc; \
-		retries--; \
-	} \
-	return rc; \
-
-int efx_i2c_check_presence_retry(struct efx_i2c_interface *i2c, u8 device_id)
-{
-	RETRY_WRAPPER(efx_i2c_check_presence(i2c, device_id))
-}
-
-int efx_i2c_read_retry(struct efx_i2c_interface *i2c,
-		 u8 device_id, u8 offset, u8 *data, unsigned int len)
-{
-	RETRY_WRAPPER(efx_i2c_read(i2c, device_id, offset, data, len))
-}
-
-int efx_i2c_write_retry(struct efx_i2c_interface *i2c,
-		  u8 device_id, u8 offset, const u8 *data, unsigned int len)
-{
-	RETRY_WRAPPER(efx_i2c_write(i2c, device_id, offset, data, len))
-}
diff --git a/drivers/net/sfc/i2c-direct.h b/drivers/net/sfc/i2c-direct.h
deleted file mode 100644
index 291e561..0000000
--- a/drivers/net/sfc/i2c-direct.h
+++ /dev/null
@@ -1,91 +0,0 @@
-/****************************************************************************
- * Driver for Solarflare Solarstorm network controllers and boards
- * Copyright 2005 Fen Systems Ltd.
- * Copyright 2006 Solarflare Communications Inc.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License version 2 as published
- * by the Free Software Foundation, incorporated herein by reference.
- */
-
-#ifndef EFX_I2C_DIRECT_H
-#define EFX_I2C_DIRECT_H
-
-#include "net_driver.h"
-
-/*
- * Direct control of an I2C bus
- */
-
-struct efx_i2c_interface;
-
-/**
- * struct efx_i2c_bit_operations - I2C bus direct control methods
- *
- * I2C bus direct control methods.
- *
- * @setsda: Set state of SDA line
- * @setscl: Set state of SCL line
- * @getsda: Get state of SDA line
- * @getscl: Get state of SCL line
- * @udelay: Delay between each bit operation
- * @mdelay: Delay between each byte write
- */
-struct efx_i2c_bit_operations {
-	void (*setsda) (struct efx_i2c_interface *i2c);
-	void (*setscl) (struct efx_i2c_interface *i2c);
-	int (*getsda) (struct efx_i2c_interface *i2c);
-	int (*getscl) (struct efx_i2c_interface *i2c);
-	unsigned int udelay;
-	unsigned int mdelay;
-};
-
-/**
- * struct efx_i2c_interface - an I2C interface
- *
- * An I2C interface.
- *
- * @efx: Attached Efx NIC
- * @op: I2C bus control methods
- * @sda: Current output state of SDA line
- * @scl: Current output state of SCL line
- */
-struct efx_i2c_interface {
-	struct efx_nic *efx;
-	struct efx_i2c_bit_operations *op;
-	unsigned int sda:1;
-	unsigned int scl:1;
-};
-
-extern int efx_i2c_check_presence(struct efx_i2c_interface *i2c, u8 device_id);
-extern int efx_i2c_fast_read(struct efx_i2c_interface *i2c,
-			     u8 device_id, u8 offset,
-			     u8 *data, unsigned int len);
-extern int efx_i2c_fast_write(struct efx_i2c_interface *i2c,
-			      u8 device_id, u8 offset,
-			      const u8 *data, unsigned int len);
-extern int efx_i2c_read(struct efx_i2c_interface *i2c,
-			u8 device_id, u8 offset, u8 *data, unsigned int len);
-extern int efx_i2c_write(struct efx_i2c_interface *i2c,
-			 u8 device_id, u8 offset,
-			 const u8 *data, unsigned int len);
-
-extern int efx_i2c_send_bytes(struct efx_i2c_interface *i2c, u8 device_id,
-			      const u8 *bytes, unsigned int len);
-
-extern int efx_i2c_recv_bytes(struct efx_i2c_interface *i2c, u8 device_id,
-			      u8 *bytes, unsigned int len);
-
-
-/* Versions of the API that retry on failure. */
-extern int efx_i2c_check_presence_retry(struct efx_i2c_interface *i2c,
-					u8 device_id);
-
-extern int efx_i2c_read_retry(struct efx_i2c_interface *i2c,
-			u8 device_id, u8 offset, u8 *data, unsigned int len);
-
-extern int efx_i2c_write_retry(struct efx_i2c_interface *i2c,
-			 u8 device_id, u8 offset,
-			 const u8 *data, unsigned int len);
-
-#endif /* EFX_I2C_DIRECT_H */
diff --git a/drivers/net/sfc/net_driver.h b/drivers/net/sfc/net_driver.h
index 5e20e75..d803b86 100644
--- a/drivers/net/sfc/net_driver.h
+++ b/drivers/net/sfc/net_driver.h
@@ -26,10 +26,10 @@
 #include <linux/highmem.h>
 #include <linux/workqueue.h>
 #include <linux/inet_lro.h>
+#include <linux/i2c.h>
 
 #include "enum.h"
 #include "bitfield.h"
-#include "i2c-direct.h"
 
 #define EFX_MAX_LRO_DESCRIPTORS 8
 #define EFX_MAX_LRO_AGGR MAX_SKB_FRAGS
@@ -418,7 +418,10 @@ struct efx_blinker {
  * @init_leds: Sets up board LEDs
  * @set_fault_led: Turns the fault LED on or off
  * @blink: Starts/stops blinking
+ * @fini: Cleanup function
  * @blinker: used to blink LEDs in software
+ * @hwmon_client: I2C client for hardware monitor
+ * @ioexp_client: I2C client for power/port control
  */
 struct efx_board {
 	int type;
@@ -431,7 +434,9 @@ struct efx_board {
 	int (*init_leds)(struct efx_nic *efx);
 	void (*set_fault_led) (struct efx_nic *efx, int state);
 	void (*blink) (struct efx_nic *efx, int start);
+	void (*fini) (struct efx_nic *nic);
 	struct efx_blinker blinker;
+	struct i2c_client *hwmon_client, *ioexp_client;
 };
 
 #define STRING_TABLE_LOOKUP(val, member)	\
@@ -618,7 +623,7 @@ union efx_multicast_hash {
  * @membase: Memory BAR value
  * @biu_lock: BIU (bus interface unit) lock
  * @interrupt_mode: Interrupt mode
- * @i2c: I2C interface
+ * @i2c_adap: I2C adapter
  * @board_info: Board-level information
  * @state: Device state flag. Serialised by the rtnl_lock.
  * @reset_pending: Pending reset method (normally RESET_TYPE_NONE)
@@ -686,7 +691,7 @@ struct efx_nic {
 	spinlock_t biu_lock;
 	enum efx_int_mode interrupt_mode;
 
-	struct efx_i2c_interface i2c;
+	struct i2c_adapter i2c_adap;
 	struct efx_board board_info;
 
 	enum nic_state state;
diff --git a/drivers/net/sfc/sfe4001.c b/drivers/net/sfc/sfe4001.c
index 66a0d14..b278495 100644
--- a/drivers/net/sfc/sfe4001.c
+++ b/drivers/net/sfc/sfe4001.c
@@ -106,28 +106,27 @@
 
 static const u8 xgphy_max_temperature = 90;
 
-void sfe4001_poweroff(struct efx_nic *efx)
+static void sfe4001_poweroff(struct efx_nic *efx)
 {
-	struct efx_i2c_interface *i2c = &efx->i2c;
+	struct i2c_client *ioexp_client = efx->board_info.ioexp_client;
+	struct i2c_client *hwmon_client = efx->board_info.hwmon_client;
 
-	u8 cfg, out, in;
+	/* Turn off all power rails and disable outputs */
+	i2c_smbus_write_byte_data(ioexp_client, P0_OUT, 0xff);
+	i2c_smbus_write_byte_data(ioexp_client, P1_CONFIG, 0xff);
+	i2c_smbus_write_byte_data(ioexp_client, P0_CONFIG, 0xff);
 
-	EFX_INFO(efx, "%s\n", __func__);
-
-	/* Turn off all power rails */
-	out = 0xff;
-	efx_i2c_write(i2c, PCA9539, P0_OUT, &out, 1);
-
-	/* Disable port 1 outputs on IO expander */
-	cfg = 0xff;
-	efx_i2c_write(i2c, PCA9539, P1_CONFIG, &cfg, 1);
+	/* Clear any over-temperature alert */
+	i2c_smbus_read_byte_data(hwmon_client, RSL);
+}
 
-	/* Disable port 0 outputs on IO expander */
-	cfg = 0xff;
-	efx_i2c_write(i2c, PCA9539, P0_CONFIG, &cfg, 1);
+static void sfe4001_fini(struct efx_nic *efx)
+{
+	EFX_INFO(efx, "%s\n", __func__);
 
-	/* Clear any over-temperature alert */
-	efx_i2c_read(i2c, MAX6647, RSL, &in, 1);
+	sfe4001_poweroff(efx);
+ 	i2c_unregister_device(efx->board_info.ioexp_client);
+ 	i2c_unregister_device(efx->board_info.hwmon_client);
 }
 
 /* The P0_EN_3V3X line on SFE4001 boards (from A2 onward) is connected
@@ -143,14 +142,26 @@ MODULE_PARM_DESC(phy_flash_cfg,
  * be turned on before the PHY can be used.
  * Context: Process context, rtnl lock held
  */
-int sfe4001_poweron(struct efx_nic *efx)
+int sfe4001_init(struct efx_nic *efx)
 {
-	struct efx_i2c_interface *i2c = &efx->i2c;
+	struct i2c_client *hwmon_client, *ioexp_client;
 	unsigned int count;
 	int rc;
-	u8 out, in, cfg;
+	u8 out;
 	efx_dword_t reg;
 
+	hwmon_client = i2c_new_dummy(&efx->i2c_adap, MAX6647);
+	if (!hwmon_client)
+		return -EIO;
+	efx->board_info.hwmon_client = hwmon_client;
+
+	ioexp_client = i2c_new_dummy(&efx->i2c_adap, PCA9539);
+	if (!ioexp_client) {
+		rc = -EIO;
+		goto fail_hwmon;
+	}
+	efx->board_info.ioexp_client = ioexp_client;
+
 	/* 10Xpress has fixed-function LED pins, so there is no board-specific
 	 * blink code. */
 	efx->board_info.blink = tenxpress_phy_blink;
@@ -166,44 +177,45 @@ int sfe4001_poweron(struct efx_nic *efx)
 	falcon_xmac_writel(efx, &reg, XX_PWR_RST_REG_MAC);
 	udelay(10);
 
+	efx->board_info.fini = sfe4001_fini;
+
 	/* Set DSP over-temperature alert threshold */
 	EFX_INFO(efx, "DSP cut-out at %dC\n", xgphy_max_temperature);
-	rc = efx_i2c_write(i2c, MAX6647, WLHO,
-			   &xgphy_max_temperature, 1);
+	rc = i2c_smbus_write_byte_data(hwmon_client, WLHO,
+				       xgphy_max_temperature);
 	if (rc)
-		goto fail1;
+		goto fail_ioexp;
 
 	/* Read it back and verify */
-	rc = efx_i2c_read(i2c, MAX6647, RLHN, &in, 1);
-	if (rc)
-		goto fail1;
-	if (in != xgphy_max_temperature) {
+	rc = i2c_smbus_read_byte_data(hwmon_client, RLHN);
+	if (rc < 0)
+		goto fail_ioexp;
+	if (rc != xgphy_max_temperature) {
 		rc = -EFAULT;
-		goto fail1;
+		goto fail_ioexp;
 	}
 
 	/* Clear any previous over-temperature alert */
-	rc = efx_i2c_read(i2c, MAX6647, RSL, &in, 1);
-	if (rc)
-		goto fail1;
+	rc = i2c_smbus_read_byte_data(hwmon_client, RSL);
+	if (rc < 0)
+		goto fail_ioexp;
 
 	/* Enable port 0 and port 1 outputs on IO expander */
-	cfg = 0x00;
-	rc = efx_i2c_write(i2c, PCA9539, P0_CONFIG, &cfg, 1);
+	rc = i2c_smbus_write_byte_data(ioexp_client, P0_CONFIG, 0x00);
 	if (rc)
-		goto fail1;
-	cfg = 0xff & ~(1 << P1_SPARE_LBN);
-	rc = efx_i2c_write(i2c, PCA9539, P1_CONFIG, &cfg, 1);
+		goto fail_ioexp;
+	rc = i2c_smbus_write_byte_data(ioexp_client, P1_CONFIG,
+				       0xff & ~(1 << P1_SPARE_LBN));
 	if (rc)
-		goto fail2;
+		goto fail_on;
 
 	/* Turn all power off then wait 1 sec. This ensures PHY is reset */
 	out = 0xff & ~((0 << P0_EN_1V2_LBN) | (0 << P0_EN_2V5_LBN) |
 		       (0 << P0_EN_3V3X_LBN) | (0 << P0_EN_5V_LBN) |
 		       (0 << P0_EN_1V0X_LBN));
-	rc = efx_i2c_write(i2c, PCA9539, P0_OUT, &out, 1);
+	rc = i2c_smbus_write_byte_data(ioexp_client, P0_OUT, out);
 	if (rc)
-		goto fail3;
+		goto fail_on;
 
 	schedule_timeout_uninterruptible(HZ);
 	count = 0;
@@ -215,26 +227,26 @@ int sfe4001_poweron(struct efx_nic *efx)
 		if (sfe4001_phy_flash_cfg)
 			out |= 1 << P0_EN_3V3X_LBN;
 
-		rc = efx_i2c_write(i2c, PCA9539, P0_OUT, &out, 1);
+		rc = i2c_smbus_write_byte_data(ioexp_client, P0_OUT, out);
 		if (rc)
-			goto fail3;
+			goto fail_on;
 		msleep(10);
 
 		/* Turn on 1V power rail */
 		out &= ~(1 << P0_EN_1V0X_LBN);
-		rc = efx_i2c_write(i2c, PCA9539, P0_OUT, &out, 1);
+		rc = i2c_smbus_write_byte_data(ioexp_client, P0_OUT, out);
 		if (rc)
-			goto fail3;
+			goto fail_on;
 
 		EFX_INFO(efx, "waiting for power (attempt %d)...\n", count);
 
 		schedule_timeout_uninterruptible(HZ);
 
 		/* Check DSP is powered */
-		rc = efx_i2c_read(i2c, PCA9539, P1_IN, &in, 1);
-		if (rc)
-			goto fail3;
-		if (in & (1 << P1_AFE_PWD_LBN))
+		rc = i2c_smbus_read_byte_data(ioexp_client, P1_IN);
+		if (rc < 0)
+			goto fail_on;
+		if (rc & (1 << P1_AFE_PWD_LBN))
 			goto done;
 
 		/* DSP doesn't look powered in flash config mode */
@@ -244,23 +256,17 @@ int sfe4001_poweron(struct efx_nic *efx)
 
 	EFX_INFO(efx, "timed out waiting for power\n");
 	rc = -ETIMEDOUT;
-	goto fail3;
+	goto fail_on;
 
 done:
 	EFX_INFO(efx, "PHY is powered on\n");
 	return 0;
 
-fail3:
-	/* Turn off all power rails */
-	out = 0xff;
-	efx_i2c_write(i2c, PCA9539, P0_OUT, &out, 1);
-	/* Disable port 1 outputs on IO expander */
-	out = 0xff;
-	efx_i2c_write(i2c, PCA9539, P1_CONFIG, &out, 1);
-fail2:
-	/* Disable port 0 outputs on IO expander */
-	out = 0xff;
-	efx_i2c_write(i2c, PCA9539, P0_CONFIG, &out, 1);
-fail1:
+fail_on:
+	sfe4001_poweroff(efx);
+fail_ioexp:
+ 	i2c_unregister_device(ioexp_client);
+fail_hwmon:
+ 	i2c_unregister_device(hwmon_client);
 	return rc;
 }
-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

[PATCH 2/2] sfc: Reduce I2C udelay to 5 resulting in a clock frequency of 100 kHz

[Ben Hutchings]

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sfc/falcon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/sfc/falcon.c b/drivers/net/sfc/falcon.c
index 8acd53d..8cb5798 100644
--- a/drivers/net/sfc/falcon.c
+++ b/drivers/net/sfc/falcon.c
@@ -222,7 +222,7 @@ static struct i2c_algo_bit_data falcon_i2c_bit_operations = {
 	.setscl		= falcon_setscl,
 	.getsda		= falcon_getsda,
 	.getscl		= falcon_getscl,
-	.udelay		= 100,
+	.udelay		= 5,
 	/*
 	 * This is the number of system clock ticks after which
 	 * i2c-algo-bit gives up waiting for SCL to become high.
-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 1/2] sfc: Use kernel I2C system and i2c-algo-bit driver

[Jeff Garzik]

Ben Hutchings wrote:
> Remove our own implementation of I2C bit-banging.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/sfc/Kconfig      |    2 +
>  drivers/net/sfc/Makefile     |    2 +-
>  drivers/net/sfc/boards.c     |    2 +-
>  drivers/net/sfc/boards.h     |    3 +-
>  drivers/net/sfc/efx.c        |    2 +
>  drivers/net/sfc/falcon.c     |   72 ++++++---
>  drivers/net/sfc/i2c-direct.c |  381 ------------------------------------------
>  drivers/net/sfc/i2c-direct.h |   91 ----------
>  drivers/net/sfc/net_driver.h |   11 +-
>  drivers/net/sfc/sfe4001.c    |  126 ++++++++-------
>  10 files changed, 133 insertions(+), 559 deletions(-)
>  delete mode 100644 drivers/net/sfc/i2c-direct.c
>  delete mode 100644 drivers/net/sfc/i2c-direct.h

applied 1-2

Re: [PATCH 1/2] sfc: Use kernel I2C system and i2c-algo-bit driver

[Jean Delvare]

Hi Ben,

On Fri, 30 May 2008 22:27:04 +0100, Ben Hutchings wrote:
> Remove our own implementation of I2C bit-banging.

Thanks a lot for doing this. It's always nice to see hundreds of lines
of code being removed from the kernel :) Some comments:

> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/sfc/Kconfig      |    2 +
>  drivers/net/sfc/Makefile     |    2 +-
>  drivers/net/sfc/boards.c     |    2 +-
>  drivers/net/sfc/boards.h     |    3 +-
>  drivers/net/sfc/efx.c        |    2 +
>  drivers/net/sfc/falcon.c     |   72 ++++++---
>  drivers/net/sfc/i2c-direct.c |  381 ------------------------------------------
>  drivers/net/sfc/i2c-direct.h |   91 ----------
>  drivers/net/sfc/net_driver.h |   11 +-
>  drivers/net/sfc/sfe4001.c    |  126 ++++++++-------
>  10 files changed, 133 insertions(+), 559 deletions(-)
>  delete mode 100644 drivers/net/sfc/i2c-direct.c
>  delete mode 100644 drivers/net/sfc/i2c-direct.h
> 
> (...)
> diff --git a/drivers/net/sfc/falcon.c b/drivers/net/sfc/falcon.c
> index d3f749c..8acd53d 100644
> --- a/drivers/net/sfc/falcon.c
> +++ b/drivers/net/sfc/falcon.c
> (...)
> -static struct efx_i2c_bit_operations falcon_i2c_bit_operations = {
> -	.setsda		= falcon_setsdascl,
> -	.setscl		= falcon_setsdascl,
> +static struct i2c_algo_bit_data falcon_i2c_bit_operations = {
> +	.setsda		= falcon_setsda,
> +	.setscl		= falcon_setscl,
>  	.getsda		= falcon_getsda,
>  	.getscl		= falcon_getscl,
>  	.udelay		= 100,
> -	.mdelay		= 10,
> +	/*
> +	 * This is the number of system clock ticks after which
> +	 * i2c-algo-bit gives up waiting for SCL to become high.
> +	 * It must be at least 2 since the first tick can happen
> +	 * immediately after it starts waiting.
> +	 */
> +	.timeout	= 2,

This is unreasonably low at HZ=1000 (2 ms). The SMBus specification
states that clients can hold the clock line low for up to 50 ms if they
need additional time to process the previous commands. So I would use
msecs_to_jiffies(50) (don't forget to include <linux/jiffies.h>.)

>  };
>  
>  /**************************************************************************
> @@ -2403,12 +2425,6 @@ int falcon_probe_nic(struct efx_nic *efx)
>  	struct falcon_nic_data *nic_data;
>  	int rc;
>  
> -	/* Initialise I2C interface state */
> -	efx->i2c.efx = efx;
> -	efx->i2c.op = &falcon_i2c_bit_operations;
> -	efx->i2c.sda = 1;
> -	efx->i2c.scl = 1;
> -
>  	/* Allocate storage for hardware specific data */
>  	nic_data = kzalloc(sizeof(*nic_data), GFP_KERNEL);
>  	efx->nic_data = nic_data;
> @@ -2459,6 +2475,18 @@ int falcon_probe_nic(struct efx_nic *efx)
>  	if (rc)
>  		goto fail5;
>  
> +	/* Initialise I2C adapter */
> + 	efx->i2c_adap.owner = THIS_MODULE;
> + 	efx->i2c_adap.class = I2C_CLASS_HWMON;

I doubt you want to do this. This would let any hardware monitoring
driver probe your bus for a device, while presumably you already know
which hardware monitoring device is present and you want to instantiate
the i2c client yourself. This will probably become clearer when you
start using the lm87 driver and modify it to support new-style i2c
binding.

> +	nic_data->i2c_data = falcon_i2c_bit_operations;
> +	nic_data->i2c_data.data = efx;
> + 	efx->i2c_adap.algo_data = &nic_data->i2c_data;
> +	efx->i2c_adap.dev.parent = &efx->pci_dev->dev;
> +	strcpy(efx->i2c_adap.name, "SFC4000 GPIO");

Please always use strlcpy.

> +	rc = i2c_bit_add_bus(&efx->i2c_adap);
> +	if (rc)
> +		goto fail5;
> +
>  	return 0;
>  
>   fail5:
> @@ -2633,6 +2661,10 @@ int falcon_init_nic(struct efx_nic *efx)
>  void falcon_remove_nic(struct efx_nic *efx)
>  {
>  	struct falcon_nic_data *nic_data = efx->nic_data;
> +	int rc;
> +
> +	rc = i2c_del_adapter(&efx->i2c_adap);
> +	BUG_ON(rc);

That's pretty aggressive and probably not needed.

>  
>  	falcon_free_buffer(efx, &efx->irq_status);
>  

-- 
Jean Delvare

Re: [PATCH 2/2] sfc: Reduce I2C udelay to 5 resulting in a clock frequency of 100 kHz

[Jean Delvare]

On Fri, 30 May 2008 22:27:46 +0100, Ben Hutchings wrote:
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/sfc/falcon.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/sfc/falcon.c b/drivers/net/sfc/falcon.c
> index 8acd53d..8cb5798 100644
> --- a/drivers/net/sfc/falcon.c
> +++ b/drivers/net/sfc/falcon.c
> @@ -222,7 +222,7 @@ static struct i2c_algo_bit_data falcon_i2c_bit_operations = {
>  	.setscl		= falcon_setscl,
>  	.getsda		= falcon_getsda,
>  	.getscl		= falcon_getscl,
> -	.udelay		= 100,
> +	.udelay		= 5,
>  	/*
>  	 * This is the number of system clock ticks after which
>  	 * i2c-algo-bit gives up waiting for SCL to become high.

Acked-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 1/2] sfc: Use kernel I2C system and i2c-algo-bit driver

[Ben Hutchings]

Jean Delvare wrote:
[...]
> > diff --git a/drivers/net/sfc/falcon.c b/drivers/net/sfc/falcon.c
> > index d3f749c..8acd53d 100644
> > --- a/drivers/net/sfc/falcon.c
> > +++ b/drivers/net/sfc/falcon.c
> > (...)
> > -static struct efx_i2c_bit_operations falcon_i2c_bit_operations = {
> > -	.setsda		= falcon_setsdascl,
> > -	.setscl		= falcon_setsdascl,
> > +static struct i2c_algo_bit_data falcon_i2c_bit_operations = {
> > +	.setsda		= falcon_setsda,
> > +	.setscl		= falcon_setscl,
> >  	.getsda		= falcon_getsda,
> >  	.getscl		= falcon_getscl,
> >  	.udelay		= 100,
> > -	.mdelay		= 10,
> > +	/*
> > +	 * This is the number of system clock ticks after which
> > +	 * i2c-algo-bit gives up waiting for SCL to become high.
> > +	 * It must be at least 2 since the first tick can happen
> > +	 * immediately after it starts waiting.
> > +	 */
> > +	.timeout	= 2,
> 
> This is unreasonably low at HZ=1000 (2 ms). The SMBus specification
> states that clients can hold the clock line low for up to 50 ms if they
> need additional time to process the previous commands. So I would use
> msecs_to_jiffies(50) (don't forget to include <linux/jiffies.h>.)

OK.  We can't use msecs_to_jiffies() in a static initialiser, though.

[...]
> > @@ -2459,6 +2475,18 @@ int falcon_probe_nic(struct efx_nic *efx)
> >  	if (rc)
> >  		goto fail5;
> >  
> > +	/* Initialise I2C adapter */
> > + 	efx->i2c_adap.owner = THIS_MODULE;
> > + 	efx->i2c_adap.class = I2C_CLASS_HWMON;
> 
> I doubt you want to do this. This would let any hardware monitoring
> driver probe your bus for a device, while presumably you already know
> which hardware monitoring device is present and you want to instantiate
> the i2c client yourself. This will probably become clearer when you
> start using the lm87 driver and modify it to support new-style i2c
> binding.
 
So the class should be, what, 0?

> > +	nic_data->i2c_data = falcon_i2c_bit_operations;
> > +	nic_data->i2c_data.data = efx;
> > + 	efx->i2c_adap.algo_data = &nic_data->i2c_data;
> > +	efx->i2c_adap.dev.parent = &efx->pci_dev->dev;
> > +	strcpy(efx->i2c_adap.name, "SFC4000 GPIO");
> 
> Please always use strlcpy.
 
OK.  I thought about it but it didn't seem worthwhile for a short constant
string.

> > +	rc = i2c_bit_add_bus(&efx->i2c_adap);
> > +	if (rc)
> > +		goto fail5;
> > +
> >  	return 0;
> >  
> >   fail5:
> > @@ -2633,6 +2661,10 @@ int falcon_init_nic(struct efx_nic *efx)
> >  void falcon_remove_nic(struct efx_nic *efx)
> >  {
> >  	struct falcon_nic_data *nic_data = efx->nic_data;
> > +	int rc;
> > +
> > +	rc = i2c_del_adapter(&efx->i2c_adap);
> > +	BUG_ON(rc);
> 
> That's pretty aggressive and probably not needed.

We have no way of cancelling the removal at this point, so if the
adapter is not properly deleted then it will hang around referring to
a NIC structure that has gone away.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 1/2] sfc: Use kernel I2C system and i2c-algo-bit driver

[Jean Delvare]

On Sat, 31 May 2008 12:27:02 +0100, Ben Hutchings wrote:
> Jean Delvare wrote:
> > > @@ -2459,6 +2475,18 @@ int falcon_probe_nic(struct efx_nic *efx)
> > >  	if (rc)
> > >  		goto fail5;
> > >  
> > > +	/* Initialise I2C adapter */
> > > + 	efx->i2c_adap.owner = THIS_MODULE;
> > > + 	efx->i2c_adap.class = I2C_CLASS_HWMON;
> > 
> > I doubt you want to do this. This would let any hardware monitoring
> > driver probe your bus for a device, while presumably you already know
> > which hardware monitoring device is present and you want to instantiate
> > the i2c client yourself. This will probably become clearer when you
> > start using the lm87 driver and modify it to support new-style i2c
> > binding.
>  
> So the class should be, what, 0?

Yes. Assuming a kzalloc'd structure, you can simply omit it.

> > > +	nic_data->i2c_data = falcon_i2c_bit_operations;
> > > +	nic_data->i2c_data.data = efx;
> > > + 	efx->i2c_adap.algo_data = &nic_data->i2c_data;
> > > +	efx->i2c_adap.dev.parent = &efx->pci_dev->dev;
> > > +	strcpy(efx->i2c_adap.name, "SFC4000 GPIO");
> > 
> > Please always use strlcpy.
>  
> OK.  I thought about it but it didn't seem worthwhile for a short constant
> string.

Until someone changes the string for a longer one (it's common to
include the I/O base) or the structure field is shortened to save some
memory... Always use strlcpy. In fact I'd like to see strcpy removed
from the kernel, it's almost always the wrong function to use.

-- 
Jean Delvare