[PATCH 0/9] Goodix touchscreen enhancements

[PATCH 0/9] Goodix touchscreen enhancements

[Irina Tirdea]

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

 .../bindings/input/touchscreen/goodix.txt          |  14 +
 drivers/input/touchscreen/goodix.c                 | 481 +++++++++++++++++++--
 2 files changed, 463 insertions(+), 32 deletions(-)

-- 
1.9.1

[PATCH 1/9] input: goodix: fix alignment issues

[Irina Tirdea]

Fix alignment to match open parenthesis detected by
running checkpatch.pl --strict.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/input/touchscreen/goodix.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 0d93b1e..c2e785c 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -69,7 +69,7 @@ static const unsigned long goodix_irq_flags[] = {
  * @len: length of the buffer to write
  */
 static int goodix_i2c_read(struct i2c_client *client,
-				u16 reg, u8 *buf, int len)
+			   u16 reg, u8 *buf, int len)
 {
 	struct i2c_msg msgs[2];
 	u16 wbuf = cpu_to_be16(reg);
@@ -78,7 +78,7 @@ static int goodix_i2c_read(struct i2c_client *client,
 	msgs[0].flags = 0;
 	msgs[0].addr  = client->addr;
 	msgs[0].len   = 2;
-	msgs[0].buf   = (u8 *) &wbuf;
+	msgs[0].buf   = (u8 *)&wbuf;
 
 	msgs[1].flags = I2C_M_RD;
 	msgs[1].addr  = client->addr;
@@ -112,7 +112,7 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 		data += 1 + GOODIX_CONTACT_SIZE;
 		error = goodix_i2c_read(ts->client,
 					GOODIX_READ_COOR_ADDR +
-						1 + GOODIX_CONTACT_SIZE,
+					1 + GOODIX_CONTACT_SIZE,
 					data,
 					GOODIX_CONTACT_SIZE * (touch_num - 1));
 		if (error)
@@ -157,7 +157,8 @@ static void goodix_process_events(struct goodix_ts_data *ts)
 
 	for (i = 0; i < touch_num; i++)
 		goodix_ts_report_touch(ts,
-				&point_data[1 + GOODIX_CONTACT_SIZE * i]);
+				       &point_data[1 +
+				       GOODIX_CONTACT_SIZE * i]);
 
 	input_mt_sync_frame(ts->input_dev);
 	input_sync(ts->input_dev);
@@ -199,8 +200,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
 	int error;
 
 	error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
-			      config,
-			   GOODIX_CONFIG_MAX_LENGTH);
+				config,
+				GOODIX_CONFIG_MAX_LENGTH);
 	if (error) {
 		dev_warn(&ts->client->dev,
 			 "Error reading config (%d), using defaults\n",
@@ -297,9 +298,9 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
 				  BIT_MASK(EV_ABS);
 
 	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X, 0,
-				ts->abs_x_max, 0, 0);
+			     ts->abs_x_max, 0, 0);
 	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y, 0,
-				ts->abs_y_max, 0, 0);
+			     ts->abs_y_max, 0, 0);
 	input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
 	input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
 
-- 
1.9.1

Re: [PATCH 1/9] input: goodix: fix alignment issues

[Bastien Nocera]

On Thu, 2015-05-28 at 15:47 +0300, Irina Tirdea wrote:
> Fix alignment to match open parenthesis detected by
> running checkpatch.pl --strict.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>

Looks innocuous enough.

Acked-by: Bastien Nocera <hadess@hadess.net>

> ---
>  drivers/input/touchscreen/goodix.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c 
> b/drivers/input/touchscreen/goodix.c
> index 0d93b1e..c2e785c 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -69,7 +69,7 @@ static const unsigned long goodix_irq_flags[] = {
>   * @len: length of the buffer to write
>   */
>  static int goodix_i2c_read(struct i2c_client *client,
> -                             u16 reg, u8 *buf, int len)
> +                        u16 reg, u8 *buf, int len)
>  {
>       struct i2c_msg msgs[2];
>       u16 wbuf = cpu_to_be16(reg);
> @@ -78,7 +78,7 @@ static int goodix_i2c_read(struct i2c_client 
> *client,
>       msgs[0].flags = 0;
>       msgs[0].addr  = client->addr;
>       msgs[0].len   = 2;
> -     msgs[0].buf   = (u8 *) &wbuf;
> +     msgs[0].buf   = (u8 *)&wbuf;
>  
>       msgs[1].flags = I2C_M_RD;
>       msgs[1].addr  = client->addr;
> @@ -112,7 +112,7 @@ static int goodix_ts_read_input_report(struct 
> goodix_ts_data *ts, u8 *data)
>               data += 1 + GOODIX_CONTACT_SIZE;
>               error = goodix_i2c_read(ts->client,
>                                       GOODIX_READ_COOR_ADDR +
> -                                             1 + 
> GOODIX_CONTACT_SIZE,
> +                                     1 + GOODIX_CONTACT_SIZE,
>                                       data,
>                                       GOODIX_CONTACT_SIZE * 
> (touch_num - 1));
>               if (error)
> @@ -157,7 +157,8 @@ static void goodix_process_events(struct 
> goodix_ts_data *ts)
>  
>       for (i = 0; i < touch_num; i++)
>               goodix_ts_report_touch(ts,
> -                             &point_data[1 + GOODIX_CONTACT_SIZE 
> * i]);
> +                                    &point_data[1 +
> +                                    GOODIX_CONTACT_SIZE * i]);
>  
>       input_mt_sync_frame(ts->input_dev);
>       input_sync(ts->input_dev);
> @@ -199,8 +200,8 @@ static void goodix_read_config(struct 
> goodix_ts_data *ts)
>       int error;
>  
>       error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
> -                           config,
> -                        GOODIX_CONFIG_MAX_LENGTH);
> +                             config,
> +                             GOODIX_CONFIG_MAX_LENGTH);
>       if (error) {
>               dev_warn(&ts->client->dev,
>                        "Error reading config (%d), using 
> defaults\n",
> @@ -297,9 +298,9 @@ static int goodix_request_input_dev(struct 
> goodix_ts_data *ts)
>                                 BIT_MASK(EV_ABS);
>  
>       input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X, 0,
> -                             ts->abs_x_max, 0, 0);
> +                          ts->abs_x_max, 0, 0);
>       input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y, 0,
> -                             ts->abs_y_max, 0, 0);
> +                          ts->abs_y_max, 0, 0);
>       input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 
> 255, 0, 0);
>       input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0, 
> 255, 0, 0);
>  

Re: [PATCH 1/9] input: goodix: fix alignment issues

[Dmitry Torokhov]

Hi Irina,

On Thu, May 28, 2015 at 03:47:37PM +0300, Irina Tirdea wrote:
> Fix alignment to match open parenthesis detected by
> running checkpatch.pl --strict.

Mixed bag of changes here, but that's checkpatch for you.

> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> ---
>  drivers/input/touchscreen/goodix.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 0d93b1e..c2e785c 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -69,7 +69,7 @@ static const unsigned long goodix_irq_flags[] = {
>   * @len: length of the buffer to write
>   */
>  static int goodix_i2c_read(struct i2c_client *client,
> -				u16 reg, u8 *buf, int len)
> +			   u16 reg, u8 *buf, int len)
>  {
>  	struct i2c_msg msgs[2];
>  	u16 wbuf = cpu_to_be16(reg);
> @@ -78,7 +78,7 @@ static int goodix_i2c_read(struct i2c_client *client,
>  	msgs[0].flags = 0;
>  	msgs[0].addr  = client->addr;
>  	msgs[0].len   = 2;
> -	msgs[0].buf   = (u8 *) &wbuf;
> +	msgs[0].buf   = (u8 *)&wbuf;

Good.

>  
>  	msgs[1].flags = I2C_M_RD;
>  	msgs[1].addr  = client->addr;
> @@ -112,7 +112,7 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
>  		data += 1 + GOODIX_CONTACT_SIZE;
>  		error = goodix_i2c_read(ts->client,
>  					GOODIX_READ_COOR_ADDR +
> -						1 + GOODIX_CONTACT_SIZE,
> +					1 + GOODIX_CONTACT_SIZE,

Bad - makes 1 + GOODIX_CONTACT_SIZE look like extra argument, not
continuation of expression.


>  					data,
>  					GOODIX_CONTACT_SIZE * (touch_num - 1));
>  		if (error)
> @@ -157,7 +157,8 @@ static void goodix_process_events(struct goodix_ts_data *ts)
>  
>  	for (i = 0; i < touch_num; i++)
>  		goodix_ts_report_touch(ts,
> -				&point_data[1 + GOODIX_CONTACT_SIZE * i]);
> +				       &point_data[1 +
> +				       GOODIX_CONTACT_SIZE * i]);

No, this is plain ugly.

>  
>  	input_mt_sync_frame(ts->input_dev);
>  	input_sync(ts->input_dev);
> @@ -199,8 +200,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
>  	int error;
>  
>  	error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
> -			      config,
> -			   GOODIX_CONFIG_MAX_LENGTH);
> +				config,
> +				GOODIX_CONFIG_MAX_LENGTH);

Good.

>  	if (error) {
>  		dev_warn(&ts->client->dev,
>  			 "Error reading config (%d), using defaults\n",
> @@ -297,9 +298,9 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
>  				  BIT_MASK(EV_ABS);
>  
>  	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X, 0,
> -				ts->abs_x_max, 0, 0);
> +			     ts->abs_x_max, 0, 0);

Good.

>  	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y, 0,
> -				ts->abs_y_max, 0, 0);
> +			     ts->abs_y_max, 0, 0);

Good.

>  	input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
>  	input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>  
> -- 
> 1.9.1
> 

Thanks.

-- 
Dmitry

RE: [PATCH 1/9] input: goodix: fix alignment issues

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 05 June, 2015 19:49
> To: Tirdea, Irina
> Cc: Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/9] input: goodix: fix alignment issues
> 
> Hi Irina,
> 

Hi Dmitry,

Thanks for the review!

Will keep the good parts below and remove the bad ones.

Thanks,
Irina

> On Thu, May 28, 2015 at 03:47:37PM +0300, Irina Tirdea wrote:
> > Fix alignment to match open parenthesis detected by
> > running checkpatch.pl --strict.
> 
> Mixed bag of changes here, but that's checkpatch for you.
> 
> >
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > ---
> >  drivers/input/touchscreen/goodix.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > index 0d93b1e..c2e785c 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -69,7 +69,7 @@ static const unsigned long goodix_irq_flags[] = {
> >   * @len: length of the buffer to write
> >   */
> >  static int goodix_i2c_read(struct i2c_client *client,
> > -				u16 reg, u8 *buf, int len)
> > +			   u16 reg, u8 *buf, int len)
> >  {
> >  	struct i2c_msg msgs[2];
> >  	u16 wbuf = cpu_to_be16(reg);
> > @@ -78,7 +78,7 @@ static int goodix_i2c_read(struct i2c_client *client,
> >  	msgs[0].flags = 0;
> >  	msgs[0].addr  = client->addr;
> >  	msgs[0].len   = 2;
> > -	msgs[0].buf   = (u8 *) &wbuf;
> > +	msgs[0].buf   = (u8 *)&wbuf;
> 
> Good.
> 
> >
> >  	msgs[1].flags = I2C_M_RD;
> >  	msgs[1].addr  = client->addr;
> > @@ -112,7 +112,7 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
> >  		data += 1 + GOODIX_CONTACT_SIZE;
> >  		error = goodix_i2c_read(ts->client,
> >  					GOODIX_READ_COOR_ADDR +
> > -						1 + GOODIX_CONTACT_SIZE,
> > +					1 + GOODIX_CONTACT_SIZE,
> 
> Bad - makes 1 + GOODIX_CONTACT_SIZE look like extra argument, not
> continuation of expression.
> 
> 
> >  					data,
> >  					GOODIX_CONTACT_SIZE * (touch_num - 1));
> >  		if (error)
> > @@ -157,7 +157,8 @@ static void goodix_process_events(struct goodix_ts_data *ts)
> >
> >  	for (i = 0; i < touch_num; i++)
> >  		goodix_ts_report_touch(ts,
> > -				&point_data[1 + GOODIX_CONTACT_SIZE * i]);
> > +				       &point_data[1 +
> > +				       GOODIX_CONTACT_SIZE * i]);
> 
> No, this is plain ugly.
> 
> >
> >  	input_mt_sync_frame(ts->input_dev);
> >  	input_sync(ts->input_dev);
> > @@ -199,8 +200,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
> >  	int error;
> >
> >  	error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
> > -			      config,
> > -			   GOODIX_CONFIG_MAX_LENGTH);
> > +				config,
> > +				GOODIX_CONFIG_MAX_LENGTH);
> 
> Good.
> 
> >  	if (error) {
> >  		dev_warn(&ts->client->dev,
> >  			 "Error reading config (%d), using defaults\n",
> > @@ -297,9 +298,9 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
> >  				  BIT_MASK(EV_ABS);
> >
> >  	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X, 0,
> > -				ts->abs_x_max, 0, 0);
> > +			     ts->abs_x_max, 0, 0);
> 
> Good.
> 
> >  	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y, 0,
> > -				ts->abs_y_max, 0, 0);
> > +			     ts->abs_y_max, 0, 0);
> 
> Good.
> 
> >  	input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
> >  	input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> >
> > --
> > 1.9.1
> >
> 
> Thanks.
> 
> --
> Dmitry

Re: [PATCH 1/9] input: goodix: fix alignment issues

[Joe Perches]

On Fri, 2015-06-05 at 09:49 -0700, Dmitry Torokhov wrote:
> Hi Irina,
> 
> On Thu, May 28, 2015 at 03:47:37PM +0300, Irina Tirdea wrote:
> > Fix alignment to match open parenthesis detected by
> > running checkpatch.pl --strict.
> 
> Mixed bag of changes here, but that's checkpatch for you.

Yup,

checkpatch output is definitely a mix of automated
semi-competence and brain-deadness.

> > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
[]
> > @@ -112,7 +112,7 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
> >  		data += 1 + GOODIX_CONTACT_SIZE;
> >  		error = goodix_i2c_read(ts->client,
> >  					GOODIX_READ_COOR_ADDR +
> > -						1 + GOODIX_CONTACT_SIZE,
> > +					1 + GOODIX_CONTACT_SIZE,
> 
> Bad - makes 1 + GOODIX_CONTACT_SIZE look like extra argument, not
> continuation of expression.

<shrug>  There's no great way to do this.

Parentheses around the longer expression work.

		error = goodix_i2c_read(ts->client,
					(GOODIX_READ_COOR_ADDR +
					 1 + GOODIX_CONTACT_SIZE),

Exceeding 80 columns may be better.
Leaving it alone would be OK too.

Maybe starting with 1 to be more similar
to the below would be better.

> > @@ -157,7 +157,8 @@ static void goodix_process_events(struct goodix_ts_data *ts)
> >  
> >  	for (i = 0; i < touch_num; i++)
> >  		goodix_ts_report_touch(ts,
> > -				&point_data[1 + GOODIX_CONTACT_SIZE * i]);
> > +				       &point_data[1 +
> > +				       GOODIX_CONTACT_SIZE * i]);
> 
> No, this is plain ugly.

Sometimes it's better to exceed 80 columns or
maybe use temporaries.  Something like:

	u8 *tmp = point_data + 1;
	...

	for (i = 0; i < touch_num; i++, tmp += GOODIX_CONTACT_SIZE)
		goodix_ts_report_touch(ts, tmp);

Re: [PATCH 1/9] input: goodix: fix alignment issues

[Dmitry Torokhov]

On Fri, Jun 05, 2015 at 10:17:58AM -0700, Joe Perches wrote:
> On Fri, 2015-06-05 at 09:49 -0700, Dmitry Torokhov wrote:
> > Hi Irina,
> > 
> > On Thu, May 28, 2015 at 03:47:37PM +0300, Irina Tirdea wrote:
> > > Fix alignment to match open parenthesis detected by
> > > running checkpatch.pl --strict.
> > 
> > Mixed bag of changes here, but that's checkpatch for you.
> 
> Yup,
> 
> checkpatch output is definitely a mix of automated
> semi-competence and brain-deadness.
> 
> > > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> []
> > > @@ -112,7 +112,7 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
> > >  		data += 1 + GOODIX_CONTACT_SIZE;
> > >  		error = goodix_i2c_read(ts->client,
> > >  					GOODIX_READ_COOR_ADDR +
> > > -						1 + GOODIX_CONTACT_SIZE,
> > > +					1 + GOODIX_CONTACT_SIZE,
> > 
> > Bad - makes 1 + GOODIX_CONTACT_SIZE look like extra argument, not
> > continuation of expression.
> 
> <shrug>  There's no great way to do this.
> 
> Parentheses around the longer expression work.
> 
> 		error = goodix_i2c_read(ts->client,
> 					(GOODIX_READ_COOR_ADDR +
> 					 1 + GOODIX_CONTACT_SIZE),
> 
> Exceeding 80 columns may be better.
> Leaving it alone would be OK too.
> 
> Maybe starting with 1 to be more similar
> to the below would be better.
> 
> > > @@ -157,7 +157,8 @@ static void goodix_process_events(struct goodix_ts_data *ts)
> > >  
> > >  	for (i = 0; i < touch_num; i++)
> > >  		goodix_ts_report_touch(ts,
> > > -				&point_data[1 + GOODIX_CONTACT_SIZE * i]);
> > > +				       &point_data[1 +
> > > +				       GOODIX_CONTACT_SIZE * i]);
> > 
> > No, this is plain ugly.
> 
> Sometimes it's better to exceed 80 columns or
> maybe use temporaries.  Something like:

Or not aligning the arguments (when it makes sense).

Thankfully we are not using FORTRAN, COBOL or even Python here.

Thanks.

-- 
Dmitry

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

[Irina Tirdea]

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.tirdea@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];
 	int touch_num;
 	int i;
 
-- 
1.9.1

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

[Antonio Ospite]

On Thu, 28 May 2015 15:47:38 +0300
Irina Tirdea <irina.tirdea@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.tirdea@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,

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.

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 majordomo@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?

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

[Tirdea, Irina]

> -----Original Message-----
> From: Antonio Ospite [mailto:ao2@ao2.it]
> Sent: 28 May, 2015 18:58
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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.tirdea@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.tirdea@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 majordomo@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?

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

[Antonio Ospite]

On Wed, 3 Jun 2015 10:26:47 +0000
"Tirdea, Irina" <irina.tirdea@intel.com> wrote:

> > -----Original Message-----
> > From: Antonio Ospite [mailto:ao2@ao2.it]
> > Sent: 28 May, 2015 18:58
> > To: Tirdea, Irina
> > Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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.tirdea@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.tirdea@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.

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 majordomo@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: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@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; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
> 
> On Wed, 3 Jun 2015 10:26:47 +0000
> "Tirdea, Irina" <irina.tirdea@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Antonio Ospite [mailto:ao2@ao2.it]
> > > Sent: 28 May, 2015 18:58
> > > To: Tirdea, Irina
> > > Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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.tirdea@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.tirdea@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 majordomo@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

[Dmitry Torokhov]

On Fri, Jun 05, 2015 at 04:34:38PM +0000, Tirdea, Irina wrote:
> 
> 
> > -----Original Message-----
> > From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@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; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
> > 
> > On Wed, 3 Jun 2015 10:26:47 +0000
> > "Tirdea, Irina" <irina.tirdea@intel.com> wrote:
> > 
> > > > -----Original Message-----
> > > > From: Antonio Ospite [mailto:ao2@ao2.it]
> > > > Sent: 28 May, 2015 18:58
> > > > To: Tirdea, Irina
> > > > Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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.tirdea@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.tirdea@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.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@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.torokhov@gmail.com]
> Sent: 05 June, 2015 19:41
> To: Tirdea, Irina
> Cc: 'Antonio Ospite'; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
> 
> On Fri, Jun 05, 2015 at 04:34:38PM +0000, Tirdea, Irina wrote:
> >
> >
> > > -----Original Message-----
> > > From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@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; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
> > >
> > > On Wed, 3 Jun 2015 10:26:47 +0000
> > > "Tirdea, Irina" <irina.tirdea@intel.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Antonio Ospite [mailto:ao2@ao2.it]
> > > > > Sent: 28 May, 2015 18:58
> > > > > To: Tirdea, Irina
> > > > > Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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.tirdea@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.tirdea@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

[Dmitry Torokhov]

On Fri, Jun 05, 2015 at 05:00:05PM +0000, Tirdea, Irina wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: 05 June, 2015 19:41
> > To: Tirdea, Irina
> > Cc: 'Antonio Ospite'; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
> > 
> > On Fri, Jun 05, 2015 at 04:34:38PM +0000, Tirdea, Irina wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@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; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
> > > >
> > > > On Wed, 3 Jun 2015 10:26:47 +0000
> > > > "Tirdea, Irina" <irina.tirdea@intel.com> wrote:
> > > >
> > > > > > -----Original Message-----
> > > > > > From: Antonio Ospite [mailto:ao2@ao2.it]
> > > > > > Sent: 28 May, 2015 18:58
> > > > > > To: Tirdea, Irina
> > > > > > Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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.tirdea@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.tirdea@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.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@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.torokhov@gmail.com]
> Sent: 05 June, 2015 20:12
> To: Tirdea, Irina
> Cc: 'Antonio Ospite'; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
> 
> On Fri, Jun 05, 2015 at 05:00:05PM +0000, Tirdea, Irina wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > > Sent: 05 June, 2015 19:41
> > > To: Tirdea, Irina
> > > Cc: 'Antonio Ospite'; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
> > >
> > > On Fri, Jun 05, 2015 at 04:34:38PM +0000, Tirdea, Irina wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@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; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > > Subject: Re: [PATCH 2/9] input: goodix: fix variable length array warning
> > > > >
> > > > > On Wed, 3 Jun 2015 10:26:47 +0000
> > > > > "Tirdea, Irina" <irina.tirdea@intel.com> wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Antonio Ospite [mailto:ao2@ao2.it]
> > > > > > > Sent: 28 May, 2015 18:58
> > > > > > > To: Tirdea, Irina
> > > > > > > Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@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.tirdea@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.tirdea@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

[PATCH 3/9] input: goodix: export id and version read from device

[Irina Tirdea]

Goodix touchscreens export through their registers a Product ID
and Firmware Version. The Product ID is an ASCII encoding of the
product name (e.g.: "911").

Export to sysfs (through the input subsystem) the product id and
firmware version read from the device rather than using constant
values.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/input/touchscreen/goodix.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index dac1b3c..9561396 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -47,7 +47,7 @@ struct goodix_ts_data {
 /* Register defines */
 #define GOODIX_READ_COOR_ADDR		0x814E
 #define GOODIX_REG_CONFIG_DATA		0x8047
-#define GOODIX_REG_VERSION		0x8140
+#define GOODIX_REG_ID			0x8140
 
 #define RESOLUTION_LOC		1
 #define MAX_CONTACTS_LOC	5
@@ -231,22 +231,27 @@ static void goodix_read_config(struct goodix_ts_data *ts)
  *
  * @client: the i2c client
  * @version: output buffer containing the version on success
+ * @id: output buffer containing the id on success
  */
-static int goodix_read_version(struct i2c_client *client, u16 *version)
+static int goodix_read_version(struct i2c_client *client, u16 *version, u16 *id)
 {
 	int error;
 	u8 buf[6];
+	char id_str[5];
 
-	error = goodix_i2c_read(client, GOODIX_REG_VERSION, buf, sizeof(buf));
+	error = goodix_i2c_read(client, GOODIX_REG_ID, buf, sizeof(buf));
 	if (error) {
 		dev_err(&client->dev, "read version failed: %d\n", error);
 		return error;
 	}
 
-	if (version)
-		*version = get_unaligned_le16(&buf[4]);
+	memcpy(id_str, buf, 4);
+	id_str[4] = 0;
+	if (kstrtou16(id_str, 10, id))
+		*id = 0x1001;
+	*version = get_unaligned_le16(&buf[4]);
 
-	dev_info(&client->dev, "IC VERSION: %6ph\n", buf);
+	dev_info(&client->dev, "ID %d, version: %04x\n", *id, *version);
 
 	return 0;
 }
@@ -280,10 +285,13 @@ static int goodix_i2c_test(struct i2c_client *client)
  * goodix_request_input_dev - Allocate, populate and register the input device
  *
  * @ts: our goodix_ts_data pointer
+ * @version: device firmware version
+ * @id: device ID
  *
  * Must be called during probe
  */
-static int goodix_request_input_dev(struct goodix_ts_data *ts)
+static int goodix_request_input_dev(struct goodix_ts_data *ts, u16 version,
+				    u16 id)
 {
 	int error;
 
@@ -311,8 +319,8 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
 	ts->input_dev->phys = "input/ts";
 	ts->input_dev->id.bustype = BUS_I2C;
 	ts->input_dev->id.vendor = 0x0416;
-	ts->input_dev->id.product = 0x1001;
-	ts->input_dev->id.version = 10427;
+	ts->input_dev->id.product = id;
+	ts->input_dev->id.version = version;
 
 	error = input_register_device(ts->input_dev);
 	if (error) {
@@ -330,7 +338,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 	struct goodix_ts_data *ts;
 	unsigned long irq_flags;
 	int error;
-	u16 version_info;
+	u16 version_info, id_info;
 
 	dev_dbg(&client->dev, "I2C Address: 0x%02x\n", client->addr);
 
@@ -352,7 +360,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
-	error = goodix_read_version(client, &version_info);
+	error = goodix_read_version(client, &version_info, &id_info);
 	if (error) {
 		dev_err(&client->dev, "Read version failed.\n");
 		return error;
@@ -360,7 +368,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 
 	goodix_read_config(ts);
 
-	error = goodix_request_input_dev(ts);
+	error = goodix_request_input_dev(ts, version_info, id_info);
 	if (error)
 		return error;
 
-- 
1.9.1

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

[Irina Tirdea]

Add ACPI IDs to support Goodix GT911 and GT9271
touchscreens.

Signed-off-by: Irina Tirdea <irina.tirdea@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);
-- 
1.9.1

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

[Bastien Nocera]

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?

> Signed-off-by: Irina Tirdea <irina.tirdea@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 4/9] input: goodix: add ACPI IDs for GT911 and GT9271

[Tirdea, Irina]

> -----Original Message-----
> From: Bastien Nocera [mailto:hadess@hadess.net]
> Sent: 04 June, 2015 15:51
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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.tirdea@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 4/9] input: goodix: add ACPI IDs for GT911 and GT9271

[Dmitry Torokhov]

On Fri, Jun 05, 2015 at 04:36:24PM +0000, Tirdea, Irina wrote:
> 
> 
> > -----Original Message-----
> > From: Bastien Nocera [mailto:hadess@hadess.net]
> > Sent: 04 June, 2015 15:51
> > To: Tirdea, Irina
> > Cc: Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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.

Thanks.

> 
> > > Signed-off-by: Irina Tirdea <irina.tirdea@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

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

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> Sent: 05 June, 2015 19:42
> To: Tirdea, Irina
> Cc: 'Bastien Nocera'; linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 4/9] input: goodix: add ACPI IDs for GT911 and GT9271
> 
> On Fri, Jun 05, 2015 at 04:36:24PM +0000, Tirdea, Irina wrote:
> >
> >
> > > -----Original Message-----
> > > From: Bastien Nocera [mailto:hadess-0MeiytkfxGOsTnJN9+BGXg@public.gmane.org]
> > > Sent: 04 June, 2015 15:51
> > > To: Tirdea, Irina
> > > Cc: Dmitry Torokhov; linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.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.tirdea-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > ---
> > > >  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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Irina Tirdea]

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).

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Irina Tirdea <irina.tirdea@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>
 
 struct goodix_ts_data {
 	struct i2c_client *client;
@@ -34,8 +35,12 @@ struct goodix_ts_data {
 	int abs_y_max;
 	unsigned int max_touch_num;
 	unsigned int int_trigger_type;
+	struct gpio_desc *gpiod_int;
+	struct gpio_desc *gpiod_rst;
 };
 
+#define GOODIX_GPIO_INT_NAME		"irq"
+#define GOODIX_GPIO_RST_NAME		"reset"
 #define GOODIX_MAX_HEIGHT		4096
 #define GOODIX_MAX_WIDTH		4096
 #define GOODIX_INT_TRIGGER		1
@@ -187,6 +192,89 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int goodix_int_sync(struct goodix_ts_data *ts)
+{
+	int ret;
+
+	ret = gpiod_direction_output(ts->gpiod_int, 0);
+	if (ret)
+		return ret;
+
+	return gpiod_direction_input(ts->gpiod_int);
+}
+
+/**
+ * goodix_reset - Reset device during power on
+ *
+ * @ts: goodix_ts_data pointer
+ */
+static int goodix_reset(struct goodix_ts_data *ts)
+{
+	int ret;
+
+	/* begin select I2C slave addr */
+	ret = gpiod_direction_output(ts->gpiod_rst, 0);
+	if (ret)
+		return ret;
+	msleep(20);				/* T2: > 10ms */
+	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
+	ret = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
+	if (ret)
+		return ret;
+	usleep_range(100, 2000);		/* T3: > 100us */
+	ret = gpiod_direction_output(ts->gpiod_rst, 1);
+	if (ret)
+		return ret;
+	usleep_range(6000, 10000);		/* T4: > 5ms */
+	/* end select I2C slave addr */
+	ret = gpiod_direction_input(ts->gpiod_rst);
+	if (ret)
+		return ret;
+	ret = goodix_int_sync(ts);
+	if (ret)
+		return ret;
+	msleep(50);				/* T5: 50ms */
+	return 0;
+}
+
+/**
+ * goodix_get_gpio_config - Get GPIO config from ACPI/DT
+ *
+ * @ts: goodix_ts_data pointer
+ */
+static int goodix_get_gpio_config(struct goodix_ts_data *ts)
+{
+	struct device *dev;
+	struct gpio_desc *gpiod;
+	int ret;
+
+	if (!ts->client)
+		return -EINVAL;
+	dev = &ts->client->dev;
+
+	/* Get interrupt GPIO pin number */
+	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
+	if (IS_ERR(gpiod)) {
+		ret = PTR_ERR(gpiod);
+		dev_err(dev, "Failed to get %s GPIO: %d\n",
+			GOODIX_GPIO_INT_NAME, ret);
+		return ret;
+	}
+	ts->gpiod_int = gpiod;
+
+	/* Get the reset line GPIO pin number */
+	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_RST_NAME, GPIOD_IN);
+	if (IS_ERR(gpiod)) {
+		ret = PTR_ERR(gpiod);
+		dev_err(dev, "Failed to get %s GPIO: %d\n",
+			GOODIX_GPIO_RST_NAME, ret);
+		return ret;
+	}
+	ts->gpiod_rst = gpiod;
+
+	return 0;
+}
+
 /**
  * goodix_read_config - Read the embedded configuration of the panel
  *
@@ -366,6 +454,17 @@ static int goodix_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	error = goodix_get_gpio_config(ts);
+	if (error)
+		return error;
+
+	/* reset the controller */
+	error = goodix_reset(ts);
+	if (error) {
+		dev_err(&client->dev, "Controller reset failed.\n");
+		return error;
+	}
+
 	goodix_read_config(ts);
 
 	error = goodix_request_input_dev(ts, version_info, id_info);
-- 
1.9.1

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

[Mark Rutland]

Hi,

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?

> 
> Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Irina Tirdea <irina.tirdea-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  .../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.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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.rutland@arm.com]
> Sent: 28 May, 2015 16:20
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg&usp=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.purdila@intel.com>
> > Signed-off-by: Irina Tirdea <irina.tirdea@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.

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

[Irina Tirdea]

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.purdila@intel.com>
Signed-off-by: Irina Tirdea <irina.tirdea@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.
+
 Example:
 
 	i2c@00000000 {
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 4405c55..22052c9 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -41,6 +41,8 @@ struct goodix_ts_data {
 
 #define GOODIX_GPIO_INT_NAME		"irq"
 #define GOODIX_GPIO_RST_NAME		"reset"
+#define GOODIX_DEVICE_CONFIG_PROPERTY	"device-config"
+
 #define GOODIX_MAX_HEIGHT		4096
 #define GOODIX_MAX_WIDTH		4096
 #define GOODIX_INT_TRIGGER		1
@@ -94,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, 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 +227,109 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/**
+ * goodix_get_cfg - Get device config from ACPI/DT.
+ *
+ * @ts: goodix_ts_data pointer
+ * @config: pointer to the config buffer
+ * @cfg_len: pointer to size of the config buffer
+ */
+static int goodix_get_cfg(struct goodix_ts_data *ts, u8 **config, int *cfg_len)
+{
+	int ret, i, raw_cfg_len;
+	u8 check_sum = 0;
+
+	ret = device_property_read_u8_array(&ts->client->dev,
+					    GOODIX_DEVICE_CONFIG_PROPERTY,
+					    NULL, 0);
+	if (ret <= 0) {
+		dev_err(&ts->client->dev, "No %s property.\n",
+			GOODIX_DEVICE_CONFIG_PROPERTY);
+		return 0;
+	}
+
+	*cfg_len = ret;
+	if (*cfg_len > GOODIX_CONFIG_MAX_LENGTH) {
+		dev_err(&ts->client->dev,
+			"The length of the config buffer array is not correct");
+		return -EINVAL;
+	}
+
+	*config = kmalloc(*cfg_len, GFP_KERNEL);
+	if (!*config)
+		return -ENOMEM;
+
+	ret = device_property_read_u8_array(&ts->client->dev,
+					    GOODIX_DEVICE_CONFIG_PROPERTY,
+					    *config, *cfg_len);
+	if (ret < 0) {
+		dev_err(&ts->client->dev, "Invalid %s property: %d\n",
+			GOODIX_DEVICE_CONFIG_PROPERTY, ret);
+		goto err_free;
+	}
+
+	raw_cfg_len = *cfg_len - 2;
+	for (i = 0; i < raw_cfg_len; i++)
+		check_sum += (*config)[i];
+	check_sum = (~check_sum) + 1;
+	if (check_sum != (*config)[raw_cfg_len]) {
+		dev_err(&ts->client->dev,
+			"The checksum of the config buffer array is not correct");
+		ret = -EINVAL;
+		goto err_free;
+	}
+	if ((*config)[raw_cfg_len + 1] != 1) {
+		dev_err(&ts->client->dev,
+			"The Config_Fresh register needs to be set");
+		ret = -EINVAL;
+		goto err_free;
+	}
+
+	return 0;
+
+err_free:
+	kfree(*config);
+	return ret;
+}
+
+/**
+ * goodix_send_cfg - Write device config
+ *
+ * @ts: goodix_ts_data pointer
+ */
+static int goodix_send_cfg(struct goodix_ts_data *ts)
+{
+	int ret, cfg_len = 0;
+	u8 *config = NULL;
+
+	ret = goodix_get_cfg(ts, &config, &cfg_len);
+	if (ret)
+		return ret;
+
+	/* No config property in device tree */
+	if (!cfg_len)
+		return 0;
+
+	ret = goodix_i2c_write(ts->client, GOODIX_REG_CONFIG_DATA, config,
+			       cfg_len);
+	if (ret) {
+		dev_err(&ts->client->dev, "Failed to send the config : %d",
+			ret);
+		goto err_free;
+	}
+	dev_dbg(&ts->client->dev, "Config sent successfully.");
+
+	/* Let the firmware reconfigure itself, so sleep for 10ms */
+	usleep_range(10000, 11000);
+
+	kfree(config);
+	return 0;
+
+err_free:
+	kfree(config);
+	return ret;
+}
+
 static int goodix_int_sync(struct goodix_ts_data *ts)
 {
 	int ret;
@@ -465,6 +603,11 @@ static int goodix_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	/* send device configuration to the firmware */
+	error = goodix_send_cfg(ts);
+	if (error)
+		return error;
+
 	goodix_read_config(ts);
 
 	error = goodix_request_input_dev(ts, version_info, id_info);
-- 
1.9.1

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

[Mark Rutland]

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.purdila@intel.com>
> Signed-off-by: Irina Tirdea <irina.tirdea@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?

Mark.

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

[Tirdea, Irina]

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: 28 May, 2015 16:22
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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.purdila@intel.com>
> > Signed-off-by: Irina Tirdea <irina.tirdea@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=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg&usp=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.

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

[Bastien Nocera]

On Thu, 2015-05-28 at 13:51 +0000, Tirdea, Irina wrote:
> 
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > Sent: 28 May, 2015 16:22
> > To: Tirdea, Irina
> > Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org; 
> > devicetree@vger.kernel.org; linux-kernel@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.purdila@intel.com>
> > > Signed-off-by: Irina Tirdea <irina.tirdea@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
> lktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg&usp=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.

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

[Tirdea, Irina]

> -----Original Message-----
> From: Bastien Nocera [mailto:hadess@hadess.net]
> Sent: 04 June, 2015 15:55
> To: Tirdea, Irina
> Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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 +0000, Tirdea, Irina wrote:
> >
> > > -----Original Message-----
> > > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > > Sent: 28 May, 2015 16:22
> > > To: Tirdea, Irina
> > > Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@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.purdila@intel.com>
> > > > Signed-off-by: Irina Tirdea <irina.tirdea@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
> > lktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg&usp=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

[Dmitry Torokhov]

On Fri, Jun 05, 2015 at 04:36:24PM +0000, Tirdea, Irina wrote:
> 
> 
> > -----Original Message-----
> > From: Bastien Nocera [mailto:hadess@hadess.net]
> > Sent: 04 June, 2015 15:55
> > To: Tirdea, Irina
> > Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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 +0000, Tirdea, Irina wrote:
> > >
> > > > -----Original Message-----
> > > > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > > > Sent: 28 May, 2015 16:22
> > > > To: Tirdea, Irina
> > > > Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org;
> > > > devicetree@vger.kernel.org; linux-kernel@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.purdila@intel.com>
> > > > > Signed-off-by: Irina Tirdea <irina.tirdea@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
> > > lktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg&usp=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).

Thanks.

-- 
Dmitry

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

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 05 June, 2015 19:43
> To: Tirdea, Irina
> Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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 +0000, Tirdea, Irina wrote:
> >
> >
> > > -----Original Message-----
> > > From: Bastien Nocera [mailto:hadess@hadess.net]
> > > Sent: 04 June, 2015 15:55
> > > To: Tirdea, Irina
> > > Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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 +0000, Tirdea, Irina wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > > > > Sent: 28 May, 2015 16:22
> > > > > To: Tirdea, Irina
> > > > > Cc: Dmitry Torokhov; Bastien Nocera; linux-input@vger.kernel.org;
> > > > > devicetree@vger.kernel.org; linux-kernel@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.purdila@intel.com>
> > > > > > Signed-off-by: Irina Tirdea <irina.tirdea@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
> > > > lktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg&usp=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

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

[Irina Tirdea]

Implement suspend/resume for goodix driver.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Irina Tirdea <irina.tirdea@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_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, 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);
+
+	ret = goodix_i2c_write_u8(ts->client, GOODIX_REG_COMMAND,
+				  GOODIX_CMD_SCREEN_OFF);
+	if (ret) {
+		dev_err(&ts->client->dev, "Screen off command failed\n");
+		gpiod_direction_input(ts->gpiod_int);
+		goodix_request_irq(ts);
+		return -EAGAIN;
+	}
+
+	/*
+	 * To avoid waking up while is not sleeping,
+	 * delay 58ms to ensure reliability
+	 */
+	msleep(58);
+	return 0;
+}
+
+static int goodix_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct goodix_ts_data *ts = i2c_get_clientdata(client);
+	int ret;
+
+	ret = gpiod_direction_output(ts->gpiod_int, 1);
+	if (ret)
+		return ret;
+	usleep_range(2000, 5000);		/* 2ms~5ms */
+	ret = goodix_int_sync(ts);
+	if (ret)
+		return ret;
+
+	return goodix_request_irq(ts);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend, goodix_resume);
+
 static const struct i2c_device_id goodix_ts_id[] = {
 	{ "GDIX1001:00", 0 },
 	{ }
@@ -663,6 +733,7 @@ static struct i2c_driver goodix_ts_driver = {
 		.owner = THIS_MODULE,
 		.acpi_match_table = ACPI_PTR(goodix_acpi_match),
 		.of_match_table = of_match_ptr(goodix_of_match),
+		.pm = &goodix_pm_ops,
 	},
 };
 module_i2c_driver(goodix_ts_driver);
-- 
1.9.1

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

[Bastien Nocera]

On Thu, 2015-05-28 at 15:47 +0300, Irina Tirdea wrote:
> Implement suspend/resume for goodix driver.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Signed-off-by: Irina Tirdea <irina.tirdea@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_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, 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.

> +
> +     ret = goodix_i2c_write_u8(ts->client, GOODIX_REG_COMMAND,
> +                               GOODIX_CMD_SCREEN_OFF);
> +     if (ret) {
> +             dev_err(&ts->client->dev, "Screen off command 
> failed\n");
> +             gpiod_direction_input(ts->gpiod_int);
> +             goodix_request_irq(ts);
> +             return -EAGAIN;
> +     }
> +
> +     /*
> +      * To avoid waking up while is not sleeping,

You're missing a subject in that sentence.

> +      * delay 58ms to ensure reliability
> +      */
> +     msleep(58);

Why 58ms?

> +     return 0;
> +}
> +
> +static int goodix_resume(struct device *dev)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct goodix_ts_data *ts = i2c_get_clientdata(client);
> +     int ret;
> +
> +     ret = gpiod_direction_output(ts->gpiod_int, 1);
> +     if (ret)
> +             return ret;
> +     usleep_range(2000, 5000);               /* 2ms~5ms */

Good comment, just missing a reference (anything that could help find
that information in the datasheets would be good).

> +     ret = goodix_int_sync(ts);
> +     if (ret)
> +             return ret;
> +
> +     return goodix_request_irq(ts);
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend, 
> goodix_resume);
> +
>  static const struct i2c_device_id goodix_ts_id[] = {
>       { "GDIX1001:00", 0 },
>       { }
> @@ -663,6 +733,7 @@ static struct i2c_driver goodix_ts_driver = {
>               .owner = THIS_MODULE,
>               .acpi_match_table = ACPI_PTR(goodix_acpi_match),
>               .of_match_table = of_match_ptr(goodix_of_match),
> +             .pm = &goodix_pm_ops,
>       },
>  };
>  module_i2c_driver(goodix_ts_driver);

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

[Tirdea, Irina]

> -----Original Message-----
> From: Bastien Nocera [mailto:hadess@hadess.net]
> Sent: 04 June, 2015 16:01
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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.purdila@intel.com>
> > Signed-off-by: Irina Tirdea <irina.tirdea@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_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, 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=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg&usp=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
> > failed\n");
> > +             gpiod_direction_input(ts->gpiod_int);
> > +             goodix_request_irq(ts);
> > +             return -EAGAIN;
> > +     }
> > +
> > +     /*
> > +      * To avoid waking up while is not sleeping,
>
> You're missing a subject in that sentence.

Ok, will fix.

>
> > +      * delay 58ms to ensure reliability
> > +      */
> > +     msleep(58);
>
> Why 58ms?
>

The datasheet specifies that the interval between sending screen-off
command and wake-up should be longer than 58 ms. I will add a comment
stating this.

For the publicly available datasheet for GT9271, you can find this in
Section 7.1 c. Sleep Mode.

> > +     return 0;
> > +}
> > +
> > +static int goodix_resume(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct goodix_ts_data *ts = i2c_get_clientdata(client);
> > +     int ret;
> > +
> > +     ret = gpiod_direction_output(ts->gpiod_int, 1);
> > +     if (ret)
> > +             return ret;
> > +     usleep_range(2000, 5000);               /* 2ms~5ms */
>
> Good comment, just missing a reference (anything that could help find
> that information in the datasheets would be good).
>

Ok, will add a comment describing thiat we have to output low on the
INT pin for 2-5 ms.

For the publicly available datasheet for GT9271, you can find this also in
Section 7.1 c. Sleep Mode.

> > +     ret = goodix_int_sync(ts);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return goodix_request_irq(ts);
> > +}
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend,
> > goodix_resume);
> > +
> >  static const struct i2c_device_id goodix_ts_id[] = {
> >       { "GDIX1001:00", 0 },
> >       { }
> > @@ -663,6 +733,7 @@ static struct i2c_driver goodix_ts_driver = {
> >               .owner = THIS_MODULE,
> >               .acpi_match_table = ACPI_PTR(goodix_acpi_match),
> >               .of_match_table = of_match_ptr(goodix_of_match),
> > +             .pm = &goodix_pm_ops,
> >       },
> >  };
> >  module_i2c_driver(goodix_ts_driver);

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

[Irina Tirdea]

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.tirdea-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 .../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.
 
 Example:
 
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index ce7e834..a41d17b 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -38,11 +38,14 @@ struct goodix_ts_data {
 	struct gpio_desc *gpiod_int;
 	struct gpio_desc *gpiod_rst;
 	unsigned long irq_flags;
+	unsigned int esd_timeout;
+	struct delayed_work esd_work;
 };
 
-#define GOODIX_GPIO_INT_NAME		"irq"
-#define GOODIX_GPIO_RST_NAME		"reset"
-#define GOODIX_DEVICE_CONFIG_PROPERTY	"device-config"
+#define GOODIX_GPIO_INT_NAME			"irq"
+#define GOODIX_GPIO_RST_NAME			"reset"
+#define GOODIX_DEVICE_CONFIG_PROPERTY		"device-config"
+#define GOODIX_DEVICE_ESD_TIMEOUT_PROPERTY	"esd-recovery-timeout-ms"
 
 #define GOODIX_MAX_HEIGHT		4096
 #define GOODIX_MAX_WIDTH		4096
@@ -55,6 +58,8 @@ struct goodix_ts_data {
 /* Register defines */
 #define GOODIX_REG_COMMAND		0x8040
 #define GOODIX_CMD_SCREEN_OFF		0x05
+#define GOODIX_CMD_ESD_ENABLED		0xAA
+#define GOODIX_REG_ESD_CHECK		0x8041
 
 #define GOODIX_READ_COOR_ADDR		0x814E
 #define GOODIX_REG_CONFIG_DATA		0x8047
@@ -396,6 +401,77 @@ static int goodix_reset(struct goodix_ts_data *ts)
 	return 0;
 }
 
+static void goodix_disable_esd(struct goodix_ts_data *ts)
+{
+	if (!ts->esd_timeout)
+		return;
+	cancel_delayed_work_sync(&ts->esd_work);
+}
+
+static int goodix_enable_esd(struct goodix_ts_data *ts)
+{
+	int ret;
+
+	if (!ts->esd_timeout)
+		return 0;
+
+	ret = goodix_i2c_write_u8(ts->client, GOODIX_REG_ESD_CHECK,
+				  GOODIX_CMD_ESD_ENABLED);
+	if (ret) {
+		dev_err(&ts->client->dev, "Failed to enable ESD: %d\n", ret);
+		return ret;
+	}
+
+	schedule_delayed_work(&ts->esd_work, round_jiffies_relative(
+			      msecs_to_jiffies(ts->esd_timeout)));
+	return 0;
+}
+
+static void goodix_esd_work(struct work_struct *work)
+{
+	struct goodix_ts_data *ts = container_of(work, struct goodix_ts_data,
+						 esd_work.work);
+	int retries = 3, ret;
+	u8 esd_data[2];
+
+	while (--retries) {
+		ret = goodix_i2c_read(ts->client, GOODIX_REG_COMMAND, esd_data,
+				      sizeof(esd_data));
+		if (ret)
+			continue;
+		if (esd_data[0] != GOODIX_CMD_ESD_ENABLED &&
+		    esd_data[1] == GOODIX_CMD_ESD_ENABLED) {
+			/* feed the watchdog */
+			goodix_i2c_write_u8(ts->client,
+					    GOODIX_REG_COMMAND,
+					    GOODIX_CMD_ESD_ENABLED);
+			break;
+		}
+	}
+
+	if (!retries) {
+		dev_dbg(&ts->client->dev, "Performing ESD recovery.\n");
+		goodix_free_irq(ts);
+		ret = goodix_reset(ts);
+		if (ret)
+			goto reschedule;
+		ret = goodix_send_cfg(ts);
+		if (ret)
+			goto reschedule;
+		ret = goodix_request_irq(ts);
+		if (ret)
+			goto reschedule;
+		ret = goodix_enable_esd(ts);
+		if (ret)
+			goto reschedule;
+		return;
+	}
+
+reschedule:
+	schedule_delayed_work(&ts->esd_work, round_jiffies_relative(
+			      msecs_to_jiffies(ts->esd_timeout)));
+}
+
 /**
  * goodix_get_gpio_config - Get GPIO config from ACPI/DT
  *
@@ -599,6 +675,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 
 	ts->client = client;
 	i2c_set_clientdata(client, ts);
+	INIT_DELAYED_WORK(&ts->esd_work, goodix_esd_work);
 
 	error = goodix_i2c_test(client);
 	if (error) {
@@ -641,6 +718,21 @@ static int goodix_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	error = device_property_read_u32(&ts->client->dev,
+					 GOODIX_DEVICE_ESD_TIMEOUT_PROPERTY,
+					 &ts->esd_timeout);
+	if (error < 0)
+		dev_err(&ts->client->dev, "No %s property. Will not use ESD.\n",
+			GOODIX_DEVICE_ESD_TIMEOUT_PROPERTY);
+
+	return goodix_enable_esd(ts);
+}
+
+static int goodix_ts_remove(struct i2c_client *client)
+{
+	struct goodix_ts_data *ts = i2c_get_clientdata(client);
+
+	goodix_disable_esd(ts);
 	return 0;
 }
 
@@ -651,6 +743,7 @@ static int goodix_suspend(struct device *dev)
 	struct goodix_ts_data *ts = i2c_get_clientdata(client);
 	int ret;
 
+	goodix_disable_esd(ts);
 	goodix_free_irq(ts);
 	ret = gpiod_direction_output(ts->gpiod_int, 0);
 	if (ret) {
@@ -690,7 +783,11 @@ static int goodix_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	return goodix_request_irq(ts);
+	ret = goodix_request_irq(ts);
+	if (ret)
+		return ret;
+
+	return goodix_enable_esd(ts);
 }
 #endif
 
@@ -727,6 +824,7 @@ MODULE_DEVICE_TABLE(of, goodix_of_match);
 
 static struct i2c_driver goodix_ts_driver = {
 	.probe = goodix_ts_probe,
+	.remove = goodix_ts_remove,
 	.id_table = goodix_ts_id,
 	.driver = {
 		.name = "Goodix-TS",
-- 
1.9.1

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

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

[Mark Rutland]

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.tirdea-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  .../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?

Can we not just choose a sensible default and allow this to be
overridden dynamically?

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

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

[Tirdea, Irina]

> -----Original Message-----
> From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@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; devicetree@vger.kernel.org; linux-kernel@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.tirdea@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.

> 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.

Thanks,
Irina

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

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

[Bastien Nocera]

On Thu, 2015-05-28 at 14:26 +0000, Tirdea, Irina wrote:
> 
> > -----Original Message-----
> > From: linux-input-owner@vger.kernel.org [mailto:
> > linux-input-owner@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; 
> > devicetree@vger.kernel.org; linux-kernel@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.tirdea@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?

> > 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?

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

[Tirdea, Irina]

> -----Original Message-----
> From: Bastien Nocera [mailto:hadess@hadess.net]
> Sent: 04 June, 2015 15:58
> To: Tirdea, Irina
> Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
> 
> On Thu, 2015-05-28 at 14:26 +0000, Tirdea, Irina wrote:
> >
> > > -----Original Message-----
> > > From: linux-input-owner@vger.kernel.org [mailto:
> > > linux-input-owner@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;
> > > devicetree@vger.kernel.org; linux-kernel@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.tirdea@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?

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

[Dmitry Torokhov]

On Fri, Jun 05, 2015 at 04:37:49PM +0000, Tirdea, Irina wrote:
> 
> 
> > -----Original Message-----
> > From: Bastien Nocera [mailto:hadess@hadess.net]
> > Sent: 04 June, 2015 15:58
> > To: Tirdea, Irina
> > Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
> > 
> > On Thu, 2015-05-28 at 14:26 +0000, Tirdea, Irina wrote:
> > >
> > > > -----Original Message-----
> > > > From: linux-input-owner@vger.kernel.org [mailto:
> > > > linux-input-owner@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;
> > > > devicetree@vger.kernel.org; linux-kernel@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.tirdea@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?

I'll leave the decision to DT folks here.

Thanks.

-- 
Dmitry

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

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 05 June, 2015 19:46
> To: Tirdea, Irina
> Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
> 
> On Fri, Jun 05, 2015 at 04:37:49PM +0000, Tirdea, Irina wrote:
> >
> >
> > > -----Original Message-----
> > > From: Bastien Nocera [mailto:hadess@hadess.net]
> > > Sent: 04 June, 2015 15:58
> > > To: Tirdea, Irina
> > > Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
> > >
> > > On Thu, 2015-05-28 at 14:26 +0000, Tirdea, Irina wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: linux-input-owner@vger.kernel.org [mailto:
> > > > > linux-input-owner@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;
> > > > > devicetree@vger.kernel.org; linux-kernel@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.tirdea@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

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

[Tirdea, Irina]

> -----Original Message-----
> From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@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; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 8/9] input: goodix: add support for ESD
> 
> 
> 
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: 05 June, 2015 19:46
> > To: Tirdea, Irina
> > Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
> >
> > On Fri, Jun 05, 2015 at 04:37:49PM +0000, Tirdea, Irina wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Bastien Nocera [mailto:hadess@hadess.net]
> > > > Sent: 04 June, 2015 15:58
> > > > To: Tirdea, Irina
> > > > Cc: Mark Rutland; Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH 8/9] input: goodix: add support for ESD
> > > >
> > > > On Thu, 2015-05-28 at 14:26 +0000, Tirdea, Irina wrote:
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: linux-input-owner@vger.kernel.org [mailto:
> > > > > > linux-input-owner@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;
> > > > > > devicetree@vger.kernel.org; linux-kernel@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.tirdea@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 linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 9/9] input: goodix: use goodix_i2c_write_u8 instead of i2c_master_send

[Irina Tirdea]

Use goodix_i2c_write_u8 instead of i2c_master_send to simplify code.

Signed-off-by: Irina Tirdea <irina.tirdea-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/input/touchscreen/goodix.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index a41d17b..0b3e5aa 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -226,16 +226,11 @@ static void goodix_process_events(struct goodix_ts_data *ts)
  */
 static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
 {
-	static const u8 end_cmd[] = {
-		GOODIX_READ_COOR_ADDR >> 8,
-		GOODIX_READ_COOR_ADDR & 0xff,
-		0
-	};
 	struct goodix_ts_data *ts = dev_id;
 
 	goodix_process_events(ts);
 
-	if (i2c_master_send(ts->client, end_cmd, sizeof(end_cmd)) < 0)
+	if (goodix_i2c_write_u8(ts->client, GOODIX_READ_COOR_ADDR, 0) < 0)
 		dev_err(&ts->client->dev, "I2C write end_cmd error\n");
 
 	return IRQ_HANDLED;
-- 
1.9.1

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

Re: [PATCH 0/9] Goodix touchscreen enhancements

[Bastien Nocera]

Hey Irina,

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.

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?...)

Cheers

RE: [PATCH 0/9] Goodix touchscreen enhancements

[Tirdea, Irina]

> -----Original Message-----
> From: Bastien Nocera [mailto:hadess@hadess.net]
> Sent: 04 June, 2015 16:05
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@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