Re: [PATCH v12 2/5] Input: goodix - add support for ESD

From: Bastien Nocera
Date: Thu Oct 27 2016 - 10:19:43 EST

Next message: Bastien Nocera: "Re: [PATCH v12 4/5] Input: goodix - fix reset sequence"
Previous message: Rafael J. Wysocki: "Re: [PATCH v6 00/16] ACPI IORT ARM SMMU support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote:
> Add ESD (Electrostatic Discharge) protection mechanism.
>
> The driver enables ESD protection in HW and checks a register
> to determine if ESD occurred. If ESD is signalled by the HW,
> the driver will reset the device.
>
> The ESD poll time (in ms) can be set through the sysfs property
> esd_timeout. If it is set to 0, ESD protection is disabled.
> Recommended value is 2000 ms.

What's the default value though?

> The initial value for ESD timeout
> can be set through esd-recovery-timeout-ms ACPI/DT property.
> If there is no such property defined, ESD protection is disabled.
> For ACPI 5.1, the property can be specified using _DSD properties:
> ÂDevice (STAC)
> Â{
> ÂÂÂÂÂName (_HID, "GDIX1001")
> ÂÂÂÂÂ...
>
> ÂÂÂÂÂName (_DSD,ÂÂPackage ()
> ÂÂÂÂÂ{
> ÂÂÂÂÂÂÂÂÂToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> ÂÂÂÂÂÂÂÂÂPackage ()
> ÂÂÂÂÂÂÂÂÂ{
> ÂÂÂÂÂÂÂÂÂÂÂÂÂPackage (2) { "esd-recovery-timeout-ms", Package(1) {
> 2000 }},
> ÂÂÂÂÂÂÂÂÂÂÂÂÂ...
> ÂÂÂÂÂÂÂÂÂ}
> ÂÂÂÂÂ})
> Â}

Are there any devices which ship with that information? What do the
Windows drivers use as a default for ESD, and how is it declared in
ACPI?

> The ESD protection mechanism is only available if the gpio pins
> are properly initialized from ACPI/DT.
>
> This is based on Goodix datasheets for GT911 and GT9271 and on Goodix
> driver gt9xx.c for Android (publicly available in Android kernel
> trees for various devices).

Could you link to a "close to upstream" tree that includes that
support? Do they use the same timeout, or is it set to a default
hardcoded value instead?

Cheers

> Signed-off-by: Irina Tirdea <irina.tirdea@xxxxxxxxx>
> Acked-by: Rob Herring <robh@xxxxxxxxxx>
> ---
> Â.../bindings/input/touchscreen/goodix.txtÂÂÂÂÂÂÂÂÂÂ|ÂÂÂ4 +
> Âdrivers/input/touchscreen/goodix.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ| 130
> ++++++++++++++++++++-
> Â2 files changed, 132 insertions(+), 2 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index c98757a..ef5f42d 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -18,6 +18,10 @@ Optional properties:
> Â - irq-gpios : GPIO pin used for IRQ. The driver uses
> the
> Â ÂÂinterrupt gpio pin as output to reset the
> device.
> Â - reset-gpios : GPIO pin used for reset
> + - esd-recovery-timeout-ms : ESD poll time (in milli seconds) for
> the driver to
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcheck if ESD occurred and in that case
> reset the
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdevice. ESD is disabled if this property
> is not set
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂor is set to 0.
> Â
> Â - touchscreen-inverted-xÂÂ: X axis is inverted (boolean)
> Â - touchscreen-inverted-yÂÂ: Y axis is inverted (boolean)
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 2447b73..cf39dc4 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -49,10 +49,13 @@ struct goodix_ts_data {
> Â const char *cfg_name;
> Â struct completion firmware_loading_complete;
> Â unsigned long irq_flags;
> + atomic_t esd_timeout;
> + struct delayed_work esd_work;
> Â};
> Â
> Â#define GOODIX_GPIO_INT_NAME "irq"
> Â#define GOODIX_GPIO_RST_NAME "reset"
> +#define GOODIX_DEVICE_ESD_TIMEOUT_PROPERTYÂÂÂÂÂ"esd-recovery-
> timeout-ms"
> Â
> Â#define GOODIX_MAX_HEIGHT 4096
> Â#define GOODIX_MAX_WIDTH 4096
> @@ -67,6 +70,8 @@ struct goodix_ts_data {
> Â/* Register defines */
> Â#define GOODIX_REG_COMMAND 0x8040
> Â#define GOODIX_CMD_SCREEN_OFF 0x05
> +#define GOODIX_CMD_ESD_ENABLED 0xAA
> +#define GOODIX_REG_ESD_CHECK 0x8041
> Â
> Â#define GOODIX_READ_COOR_ADDR 0x814E
> Â#define GOODIX_REG_CONFIG_DATA 0x8047
> @@ -453,10 +458,114 @@ static ssize_t goodix_dump_config_show(struct
> device *dev,
> Â return count;
> Â}
> Â
> +static void goodix_disable_esd(struct goodix_ts_data *ts)
> +{
> + if (!atomic_read(&ts->esd_timeout))
> + return;
> + cancel_delayed_work_sync(&ts->esd_work);
> +}
> +
> +static int goodix_enable_esd(struct goodix_ts_data *ts)
> +{
> + int error, esd_timeout;
> +
> + esd_timeout = atomic_read(&ts->esd_timeout);
> + if (!esd_timeout)
> + return 0;
> +
> + error = goodix_i2c_write_u8(ts->client,
> GOODIX_REG_ESD_CHECK,
> + ÂÂÂÂGOODIX_CMD_ESD_ENABLED);
> + if (error) {
> + dev_err(&ts->client->dev, "Failed to enable ESD:
> %d\n", error);
> + return error;
> + }
> +
> + schedule_delayed_work(&ts->esd_work, round_jiffies_relative(
> + ÂÂÂÂÂÂmsecs_to_jiffies(esd_timeout)));
> + return 0;
> +}
> +
> +static void goodix_esd_work(struct work_struct *work)
> +{
> + struct goodix_ts_data *ts = container_of(work, struct
> goodix_ts_data,
> + Âesd_work.work);
> + int retries = 3, error;
> + u8 esd_data[2];
> + const struct firmware *cfg = NULL;
> +
> + wait_for_completion(&ts->firmware_loading_complete);
> +
> + while (--retries) {
> + error = goodix_i2c_read(ts->client,
> GOODIX_REG_COMMAND,
> + esd_data, sizeof(esd_data));
> + if (error)
> + continue;
> + if (esd_data[0] != GOODIX_CMD_ESD_ENABLED &&
> + ÂÂÂÂesd_data[1] == GOODIX_CMD_ESD_ENABLED) {
> + /* feed the watchdog */
> + goodix_i2c_write_u8(ts->client,
> + ÂÂÂÂGOODIX_REG_COMMAND,
> + ÂÂÂÂGOODIX_CMD_ESD_ENABLED);
> + break;
> + }
> + }
> +
> + if (!retries) {
> + dev_dbg(&ts->client->dev, "Performing ESD
> recovery.\n");
> + goodix_free_irq(ts);
> + goodix_reset(ts);
> + error = request_firmware(&cfg, ts->cfg_name, &ts-
> >client->dev);
> + if (!error) {
> + goodix_send_cfg(ts, cfg);
> + release_firmware(cfg);
> + }
> + goodix_request_irq(ts);
> + goodix_enable_esd(ts);
> + return;
> + }
> +
> + schedule_delayed_work(&ts->esd_work, round_jiffies_relative(
> + ÂÂÂÂÂÂmsecs_to_jiffies(atomic_read(&ts-
> >esd_timeout))));
> +}
> +
> +static ssize_t goodix_esd_timeout_show(struct device *dev,
> + ÂÂÂÂÂÂÂstruct device_attribute
> *attr, char *buf)
> +{
> + struct goodix_ts_data *ts = dev_get_drvdata(dev);
> +
> + return scnprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ts-
> >esd_timeout));
> +}
> +
> +static ssize_t goodix_esd_timeout_store(struct device *dev,
> + struct device_attribute
> *attr,
> + const char *buf, size_t
> count)
> +{
> + struct goodix_ts_data *ts = dev_get_drvdata(dev);
> + int error, esd_timeout, new_esd_timeout;
> +
> + error = kstrtouint(buf, 10, &new_esd_timeout);
> + if (error)
> + return error;
> +
> + esd_timeout = atomic_read(&ts->esd_timeout);
> + if (esd_timeout && !new_esd_timeout)
> + goodix_disable_esd(ts);
> +
> + atomic_set(&ts->esd_timeout, new_esd_timeout);
> + if (!esd_timeout && new_esd_timeout)
> + goodix_enable_esd(ts);
> +
> + return count;
> +}
> +
> Âstatic DEVICE_ATTR(dump_config, S_IRUGO, goodix_dump_config_show,
> NULL);
> +/* ESD timeout in ms. Default disabled (0). Recommended 2000 ms. */
> +static DEVICE_ATTR(esd_timeout, S_IRUGO | S_IWUSR,
> goodix_esd_timeout_show,
> + ÂÂÂgoodix_esd_timeout_store);
> Â
> Âstatic struct attribute *goodix_attrs[] = {
> Â &dev_attr_dump_config.attr,
> + &dev_attr_esd_timeout.attr,
> Â NULL
> Â};
> Â
> @@ -713,7 +822,11 @@ static void goodix_config_cb(const struct
> firmware *cfg, void *ctx)
> Â goto err_release_cfg;
> Â }
> Â
> - goodix_configure_dev(ts);
> + error = goodix_configure_dev(ts);
> + if (error)
> + goto err_release_cfg;
> +
> + goodix_enable_esd(ts);
> Â
> Âerr_release_cfg:
> Â release_firmware(cfg);
> @@ -724,7 +837,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
> Â ÂÂÂconst struct i2c_device_id *id)
> Â{
> Â struct goodix_ts_data *ts;
> - int error;
> + int error, esd_timeout;
> Â
> Â dev_dbg(&client->dev, "I2C Address: 0x%02x\n", client-
> >addr);
> Â
> @@ -740,6 +853,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
> Â ts->client = client;
> Â i2c_set_clientdata(client, ts);
> Â init_completion(&ts->firmware_loading_complete);
> + INIT_DELAYED_WORK(&ts->esd_work, goodix_esd_work);
> Â
> Â error = goodix_get_gpio_config(ts);
> Â if (error)
> @@ -769,6 +883,12 @@ static int goodix_ts_probe(struct i2c_client
> *client,
> Â ts->cfg_len = goodix_get_cfg_len(ts->id);
> Â
> Â if (ts->gpiod_int && ts->gpiod_rst) {
> + error = device_property_read_u32(&ts->client->dev,
> + GOODIX_DEVICE_ESD_TIMEOUT_PR
> OPERTY,
> + &esd_timeout);
> + if (!error)
> + atomic_set(&ts->esd_timeout, esd_timeout);
> +
> Â error = sysfs_create_group(&client->dev.kobj,
> Â ÂÂÂ&goodix_attr_group);
> Â if (error) {
> @@ -821,6 +941,7 @@ static int goodix_ts_remove(struct i2c_client
> *client)
> Â wait_for_completion(&ts->firmware_loading_complete);
> Â
> Â sysfs_remove_group(&client->dev.kobj, &goodix_attr_group);
> + goodix_disable_esd(ts);
> Â
> Â return 0;
> Â}
> @@ -836,6 +957,7 @@ static int __maybe_unused goodix_suspend(struct
> device *dev)
> Â return 0;
> Â
> Â wait_for_completion(&ts->firmware_loading_complete);
> + goodix_disable_esd(ts);
> Â
> Â /* Free IRQ as IRQ pin is used as output in the suspend
> sequence */
> Â goodix_free_irq(ts);
> @@ -894,6 +1016,10 @@ static int __maybe_unused goodix_resume(struct
> device *dev)
> Â if (error)
> Â return error;
> Â
> + error = goodix_enable_esd(ts);
> + if (error)
> + return error;
> +
> Â return 0;
> Â}
> Â

Next message: Bastien Nocera: "Re: [PATCH v12 4/5] Input: goodix - fix reset sequence"
Previous message: Rafael J. Wysocki: "Re: [PATCH v6 00/16] ACPI IORT ARM SMMU support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]