>-----Original Message----- >From: uclinux-dist-devel-bounces@xxxxxxxxxxxxxxxxxxxx >[mailto:uclinux-dist-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On >Behalf Of Mike Frysinger >Sent: Wednesday, September 02, 2009 3:31 AM >To: Barry Song >Cc: dbrownell@xxxxxxxxxxxxxxxxxxxxx; dtor@xxxxxxx; >dmitry.torokhov@xxxxxxxxx; >spi-devel-general@xxxxxxxxxxxxxxxxxxxxx; >linux-input@xxxxxxxxxxxxxxx; uclinux-dist-devel@xxxxxxxxxxxxxxxxxxxx >Subject: Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input >driver forbutton/scrollwhell/slider/touchpad > >On Mon, Aug 31, 2009 at 23:55, Barry Song wrote: >> ad7142/ad7147 are Programmable Controllers for Capacitance >Touch Sensors. >> The chips don't restrict the specific usage, and it can be >used as button/ >> slider/scrollwheel/touchpad etc. depending on the hardware >connection. >> One special target board can include one or several these components. >> >> The driver is independent of special boards. It gets the components >> layout information from the platform_data, registers related devices, >> fullfills the algorithms and state machines for these components and >> report related input events to up level. > >seems you didnt address all the comments i posted after your >first commit ... > >please add this line before the #include's: >#define pr_fmt(fmt) "ad714x: " fmt >this will fix all of your dev_*() output so that it is >properly prefixed Fixed. > >> +#define AD714x_SPI_ADDR 0x1C >> +#define AD714x_SPI_ADDR_SHFT 11 >> +#define AD714x_SPI_READ 1 >> +#define AD714x_SPI_READ_SHFT 10 >> + >> +#define AD714X_PWR_CTRL 0x0 >> +#define AD714x_STG_CAL_EN_REG 0x1 >> +#define AD714X_AMB_COMP_CTRL0_REG 0x2 >> +#define AD714x_PARTID_REG 0x17 >> +#define AD7147_PARTID 0x1470 >> +#define AD7142_PARTID 0xE620 >> +#define AD714x_STAGECFG_REG 0x80 >> +#define AD714x_SYSCFG_REG 0x0 > >your AD714X and AD714x needs to be consistent Fixed. > >> +struct ad714x_button_drv { >> + ad714x_device_state state; >> + /* Unlike slider/wheel/touchpad, all buttons point to >> + * same input_dev instance */ > >either put the comment on one line and do: >/* fooo */ >or use proper comment style where the last */ is on a line by itself >/* > * foo > */ >this applies to many comments in this file Fixed. > >> +struct ad714x_chip { >> + unsigned short h_state; >> + unsigned short l_state; >> + unsigned short c_state; >> + unsigned short adc_reg[STAGE_NUM]; >> + unsigned short amb_reg[STAGE_NUM]; >> + unsigned short sensor_val[STAGE_NUM]; >> + >> + struct ad714x_platform_data *hw; >> + struct ad714x_driver_data *sw; >> + >> + bus_device *bus; >> + int (*read) (bus_device *, unsigned short, unsigned short *); >> + int (*write) (bus_device *, unsigned short, unsigned short); > >using function pointers is pure overhead in your current >implementation. create a ad714x_read macro that expands to either the >spi or the i2c version depending on which is enabled. Using macro can be a choice. But I don't think it can save much overhead here. Function pointers encapsulates the object better. > >> +static void stage_use_com_int(struct ad714x_chip *ad714x, >int start_stage, >> + int end_stage) >> +{ > >all funcs in here really should have ad714x_ symbol prefixes. if i >saw a crash and it just said "stage_xxx", it isnt immediately obvious >where this symbol is coming from. Fixed. > >> +static int stage_cal_highest_stage(struct ad714x_chip >*ad714x, int start_stage, >> + int end_stage) >> +{ >> + int max_res = 0; >> + int max_idx = 0; >> + int i; >> + >> + for (i = start_stage; i <= end_stage; i++) { >> + if (ad714x->sensor_val[i] > max_res) { >> + max_res = ad714x->sensor_val[i]; >> + max_idx = i; >> + } >> + } > >should the loop limit really be "<=" and not "<" ? seems to be this >way throughout, so maybe the logic is correct ... It's <=. > >> +static int stage_cal_abs_pos(struct ad714x_chip *ad714x, >int start_stage, >> + int end_stage, int highest_stage, int max_coord) >> +{ >> + int a_param, b_param; >> + >> + if (highest_stage == start_stage) { >> + a_param = ad714x->sensor_val[start_stage + 1]; >> + b_param = ad714x->sensor_val[start_stage] + >> + ad714x->sensor_val[start_stage + 1]; >> + } else if (highest_stage == end_stage) { >> + a_param = ad714x->sensor_val[end_stage] * >> + (end_stage - start_stage) + >> + ad714x->sensor_val[end_stage - 1] * >> + (end_stage - start_stage - 1); >> + b_param = ad714x->sensor_val[end_stage] + >> + ad714x->sensor_val[end_stage - 1]; >> + } else { >> + a_param = ad714x->sensor_val[highest_stage] * >> + (highest_stage - start_stage) + >> + ad714x->sensor_val[highest_stage - 1] * >> + (highest_stage - start_stage - 1) + >> + ad714x->sensor_val[highest_stage + 1] * >> + (highest_stage - start_stage + 1); >> + b_param = ad714x->sensor_val[highest_stage] + >> + ad714x->sensor_val[highest_stage - 1] + >> + ad714x->sensor_val[highest_stage + 1]; >> + } >> + >> + return (max_coord / (end_stage - start_stage)) * >a_param / b_param; >> +} > >do the local vars really need "_stage" suffix ? if you trimmed that, >i imagine it'd make the code a bit easier to digest. Yes. It need the _stage suffix. Otherwise, nobody know what's started, what's ended. > >> +/* One button can connect to multi postive and negative of CDCs >> + * Multi-buttons can connect to same postive/negative of one CDC > >please run a spell checker on your comments ... >postive -> positive > Fixed >> +static int ad714x_hw_detect(struct ad714x_chip *ad714x) > >should be __devinit > >> + default: >> + dev_err(&ad714x->bus->dev, "Fail to detect >AD714x captouch,\ >> + read ID is %04x\n", data); > >line continuation in strings produces awful output > Fixed >> +static int ad714x_hw_init(struct ad714x_chip *ad714x) >> ... >> + return 0; >> ... >> + ad714x_hw_init(ad714x); > >considering you always return 0 and you never check the return, this >should be a void function > >and ad714x_hw_init should be __devinit Fixed. > >> + drv_data = kzalloc(sizeof(struct >ad714x_driver_data), GFP_KERNEL); > >sizeof(*drv_data) ... applies to every alloc in this driver > >> + input[alloc_idx-1]->id.bustype = BUS_I2C; > >you set bustype to I2C in common code even though this will be >executed for both SPI and I2C interfaces There is a BUS_I2C, but no BUS_SPI. So people may use other serial type to imply. Do you think we should add a macro in input.h? I am really suprised why the macro hasn't been added until now. > >> + if (ad714x->bus->irq > 0) { >> + ret = request_threaded_irq(ad714x->bus->irq, >ad714x_interrupt, >> + ad714x_interrupt_thread, >IRQF_TRIGGER_FALLING, >> + "ad714x_captouch", ad714x); >> + if (ret) { >> + dev_err(&ad714x->bus->dev, "Can't >allocate irq %d\n", >> + ad714x->bus->irq); >> + goto fail_irq; >> + } >> + } else >> + dev_warn(&ad714x->bus->dev, "IRQ not configured!\n"); >> + >> + return 0; > >since the driver requires an IRQ in order to work, this should error >out rather than warn and return success > >also, wouldnt it make more sense to request the IRQ first and then >fail rather than doing all the memory/input allocation only to find >out the IRQ is already taken ? > Ok >> +int ad714x_spi_read(struct spi_device *spi, unsigned short reg, >> + unsigned short *data) > >you're missing "static" here and in a few other functions Fixed > >> +{ >> + int ret; >> + unsigned short tx[2]; >> + unsigned short rx[2]; >> + struct spi_transfer t = { >> + .tx_buf = tx, >> + .rx_buf = rx, >> + .len = 4, >> + }; >> + struct spi_message m; >> + >> + tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) | >> + (AD714x_SPI_READ << AD714x_SPI_READ_SHFT) | reg; >> + >> + spi_message_init(&m); >> + spi_message_add_tail(&t, &m); >> + ret = spi_sync(spi, &m); > >cant you use spi_write_then_read() ? dont let the u8* prototype scare >you, it should work with writing 16bits and then reading 16bits. I have never been scared by any u8* or something else. I only prefer to use raw spi API, which can show the bottom level timing and SPI bus feature better. In fact, I prefer to use raw i2c API too. > >> +int ad714x_spi_write(struct spi_device *spi, unsigned short reg, >> + unsigned short data) >> +{ >> + int ret = 0; >> + unsigned short tx[2]; >> + struct spi_transfer t = { >> + .tx_buf = tx, >> + .len = 4, >> + }; >> + struct spi_message m; >> + >> + tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) | reg; >> + tx[1] = data; >> + >> + spi_message_init(&m); >> + spi_message_add_tail(&t, &m); >> + >> + ret = spi_sync(spi, &m); >> + if (ret < 0) >> + dev_err(&spi->dev, "SPI write error\n"); >> + >> + return ret; >> +} > >seems like spi_write() would be better > >> +static int __devexit ad714x_i2c_remove(struct i2c_client *client) >> +{ >> + struct ad714x_chip *chip = i2c_get_clientdata(client); >> + ad714x_remove(chip); >> + >> + return 0; >> +} > >return ad714x_remove(chip); Fixed. > >of course, if you simply created a macro "ad714x_get_chipdata" and had >it evaluate to either the spi or i2c version depending on the >interface enabled, this function could be in common code rather than >duplicating it for the two interfaces > In fact, dev_set_drvdata and dev_get_drvdata can be used in spi/i2c probe and remove. Then the probe/remove can be unified. But at the beginning, I prefer to keep this little redundancy, which maybe make the code more readable. Maybe I will use a unified probe/remove for SPI/I2C too, Let me think... >same goes for the module init/exit functions > >> +static struct spi_driver ad714x_spi_driver = { >> + .driver = { >> + .name = "ad714x_captouch", >> + .bus = &spi_bus_type, >> + .owner = THIS_MODULE, >> + }, > >i'm pretty sure you dont need to set .bus yourself Fixed. > >> + .probe = ad714x_spi_probe, >> + .remove = __devexit_p(ad714x_spi_remove), >> + .suspend = ad714x_suspend, >> + .resume = ad714x_resume, > >there is a new power management opts struct in 2.6.31 that >should be used Ok > >> +static int __init ad714x_init(void) >> +{ >> + int ret; >> + >> + ret = spi_register_driver(&ad714x_spi_driver); >> + if (ret != 0) { >> + printk(KERN_ERR "Failed to register ad714x >SPI driver: %d\n", >> + ret); >> + } >> + >> + return ret; > Ok >pr_err(), no need for the braces, and no need to output the ret value. > this is the module init function which means that error will be >passed back up to userspace and it can handle displaying it. in that >case, it might as well read: >{ > return spi_register_driver(&ad714x_spi_driver); >} Fixed > >> + u8 tx[4]; >> + >> + /* Do raw I2C, not smbus compatible */ >> + tx[0] = (reg & 0xFF00) >> 8; >> + tx[1] = (reg & 0x00FF); >> + tx[2] = (data & 0xFF00) >> 8; >> + tx[3] = data & 0x00FF; > >the masks are pretty useless (and i wouldnt be surprised if gcc doesnt >optimize all of them away) since you're already loading into a u8 >tx[4] = { > reg >> 8, > reg, > data >> 8, > data, >}; > >> + u8 tx[2]; >> + >> + /* Do raw I2C, not smbus compatible */ >> + tx[0] = (reg & 0xFF00) >> 8; >> + tx[1] = (reg & 0x00FF); > >same here > >> + *data = rx[0]; >> + *data = (*data << 8) | rx[1]; > >erm, this is funky. why cant you do it with one assignment ? > >> +MODULE_DESCRIPTION("ad714x captouch driver"); > >please use a description like you did in the Kconfig Fixed >-mike >_______________________________________________ >Uclinux-dist-devel mailing list >Uclinux-dist-devel@xxxxxxxxxxxxxxxxxxxx >https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
