Re: [PATCH v3] input: qt602240 - Add ATMEL QT602240 touchscreen driver

[Joonyoung Shim <jy0922.shim@samsung.com>]

On 6/29/2010 2:55 AM, Dmitry Torokhov wrote:
> On Mon, Jun 28, 2010 at 08:38:11PM +0900, Joonyoung Shim wrote:
>> +
>> +static int qt602240_load_fw(struct device *dev, const char *fn)
>> +{
>> +	struct qt602240_data *data = dev_get_drvdata(dev);
>> +	struct i2c_client *client = data->client;
>> +	const struct firmware *fw = NULL;
>> +	unsigned int frame_size;
>> +	unsigned int pos = 0;
>> +	int ret;
>> +
>> +	if (!data) {
>> +		dev_err(dev, "The data is NULL\n");
>> +		return -EFAULT;
>> +	}
>> +
>> +	ret = request_firmware(&fw, fn, dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Unable to open firmware %s\n", fn);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* Change to the bootloader mode */
>> +	qt602240_write_object(data, QT602240_GEN_COMMAND,
>> +			QT602240_COMMAND_RESET, QT602240_BOOT_VALUE);
>> +	msleep(QT602240_RESET_TIME);
>> +
>> +	/* Change to slave address of bootloader */
>> +	if (client->addr == QT602240_APP_LOW)
>> +		client->addr = QT602240_BOOT_LOW;
>> +	else
>> +		client->addr = QT602240_BOOT_HIGH;
>> +
>> +	ret = qt602240_check_bootloader(client, QT602240_WAITING_BOOTLOAD_CMD);
>> +	if (ret < 0)
>> +		goto err_fw;
>> +
>> +	/* Unlock bootloader */
>> +	qt602240_unlock_bootloader(client);
>> +
>> +	while (pos < fw->size) {
>> +		ret = qt602240_check_bootloader(client,
>> +				QT602240_WAITING_FRAME_DATA);
>> +		if (ret < 0)
>> +			goto err_fw;
>> +
>> +		frame_size = ((*(fw->data + pos) << 8) | *(fw->data + pos + 1));
>> +
>> +		/* We should add 2 at frame size as the the firmware data is not
>> +		 * included the CRC bytes.
>> +		 */
>> +		frame_size += 2;
>> +
>> +		/* Write one frame to device */
>> +		qt602240_fw_write(client, fw->data + pos, frame_size);
>> +
>> +		ret = qt602240_check_bootloader(client,
>> +				QT602240_FRAME_CRC_PASS);
>> +		if (ret < 0)
>> +			goto err_fw;
>> +
>> +		pos += frame_size;
>> +
>> +		dev_dbg(dev, "Updated %zd bytes / %zd bytes\n", pos, fw->size);
>> +	}
>> +
>> +err_fw:
>> +	/* Change to slave address of application */
>> +	if (client->addr == QT602240_BOOT_LOW)
>> +		client->addr = QT602240_APP_LOW;
>> +	else
>> +		client->addr = QT602240_APP_HIGH;
> 
> You are missing call to release_firmware() and thus leaking memory.
> 

Yep, i will add it.

>> +
>> +	return ret;
>> +}
>> +
>> +static ssize_t qt602240_update_fw_store(struct device *dev,
>> +		struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> +	struct qt602240_data *data = dev_get_drvdata(dev);
>> +	unsigned int version;
>> +	int ret;
>> +
>> +	if (sscanf(buf, "%u", &version) != 1) {
>> +		dev_err(dev, "Invalid values\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Firmware update supports from version 21 */
>> +	if ((data->info.version < QT602240_VER_21) ||
>> +			(version < QT602240_VER_21)) {
>> +		dev_err(dev, "Firmware update supports from version 21\n");
> 
> "Firmware updates suppored starting with version 21"
> 

Will fix.

>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Interrupt disable */
>> +	disable_irq(data->irq);
>> +
>> +	ret = qt602240_load_fw(dev, QT602240_FW_NAME);
>> +	if (ret < 0)
>> +		dev_err(dev, "The firmware update failed(%d)\n", ret);
>> +	else {
>> +		dev_dbg(dev, "The firmware update successed\n");
> 
> "succeeded"
>

Will fix.
 
>> +
>> +		/* Wait for reset */
>> +		msleep(QT602240_FWRESET_TIME);
>> +
>> +		kfree(data->object_table);
>> +
>> +		qt602240_initialize(data);
>> +	}
>> +
>> +	/* Interrupt enable */
>> +	enable_irq(data->irq);
>> +
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR(object, 0444, qt602240_object_show, NULL);
>> +static DEVICE_ATTR(update_fw, 0664, NULL, qt602240_update_fw_store);
>> +
>> +static struct attribute *qt602240_attrs[] = {
>> +	&dev_attr_object.attr,
>> +	&dev_attr_update_fw.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group qt602240_attr_group = {
>> +	.attrs = qt602240_attrs,
>> +};
>> +
>> +static int qt602240_input_open(struct input_dev *dev)
>> +{
>> +	struct qt602240_data *data = input_get_drvdata(dev);
>> +
>> +	/* Touch enable */
>> +	qt602240_write_object(data, QT602240_TOUCH_MULTI, QT602240_TOUCH_CTRL,
>> +			0x83);
> 
> Do you want to do this if device is suspended?
> 

Yes, is it better to make and reuse start and stop functions?

>> +
>> +	return 0;
>> +}
>> +
>> +static void qt602240_input_close(struct input_dev *dev)
>> +{
>> +	struct qt602240_data *data = input_get_drvdata(dev);
>> +
>> +	/* Touch disable */
>> +	qt602240_write_object(data, QT602240_TOUCH_MULTI, QT602240_TOUCH_CTRL,
>> +			0);
> 
> Same here.
> 
>> +}
>> +
>> +static int __devinit qt602240_probe(struct i2c_client *client,
>> +		const struct i2c_device_id *id)
>> +{
>> +	struct qt602240_data *data;
>> +	struct input_dev *input_dev;
>> +	int ret;
>> +
>> +	if (!client->dev.platform_data)
>> +		return -EINVAL;
>> +
>> +	data = kzalloc(sizeof(struct qt602240_data), GFP_KERNEL);
>> +
>> +	input_dev = input_allocate_device();
>> +	if (!data || !input_dev) {
>> +		dev_err(&client->dev, "Failed to allocate memory\n");
>> +		ret = -ENOMEM;
>> +		goto err_free_mem;
>> +	}
>> +
>> +	input_dev->name = "AT42QT602240/ATMXT224 Touchscreen";
>> +	input_dev->id.bustype = BUS_I2C;
>> +	input_dev->dev.parent = &client->dev;
>> +	input_dev->open = qt602240_input_open;
>> +	input_dev->close = qt602240_input_close;
>> +
>> +	__set_bit(EV_ABS, input_dev->evbit);
>> +	__set_bit(EV_KEY, input_dev->evbit);
>> +	__set_bit(BTN_TOUCH, input_dev->keybit);
>> +
>> +	/* For single touch */
>> +	input_set_abs_params(input_dev, ABS_X, 0, QT602240_MAX_XC, 0,
>> +			0);
>> +	input_set_abs_params(input_dev, ABS_Y, 0, QT602240_MAX_YC, 0,
>> +			0);
>> +
>> +	/* For multi touch */
>> +	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
>> +			QT602240_MAX_AREA, 0, 0);
>> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0,
>> +			QT602240_MAX_XC, 0, 0);
>> +	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0,
>> +			QT602240_MAX_YC, 0, 0);
>> +
>> +	input_set_drvdata(input_dev, data);
>> +
>> +	INIT_WORK(&data->ts_resume_work, qt602240_resume_worker);
>> +	data->client = client;
>> +	data->input_dev = input_dev;
>> +	data->pdata = client->dev.platform_data;
>> +	data->irq = client->irq;
>> +
>> +	i2c_set_clientdata(client, data);
>> +
>> +	ret = qt602240_initialize(data);
>> +	if (ret < 0)
>> +		goto err_free_object;
>> +
>> +	ret = request_threaded_irq(client->irq, NULL, qt602240_interrupt,
>> +			IRQF_TRIGGER_FALLING, client->dev.driver->name, data);
>> +
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "Failed to register interrupt\n");
>> +		goto err_free_object;
>> +	}
>> +
>> +	ret = input_register_device(input_dev);
>> +	if (ret < 0)
>> +		goto err_free_irq;
>> +
>> +	ret = sysfs_create_group(&client->dev.kobj, &qt602240_attr_group);
>> +	if (ret)
>> +		goto err_unregister_device;
>> +
>> +	return 0;
>> +
>> +err_unregister_device:
>> +	input_unregister_device(input_dev);
>> +err_free_irq:
>> +	free_irq(client->irq, data);
>> +err_free_object:
>> +	kfree(data->object_table);
>> +err_free_mem:
>> +	input_free_device(input_dev);
>> +	kfree(data);
>> +	return ret;
>> +}
>> +
>> +static int __devexit qt602240_remove(struct i2c_client *client)
>> +{
>> +	struct qt602240_data *data = i2c_get_clientdata(client);
>> +
>> +	free_irq(data->irq, data);
>> +	cancel_work_sync(&data->ts_resume_work);
>> +	input_unregister_device(data->input_dev);
>> +	kfree(data->object_table);
>> +	kfree(data);
>> +
>> +	sysfs_remove_group(&client->dev.kobj, &qt602240_attr_group);
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int qt602240_suspend(struct i2c_client *client, pm_message_t mesg)
>> +{
>> +	struct qt602240_data *data = i2c_get_clientdata(client);
>> +	struct input_dev *input_dev = data->input_dev;
>> +
>> +	mutex_lock(&input_dev->mutex);
>> +
>> +	if (input_dev->users)
>> +		/* Touch disable */
>> +		qt602240_write_object(data, QT602240_TOUCH_MULTI,
>> +				QT602240_TOUCH_CTRL, 0);
>> +
>> +	mutex_unlock(&input_dev->mutex);
>> +
>> +	return 0;
>> +}
>> +
>> +static int qt602240_resume(struct i2c_client *client)
>> +{
>> +	struct qt602240_data *data = i2c_get_clientdata(client);
>> +
>> +	schedule_work(&data->ts_resume_work);
> 
> Like I said, I am concerned with pulling resume work out of line. The
> rest of the system might start accessing device that is not ready yet
> but is already marked as resumed. I'd rather did that short sleep right
> here in resume function.
> 

OK, resume work maybe cannot guarantee the device ready state 
immediately after resume. The clear way than uncertain would be better.

> Also, please CC Jean Delvare to make sure I2C bits look good.
> 

I add him to CC.

Thanks.