Re: [PATCH -mm] Blackfin: blackfin i2c driver
From: Jean Delvare
Date: Thu Mar 08 2007 - 04:29:20 EST
Hi Brian,
Thanks for the quick update.
> [PATCH] Blackfin: blackfin i2c driver
>
> The i2c linux driver for blackfin architecture which supports both GPIO
> i2c operation and blackfin on-chip TWI controller i2c operation.
>
> Signed-off-by: Bryan Wu <bryan.wu@xxxxxxxxxx>
> Reviewed-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Alexey Dobriyan <adobriyan@xxxxxxxxx>
> Reviewed-by: Jean Delvare <khali@xxxxxxxxxxxx>
Actually I had only reviewed the Kconfig part, not the driver itself.
Here's a more complete review:
> ---
>
> drivers/i2c/busses/Kconfig | 47 ++++
> drivers/i2c/busses/Makefile | 2
> drivers/i2c/busses/i2c-bfin-gpio.c | 100 +++++++++
> drivers/i2c/busses/i2c-bfin-twi.c | 589 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 738 insertions(+)
>
> Index: linux-2.6/drivers/i2c/busses/Kconfig
> ===================================================================
> --- linux-2.6.orig/drivers/i2c/busses/Kconfig 2007-03-07 17:18:49.000000000 +0800
> +++ linux-2.6/drivers/i2c/busses/Kconfig 2007-03-07 17:23:05.000000000 +0800
> @@ -91,6 +91,53 @@
> This driver can also be built as a module. If so, the module
> will be called i2c-au1550.
>
> +config I2C_BLACKFIN_GPIO
> + tristate "Blackfin GPIO based I2C interface"
> + depends on I2C && BLACKFIN && EXPERIMENTAL
> + select I2C_ALGOBIT
> + help
> + Say Y here if you have an Blackfin BF5xx based
> + system and are using GPIO lines for an I2C bus.
> +
> +menu "Blackfin I2C SDA/SCL Selection"
> + depends on I2C_BLACKFIN_GPIO
> +config I2C_BLACKFIN_GPIO_SDA
> + int "SDA GPIO pin number"
> + range 0 15 if (BF533 || BF532 || BF531)
> + range 0 47 if (BF534 || BF536 || BF537)
> + range 0 47 if BF561
> + default 2
> +
> +config I2C_BLACKFIN_GPIO_SCL
> + int "SCL GPIO pin number"
> + range 0 15 if (BF533 || BF532 || BF531)
> + range 0 47 if (BF534 || BF536 || BF537)
> + range 0 47 if BF561
> + default 3
> +endmenu
> +
> +config I2C_BLACKFIN_GPIO_CYCLE_DELAY
> + int "Cycle Delay in uSec"
The proper symbol is "us".
> + depends on I2C_BLACKFIN_GPIO
> + range 5 100
> + default 40
> +
> +config I2C_BLACKFIN_TWI
> + tristate "Blackfin TWI I2C support"
> + depends on I2C && (BF534 || BF536 || BF537)
> + help
> + This is the TWI I2C device driver for Blackfin 534/536/537.
> + This driver can also be built as a module. If so, the module
> + will be called i2c-bfin-twi.
> +
> +config I2C_BLACKFIN_TWI_CLK_KHZ
> + int "Blackfin TWI I2C clock (kHz)"
> + depends on I2C_BLACKFIN_TWI
> + range 10 400
> + default 50
> + help
> + The unit of the TWI clock is kilo HZ.
kHz
> +
> config I2C_ELEKTOR
> tristate "Elektor ISA card"
> depends on I2C && ISA && BROKEN_ON_SMP
> Index: linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c 2007-03-07 17:32:30.000000000 +0800
> @@ -0,0 +1,100 @@
> +/****************************************************************
> + * Description: *
> + * *
> + * Maintainer: Meihui Fan <mhfan@xxxxxxxx> *
Indentation problem. Also, maintainer information goes in MAINTAINERS,
not individual drivers.
> + * *
> + * CopyRight (c) 2004 HHTech *
> + * www.hhcn.com, www.hhcn.org *
> + * All rights reserved. *
> + * *
> + * This file is free software; *
> + * you are free to modify and/or redistribute it *
Indentation problem here too.
> + * under the terms of the GNU General Public Licence (GPL). *
> + * *
> + ****************************************************************/
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-algo-bit.h>
> +
> +#include <asm/blackfin.h>
> +#include <asm/gpio.h>
> +
> +#define I2C_HW_B_HHBF 0x13
Holy god, no! This define goes in include/linux/i2c-id.h.
> +
> +static void hhbf_setsda(void *data, int state)
> +{
> + if (state) {
> + gpio_direction_input(CONFIG_I2C_BLACKFIN_GPIO_SDA);
> +
Extra blank line.
> + } else {
> + gpio_direction_output(CONFIG_I2C_BLACKFIN_GPIO_SDA);
> + gpio_set_value(CONFIG_I2C_BLACKFIN_GPIO_SDA, 0);
> + }
> +}
> +
> +static void hhbf_setscl(void *data, int state)
> +{
> + gpio_set_value(CONFIG_I2C_BLACKFIN_GPIO_SCL, state);
> +}
> +
> +static int hhbf_getsda(void *data)
> +{
> + return (gpio_get_value(CONFIG_I2C_BLACKFIN_GPIO_SDA) != 0);
> +}
> +
> +
> +static struct i2c_algo_bit_data bit_hhbf_data = {
> + .setsda = hhbf_setsda,
> + .setscl = hhbf_setscl,
> + .getsda = hhbf_getsda,
Is the hardware not capable of SCL sensing? If it is, you really want
to implement it. Bit-banged I2C without SCL sensing doesn't completely
implement the I2C standard, it lacks the SCL stretching feature.
> + .udelay = CONFIG_I2C_BLACKFIN_GPIO_CYCLE_DELAY,
> + .timeout = HZ
Always add an extra comma at the end of struct declarations.
> +};
> +
> +static struct i2c_adapter hhbf_ops = {
> + .owner = THIS_MODULE,
> + .id = I2C_HW_B_HHBF,
> + .algo_data = &bit_hhbf_data,
> + .name = "Blackfin GPIO based I2C driver",
This structure describes a (virtual) device, not a driver.
> +};
> +
> +static int __init i2c_hhbf_init(void)
> +{
> +
Extra blank line.
> + if (gpio_request(CONFIG_I2C_BLACKFIN_GPIO_SCL, NULL)) {
> + printk(KERN_ERR "%s: gpio_request GPIO %d failed \n", __func__,
> + CONFIG_I2C_BLACKFIN_GPIO_SCL);
The function name doesn't matter here, what is the end user going to do
with the information? What matters is the driver name.
> + return -1;
This -1 will be interpreted as -EPERM by insmod. Please use a proper
error code.
> + }
> +
> + if (gpio_request(CONFIG_I2C_BLACKFIN_GPIO_SDA, NULL)) {
> + printk(KERN_ERR "%s: gpio_request GPIO %d failed \n", __func__,
> + CONFIG_I2C_BLACKFIN_GPIO_SDA);
> + return -1;
Broken error path. You've successfully requested the GPIO for SCL, but
return with an error without releasing it.
> + }
> +
> +
Extra blank line.
> + gpio_direction_output(CONFIG_I2C_BLACKFIN_GPIO_SCL);
> + gpio_direction_input(CONFIG_I2C_BLACKFIN_GPIO_SDA);
> + gpio_set_value(CONFIG_I2C_BLACKFIN_GPIO_SCL, 1);
> +
At this point, i2c-core will complain that you created an i2c_adapter
without a valid parent device. This needs to be fixed.
> + return i2c_bit_add_bus(&hhbf_ops);
> +}
> +
> +static void __exit i2c_hhbf_exit(void)
> +{
> + gpio_free(CONFIG_I2C_BLACKFIN_GPIO_SCL);
> + gpio_free(CONFIG_I2C_BLACKFIN_GPIO_SDA);
> + i2c_bit_del_bus(&hhbf_ops);
You're doing things the wrong way around here, you're freeing the
resources while there might still be someone using the adapter.
BTW, i2c_bit_del_bus no longer exists, so this won't compile. Use
i2c_del_adapter instead. You really want to test your code with a
recent kernel before submitting it again.
> +}
> +
> +MODULE_AUTHOR("Meihui Fan <mhfan@xxxxxxxx>");
> +MODULE_DESCRIPTION("I2C-Bus adapter routines for Blackfin and HHBF Boards");
> +MODULE_LICENSE("GPL");
> +
> +module_init(i2c_hhbf_init);
> +module_exit(i2c_hhbf_exit);
> Index: linux-2.6/drivers/i2c/busses/i2c-bfin-twi.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/drivers/i2c/busses/i2c-bfin-twi.c 2007-03-07 17:28:57.000000000 +0800
> @@ -0,0 +1,589 @@
> +/****************************************************************
> + * Description: Driver for Blackfin Two Wire Interface *
> + * Maintainer: sonicz <sonic.zhang@xxxxxxxxxx> *
Same as above, this goes to MAINTAINERS.
> + * *
> + * Copyright (c) 2005-2007 Analog Device *
> + * *
> + * This file is free software; *
> + * you are free to modify and/or redistribute it *
Broken indentation.
> + * under the terms of the GNU General Public Licence (GPL). *
> + ****************************************************************/
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/mm.h>
> +#include <linux/timer.h>
> +#include <linux/spinlock.h>
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +
> +#include <asm/blackfin.h>
> +#include <asm/irq.h>
> +
> +#define I2C_BLACKFIN_TWI 0x00
No, define a valid ID in linux/i2c-id.h if you need one, otherwise
don't set an ID at all.
> +#define POLL_TIMEOUT (2 * HZ)
> +#ifndef CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ
> +#define CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ 400
> +#endif
How could this actually not be defined?
> +
> +/* SMBus mode*/
> +#define TWI_I2C_MODE_STANDARD 0x01
> +#define TWI_I2C_MODE_STANDARDSUB 0x02
> +#define TWI_I2C_MODE_COMBINED 0x04
> +
> +struct bfin_twi_iface
> +{
The curly brace goes at the end of the previous line.
> + struct mutex twi_lock;
> + int irq;
> + spinlock_t lock;
> + char read_write;
> + u8 command;
> + u8 *transPtr;
> + int readNum;
> + int writeNum;
> + int cur_mode;
> + int manual_stop;
> + int result;
> + int timeout_count;
> + struct timer_list timeout_timer;
> + struct i2c_adapter adap;
> + struct completion complete;
> +};
> +
> +static struct bfin_twi_iface twi_iface;
> +
> +static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
> +{
> + unsigned short twi_int_status = bfin_read_TWI_INT_STAT();
> + unsigned short mast_stat = bfin_read_TWI_MASTER_STAT();
> +
> + if (twi_int_status & XMTSERV) {
> + /* Transmit next data */
> + if (iface->writeNum > 0) {
> + bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
> + iface->writeNum--;
> + }
> + /* start receive immediately after complete sending in
> + * combine mode.
> + */
> + else if (iface->cur_mode == TWI_I2C_MODE_COMBINED) {
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> + | MDIR | RSTART);
> + } else if (iface->manual_stop)
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> + | STOP);
> + SSYNC();
> + /* Clear status */
> + bfin_write_TWI_INT_STAT(XMTSERV);
> + SSYNC();
> + }
> + if (twi_int_status & RCVSERV) {
> + if (iface->readNum > 0) {
> + /* Receive next data */
> + *(iface->transPtr) = bfin_read_TWI_RCV_DATA8();
> + if (iface->cur_mode == TWI_I2C_MODE_COMBINED) {
> + /* Change combine mode into sub mode after
> + * read first data.
> + */
> + iface->cur_mode = TWI_I2C_MODE_STANDARDSUB;
> + /* Get read number from first byte in block
> + * combine mode.
> + */
> + if (iface->readNum == 1 && iface->manual_stop)
> + iface->readNum = *iface->transPtr+1;
> + }
> + iface->transPtr++;
> + iface->readNum--;
> + } else if (iface->manual_stop) {
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> + | STOP);
> + SSYNC();
> + }
> + /* Clear interrupt source */
> + bfin_write_TWI_INT_STAT(RCVSERV);
> + SSYNC();
> + }
> + if (twi_int_status & MERR) {
> + bfin_write_TWI_INT_STAT(MERR);
> + bfin_write_TWI_INT_MASK(0);
> + bfin_write_TWI_MASTER_STAT(0x3e);
> + bfin_write_TWI_MASTER_CTL(0);
> + SSYNC();
> + iface->result = -1;
> + /* if both err and complete int stats are set, return proper
> + * results.
> + */
> + if (twi_int_status & MCOMP) {
> + bfin_write_TWI_INT_STAT(MCOMP);
> + bfin_write_TWI_INT_MASK(0);
> + bfin_write_TWI_MASTER_CTL(0);
> + SSYNC();
> + /* If it is a quick transfer, only address bug no data,
> + * not an err, return 1.
> + */
> + if (iface->writeNum == 0 && (mast_stat & BUFRDERR))
> + iface->result = 1;
> + /* If address not acknowledged return -1,
> + * else return 0.
> + */
> + else if (!(mast_stat & ANAK))
> + iface->result = 0;
> + }
> + complete(&iface->complete);
> + return;
> + }
> + if (twi_int_status & MCOMP) {
> + bfin_write_TWI_INT_STAT(MCOMP);
> + SSYNC();
> + if (iface->cur_mode == TWI_I2C_MODE_COMBINED) {
> + if (iface->readNum == 0) {
> + /* set the read number to 1 and ask for manual
> + * stop in block combine mode
> + */
> + iface->readNum = 1;
> + iface->manual_stop = 1;
> + bfin_write_TWI_MASTER_CTL(
> + bfin_read_TWI_MASTER_CTL()
> + | (0xff << 6));
> + } else {
> + /* set the readd number in other
> + * combine mode.
> + */
> + bfin_write_TWI_MASTER_CTL(
> + (bfin_read_TWI_MASTER_CTL() &
> + (~(0xff << 6))) |
> + ( iface->readNum << 6));
> + }
> + /* remove restart bit and enable master receive */
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() &
> + ~RSTART);
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() |
> + MEN | MDIR);
> + SSYNC();
> + } else {
> + iface->result = 1;
> + bfin_write_TWI_INT_MASK(0);
> + bfin_write_TWI_MASTER_CTL(0);
> + SSYNC();
> + complete(&iface->complete);
> + }
> + }
> +}
> +
> +/* Interrupt handler */
> +static irqreturn_t bfin_twi_interrupt_entry(int irq, void *dev_id)
> +{
> + struct bfin_twi_iface *iface = (struct bfin_twi_iface *)dev_id;
Cast isn't needed.
> + unsigned long flags;
> +
> + spin_lock_irqsave(&iface->lock, flags);
> + del_timer(&iface->timeout_timer);
> + bfin_twi_handle_interrupt(iface);
> + spin_unlock_irqrestore(&iface->lock, flags);
> + return IRQ_HANDLED;
> +}
> +
> +static void bfin_twi_timeout(unsigned long data)
> +{
> + struct bfin_twi_iface *iface = (struct bfin_twi_iface *)data;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&iface->lock, flags);
> + bfin_twi_handle_interrupt(iface);
> + if (iface->result == 0) {
> + iface->timeout_count--;
> + if (iface->timeout_count > 0) {
> + iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
> + add_timer(&iface->timeout_timer);
> + } else {
> + iface->result = -1;
> + complete(&iface->complete);
> + }
> + }
> + spin_unlock_irqrestore(&iface->lock, flags);
> +}
> +
> +/*
> + * Generic i2c master transfer entrypoint
> + */
> +static int bfin_twi_master_xfer(struct i2c_adapter *adap,
> + struct i2c_msg msgs[], int num)
The prefered syntax is struct i2c_msg *msgs.
> +{
> + struct bfin_twi_iface* iface = adap->algo_data;
> + struct i2c_msg *pmsg;
> + int i, ret;
> + int rc = 0;
> +
> + if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> + return -ENXIO;
> +
> + mutex_lock(&iface->twi_lock);
> +
> + while (bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> + mutex_unlock(&iface->twi_lock);
> + cond_resched();
> + mutex_lock(&iface->twi_lock);
> + }
> +
> + ret = 0;
> + for (i = 0; rc >= 0 && i < num; i++) {
> + pmsg = &msgs[i];
> + if (pmsg->flags & I2C_M_TEN) {
> + printk(KERN_ERR "i2c-bfin-twi: 10 bits addr \
> + not supported !\n");
This is not how you break a string in C. The right way is:
printk(KERN_ERR "i2c-bfin-twi: 10 bits addr "
"not supported!\n");
BTW you should be using dev_err() here instead.
> + rc = -EINVAL;
> + break;
> + }
> +
> + iface->cur_mode = TWI_I2C_MODE_STANDARD;
> + iface->manual_stop = 0;
> + iface->transPtr = pmsg->buf;
> + iface->writeNum = iface->readNum = pmsg->len;
> + iface->result = 0;
> + iface->timeout_count = 10;
> + /* Set Transmit device address */
> + bfin_write_TWI_MASTER_ADDR(pmsg->addr);
> +
> + /* FIFO Initiation. Data in FIFO should be
> + * discarded before start a new operation.
> + */
> + bfin_write_TWI_FIFO_CTL(0x3);
> + SSYNC();
> + bfin_write_TWI_FIFO_CTL(0);
> + SSYNC();
> +
> + if (pmsg->flags & I2C_M_RD)
> + iface->read_write = I2C_SMBUS_READ;
> + else {
> + iface->read_write = I2C_SMBUS_WRITE;
> + /* Transmit first data */
> + if (iface->writeNum > 0) {
> + bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
> + iface->writeNum--;
> + SSYNC();
> + }
> + }
> +
> + /* clear int stat */
> + bfin_write_TWI_INT_STAT(MERR|MCOMP|XMTSERV|RCVSERV);
> +
> + /* Interrupt mask . Enable XMT, RCV interrupt */
> + bfin_write_TWI_INT_MASK(MCOMP | MERR |
> + ((iface->read_write == I2C_SMBUS_READ)?
> + RCVSERV : XMTSERV));
> + SSYNC();
> +
> + if (pmsg->len > 0 && pmsg->len <= 255)
> + bfin_write_TWI_MASTER_CTL(pmsg->len << 6);
> + else if (pmsg->len > 255) {
> + bfin_write_TWI_MASTER_CTL(0xff << 6);
> + iface->manual_stop = 1;
> + } else
> + break;
> +
> + iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
> + add_timer(&iface->timeout_timer);
> +
> + /* Master enable */
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> + ((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
> + ((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ>100) ? FAST : 0));
> + SSYNC();
> +
> + wait_for_completion(&iface->complete);
> +
> + rc = iface->result;
> + if (rc == 1)
> + ret++;
> + else if (rc == -1)
> + break;
> + }
> +
> + /* Release mutex */
> + mutex_unlock(&iface->twi_lock);
> +
> + return ret;
ret always has the same value as i as far as I can see, so you don't
need to separate variables.
> +}
> +
> +/*
> + * SMBus type transfer entrypoint
> + */
> +
> +int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write,
> + u8 command, int size, union i2c_smbus_data * data)
> +{
> + struct bfin_twi_iface* iface = adap->algo_data;
> + int rc = 0;
> +
> + if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> + return -ENXIO;
> +
> + mutex_lock(&iface->twi_lock);
> +
> + while(bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> + mutex_unlock(&iface->twi_lock);
> + cond_resched();
> + mutex_lock(&iface->twi_lock);
> + }
> +
> + iface->writeNum = 0;
> + iface->readNum = 0;
> +
> + /* Prepare datas & select mode */
> + switch (size) {
> + case I2C_SMBUS_QUICK:
> + iface->transPtr = NULL;
> + iface->cur_mode = TWI_I2C_MODE_STANDARD;
> + break;
> + case I2C_SMBUS_BYTE:
> + if (data == NULL)
> + iface->transPtr = NULL;
> + else {
> + if (read_write == I2C_SMBUS_READ)
> + iface->readNum = 1;
> + else
> + iface->writeNum = 1;
> + iface->transPtr = &data->byte;
> + }
> + iface->cur_mode = TWI_I2C_MODE_STANDARD;
> + break;
> + case I2C_SMBUS_BYTE_DATA:
> + if (read_write == I2C_SMBUS_READ) {
> + iface->readNum = 1;
> + iface->cur_mode = TWI_I2C_MODE_COMBINED;
> + } else {
> + iface->writeNum = 1;
> + iface->cur_mode = TWI_I2C_MODE_STANDARDSUB;
> + }
> + iface->transPtr = &data->byte;
> + break;
> + case I2C_SMBUS_WORD_DATA:
> + if (read_write == I2C_SMBUS_READ) {
> + iface->readNum = 2;
> + iface->cur_mode = TWI_I2C_MODE_COMBINED;
> + } else {
> + iface->writeNum = 2;
> + iface->cur_mode = TWI_I2C_MODE_STANDARDSUB;
> + }
> + iface->transPtr = (u8 *)&data->word;
> + break;
> + case I2C_SMBUS_PROC_CALL:
> + iface->writeNum = 2;
> + iface->readNum = 2;
> + iface->cur_mode = TWI_I2C_MODE_COMBINED;
> + iface->transPtr = (u8 *)&data->word;
> + break;
> + case I2C_SMBUS_BLOCK_DATA:
> + if (read_write == I2C_SMBUS_READ) {
> + iface->readNum = 0;
> + iface->cur_mode = TWI_I2C_MODE_COMBINED;
> + } else {
> + iface->writeNum = data->block[0]+1;
> + iface->cur_mode = TWI_I2C_MODE_STANDARDSUB;
> + }
> + iface->transPtr = data->block;
> + break;
> + default:
> + return -1;
> + }
> +
> + iface->result = 0;
> + iface->manual_stop = 0;
> + iface->read_write = read_write;
> + iface->command = command;
> + iface->timeout_count = 10;
> +
> + /* FIFO Initiation. Data in FIFO should be discarded before
> + * start a new operation.
> + */
> + bfin_write_TWI_FIFO_CTL(0x3);
> + SSYNC();
> + bfin_write_TWI_FIFO_CTL(0);
> +
> + /* clear int stat */
> + bfin_write_TWI_INT_STAT(MERR|MCOMP|XMTSERV|RCVSERV);
> +
> + /* Set Transmit device address */
> + bfin_write_TWI_MASTER_ADDR(addr);
> + SSYNC();
> +
> + iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
> + add_timer(&iface->timeout_timer);
> +
> + switch (iface->cur_mode) {
> + case TWI_I2C_MODE_STANDARDSUB:
> + bfin_write_TWI_XMT_DATA8(iface->command);
> + bfin_write_TWI_INT_MASK(MCOMP | MERR |
> + ((iface->read_write == I2C_SMBUS_READ) ?
> + RCVSERV : XMTSERV));
> + SSYNC();
> +
> + if (iface->writeNum + 1 <= 255)
> + bfin_write_TWI_MASTER_CTL((iface->writeNum+1) << 6);
> + else {
> + bfin_write_TWI_MASTER_CTL(0xff << 6);
> + iface->manual_stop = 1;
> + }
> + /* Master enable */
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> + ((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ>100) ? FAST : 0));
> + break;
> + case TWI_I2C_MODE_COMBINED:
> + bfin_write_TWI_XMT_DATA8(iface->command);
> + bfin_write_TWI_INT_MASK(MCOMP | MERR | RCVSERV | XMTSERV);
> + SSYNC();
> +
> + if (iface->writeNum > 0)
> + bfin_write_TWI_MASTER_CTL((iface->writeNum+1) << 6);
> + else
> + bfin_write_TWI_MASTER_CTL(0x1 << 6);
> + /* Master enable */
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> + ((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ>100) ? FAST : 0));
> + break;
> + default:
> + bfin_write_TWI_MASTER_CTL(0);
> + if (size != I2C_SMBUS_QUICK) {
> + /* Don't access xmit data register when this is a
> + * read operation.
> + */
> + if (iface->read_write != I2C_SMBUS_READ) {
> + if (iface->writeNum > 0) {
> + bfin_write_TWI_XMT_DATA8
> + (*(iface->transPtr++));
> + if (iface->writeNum <= 255)
> + bfin_write_TWI_MASTER_CTL
> + (iface->writeNum << 6);
> + else {
> + bfin_write_TWI_MASTER_CTL
> + (0xff << 6);
> + iface->manual_stop = 1;
> + }
> + iface->writeNum--;
> + } else {
> + bfin_write_TWI_XMT_DATA8
> + (iface->command);
> + bfin_write_TWI_MASTER_CTL(1 << 6);
> + }
> + } else {
> + if (iface->readNum > 0
> + && iface->readNum <= 255)
> + bfin_write_TWI_MASTER_CTL
> + (iface->readNum << 6);
> + else if (iface->readNum > 255) {
> + bfin_write_TWI_MASTER_CTL( 0xff << 6 );
> + iface->manual_stop = 1;
> + } else {
> + del_timer(&iface->timeout_timer);
> + break;
> + }
> + }
> + }
> + bfin_write_TWI_INT_MASK(MCOMP | MERR |
> + ((iface->read_write == I2C_SMBUS_READ) ?
> + RCVSERV : XMTSERV));
> + SSYNC();
> +
> + /* Master enable */
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> + ((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
> + ((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ > 100) ? FAST : 0));
> + break;
> + }
> + SSYNC();
> +
> + wait_for_completion(&iface->complete);
> +
> + rc = (iface->result >= 0) ? 0 : -1;
> +
> + /* Release mutex */
> + mutex_unlock(&iface->twi_lock);
> +
> + return rc;
> +}
i2c-core can emulate SMBus transactions using master_xfer, so in
general when you have a complete master_xfer implementation you do not
need to define a separate smbus_xfer function. This would save a lot of
code.
> +
> +/*
> + * Return what the adapter supports
> + */
> +static u32 bfin_twi_functionality(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_PROC_CALL |
> + I2C_FUNC_I2C;
> +}
> +
> +
> +static struct i2c_algorithm bfin_twi_algorithm = {
> + .master_xfer = bfin_twi_master_xfer,
> + .smbus_xfer = bfin_twi_smbus_xfer,
> + .functionality = bfin_twi_functionality,
> +};
> +
> +static int __init i2c_bfin_twi_init(void)
> +{
> + struct i2c_adapter *p_adap;
> + int rc;
> +
> + mutex_init(&(twi_iface.twi_lock));
> + spin_lock_init(&twi_iface.lock);
> + init_completion(&twi_iface.complete);
> + twi_iface.irq = IRQ_TWI;
> +
> + init_timer(&twi_iface.timeout_timer);
> + twi_iface.timeout_timer.function = bfin_twi_timeout;
> + twi_iface.timeout_timer.data = (unsigned long)&twi_iface;
> +
> + p_adap = &twi_iface.adap;
> + p_adap->id = I2C_BLACKFIN_TWI;
> + strcpy(p_adap->name, "i2c-bfin-twi");
Please use strlcpy intead.
> + p_adap->algo = &bfin_twi_algorithm;
> + p_adap->algo_data = &twi_iface;
> + p_adap->client_register = NULL;
> + p_adap->client_unregister = NULL;
These are already NULL, no need to set them explicitly.
> + p_adap->class = I2C_CLASS_ALL;
> +
> + rc = request_irq(twi_iface.irq, bfin_twi_interrupt_entry,
> + SA_INTERRUPT, "i2c-bfin-twi", &twi_iface);
> + if (rc) {
> + printk(KERN_ERR "i2c-bfin-twi: can't get IRQ %d !\n",
No space before exclamation mark in english.
> + twi_iface.irq);
> + return -ENODEV;
> + }
> +
> + /* Set TWI internal clock as 10MHz */
> + bfin_write_TWI_CONTROL(((get_sclk() / 1024 / 1024 + 5) / 10) & 0x7F);
> +
> + /* Set Twi interface clock as specified */
> + if (CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ > 400)
> + bfin_write_TWI_CLKDIV((( 5*1024 / 400 ) << 8) |
> + (( 5*1024 / 400 ) & 0xFF));
> + else
> + bfin_write_TWI_CLKDIV((( 5*1024 / CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ ) << 8) |
> + (( 5*1024 / CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ ) & 0xFF));
Line too long, please fold more.
> +
> + /* Enable TWI */
> + bfin_write_TWI_CONTROL(bfin_read_TWI_CONTROL() | TWI_ENA);
> + SSYNC();
> +
> + rc = i2c_add_adapter(p_adap);
Here too, i2c-core will complain that you failed to provide a valid
parent device. You need to fix this, most probably by converting your
driver to a platform driver.
> + if(rc < 0)
> + free_irq(twi_iface.irq, &twi_iface);
> + return rc;
> +}
> +
> +static void __exit i2c_bfin_twi_exit(void)
> +{
> + i2c_del_adapter(&twi_iface.adap);
> + free_irq(twi_iface.irq, &twi_iface);
> +}
> +
> +MODULE_AUTHOR("Sonic Zhang <sonic.zhang@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("I2C-Bus adapter routines for Blackfin TWI");
> +MODULE_LICENSE("GPL");
> +
> +module_init(i2c_bfin_twi_init);
> +module_exit(i2c_bfin_twi_exit);
> Index: linux-2.6/drivers/i2c/busses/Makefile
> ===================================================================
> --- linux-2.6.orig/drivers/i2c/busses/Makefile 2007-03-07 17:18:49.000000000 +0800
> +++ linux-2.6/drivers/i2c/busses/Makefile 2007-03-07 17:30:11.000000000 +0800
> @@ -10,6 +10,8 @@
> obj-$(CONFIG_I2C_AMD8111) += i2c-amd8111.o
> obj-$(CONFIG_I2C_AT91) += i2c-at91.o
> obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
> +obj-$(CONFIG_I2C_BLACKFIN_GPIO) += i2c-bfin-gpio.o
> +obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
> obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o
> obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
> obj-$(CONFIG_I2C_I801) += i2c-i801.o
--
Jean Delvare
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/