Re: [PATCH] input: add driver for EETI touchpanels [v3]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Daniel,

Looks pretty good, a couple of things below. On the next iteration
please CC Jean Delvare for the I2C bits.

On Fri, May 15, 2009 at 11:41:11AM +0200, Daniel Mack wrote:
> +
> +	mutex_lock(&priv->mutex);
> +
> +	while (!gpio_get_value(irq_to_gpio(priv->irq)) && --to)
> +		i2c_master_recv(priv->client, buf, sizeof(buf));
> +
> +	if (!to) {
> +		dev_err(&priv->client->dev,
> +			"unable to clear IRQ - line stuck?\n");
> +		mutex_lock(&priv->mutex);

Tsk, tsk... Please just use standard "goto out;" schema and unlock the
mutex there.

> +		return;
> +	}
> +
> +	/* drop non-report packets */
> +	if (!(buf[0] & 0x80)) {
> +		mutex_unlock(&priv->mutex);
> +		return;
> +	}
> +
> +	pressed = buf[0] & REPORT_BIT_PRESSED;
> +	res = REPORT_RES_BITS(buf[0] & (REPORT_BIT_AD0 | REPORT_BIT_AD1));
> +	x = buf[2] | (buf[1] << 8);
> +	y = buf[4] | (buf[3] << 8);
> +
> +	/* fix the range to 11 bits */
> +	x >>= res - EETI_TS_BITDEPTH;
> +	y >>= res - EETI_TS_BITDEPTH;
> +
> +	if (flip_x)
> +		x = EETI_MAXVAL - x;
> +
> +	if (flip_y)
> +		y = EETI_MAXVAL - y;
> +
> +	if ((buf[0] & REPORT_BIT_HAS_PRESSURE) && !pressed)
> +		input_report_abs(priv->input, ABS_PRESSURE, buf[5]);

Please report the pressure always (if you have it of course), not only
at the beginning of the touch. This will allow userspace control
sensitivity.

> +
> +	input_report_abs(priv->input, ABS_X, x);
> +	input_report_abs(priv->input, ABS_Y, y);
> +	input_report_key(priv->input, BTN_TOUCH, !!pressed);
> +	input_sync(priv->input);
> +	mutex_unlock(&priv->mutex);
> +}
> +
> +static irqreturn_t eeti_ts_isr(int irq, void *dev_id)
> +{
> +	struct eeti_ts_priv *priv = dev_id;
> +
> +	 /* postpone I2C transactions as we are atomic */
> +	schedule_delayed_work(&priv->work, HZ / 100);

Why delayed work? I'd think you want to schedule it immediately.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int eeti_ts_open(struct input_dev *dev)
> +{
> +	struct eeti_ts_priv *priv = input_get_drvdata(dev);
> +
> +	enable_irq(priv->irq);
> +
> +	/* Read the events once to arm the IRQ */
> +	eeti_ts_read(&priv->work.work);
> +
> +	return 0;
> +}
> +
> +static void eeti_ts_close(struct input_dev *dev)
> +{
> +	struct eeti_ts_priv *priv = input_get_drvdata(dev);
> +
> +	disable_irq(priv->irq);
> +	cancel_delayed_work_sync(&priv->work);
> +}
> +
> +static int eeti_ts_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *idp)

__devinit?

> +{
> +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +	struct eeti_ts_priv *priv;
> +	struct input_dev *input;
> +	int err = -ENOMEM;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> +		return -EIO;
> +
> +	/* In contrast to what's described in the datasheet, there seems
> +	 * to be no way of probing the presence of that device using I2C
> +	 * commands. So we need to blindly believe it is there, and wait
> +	 * for interrupts to occur. */
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		dev_err(&client->dev, "failed to allocate driver data\n");
> +		goto err0;
> +	}
> +
> +	mutex_init(&priv->mutex);
> +	input = input_allocate_device();
> +
> +	if (!input) {
> +		dev_err(&client->dev, "Failed to allocate input device.\n");
> +		goto err1;
> +	}
> +
> +	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +
> +	input_set_abs_params(input, ABS_X, 0, EETI_MAXVAL, 0, 0);
> +	input_set_abs_params(input, ABS_Y, 0, EETI_MAXVAL, 0, 0);
> +	input_set_abs_params(input, ABS_PRESSURE, 0, 0xff, 0, 0);
> +
> +	input->name = client->name;
> +	input->id.bustype = BUS_I2C;
> +	input->dev.parent = &client->dev;
> +	input->open = eeti_ts_open;
> +	input->close = eeti_ts_close;
> +
> +	input_set_drvdata(input, priv);
> +
> +	priv->client = client;
> +	priv->input = input;
> +	priv->irq = client->irq;
> +	dev_set_drvdata(&client->dev, priv);
> +	INIT_DELAYED_WORK(&priv->work, eeti_ts_read);
> +
> +	err = input_register_device(input);
> +	if (err)
> +		goto err1;
> +
> +	err = request_irq(priv->irq, eeti_ts_isr, IRQF_TRIGGER_FALLING,
> +			  client->name, priv);
> +	if (err) {
> +		dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
> +		goto err2;
> +	}
> +
> +	/* Disable the irq for now. It will be enabled once the input device
> +	 * is opened. */
> +	disable_irq(priv->irq);
> +
> +	device_init_wakeup(&client->dev, 0);
> +	return 0;
> +
> +err2:
> +	input_unregister_device(input);
> +	input = NULL; /* so we dont try to free it below */
> +err1:
> +	input_free_device(input);
> +	kfree(priv);
> +err0:
> +	dev_set_drvdata(&client->dev, NULL);
> +	return err;
> +}
> +
> +static int eeti_ts_remove(struct i2c_client *client)

__devexit?

> +{
> +	struct eeti_ts_priv *priv = dev_get_drvdata(&client->dev);
> +
> +	free_irq(priv->irq, priv);
> +	input_unregister_device(priv->input);
> +	dev_set_drvdata(&client->dev, NULL);
> +	kfree(priv);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int eeti_ts_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	struct eeti_ts_priv *priv = dev_get_drvdata(&client->dev);
> +
> +	if (device_may_wakeup(&client->dev))
> +		enable_irq_wake(priv->irq);
> +
> +	return 0;
> +}
> +
> +static int eeti_ts_resume(struct i2c_client *client)
> +{
> +	struct eeti_ts_priv *priv = dev_get_drvdata(&client->dev);
> +
> +	if (device_may_wakeup(&client->dev))
> +		disable_irq_wake(priv->irq);
> +
> +	return 0;
> +}
> +#else
> +#define eeti_ts_suspend NULL
> +#define eeti_ts_resume NULL
> +#endif
> +
> +static const struct i2c_device_id eeti_ts_id[] = {
> +	{ "eeti_ts", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, eeti_ts_id);
> +
> +static struct i2c_driver eeti_ts_driver = {
> +	.driver = {
> +		.name = "eeti_ts",
> +	},
> +	.probe = eeti_ts_probe,
> +	.remove = eeti_ts_remove,

__devexit_p?

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

[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux