Re: [PATCH -mm] Blackfin: blackfin i2c driver

[Andrew Morton]

On Tue, 06 Mar 2007 14:54:18 +0800 "Wu, Bryan" <bryan.wu@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/