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, 06 Mar 2007 14:54:18 +0800 "Wu, Bryan" <bryan.wu@analog.com> wrote:

    > Hi folks,
    >
    > [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.
    >

    Little things...

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

    whitespace breakage there

    > +
    > + 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);
    > +}
    > +
    > +#define TWI_I2C_MODE_COMBINED 0x04
    > +
    > +struct bfin_twi_iface
    > +{

    struct bfin_twi_iface {

    > + struct semaphore twi_lock;

    Can this be converted to a mutex?

    > + int irq;
    > + spinlock_t lock;
    > + */
    >
    > ...
    >
    > +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;

    This code has zillions of unneeded casts of void*

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

    That's a busy loop until this task's timeslice has expired. It'll work,
    but it'll suck a bit. (Repeated in several places)

    > + ret = 0;
    > + for (i = 0; rc >= 0 && i < num;) {
    > + pmsg = &msgs[i++];

    Strange. Why not do the i++ in the `for' statement?

    > + if (pmsg->flags & I2C_M_TEN) {
    > + printk(KERN_ERR "i2c-bfin-twi: 10 bits addr not supported !\n");
    > + rc = -EINVAL;
    > + break;
    > + }
    > +
    >
    > ...
    >
    > + 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));

    It's preferred if code is readable in an 80-col display.

    > + SSYNC();
    > +
    > + if (iface->writeNum + 1 <= 255)
    > + bfin_write_TWI_MASTER_CTL(((iface->writeNum+1) << 6));

    -
    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 08:21    [W:0.039 / U:0.180 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site