lkml.org 
[lkml]   [2007]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -mm] Blackfin: blackfin i2c driver
On Tue, Mar 06, 2007 at 02:54:18PM +0800, Wu, Bryan wrote:
> [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.

> +config TWICLK_KHZ
> + int "TWI clock (kHZ)"
> + depends on I2C_BFIN_TWI
> + default 50
> + help
> + The unit of the TWI clock is kilo HZ. Please divide the clock
> + by 1024 if you count it in HZ. The value should be less than 400.

Indentation in Kconfig files is Tab-Space-Space.

> --- /dev/null
> +++ linux-2.6/drivers/i2c/busses/i2c-bfin-gpio.c

> +static int __init i2c_hhbf_init(void)
> +{
> +
> + if(gpio_request(CONFIG_BFIN_SCL, NULL)) {
> + printk(KERN_ERR "%s: gpio_request GPIO %d failed \n",__FUNCTION__, CONFIG_BFIN_SCL);

I may be first to ask this, but, please, use __func__ in all new code.
__FUNCTION__ is gcc-2.95-ism, which is no longer supported.

> + return -1;
> + }
> +
> + if(gpio_request(CONFIG_BFIN_SDA, NULL)) {
> + printk(KERN_ERR "%s: gpio_request GPIO %d failed \n",__FUNCTION__, CONFIG_BFIN_SDA);

__func__, Do you need to gpio_free() ?

> + return -1;
> + }
> +
> +
> + gpio_direction_output(CONFIG_BFIN_SCL);
> + gpio_direction_input(CONFIG_BFIN_SDA);
> + gpio_set_value(CONFIG_BFIN_SCL, 1);
> +
> + return i2c_bit_add_bus(&hhbf_ops);

Do you need unwinding with gpio_free() like below?

> +static void __exit i2c_hhbf_exit(void)
> +{
> + gpio_free(CONFIG_BFIN_SCL);
> + gpio_free(CONFIG_BFIN_SDA);

trailing whitespace.

> + i2c_bit_del_bus(&hhbf_ops);
> +}

> --- /dev/null
> +++ linux-2.6/drivers/i2c/busses/i2c-bfin-twi.c

> +static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
> +{
> + unsigned short twi_int_stat = bfin_read_TWI_INT_STAT();
> + unsigned short mast_stat = bfin_read_TWI_MASTER_STAT();
> +
> + if (XMTSERV & twi_int_stat) {

Please, put bitmasks on the right. Much easier to read. See below for
other places.

if (twi_int_stat & XMTSERV)
Also, I'd rename variable to twi_intr_status, to not confuse with
statistics or something.

> + /* Transmit next data */
> + if (iface->writeNum > 0) {
> + bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
> + iface->writeNum--;

StudlyCaps.

> +static int bfin_twi_master_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +{
> + struct bfin_twi_iface* iface = (struct bfin_twi_iface*)adap->algo_data;
> + struct i2c_msg *pmsg;
> + int i, ret;
> + int rc = 0;
> +
> + if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> + return -ENXIO;
> +
> + down(&iface->twi_lock);
> +
> + while (bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> + up(&iface->twi_lock);
> + schedule();
> + down(&iface->twi_lock);
> + }

> +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 = (struct bfin_twi_iface*)adap->algo_data;
> + int rc = 0;
> +
> + if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> + return -ENXIO;
> +
> + down(&iface->twi_lock);
> +
> + while(bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> + up(&iface->twi_lock);
> + schedule();
> + down(&iface->twi_lock);
> + }

Smells like schedule() abuse for delays.

> + 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));

Too many ():

bfin_write_TWI_MASTER_CTL((iface->writeNum + 1) << 6);

> + else {
> + bfin_write_TWI_MASTER_CTL(( 0xff << 6 ));

ditto

> + iface->manual_stop = 1;
> + }
> + /* Master enable */
> + bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN | ((CONFIG_TWICLK_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));

ditto

> + else
> + bfin_write_TWI_MASTER_CTL(( 0x1 << 6 ));

ditto, etc

> +static int __init i2c_bfin_twi_init(void)
> +{

> + 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", 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_TWICLK_KHZ > 400)
> + bfin_write_TWI_CLKDIV((( 5*1024 / 400 ) << 8) | (( 5*1024 / 400 ) & 0xFF));
> + else
> + bfin_write_TWI_CLKDIV((( 5*1024 / CONFIG_TWICLK_KHZ ) << 8) | (( 5*1024 / CONFIG_TWICLK_KHZ ) & 0xFF));
> +
> + /* Enable TWI */
> + bfin_write_TWI_CONTROL(bfin_read_TWI_CONTROL() | TWI_ENA);
> + SSYNC();
> +
> + return i2c_add_adapter(p_adap);

free_irq on error?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-03-06 22:45    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site