Hi, On Mon, May 11, 2009 at 09:28:30PM +0900, Joonyoung Shim wrote: > The MELPAS MCS-5000 is the touchscreen controller. The overview of this > controller can see at the following website: > > http://www.melfas.com/product/product01.asp?k_r=eng_ > > This patch supports only the i2c interface. > Thank you for the patch. Some comments below. > + > +/* Touchscreen absolute values */ > +static int abs_x[3] = {0, 1024, 0}; > +module_param_array(abs_x, int, NULL, 0); > +MODULE_PARM_DESC(abs_x, "Touchscreen absolute X min, max, fuzz"); > + > +static int abs_y[3] = {0, 1024, 0}; > +module_param_array(abs_y, int, NULL, 0); > +MODULE_PARM_DESC(abs_y, "Touchscreen absolute Y min, max, fuzz"); > + Do we need to have these as module parameters give the fact that we can set them dynamically with EVIOCSABS? I'd =say they should go. > +static int abs_p[3] = {0, 256, 0}; > +module_param_array(abs_p, int, NULL, 0); > +MODULE_PARM_DESC(abs_p, "Touchscreen absolute Pressure min, max, fuzz"); > + > +/* Each client has this additional data */ > +struct mcs5000_ts_data { > + struct i2c_client *client; > + struct input_dev *input_dev; > + struct work_struct ts_event_work; > + struct mcs5000_ts_platform_data *platform_data; > + > + unsigned int irq; > +}; > + > +static struct i2c_driver mcs5000_ts_driver; > + > +static void mcs5000_ts_input_read(struct mcs5000_ts_data *data) > +{ > + struct i2c_client *client = data->client; > + u8 buffer[READ_BLOCK_SIZE]; > + int err; > + int x; > + int y; > + > + err = i2c_smbus_read_i2c_block_data(client, MCS5000_TS_INPUT_INFO, > + READ_BLOCK_SIZE, buffer); > + if (err < 0) { > + dev_err(&client->dev, "%s, err[%d]\n", __func__, err); > + return; > + } > + > + switch (buffer[READ_INPUT_INFO]) { > + case INPUT_TYPE_NONTOUCH: > + input_report_abs(data->input_dev, ABS_PRESSURE, 0); > + input_report_key(data->input_dev, BTN_TOUCH, 0); > + input_sync(data->input_dev); > + break; > + case INPUT_TYPE_SINGLE: > + x = (buffer[READ_X_POS_UPPER] << 8) | buffer[READ_X_POS_LOWER]; > + y = (buffer[READ_Y_POS_UPPER] << 8) | buffer[READ_Y_POS_LOWER]; > + > + input_report_key(data->input_dev, BTN_TOUCH, 1); > + input_report_abs(data->input_dev, ABS_X, x); > + input_report_abs(data->input_dev, ABS_Y, y); > + input_report_abs(data->input_dev, ABS_PRESSURE, > + DEFAULT_PRESSURE); If the hardware does not support pressure reading don't fake it. BTN_TOUCH should be enough to signal touch. > + input_sync(data->input_dev); > + break; > + case INPUT_TYPE_DUAL: > + /* TODO */ > + break; > + case INPUT_TYPE_PALM: > + /* TODO */ > + break; > + case INPUT_TYPE_PROXIMITY: > + /* TODO */ > + break; > + default: > + dev_err(&client->dev, "Unknown ts input type %d\n", > + buffer[READ_INPUT_INFO]); > + break; > + } > +} > + > +static void mcs5000_ts_irq_worker(struct work_struct *work) > +{ > + struct mcs5000_ts_data *data = container_of(work, > + struct mcs5000_ts_data, ts_event_work); > + > + mcs5000_ts_input_read(data); > + > + enable_irq(data->irq); > +} > + > +static irqreturn_t mcs5000_ts_interrupt(int irq, void *dev_id) > +{ > + struct mcs5000_ts_data *data = dev_id; > + > + if (!work_pending(&data->ts_event_work)) { > + disable_irq_nosync(data->irq); > + schedule_work(&data->ts_event_work); > + } > + > + return IRQ_HANDLED; > +} > + > +static int mcs5000_ts_input_init(struct mcs5000_ts_data *data) > +{ > + struct input_dev *input_dev; > + int ret = 0; > + > + INIT_WORK(&data->ts_event_work, mcs5000_ts_irq_worker); > + > + data->input_dev = input_allocate_device(); > + if (data->input_dev == NULL) { > + ret = -ENOMEM; > + goto err_input; > + } > + > + input_dev = data->input_dev; > + input_dev->name = "MELPAS MCS-5000 Touchscreen"; input_dev->id.bustype = BUS_I2C; input_dev->dev.parent = <i2c device>; > + set_bit(EV_ABS, input_dev->evbit); > + set_bit(ABS_X, input_dev->absbit); > + set_bit(ABS_Y, input_dev->absbit); > + set_bit(ABS_PRESSURE, input_dev->absbit); > + input_set_abs_params(input_dev, ABS_X, abs_x[0], abs_x[1], abs_x[2], 0); > + input_set_abs_params(input_dev, ABS_Y, abs_y[0], abs_y[1], abs_y[2], 0); > + input_set_abs_params(input_dev, ABS_PRESSURE, abs_p[0], abs_p[1], > + abs_p[2], 0); This line should go. > + set_bit(EV_KEY, input_dev->evbit); > + set_bit(BTN_TOUCH, input_dev->keybit); > + > + ret = input_register_device(data->input_dev); > + if (ret < 0) > + goto err_register; > + > + if (request_irq(data->irq, mcs5000_ts_interrupt, > + IRQF_TRIGGER_LOW, "mcs5000_ts_input", data)) { > + dev_err(&data->client->dev, "Failed to register interrupt\n"); > + ret = -EINVAL; Why EINVAL and not EBUSY? Or better yet, why don't you propagate what reqiest_irq returned? > + goto err_irq; > + } > + > + input_set_drvdata(input_dev, data); > + > + return 0; > +err_irq: > + input_unregister_device(data->input_dev); > +err_register: > + input_free_device(data->input_dev); Do not call input_free_device() after input_unregister_device(), it will cause double-free. EIther rearrange initialization order or do data->input_dev = NULL; right after calling input_unregister_device(). > +err_input: > + return ret; > +} > + > +static void mcs5000_ts_phys_init(struct mcs5000_ts_data *data) > +{ > + struct i2c_client *client = data->client; > + struct mcs5000_ts_platform_data *platform_data = data->platform_data; > + > + /* Touch reset & sleep mode */ > + i2c_smbus_write_byte_data(client, MCS5000_TS_OP_MODE, > + RESET_EXT_SOFT | OP_MODE_SLEEP); > + > + /* Touch size */ > + i2c_smbus_write_byte_data(client, MCS5000_TS_X_SIZE_UPPER, > + platform_data->x_size >> 8); > + i2c_smbus_write_byte_data(client, MCS5000_TS_X_SIZE_LOWER, > + platform_data->x_size & 0xff); > + i2c_smbus_write_byte_data(client, MCS5000_TS_Y_SIZE_UPPER, > + platform_data->y_size >> 8); > + i2c_smbus_write_byte_data(client, MCS5000_TS_Y_SIZE_LOWER, > + platform_data->y_size & 0xff); > + > + /* Touch active mode & 80 report rate */ > + i2c_smbus_write_byte_data(data->client, MCS5000_TS_OP_MODE, > + OP_MODE_ACTIVE | REPORT_RATE_80); > +} > + > +static int mcs5000_ts_probe(struct i2c_client *client, > + const struct i2c_device_id *idp) This should be marked __devinit. > +{ > + struct mcs5000_ts_data *data; > + int ret; > + > + data = kzalloc(sizeof(struct mcs5000_ts_data), GFP_KERNEL); > + if (!data) > + dev_err(&client->dev, "Failed to allocate driver data\n"); > + ret = -ENOMEM; > + goto exit; > + } > + > + data->client = client; > + data->irq = client->irq; > + data->platform_data = client->dev.platform_data; > + > + i2c_set_clientdata(client, data); > + > + if (data->platform_data->set_pin) > + data->platform_data->set_pin(); > + > + ret = mcs5000_ts_input_init(data); > + if (ret) > + goto exit_free; > + > + mcs5000_ts_phys_init(data); > + > + return 0; > + > +exit_free: > + kfree(data); > + i2c_set_clientdata(client, NULL); > +exit: > + return ret; > +} > + > +static int mcs5000_ts_remove(struct i2c_client *client) This should be marked __devexit. > +{ > + struct mcs5000_ts_data *data = i2c_get_clientdata(client); > + > + cancel_work_sync(&data->ts_event_work); There is a race here, IRQ handler may resubmit work after cancel_work_sync() returns. You need to make sure that IRQ handler does not resubmit work while device is being shut down. > + free_irq(data->irq, data); > + input_unregister_device(data->input_dev); > + input_free_device(data->input_dev); Just input_unregister_device() is enough. > + kfree(data); > + > + i2c_set_clientdata(client, NULL); > + > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int mcs5000_ts_suspend(struct i2c_client *client, pm_message_t mesg) > +{ > + /* Touch sleep mode */ > + i2c_smbus_write_byte_data(client, MCS5000_TS_OP_MODE, OP_MODE_SLEEP); > + > + return 0; > +} > + > +static int mcs5000_ts_resume(struct i2c_client *client) > +{ > + struct mcs5000_ts_data *data; > + > + data = i2c_get_clientdata(client); > + mcs5000_ts_phys_init(data); > + > + return 0; > +} > +#else > +#define mcs5000_ts_suspend NULL > +#define mcs5000_ts_resume NULL > +#endif > + > +static const struct i2c_device_id mcs5000_ts_id[] = { > + { "mcs5000_ts", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, mcs5000_ts_id); > + > +static struct i2c_driver mcs5000_ts_driver = { > + .driver = { > + .name = "mcs5000_ts", > + }, > + .probe = mcs5000_ts_probe, > + .remove = mcs5000_ts_remove, __devexit_p() #ifdef CONFIG_PM > + .suspend = mcs5000_ts_suspend, > + .resume = mcs5000_ts_resume, #endif > + .id_table = mcs5000_ts_id, > +}; > + Thanks. -- Dmitry -- 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
