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

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

 



Hi Dmitry, Stefan,

Thanks for your valuable feedback, I really appreciate it. I will review and submit V2 patchset based on this. Please see a few comments in-line.

-Tony


On Fri, Jun 24, 2016 at 6:04 PM, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
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?

I dug up an old email thread about this - According to this, the reason for the failure is the crtouch chip is configured with the CRTSLEEP bit initially set.  This means the chip goes into sleep mode between touchscreen scans, and if the host tries to access it during this time it will start to wake up, but will NAK until it is initialized.   The hardware solution is to tie the WAKEUP pin low on the crtouch chip; otherwise, the driver needs to run a loop to clear the CRTSLEEP bit in the crtouch configuration register to prevent the device from sleeping. I will investigate further, if this is the solution I will include as part of V2 patchset.

>
> 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 = ""> > +     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?

Yes, the HOR/VER info can at least be fetched from DTS. I will fix as part of V2 patchset.
 

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 = "" 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 = ""> > +
> +     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?


Well, the main reason for no PM methods is the original driver did not have PM methods. I can implement sleep/resume methods, and if all is working well will submit as part of V2.
 
> +                },
> +     .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



--
Tony Felice
Support Engineering Manager
Timesys Corporation
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux