| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20070306214301.GA5590@martell.zuzino.mipt.ru>
Date: Wed, 7 Mar 2007 00:43:02 +0300
From: Alexey Dobriyan <adobriyan@...il.com>
To: "Wu, Bryan" <bryan.wu@...log.com>
Cc: khali@...ux-fr.org, Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org, mhfan@...c.edu
Subject: Re: [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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists