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
> +#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
> +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
> +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.
> +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.
> +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 ...
> +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.
> +/* 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
> +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
> +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
> + 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
> + 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 ?
> +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
> +{
> + 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.
> +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);
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
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
> + .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
> +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;
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);
}
> + 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
-mike
--
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
