RE: [PATCH v11 6/8] Input: goodix - add support for ESD

[Tirdea, Irina]

> -Original Message-
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: 20 November, 2015 17:45
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; Karsten Merker; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian;
> linux-ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v11 6/8] Input: goodix - add support for ESD
> 
> On Thu, Nov 19, 2015 at 02:26:39PM +0200, Irina Tirdea wrote:
> > Add ESD (Electrostatic Discharge) protection mechanism.
> 
> [...]
> 
> > 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).
> >
> > Signed-off-by: Irina Tirdea <irina.tir...@intel.com>
> > For the binding: Acked-by: Rob Herring <r...@kernel.org>
> 
> You should not have the "For the binding:" part here. It was just a note
> so it was clear what part I looked at.
> 

I saw it done like that in another patch already merged, so I thought it's
the right way [1]. Dmitry, could you fix this at merge or you need me to
send another patchset?

Thanks,
Irina

[1] 
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/commit/?id=d2a3e0931a8f3b95b910096d022ffd98adbd075c

> It is preferred to split DT bindings to separate patches for this
> reason.
> 
> > Signed-off-by: Irina Tirdea <irina.tir...@intel.com>
> > ---
> >  .../bindings/input/touchscreen/goodix.txt  |   4 +
> >  drivers/input/touchscreen/goodix.c | 160 
> > -
> >  2 files changed, 159 insertions(+), 5 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v10 2/8] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: 18 November, 2015 23:24
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; Karsten Merker; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian;
> linux-ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v10 2/8] Input: goodix - reset device at init
> 
> On Wed, Nov 18, 2015 at 06:31:35PM +0200, Irina Tirdea wrote:
> > After power on, it is recommended that the driver resets the device.
> > The reset procedure timing is described in the datasheet and is used
> > at device init (before writing device configuration) and
> > for power management. It is a sequence of setting the interrupt
> > and reset pins high/low at specific timing intervals. This procedure
> > also includes setting the slave address to the one specified in the
> > ACPI/device tree.
> >
> > 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).
> >
> > For reset the driver needs to control the interrupt and
> > reset gpio pins (configured through ACPI/device tree). For devices
> > that do not have the gpio pins properly declared, the functionality
> > depending on these pins will not be available, but the device can still
> > be used with basic functionality.
> >
> > For both device tree and ACPI, the interrupt gpio pin configuration is
> > read from the "irq-gpio" property and the reset pin configuration is
> > read from the "reset-gpio" property. For ACPI 5.1, named properties
> > can be specified using the _DSD section. This functionality will not be
> > available for devices that use indexed gpio pins declared in the _CRS
> > section (we need to provide backward compatibility with devices
> > that do not support using the interrupt gpio pin as output).
> >
> > For ACPI, the pins can be specified using ACPI 5.1:
> > Device (STAC)
> > {
> > Name (_HID, "GDIX1001")
> > ...
> >
> > Method (_CRS, 0, Serialized)
> > {
> > Name (RBUF, ResourceTemplate ()
> > {
> > I2cSerialBus (0x0014, ControllerInitiated, 0x00061A80,
> > AddressingMode7Bit, "\\I2C0",
> > 0x00, ResourceConsumer, ,
> > )
> >
> > GpioInt (Edge, ActiveHigh, Exclusive, PullNone, 0x,
> > "\\I2C0", 0x00, ResourceConsumer, ,
> >  )
> >  {   // Pin list
> >  0
> >  }
> >
> > GpioIo (Exclusive, PullDown, 0x, 0x,
> > IoRestrictionOutputOnly, "\\I2C0", 0x00,
> > ResourceConsumer, ,
> > )
> > {
> >  1
> > }
> > })
> > Return (RBUF)
> > }
> >
> > Name (_DSD,  Package ()
> > {
> > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > Package ()
> > {
> > Package (2) {"irq-gpio", Package() {^STAC, 0, 0, 0 }},
> > Package (2) {"reset-gpio", Package() {^STAC, 1, 0, 0 }},
> > ...
> > }
> > }
> >
> > Signed-off-by: Octavian Purdila <octavian.purd...@intel.com>
> > Signed-off-by: Irina Tirdea <irina.tir...@intel.com>
> > ---
> >  .../bindings/input/touchscreen/goodix.txt  |   5 +
> >  drivers/input/touchscreen/Kconfig  |   1 +
> >  drivers/input/touchscreen/goodix.c | 101 
> > +
> >  3 files changed, 107 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > index 8ba98ee..7137881 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > @@ -12,6 +12,8 @@ Required properties:
> >   - reg : I2C address of the chip. Should be 0x5d or 
> > 0x14
> >   - interrupt-parent: Interrupt controller to which the chip is 
> > connected
> >   - interrupts  : Interrupt to which the chip is connected
> > + - irq-gpio: GPIO pin used for IRQ
> 
> Please note here why you need this in addition to just "interrupts".

Ok.

> Also, it should be irq-gpios instead.
> 
> > + - reset-gpio  : GPIO pin used for reset
> 
> Should be reset-gpios instead.
> 

I'll fix this, I wasn't aware that irq/reset-gpio is deprecated.

Thanks,
Irina

> Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v11 2/8] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: Bastien Nocera [mailto:had...@hadess.net]
> Sent: 19 November, 2015 17:25
> To: Tirdea, Irina; Dmitry Torokhov; Aleksei Mamlin; Karsten Merker; 
> linux-input@vger.kernel.org
> Cc: Mark Rutland; Rob Herring; Purdila, Octavian; 
> linux-ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v11 2/8] Input: goodix - reset device at init
> 
> On Thu, 2015-11-19 at 14:26 +0200, Irina Tirdea wrote:
> > After power on, it is recommended that the driver resets the device.
> > The reset procedure timing is described in the datasheet and is used
> > at device init (before writing device configuration) and
> > for power management. It is a sequence of setting the interrupt
> > and reset pins high/low at specific timing intervals. This procedure
> > also includes setting the slave address to the one specified in the
> > ACPI/device tree.
> 
> This fails on a 4.3 kernel with an ACPI device (WinBook TW100):
> Goodix-TS: probe of i2c-GDIX1001:00 failed with error -16
> 
> Can you please document which upstream commit is necessary to make this
> behave properly?
> 

You need the patch that fixes the GPIO API [1] so that
devm_gpiod_get_optional works properly (I mentioned that in the cover
letter). This patch just got merged in the gpio tree, so it will take a
while until it will be merged in the main kernel tree or input tree. 

Thanks,
Irina

 [1] https://lkml.org/lkml/2015/11/11/465

> I'll test again with a newer kernel.
> 
> Cheers

RE: [PATCH v9 0/9] Goodix touchscreen enhancements

[Tirdea, Irina]

> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: 27 October, 2015 1:33
> To: Karsten Merker
> Cc: Bastien Nocera; Tirdea, Irina; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v9 0/9] Goodix touchscreen enhancements
> 
> On Mon, Oct 26, 2015 at 07:21:12PM +0100, Karsten Merker wrote:
> > On Mon, Oct 26, 2015 at 04:06:29PM +0100, Bastien Nocera wrote:
> > > On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:
> >
> > > > v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> > > > "Input: goodix - reset device at init". There are no other code
> > > > changes from v8.
> > > >
> > > > Thanks for testing these changes, Bastien and Aleksei!
> > > >
> > > > Karsten, there is no need to rebase your series on top of v9.
> > >
> > > Are we waiting on anything else before merging this? I'd like it to be
> > > scheduled to be merged so I can start focusing on the subsequent and
> > > dependent patches for that same driver.
> >
> > Hello,
> >
> > AFAICS there is one open point (cf.
> > http://www.spinics.net/lists/linux-input/msg41567.html) which
> > Irina wanted to address in a v10 of the patchset (cf.
> > http://www.spinics.net/lists/linux-input/msg41642.html).
> 
> There is also the whole thing about insane handling of named gpios in
> ACPI layer, which stops me from merging the reset code since these gpios
> should be marked as optional and we should stop ignoring errors coming
> from gpiolib.

The ACPI layer change is quite complex, since it includes changing the drivers
that use the gpio API before removing the fallback to indexed ACPI.
Not sure that will not also break current drivers that already count on this
fallback. Unfortunately, I do not have the time right now to get involved in
fixing the ACPI core myself.

Dmitry, is there anything I can do in the driver to get these patches merged?
I could go back to using indexed gpios and add an additional property
to specify if irq can be used as output or not (as suggested in one of the
previous reviews).

Thanks,
Irina

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

RE: [PATCH v9 0/9] Goodix touchscreen enhancements

[Tirdea, Irina]

> -Original Message-
> From: Karsten Merker [mailto:mer...@debian.org]
> Sent: 26 October, 2015 20:21
> To: Bastien Nocera; Tirdea, Irina
> Cc: Dmitry Torokhov; Aleksei Mamlin; Karsten Merker; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v9 0/9] Goodix touchscreen enhancements
> 
> On Mon, Oct 26, 2015 at 04:06:29PM +0100, Bastien Nocera wrote:
> > On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:
> 
> > > v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> > > "Input: goodix - reset device at init". There are no other code
> > > changes from v8.
> > >
> > > Thanks for testing these changes, Bastien and Aleksei!
> > >
> > > Karsten, there is no need to rebase your series on top of v9.
> >
> > Are we waiting on anything else before merging this? I'd like it to be
> > scheduled to be merged so I can start focusing on the subsequent and
> > dependent patches for that same driver.
> 
> Hello,
> 
> AFAICS there is one open point (cf.
> http://www.spinics.net/lists/linux-input/msg41567.html) which
> Irina wanted to address in a v10 of the patchset (cf.
> http://www.spinics.net/lists/linux-input/msg41642.html).
> 
> Irina, how are your plans regarding the v10? It would be really
> nice if the patches could go into kernel 4.4, but the merge
> window opens on the coming weekend, so there is not much time
> left.

I can send v10 with the change mentioned above by the end of this week.

However, as Dmitry already mentioned, there is another issue with
the gpio ACPI layer that is blocking the entire patchset.

> 
> Bastien, did you have time to look at v3 of the axis
> swapping/inversion set?
> (http://www.spinics.net/lists/linux-input/msg41628.html)
> You had acked v2, but I had to do some small changes to address
> Irina's review comments after you had acked it, so I didn't want
> to carry your "acked-by" on to v3 without an ok from you.
> 
> Regards,
> Karsten
> --
> Gem. Par. 28 Abs. 4 Bundesdatenschutzgesetz widerspreche ich der Nutzung
> sowie der Weitergabe meiner personenbezogenen Daten für Zwecke der
> Werbung sowie der Markt- oder Meinungsforschung.

RE: [PATCH v7 3/9] Input: goodix - write configuration data to device

[Tirdea, Irina]

> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: 14 October, 2015 9:59
> To: Tirdea, Irina
> Cc: Bastien Nocera; Aleksei Mamlin; linux-input@vger.kernel.org; Mark 
> Rutland; Purdila, Octavian; linux-ker...@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v7 3/9] Input: goodix - write configuration data to device
> 
> On Thu, Oct 08, 2015 at 01:19:29PM +0300, Irina Tirdea wrote:
> > Goodix devices can be configured by writing custom data to the device at
> > init. The configuration data is read with request_firmware from
> > "goodix__cfg.bin", where  is the product id read from the device
> > (e.g.: goodix_911_cfg.bin for Goodix GT911, goodix_9271_cfg.bin for
> > GT9271).
> >
> > The configuration information has a specific format described in the Goodix
> > datasheet. It includes X/Y resolution, maximum supported touch points,
> > interrupt flags, various sensitivity factors and settings for advanced
> > features (like gesture recognition).
> >
> > Before writing the firmware, it is necessary to reset the device. If
> > the device ACPI/DT information does not declare gpio pins (needed for
> > reset), writing the firmware will not be available for these devices.
> >
> > 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).
> >
> > Signed-off-by: Octavian Purdila <octavian.purd...@intel.com>
> > Signed-off-by: Irina Tirdea <irina.tir...@intel.com>
> > ---
> >  drivers/input/touchscreen/goodix.c | 229 
> > +++--
> >  1 file changed, 196 insertions(+), 33 deletions(-)
> >

> > +/**
> > + * goodix_config_cb - Callback to finish device init
> > + *
> > + * @ts: our goodix_ts_data pointer
> > + *
> > + * request_firmware_wait callback that finishes
> > + * initialization of the device.
> > + */
> > +static void goodix_config_cb(const struct firmware *cfg, void *ctx)
> > +{
> > +   struct goodix_ts_data *ts = (struct goodix_ts_data *)ctx;
> > +   int error;
> > +
> > +   if (cfg) {
> > +   /* send device configuration to the firmware */
> > +   error = goodix_send_cfg(ts, cfg);
> > +   if (error)
> > +   goto err_release_cfg;
> > +   }
> > +   goodix_configure_dev(ts);
> > +
> > +err_release_cfg:
> > +   kfree(ts->cfg_name);
> > +   release_firmware(cfg);
> 
> You need to use completion to signal remove() (and also probably
> suspend/resume in the subsequent patches) that you are done handling
> config, otherwise if you do bind/unbind via sysfs in a tight loop you
> will observe a nice crash.
> 
> Thanks.
> 

Right, missed that. Will fix in next version.

Thanks,
Irina


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

RE: [PATCH RFC V2 1/3] Input: goodix - add dt axis swapping and axis inversion support

[Tirdea, Irina]

> -Original Message-
> From: linux-input-ow...@vger.kernel.org 
> [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Karsten Merker
> Sent: 09 October, 2015 20:56
> To: Bastien Nocera; Dmitry Torokhov; Tirdea, Irina; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Ian Campbell
> Cc: devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Chen-Yu Tsai; 
> Karsten Merker
> Subject: [PATCH RFC V2 1/3] Input: goodix - add dt axis swapping and axis 
> inversion support
> 
> Implement support for the following device-tree properties
> in the goodix touchscreen driver:
> 
>  - touchscreen-inverted-x:  X axis is inverted (boolean)
>  - touchscreen-inverted-y:  Y axis is inverted (boolean)
>  - touchscreen-swapped-x-y: X and Y axis are swapped (boolean)
> 
> These are necessary on tablets which have a display in portrait
> format while the touchscreen is in landscape format, such as e.g.
> the MSI Primo 81.
> 
> Signed-off-by: Karsten Merker <mer...@debian.org>
> ---
>  drivers/input/touchscreen/goodix.c | 27 +++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/goodix.c 
> b/drivers/input/touchscreen/goodix.c
> index 22bfc4b..a05bdad 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -2,6 +2,7 @@
>   *  Driver for Goodix Touchscreens
>   *
>   *  Copyright (c) 2014 Red Hat Inc.
> + *  Copyright (c) 2015 K. Merker <mer...@debian.org>
>   *
>   *  This code is based on gt9xx.c authored by and...@goodix.com:
>   *
> @@ -53,6 +54,9 @@ struct goodix_ts_data {
>   atomic_t open_count;
>   /* Protects power management calls and access to suspended flag */
>   struct mutex mutex;
> + bool swapped_x_y;
> + bool inverted_x;
> + bool inverted_y;
>  };
> 
>  #define GOODIX_GPIO_INT_NAME "irq"
> @@ -271,6 +275,14 @@ static void goodix_ts_report_touch(struct goodix_ts_data 
> *ts, u8 *coor_data)
>   input_y = ts->abs_y_max - input_y;
>   }
> 
> + /* Inversions have to happen before axis swapping */
> + if (ts->inverted_x)
> + input_x = ts->abs_x_max - input_x;
> + if (ts->inverted_y)
> + input_y = ts->abs_y_max - input_y;
> + if (ts->swapped_x_y)
> + swap(input_x, input_y);
> +
>   input_mt_slot(ts->input_dev, id);
>   input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true);
>   input_report_abs(ts->input_dev, ABS_MT_POSITION_X, input_x);
> @@ -666,6 +678,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
>error);
>   ts->abs_x_max = GOODIX_MAX_WIDTH;
>   ts->abs_y_max = GOODIX_MAX_HEIGHT;
> + if (ts->swapped_x_y)
> + swap(ts->abs_x_max, ts->abs_y_max);
>   ts->int_trigger_type = GOODIX_INT_TRIGGER;
>   ts->max_touch_num = GOODIX_MAX_CONTACTS;
>   return;
> @@ -673,6 +687,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
> 
>   ts->abs_x_max = get_unaligned_le16([RESOLUTION_LOC]);
>   ts->abs_y_max = get_unaligned_le16([RESOLUTION_LOC + 2]);
> + if (ts->swapped_x_y)
> + swap(ts->abs_x_max, ts->abs_y_max);
>   ts->int_trigger_type = config[TRIGGER_LOC] & 0x03;
>   ts->max_touch_num = config[MAX_CONTACTS_LOC] & 0x0f;
>   if (!ts->abs_x_max || !ts->abs_y_max || !ts->max_touch_num) {
> @@ -680,6 +696,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
>   "Invalid config, using defaults\n");
>   ts->abs_x_max = GOODIX_MAX_WIDTH;
>   ts->abs_y_max = GOODIX_MAX_HEIGHT;
> + if (ts->swapped_x_y)
> + swap(ts->abs_x_max, ts->abs_y_max);
>   ts->max_touch_num = GOODIX_MAX_CONTACTS;
>   }
> 
> @@ -950,6 +968,15 @@ static int goodix_ts_probe(struct i2c_client *client,
>   return 0;
>   }
> 
> +#ifdef CONFIG_OF
> + ts->swapped_x_y = of_property_read_bool(client->dev.of_node,
> + "touchscreen-swapped-x-y");
> + ts->inverted_x = of_property_read_bool(client->dev.of_node,
> +"touchscreen-inverted-x");
> + ts->inverted_y = of_property_read_bool(client->dev.of_node,
> +"touchscreen-inverted-y");
> +#endif
> +

If interrupt and reset gpio pins are declared in the DT configuration, this 
co

RE: [PATCH v9 2/9] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: 12 October, 2015 19:48
> To: Tirdea, Irina
> Cc: Bastien Nocera; Aleksei Mamlin; Karsten Merker; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> 
> On Mon, Oct 12, 2015 at 06:24:30PM +0300, Irina Tirdea wrote:
> > After power on, it is recommended that the driver resets the device.
> > The reset procedure timing is described in the datasheet and is used
> > at device init (before writing device configuration) and
> > for power management. It is a sequence of setting the interrupt
> > and reset pins high/low at specific timing intervals. This procedure
> > also includes setting the slave address to the one specified in the
> > ACPI/device tree.
> >
> > 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).
> >
> > For reset the driver needs to control the interrupt and
> > reset gpio pins (configured through ACPI/device tree). For devices
> > that do not have the gpio pins properly declared, the functionality
> > depending on these pins will not be available, but the device can still
> > be used with basic functionality.
> >
> > For both device tree and ACPI, the interrupt gpio pin configuration is
> > read from the "irq-gpio" property and the reset pin configuration is
> > read from the "reset-gpio" property. For ACPI 5.1, named properties
> > can be specified using the _DSD section. This functionality will not be
> > available for devices that use indexed gpio pins declared in the _CRS
> > section (we need to provide backward compatibility with devices
> > that do not support using the interrupt gpio pin as output).
> >
> > For ACPI, the pins can be specified using ACPI 5.1:
> > Device (STAC)
> > {
> > Name (_HID, "GDIX1001")
> > ...
> >
> > Method (_CRS, 0, Serialized)
> > {
> > Name (RBUF, ResourceTemplate ()
> > {
> > I2cSerialBus (0x0014, ControllerInitiated, 0x00061A80,
> > AddressingMode7Bit, "\\I2C0",
> > 0x00, ResourceConsumer, ,
> > )
> >
> > GpioInt (Edge, ActiveHigh, Exclusive, PullNone, 0x,
> > "\\I2C0", 0x00, ResourceConsumer, ,
> >  )
> >  {   // Pin list
> >  0
> >  }
> >
> > GpioIo (Exclusive, PullDown, 0x, 0x,
> > IoRestrictionOutputOnly, "\\I2C0", 0x00,
> > ResourceConsumer, ,
> > )
> > {
> >  1
> > }
> > })
> > Return (RBUF)
> > }
> >
> > Name (_DSD,  Package ()
> > {
> > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > Package ()
> > {
> > Package (2) {"irq-gpio", Package() {^STAC, 0, 0, 0 }},
> > Package (2) {"reset-gpio", Package() {^STAC, 1, 0, 0 }},
> > ...
> > }
> > }
> >
> > Signed-off-by: Octavian Purdila <octavian.purd...@intel.com>
> > Signed-off-by: Irina Tirdea <irina.tir...@intel.com>
> > Acked-by: Bastien Nocera <had...@hadess.net>
> > Tested-by: Bastien Nocera <had...@hadess.net>
> > Tested-by: Aleksei Mamlin <mamli...@gmail.com>
> > ---
> >  .../bindings/input/touchscreen/goodix.txt  |   5 +
> >  drivers/input/touchscreen/Kconfig  |   1 +
> >  drivers/input/touchscreen/goodix.c | 101 
> > +
> >  3 files changed, 107 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > index 8ba98ee..7137881 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > @@ -12,6 +12,8 @@ Required properties:
> >   - reg : I2C address of the chip. Should be 0x5d or 
> > 0x14
> >   - interrupt-parent: Interrupt controller to which the chip is 
> > conn

RE: [PATCH RFC V2 2/3] Input: goodix - use "inverted_[xy]" flags instead of "rotated_screen"

[Tirdea, Irina]

> -Original Message-
> From: Karsten Merker [mailto:mer...@debian.org]
> Sent: 09 October, 2015 20:56
> To: Bastien Nocera; Dmitry Torokhov; Tirdea, Irina; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Ian Campbell
> Cc: devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Chen-Yu Tsai; 
> Karsten Merker
> Subject: [PATCH RFC V2 2/3] Input: goodix - use "inverted_[xy]" flags instead 
> of "rotated_screen"
> 
> The goodix touchscreen driver uses a "rotated_screen" flag for
> systems on which the touchscreen is mounted rotated by 180
> degrees with respect to the display.  With the addition of
> support for the dt properties "touchscreen-inverted-x" and
> "touchscreen-inverted-y", a separate "rotated_screen" flag
> is not necessary any more. This patch replaces it by setting
> the inverted_x and inverted_y flags instead.
> 
> Signed-off-by: Karsten Merker <mer...@debian.org>
> ---

Reviewed-by: Irina Tirdea <irina.tir...@intel.com>

>  drivers/input/touchscreen/goodix.c | 12 
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c 
> b/drivers/input/touchscreen/goodix.c
> index a05bdad..d910b27 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -40,7 +40,6 @@ struct goodix_ts_data {
>   int abs_y_max;
>   unsigned int max_touch_num;
>   unsigned int int_trigger_type;
> - bool rotated_screen;
>   int cfg_len;
>   struct gpio_desc *gpiod_int;
>   struct gpio_desc *gpiod_rst;
> @@ -270,11 +269,6 @@ static void goodix_ts_report_touch(struct goodix_ts_data 
> *ts, u8 *coor_data)
>   int input_y = get_unaligned_le16(_data[3]);
>   int input_w = get_unaligned_le16(_data[5]);
> 
> - if (ts->rotated_screen) {
> - input_x = ts->abs_x_max - input_x;
> - input_y = ts->abs_y_max - input_y;
> - }
> -
>   /* Inversions have to happen before axis swapping */
>   if (ts->inverted_x)
>   input_x = ts->abs_x_max - input_x;
> @@ -701,10 +695,12 @@ static void goodix_read_config(struct goodix_ts_data 
> *ts)
>   ts->max_touch_num = GOODIX_MAX_CONTACTS;
>   }
> 
> - ts->rotated_screen = dmi_check_system(rotated_screen);
> - if (ts->rotated_screen)
> + if (dmi_check_system(rotated_screen)) {
> + ts->inverted_x = true;
> + ts->inverted_y = true;
>   dev_dbg(>client->dev,
>"Applying '180 degrees rotated screen' quirk\n");
> + }
>  }
> 
>  /**
> --
> 2.1.4

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

RE: [PATCH RFC V2 0/3] Input: goodix - add axis swapping and axis inversion support

[Tirdea, Irina]

> -Original Message-
> From: Karsten Merker [mailto:mer...@debian.org]
> Sent: 09 October, 2015 20:56
> To: Bastien Nocera; Dmitry Torokhov; Tirdea, Irina; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Ian Campbell
> Cc: devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Chen-Yu Tsai; 
> Karsten Merker
> Subject: [PATCH RFC V2 0/3] Input: goodix - add axis swapping and axis 
> inversion support
> 
> Hello,
> 
> this is v2 of my "Input: goodix - add axis swapping and axis inversion
> support" patchset.
> The goodix touchscreen driver has gained device-tree support in kernel
> 4.1, but doesn't currently support the touchscreen-swapped-x-y,
> touchscreen-inverted-x and touchscreen-inverted-y properties.
> 
> On systems which combine a portrait-mode display with a landscape-mode
> touchscreen, such as e.g. the MSI Primo 81 tablet, support for these
> features is necessary to have the touchscreen and the display use the
> same coordinate system.
> 
> With support for axis inversion, the "rotated_screen" flag in the
> driver can also be removed, as "rotated_screen" is just a special case
> of x/y axis inversion.
> 
> This patchset sits on top of the "[PATCH v8 0/9] Goodix touchscreen
> enhancements" series by Irina Tirdea:
> https://www.spinics.net/lists/linux-input/msg41437.html
> 
> I have successfully tested the axis swapping on an (arm-based) MSI
> Primo 81 tablet, but I lack appropriate hardware to do a real-world
> test of the "rotated_screen" code path, so I would appreciate very
> much if somebody with appropriate hardware (WinBook TW100 or TW700)
> could give it a try.
> 
> Regards,
> Karsten
> 

Hi Karsten,

I took a look at your patches and also did a quick test on my setup.
Code looks good, I have just one comment I've mentioned on the
first patch in the series.

Thanks,
Irina

> Changelog:
> 
> v1: * Initial version (based von v6 of Irina Tirdea's "Goodix
>   touchscreen enhancements" series).
>   Reviewed-by: Bastien Nocera <had...@hadess.net>
> 
> v2: * Rebase against v8 of Irina Tirdea's "Goodix touchscreen
>   enhancements" series.
> * Fix a typo in the commit message.
> * Add an update for the goodix dt bindings documentation
>   (patch No. 3).
> 
> 
> Karsten Merker (3):
>   Input: goodix - add dt axis swapping and axis inversion support
>   Input: goodix - use "inverted_[xy]" flags instead of "rotated_screen"
>   Input: goodix - update dt bindings documentation (axis
> swapping/inversion)
> 
>  .../bindings/input/touchscreen/goodix.txt  |  6 
>  drivers/input/touchscreen/goodix.c | 33 
> ++
>  2 files changed, 34 insertions(+), 5 deletions(-)
> 
> --
> 2.1.4

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

RE: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Tirdea, Irina]

> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: 12 October, 2015 19:31
> To: Bastien Nocera
> Cc: Mark Rutland; Tirdea, Irina; Aleksei Mamlin; Karsten Merker; 
> linux-input@vger.kernel.org; Purdila, Octavian; lkml;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas 
> tree order
> 
> On Mon, Oct 12, 2015 at 8:53 AM, Bastien Nocera <had...@hadess.net> wrote:
> > On Mon, 2015-10-12 at 16:51 +0100, Mark Rutland wrote:
> >> On Mon, Oct 12, 2015 at 05:40:37PM +0200, Bastien Nocera wrote:
> >> > On Mon, 2015-10-12 at 16:39 +0100, Mark Rutland wrote:
> >> > > Why?
> >> >
> >> > It was already discussed in the thread for a previous version,
> >> > please
> >> > refer to that.
> >>
> >> Fine, but surely that should be in the commit message, to prevent
> >> others
> >> like myself repeatedly asking the same question?
> >
> > Fair point. Irina?
> 
> No, let's just leave poor includes alone.
> 

OK, will drop this :)

Thanks,
Irina

> --
> Dmitry

RE: [PATCH v9 2/9] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: 13 October, 2015 10:08
> To: Tirdea, Irina
> Cc: Bastien Nocera; Aleksei Mamlin; Karsten Merker; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> 
> On Tue, Oct 13, 2015 at 06:38:23AM +, Tirdea, Irina wrote:
> >
> >
> > > -Original Message-
> > > From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> > > Sent: 12 October, 2015 19:48
> > > To: Tirdea, Irina
> > > Cc: Bastien Nocera; Aleksei Mamlin; Karsten Merker; 
> > > linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> > > ker...@vger.kernel.org; devicet...@vger.kernel.org
> > > Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> > >
> > > On Mon, Oct 12, 2015 at 06:24:30PM +0300, Irina Tirdea wrote:
> > > > After power on, it is recommended that the driver resets the device.
> > > > The reset procedure timing is described in the datasheet and is used
> > > > at device init (before writing device configuration) and
> > > > for power management. It is a sequence of setting the interrupt
> > > > and reset pins high/low at specific timing intervals. This procedure
> > > > also includes setting the slave address to the one specified in the
> > > > ACPI/device tree.
> > > >
> > > > 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).
> > > >
> > > > For reset the driver needs to control the interrupt and
> > > > reset gpio pins (configured through ACPI/device tree). For devices
> > > > that do not have the gpio pins properly declared, the functionality
> > > > depending on these pins will not be available, but the device can still
> > > > be used with basic functionality.
> > > >
> > > > For both device tree and ACPI, the interrupt gpio pin configuration is
> > > > read from the "irq-gpio" property and the reset pin configuration is
> > > > read from the "reset-gpio" property. For ACPI 5.1, named properties
> > > > can be specified using the _DSD section. This functionality will not be
> > > > available for devices that use indexed gpio pins declared in the _CRS
> > > > section (we need to provide backward compatibility with devices
> > > > that do not support using the interrupt gpio pin as output).
> > > >
> > > > For ACPI, the pins can be specified using ACPI 5.1:
> > > > Device (STAC)
> > > > {
> > > > Name (_HID, "GDIX1001")
> > > > ...
> > > >
> > > > Method (_CRS, 0, Serialized)
> > > > {
> > > > Name (RBUF, ResourceTemplate ()
> > > > {
> > > > I2cSerialBus (0x0014, ControllerInitiated, 0x00061A80,
> > > > AddressingMode7Bit, "\\I2C0",
> > > > 0x00, ResourceConsumer, ,
> > > > )
> > > >
> > > > GpioInt (Edge, ActiveHigh, Exclusive, PullNone, 0x,
> > > > "\\I2C0", 0x00, ResourceConsumer, ,
> > > >  )
> > > >  {   // Pin list
> > > >  0
> > > >  }
> > > >
> > > > GpioIo (Exclusive, PullDown, 0x, 0x,
> > > > IoRestrictionOutputOnly, "\\I2C0", 0x00,
> > > > ResourceConsumer, ,
> > > > )
> > > > {
> > > >  1
> > > > }
> > > > })
> > > > Return (RBUF)
> > > > }
> > > >
> > > > Name (_DSD,  Package ()
> > > > {
> > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > > > Package ()
> > > > {
> > > > Package (2) {"irq-gpio", Package() {^STAC, 0, 0, 0 }},
> > > > Package (2) {"reset-gpio", Package() {^STAC, 1, 0, 0 }},
> > > > ...
>

RE: [PATCH RFC V2 3/3] Input: goodix - update dt bindings documentation (axis swapping/inversion)

[Tirdea, Irina]

> -Original Message-
> From: linux-input-ow...@vger.kernel.org 
> [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Karsten Merker
> Sent: 09 October, 2015 20:56
> To: Bastien Nocera; Dmitry Torokhov; Tirdea, Irina; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Ian Campbell
> Cc: devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Chen-Yu Tsai; 
> Karsten Merker
> Subject: [PATCH RFC V2 3/3] Input: goodix - update dt bindings documentation 
> (axis swapping/inversion)
> 
> The goodix touchscreen driver has gained support for the
> optional touchscreen-inverted-x, touchscreen-inverted-y
> and touchscreen-swapped-x-y properties as described in
> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt.
> 
> Document these properties in the goodix bindings description.
> 
> Signed-off-by: Karsten Merker <mer...@debian.org>

Reviewed-by: Irina Tirdea <irina.tir...@intel.com>

> ---
>  Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index 4db3393..4ecd0e1 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -14,6 +14,7 @@ Required properties:
>   - interrupts: Interrupt to which the chip is connected
>   - irq-gpio  : GPIO pin used for IRQ
>   - reset-gpio: GPIO pin used for reset
> +
>  Optional properties:
> 
>   - esd-recovery-timeout-ms : ESD poll time (in milli seconds) for the driver 
> to
> @@ -21,6 +22,11 @@ Optional properties:
>  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)
> + - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
> + (swapping is done after inverting the axis)
> +
>  Example:
> 
>   i2c@ {
> --
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v8 0/9] Goodix touchscreen enhancements

[Tirdea, Irina]

> -Original Message-
> From: linux-input-ow...@vger.kernel.org 
> [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Arnd Bergmann
> Sent: 08 October, 2015 17:32
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v8 0/9] Goodix touchscreen enhancements
> 
> On Thursday 08 October 2015 17:23:42 Irina Tirdea wrote:
> > Add several enhancements to the Goodix touchscreen driver.
> >
> > The new functionality is only available for devices that
> > declare named gpio pins for interrupt and reset in their
> > ACPI/DT configuration.
> >
> > Thanks,
> > Irina
> >
> > Changes in v8:
> >  - only allow new functionality for devices that declare named
> > gpios (using _DSD properties in ACPI or named DT properties)
> >
> >
> 
> Looks much cleaner this way, thanks!
> 
> One remaining question: how would you handle the case where
> the hardware doesn't support configuring the int-gpios line
> as output but you still want to use the named gpios?
> 

The only reason to use named gpios is to enable the new functionality
introduced by this patch set (mainly reset and power management).
You cannot use only one gpio either (e.g. only reset gpio but not the
interrupt gpio), you need both for this functionality to work.

If the hardware does not support using the interrupt gpio pin as output,
it should not declare the named int-gpio pin in ACPI. It is true that
we might later run into platforms that declare the int-gpio pin even if it
does not actually work (just like we now have the indexed gpio pins).
I could have added an additional property for this as you first suggested,
but if we can modify the ACPI to add it, we could simply remove the
named interrupt pin instead (that does not work anyway).

> I assume you have that case covered, but I don't see it immediately.
> 
>   Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v7 2/9] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: 08 October, 2015 13:54
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v7 2/9] Input: goodix - reset device at init
> 
> On Thursday 08 October 2015 13:19:28 Irina Tirdea wrote:
> > After power on, it is recommended that the driver resets the device.
> > The reset procedure timing is described in the datasheet and is used
> > at device init (before writing device configuration) and
> > for power management. It is a sequence of setting the interrupt
> > and reset pins high/low at specific timing intervals. This procedure
> > also includes setting the slave address to the one specified in the
> > ACPI/device tree.
> >
> > 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).
> >
> > For reset the driver needs to control the interrupt and
> > reset gpio pins (configured through ACPI/device tree). For devices
> > that do not have the gpio pins declared, the functionality depending
> > on these pins will not be available, but the device can still be used
> > with basic functionality.
> >
> > For both device tree and ACPI, the interrupt gpio pin configuration is
> > read from the "irq-gpio" property and the reset pin configuration is
> > read from the "reset-gpio" property. For ACPI 5.1, named properties
> > can be specified using the _DSD section. If there is no _DSD section
> > in the ACPI table, the driver will fall back to using indexed gpio
> > pins declared in the _CRS section.
> 
> Would it help to use a plain "gpios" property here to always look
> up the lines by index?
> 

The problem with ACPI indexed gpios is that platforms declare the
pins in random order. In this case we have some platforms that declare
the interrupt pin first and others that declare the reset pin first.
There is no way to differentiate between them so the only way to support
these platforms is to pick a default and list all exceptions in the driver.
My previous implementation did that with indexed gpios and dmi quirks. [1]

This can be solved by using named gpios, which are available starting with ACPI 
5.1.
In this way we know exactly which is the interrupt pin and which is the reset 
pin
and we do not need to add any additional exceptions to the driver.
However, we still need to support the platforms that are already out there so
we fall back to indexed gpios.

[1] https://lkml.org/lkml/2015/9/15/609

> > +/*
> > + * Some platforms specify the gpio pins for interrupt and reset properly
> > + * in ACPI, but cannot use the interrupt pin as output due to their 
> > specific
> > + * HW configuration.
> > + */
> > +static const struct dmi_system_id goodix_no_gpio_pins_support[] = {
> > +#if defined(CONFIG_DMI) && defined(CONFIG_X86)
> > +   {
> > +   .ident = "Onda v975w",
> > +   .matches = {
> > +   DMI_MATCH(DMI_BIOS_VENDOR, "American Megatrends Inc."),
> > +   DMI_MATCH(DMI_PRODUCT_UUID,
> > + "03000200-0400-0500-0006-000700080009"),
> > +   DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
> > +   DMI_MATCH(DMI_BOARD_NAME, "Aptio CRB"),
> > +   }
> > +   },
> 
> I think lists like this in drivers should be avoided if at all possible,
> it just leads to other people adding their platform in the lists as
> opposed to fixing their boot loaders.
> 
> Can you find another way to detect at runtime whether it works, and
> print a warning if it doesn't?

I agree with you on this, but unfortunately I have not found a better way to do 
it.

The main problem comes from the interrupt pin. This device uses the interrupt 
pin
as output, which some platforms do not support (either due to the HW 
configuration
or due to flagging it wrong in BIOS) [2] [3]. There is no error returned, just 
a warning
in dmesg.

[2] https://lkml.org/lkml/2015/9/28/851
[3] https://lkml.org/lkml/2015/9/30/607

> If there is no way to detect that kind
> of device, we should probably have another property that the driver
> can read to determine this, so we can avoid adding each system here.
> 

If one or both gpio pins are not declared in ACPI/DT or if gpio initialization 
fails,
the driver will work without the functionality that depends on them. 

However, this will not work

RE: [PATCH v7 0/9] Goodix touchscreen enhancements

[Tirdea, Irina]

> -Original Message-
> From: linux-input-ow...@vger.kernel.org 
> [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Arnd Bergmann
> Sent: 08 October, 2015 14:10
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v7 0/9] Goodix touchscreen enhancements
> 
> On Thursday 08 October 2015 13:19:26 Irina Tirdea wrote:
> > Add several enhancements to the Goodix touchscreen driver.
> >
> > Bastien, the functionality from this patches should be now skipped
> > for your devices (Onda v975w, WinBook TW100 and WinBook TW700).
> >
> > Aleksei, I have removed your Tested-by tag since I have done some
> > non-trivial changes to the "reset device at init" patch.
> >
> >
> 
> I've read over all 9 patches now, most of them look good, but I
> commented on the one that I have concerns about.
> 

Thanks for the review, Arnd!

Irina

>   Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 1/9] Input: goodix - sort includes alphabetically

[Tirdea, Irina]

> -Original Message-
> From: Pavel Machek [mailto:pa...@ucw.cz]
> Sent: 05 October, 2015 23:58
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v5 1/9] Input: goodix - sort includes alphabetically
> 
> Hi!
> 

Hi!

> > > Subject: Re: [PATCH v5 1/9] Input: goodix - sort includes alphabetically
> > >
> > > On Mon 2015-09-07 17:36:15, Irina Tirdea wrote:
> > > > Signed-off-by: Irina Tirdea <irina.tir...@intel.com>
> > > > ---
> > > >  drivers/input/touchscreen/goodix.c | 12 ++--
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > Don't.
> > >
> > > Yes, we sometimes sort includes... to avoid patch rejects.
> > >
> > > This is guaranteed to cause rejects.
> > >
> > > If we do sort them, we use "longest-first" usually.
> > >
> >
> > When using random order, I've been told by reviewers that my ordering
> > is wrong [1], but I could not figure out what the "right" ordering is in 
> > order to apply.
> > I did not find any mention of how to sort includes in  
> > Documentation/CodingStyle,
> > but I've seen patches in the kernel that fix random ordering by sorting them
> > alphabetically [2].
> 
> Ok, I have seen this more often:
> 
> https://lkml.org/lkml/2009/3/28/99
> 

I see. I wasn't aware of the inverse Xmas tree ordering.

> > However, I do not feel strongly about this so I can drop this patch.
> 
> Well, its really nit picking.
> 

I was thinking of dropping it since there does not seem to be a consensus on how
to order the includes (or even if we should order them).  In this case, I guess 
it's
up to Dmitry.
I'll use the inverse Xmas tree ordering in next patch set and see if that works 
for him.

Thanks,
Irina

>   
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v7 2/9] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: 08 October, 2015 16:01
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v7 2/9] Input: goodix - reset device at init
> 
> On Thursday 08 October 2015 12:18:37 Tirdea, Irina wrote:
> > > From: Arnd Bergmann [mailto:a...@arndb.de]
> > > Sent: 08 October, 2015 13:54
> > > To: Tirdea, Irina
> > > Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; 
> > > linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> > > ker...@vger.kernel.org; devicet...@vger.kernel.org
> > > Subject: Re: [PATCH v7 2/9] Input: goodix - reset device at init
> > >
> > > On Thursday 08 October 2015 13:19:28 Irina Tirdea wrote:
> > > > After power on, it is recommended that the driver resets the device.
> > > > The reset procedure timing is described in the datasheet and is used
> > > > at device init (before writing device configuration) and
> > > > for power management. It is a sequence of setting the interrupt
> > > > and reset pins high/low at specific timing intervals. This procedure
> > > > also includes setting the slave address to the one specified in the
> > > > ACPI/device tree.
> > > >
> > > > 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).
> > > >
> > > > For reset the driver needs to control the interrupt and
> > > > reset gpio pins (configured through ACPI/device tree). For devices
> > > > that do not have the gpio pins declared, the functionality depending
> > > > on these pins will not be available, but the device can still be used
> > > > with basic functionality.
> > > >
> > > > For both device tree and ACPI, the interrupt gpio pin configuration is
> > > > read from the "irq-gpio" property and the reset pin configuration is
> > > > read from the "reset-gpio" property. For ACPI 5.1, named properties
> > > > can be specified using the _DSD section. If there is no _DSD section
> > > > in the ACPI table, the driver will fall back to using indexed gpio
> > > > pins declared in the _CRS section.
> > >
> > > Would it help to use a plain "gpios" property here to always look
> > > up the lines by index?
> > >
> >
> > The problem with ACPI indexed gpios is that platforms declare the
> > pins in random order. In this case we have some platforms that declare
> > the interrupt pin first and others that declare the reset pin first.
> > There is no way to differentiate between them so the only way to support
> > these platforms is to pick a default and list all exceptions in the driver.
> > My previous implementation did that with indexed gpios and dmi quirks. [1]
> >
> > This can be solved by using named gpios, which are available starting with 
> > ACPI 5.1.
> > In this way we know exactly which is the interrupt pin and which is the 
> > reset pin
> > and we do not need to add any additional exceptions to the driver.
> > However, we still need to support the platforms that are already out there 
> > so
> > we fall back to indexed gpios.
> >
> > [1] https://lkml.org/lkml/2015/9/15/609
> 
> Right.
> 
> > > > +/*
> > > > + * Some platforms specify the gpio pins for interrupt and reset 
> > > > properly
> > > > + * in ACPI, but cannot use the interrupt pin as output due to their 
> > > > specific
> > > > + * HW configuration.
> > > > + */
> > > > +static const struct dmi_system_id goodix_no_gpio_pins_support[] = {
> > > > +#if defined(CONFIG_DMI) && defined(CONFIG_X86)
> > > > +   {
> > > > +   .ident = "Onda v975w",
> > > > +   .matches = {
> > > > +   DMI_MATCH(DMI_BIOS_VENDOR, "American Megatrends 
> > > > Inc."),
> > > > +   DMI_MATCH(DMI_PRODUCT_UUID,
> > > > + 
> > > > "03000200-0400-0500-0006-000700080009"),
> > > > +   DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
> > > 

RE: [PATCH v5 1/9] Input: goodix - sort includes alphabetically

[Tirdea, Irina]

> -Original Message-
> From: Pavel Machek [mailto:pa...@ucw.cz]
> Sent: 04 October, 2015 22:47
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; 
> linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v5 1/9] Input: goodix - sort includes alphabetically
> 
> On Mon 2015-09-07 17:36:15, Irina Tirdea wrote:
> > Signed-off-by: Irina Tirdea <irina.tir...@intel.com>
> > ---
> >  drivers/input/touchscreen/goodix.c | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Don't.
> 
> Yes, we sometimes sort includes... to avoid patch rejects.
> 
> This is guaranteed to cause rejects.
> 
> If we do sort them, we use "longest-first" usually.
> 

When using random order, I've been told by reviewers that my ordering
is wrong [1], but I could not figure out what the "right" ordering is in order 
to apply.
I did not find any mention of how to sort includes in  
Documentation/CodingStyle,
but I've seen patches in the kernel that fix random ordering by sorting them
alphabetically [2].

However, I do not feel strongly about this so I can drop this patch.

Thanks,
Irina

[1] https://lkml.org/lkml/2015/5/28/363
[2] https://patchwork.ozlabs.org/patch/408022/

> >
> > diff --git a/drivers/input/touchscreen/goodix.c 
> > b/drivers/input/touchscreen/goodix.c
> > index e36162b..6ae28c5 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -14,18 +14,18 @@
> >   * Software Foundation; version 2 of the License.
> >   */
> >
> > -#include 
> > +#include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> > -#include 
> > -#include 
> >  #include 
> > -#include 
> > -#include 
> > +#include 
> > +#include 
> > +#include 
> >  #include 
> > +#include 
> >  #include 
> >
> >  struct goodix_ts_data {
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) 
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/5] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: mrgarna...@gmail.com [mailto:mrgarna...@gmail.com] On Behalf Of Carlos 
> Garnacho
> Sent: 30 September, 2015 17:02
> To: Bastien Nocera
> Cc: Tirdea, Irina; linux-input@vger.kernel.org; Cosimo Cecchi; Christian 
> Hergert; linux-ker...@vger.kernel.org; Rob Herring; Pawel
> Moll; Ian Campbell; Kumar Gala; Purdila, Octavian; Dmitry Torokhov; Mark 
> Rutland; devicet...@vger.kernel.org
> Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at init
> 
> Hey,
> 

Hi Carlos and Bastien,

Thanks for your help and for testing these patches!



> >> > I'm not sure how we can detect, and blacklist, those devices. At
> >> > least
> >> > my original device, the Onda v975w, and the WinBook TW100 would
> >> > have
> >> > those problems.
> >> >
> >>
> >> I can use DMI quirks to exclude these devices from using the features
> >> that
> >> depend on the gpio pins. I already have the DMI information for
> >> WinBook TW100
> >> and WinBook TW700. Could you tell me the DMI_SYS_VENDOR and
> >> DMI_PRODUCT_NAME for Onda v975w so I can add it as well?
> >
> > I don't have access to the Onda v975w anymore, but let me CC: a few
> > people that could also help with testing.
> >
> > Carlos, Cosimo, Christian, there's a patchset for you to test on the
> > Onda v975w at:
> > https://github.com/hadess/gt9xx/commits/irina-tirdea
> >
> > Doing an "rmmod goodix ; insmod ./goodix_backport.ko" should be enough
> > to test whether the patch set works. If it doesn't work correctly,
> > you'll need to reboot the machine, swap the irq_idx and rst_idx values
> > at:
> > https://github.com/hadess/gt9xx/commit/c27de79f494c2b2e7a198ff4d27976ae93669dbd#diff-
> dddc2849e36327439530f3e2faacec4fR321
> >
> > and try again.
> 
> Unswapped does fail with:
> 
> sep 30 15:37:29 tablet kernel: Goodix-TS i2c-GDIX1001:00: i2c test
> failed attempt 1: -121
> sep 30 15:37:29 tablet kernel: Goodix-TS i2c-GDIX1001:00: i2c test
> failed attempt 2: -121
> sep 30 15:37:29 tablet kernel: Goodix-TS i2c-GDIX1001:00: I2C
> communication failure: -121
> sep 30 15:37:29 tablet kernel: Goodix-TS: probe of i2c-GDIX1001:00
> failed with error -121
> 
> Swapping the values triggers some errors:
> 
> sep 30 15:48:17 tablet kernel: [ cut here ]
> sep 30 15:48:17 tablet kernel: WARNING: CPU: 1 PID: 2341 at
> drivers/pinctrl/intel/pinctrl-baytrail.c:342
> byt_gpio_direction_output+0xa1/0xb0()
> sep 30 15:48:17 tablet kernel: Potential Error: Setting GPIO with
> direct_irq_en to output
> sep 30 15:48:17 tablet kernel: Modules linked in:
> sep 30 15:48:17 tablet kernel:  goodix_backport(OE+) r8723bs(OE) nfsd
> lockd grace sunrpc [last unloaded: goodix]
> sep 30 15:48:17 tablet kernel: CPU: 1 PID: 2341 Comm: insmod Tainted:
> G   OE   4.3.0-rc1+ #36
> sep 30 15:48:17 tablet kernel: Hardware name: To be filled by O.E.M.
> To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 07/25/2014
> sep 30 15:48:17 tablet kernel:    d3f61c08 c13b347f
> d3f61c48 d3f61c38 c10a9e1d c20f10e0
> sep 30 15:48:17 tablet kernel:  d3f61c64 0925 c20f111c 0156
> c13e7ce1 c13e7ce1 f80580b8 f53a4b0c
> sep 30 15:48:17 tablet kernel:  0156 d3f61c50 c10a9e93 0009
> d3f61c48 c20f10e0 d3f61c64 d3f61c78
> sep 30 15:48:17 tablet kernel: Call Trace:
> sep 30 15:48:17 tablet kernel:  [] dump_stack+0x48/0x69
> sep 30 15:48:17 tablet kernel:  [] warn_slowpath_common+0x8d/0xd0
> sep 30 15:48:17 tablet kernel:  [] ?
> byt_gpio_direction_output+0xa1/0xb0
> sep 30 15:48:17 tablet kernel:  [] ?
> byt_gpio_direction_output+0xa1/0xb0
> sep 30 15:48:17 tablet kernel:  [] warn_slowpath_fmt+0x33/0x40
> sep 30 15:48:17 tablet kernel:  [] 
> byt_gpio_direction_output+0xa1/0xb0
> sep 30 15:48:17 tablet kernel:  [] ? byt_gpio_irq_handler+0xb0/0xb0
> sep 30 15:48:17 tablet kernel:  []
> _gpiod_direction_output_raw+0x63/0x350
> sep 30 15:48:17 tablet kernel:  [] gpiod_direction_output+0x2a/0x50
> sep 30 15:48:17 tablet kernel:  [] ? msleep+0x36/0x40
> sep 30 15:48:17 tablet kernel:  [] goodix_reset+0x3e/0x90
> [goodix_backport]
> sep 30 15:48:17 tablet kernel:  []
> goodix_ts_probe+0x12a/0x5aa [goodix_backport]
> sep 30 15:48:17 tablet kernel:  [] ? acpi_device_wakeup+0x7a/0x80
> sep 30 15:48:17 tablet kernel:  [] ? acpi_dev_pm_attach+0x71/0x87
> sep 30 15:48:17 tablet kernel:  [] i2c_device_probe+0x121/0x1d0
> sep 30 15:48:17 tablet kernel:  [] ? _raw_spin_unlock+0x22/0x30
> sep 30 15:48:17 tablet kernel:  [] ?
> goodix_config_cb+0xc0/0xc0 [goodix_backport]
> sep 30 15:48:17 tablet kernel: 

RE: [PATCH v3 1/5] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: Bastien Nocera [mailto:had...@hadess.net]
> Sent: 29 September, 2015 5:04
> To: Tirdea, Irina; linux-input@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org; Rob Herring; Pawel Moll; Ian Campbell; 
> Kumar Gala; Purdila, Octavian; Dmitry Torokhov; Mark
> Rutland; devicet...@vger.kernel.org
> Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at init
> 
> On Fri, 2015-09-25 at 21:04 +, Tirdea, Irina wrote:
> >

> >
> > The warning from your dmesg output will not cause probe to fail.
> > If you look at the code for byt_gpio_direction_output, it will just
> > print
> > a warning and continue [1]:
> > WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
> > "Potential Error: Setting GPIO with direct_irq_en to
> > output");
> > I thought probe finishes successfully, but due to the warning in
> > dmesg you
> > are not sure whether the IRQ GPIO pin can be used as output.
> > If probe fails, it must be for another reason than the direct_irq_en
> > warning.
> >
> > > Would you have a patch for me to test that would bypass this error,
> > > or
> > > at least fallback gracefully to not resetting, not probing GPIOs if
> > > they're badly setup?
> >
> > If the driver fails to initialize the GPIOs, it will at least print
> > some
> > "Failed to get GPIO" warnings in dmesg. Do you have such messages in
> > dmesg or any additional information on why probe fails?
> >
> > The current code will ignore GPIOs if they are not defined in ACPI
> > (see the check for -ENOENT), but does not ignore other error codes.
> > If you want to bypass all GPIO errors, you can use the code below.
> 
> The failure isn't there, it's when running goodix_i2c_test():
> Sep 25 16:39:20 winbook kernel: Goodix-TS i2c-GDIX1001:00: i2c test failed 
> attempt 1: -121
> Sep 25 16:39:20 winbook kernel: Goodix-TS i2c-GDIX1001:00: i2c test failed 
> attempt 2: -121
> Sep 25 16:39:20 winbook kernel: Goodix-TS i2c-GDIX1001:00: I2C communication 
> failure: -121
> Sep 25 16:39:20 winbook kernel: Goodix-TS: probe of i2c-GDIX1001:00 failed 
> with error -121
> 

Are you using v6 of the patches? There was an issue with reset that Aleksei 
reported
and was fixed in v6 (although he had a different i2c error and a different 
scenario).

> The GPIO setup seems to work (bar the warnings), and the reset as well,
> but then the device fails to communicate. Likely a fallout from the
> reset actually failing.
> 
> Swapping around the RST and INT pins leads to the same problem. Either
> this device's GPIO PINs aren't actually functional, and the firmware
> contains garbage, or something else is wrong.
> 

I agree. Either the interrupt pin cannot be used as output in your configuration
or there are some specifics in the ACPI tables that prevent using these pins. 

> I'm not sure how we can detect, and blacklist, those devices. At least
> my original device, the Onda v975w, and the WinBook TW100 would have
> those problems.
> 

I can use DMI quirks to exclude these devices from using the features that
depend on the gpio pins. I already have the DMI information for WinBook TW100
and WinBook TW700. Could you tell me the DMI_SYS_VENDOR and
DMI_PRODUCT_NAME for Onda v975w so I can add it as well?

Thanks,
Irina

> Cheers

RE: [PATCH v3 1/5] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: Bastien Nocera [mailto:had...@hadess.net]
> Sent: 25 September, 2015 17:44
> To: Tirdea, Irina; linux-input@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org; Rob Herring; Pawel Moll; Ian Campbell; 
> Kumar Gala; Purdila, Octavian; Dmitry Torokhov; Mark
> Rutland; devicet...@vger.kernel.org
> Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at init
> 
> On Thu, 2015-09-10 at 14:04 +, Tirdea, Irina wrote:
> >
> > > -Original Message-
> > > From: Bastien Nocera [mailto:had...@hadess.net]
> > > Sent: 09 September, 2015 20:03
> > > To: Tirdea, Irina; linux-input@vger.kernel.org
> > > Cc: linux-ker...@vger.kernel.org; Rob Herring; Pawel Moll; Ian
> > > Campbell; Kumar Gala; Purdila, Octavian; Dmitry Torokhov; Mark
> > > Rutland; devicet...@vger.kernel.org
> > > Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at init
> > >
> > > On Thu, 2015-07-30 at 11:27 +, Tirdea, Irina wrote:
> > > > I can send some additional patches that will simplify testing the
> > > > configuration update to the Goodix device. I think this feature
> > > > is
> > > > the easiest
> > > > to test so we can determine if writing to the interrupt pin
> > > > actually
> > > > works.
> > > > However, even if it is a BIOS problem and the code will work, the
> > > > warning
> > > > will still be printed in dmesg.
> > >
> > >
> > > Somehow missed this mail before replying to the current patchset.
> > > I'd
> > > be fine with that, though it's still not clear to me whether the
> > > BIOS/hardware is at fault, or the code that's being added to the
> > > driver
> > > ;)
> > >
> >
> > The reset procedure is described in the Goodix GT911 datasheet [1]
> > and is
> > used for power-on reset and power management. The power-on sequence
> > is described in chapter 6.1. I2C Timing, in the Power-on Timing
> > diagram.
> > The sequence for putting the device to sleep is described in chapter
> > 7.1. Operating Modes, c) Sleep mode. These sequences use the
> > interrupt
> > pin as output.
> >
> > The warning you mentioned comes from the following code in the goodix
> > driver, which sets the interrupt to be used as output:
> >
> > +   error = gpiod_direction_output(ts->gpiod_int, ts->client-
> > >addr == 0x14);
> >
> > The gpiod_direction_output() call ends up in
> > drivers/pinctrl/intel/pinctrl-baytrail.c:
> > /*
> >  * Before making any direction modifications, do a check if gpio
> >  * is set for direct IRQ.  On baytrail, setting GPIO to output does
> >  * not make sense, so let's at least warn the caller before they
> > shoot
> >  * themselves in the foot.
> >  */
> > WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
> > "Potential Error: Setting GPIO with direct_irq_en to output");
> >
> > So the problem comes from using the gpio interrupt pin as output,
> > which
> > should not work on Baytrail if BYT_DIRECT_IRQ_EN is set by BIOS.
> > The above warning is introduced and discussed in [2] and [3]. As I
> > mentioned,
> > this could be a real HW issue or the BIOS sets BYT_DIRECT_IRQ_EN when
> > it should not. I have also tested these patches on a Baytrail
> > plarform
> > (that uses the same pinctrl driver), but I did not see any issues
> > since
> > BYT_DIRECT_IRQ_EN is not set in my case for the interrupt gpio pin.
> 
> Do we have a way to work-around this in the GPIO driver?
> 
> > To determine if using the interrupt pin as output works, you can test
> > updating
> > the goodix configuration [4].
> 
> Right, the problem being that it's a later patch in the branch, and
> that the driver will fail to probe if there's an error from the GPIO
> call.
> 

The warning from your dmesg output will not cause probe to fail.
If you look at the code for byt_gpio_direction_output, it will just print
a warning and continue [1]:
WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
"Potential Error: Setting GPIO with direct_irq_en to output");
I thought probe finishes successfully, but due to the warning in dmesg you
are not sure whether the IRQ GPIO pin can be used as output.
If probe fails, it must be for another reason than the direct_irq_en warning.

> Would you have a patch for me to test that would bypass this error, or
> at least fallback gracefully to not resetting, not probing GPIOs if
> they're badly setup?

If the driver fails to i

RE: [PATCH v6 4/9] Input: goodix - write configuration data to device

[Tirdea, Irina]

> -Original Message-
> From: Bastien Nocera [mailto:had...@hadess.net]
> Sent: 25 September, 2015 17:41
> To: Tirdea, Irina; Dmitry Torokhov; Aleksei Mamlin; 
> linux-input@vger.kernel.org
> Cc: Mark Rutland; Purdila, Octavian; linux-ker...@vger.kernel.org; 
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v6 4/9] Input: goodix - write configuration data to device
> 
> On Tue, 2015-09-15 at 17:31 +0300, Irina Tirdea wrote:
> > Goodix devices can be configured by writing custom data to the device
> > at
> > init. The configuration data is read with request_firmware from
> > "goodix__cfg.bin", where  is the product id read from the
> > device
> > (e.g.: goodix_911_cfg.bin for Goodix GT911, goodix_9271_cfg.bin for
> > GT9271).
> >
> > The configuration information has a specific format described in the
> > Goodix
> > datasheet. It includes X/Y resolution, maximum supported touch
> > points,
> > interrupt flags, various sesitivity factors and settings for advanced
> 
> sensitivity.

Will fix this in next version of the patches. I will wait for you reply on
the gpio issue before sending a new version.

Thanks,
Irina

N�r��yb�X��ǧv�^�)޺{.n�+{��zn�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥

RE: [PATCH v3] Add generic driver for Silead tochscreens

[Tirdea, Irina]

> -Original Message-
> From: linux-input-ow...@vger.kernel.org 
> [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Robert Dolca
> Sent: 26 August, 2015 0:32
> To: linux-input@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org; Dmitry Torokhov; Henrik Rydberg; Gregor 
> Riepl; Dolca, Robert
> Subject: [PATCH v3] Add generic driver for Silead tochscreens
> 
> This driver adds support for Silead touchscreens. It has been tested
> with GSL1680 and GSL3680 touch panels.
> 
> It supports ACPI and device tree enumeration. Screen resolution,
> the maximum number of fingers supported and firmware name are
> configurable using ACPI/DT properties.
> 
> If the device properties are not present it falls back to using defaults:
>  - x 4095
>  - y 4095
>  - max fingers 10
>  - firmware name [HID/name].fw
> 
> If there is no named GPIO for power it falls back to using an indexed GPIO
> and it requests the GPIO pin with index 1. If there isn't one it disables
> PM support.
> 
> All the hardware variants tested report finger id 0 for all fingers so
> the finger tracking is done using the input subsystem's slot assignment.
> 
> Signed-off-by: Robert Dolca 

Hi Robert,

The code looks good, just a couple of comments below.

> ---
> Changes since v2
> - removed device properties requirements
> - max x and y default to 4095
> - max fingers default to 10
> - firmware name uses the HID / device name
> - power named GPIO optional with fallback to indexed GPIO
>   (without it there is no pm support in the driver)
> - finger tracking in the kernel using slot assignment
> - add device property for x/y inverting and xy swaping
> 
> Changes since v1
> - changed device tree properties names
> - removed cast for `void *id`
> - removed ifdef from suspend and resume and use __maybe_unused
> - remove ifdef from ACPI_PTR
> - renamed ret to error
> - removed input_set_capability for EV_ABS
> - fixed endianess issues
> - added mask for y in order to use only 12 bits
> - using the 4 MSb for touch ID instead of LSb (bug)
> - using the 4 LSB for X instead of MSb (bug)
> 
> 
> 
>  drivers/input/touchscreen/Kconfig  |  12 +
>  drivers/input/touchscreen/Makefile |   1 +
>  drivers/input/touchscreen/silead.c | 635 
> +
>  3 files changed, 648 insertions(+)
>  create mode 100644 drivers/input/touchscreen/silead.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig 
> b/drivers/input/touchscreen/Kconfig
> index 80f6386..05fda4a 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1027,4 +1027,16 @@ config TOUCHSCREEN_ZFORCE
> To compile this driver as a module, choose M here: the
> module will be called zforce_ts.
> 
> +config TOUCHSCREEN_SILEAD
> + tristate "Silead I2C touchscreen"
> + depends on I2C
> + help
> +   Say Y here if you have the Silead touchscreen connected to
> +   your system.
> +
> +   If unsure, say N.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called silead.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile 
> b/drivers/input/touchscreen/Makefile
> index 44deea7..2c6beaa 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)   += w90p910_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_SX8654) += sx8654.o
>  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)   += tps6507x-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
> +obj-$(CONFIG_TOUCHSCREEN_SILEAD) += silead.o
> diff --git a/drivers/input/touchscreen/silead.c 
> b/drivers/input/touchscreen/silead.c
> new file mode 100644
> index 000..5339f93
> --- /dev/null
> +++ b/drivers/input/touchscreen/silead.c
> @@ -0,0 +1,635 @@
> +/* -
> + * Copyright (C) 2014-2015, Intel Corporation
> + *
> + * Derived from:
> + *  gslX68X.c
> + *  Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
> + *
> + *  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.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + * - 
> */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SILEAD_TS_NAME "silead_ts"
> +
> +#define SILEAD_REG_RESET 0xE0
> +#define SILEAD_REG_DATA  0x80
> 

RE: [PATCH v5 3/9] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: Aleksei Mamlin [mailto:mamli...@gmail.com]
> Sent: 15 September, 2015 12:48
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; Mark 
> Rutland; Purdila, Octavian; linux-ker...@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v5 3/9] Input: goodix - reset device at init
> 
> On Mon,  7 Sep 2015 17:36:17 +0300
> Irina Tirdea <irina.tir...@intel.com> wrote:
> 
> > After power on, it is recommended that the driver resets the device.
> > The reset procedure timing is described in the datasheet and is used
> > at device init (before writing device configuration) and
> > for power management. It is a sequence of setting the interrupt
> > and reset pins high/low at specific timing intervals. This procedure
> > also includes setting the slave address to the one specified in the
> > ACPI/device tree.
> >
> > 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).
> >
> > For reset the driver needs to control the interrupt and
> > reset gpio pins (configured through ACPI/device tree). For devices
> > that do not have the gpio pins declared, the functionality depending
> > on these pins will not be available, but the device can still be used
> > with basic functionality.
> >
> > Signed-off-by: Octavian Purdila <octavian.purd...@intel.com>
> > Signed-off-by: Irina Tirdea <irina.tir...@intel.com>
> > ---
> >  .../bindings/input/touchscreen/goodix.txt  |   5 +
> >  drivers/input/touchscreen/goodix.c | 136 
> > +
> >  2 files changed, 141 insertions(+)
> >

> > +/**
> > + * goodix_reset - Reset device during power on
> > + *
> > + * @ts: goodix_ts_data pointer
> > + */
> > +static int goodix_reset(struct goodix_ts_data *ts)
> > +{
> > +   int error;
> > +
> > +   /* begin select I2C slave addr */
> > +   error = gpiod_direction_output(ts->gpiod_rst, 0);
> > +   if (error)
> > +   return error;
> > +   msleep(20); /* T2: > 10ms */
> > +   /* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> > +   error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
> > +   if (error)
> > +   return error;
> > +   usleep_range(100, 2000);/* T3: > 100us */
> > +   error = gpiod_direction_output(ts->gpiod_rst, 1);
> > +   if (error)
> > +   return error;
> > +   usleep_range(6000, 1);  /* T4: > 5ms */
> > +   /* end select I2C slave addr */
> > +   error = gpiod_direction_input(ts->gpiod_rst);
> > +   if (error)
> > +   return error;
> > +   return goodix_int_sync(ts);
> > +}
> > +

> >  /**
> >   * goodix_read_config - Read the embedded configuration of the panel
> >   *
> > @@ -419,6 +542,19 @@ static int goodix_ts_probe(struct i2c_client *client,
> >
> > ts->cfg_len = goodix_get_cfg_len(id_info);
> >
> > +   error = goodix_get_gpio_config(ts, id);
> > +   if (error)
> > +   return error;
> > +
> > +   if (ts->gpiod_int && ts->gpiod_rst) {
> > +   /* reset the controller */
> > +   error = goodix_reset(ts);
> > +   if (error) {
> > +   dev_err(>dev, "Controller reset failed.\n");
> > +   return error;
> > +   }
> > +   }
> > +
> 
> On devices with devicetree, such as ARM tablets, we can set I2C address via 
> DT, so driver should reset controller and set right address.
> If we don't do this we get "I2C communication failure: -6".
>

This is exactly what this patch tries to do. The address set in ACPI or DT will 
be available
in ts->client->addr. The reset code checks for the address set by ACPI/DT and 
configures
the device to use that address.
 
> Also, most of touchscreen drivers tries to reset controllers before start 
> communicating, so we need do the same.

Good catch! goodix_reset should be called before goodix_i2c_test. I'll send a 
new version with this fix.

Thanks,
Irina

> 
> > goodix_read_config(ts);
> >
> > error = goodix_request_input_dev(ts, version_info, id_info);
> > --
> > 1.9.1
> >
> 
> 
> --
> Thanks and regards,
> Aleksei Mamlin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 1/5] Input: goodix - reset device at init

[Tirdea, Irina]

> -Original Message-
> From: Bastien Nocera [mailto:had...@hadess.net]
> Sent: 09 September, 2015 20:03
> To: Tirdea, Irina; linux-input@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org; Rob Herring; Pawel Moll; Ian Campbell; 
> Kumar Gala; Purdila, Octavian; Dmitry Torokhov; Mark
> Rutland; devicet...@vger.kernel.org
> Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at init
> 
> On Thu, 2015-07-30 at 11:27 +, Tirdea, Irina wrote:
> > I can send some additional patches that will simplify testing the
> > configuration update to the Goodix device. I think this feature is
> > the easiest
> > to test so we can determine if writing to the interrupt pin actually
> > works.
> > However, even if it is a BIOS problem and the code will work, the
> > warning
> > will still be printed in dmesg.
> 
> 
> Somehow missed this mail before replying to the current patchset. I'd
> be fine with that, though it's still not clear to me whether the
> BIOS/hardware is at fault, or the code that's being added to the driver
> ;)
> 

The reset procedure is described in the Goodix GT911 datasheet [1] and is
used for power-on reset and power management. The power-on sequence
is described in chapter 6.1. I2C Timing, in the Power-on Timing diagram.
The sequence for putting the device to sleep is described in chapter
7.1. Operating Modes, c) Sleep mode. These sequences use the interrupt
pin as output.

The warning you mentioned comes from the following code in the goodix
driver, which sets the interrupt to be used as output:

+   error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);

The gpiod_direction_output() call ends up in 
drivers/pinctrl/intel/pinctrl-baytrail.c:
/*
 * Before making any direction modifications, do a check if gpio
 * is set for direct IRQ.  On baytrail, setting GPIO to output does
 * not make sense, so let's at least warn the caller before they shoot
 * themselves in the foot.
 */
WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
"Potential Error: Setting GPIO with direct_irq_en to output");

So the problem comes from using the gpio interrupt pin as output, which 
should not work on Baytrail if BYT_DIRECT_IRQ_EN is set by BIOS.
The above warning is introduced and discussed in [2] and [3]. As I mentioned,
this could be a real HW issue or the BIOS sets BYT_DIRECT_IRQ_EN when
it should not. I have also tested these patches on a Baytrail plarform
(that uses the same pinctrl driver), but I did not see any issues since
BYT_DIRECT_IRQ_EN is not set in my case for the interrupt gpio pin.

To determine if using the interrupt pin as output works, you can test updating
the goodix configuration [4]. Normally if something goes wrong with the reset
procedure, the configuration will not be updated. To make that easier, I have
already included a sysfs interface to dump the current configuration [5] and
a script to help generate a new configuration [6] (with details on how to use it
in the commit message). Please let me know if you need something more to
test this.

Thanks,
Irina

[1] 
https://drive.google.com/a/intel.com/folderview?id=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg
[2] https://lkml.org/lkml/2014/6/2/654
[3] https://lkml.org/lkml/2014/9/18/129
[4] https://lkml.org/lkml/2015/9/7/339
[5] https://lkml.org/lkml/2015/9/7/337
[6] https://lkml.org/lkml/2015/7/31/706

