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    [W:0.031 / U:30.416 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site