>-----Original Message-----
>From: [email protected]
>[mailto:[email protected]] On
>Behalf Of Mike Frysinger
>Sent: Wednesday, September 02, 2009 3:31 AM
>To: Barry Song
>Cc: [email protected]; [email protected];
>[email protected];
>[email protected];
>[email protected]; [email protected]
>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
>[email protected]
>https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
>
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
_______________________________________________
spi-devel-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