N�r��yb�X��ǧv�^�)޺{.n�+{��zn�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥

RE: [PATCH v4 0/8] Goodix touchscreen enhancements

[Tirdea, Irina]

 -Original Message-
 From: Bastien Nocera [mailto:had...@hadess.net]
 Sent: 03 August, 2015 13:30
 To: Tirdea, Irina; Dmitry Torokhov; linux-input@vger.kernel.org
 Cc: Mark Rutland; Purdila, Octavian; linux-ker...@vger.kernel.org; 
 devicet...@vger.kernel.org
 Subject: Re: [PATCH v4 0/8] Goodix touchscreen enhancements
 
 On Fri, 2015-07-31 at 20:42 +0300, Irina Tirdea wrote:
  Add several enhancements to the Goodix touchscreen driver.
 
 It's going to be a couple of weeks before I have the time to sit down
 and test most of those. Also note that I'll be using a different device
 (WinBook TW100), though the new owner of the Onda v975w should be able
 to run tests as well.
 

Thanks for letting me know, Bastien! I might send one more version in the 
meantime
if I have more updates, so please use the latest version when you test them.

Thanks,
Irina 

 Cheers

RE: [PATCH v3 3/5] Input: goodix - add power management support

[Tirdea, Irina]

 -Original Message-
 From: linux-input-ow...@vger.kernel.org 
 [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Bastien Nocera
 Sent: 30 June, 2015 18:56
 To: Tirdea, Irina; Dmitry Torokhov; Mark Rutland; 
 linux-input@vger.kernel.org; devicet...@vger.kernel.org
 Cc: linux-ker...@vger.kernel.org; Rob Herring; Pawel Moll; Ian Campbell; 
 Kumar Gala; Purdila, Octavian
 Subject: Re: [PATCH v3 3/5] Input: goodix - add power management support
 
 On Mon, 2015-06-29 at 19:28 +0300, Irina Tirdea wrote:
  Implement suspend/resume for goodix driver.
 
  The suspend and resume process uses the gpio pins.
  If the device ACPI/DT information does not declare gpio pins,
  suspend/resume will not be available for these devices.
 
  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).
 
 How can I functionally test this?

We are testing the power management feature by putting the system
in suspend and measuring the power consumed by the touchscreen
using HW measurement tools.

I can send a set of patches that export an interface to put
the touchscreen into suspend from sysfs (something similar to
what ads7846 has). In this way you can test what happens to
the touchscreen alone without putting the entire system into
suspend.

When suspending the device you will see that there are no events
received from the touchscreen (the interrupt is unregistered before
suspending the device). To actually check that the device
has been suspended and is not consuming power you need to measure
the power consumed by the touchscreen. To do that entirely from
software, you could compare the power consumed by the battery when the
touchscreen is powered on with the power consumed by the battery
when the touchscreen is powered off (batteries export interfaces in
sysfs that can be used for that). The difference should match the
device specifications. I am doing some tests to see how that works,
but I am not getting the expected results so far (seems there are other
components that affect the measurements). I will look more into this and
come back with details if you think this would work for you.

Thanks,
Irina

 --
 To unsubscribe from this list: send the line unsubscribe linux-input in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
N�r��yb�X��ǧv�^�)޺{.n�+{��zn�)w*jg����ݢj/���z�ޖ��2�ޙ�)ߡ�a�����G���h��j:+v���w��٥

RE: [PATCH 8/9] input: goodix: add support for ESD

[Tirdea, Irina]

 -Original Message-
 From: linux-input-ow...@vger.kernel.org 
 [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Tirdea, Irina
 Sent: 08 June, 2015 17:29
 To: Dmitry Torokhov
 Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: RE: [PATCH 8/9] input: goodix: add support for ESD
 
 
 
  -Original Message-
  From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
  Sent: 05 June, 2015 19:46
  To: Tirdea, Irina
  Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; 
  devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
  Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
 
  On Fri, Jun 05, 2015 at 04:37:49PM +, Tirdea, Irina wrote:
  
  
-Original Message-
From: Bastien Nocera [mailto:had...@hadess.net]
Sent: 04 June, 2015 15:58
To: Tirdea, Irina
Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; 
devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
   
On Thu, 2015-05-28 at 14:26 +, Tirdea, Irina wrote:

  -Original Message-
  From: linux-input-ow...@vger.kernel.org [mailto:
  linux-input-ow...@vger.kernel.org] On Behalf Of Mark Rutland
  Sent: 28 May, 2015 16:24
  To: Tirdea, Irina
  Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org;
  devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
  Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
 
  On Thu, May 28, 2015 at 01:47:44PM +0100, 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
   esd-recovery-timeout-ms ACPI/DT property. If it is set to 0,
   ESD protection is disabled.
  
   Signed-off-by: Irina Tirdea irina.tir...@intel.com
   ---
.../bindings/input/touchscreen/goodix.txt  |   4 +
drivers/input/touchscreen/goodix.c | 106
   -
2 files changed, 106 insertions(+), 4 deletions(-)
  
   diff --git
   a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
   index 9e4ff69..9132ee0 100644
   ---
   a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
   +++
   b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
   @@ -19,6 +19,10 @@ Optional properties:
  
 - device-config : device configuration information
   (specified as byte
   array). Maximum size is 240 bytes.
   + - 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.
 
  This sounds like software configuration rather than HW description.
  Is
  there any reason this needs to be a DT property?
 

 Although this enables a software feature, it depends on the platform
 if electrostatic discharge
 protection should be enabled or not. Some platform designs handle ESD
 better and do not need
 the SW mechanism, so the property can be used to disable it.
 
  Even though it depends on the platform it describes software function,
  not hardware. Since it is not necessary for starting the device maybe we
  should indeed export it through sysfs and userspace board code should
  activate it as needed?
 
 Sure, I can do it through sysfs.
 The only confusing part is that the tsc2005 touchscreen driver uses the
 device tree property esd-recovery-timeout-ms.
 
 
  I'll leave the decision to DT folks here.
 
 
 OK. I will keep this version of the esd patch in v2 and wait for an answer
 from DT folks so I know exactly how to handle it.
 

Hi Mark, Dmitry,

We have been experimenting with the ESD property exported through sysfs
and found it difficult to use. We are testing this on Android tablets which
do not provide userspace code customized for each subvariant of the same
platform. So the only way for us to set specific parameters for each subvariant
is to specify this in ACPI or DT. 

I could keep the sysfs interface that exports the esd_timeout attribute, but
initialize its value to what ACPI/DT provides. As I mentioned before, I could
separate the esd enabling from the actual timeout if that makes more sense.

What do you think?

Thanks,
Irina


 Thanks,
 Irina
 
  Thanks.
 
  --
  Dmitry
 --
 To unsubscribe from this list: send the line unsubscribe

RE: [PATCH v3 1/5] Input: goodix - reset device at init

[Tirdea, Irina]

 -Original Message-
 From: Bastien Nocera [mailto:had...@hadess.net]
 Sent: 30 June, 2015 18:57
 To: Tirdea, Irina; Dmitry Torokhov; Mark Rutland; 
 linux-input@vger.kernel.org; devicet...@vger.kernel.org
 Cc: linux-ker...@vger.kernel.org; Rob Herring; Pawel Moll; Ian Campbell; 
 Kumar Gala; Purdila, Octavian
 Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at init
 
 On Mon, 2015-06-29 at 19:28 +0300, Irina Tirdea wrote:
  After power on, it is recommended that the driver resets the device.
  The reset procedure timing is described in the datasheet and is used
  at device init (before writing device configuration) and
  for power management. It is a sequence of setting the interrupt
  and reset pins high/low at specific timing intervals. This procedure
  also includes setting the slave address to the one specified in the
  ACPI/device tree.
 
  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).
 
  For reset the driver needs to control the interrupt and
  reset gpio pins (configured through ACPI/device tree). For devices
  that do not have the gpio pins declared, the functionality depending
  on these pins will not be available, but the device can still be used
  with basic functionality.
 
 
 I'm having a little trouble with this first patch, on a 4.2 pre Linus
 tree and on a 4.1 kernel.
 
 [6.720214] [ cut here ]
 [6.720230] WARNING: CPU: 2 PID: 475 at 
 drivers/pinctrl/intel/pinctrl-baytrail.c:338 
 byt_gpio_direction_output+0x97/0xa0()
 [6.720234] Potential Error: Setting GPIO with direct_irq_en to output
 [6.720238] Modules linked in:
 [6.720241]  regmap_i2c intel_soc_dts_iosf int340x_thermal_zone soundcore 
 industrialio battery iosf_mbi acpi_thermal_rel
 dell_smo8800 snd_soc_sst_acpi goodix_backport(OE+) i2c_hid acpi_pad ac 
 rfkill_gpio i2c_designware_platform rfkill
 pwm_lpss_platform pwm_lpss i2c_designware_core nfsd auth_rpcgss nfs_acl lockd 
 grace sunrpc i915 mmc_block i2c_algo_bit
 drm_kms_helper drm video sdhci_acpi sdhci mmc_core
 [6.720292] CPU: 2 PID: 475 Comm: systemd-udevd Tainted: GW  OE   
 4.2.0-0.rc0.git2.2.fc22.i686 #1
 [6.720295] Hardware name: To be filled by O.E.M. To be filled by 
 O.E.M./Aptio CRB, BIOS 5.6.5 07/25/2014
 [6.720299]  c0d39967 11197204  f6d37bc8 c0a9ce3c f6d37c08 
 f6d37bf8 c045c677
 [6.720311]  c0cb62a4 f6d37c28 01db c0cb62e0 0152 c073bfc7 
 c073bfc7 f7c580b8
 [6.720321]  f441409c f7c580b0 f6d37c14 c045c6ee 0009 f6d37c08 
 c0cb62a4 f6d37c28
 [6.720331] Call Trace:
 [6.720342]  [c0a9ce3c] dump_stack+0x41/0x52
 [6.720349]  [c045c677] warn_slowpath_common+0x87/0xc0
 [6.720355]  [c073bfc7] ? byt_gpio_direction_output+0x97/0xa0
 [6.720360]  [c073bfc7] ? byt_gpio_direction_output+0x97/0xa0
 [6.720365]  [c045c6ee] warn_slowpath_fmt+0x3e/0x60
 [6.720370]  [c073bfc7] byt_gpio_direction_output+0x97/0xa0
 [6.720376]  [c073bf30] ? byt_gpio_irq_handler+0xc0/0xc0
 [6.720382]  [c073e939] _gpiod_direction_output_raw+0x59/0x1c0
 [6.720388]  [c0aa202d] ? _raw_spin_unlock_irqrestore+0xd/0x10
 [6.720393]  [c073bb73] ? byt_gpio_direction_input+0x43/0x50
 [6.720398]  [c073bb30] ? byt_gpio_set+0x70/0x70
 [6.720404]  [c073eb0a] gpiod_direction_output+0x2a/0x50
 [6.720413]  [f7fe768b] goodix_ts_probe+0x2fb/0x5fd [goodix_backport]
 [6.720422]  [c0909691] i2c_device_probe+0x101/0x1b0
 [6.720428]  [c0612e95] ? sysfs_create_link+0x25/0x50
 [6.720436]  [f7fe7390] ? goodix_ts_irq_handler+0x1f0/0x1f0 
 [goodix_backport]
 [6.720442]  [c0819c52] ? driver_sysfs_add+0x62/0x80
 [6.720448]  [c081a56a] driver_probe_device+0x1ca/0x460
 [6.720454]  [c081a800] ? driver_probe_device+0x460/0x460
 [6.720461]  [c078d0b1] ? acpi_driver_match_device+0x36/0x3f
 [6.720467]  [c081a879] __driver_attach+0x79/0x80
 [6.720473]  [c081a800] ? driver_probe_device+0x460/0x460
 [6.720478]  [c0818467] bus_for_each_dev+0x57/0xa0
 [6.720484]  [c0819dfe] driver_attach+0x1e/0x20
 [6.720489]  [c081a800] ? driver_probe_device+0x460/0x460
 [6.720494]  [c08199bf] bus_add_driver+0x1ef/0x290
 [6.720501]  [f7ca7000] ? 0xf7ca7000
 [6.720506]  [f7ca7000] ? 0xf7ca7000
 [6.720512]  [c081b07d] driver_register+0x5d/0xf0
 [6.720518]  [c090a3ca] i2c_register_driver+0x2a/0xa0
 [6.720524]  [c040046f] ? do_one_initcall+0x9f/0x200
 [6.720531]  [f7ca7012] goodix_ts_driver_init+0x12/0x1000 
 [goodix_backport]
 [6.720536]  [c040047a] do_one_initcall+0xaa/0x200
 [6.720541]  [f7ca7000] ? 0xf7ca7000
 [6.720547]  [c05801c8] ? free_vmap_area_noflush+0x38/0x80
 [6.720554]  [c0594b95] ? kmem_cache_alloc_trace+0x175/0x1f0
 [6.720560]  [c0a9c64f] ? do_init_module+0x21/0x1a1
 [6.720565]  [c0a9c64f] ? do_init_module+0x21/0x1a1
 [6.720572]  [c0a9c67e] do_init_module+0x50/0x1a1

RE: [PATCH v2 5/8] input: goodix: write configuration data to device

[Tirdea, Irina]

 -Original Message-
 From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
 Sent: 23 June, 2015 21:59
 To: Tirdea, Irina
 Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Rob
 Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
 Subject: Re: [PATCH v2 5/8] input: goodix: write configuration data to device
 
 On Tue, Jun 23, 2015 at 01:27:01PM +, Tirdea, Irina wrote:
 
 
   -Original Message-
   From: linux-input-ow...@vger.kernel.org 
   [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Dmitry Torokhov
   Sent: 09 June, 2015 21:14
   To: Tirdea, Irina
   Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; 
   devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Rob
   Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
   Subject: Re: [PATCH v2 5/8] input: goodix: write configuration data to 
   device
  
   Hi Irina,
  
 
  Hi Dmitry,
 
   On Mon, Jun 08, 2015 at 05:37:50PM +0300, Irina Tirdea wrote:
Goodix devices can be configured by writing custom data to the device at
init. The configuration data is read with request_firmware from
goodix_id_cfg.bin, where id is the product id read from the device
(e.g.: goodix_911_cfg.bin for Goodix GT911, goodix_9271_cfg.bin for
GT9271).
   
The configuration information has a specific format described in the 
Goodix
datasheet. It includes X/Y resolution, maximum supported touch points,
interrupt flags, various sesitivity factors and settings for advanced
features (like gesture recognition).
   
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).
  
   I am afraid that just using request_firmware() in probe() is not the
   best option as it may cause either errors or delays in
   initialization of the driver is compiled into the kernel and tries to
   initialize before filesystem with configuration file is mounted (and
   firmware is not built into the kernel). We can either try switch to
   request_firmware_nowait() or at least document that we need firmware at
   boot.
  
 
  The reason I did not use request_firmware_nowait() is that this 
  configuration data
  includes information needed by probe (x/y resolution, interrupt trigger 
  type, max
  touch num), used in goodix_read_config, goodix_ts_report_touch  and for irq 
  flags
  when requesting the interrupt. I will document that we need this 
  configuration
  firmware at boot in the commit message and add a comment in the code.
  Is there any other documentation I need to update?
 
   
Signed-off-by: Octavian Purdila octavian.purd...@intel.com
Signed-off-by: Irina Tirdea irina.tir...@intel.com
---
 drivers/input/touchscreen/goodix.c | 128 
+
 1 file changed, 128 insertions(+)
   
diff --git a/drivers/input/touchscreen/goodix.c 
b/drivers/input/touchscreen/goodix.c
index c345eb7..1ce9278 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -24,6 +24,7 @@
 #include linux/interrupt.h
 #include linux/slab.h
 #include linux/acpi.h
+#include linux/firmware.h
 #include linux/gpio.h
 #include linux/of.h
 #include asm/unaligned.h
@@ -95,6 +96,39 @@ static int goodix_i2c_read(struct i2c_client *client,
return ret  0 ? ret : (ret != ARRAY_SIZE(msgs) ? -EIO : 0);
 }
   
+/**
+ * goodix_i2c_write - write data to a register of the i2c slave device.
+ *
+ * @client: i2c device.
+ * @reg: the register to write to.
+ * @buf: raw data buffer to write.
+ * @len: length of the buffer to write
+ */
+static int goodix_i2c_write(struct i2c_client *client, u16 reg, const 
u8 *buf,
+   unsigned len)
+{
+   u8 *addr_buf;
+   struct i2c_msg msg;
+   int ret;
+
+   addr_buf = kmalloc(len + 2, GFP_KERNEL);
+   if (!addr_buf)
+   return -ENOMEM;
+
+   addr_buf[0] = reg  8;
+   addr_buf[1] = reg  0xFF;
+   memcpy(addr_buf[2], buf, len);
+
+   msg.flags = 0;
+   msg.addr = client-addr;
+   msg.buf = addr_buf;
+   msg.len = len + 2;
+
+   ret = i2c_transfer(client-adapter, msg, 1);
+   kfree(addr_buf);
+   return ret  0 ? ret : (ret != 1 ? -EIO : 0);
+}
+
 static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 
*data)
 {
int touch_num;
@@ -192,6 +226,95 @@ static irqreturn_t goodix_ts_irq_handler(int irq, 
void *dev_id)
return IRQ_HANDLED;
 }
   
+/**
+ * goodix_check_cfg - Checks if config buffer is valid
+ *
+ * @ts: goodix_ts_data pointer
+ * @fw: firmware

RE: [PATCH v2 4/8] input: goodix: reset device at init

[Tirdea, Irina]

 -Original Message-
 From: Octavian Purdila [mailto:octavian.purd...@intel.com]
 Sent: 23 June, 2015 17:51
 To: Bastien Nocera
 Cc: Tirdea, Irina; Dmitry Torokhov; Mark Rutland; 
 linux-input@vger.kernel.org; devicet...@vger.kernel.org; linux-
 ker...@vger.kernel.org; Rob Herring; Pawel Moll; Ian Campbell; Kumar Gala
 Subject: Re: [PATCH v2 4/8] input: goodix: reset device at init
 
 On Tue, Jun 23, 2015 at 5:12 PM, Bastien Nocera had...@hadess.net wrote:
 
  On Tue, 2015-06-23 at 13:23 +, Tirdea, Irina wrote:
  
-Original Message-
From: linux-input-ow...@vger.kernel.org [mailto:
linux-input-ow...@vger.kernel.org] On Behalf Of Bastien Nocera
Sent: 09 June, 2015 18:53
To: Tirdea, Irina
Cc: Dmitry Torokhov; Mark Rutland; linux-input@vger.kernel.org;
devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Rob
Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
Subject: Re: [PATCH v2 4/8] input: goodix: reset device at init
   
On Tue, 2015-06-09 at 17:34 +0200, Bastien Nocera wrote:
 On Mon, 2015-06-08 at 17:37 +0300, Irina Tirdea wrote:
  After power on, it is recommended that the driver resets the
  device.
  The reset procedure timing is described in the datasheet and is
  used
  at device init (before writing device configuration) and
  for power management. It is a sequence of setting the interrupt
  and reset pins high/low at specific timing intervals. This
  procedure
  also includes setting the slave address to the one specified in
  the
  ACPI/device tree.

 This breaks the touchscreen on my Onda v975w:
 [  239.732858] Goodix-TS i2c-GDIX1001:00: ID 9271, version: 1020
 [  239.732977] Goodix-TS i2c-GDIX1001:00: Failed to get reset
 GPIO:
 -16
 [  239.736071] Goodix-TS: probe of i2c-GDIX1001:00 failed with
 error
 -16

 This is the DSDT for that device:
 https://bugzilla.kernel.org/attachment.cgi?id=149331
   
  
   Oops. That's right - I am using named interrupts which are available
   only for ACPI 5.1, so
   devices already out there will not work.
  
   The problem here is that handling -ENOENT will not help. The gpio
   pins are declared in the
   ACPI DSDT, but are not named. The devm_gpiod_get API will look for
   the named interrupt
   first and fallback to searching by index if not found. Index will be
   0 in both cases, this is why
   the first call will succeed and the second will fail with -EBUSY.
  
   One way to handle this would be to use indexed gpio pins instead of
   named gpio pins:
   e.g. consider the first gpio pin to be the reset pin and the second
   to be the interrupt pin.
   This is used in other drivers as well, e.g. zforce_ts. The problem
   with this approach is that
   any devices that declare the gpio pins in reversed order in the DSDT
   will not work and give
   no warning (the pins will be requested successfully, but some of the
   functionality will not
   work). The DSDT mentioned in
   https://bugzilla.kernel.org/attachment.cgi?id=149331 lists
   the reset pin first, while I am working on some devices that declare
   the interrupt gpio pin
   first.
  
   Another way to handle this is to treat -EBUSY like -ENOENT and not
   use any functionality
   that depends on the gpio pins. Unfortunately, this means we will not
   have suspend, esd and
   custom configs even if the pins are connected on the board and
   available in ACPI(just not
   named).
  
   I would go with the first approach and document the order requested
   for the pins, but I would
   like to hear what you think first. Is there a better way to do this?
  
(Note that this means that I haven't been able to test any
following
patches in that series than 4/8).
  
   Sure, that makes sense. The following patches depend on the gpio pins
   so they would not have
   worked either.
 
  We can apply quirks based on DMI information, so that devices with ACPI
  in different orders will work after applying a quirk (as long as
  there's a way to detect that it's in the wrong order, obviously).
 
 
 I think even using the ACPI id (_HID) should be enough (at least in
 the cases we know so far) to make the difference in how the pins are
 used.

Thanks for the suggestions Bastien and Octavian!
I will use the ACPI ID in v3 considering the ACPI table Bastien has mentioned 
[1].

Thanks,
Irina

[1] https://bugzilla.kernel.org/attachment.cgi?id=149331

RE: [PATCH v2 5/8] input: goodix: write configuration data to device

[Tirdea, Irina]

 -Original Message-
 From: linux-input-ow...@vger.kernel.org 
 [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Dmitry Torokhov
 Sent: 09 June, 2015 21:14
 To: Tirdea, Irina
 Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Rob
 Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
 Subject: Re: [PATCH v2 5/8] input: goodix: write configuration data to device
 
 Hi Irina,
 

Hi Dmitry,

 On Mon, Jun 08, 2015 at 05:37:50PM +0300, Irina Tirdea wrote:
  Goodix devices can be configured by writing custom data to the device at
  init. The configuration data is read with request_firmware from
  goodix_id_cfg.bin, where id is the product id read from the device
  (e.g.: goodix_911_cfg.bin for Goodix GT911, goodix_9271_cfg.bin for
  GT9271).
 
  The configuration information has a specific format described in the Goodix
  datasheet. It includes X/Y resolution, maximum supported touch points,
  interrupt flags, various sesitivity factors and settings for advanced
  features (like gesture recognition).
 
  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).
 
 I am afraid that just using request_firmware() in probe() is not the
 best option as it may cause either errors or delays in
 initialization of the driver is compiled into the kernel and tries to
 initialize before filesystem with configuration file is mounted (and
 firmware is not built into the kernel). We can either try switch to
 request_firmware_nowait() or at least document that we need firmware at
 boot.
 

The reason I did not use request_firmware_nowait() is that this configuration 
data
includes information needed by probe (x/y resolution, interrupt trigger type, 
max
touch num), used in goodix_read_config, goodix_ts_report_touch  and for irq 
flags
when requesting the interrupt. I will document that we need this configuration
firmware at boot in the commit message and add a comment in the code.
Is there any other documentation I need to update?

 
  Signed-off-by: Octavian Purdila octavian.purd...@intel.com
  Signed-off-by: Irina Tirdea irina.tir...@intel.com
  ---
   drivers/input/touchscreen/goodix.c | 128 
  +
   1 file changed, 128 insertions(+)
 
  diff --git a/drivers/input/touchscreen/goodix.c 
  b/drivers/input/touchscreen/goodix.c
  index c345eb7..1ce9278 100644
  --- a/drivers/input/touchscreen/goodix.c
  +++ b/drivers/input/touchscreen/goodix.c
  @@ -24,6 +24,7 @@
   #include linux/interrupt.h
   #include linux/slab.h
   #include linux/acpi.h
  +#include linux/firmware.h
   #include linux/gpio.h
   #include linux/of.h
   #include asm/unaligned.h
  @@ -95,6 +96,39 @@ static int goodix_i2c_read(struct i2c_client *client,
  return ret  0 ? ret : (ret != ARRAY_SIZE(msgs) ? -EIO : 0);
   }
 
  +/**
  + * goodix_i2c_write - write data to a register of the i2c slave device.
  + *
  + * @client: i2c device.
  + * @reg: the register to write to.
  + * @buf: raw data buffer to write.
  + * @len: length of the buffer to write
  + */
  +static int goodix_i2c_write(struct i2c_client *client, u16 reg, const u8 
  *buf,
  +   unsigned len)
  +{
  +   u8 *addr_buf;
  +   struct i2c_msg msg;
  +   int ret;
  +
  +   addr_buf = kmalloc(len + 2, GFP_KERNEL);
  +   if (!addr_buf)
  +   return -ENOMEM;
  +
  +   addr_buf[0] = reg  8;
  +   addr_buf[1] = reg  0xFF;
  +   memcpy(addr_buf[2], buf, len);
  +
  +   msg.flags = 0;
  +   msg.addr = client-addr;
  +   msg.buf = addr_buf;
  +   msg.len = len + 2;
  +
  +   ret = i2c_transfer(client-adapter, msg, 1);
  +   kfree(addr_buf);
  +   return ret  0 ? ret : (ret != 1 ? -EIO : 0);
  +}
  +
   static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
   {
  int touch_num;
  @@ -192,6 +226,95 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void 
  *dev_id)
  return IRQ_HANDLED;
   }
 
  +/**
  + * goodix_check_cfg - Checks if config buffer is valid
  + *
  + * @ts: goodix_ts_data pointer
  + * @fw: firmware config data
  + */
  +static int goodix_check_cfg(struct goodix_ts_data *ts,
  +   const struct firmware *fw)
  +{
  +   int i, raw_cfg_len;
  +   u8 check_sum = 0;
  +
  +   if (fw-size  GOODIX_CONFIG_MAX_LENGTH) {
  +   dev_err(ts-client-dev,
  +   The length of the config buffer array is not correct);
  +   return -EINVAL;
  +   }
  +
  +   raw_cfg_len = fw-size - 2;
  +   for (i = 0; i  raw_cfg_len; i++)
  +   check_sum += fw-data[i];
  +   check_sum = (~check_sum) + 1;
  +   if (check_sum != fw-data[raw_cfg_len]) {
  +   dev_err(ts-client-dev,
  +   The checksum of the config buffer array is not 
  correct);
  +   return -EINVAL;
  +   }
  +
  +   if (fw-data[raw_cfg_len + 1] != 1

RE: [PATCH v2 6/8] input: goodix: add power management support

[Tirdea, Irina]

 -Original Message-
 From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
 Sent: 09 June, 2015 21:16
 To: Tirdea, Irina
 Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Rob
 Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
 Subject: Re: [PATCH v2 6/8] input: goodix: add power management support
 
 On Mon, Jun 08, 2015 at 05:37:51PM +0300, Irina Tirdea wrote:
  Implement suspend/resume for goodix driver.
 
  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).
 
  Signed-off-by: Octavian Purdila octavian.purd...@intel.com
  Signed-off-by: Irina Tirdea irina.tir...@intel.com
  ---
   drivers/input/touchscreen/goodix.c | 88 
  +++---
   1 file changed, 83 insertions(+), 5 deletions(-)
 
  diff --git a/drivers/input/touchscreen/goodix.c 
  b/drivers/input/touchscreen/goodix.c
  index 1ce9278..903c83d 100644
  --- a/drivers/input/touchscreen/goodix.c
  +++ b/drivers/input/touchscreen/goodix.c
  @@ -38,6 +38,7 @@ struct goodix_ts_data {
  unsigned int int_trigger_type;
  struct gpio_desc *gpiod_int;
  struct gpio_desc *gpiod_rst;
  +   unsigned long irq_flags;
   };
 
   #define GOODIX_GPIO_INT_NAME   irq
  @@ -52,6 +53,9 @@ struct goodix_ts_data {
   #define GOODIX_CONFIG_MAX_LENGTH   240
 
   /* Register defines */
  +#define GOODIX_REG_COMMAND 0x8040
  +#define GOODIX_CMD_SCREEN_OFF  0x05
  +
   #define GOODIX_READ_COOR_ADDR  0x814E
   #define GOODIX_REG_CONFIG_DATA 0x8047
   #define GOODIX_REG_ID  0x8140
  @@ -129,6 +133,11 @@ static int goodix_i2c_write(struct i2c_client *client, 
  u16 reg, const u8 *buf,
  return ret  0 ? ret : (ret != 1 ? -EIO : 0);
   }
 
  +static int goodix_i2c_write_u8(struct i2c_client *client, u16 reg, u8 
  value)
  +{
  +   return goodix_i2c_write(client, reg, value, sizeof(value));
  +}
  +
   static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
   {
  int touch_num;
  @@ -226,6 +235,18 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void 
  *dev_id)
  return IRQ_HANDLED;
   }
 
  +static void goodix_free_irq(struct goodix_ts_data *ts)
  +{
  +   devm_free_irq(ts-client-dev, ts-client-irq, ts);
  +}
  +
  +static int goodix_request_irq(struct goodix_ts_data *ts)
  +{
  +   return devm_request_threaded_irq(ts-client-dev, ts-client-irq,
  +NULL, goodix_ts_irq_handler,
  +ts-irq_flags, ts-client-name, ts);
  +}
  +
   /**
* goodix_check_cfg - Checks if config buffer is valid
*
  @@ -547,7 +568,6 @@ static int goodix_ts_probe(struct i2c_client *client,
 const struct i2c_device_id *id)
   {
  struct goodix_ts_data *ts;
  -   unsigned long irq_flags;
  int error;
  u16 version_info, id_info;
 
  @@ -599,10 +619,8 @@ static int goodix_ts_probe(struct i2c_client *client,
  if (error)
  return error;
 
  -   irq_flags = goodix_irq_flags[ts-int_trigger_type] | IRQF_ONESHOT;
  -   error = devm_request_threaded_irq(ts-client-dev, client-irq,
  - NULL, goodix_ts_irq_handler,
  - irq_flags, client-name, ts);
  +   ts-irq_flags = goodix_irq_flags[ts-int_trigger_type] | IRQF_ONESHOT;
  +   error = goodix_request_irq(ts);
  if (error) {
  dev_err(client-dev, request IRQ failed: %d\n, error);
  return error;
  @@ -611,6 +629,65 @@ static int goodix_ts_probe(struct i2c_client *client,
  return 0;
   }
 
  +#ifdef CONFIG_PM_SLEEP
  +static int goodix_suspend(struct device *dev)
 
 Please drop #ifdef CONFIG_PM_SLEEP and annotate with __maybe_unused
 instead.
 
Ok, will fix this.

  +{
  +   struct i2c_client *client = to_i2c_client(dev);
  +   struct goodix_ts_data *ts = i2c_get_clientdata(client);
  +   int ret;
  +
  +   goodix_free_irq(ts);
 
 Why do we need to free IRQ here?
 

The suspend sequence described in Goodix datasheets requires using the irq pin 
as output pin.
If the interrupt is not released, gpiod_direction_output(ts-gpiod_int, 0) will 
fail with the message
tried to set a GPIO tied to an IRQ as output.  Initially I used 
gpiod_lock/unlock_as_irq, but this API
is no longer available (and gpiochip_lock_as_irq is only available for gpio 
chips). Is there a better way
to do this?

  +   /* Output LOW on the INT pin for 5 ms */
  +   ret = gpiod_direction_output(ts-gpiod_int, 0);
  +   if (ret) {
  +   goodix_request_irq(ts);
  +   return ret;
  +   }
  +   usleep_range(5000, 6000);
  +
  +   ret = goodix_i2c_write_u8(ts-client, GOODIX_REG_COMMAND,
  + GOODIX_CMD_SCREEN_OFF);
  +   if (ret) {
  +   dev_err(ts-client-dev, Screen off

RE: [PATCH 8/9] input: goodix: add support for ESD

[Tirdea, Irina]

 -Original Message-
 From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
 Sent: 05 June, 2015 19:46
 To: Tirdea, Irina
 Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
 
 On Fri, Jun 05, 2015 at 04:37:49PM +, Tirdea, Irina wrote:
 
 
   -Original Message-
   From: Bastien Nocera [mailto:had...@hadess.net]
   Sent: 04 June, 2015 15:58
   To: Tirdea, Irina
   Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; 
   devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
   Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
  
   On Thu, 2015-05-28 at 14:26 +, Tirdea, Irina wrote:
   
 -Original Message-
 From: linux-input-ow...@vger.kernel.org [mailto:
 linux-input-ow...@vger.kernel.org] On Behalf Of Mark Rutland
 Sent: 28 May, 2015 16:24
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org;
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 8/9] input: goodix: add support for ESD

 On Thu, May 28, 2015 at 01:47:44PM +0100, 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
  esd-recovery-timeout-ms ACPI/DT property. If it is set to 0,
  ESD protection is disabled.
 
  Signed-off-by: Irina Tirdea irina.tir...@intel.com
  ---
   .../bindings/input/touchscreen/goodix.txt  |   4 +
   drivers/input/touchscreen/goodix.c | 106
  -
   2 files changed, 106 insertions(+), 4 deletions(-)
 
  diff --git
  a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
 b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  index 9e4ff69..9132ee0 100644
  ---
  a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  +++
  b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  @@ -19,6 +19,10 @@ Optional properties:
 
- device-config : device configuration information
  (specified as byte
  array). Maximum size is 240 bytes.
  + - 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.

 This sounds like software configuration rather than HW description.
 Is
 there any reason this needs to be a DT property?

   
Although this enables a software feature, it depends on the platform
if electrostatic discharge
protection should be enabled or not. Some platform designs handle ESD
better and do not need
the SW mechanism, so the property can be used to disable it.
 
 Even though it depends on the platform it describes software function,
 not hardware. Since it is not necessary for starting the device maybe we
 should indeed export it through sysfs and userspace board code should
 activate it as needed?

Sure, I can do it through sysfs.
The only confusing part is that the tsc2005 touchscreen driver uses the
device tree property esd-recovery-timeout-ms.

 
 I'll leave the decision to DT folks here.
 

OK. I will keep this version of the esd patch in v2 and wait for an answer
from DT folks so I know exactly how to handle it.

Thanks,
Irina

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

RE: [PATCH 2/9] input: goodix: fix variable length array warning

[Tirdea, Irina]

 -Original Message-
 From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
 Sent: 05 June, 2015 20:12
 To: Tirdea, Irina
 Cc: 'Antonio Ospite'; Bastien Nocera; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
 
 On Fri, Jun 05, 2015 at 05:00:05PM +, Tirdea, Irina wrote:
 
 
   -Original Message-
   From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
   Sent: 05 June, 2015 19:41
   To: Tirdea, Irina
   Cc: 'Antonio Ospite'; Bastien Nocera; linux-input@vger.kernel.org; 
   devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
   Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
  
   On Fri, Jun 05, 2015 at 04:34:38PM +, Tirdea, Irina wrote:
   
   
 -Original Message-
 From: linux-input-ow...@vger.kernel.org 
 [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Antonio Ospite
 Sent: 03 June, 2015 23:50
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 2/9] input: goodix: fix variable length array 
 warning

 On Wed, 3 Jun 2015 10:26:47 +
 Tirdea, Irina irina.tir...@intel.com wrote:

   -Original Message-
   From: Antonio Ospite [mailto:a...@ao2.it]
   Sent: 28 May, 2015 18:58
   To: Tirdea, Irina
   Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; 
   devicet...@vger.kernel.org; linux-
 ker...@vger.kernel.org
   Subject: Re: [PATCH 2/9] input: goodix: fix variable length array 
   warning
  
   On Thu, 28 May 2015 15:47:38 +0300
   Irina Tirdea irina.tir...@intel.com wrote:
  
Fix sparse warning:
drivers/input/touchscreen/goodix.c:182:26: warning:
Variable length array is used.
   
Replace the variable length array with fixed length.
   
Signed-off-by: Irina Tirdea irina.tir...@intel.com
---
 drivers/input/touchscreen/goodix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
   
diff --git a/drivers/input/touchscreen/goodix.c 
b/drivers/input/touchscreen/goodix.c
index c2e785c..dac1b3c 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -147,7 +147,7 @@ static void goodix_ts_report_touch(struct 
goodix_ts_data *ts, u8 *coor_data)
  */
 static void goodix_process_events(struct goodix_ts_data *ts)
 {
-   u8  point_data[1 + GOODIX_CONTACT_SIZE * 
ts-max_touch_num];
+   u8  point_data[1 + GOODIX_CONTACT_SIZE * 
GOODIX_MAX_CONTACTS];
  
   Hi,
  
 
  Hi Antonio,
 
   this fixes the warning from sparse, but also changes the 
   semantics of
   the code: ts-max_touch_num is less that GOODIX_MAX_CONTACTS for 5
   touches devices and in this case we'll end up using more memory 
   than is
   necessary.
  
 
  I wasn't sure if it is better to save the 5 bytes or fix the 
  warning,
  so I sent this to get some more input.
  Thanks for the feedback, I will  drop this patch.
 

 Use kmalloc() or, alternatively, add at least a comment telling why 
 you
 think that sacrificing a few bytes —only for some devices— has
 advantages over dynamic allocation.

   
You are right, kmalloc will solve both problems - the sparse warning 
and allocating
more bytes than necessary. Don't know why I did not think of that.
Will use that in v2.
  
   Please leave the patch as is. We can spare 80 bytes on the stack given
   that we are running in threaded IRQ. kmallocing will use more code and
   runtime resources and won't give any benefits.
 
  I was actually thinking of allocating it with devm_kzalloc just once at 
  device init and
  storing a pointer to it in the goodix_ts_data structure.
 
 Still, why? Even if you do this once you will need both code and runtime
 resources to bookkeeping whereas allocating 80 bytes on stack are just a
 matter of register addition. And because we are running in threaded IRQ
 context there is no concern of blowing up the limited stack.
 
 The story would be different if you needed, let's say 800 or 1600 bytes.
 

Ok, I see your point. Will leave the patch as is.

Thanks,
Irina


 Thanks.
 
 --
 Dmitry

RE: [PATCH 8/9] input: goodix: add support for ESD

[Tirdea, Irina]

 -Original Message-
 From: Bastien Nocera [mailto:had...@hadess.net]
 Sent: 04 June, 2015 15:58
 To: Tirdea, Irina
 Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
 
 On Thu, 2015-05-28 at 14:26 +, Tirdea, Irina wrote:
 
   -Original Message-
   From: linux-input-ow...@vger.kernel.org [mailto:
   linux-input-ow...@vger.kernel.org] On Behalf Of Mark Rutland
   Sent: 28 May, 2015 16:24
   To: Tirdea, Irina
   Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org;
   devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
   Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
  
   On Thu, May 28, 2015 at 01:47:44PM +0100, 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
esd-recovery-timeout-ms ACPI/DT property. If it is set to 0,
ESD protection is disabled.
   
Signed-off-by: Irina Tirdea irina.tir...@intel.com
---
 .../bindings/input/touchscreen/goodix.txt  |   4 +
 drivers/input/touchscreen/goodix.c | 106
-
 2 files changed, 106 insertions(+), 4 deletions(-)
   
diff --git
a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
   b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 9e4ff69..9132ee0 100644
---
a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++
b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -19,6 +19,10 @@ Optional properties:
   
  - device-config : device configuration information
(specified as byte
array). Maximum size is 240 bytes.
+ - 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.
  
   This sounds like software configuration rather than HW description.
   Is
   there any reason this needs to be a DT property?
  
 
  Although this enables a software feature, it depends on the platform
  if electrostatic discharge
  protection should be enabled or not. Some platform designs handle ESD
  better and do not need
  the SW mechanism, so the property can be used to disable it.
 
  I also saw an example in
  Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt
  and I though this is the standard way to use it.
 
 As for the configuration data, is this useful/used on ACPI?

This can be used with ACPI 5.1 by specifying the timeout as a _DSD property.
I will add an excerpt from an ACPI DSDT file in the commit message.

 
   Can we not just choose a sensible default and allow this to be
   overridden dynamically?
 
  I could separate the enabling part from the actual poll timeout by
  using a DT property to enable/disable
  ESD and a sysfs property to set the timeout. I can also move this
  over to sysfs entirely if you prefer.
 
 If this needs to be in the DT for some devices, and isn't available
 from ACPI, maybe we should have a list of DMI matches for ACPI instead,
 with a module option to override it?

N�r��yb�X��ǧv�^�)޺{.n�+{��zn�)w*jg����ݢj/���z�ޖ��2�ޙ�)ߡ�a�����G���h��j:+v���w��٥

RE: [PATCH 0/9] Goodix touchscreen enhancements

[Tirdea, Irina]

 -Original Message-
 From: Bastien Nocera [mailto:had...@hadess.net]
 Sent: 04 June, 2015 16:05
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; linux-input@vger.kernel.org; devicet...@vger.kernel.org; 
 linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 0/9] Goodix touchscreen enhancements

 Hey Irina,


Hi Bastien,

 On Thu, 2015-05-28 at 15:47 +0300, Irina Tirdea wrote:
  Add several enhancements to the Goodix touchscreen driver:
   - write configuration data to the device
   - power management support
   - cleanup and refactoring
 
  Irina Tirdea (9):
input: goodix: fix alignment issues
input: goodix: fix variable length array warning
input: goodix: export id and version read from device
input: goodix: add ACPI IDs for GT911 and GT9271
input: goodix: reset device at init
input: goodix: write configuration data to device
input: goodix: add power management support
input: goodix: add support for ESD
input: goodix: use goodix_i2c_write_u8 instead of i2c_master_send

 Thanks a lot of that patch series. I was travelling when you sent it,
 but I should be able to test the patch series quicker when you send a
 v2.

Thanks for the review and for offering to test the patches. I will send v2 asap.


 Could you please also add some references to hardware that you're
 enabling (and presumably have been testing on) to the driver's Kconfig?
 If the hardware is still not public, any sort of other reference would
 be useful (Windows 8 compatible harwdare? Hardware that usually runs a
 particular Android vendor tree?...)

I have been using a custom setup by connecting the touchscreen directly
to a virtual Linux environment through an USB-I2C adapter 
(https://diolan.com/dln-2).
This is how I could test both ACPI and DT configurations and why the ACPI IDs I 
used do not
correspond to an actual device.

I have also done tests for the main functionality (writing config, reset, 
suspend/resume,
ESD) on an Android tablet. Unfortunately the HW is not public and neither is the
Android tree. There is an Android tree publicly available at 
https://01.org/android-ia,
but it is quite different from the internal tree I am using.

I can add a reference in Kconfig that this driver supports chip models that can 
be found
on Intel tablets running Android.

Thanks,
Irina


 Cheers
N�r��yb�X��ǧv�^�)޺{.n�+{��zn�)w*jg����ݢj/���z�ޖ��2�ޙ�)ߡ�a�����G���h��j:+v���w��٥

RE: [PATCH 2/9] input: goodix: fix variable length array warning

[Tirdea, Irina]

 -Original Message-
 From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
 Sent: 05 June, 2015 19:41
 To: Tirdea, Irina
 Cc: 'Antonio Ospite'; Bastien Nocera; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
 
 On Fri, Jun 05, 2015 at 04:34:38PM +, Tirdea, Irina wrote:
 
 
   -Original Message-
   From: linux-input-ow...@vger.kernel.org 
   [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Antonio Ospite
   Sent: 03 June, 2015 23:50
   To: Tirdea, Irina
   Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; 
   devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
   Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
  
   On Wed, 3 Jun 2015 10:26:47 +
   Tirdea, Irina irina.tir...@intel.com wrote:
  
 -Original Message-
 From: Antonio Ospite [mailto:a...@ao2.it]
 Sent: 28 May, 2015 18:58
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 2/9] input: goodix: fix variable length array 
 warning

 On Thu, 28 May 2015 15:47:38 +0300
 Irina Tirdea irina.tir...@intel.com wrote:

  Fix sparse warning:
  drivers/input/touchscreen/goodix.c:182:26: warning:
  Variable length array is used.
 
  Replace the variable length array with fixed length.
 
  Signed-off-by: Irina Tirdea irina.tir...@intel.com
  ---
   drivers/input/touchscreen/goodix.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
 
  diff --git a/drivers/input/touchscreen/goodix.c 
  b/drivers/input/touchscreen/goodix.c
  index c2e785c..dac1b3c 100644
  --- a/drivers/input/touchscreen/goodix.c
  +++ b/drivers/input/touchscreen/goodix.c
  @@ -147,7 +147,7 @@ static void goodix_ts_report_touch(struct 
  goodix_ts_data *ts, u8 *coor_data)
*/
   static void goodix_process_events(struct goodix_ts_data *ts)
   {
  -   u8  point_data[1 + GOODIX_CONTACT_SIZE * ts-max_touch_num];
  +   u8  point_data[1 + GOODIX_CONTACT_SIZE * GOODIX_MAX_CONTACTS];

 Hi,

   
Hi Antonio,
   
 this fixes the warning from sparse, but also changes the semantics of
 the code: ts-max_touch_num is less that GOODIX_MAX_CONTACTS for 5
 touches devices and in this case we'll end up using more memory than 
 is
 necessary.

   
I wasn't sure if it is better to save the 5 bytes or fix the warning,
so I sent this to get some more input.
Thanks for the feedback, I will  drop this patch.
   
  
   Use kmalloc() or, alternatively, add at least a comment telling why you
   think that sacrificing a few bytes —only for some devices— has
   advantages over dynamic allocation.
  
 
  You are right, kmalloc will solve both problems - the sparse warning and 
  allocating
  more bytes than necessary. Don't know why I did not think of that.
  Will use that in v2.
 
 Please leave the patch as is. We can spare 80 bytes on the stack given
 that we are running in threaded IRQ. kmallocing will use more code and
 runtime resources and won't give any benefits.

I was actually thinking of allocating it with devm_kzalloc just once at device 
init and
storing a pointer to it in the goodix_ts_data structure. 

Thanks,
Irina

 
 Thanks.
 
 --
 Dmitry

RE: [PATCH 2/9] input: goodix: fix variable length array warning

[Tirdea, Irina]

 -Original Message-
 From: linux-input-ow...@vger.kernel.org 
 [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of Antonio Ospite
 Sent: 03 June, 2015 23:50
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
 
 On Wed, 3 Jun 2015 10:26:47 +
 Tirdea, Irina irina.tir...@intel.com wrote:
 
   -Original Message-
   From: Antonio Ospite [mailto:a...@ao2.it]
   Sent: 28 May, 2015 18:58
   To: Tirdea, Irina
   Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; 
   devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
   Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
  
   On Thu, 28 May 2015 15:47:38 +0300
   Irina Tirdea irina.tir...@intel.com wrote:
  
Fix sparse warning:
drivers/input/touchscreen/goodix.c:182:26: warning:
Variable length array is used.
   
Replace the variable length array with fixed length.
   
Signed-off-by: Irina Tirdea irina.tir...@intel.com
---
 drivers/input/touchscreen/goodix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
   
diff --git a/drivers/input/touchscreen/goodix.c 
b/drivers/input/touchscreen/goodix.c
index c2e785c..dac1b3c 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -147,7 +147,7 @@ static void goodix_ts_report_touch(struct 
goodix_ts_data *ts, u8 *coor_data)
  */
 static void goodix_process_events(struct goodix_ts_data *ts)
 {
-   u8  point_data[1 + GOODIX_CONTACT_SIZE * ts-max_touch_num];
+   u8  point_data[1 + GOODIX_CONTACT_SIZE * GOODIX_MAX_CONTACTS];
  
   Hi,
  
 
  Hi Antonio,
 
   this fixes the warning from sparse, but also changes the semantics of
   the code: ts-max_touch_num is less that GOODIX_MAX_CONTACTS for 5
   touches devices and in this case we'll end up using more memory than is
   necessary.
  
 
  I wasn't sure if it is better to save the 5 bytes or fix the warning,
  so I sent this to get some more input.
  Thanks for the feedback, I will  drop this patch.
 
 
 Use kmalloc() or, alternatively, add at least a comment telling why you
 think that sacrificing a few bytes —only for some devices— has
 advantages over dynamic allocation.
 

You are right, kmalloc will solve both problems - the sparse warning and 
allocating
more bytes than necessary. Don't know why I did not think of that.
Will use that in v2.

Thanks,
Irina

 I am not necessarily against the static allocation change, I was just
 pointing out the issue.
 
 Thanks,
Antonio
 
 --
 Antonio Ospite
 http://ao2.it
 
 A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
 Q: Why is top-posting such a bad thing?
 --
 To unsubscribe from this list: send the line unsubscribe linux-input in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/9] input: goodix: add ACPI IDs for GT911 and GT9271

[Tirdea, Irina]

 -Original Message-
 From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
 Sent: 05 June, 2015 19:42
 To: Tirdea, Irina
 Cc: 'Bastien Nocera'; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 4/9] input: goodix: add ACPI IDs for GT911 and GT9271
 
 On Fri, Jun 05, 2015 at 04:36:24PM +, Tirdea, Irina wrote:
 
 
   -Original Message-
   From: Bastien Nocera [mailto:had...@hadess.net]
   Sent: 04 June, 2015 15:51
   To: Tirdea, Irina
   Cc: Dmitry Torokhov; linux-input@vger.kernel.org; 
   devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
   Subject: Re: [PATCH 4/9] input: goodix: add ACPI IDs for GT911 and GT9271
  
   On Thu, 2015-05-28 at 15:47 +0300, Irina Tirdea wrote:
Add ACPI IDs to support Goodix GT911 and GT9271
touchscreens.
  
   Those devices are present on which systems which you would have tested
   this on?
  
 
  I have been using a custom setup by connecting the touchscreen directly
  to a virtual Linux environment through an USB-I2C adapter 
  (https://diolan.com/dln-2).
  So these ACPI IDs do not correspond to any device publicly available.
  However, once in upstream we can use this IDs for our future platforms.
 
 I'd rather wait until they become available then.
 

Ok, I will drop this patch then.

 Thanks.
 
 
Signed-off-by: Irina Tirdea irina.tir...@intel.com
---
 drivers/input/touchscreen/goodix.c | 2 ++
 1 file changed, 2 insertions(+)
   
diff --git a/drivers/input/touchscreen/goodix.c
b/drivers/input/touchscreen/goodix.c
index 9561396..9e7d215 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -392,6 +392,8 @@ static const struct i2c_device_id goodix_ts_id[]
= {
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id goodix_acpi_match[] = {
  { GDIX1001, 0 },
+ { GDIX0911, 0 },
+ { GDIX9271, 0 },
  { }
 };
 MODULE_DEVICE_TABLE(acpi, goodix_acpi_match);
 
 --
 Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-input in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/9] input: goodix: add ACPI IDs for GT911 and GT9271

[Tirdea, Irina]

 -Original Message-
 From: Bastien Nocera [mailto:had...@hadess.net]
 Sent: 04 June, 2015 15:51
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; linux-input@vger.kernel.org; devicet...@vger.kernel.org; 
 linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 4/9] input: goodix: add ACPI IDs for GT911 and GT9271
 
 On Thu, 2015-05-28 at 15:47 +0300, Irina Tirdea wrote:
  Add ACPI IDs to support Goodix GT911 and GT9271
  touchscreens.
 
 Those devices are present on which systems which you would have tested
 this on?
 

I have been using a custom setup by connecting the touchscreen directly
to a virtual Linux environment through an USB-I2C adapter 
(https://diolan.com/dln-2).
So these ACPI IDs do not correspond to any device publicly available.
However, once in upstream we can use this IDs for our future platforms.

  Signed-off-by: Irina Tirdea irina.tir...@intel.com
  ---
   drivers/input/touchscreen/goodix.c | 2 ++
   1 file changed, 2 insertions(+)
 
  diff --git a/drivers/input/touchscreen/goodix.c
  b/drivers/input/touchscreen/goodix.c
  index 9561396..9e7d215 100644
  --- a/drivers/input/touchscreen/goodix.c
  +++ b/drivers/input/touchscreen/goodix.c
  @@ -392,6 +392,8 @@ static const struct i2c_device_id goodix_ts_id[]
  = {
   #ifdef CONFIG_ACPI
   static const struct acpi_device_id goodix_acpi_match[] = {
{ GDIX1001, 0 },
  + { GDIX0911, 0 },
  + { GDIX9271, 0 },
{ }
   };
   MODULE_DEVICE_TABLE(acpi, goodix_acpi_match);

RE: [PATCH 7/9] input: goodix: add power management support

[Tirdea, Irina]

 -Original Message-
 From: Bastien Nocera [mailto:had...@hadess.net]
 Sent: 04 June, 2015 16:01
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; linux-input@vger.kernel.org; devicet...@vger.kernel.org; 
 linux-ker...@vger.kernel.org; Purdila, Octavian
 Subject: Re: [PATCH 7/9] input: goodix: add power management support

 On Thu, 2015-05-28 at 15:47 +0300, Irina Tirdea wrote:
  Implement suspend/resume for goodix driver.
 
  Signed-off-by: Octavian Purdila octavian.purd...@intel.com
  Signed-off-by: Irina Tirdea irina.tir...@intel.com
  ---
   drivers/input/touchscreen/goodix.c | 81
  +++---
   1 file changed, 76 insertions(+), 5 deletions(-)
 
  diff --git a/drivers/input/touchscreen/goodix.c
  b/drivers/input/touchscreen/goodix.c
  index 22052c9..ce7e834 100644
  --- a/drivers/input/touchscreen/goodix.c
  +++ b/drivers/input/touchscreen/goodix.c
  @@ -37,6 +37,7 @@ struct goodix_ts_data {
unsigned int int_trigger_type;
struct gpio_desc *gpiod_int;
struct gpio_desc *gpiod_rst;
  + unsigned long irq_flags;
   };
 
   #define GOODIX_GPIO_INT_NAME irq
  @@ -52,6 +53,9 @@ struct goodix_ts_data {
   #define GOODIX_CONFIG_MAX_LENGTH 240
 
   /* Register defines */
  +#define GOODIX_REG_COMMAND   0x8040
  +#define GOODIX_CMD_SCREEN_OFF0x05
  +
   #define GOODIX_READ_COOR_ADDR0x814E
   #define GOODIX_REG_CONFIG_DATA   0x8047
   #define GOODIX_REG_ID0x8140
  @@ -129,6 +133,11 @@ static int goodix_i2c_write(struct i2c_client
  *client, u16 reg, u8 *buf,
return ret  0 ? ret : (ret != 1 ? -EIO : 0);
   }
 
  +static int goodix_i2c_write_u8(struct i2c_client *client, u16 reg,
  u8 value)
  +{
  + return goodix_i2c_write(client, reg, value, sizeof(value));
  +}
  +
   static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8
  *data)
   {
int touch_num;
  @@ -227,6 +236,18 @@ static irqreturn_t goodix_ts_irq_handler(int
  irq, void *dev_id)
return IRQ_HANDLED;
   }
 
  +static void goodix_free_irq(struct goodix_ts_data *ts)
  +{
  + devm_free_irq(ts-client-dev, ts-client-irq, ts);
  +}
  +
  +static int goodix_request_irq(struct goodix_ts_data *ts)
  +{
  + return devm_request_threaded_irq(ts-client-dev, ts
  -client-irq,
  +  NULL,
  goodix_ts_irq_handler,
  +  ts-irq_flags, ts-client
  -name, ts);
  +}
  +
   /**
* goodix_get_cfg - Get device config from ACPI/DT.
*
  @@ -562,7 +583,6 @@ static int goodix_ts_probe(struct i2c_client
  *client,
   const struct i2c_device_id *id)
   {
struct goodix_ts_data *ts;
  - unsigned long irq_flags;
int error;
u16 version_info, id_info;
 
  @@ -614,10 +634,8 @@ static int goodix_ts_probe(struct i2c_client
  *client,
if (error)
return error;
 
  - irq_flags = goodix_irq_flags[ts-int_trigger_type] |
  IRQF_ONESHOT;
  - error = devm_request_threaded_irq(ts-client-dev, client
  -irq,
  -   NULL,
  goodix_ts_irq_handler,
  -   irq_flags, client-name,
  ts);
  + ts-irq_flags = goodix_irq_flags[ts-int_trigger_type] |
  IRQF_ONESHOT;
  + error = goodix_request_irq(ts);
if (error) {
dev_err(client-dev, request IRQ failed: %d\n,
  error);
return error;
  @@ -626,6 +644,58 @@ static int goodix_ts_probe(struct i2c_client
  *client,
return 0;
   }
 
  +#ifdef CONFIG_PM_SLEEP
  +static int goodix_suspend(struct device *dev)
  +{
  + struct i2c_client *client = to_i2c_client(dev);
  + struct goodix_ts_data *ts = i2c_get_clientdata(client);
  + int ret;
  +
  + goodix_free_irq(ts);
  + ret = gpiod_direction_output(ts-gpiod_int, 0);
  + if (ret) {
  + goodix_request_irq(ts);
  + return ret;
  + }
  + usleep_range(5000, 6000);

 I'd appreciate some references and explanations for those magic
 numbers.


Most of these  numbers are taken directly from the datasheet or
from the Android gt9xx.c driver. I used datasheets and
programming guides from Goodix that are not publicly available
(for gt911 and gt9271). I have later found some datasheets specified
in a previous patch for Goodix:
https://drive.google.com/folderview?id=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzgusp=sharing
However, I am not sure of their provenience so I would rather
not add them to the commit message or source code.

In this particular case, the datasheet does not specify how long to
output low on the INT pin, but 5 ms is the time used by the Android driver.

  +
  + ret = goodix_i2c_write_u8(ts-client, GOODIX_REG_COMMAND,
  +   GOODIX_CMD_SCREEN_OFF);
  + if (ret) {
  + dev_err(ts-client-dev, Screen off command

RE: [PATCH 6/9] input: goodix: write configuration data to device

[Tirdea, Irina]

 -Original Message-
 From: Bastien Nocera [mailto:had...@hadess.net]
 Sent: 04 June, 2015 15:55
 To: Tirdea, Irina
 Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Purdila,
 Octavian
 Subject: Re: [PATCH 6/9] input: goodix: write configuration data to device
 
 On Thu, 2015-05-28 at 13:51 +, Tirdea, Irina wrote:
 
   -Original Message-
   From: Mark Rutland [mailto:mark.rutl...@arm.com]
   Sent: 28 May, 2015 16:22
   To: Tirdea, Irina
   Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org;
   devicet...@vger.kernel.org; linux-ker...@vger.kernel.org;
   Purdila, Octavian
   Subject: Re: [PATCH 6/9] input: goodix: write configuration data to
   device
  
   On Thu, May 28, 2015 at 01:47:42PM +0100, Irina Tirdea wrote:
Goodix devices can be configured by writing this information
to the device at init. The configuration data can
be provided through the ACPI/device tree property
device-config. If device-config is not set, the default
device configuration will be used.
   
Signed-off-by: Octavian Purdila octavian.purd...@intel.com
Signed-off-by: Irina Tirdea irina.tir...@intel.com
---
 .../bindings/input/touchscreen/goodix.txt  |   5 +
 drivers/input/touchscreen/goodix.c | 143
+
 2 files changed, 148 insertions(+)
   
diff --git
a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
   b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 7137881..9e4ff69 100644
---
a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++
b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -15,6 +15,11 @@ Required properties:
  - irq-gpio  : GPIO pin used for IRQ
  - reset-gpio: GPIO pin used for reset
   
+Optional properties:
+
+ - device-config : device configuration information
(specified as byte
+   array). Maximum size is 240 bytes.
  
   Generally we frown on passing opaque data.
  
   What exactly is encoded in device-config? The description is very
   vague.
  
   Does this correspond to anything in a data sheet or manual?
 
  Yes, it is configuration data described in chapter 6.2. b of the
  datasheet:
  https://drive.google.com/folderview?id=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKN
  lktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzgusp=sharing.
  This includes things like x/y resolution, maximum touch numbers
  supported,
  interrupt flags, various sensitivity factors.
 
  I should have included a link to the datasheet in the commit message
  - will do that in v2 for all patches.
 
  I did consider writing this as firmware, but it seems that some
  Goodix touchscreens have a
  real firmware they need to write in addition to this configuration
  data.
  If there is a better way to do this, please let me know and I will
  change it.
 
 I would at least expect an excerpt from an ACPI DSDT that would make
 use of that feature.

Ok, I will add this to the commit message.

RE: [PATCH 6/9] input: goodix: write configuration data to device

[Tirdea, Irina]

 -Original Message-
 From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
 Sent: 05 June, 2015 19:43
 To: Tirdea, Irina
 Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Purdila,
 Octavian
 Subject: Re: [PATCH 6/9] input: goodix: write configuration data to device
 
 On Fri, Jun 05, 2015 at 04:36:24PM +, Tirdea, Irina wrote:
 
 
   -Original Message-
   From: Bastien Nocera [mailto:had...@hadess.net]
   Sent: 04 June, 2015 15:55
   To: Tirdea, Irina
   Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; 
   devicet...@vger.kernel.org; linux-ker...@vger.kernel.org;
 Purdila,
   Octavian
   Subject: Re: [PATCH 6/9] input: goodix: write configuration data to device
  
   On Thu, 2015-05-28 at 13:51 +, Tirdea, Irina wrote:
   
 -Original Message-
 From: Mark Rutland [mailto:mark.rutl...@arm.com]
 Sent: 28 May, 2015 16:22
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org;
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org;
 Purdila, Octavian
 Subject: Re: [PATCH 6/9] input: goodix: write configuration data to
 device

 On Thu, May 28, 2015 at 01:47:42PM +0100, Irina Tirdea wrote:
  Goodix devices can be configured by writing this information
  to the device at init. The configuration data can
  be provided through the ACPI/device tree property
  device-config. If device-config is not set, the default
  device configuration will be used.
 
  Signed-off-by: Octavian Purdila octavian.purd...@intel.com
  Signed-off-by: Irina Tirdea irina.tir...@intel.com
  ---
   .../bindings/input/touchscreen/goodix.txt  |   5 +
   drivers/input/touchscreen/goodix.c | 143
  +
   2 files changed, 148 insertions(+)
 
  diff --git
  a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
 b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  index 7137881..9e4ff69 100644
  ---
  a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  +++
  b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  @@ -15,6 +15,11 @@ Required properties:
- irq-gpio  : GPIO pin used for IRQ
- reset-gpio: GPIO pin used for reset
 
  +Optional properties:
  +
  + - device-config : device configuration information
  (specified as byte
  +   array). Maximum size is 240 bytes.

 Generally we frown on passing opaque data.

 What exactly is encoded in device-config? The description is very
 vague.

 Does this correspond to anything in a data sheet or manual?
   
Yes, it is configuration data described in chapter 6.2. b of the
datasheet:
https://drive.google.com/folderview?id=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKN
lktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzgusp=sharing.
This includes things like x/y resolution, maximum touch numbers
supported,
interrupt flags, various sensitivity factors.
   
I should have included a link to the datasheet in the commit message
- will do that in v2 for all patches.
   
I did consider writing this as firmware, but it seems that some
Goodix touchscreens have a
real firmware they need to write in addition to this configuration
data.
If there is a better way to do this, please let me know and I will
change it.
  
   I would at least expect an excerpt from an ACPI DSDT that would make
   use of that feature.
 
  Ok, I will add this to the commit message.
 
 No, it really shoudl be done via request_firmware() mechanism. There are
 drivers in kernel that use request_firmware() for both firmware and
 configuration data (see elan_i2c, elants_i2c, atmel_mxt_ts and I am sure
 there are others).
 

Ok, I didn't know there are these other drivers that already do that.
I will change this to use request_firmware then.

Thanks,
Irina

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

RE: [PATCH 2/9] input: goodix: fix variable length array warning

[Tirdea, Irina]

 -Original Message-
 From: Antonio Ospite [mailto:a...@ao2.it]
 Sent: 28 May, 2015 18:58
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org
 Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
 
 On Thu, 28 May 2015 15:47:38 +0300
 Irina Tirdea irina.tir...@intel.com wrote:
 
  Fix sparse warning:
  drivers/input/touchscreen/goodix.c:182:26: warning:
  Variable length array is used.
 
  Replace the variable length array with fixed length.
 
  Signed-off-by: Irina Tirdea irina.tir...@intel.com
  ---
   drivers/input/touchscreen/goodix.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
 
  diff --git a/drivers/input/touchscreen/goodix.c 
  b/drivers/input/touchscreen/goodix.c
  index c2e785c..dac1b3c 100644
  --- a/drivers/input/touchscreen/goodix.c
  +++ b/drivers/input/touchscreen/goodix.c
  @@ -147,7 +147,7 @@ static void goodix_ts_report_touch(struct 
  goodix_ts_data *ts, u8 *coor_data)
*/
   static void goodix_process_events(struct goodix_ts_data *ts)
   {
  -   u8  point_data[1 + GOODIX_CONTACT_SIZE * ts-max_touch_num];
  +   u8  point_data[1 + GOODIX_CONTACT_SIZE * GOODIX_MAX_CONTACTS];
 
 Hi,
 

Hi Antonio,

 this fixes the warning from sparse, but also changes the semantics of
 the code: ts-max_touch_num is less that GOODIX_MAX_CONTACTS for 5
 touches devices and in this case we'll end up using more memory than is
 necessary.
 

I wasn't sure if it is better to save the 5 bytes or fix the warning, so I sent 
this to get some more input.
Thanks for the feedback, I will  drop this patch.

Thanks,
Irina

 Thanks,
Antonio
 
  int touch_num;
  int i;
 
  --
  1.9.1
 
  --
  To unsubscribe from this list: send the line unsubscribe linux-input in
  the body of a message to majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
 --
 Antonio Ospite
 http://ao2.it
 
 A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
 Q: Why is top-posting such a bad thing?
--
To unsubscribe from this list: send the line unsubscribe linux-input in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 6/9] input: goodix: write configuration data to device

[Tirdea, Irina]

 -Original Message-
 From: Mark Rutland [mailto:mark.rutl...@arm.com]
 Sent: 28 May, 2015 16:22
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org;
 Purdila, Octavian
 Subject: Re: [PATCH 6/9] input: goodix: write configuration data to device
 
 On Thu, May 28, 2015 at 01:47:42PM +0100, Irina Tirdea wrote:
  Goodix devices can be configured by writing this information
  to the device at init. The configuration data can
  be provided through the ACPI/device tree property
  device-config. If device-config is not set, the default
  device configuration will be used.
 
  Signed-off-by: Octavian Purdila octavian.purd...@intel.com
  Signed-off-by: Irina Tirdea irina.tir...@intel.com
  ---
   .../bindings/input/touchscreen/goodix.txt  |   5 +
   drivers/input/touchscreen/goodix.c | 143 
  +
   2 files changed, 148 insertions(+)
 
  diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
 b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  index 7137881..9e4ff69 100644
  --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  @@ -15,6 +15,11 @@ Required properties:
- irq-gpio: GPIO pin used for IRQ
- reset-gpio  : GPIO pin used for reset
 
  +Optional properties:
  +
  + - device-config   : device configuration information (specified as byte
  + array). Maximum size is 240 bytes.
 
 Generally we frown on passing opaque data.
 
 What exactly is encoded in device-config? The description is very vague.
 
 Does this correspond to anything in a data sheet or manual?

Yes, it is configuration data described in chapter 6.2. b of the datasheet: 
https://drive.google.com/folderview?id=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzgusp=sharing.
This includes things like x/y resolution, maximum touch numbers supported, 
interrupt flags, various sensitivity factors. 

I should have included a link to the datasheet in the commit message - will do 
that in v2 for all patches.

I did consider writing this as firmware, but it seems that some Goodix 
touchscreens have a
real firmware they need to write in addition to this configuration data.
If there is a better way to do this, please let me know and I will change it.

Thanks,
Irina

 
 Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-input in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 5/9] input: goodix: reset device at init

[Tirdea, Irina]

 -Original Message-
 From: Mark Rutland [mailto:mark.rutl...@arm.com]
 Sent: 28 May, 2015 16:20
 To: Tirdea, Irina
 Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; 
 devicet...@vger.kernel.org; linux-ker...@vger.kernel.org;
 Purdila, Octavian
 Subject: Re: [PATCH 5/9] input: goodix: reset device at init
 
 Hi,
 

Hi Mark,

Thanks for your quick review!

 On Thu, May 28, 2015 at 01:47:41PM +0100, Irina Tirdea wrote:
  After power on, it is recommended that the driver resets the device.
  For reset the driver needs to control the interrupt and
  reset gpio pins (configured through ACPI/device tree).
 
 Why is it necessary to mess with the GPIO the interrupts is wired up to?
 What exactly does the device expect at reset w.r.t. the interrupt line?
 

The reset procedure is described in the Goodix documentation 
(https://drive.google.com/folderview?id=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzgusp=sharing)
 and implemented in their reference driver.

It is used at device init (before writing device configuration) and for power 
management (as described in chapter 7.1 of the documentation, when entering 
suspend the device must output low on the interrupt pin and when resuming it 
must output high on the interrupt pin). 

 
  Signed-off-by: Octavian Purdila octavian.purd...@intel.com
  Signed-off-by: Irina Tirdea irina.tir...@intel.com
  ---
   .../bindings/input/touchscreen/goodix.txt  |  5 ++
   drivers/input/touchscreen/goodix.c | 99 
  ++
   2 files changed, 104 insertions(+)
 
  diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
 b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  index 8ba98ee..7137881 100644
  --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
  @@ -12,6 +12,8 @@ Required properties:
- reg : I2C address of the chip. Should be 0x5d or 
  0x14
- interrupt-parent: Interrupt controller to which the chip is 
  connected
- interrupts  : Interrupt to which the chip is connected
  + - irq-gpio: GPIO pin used for IRQ
  + - reset-gpio  : GPIO pin used for reset
 
   Example:
 
  @@ -23,6 +25,9 @@ Example:
  reg = 0x5d;
  interrupt-parent = gpio;
  interrupts = 0 0;
  +
  +   irq-gpio = gpio1 0 0;
  +   reset-gpio = gpio1 1 0;
  };
 
  /* ... */
  diff --git a/drivers/input/touchscreen/goodix.c 
  b/drivers/input/touchscreen/goodix.c
  index 9e7d215..4405c55 100644
  --- a/drivers/input/touchscreen/goodix.c
  +++ b/drivers/input/touchscreen/goodix.c
  @@ -26,6 +26,7 @@
   #include linux/acpi.h
   #include linux/of.h
   #include asm/unaligned.h
  +#include linux/gpio.h
 
 Nit: weird include ordering.

Will fix this in v2.

 
 Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-input in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html