[i2c] [PATCH] I2C bus driver for Philips ARM boards
Jean Delvare
khali at linux-fr.org
Mon Jul 3 23:40:19 CEST 2006
Hi Vitaly,
Posting to three lists at once, hmm... Stripping LKML.
> Please find below the updated version of the I2C bus driver
> for Philips ARM boards (Philips IP3204 I2C IP block). This
> I2C controller can be found on PNX010x, PNX52xx and PNX4008
> Philips boards.
> Any comments are welcome.
>
> arch/arm/mach-pnx4008/Makefile | 2
> arch/arm/mach-pnx4008/i2c.c | 186 +++++++++
> drivers/i2c/busses/Kconfig | 10
> drivers/i2c/busses/Makefile | 2
> drivers/i2c/busses/i2c-pnx.c | 761 +++++++++++++++++++++++++++++++++++++
> include/asm-arm/arch-pnx4008/i2c.h | 80 +++
> include/linux/i2c-pnx.h | 51 ++
> 7 files changed, 1091 insertions(+), 1 deletion(-)
The name "i2c-pnx" is a bit too generic IMHO. What if another PNX board
is later released with a different I2C implementation?
> Index: linux-2.6.git/drivers/i2c/busses/Makefile
> ===================================================================
> --- linux-2.6.git.orig/drivers/i2c/busses/Makefile
> +++ linux-2.6.git/drivers/i2c/busses/Makefile
> @@ -42,6 +42,8 @@ obj-$(CONFIG_I2C_VIAPRO) += i2c-viapro.o
> obj-$(CONFIG_I2C_VOODOO3) += i2c-voodoo3.o
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> obj-$(CONFIG_SCx200_I2C) += scx200_i2c.o
> +obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
> +
In alphabetical order, please.
> Index: linux-2.6.git/drivers/i2c/busses/i2c-pnx.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.git/drivers/i2c/busses/i2c-pnx.c
> @@ -0,0 +1,761 @@
> +/*
> + * drivers/i2c/i2c-pnx.c
Not even true. Having the full path here doesn't help (and is actually
a burden when we want to move drivers around) so please just don't do
it.
> + *
> + * Provides i2c support for PNX010x/PNX4008.
Mentioning that these are Philips board would be welcome.
> + *
> + * Authors: Dennis Kovalev <dkovalev at ru.mvista.com>
> + * Vitaly Wool <vwool at ru.mvista.com>
> + *
> + * 2004-2006 (c) MontaVista Software, Inc. 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/module.h>
> +#include <linux/config.h>
> +#include <linux/interrupt.h>
> +#include <linux/version.h>
> +#include <linux/kernel.h>
> +#include <linux/ioport.h>
> +#include <linux/pci.h>
> +#include <linux/types.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/mm.h>
> +#include <linux/miscdevice.h>
> +#include <linux/timer.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/completion.h>
> +#include <linux/platform_device.h>
> +#include <asm/hardware.h>
> +#include <asm/irq.h>
> +#include <asm/uaccess.h>
> +#include <linux/i2c-pnx.h>
> +#include <asm/arch/i2c.h>
This is by far the longest include list I ever saw in a driver ;) Do
you really need all of these? For example you obviously don't need
pci.h, nor miscdevice.h. I don't quite see why you would need fs.h
either. And you don't need config.h. I'm not going to check them all,
but please do, and strip down the list to the minimum required. Useless
includes increase the recompilation times.
Also, all <linux/...> includes should come first, and <asm/...>
includes after that.
The fact that you need to include <asm/arch/i2c.h>, I think betrays a
design error. Any device-specific information should come to the driver
through the device's private data rather than through an include.
> +
> +#define COMPLETION_COUNT 10000
> +
> +struct pnx_i2c {
> + int (*suspend) (struct platform_device * pdev, pm_message_t state);
The usual coding style suggests no space between * and pdev.
> + int (*resume) (struct platform_device * pdev);
> + u32(*calculate_input_freq) (struct platform_device * pdev);
Broken indentation.
> + int (*set_clock_run) (struct platform_device * pdev);
> + int (*set_clock_stop) (struct platform_device * pdev);
> + struct i2c_adapter *adapter;
> +};
> +
> +char *i2c_pnx_modestr[] = {
> + "Transmit",
> + "Receive",
> + "No Data",
> +};
Could be static const. You also seem to only use this array in debug
messages, so please surround its definition with #ifdef DEBUG/#endif.
> +
> +static inline void i2c_pnx_arm_timer(struct i2c_adapter *adap)
> +{
> + struct i2c_pnx_algo_data *data = adap->algo_data;
> + struct timer_list *timer = &data->mif.timer;
> +
> + del_timer_sync(timer);
> +
> + dev_dbg(&adap->dev, "Timer armed at %lu plus %u jiffies.\n",
> + jiffies, TIMEOUT);
> +
> + timer->expires = jiffies + TIMEOUT;
> + timer->data = (unsigned long)data;
> +
> + add_timer(timer);
> +}
> +
> +static inline int i2c_pnx_bus_busy(struct i2c_pnx_algo_data *adata)
> +{
> + long timeout;
> +
> + timeout = TIMEOUT * COMPLETION_COUNT;
> + if (timeout > 0 && (ioread32(I2C_REG_STS(adata)) & mstatus_active)) {
> + udelay(500);
> + timeout -= 500;
> + }
> +
> + return (timeout <= 0) ? 1 : 0;
> +}
The timeout is set to TIMEOUT * COMPLETION_COUNT us. This is a
duration which depends on the value of HZ! For HZ = 100, this is 2
seconds, which is fine, but for HZ = 1000 it climbs to 20 seconds, a bit
too much IMHO. The timeout should not depend on HZ. If you want a 2
seconds timeout, the proper formula is TIMEOUT / HZ * 1000000.
Anyway your function is broken, I presume that the "if" should be a
"while", right? Right now the function will never return 1.
As a side note, setting the i2c timeout as a define with such a generic
name is a bad idea. It could easily conflict. There is a timeout field
in the i2c_adapter structure, which you could use instead. As a bonus,
this field can be controlled by userspace (through the I2C_TIMEOUT
ioctl). I also notice that you have TIMEOUT defined twice, once in
include/linux/i2c-pnx.h and once in include/asm-arm/arch-pnx4008/i2c.h.
This can't be correct.
Lastly, busy-waiting while waiting for a condition is not great, please
consider switching to non-busy waiting.
> +
> +/**
> + * i2c_pnx_start - start a device
> + * @slave_addr: slave address
> + * @adap: pointer to adapter structure
> + *
> + * Generate a START signal in the desired mode.
> + */
> +static int i2c_pnx_start(unsigned char slave_addr, struct i2c_adapter *adap)
> +{
> + struct i2c_pnx_algo_data *alg_data =
> + (struct i2c_pnx_algo_data *)adap->algo_data;
> +
> + dev_dbg(&adap->dev, "%s(): addr 0x%x mode %s\n", __FUNCTION__,
> + slave_addr, i2c_pnx_modestr[alg_data->mif.mode]);
> +
> + /* Check for 7 bit slave addresses only */
> + if (slave_addr & ~0x7f) {
> + printk(KERN_ERR "%s: Invalid slave address %x. "
> + "Only 7-bit addresses are supported\n",
> + adap->name, slave_addr);
> + return -EINVAL;
> + }
> +
> + /* First, make sure bus is idle */
> + if (i2c_pnx_bus_busy(alg_data)) {
> + /* Somebody else is monopolising the bus */
Spelling: monopolizing.
> + printk(KERN_ERR "%s: Bus busy. Slave addr = %02x, "
> + "cntrl = %x, stat = %x\n",
> + adap->name, slave_addr,
> + ioread32(I2C_REG_CTL(alg_data)),
> + ioread32(I2C_REG_STS(alg_data)));
> + return -EBUSY;
Please use dev_err() instead. Same thing everywhere you have a device
at hand, you want to use dev_err(&adap->dev, ...) or similar instead of
printk().
> + } else if (ioread32(I2C_REG_STS(alg_data)) & mstatus_afi) {
> + /* Sorry, we lost the bus */
> + printk(KERN_ERR "%s: Arbitration failure. "
> + "Slave addr = %02x\n", adap->name, slave_addr);
> + return -EIO;
> + }
> +
> + /* OK, I2C is enabled and we have the bus. */
> + /* Clear the current TDI and AFI status flags. */
> + iowrite32(ioread32(I2C_REG_STS(alg_data)) | mstatus_tdi | mstatus_afi,
> + I2C_REG_STS(alg_data));
> +
> + dev_dbg(&adap->dev, "%s(): sending %#x\n", __FUNCTION__,
> + (slave_addr << 1) | start_bit |
> + (alg_data->mif.mode == rcv ? rw_bit : 0));
> +
> + /* Write the slave address, START bit and R/W bit */
> + iowrite32((slave_addr << 1) | start_bit |
> + (alg_data->mif.mode == rcv ? rw_bit : 0),
> + I2C_REG_TX(alg_data));
You could use I2C_SMBUS_READ and I2C_SMBUS_WRITE instead of inventing
your own rw_bit.
> +
> + dev_dbg(&adap->dev, "%s(): exit\n", __FUNCTION__);
> +
> + return 0;
> +}
> +
> +/**
> + * i2c_pnx_start - stop a device
> + * @adap: pointer to I2C adapter structure
Bogus comment, this is i2c_pnx_stop, not i2c_pnx_start.
> + *
> + * Generate a STOP signal to terminate the master transaction.
> + */
> +static void i2c_pnx_stop(struct i2c_adapter *adap)
> +{
> + struct i2c_pnx_algo_data *alg_data = adap->algo_data;
> + int cnt = COMPLETION_COUNT;
> +
> + dev_dbg(&adap->dev, "%s(): entering: stat = %04x.\n",
> + __FUNCTION__, ioread32(I2C_REG_STS(alg_data)));
> +
> + /* Write a STOP bit to TX FIFO */
> + iowrite32(0x000000ff | stop_bit, I2C_REG_TX(alg_data));
> +
> + /* Wait until the STOP is seen. */
> + while (ioread32(I2C_REG_STS(alg_data)) & mstatus_active && cnt--);
This is bad, this is a busy loop, you can do up to COMPLETION_COUNT
(10000) consecutive ioreads. Please implement a proper duration-based
timeout with delays between read attempts.
> +
> + dev_dbg(&adap->dev, "%s(): exiting: stat = %04x.\n",
> + __FUNCTION__, ioread32(I2C_REG_STS(alg_data)));
> +}
> +
> +/**
> + * i2c_pnx_master_xmit - transmit data to slave
> + * @adap: pointer to I2C adapter structure
> + *
> + * Sends one byte of data to the slave
> + */
> +static int i2c_pnx_master_xmit(struct i2c_adapter *adap)
> +{
> + struct i2c_pnx_algo_data *alg_data = adap->algo_data;
> + u32 val;
> + int cnt = COMPLETION_COUNT;
> +
> + dev_dbg(&adap->dev, "%s(): entering: stat = %04x.\n",
> + __FUNCTION__, ioread32(I2C_REG_STS(alg_data)));
> +
> + if (alg_data->mif.len > 0) {
> + /* We still have somthing to talk about... */
Typo: something.
> + val = *alg_data->mif.buf++;
> + val &= 0x000000ff;
Useless, *alg_data->mif.buf is an 8-bit value already.
> +
> + if (alg_data->mif.len == 1) {
> + /* Last byte, issue a stop */
> + val |= stop_bit;
> + }
> +
> + alg_data->mif.len--;
> + iowrite32(val, I2C_REG_TX(alg_data));
> +
> + dev_dbg(&adap->dev, "%s(): xmit %#x [%d]\n", __FUNCTION__,
> + val, alg_data->mif.len + 1);
> +
> + if (alg_data->mif.len == 0) {
> + /* Wait until the STOP is seen. */
> + while(ioread32(I2C_REG_STS(alg_data)) & mstatus_active
> + && cnt--);
Please add an extra pair of parentheses to avoid confusion.
As for i2c_pnx_stop, please implement a proper duration-based timeout.
> + if (cnt <= 0)
> + printk(KERN_WARNING "The bus is still "
> + "active after timeout\n");
> +
> + /* Disable master interrupts */
> + iowrite32(ioread32(I2C_REG_CTL(alg_data)) &
> + ~(mcntrl_afie | mcntrl_naie | mcntrl_drmie),
> + I2C_REG_CTL(alg_data));
> +
> + del_timer_sync(&alg_data->mif.timer);
> +
> + dev_dbg(&adap->dev, "%s(): Waking up xfer routine.\n",
> + __FUNCTION__);
> +
> + complete(&alg_data->mif.complete);
> + }
> + } else /* For zero-sized transfers */ if (alg_data->mif.len == 0) {
Don't mix comments with code that way, it's confusing. Also, this test
seems to be exclusive with the one in the "if" part, so if wouldn't be
needed.
> + i2c_pnx_stop(adap); /* Issue a stop. */
> +
> + /* Disable master interrupts. */
> + iowrite32(ioread32(I2C_REG_CTL(alg_data)) &
> + ~(mcntrl_afie | mcntrl_naie | mcntrl_drmie),
> + I2C_REG_CTL(alg_data));
> +
> + /* Stop timer. */
> + del_timer_sync(&alg_data->mif.timer);
> + dev_dbg(&adap->dev, "%s(): Waking up xfer routine after "
> + "zero-xfer.\n", __FUNCTION__);
> +
> + complete(&alg_data->mif.complete);
> + }
> +
> + dev_dbg(&adap->dev, "%s(): exiting: stat = %04x.\n",
> + __FUNCTION__, ioread32(I2C_REG_STS(alg_data)));
> +
> + return 0;
> +}
> +
> +/**
> + * i2c_pnx_master_rcv - receive data from slave
> + * @adap: pointer to I2C adapter structure
> + *
> + * Reads one byte data from the slave
> + */
> +static int i2c_pnx_master_rcv(struct i2c_adapter *adap)
> +{
> + struct i2c_pnx_algo_data *alg_data = adap->algo_data;
> + unsigned int val = 0, cnt = COMPLETION_COUNT;
> + u32 ctl;
> +
> + dev_dbg(&adap->dev, "%s(): entering: stat = %04x.\n",
> + __FUNCTION__, ioread32(I2C_REG_STS(alg_data)));
> +
> + /* Check, whether there is already data,
> + * or we didn't 'ask' for it yet.
> + */
> + if (ioread32(I2C_REG_STS(alg_data)) & mstatus_rfe) {
> + dev_dbg(&adap->dev, "%s(): Write dummy data to fill "
> + "Rx-fifo...\n", __FUNCTION__);
> +
> + if (alg_data->mif.len == 1) {
> + /* Last byte, do not acknowledge next rcv. */
> + val |= stop_bit;
> +
> + /* Enable interrupt RFDAIE (= data in Rx-fifo.),
> + * and disable DRMIE (= need data for Tx.)
> + */
> + ctl = ioread32(I2C_REG_CTL(alg_data));
> + ctl |= mcntrl_rffie | mcntrl_daie;
> + ctl &= ~mcntrl_drmie;
> + iowrite32(ctl, I2C_REG_CTL(alg_data));
> + }
> +
> + /* Now we'll 'ask' for data:
> + * For each byte we want to receive, we must
> + * write a (dummy) byte to the Tx-FIFO.
> + */
> + iowrite32(val, I2C_REG_TX(alg_data));
> +
> + return 0;
> + }
> +
> + /* Handle data. */
> + if (alg_data->mif.len > 0) {
> +
> + val = ioread32(I2C_REG_RX(alg_data));
> + *alg_data->mif.buf++ = (u8) (val & 0xff);
> + dev_dbg(&adap->dev, "%s(): rcv 0x%x [%d]\n", __FUNCTION__, val,
> + alg_data->mif.len);
> +
> + alg_data->mif.len--;
> + if (alg_data->mif.len == 0) {
> + /* Wait until the STOP is seen. */
> + while(ioread32(I2C_REG_STS(alg_data)) & mstatus_active
> + && cnt--);
Same as above. There are many more later in the code, I won't comment
on all of them but the problem is the same.
> +
> + /* Disable master interrupts */
> + ctl = ioread32(I2C_REG_CTL(alg_data));
> + ctl &= ~(mcntrl_afie | mcntrl_naie |mcntrl_rffie |
> + mcntrl_drmie | mcntrl_daie);
Coding style: missing space.
> + iowrite32(ctl, I2C_REG_CTL(alg_data));
> +
> + /* Kill timer. */
> + del_timer_sync(&alg_data->mif.timer);
> + complete(&alg_data->mif.complete);
> + }
> + }
> +
> + dev_dbg(&adap->dev, "%s(): exiting: stat = %04x.\n",
> + __FUNCTION__, ioread32(I2C_REG_STS(alg_data)));
> +
> + return 0;
> +}
> +
> +static irqreturn_t
> +i2c_pnx_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> +{
> + u32 stat, ctl;
> + struct i2c_adapter *adap = dev_id;
> + struct i2c_pnx_algo_data *alg_data = adap->algo_data;
> +
> + dev_dbg(&adap->dev, "%s(): mstat = %x mctrl = %x, mmode = %s\n",
> + __FUNCTION__,
> + ioread32(I2C_REG_STS(alg_data)),
> + ioread32(I2C_REG_CTL(alg_data)),
> + i2c_pnx_modestr[alg_data->mif.mode]);
> + stat = ioread32(I2C_REG_STS(alg_data));
> +
> + /* let's see what kind of event this is */
> + if (stat & mstatus_afi) {
> + /* We lost arbitration in the midst of a transfer */
> + alg_data->mif.ret = -EIO;
> +
> + /* Disable master interrupts. */
> + ctl = ioread32(I2C_REG_CTL(alg_data));
> + ctl &= ~(mcntrl_afie | mcntrl_naie | mcntrl_rffie |
> + mcntrl_drmie);
> + iowrite32(ctl, I2C_REG_CTL(alg_data));
> +
> + /* Stop timer, to prevent timeout. */
> + del_timer_sync(&alg_data->mif.timer);
> + complete(&alg_data->mif.complete);
> + } else if (stat & mstatus_nai) {
> + /* Slave did not acknowledge, generate a STOP */
> + dev_dbg(&adap->dev, "%s(): "
> + "Slave did not acknowledge, generating a STOP.\n",
> + __FUNCTION__);
> + i2c_pnx_stop(adap);
> +
> + /* Disable master interrupts. */
> + ctl = ioread32(I2C_REG_CTL(alg_data));
> + ctl &= ~(mcntrl_afie | mcntrl_naie | mcntrl_rffie |
> + mcntrl_drmie);
> + iowrite32(ctl, I2C_REG_CTL(alg_data));
> +
> + /* Our return value. */
> + alg_data->mif.ret = -EFAULT;
-EIO seems more appropriate.
> +
> + /* Stop timer, to prevent timeout. */
> + del_timer_sync(&alg_data->mif.timer);
> + complete(&alg_data->mif.complete);
> + } else {
> + /* Two options:
> + * - Master Tx needs data.
> + * - There is data in the Rx-fifo
> + * The latter is only the case if we have requested for data,
> + * via a dummy write. (See 'i2c_pnx_master_rcv'.)
> + * We therefore check, as a sanity check, whether that interrupt
> + * has been enabled.
> + */
> + if ((stat & mstatus_drmi) || !(stat & mstatus_rfe)) {
> + if (alg_data->mif.mode == xmit) {
> + i2c_pnx_master_xmit(adap);
> + } else if (alg_data->mif.mode == rcv) {
> + i2c_pnx_master_rcv(adap);
> + }
> + }
> + }
> +
> + /* Clear TDI and AFI bits */
> + stat = ioread32(I2C_REG_STS(alg_data));
> + iowrite32(stat | mstatus_tdi | mstatus_afi, I2C_REG_STS(alg_data));
> +
> + dev_dbg(&adap->dev, "%s(): exiting, stat = %x ctrl = %x.\n",
> + __FUNCTION__, ioread32(I2C_REG_STS(alg_data)),
> + ioread32(I2C_REG_CTL(alg_data)));
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void i2c_pnx_timeout(unsigned long data)
> +{
> + struct i2c_pnx_algo_data *alg_data = (struct i2c_pnx_algo_data *)data;
> + u32 ctl;
> + int cnt = COMPLETION_COUNT;
> +
> + printk(KERN_ERR "Master timed out. stat = %04x, cntrl = %04x. "
> + "Resetting master ...\n",
> + ioread32(I2C_REG_STS(alg_data)),
> + ioread32(I2C_REG_CTL(alg_data)));
> +
> + /* Reset master and disable interrupts */
> + ctl = ioread32(I2C_REG_CTL(alg_data));
> + ctl &= ~(mcntrl_afie | mcntrl_naie | mcntrl_rffie | mcntrl_drmie);
> + iowrite32(ctl, I2C_REG_CTL(alg_data));
> +
> + ctl |= mcntrl_reset;
> + iowrite32(ctl, I2C_REG_CTL(alg_data));
> + while (ioread32(I2C_REG_CTL(alg_data)) & mcntrl_reset && cnt--);
> +
> + alg_data->mif.ret = -EIO;
> +
> + complete(&alg_data->mif.complete);
> +}
> +
> +/**
> + * i2c_pnx_xfer - generic transfer entry point
> + * @adap: pointer to I2C adapter structure
> + * @msgs: array of messages
> + * @num: number of messages
> + *
> + * Initiates the transfer
> + */
> +static int
> +i2c_pnx_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +{
I think we no more want [] in function parameters, use * instead.
> + struct i2c_msg *pmsg;
> + int rc = 0, completed = 0, i, cnt = COMPLETION_COUNT;
> + struct i2c_pnx_algo_data *alg_data = adap->algo_data;
> + u32 stat;
> +
> + dev_dbg(&adap->dev, "%s(): entering: stat = %04x.\n",
> + __FUNCTION__, ioread32(I2C_REG_STS(alg_data)));
> +
> + down(&alg_data->mif.sem);
There is already a mutex in struct i2c_adapter, operated by i2c-core,
which guarantees access exclusivity, so you don't need your own
semaphore.
> +
> + /* If the bus is still active, or there is already
> + * data in one of the fifo's, there is something wrong.
> + * Repair this by a reset ...
> + */
Typo: No space before the dots.
> + if (ioread32(I2C_REG_STS(alg_data)) & mstatus_active) {
> + iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
> + I2C_REG_CTL(alg_data));
> + while(ioread32(I2C_REG_CTL(alg_data)) & mcntrl_reset && cnt--);
> + } else if (!(ioread32(I2C_REG_STS(alg_data)) & mstatus_rfe) ||
> + !(ioread32(I2C_REG_STS(alg_data)) & mstatus_tfe)) {
You could store the value returned by ioread32(I2C_REG_STS(alg_data))
to save a couple reads. You even have a "stat" variable handy.
> + printk(KERN_ERR "%s: FIFO's contain already data before xfer."
> + " Reset it ...\n", adap->name);
When splitting a string please place the space at end of first half.
> + iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
> + I2C_REG_CTL(alg_data));
> + while(ioread32(I2C_REG_CTL(alg_data)) & mcntrl_reset && cnt--);
> + }
> +
> + /* Process transactions in a loop. */
> + for (i = 0; rc >= 0 && i < num;) {
> + u8 addr;
> +
> + pmsg = &msgs[i++];
Please move the i++ inside the "for" construct, it's way easier to read
that way.
> + addr = pmsg->addr;
> +
> + if (pmsg->flags & I2C_M_TEN) {
> + printk(KERN_ERR "%s: 10 bits addr not supported!\n",
> + adap->name);
> + rc = -EINVAL;
> + break;
> + }
> +
> + /* Set up address and r/w bit */
> + if (pmsg->flags & I2C_M_REV_DIR_ADDR) {
> + addr ^= 1;
> + }
You are not actually setting the r/w bit here, your address is still
right-padded at this time, so you are inverting the LSB of the address
itself. This isn't what I2C_M_REV_DIR_ADDR is supposed to do.
Do you actually need I2C_M_REV_DIR_ADDR? This is a weird protocol
extension, so unless you actually need it, please do not implement it.
As far as know, only the Matrox Maven chip ever needed this.
> +
> + alg_data->mif.buf = pmsg->buf;
> + alg_data->mif.len = pmsg->len;
> + alg_data->mif.mode = (pmsg->flags & I2C_M_RD) ? rcv : xmit;
> + alg_data->mif.ret = 0;
> +
> + dev_dbg(&adap->dev, "%s(): %s mode, %d bytes\n", __FUNCTION__,
> + i2c_pnx_modestr[alg_data->mif.mode],
> + alg_data->mif.len);
> +
> + i2c_pnx_arm_timer(adap);
> +
> + /* initialize the completion var */
> + init_completion(&alg_data->mif.complete);
> +
> + /* Enable master interrupt */
> + iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_afie |
> + mcntrl_naie | mcntrl_drmie,
> + I2C_REG_CTL(alg_data));
> +
> + /* Put start-code and slave-address on the bus. */
> + rc = i2c_pnx_start(addr, adap);
> + if (rc < 0)
> + break;
> +
> + /* Wait for completion */
> + wait_for_completion(&alg_data->mif.complete);
> +
> + if (!(rc = alg_data->mif.ret))
> + completed++;
> + dev_dbg(&adap->dev, "%s(): Complete, return code = %d.\n",
> + __FUNCTION__, rc);
> +
> + /* Clear TDI and AFI bits in case they are set. */
> + if ((stat = ioread32(I2C_REG_STS(alg_data))) & mstatus_tdi) {
> + printk(KERN_WARNING
> + "%s: TDI still set ... clearing now.\n",
> + adap->name);
> + stat |= mstatus_tdi;
This doesn't seem to have any effect, as you just checked that this bit
was already set.
> + iowrite32(stat, I2C_REG_STS(alg_data));
> + }
> + if ((stat = ioread32(I2C_REG_STS(alg_data))) & mstatus_afi) {
> + printk(KERN_WARNING
> + "%s: AFI still set ... clearing now.\n",
> + adap->name);
> + stat |= mstatus_afi;
Same here.
> + iowrite32(stat, I2C_REG_STS(alg_data));
> + }
> + }
> +
> + /* If the bus is still active, reset it to prevent
> + * corruption of future transactions.
> + */
> + if (ioread32(I2C_REG_STS(alg_data)) & mstatus_active) {
> + printk(KERN_WARNING
> + "%s: Bus is still active after xfer. Reset it ...\n",
> + adap->name);
> + iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
> + I2C_REG_CTL(alg_data));
> + while(ioread32(I2C_REG_CTL(alg_data)) & mcntrl_reset && cnt--);
> + } else if (!((stat = ioread32(I2C_REG_STS(alg_data))) & mstatus_rfe) ||
> + !(stat & mstatus_tfe)) {
> + /* If there is data in the fifo's after transfer,
> + * flush fifo's by reset.
> + */
> + iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
> + I2C_REG_CTL(alg_data));
> + while(ioread32(I2C_REG_CTL(alg_data)) & mcntrl_reset && cnt--);
> + } else if (ioread32(I2C_REG_STS(alg_data)) & mstatus_nai) {
> + iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
> + I2C_REG_CTL(alg_data));
> + while(ioread32(I2C_REG_CTL(alg_data)) & mcntrl_reset && cnt--);
> + }
This seems to be very similar to the first part of your function. I see
little reason to do it both before and after the transactions, but if
you really want to, please move that common code to a seperate function.
> +
> + /* Cleanup to be sure ... */
> + alg_data->mif.buf = NULL;
> + alg_data->mif.len = 0;
Useless as far as I can see, but well...
> +
> + /* Release sem */
> + up(&alg_data->mif.sem);
> + dev_dbg(&adap->dev, "%s(): exiting, stat = %x\n",
> + __FUNCTION__, ioread32(I2C_REG_STS(alg_data)));
> +
> + if (completed != num)
> + return ((rc < 0) ? rc : -EREMOTEIO);
> +
> + return num;
> +}
When sending more than once message at once, the I2C protocol says
that you should have a "repeated start" condition between messages. It
seems that your driver is instead sending a stop condition followed by
a start condition, which is different. Please check the hardware
documentation for your device and the I2C specification, you are
probably not sending the correct signals on the wire (could be checked
with an oscilloscope.) It may still work in some cases, but some
devices may not accept the alternative (start/stop) formatting.
> +
> +static u32 i2c_pnx_func(struct i2c_adapter *adapter)
> +{
> + return I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA | I2C_FUNC_SMBUS_READ_BYTE_DATA |
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA | I2C_FUNC_SMBUS_READ_WORD_DATA |
> + I2C_FUNC_SMBUS_QUICK;
> +}
I see nothing preventing your code from sending and receiving longer
(block) messages, so this should be: I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> +
> +/* For now, we only handle combined mode (smbus) */
This comment doesn't make much sense.
> +static struct i2c_algorithm pnx_algorithm = {
> + .master_xfer = i2c_pnx_xfer,
> + .functionality = i2c_pnx_func,
> +};
> +
> +static int i2c_pnx_controller_suspend(struct platform_device *pdev,
> + pm_message_t state)
> +{
> + struct pnx_i2c *i2c_pnx = platform_get_drvdata(pdev);
> + return i2c_pnx->suspend(pdev, state);
> +}
> +
> +static int i2c_pnx_controller_resume(struct platform_device *pdev)
> +{
> + struct pnx_i2c *i2c_pnx = platform_get_drvdata(pdev);
> + return i2c_pnx->resume(pdev);
> +}
> +
> +static int i2c_pnx_probe(struct platform_device *pdev)
Can be tagged __devinit.
> +{
> + unsigned long tmp;
> + int ret = 0, cnt = COMPLETION_COUNT;
> + struct i2c_pnx_platform_data *plat_data = pdev->dev.platform_data;
> + struct i2c_pnx_algo_data *alg_data;
> + int freq_mhz;
> + struct pnx_i2c *i2c_pnx;
> +
> + if (!plat_data) {
> + printk(KERN_ERR "%s: no platform data supplied\n",
> + __FUNCTION__);
dev_err(...)
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + i2c_pnx = kzalloc(sizeof(struct pnx_i2c), SLAB_KERNEL);
> + if (!i2c_pnx) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + i2c_pnx->suspend = plat_data->suspend;
> + i2c_pnx->resume = plat_data->resume;
> + i2c_pnx->calculate_input_freq = plat_data->calculate_input_freq;
> + i2c_pnx->set_clock_run = plat_data->set_clock_run;
> + i2c_pnx->set_clock_stop = plat_data->set_clock_stop;
Why don't you simply store a pointer to the platform data, rather than
duplicating every entry?
> + i2c_pnx->adapter = plat_data->adapter;
> + platform_set_drvdata(pdev, i2c_pnx);
> +
> + if (i2c_pnx->calculate_input_freq)
> + freq_mhz = i2c_pnx->calculate_input_freq(pdev);
> + else {
> + freq_mhz = 13;
Maybe define e.g. PNX_DEFAULT_FREQ at the top of the file?
> + printk(KERN_WARNING "Setting bus frequency to default value: "
> + "%d MHz", freq_mhz);
> + }
How does this constitute a warning? I would use dev_info().
> +
> + i2c_pnx->adapter->algo = &pnx_algorithm;
> +
> + alg_data = i2c_pnx->adapter->algo_data;
> + memset(&alg_data->mif, 0, sizeof(alg_data->mif));
Already done by kzalloc above.
> + init_MUTEX(&alg_data->mif.sem);
> + init_timer(&alg_data->mif.timer);
> + alg_data->mif.timer.function = i2c_pnx_timeout;
> + alg_data->mif.timer.data = (unsigned long)alg_data;
> +
> + /* Register I/O resource */
> + if (!request_region(alg_data->base, I2C_BLOCK_SIZE, "i2c-pnx")) {
I suggest using the device .name instead. This avoid duplicating the
string, and ensures consistency.
> + printk(KERN_ERR
> + "I/O region 0x%08x for I2C already in use.\n",
> + alg_data->base);
dev_err()
> + ret = -ENODEV;
> + goto out0;
> + }
> +
> + if (!(alg_data->ioaddr =
> + (u32)ioremap(alg_data->base, I2C_BLOCK_SIZE))) {
> + printk(KERN_ERR "Couldn't ioremap I2C I/O region\n");
> + ret = -ENOMEM;
> + goto out1;
> + }
> +
> + i2c_pnx->set_clock_run(pdev);
> +
> + /* Clock Divisor High This value is the number of system clocks
> + * the serial clock (SCL) will be high. n is defined at compile
> + * time based on the system clock (PCLK) and minimum serial frequency.
> + * For example, if the system clock period is 50 ns and the maximum
> + * desired serial period is 10000 ns (100 kHz), then CLKHI would be
> + * set to 0.5*(f_sys/f_i2c)-2=0.5*(20e6/100e3)-2=98. The actual value
> + * programmed into CLKHI will vary from this slightly due to
> + * variations in the output pad s rise and fall times as well as
"pad s"?
> + * the deglitching filter length. In this example, n = 7, since
> + * eight bits are needed to hold the clock divider count.
> + */
I don't understand the explanation, especially the last sentence. How
do you go from 98 to 7?
> +
> + tmp = ((freq_mhz * 1000) / I2C_PNX_SPEED_KHZ) / 2 - 2;
> + iowrite32(tmp, I2C_REG_CKH(alg_data));
> + iowrite32(tmp, I2C_REG_CKL(alg_data));
> + iowrite32(0, I2C_REG_CTL(alg_data));
> +
> + iowrite32(mcntrl_reset, I2C_REG_CTL(alg_data));
Is it necessary to write 0 if you are going to reset just after?
> + while ((ioread32(I2C_REG_CTL(alg_data)) & mcntrl_reset) && cnt--);
> + if (!cnt) {
> + ret = -ENODEV;
> + goto out2;
> + }
> + init_completion(&alg_data->mif.complete);
> +
> + ret = request_irq(alg_data->irq, i2c_pnx_interrupt,
> + 0, CHIP_NAME, i2c_pnx->adapter);
Here again why not use the device .name?
> + if (ret)
> + goto out3;
> +
> + /* Register this adapter with the I2C subsystem */
> + i2c_pnx->adapter->dev.parent = &pdev->dev;
> + ret = i2c_add_adapter(i2c_pnx->adapter);
> + if (ret < 0) {
> + printk(KERN_INFO "I2C: Failed to add bus\n");
> + goto out4;
> + }
> +
> + printk(KERN_INFO "%s: Master at %#8x, irq %d.\n",
> + i2c_pnx->adapter->name, alg_data->base, alg_data->irq);
> +
> + return 0;
> +
> +out4:
> + free_irq(alg_data->irq, alg_data);
> +out3:
> + i2c_pnx->set_clock_stop(pdev);
> +out2:
> + iounmap((void *)alg_data->ioaddr);
Unnecessary cast?
> +out1:
> + release_region(alg_data->base, I2C_BLOCK_SIZE);
> +out0:
Missing platform_set_drvdata(pdev, NULL) before the kfree.
> + kfree(i2c_pnx);
> +out:
> + return ret;
> +}
Can you please rename labels to make them more explicit? It helps when
you have to check, add or remove error paths. Names like out_release,
out_irq, etc...
> +
> +static int i2c_pnx_remove(struct platform_device *pdev)
Can be tagged __devexit.
> +{
> + struct pnx_i2c *i2c_pnx = platform_get_drvdata(pdev);
> + struct i2c_adapter *adap = i2c_pnx->adapter;
> + struct i2c_pnx_algo_data *alg_data =
> + (struct i2c_pnx_algo_data *)adap->algo_data;
> + int cnt = COMPLETION_COUNT;
> +
> + /* Reset the I2C controller. The reset bit is self clearing. */
> + iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
> + I2C_REG_CTL(alg_data));
What sense does it make to reset the device on exit?
> + while (ioread32(I2C_REG_CTL(alg_data)) & mcntrl_reset && cnt--);
> +
> + free_irq(alg_data->irq, alg_data);
> + i2c_del_adapter(adap);
> + i2c_pnx->set_clock_stop(pdev);
> + iounmap((void *)alg_data->ioaddr);
Unnecessary cast?
> + release_region(alg_data->base, I2C_BLOCK_SIZE);
> + kfree(i2c_pnx);
> + platform_set_drvdata(pdev, NULL);
Race condition here (at least potential), please swap these last two
lines.
> +
> + return 0;
> +}
> +
> +static struct platform_driver i2c_pnx_driver = {
> + .driver = {
> + .name = "pnx-i2c",
Missing ".owner = THIS_MODULE".
> + },
> + .probe = i2c_pnx_probe,
> + .remove = i2c_pnx_remove,
Becomes __devexit_p(i2c_pnx_remove) if you tag i2c_pnx_remove
__devexit.
> + .suspend = i2c_pnx_controller_suspend,
> + .resume = i2c_pnx_controller_resume,
> +};
> +
> +static int __init i2c_adap_pnx_init(void)
> +{
> + return platform_driver_register(&i2c_pnx_driver);
> +}
> +
> +static void i2c_adap_pnx_exit(void)
Can be tagged __exit.
> +{
> + return platform_driver_unregister(&i2c_pnx_driver);
> +}
No "return" here.
> +
> +MODULE_AUTHOR("Dennis Kovalev <dkovalev at ru.mvista.com>");
> +MODULE_DESCRIPTION("I2C driver for PNX bus");
"PNX bus" doesn't make much sense, does it?
> +MODULE_LICENSE("GPL");
> +
> +subsys_initcall(i2c_adap_pnx_init);
Why not simply module_init?
> +module_exit(i2c_adap_pnx_exit);
> Index: linux-2.6.git/include/asm-arm/arch-pnx4008/i2c.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.git/include/asm-arm/arch-pnx4008/i2c.h
> @@ -0,0 +1,80 @@
> +/*
> + * include/asm-arm/arch-pnx4008/i2c.h
> + *
> + * Author: Vitaly Wool <vwool at ru.mvista.com>
> + *
> + * PNX4008-specific tweaks for I2C IP3204 block
> + *
> + * 2005 (c) MontaVista Software, Inc. 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.
> + */
> +
> +#ifndef __ASM_ARCH_I2C_H__
> +#define __ASM_ARCH_I2C_H__
> +
> +#include <linux/i2c.h>
> +#include <linux/i2c-pnx.h>
You don't seem to need this one, do you?
> +#include <linux/platform_device.h>
> +#include <asm/irq.h>
Nor this one. OTOH you would need <linux/pm.h> for pm_message_t.
> +
> +enum {
> + mstatus_tdi = 0x00000001,
> + mstatus_afi = 0x00000002,
> + mstatus_nai = 0x00000004,
> + mstatus_drmi = 0x00000008,
> + mstatus_active = 0x00000020,
> + mstatus_scl = 0x00000040,
> + mstatus_sda = 0x00000080,
> + mstatus_rff = 0x00000100,
> + mstatus_rfe = 0x00000200,
> + mstatus_tff = 0x00000400,
> + mstatus_tfe = 0x00000800,
> +};
> +
> +enum {
> + mcntrl_tdie = 0x00000001,
> + mcntrl_afie = 0x00000002,
> + mcntrl_naie = 0x00000004,
> + mcntrl_drmie = 0x00000008,
> + mcntrl_daie = 0x00000020,
> + mcntrl_rffie = 0x00000040,
> + mcntrl_tffie = 0x00000080,
> + mcntrl_reset = 0x00000100,
> + mcntrl_cdbmode = 0x00000400,
> +};
> +
> +enum {
> + rw_bit = 1 << 0,
> + start_bit = 1 << 8,
> + stop_bit = 1 << 9,
> +};
> +
> +#define I2C_REG_RX(a) ((a)->ioaddr) /* Rx FIFO reg (RO) */
> +#define I2C_REG_TX(a) ((a)->ioaddr) /* Tx FIFO reg (WO) */
> +#define I2C_REG_STS(a) ((a)->ioaddr + 0x04) /* Status reg (RO) */
> +#define I2C_REG_CTL(a) ((a)->ioaddr + 0x08) /* Ctl reg */
> +#define I2C_REG_CKL(a) ((a)->ioaddr + 0x0c) /* Clock divider low */
> +#define I2C_REG_CKH(a) ((a)->ioaddr + 0x10) /* Clock divider high */
> +#define I2C_REG_ADR(a) ((a)->ioaddr + 0x14) /* I2C address */
> +#define I2C_REG_RFL(a) ((a)->ioaddr + 0x18) /* Rx FIFO level (RO) */
> +#define I2C_REG_TFL(a) ((a)->ioaddr + 0x1c) /* Tx FIFO level (RO) */
> +#define I2C_REG_RXB(a) ((a)->ioaddr + 0x20) /* Num of bytes Rx-ed (RO) */
> +#define I2C_REG_TXB(a) ((a)->ioaddr + 0x24) /* Num of bytes Tx-ed (RO) */
> +#define I2C_REG_TXS(a) ((a)->ioaddr + 0x28) /* Tx slave FIFO (RO) */
> +#define I2C_REG_STFL(a) ((a)->ioaddr + 0x2c) /* Tx slave FIFO level (RO) */
> +
> +#define HCLK_MHZ 13
> +#define TIMEOUT (2*(HZ))
> +
> +struct i2c_pnx_platform_data {
> + int (*suspend) (struct platform_device *pdev, pm_message_t state);
> + int (*resume) (struct platform_device *pdev);
> + u32 (*calculate_input_freq) (struct platform_device *pdev);
> + int (*set_clock_run) (struct platform_device *pdev);
> + int (*set_clock_stop) (struct platform_device *pdev);
> + struct i2c_adapter *adapter;
> +};
> +
> +#endif /* __ASM_ARCH_I2C_H___ */
One extra "_".
> Index: linux-2.6.git/include/linux/i2c-pnx.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.git/include/linux/i2c-pnx.h
> @@ -0,0 +1,51 @@
> +/*
> + * include/linux/i2c-pnx.h
> + *
> + * Header file for i2c support for PNX010x/4008.
> + *
> + * Author: Dennis Kovalev <dkovalev at ru.mvista.com>
> + *
> + * 2004-2006 (c) MontaVista Software, Inc. 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.
> + */
> +
> +#ifndef
> +#define _I2C_PNX_H
> +
> +typedef enum {
> + xmit,
> + rcv,
> + nodata,
> +} i2c_pnx_mode_t;
"nodata" is not used anywhere. This suggests that this enum could go
away entirely, and you simply use I2C_SMBUS_READ and I2C_SMBUS_WRITE.
> +
> +#define I2C_BUFFER_SIZE 0x100
> +
> +struct i2c_pnx_mif {
> + int ret; /* Return value */
> + i2c_pnx_mode_t mode; /* Interface mode */
> + struct semaphore sem; /* Mutex for this interface */
> + struct completion complete; /* I/O completion */
> + struct timer_list timer; /* Timeout */
> + char * buf; /* Data buffer */
> + int len; /* Length of data buffer */
> +};
> +
> +struct i2c_pnx_algo_data {
> + u32 base;
> + u32 ioaddr;
> + int irq;
> + int clock_id;
> + /* buffer for data received from I2C bus */
> + char buffer[I2C_BUFFER_SIZE];
> + int buf_index;
buffer and buf_index are not used anywhere!
> + struct i2c_pnx_mif mif;
> +};
> +
> +#define TIMEOUT (2*(HZ))
> +#define I2C_PNX_SPEED_KHZ 100
> +#define I2C_BLOCK_SIZE 0x100
"I2C_BLOCK_SIZE" is way too generic for something which is specific to
your device. Also, it's the size of the I/O region, so "BLOCK" doesn't
fit very well. I2C_PNX_REGION_SIZE maybe?
This is also an implementation detail, platform doesn't need to know,
so this define should be moved to the driver itself (i2c-pnx.c). Same
for I2C_PNX_SPEED_KHZ.
> +#define CHIP_NAME "PNX-I2C"
> +
> +#endif
/* _I2C_PNX_H */
> Index: linux-2.6.git/arch/arm/mach-pnx4008/Makefile
> ===================================================================
> --- linux-2.6.git.orig/arch/arm/mach-pnx4008/Makefile
> +++ linux-2.6.git/arch/arm/mach-pnx4008/Makefile
> @@ -2,7 +2,7 @@
> # Makefile for the linux kernel.
> #
>
> -obj-y := core.o irq.o time.o clock.o gpio.o serial.o dma.o
> +obj-y := core.o irq.o time.o clock.o gpio.o serial.o dma.o i2c.o
> obj-m :=
> obj-n :=
> obj- :=
> Index: linux-2.6.git/arch/arm/mach-pnx4008/i2c.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.git/arch/arm/mach-pnx4008/i2c.c
> @@ -0,0 +1,186 @@
> +/*
> + * arch/arm/mach-pnx4008/i2c.c
> + *
> + * I2C initialization for PNX4008.
> + *
> + * Author: Vitaly Wool <vitalywool at gmail.com>
> + *
> + * 2005-2006 (c) MontaVista Software, Inc. 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 <asm/arch/platform.h>
> +#include <asm/arch/i2c.h>
Move them at the end.
> +#include <linux/clk.h>
> +#include <linux/config.h>
No more include config.h explicitely.
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +
> +static inline int i2c_pnx_suspend(struct platform_device *pdev,
> + pm_message_t state)
> +{
> + int retval = 0;
> +#ifdef CONFIG_PM
> + struct clk *clk;
> + char name[10];
> +
> + sprintf(name, "i2c%d_ck", pdev->id);
snprintf, please.
> + clk = clk_get(&pdev->dev, name);
> + if (!IS_ERR(clk)) {
> + clk_set_rate(clk, 0);
> + clk_put(clk);
> + } else
> + retval = -ENOENT;
> +#endif
> + return retval;
> +}
> +
> +static inline int i2c_pnx_resume(struct platform_device *pdev)
> +{
> + int retval = 0;
> +#ifdef CONFIG_PM
> + struct clk *clk;
> + char name[10];
> +
> + sprintf(name, "i2c%d_ck", pdev->id);
snprintf
> + clk = clk_get(&pdev->dev, name);
> + if (!IS_ERR(clk)) {
> + clk_set_rate(clk, 1);
> + clk_put(clk);
> + } else
> + retval = -ENOENT;
> +#endif
> + return retval;
> +}
Why don't you call set_clock_run and set_clock_stop instead of
rewriting them?
> +
> +static inline u32 calculate_input_freq(struct platform_device *pdev)
> +{
> + return HCLK_MHZ;
> +}
You can't inline a function which is latter referenced by address. Same
for the functions below.
> +
> +static inline int set_clock_run(struct platform_device *pdev)
> +{
> + struct clk *clk;
> + char name[10];
> + int retval = 0;
> +
> + sprintf(name, "i2c%d_ck", pdev->id);
snprintf
> + clk = clk_get(&pdev->dev, name);
> + if (!IS_ERR(clk)) {
> + clk_set_rate(clk, 1);
> + clk_put(clk);
> + } else
> + retval = -ENOENT;
> +
> + return retval;
> +}
> +
> +static inline int set_clock_stop(struct platform_device *pdev)
> +{
> + struct clk *clk;
> + char name[10];
> + int retval = 0;
> +
> + sprintf(name, "i2c%d_ck", pdev->id);
snprintf
> + clk = clk_get(&pdev->dev, name);
> + if (!IS_ERR(clk)) {
> + clk_set_rate(clk, 0);
> + clk_put(clk);
> + } else
> + retval = -ENOENT;
> +
> + return retval;
> +}
> +
> +static struct i2c_pnx_algo_data pnx_algo_data0 = {
> + .base = PNX4008_I2C1_BASE,
> + .irq = I2C_1_INT,
> +};
> +
> +static struct i2c_pnx_algo_data pnx_algo_data1 = {
> + .base = PNX4008_I2C2_BASE,
> + .irq = I2C_2_INT,
> +};
> +
> +static struct i2c_pnx_algo_data pnx_algo_data2 = {
> + .base = (PNX4008_USB_CONFIG_BASE + 0x300),
> + .irq = USB_I2C_INT,
> +};
> +
> +static struct i2c_adapter pnx_adapter0 = {
> + .name = CHIP_NAME "0",
> + .algo_data = (void *)&pnx_algo_data0,
> +};
I don't think the cast is needed, is it?
> +static struct i2c_adapter pnx_adapter1 = {
> + .name = CHIP_NAME "1",
> + .algo_data = (void *)&pnx_algo_data1,
> +};
> +
> +static struct i2c_adapter pnx_adapter2 = {
> + .name = "USB-I2C",
> + .algo_data = (void *)&pnx_algo_data2,
> +};
> +
> +static struct i2c_pnx_platform_data i2c0_platform_data = {
> + .suspend = i2c_pnx_suspend,
> + .resume = i2c_pnx_resume,
> + .calculate_input_freq = calculate_input_freq,
> + .set_clock_run = set_clock_run,
> + .set_clock_stop = set_clock_stop,
> + .adapter = &pnx_adapter0,
> +};
> +
> +static struct i2c_pnx_platform_data i2c1_platform_data = {
> + .suspend = i2c_pnx_suspend,
> + .resume = i2c_pnx_resume,
> + .calculate_input_freq = calculate_input_freq,
> + .set_clock_run = set_clock_run,
> + .set_clock_stop = set_clock_stop,
> + .adapter = &pnx_adapter1,
> +};
> +
> +static struct i2c_pnx_platform_data i2c2_platform_data = {
> + .suspend = i2c_pnx_suspend,
> + .resume = i2c_pnx_resume,
> + .calculate_input_freq = calculate_input_freq,
> + .set_clock_run = set_clock_run,
> + .set_clock_stop = set_clock_stop,
> + .adapter = &pnx_adapter2,
> +};
> +
> +static struct platform_device i2c0_device = {
> + .name = "pnx-i2c",
> + .id = 0,
> + .dev = {
> + .platform_data = &i2c0_platform_data,
> + },
Broken indentation, the closing curly brace should not be indented.
> +};
> +
> +static struct platform_device i2c1_device = {
> + .name = "pnx-i2c",
> + .id = 1,
> + .dev = {
> + .platform_data = &i2c1_platform_data,
> + },
> +};
> +
> +static struct platform_device i2c2_device = {
> + .name = "pnx-i2c",
> + .id = 2,
> + .dev = {
> + .platform_data = &i2c2_platform_data,
> + },
> +};
> +
> +static struct platform_device *devices[] __initdata = {
> + &i2c0_device,
> + &i2c1_device,
> + &i2c2_device,
> +};
> +
> +void __init pnx4008_register_i2c_devices(void)
> +{
> + platform_add_devices(devices, ARRAY_SIZE(devices));
> +}
This function not called anywhere?
Pfew, done :) Sorry for the delay, but as you can see I had much to
say. Please fix as much as you can, preferably everything ;) Of
course if you don't agree with some things, we might discuss them. The
most important changes (in terms of amount of work) are the timeout
loops and the way the arch-specific data is carried around, but if you
don't fix these now, I bet you'll have to do it later when more archs
use the driver.
--
Jean Delvare
More information about the i2c
mailing list