Re: [PATCH 2/4] input: touchscreen: crtouch_ts: Add driver

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

 




Hi Anthony,

On Fri, Jun 24, 2016 at 03:44:44PM -0400, Anthony Felice wrote:
> Add driver for the Vybrid Tower CRTouch-based touchscreen. This is
> required for the touchscreen on the TWR-LCD-RGB to work on the Vybrid
> Tower platform.
> 
> There is a known issue with this driver: rarely, SW1 on the TWR-LCD-RGB
> module needs to be pressed in order for the touchscreen to begin
> functioning.

Hmm. Could it be that you want level interrupt and not edge? Or maybe
you want to read state after requesting interrupt?

> 
> Signed-off-by: Anthony Felice <tony.felice@xxxxxxxxxxx>
> ---
>  .../bindings/input/touchscreen/crtouch_ts.txt      |  14 ++
>  drivers/input/touchscreen/Kconfig                  |  10 +
>  drivers/input/touchscreen/Makefile                 |   1 +
>  drivers/input/touchscreen/crtouch_ts.c             | 279 +++++++++++++++++++++
>  4 files changed, 304 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/crtouch_ts.txt
>  create mode 100644 drivers/input/touchscreen/crtouch_ts.c
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/crtouch_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/crtouch_ts.txt
> new file mode 100644
> index 0000000..cfb966c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/crtouch_ts.txt
> @@ -0,0 +1,14 @@
> +* Freescale CRTOUCH based touchscreen
> +
> +Required Properties:
> +- compatible must be fsl,crtouch_ts
> +- reg: I2C address of the touchscreen
> +- irq-gpio: GPIO to use as event IRQ
> +
> +Example:
> +
> +	touch: crtouch@49 {
> +		compatible = "fsl,crtouch_ts";
> +		reg = <0x49>;
> +		irq-gpio = <&gpio0 21 GPIO_ACTIVE_HIGH>;

You are not using it as gpio but only as interrupt source, so please
set it up as regular interrupt and let I2C core set it up for you so
that you will only need to call request_irq(client->irq, ...)

		interrupts = <&gpio0 21 IRQ_TYPE_EDGE_FALLING>;

> +	};
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 8ecdc38..799e342 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1155,4 +1155,14 @@ config TOUCHSCREEN_ROHM_BU21023
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called bu21023_ts.
>  
> +config TOUCHSCREEN_CRTOUCH
> +	tristate "Freescale CRTOUCH based touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here if you have a CRTOUCH based touchscreen
> +	  controller.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called crtouch_ts.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index f42975e..8cb0a7a 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -95,3 +95,4 @@ obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50)	+= colibri-vf50-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)	+= rohm_bu21023.o
> +obj-$(CONFIG_TOUCHSCREEN_CRTOUCH)	+= crtouch_ts.o
> diff --git a/drivers/input/touchscreen/crtouch_ts.c b/drivers/input/touchscreen/crtouch_ts.c
> new file mode 100644
> index 0000000..bb87a8e
> --- /dev/null
> +++ b/drivers/input/touchscreen/crtouch_ts.c
> @@ -0,0 +1,279 @@
> +/*
> + * Driver for Freescale Semiconductor CRTOUCH - A Resistive and Capacitive
> + * touch device with i2c interface
> + *
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/slab.h>
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +
> +/* Resistive touch sense status registers */
> +#define RES_STA_ERROR			0x00
> +#define RES_STA_STATUS1			0x01
> +#define	RES_STA_STATUS2			0x02
> +#define RES_STA_X_MSB			0x03
> +#define RES_STA_X_LSB			0x04
> +#define RES_STA_Y_MSB			0x05
> +#define RES_STA_Y_LSB			0x06
> +#define RES_STA_PRES_MSB		0x07
> +#define RES_STA_RRES_LSB		0x08
> +#define RES_STA_FIFO_STATUS		0x09
> +#define RES_STA_FIFO_X_MSB		0x0a
> +#define RES_STA_FIFO_X_LSB		0x0b
> +#define RES_STA_FIFO_Y_MSB		0x0c
> +#define RES_STA_FIFO_Y_LSB		0x0d
> +#define RES_STA_FIFO_PRES_MSB		0x0e
> +#define RES_STA_FIFO_PRES_LSB		0x0f
> +#define RES_STA_UART_BRATE_MSB		0x10
> +#define RES_STA_UART_BRATE_MID		0x11
> +#define RES_STA_UART_BRATE_LSB		0x12
> +#define RES_STA_DEV_IDEN		0x13
> +#define RES_STA_SLIDE_DISPLACE		0x14
> +#define RES_STA_ROTATE_ANGLE		0x15
> +
> +/* Resistive touch configuration registers */
> +#define CR_CON_SYSTEM			0x40
> +#define CR_CON_TRIG_EVENT		0x41
> +#define CR_CON_FIFO_SETUP		0x42
> +#define CR_CON_SAMPLING_RATE		0x43
> +#define CR_CON_X_DELAY_MSB		0x44
> +#define CR_CON_X_DELAY_LSB		0x45
> +#define CR_CON_Y_DELAY_MSB		0x46
> +#define CR_CON_Y_DELAY_LSB		0x47
> +#define CR_CON_Z_DELAY_MSB		0x48
> +#define CR_CON_Z_DELAY_LSB		0x49
> +#define CR_CON_DIS_HOR_MSB		0x4a
> +#define CR_CON_DIS_HOR_LSB		0x4b
> +#define CR_CON_DIS_VER_MSB		0x4c
> +#define CR_CON_DIS_VER_LSB		0x4d
> +#define CR_CON_SLIDE_STEPS		0x4e
> +
> +#define RTST_EVENT			BIT(7)
> +#define RTS2T_EVENT			BIT(6)
> +#define RTSZ_EVENT			BIT(5)
> +#define RTSR_EVENT			BIT(4)
> +#define RTSS_EVENT			BIT(3)
> +#define RTSF_EVENT			BIT(2)
> +#define RTSRDY_EVENT			BIT(0)
> +
> +#define RTSSD_MASK			BIT(2)
> +#define RTSSD_H_NEG			BIT(2)
> +#define RTSSD_V_POS			BIT(3)
> +#define RTSSD_V_NEG			BIT(4)
> +
> +#define RTSRD_MASK			BIT(4)
> +#define RTSRD_COUNTER_CLK_WISE		BIT(4)
> +
> +#define RTSZD_MASK			BIT(5)
> +#define RTSZD_ZOOM_OUT			BIT(5)
> +
> +#define CRTOUCH_MAX_FINGER		2
> +#define CRTOUCH_MAX_AREA		0xfff
> +#define CRTOUCH_MAX_X			0x01df
> +#define CRTOUCH_MAX_Y			0x010f
> +
> +struct crtouch_ts_data {
> +	struct i2c_client	*client;
> +	struct input_dev	*input_dev;
> +};
> +
> +static u8 crtouch_read_reg(struct i2c_client *client, int addr)
> +{
> +	return i2c_smbus_read_byte_data(client, addr);
> +}
> +
> +static int crtouch_write_reg(struct i2c_client *client, int addr, int data)
> +{
> +	return i2c_smbus_write_byte_data(client, addr, data);
> +}
> +
> +static void calibration_pointer(u16 *x_orig, u16 *y_orig)
> +{
> +	u16 x, y;
> +
> +	x = CRTOUCH_MAX_X - *x_orig;
> +	*x_orig = x;
> +
> +	y = CRTOUCH_MAX_Y - *y_orig;
> +	*y_orig = y;
> +}
> +
> +static irqreturn_t crtouch_ts_interrupt(int irq, void *dev_id)
> +{
> +	struct crtouch_ts_data *data = dev_id;
> +	struct i2c_client *client = data->client;
> +	u8 status1;
> +	u16 valuep, valuex, valuey;
> +
> +	status1 = crtouch_read_reg(client, RES_STA_STATUS1);
> +
> +	/* For single touch */
> +	if (status1 & RTST_EVENT) {
> +		valuep = crtouch_read_reg(client, RES_STA_PRES_MSB);
> +		valuep = ((valuep << 8) |
> +			crtouch_read_reg(client, RES_STA_RRES_LSB));

Is it possible to read 2 bytes at once?

> +		valuex = crtouch_read_reg(client, RES_STA_X_MSB);
> +		valuex = ((valuex << 8) |
> +			crtouch_read_reg(client, RES_STA_X_LSB));
> +		valuey = crtouch_read_reg(client, RES_STA_Y_MSB);
> +		valuey = ((valuey << 8) |
> +			crtouch_read_reg(client, RES_STA_Y_LSB));
> +		calibration_pointer(&valuex, &valuey);

I'd do conversion inline - it is clearer what is going on.

> +		input_report_key(data->input_dev, BTN_TOUCH, 1);
> +		input_report_abs(data->input_dev, ABS_X, valuex);
> +		input_report_abs(data->input_dev, ABS_Y, valuey);
> +		input_report_abs(data->input_dev, ABS_PRESSURE, valuep);
> +		input_sync(data->input_dev);
> +	} else {
> +		input_report_abs(data->input_dev, ABS_PRESSURE, 0);
> +		input_event(data->input_dev, EV_KEY, BTN_TOUCH, 0);
> +		input_sync(data->input_dev);
> +	}

input_sync() can go here.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void crtouch_ts_reg_init(struct crtouch_ts_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +
> +	crtouch_write_reg(client, CR_CON_SYSTEM, 0x9c);
> +	crtouch_write_reg(client, CR_CON_TRIG_EVENT, 0xf9);
> +	crtouch_write_reg(client, CR_CON_FIFO_SETUP, 0x1f);
> +	crtouch_write_reg(client, CR_CON_SAMPLING_RATE, 0x08);
> +	crtouch_write_reg(client, CR_CON_DIS_HOR_MSB, 0x01);
> +	crtouch_write_reg(client, CR_CON_DIS_HOR_LSB, 0xdf);
> +	crtouch_write_reg(client, CR_CON_DIS_VER_MSB, 0x01);
> +	crtouch_write_reg(client, CR_CON_DIS_VER_LSB, 0x0f);

Should we fetch any of this data from DTS? I assume CR_CON_DIS_HOR/VER
is resolution? Or not?

Also, why no error handling?

> +}
> +
> +static int crtouch_ts_probe(struct i2c_client *client,
> +			    const struct i2c_device_id *id)
> +{
> +	struct crtouch_ts_data *data;
> +	struct input_dev *input_dev;
> +	int error = -1, irq_gpio;
> +
> +	dev_info(&client->dev, "Freescale CRTOUCH driver\n");

Please drop this.

> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	input_dev = input_allocate_device();

Please use devm* and you will be able to get rid of crtouch_ts_remove().

> +	if (!data || !input_dev) {
> +		dev_err(&client->dev, "Failed to allocate memory\n");
> +		error = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +
> +	data->client = client;
> +	data->input_dev = input_dev;
> +
> +	input_dev->name = "crtouch_ts";
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->dev.parent = &client->dev;
> +
> +	/* For single touch */
> +	__set_bit(EV_KEY, input_dev->evbit);
> +	__set_bit(BTN_TOUCH, input_dev->keybit);
> +	__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, 0, CRTOUCH_MAX_X, 0, 0);
> +	input_set_abs_params(input_dev, ABS_Y, 0, CRTOUCH_MAX_Y, 0, 0);
> +	input_set_abs_params(input_dev, ABS_PRESSURE, 0,
> +			     CRTOUCH_MAX_AREA, 0, 0);
> +
> +	input_set_drvdata(input_dev, data);
> +
> +	crtouch_ts_reg_init(data);
> +
> +	irq_gpio = of_get_named_gpio(client->dev.of_node, "irq-gpio", 0);
> +	if (!gpio_is_valid(irq_gpio))
> +		goto err_free_mem;
> +
> +	error = gpio_request_one(irq_gpio, GPIOF_IN, "TS_IRQ");
> +	if (error) {
> +		dev_err(&client->dev, "Failed to request gpio\n");
> +		goto err_free_mem;
> +	}
> +
> +	error = request_threaded_irq(gpio_to_irq(irq_gpio), NULL,
> +				     crtouch_ts_interrupt,
> +				     IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> +				     "crtouch_ts", data);

As i mentioned above replace everything starting with of_get_named_gpio
with:

	error = devm_request_threaded_irq(&cleint->dev, client->irq,
					  NULL, crtouch_ts_interrupt,
					  IRQF_ONESHOT,
					  "crtouch_ts", data);

> +
> +	if (error) {
> +		dev_err(&client->dev, "Failed to register interrupt\n");
> +		goto err_free_mem;
> +	}
> +
> +	error = input_register_device(data->input_dev);
> +	if (error)
> +		goto err_free_irq;
> +
> +	i2c_set_clientdata(client, data);
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(client->irq, data);
> +err_free_mem:
> +	input_free_device(input_dev);
> +	kfree(data);
> +	return error;
> +}
> +
> +static int crtouch_ts_remove(struct i2c_client *client)
> +{
> +	struct crtouch_ts_data *data = i2c_get_clientdata(client);
> +
> +	free_irq(client->irq, data);
> +	input_unregister_device(data->input_dev);
> +	kfree(data);

With devm it can all be dropped.

> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id crtouch_ts_id[] = {
> +	{"crtouch_ts", 0},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, crtouch_ts_id);
> +
> +static const struct of_device_id crtouch_ts_dt_ids[] = {
> +	{ .compatible = "fsl,crtouch_ts", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, crtouch_ts_dt_ids);
> +
> +static struct i2c_driver crtouch_ts_i2c_driver = {
> +	.driver = {
> +		   .name = "crtouch_ts",
> +		   .owner = THIS_MODULE,

No need to set owner.

> +		   .of_match_table = crtouch_ts_dt_ids,

			.of_match_table = of_match_ptr(crtouch_ts_dt_ids),

Also, no PM methods?

> +		   },
> +	.probe = crtouch_ts_probe,
> +	.remove = crtouch_ts_remove,
> +	.id_table = crtouch_ts_id,
> +};
> +
> +module_i2c_driver(crtouch_ts_i2c_driver);
> +
> +MODULE_AUTHOR("Alison Wang <b18965@xxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Touchscreen driver for Freescale CRTOUCH");
> +MODULE_LICENSE("GPL");
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux