[PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Sergei A. Trusov]

On some x86 tablets with a goodix touchscreen the windows logo on the
front is a capacitive home button. Touching this button results in a touch
with bit 4 of the 0th byte set, while normally only the lower 4 bits
are used to indicate the number of touches.

Detect this and report a KEY_LEFTMETA press when this happens. Note the
hardware might support more than one button, the number of a button is
reported by the 'id' byte of coor_data. Now we ignore button id.

Signed-off-by: Sergei A. Trusov <sergei.a.trusov@ya.ru>
---
 drivers/input/touchscreen/goodix.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 240b16f3ee97..a4c17c461792 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -76,6 +76,10 @@ struct goodix_ts_data {
 #define MAX_CONTACTS_LOC	5
 #define TRIGGER_LOC		6
 
+#define GOODIX_TOUCH_NUM_MASK		0x0f
+#define GOODIX_SOFTBUTTON_MASK		0x10
+#define GOODIX_SOFTBUTTON1		KEY_LEFTMETA
+
 static const unsigned long goodix_irq_flags[] = {
 	IRQ_TYPE_EDGE_RISING,
 	IRQ_TYPE_EDGE_FALLING,
@@ -208,7 +212,7 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 	if (!(data[0] & 0x80))
 		return -EAGAIN;
 
-	touch_num = data[0] & 0x0f;
+	touch_num = data[0] & GOODIX_TOUCH_NUM_MASK;
 	if (touch_num > ts->max_touch_num)
 		return -EPROTO;
 
@@ -223,7 +227,7 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 			return error;
 	}
 
-	return touch_num;
+	return data[0];
 }
 
 static void goodix_ts_report_touch(struct goodix_ts_data *ts, u8 *coor_data)
@@ -260,13 +264,17 @@ 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 * GOODIX_MAX_CONTACTS];
-	int touch_num;
+	int touch_num, status;
 	int i;
 
-	touch_num = goodix_ts_read_input_report(ts, point_data);
-	if (touch_num < 0)
+	status = goodix_ts_read_input_report(ts, point_data);
+	if (status < 0)
 		return;
 
+	input_report_key(ts->input_dev, GOODIX_SOFTBUTTON1,
+			!!(status & GOODIX_SOFTBUTTON_MASK));
+
+	touch_num = status & GOODIX_TOUCH_NUM_MASK;
 	for (i = 0; i < touch_num; i++)
 		goodix_ts_report_touch(ts,
 				&point_data[1 + GOODIX_CONTACT_SIZE * i]);
@@ -612,6 +620,8 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
 	ts->input_dev->id.product = ts->id;
 	ts->input_dev->id.version = ts->version;
 
+	input_set_capability(ts->input_dev, EV_KEY, GOODIX_SOFTBUTTON1);
+
 	error = input_register_device(ts->input_dev);
 	if (error) {
 		dev_err(&ts->client->dev,

Re: [PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Hans de Goede]

Hi,

On 17-06-17 11:42, Sergei A. Trusov wrote:
> On some x86 tablets with a goodix touchscreen the windows logo on the
> front is a capacitive home button. Touching this button results in a touch
> with bit 4 of the 0th byte set, while normally only the lower 4 bits
> are used to indicate the number of touches.
> 
> Detect this and report a KEY_LEFTMETA press when this happens. Note the
> hardware might support more than one button, the number of a button is
> reported by the 'id' byte of coor_data. Now we ignore button id.
> 
> Signed-off-by: Sergei A. Trusov <sergei.a.trusov@ya.ru>

The changes look good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Also this makes the capacitive home button on my HP stream 7 work, so:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>   drivers/input/touchscreen/goodix.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 240b16f3ee97..a4c17c461792 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -76,6 +76,10 @@ struct goodix_ts_data {
>   #define MAX_CONTACTS_LOC	5
>   #define TRIGGER_LOC		6
>   
> +#define GOODIX_TOUCH_NUM_MASK		0x0f
> +#define GOODIX_SOFTBUTTON_MASK		0x10
> +#define GOODIX_SOFTBUTTON1		KEY_LEFTMETA
> +
>   static const unsigned long goodix_irq_flags[] = {
>   	IRQ_TYPE_EDGE_RISING,
>   	IRQ_TYPE_EDGE_FALLING,
> @@ -208,7 +212,7 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
>   	if (!(data[0] & 0x80))
>   		return -EAGAIN;
>   
> -	touch_num = data[0] & 0x0f;
> +	touch_num = data[0] & GOODIX_TOUCH_NUM_MASK;
>   	if (touch_num > ts->max_touch_num)
>   		return -EPROTO;
>   
> @@ -223,7 +227,7 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
>   			return error;
>   	}
>   
> -	return touch_num;
> +	return data[0];
>   }
>   
>   static void goodix_ts_report_touch(struct goodix_ts_data *ts, u8 *coor_data)
> @@ -260,13 +264,17 @@ 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 * GOODIX_MAX_CONTACTS];
> -	int touch_num;
> +	int touch_num, status;
>   	int i;
>   
> -	touch_num = goodix_ts_read_input_report(ts, point_data);
> -	if (touch_num < 0)
> +	status = goodix_ts_read_input_report(ts, point_data);
> +	if (status < 0)
>   		return;
>   
> +	input_report_key(ts->input_dev, GOODIX_SOFTBUTTON1,
> +			!!(status & GOODIX_SOFTBUTTON_MASK));
> +
> +	touch_num = status & GOODIX_TOUCH_NUM_MASK;
>   	for (i = 0; i < touch_num; i++)
>   		goodix_ts_report_touch(ts,
>   				&point_data[1 + GOODIX_CONTACT_SIZE * i]);
> @@ -612,6 +620,8 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
>   	ts->input_dev->id.product = ts->id;
>   	ts->input_dev->id.version = ts->version;
>   
> +	input_set_capability(ts->input_dev, EV_KEY, GOODIX_SOFTBUTTON1);
> +
>   	error = input_register_device(ts->input_dev);
>   	if (error) {
>   		dev_err(&ts->client->dev,
> 

Re: [PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Bastien Nocera]

On Sat, 2017-06-17 at 19:42 +1000, Sergei A. Trusov wrote:
> On some x86 tablets with a goodix touchscreen the windows logo on the
> front is a capacitive home button. Touching this button results in a
> touch
> with bit 4 of the 0th byte set, while normally only the lower 4 bits
> are used to indicate the number of touches.
> 
> Detect this and report a KEY_LEFTMETA press when this happens. Note
> the
> hardware might support more than one button, the number of a button
> is
> reported by the 'id' byte of coor_data. Now we ignore button id.
> 
> Signed-off-by: Sergei A. Trusov <sergei.a.trusov@ya.ru>
> ---
>  drivers/input/touchscreen/goodix.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 240b16f3ee97..a4c17c461792 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -76,6 +76,10 @@ struct goodix_ts_data {
>  #define MAX_CONTACTS_LOC	5
>  #define TRIGGER_LOC		6
>  
> +#define GOODIX_TOUCH_NUM_MASK		0x0f
> +#define GOODIX_SOFTBUTTON_MASK		0x10
> +#define GOODIX_SOFTBUTTON1		KEY_LEFTMETA
> +
>  static const unsigned long goodix_irq_flags[] = {
>  	IRQ_TYPE_EDGE_RISING,
>  	IRQ_TYPE_EDGE_FALLING,
> @@ -208,7 +212,7 @@ static int goodix_ts_read_input_report(struct
> goodix_ts_data *ts, u8 *data)
>  	if (!(data[0] & 0x80))
>  		return -EAGAIN;
>  
> -	touch_num = data[0] & 0x0f;
> +	touch_num = data[0] & GOODIX_TOUCH_NUM_MASK;

This should be a separate patch.

>  	if (touch_num > ts->max_touch_num)
>  		return -EPROTO;
>  
> @@ -223,7 +227,7 @@ static int goodix_ts_read_input_report(struct
> goodix_ts_data *ts, u8 *data)
>  			return error;
>  	}
>  
> -	return touch_num;
> +	return data[0];

I really don't like it when we change the meaning of a function's
return value without changing the function's name, or explaining that
we're doing that. This should be a separate patch please.

>  }
>  
>  static void goodix_ts_report_touch(struct goodix_ts_data *ts, u8
> *coor_data)
> @@ -260,13 +264,17 @@ 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 *
> GOODIX_MAX_CONTACTS];
> -	int touch_num;
> +	int touch_num, status;
>  	int i;
>  
> -	touch_num = goodix_ts_read_input_report(ts, point_data);
> -	if (touch_num < 0)
> +	status = goodix_ts_read_input_report(ts, point_data);
> +	if (status < 0)
>  		return;
>  
> +	input_report_key(ts->input_dev, GOODIX_SOFTBUTTON1,
> +			!!(status & GOODIX_SOFTBUTTON_MASK));
> +
> +	touch_num = status & GOODIX_TOUCH_NUM_MASK;
>  	for (i = 0; i < touch_num; i++)
>  		goodix_ts_report_touch(ts,
>  				&point_data[1 + GOODIX_CONTACT_SIZE
> * i]);
> @@ -612,6 +620,8 @@ static int goodix_request_input_dev(struct
> goodix_ts_data *ts)
>  	ts->input_dev->id.product = ts->id;
>  	ts->input_dev->id.version = ts->version;
>  
> +	input_set_capability(ts->input_dev, EV_KEY,
> GOODIX_SOFTBUTTON1);

I don't like this define though, please use KEY_LEFTMETA directly. We
can change it to a constant when we have more than one softbutton.

General question though, should we not only export and advertise the
button if it exists on the device?

> +
>  	error = input_register_device(ts->input_dev);
>  	if (error) {
>  		dev_err(&ts->client->dev,
> 

Re: [PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Dmitry Torokhov]

On Tue, Jun 20, 2017 at 12:32:03AM +0200, Bastien Nocera wrote:
> On Sat, 2017-06-17 at 19:42 +1000, Sergei A. Trusov wrote:
> > On some x86 tablets with a goodix touchscreen the windows logo on the
> > front is a capacitive home button. Touching this button results in a
> > touch
> > with bit 4 of the 0th byte set, while normally only the lower 4 bits
> > are used to indicate the number of touches.
> > 
> > Detect this and report a KEY_LEFTMETA press when this happens. Note
> > the
> > hardware might support more than one button, the number of a button
> > is
> > reported by the 'id' byte of coor_data. Now we ignore button id.
> > 
> > Signed-off-by: Sergei A. Trusov <sergei.a.trusov@ya.ru>
> > ---
> >  drivers/input/touchscreen/goodix.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/goodix.c
> > b/drivers/input/touchscreen/goodix.c
> > index 240b16f3ee97..a4c17c461792 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -76,6 +76,10 @@ struct goodix_ts_data {
> >  #define MAX_CONTACTS_LOC	5
> >  #define TRIGGER_LOC		6
> >  
> > +#define GOODIX_TOUCH_NUM_MASK		0x0f
> > +#define GOODIX_SOFTBUTTON_MASK		0x10
> > +#define GOODIX_SOFTBUTTON1		KEY_LEFTMETA
> > +
> >  static const unsigned long goodix_irq_flags[] = {
> >  	IRQ_TYPE_EDGE_RISING,
> >  	IRQ_TYPE_EDGE_FALLING,
> > @@ -208,7 +212,7 @@ static int goodix_ts_read_input_report(struct
> > goodix_ts_data *ts, u8 *data)
> >  	if (!(data[0] & 0x80))
> >  		return -EAGAIN;
> >  
> > -	touch_num = data[0] & 0x0f;
> > +	touch_num = data[0] & GOODIX_TOUCH_NUM_MASK;
> 
> This should be a separate patch.
> 
> >  	if (touch_num > ts->max_touch_num)
> >  		return -EPROTO;
> >  
> > @@ -223,7 +227,7 @@ static int goodix_ts_read_input_report(struct
> > goodix_ts_data *ts, u8 *data)
> >  			return error;
> >  	}
> >  
> > -	return touch_num;
> > +	return data[0];
> 
> I really don't like it when we change the meaning of a function's
> return value without changing the function's name, or explaining that
> we're doing that. This should be a separate patch please.
> 
> >  }
> >  
> >  static void goodix_ts_report_touch(struct goodix_ts_data *ts, u8
> > *coor_data)
> > @@ -260,13 +264,17 @@ 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 *
> > GOODIX_MAX_CONTACTS];
> > -	int touch_num;
> > +	int touch_num, status;
> >  	int i;
> >  
> > -	touch_num = goodix_ts_read_input_report(ts, point_data);
> > -	if (touch_num < 0)
> > +	status = goodix_ts_read_input_report(ts, point_data);
> > +	if (status < 0)
> >  		return;
> >  
> > +	input_report_key(ts->input_dev, GOODIX_SOFTBUTTON1,
> > +			!!(status & GOODIX_SOFTBUTTON_MASK));
> > +
> > +	touch_num = status & GOODIX_TOUCH_NUM_MASK;
> >  	for (i = 0; i < touch_num; i++)
> >  		goodix_ts_report_touch(ts,
> >  				&point_data[1 + GOODIX_CONTACT_SIZE
> > * i]);
> > @@ -612,6 +620,8 @@ static int goodix_request_input_dev(struct
> > goodix_ts_data *ts)
> >  	ts->input_dev->id.product = ts->id;
> >  	ts->input_dev->id.version = ts->version;
> >  
> > +	input_set_capability(ts->input_dev, EV_KEY,
> > GOODIX_SOFTBUTTON1);
> 
> I don't like this define though, please use KEY_LEFTMETA directly. We
> can change it to a constant when we have more than one softbutton.
> 
> General question though, should we not only export and advertise the
> button if it exists on the device?

Yes, this is preferred behavior.

> 
> > +
> >  	error = input_register_device(ts->input_dev);
> >  	if (error) {
> >  		dev_err(&ts->client->dev,
> > 

-- 
Dmitry

Re: [PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Bastien Nocera]

On Mon, 2017-06-19 at 19:51 -0700, Dmitry Torokhov wrote:
> > General question though, should we not only export and advertise
> > the
> > button if it exists on the device?
> 
> Yes, this is preferred behavior.

So we'll want a dmi match table for the devices that need this feature,
and only export the button for those.

Cheers

Re: [PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Hans de Goede]

Hi,

On 20-06-17 10:36, Bastien Nocera wrote:
> On Mon, 2017-06-19 at 19:51 -0700, Dmitry Torokhov wrote:
>>> General question though, should we not only export and advertise
>>> the
>>> button if it exists on the device?
>>
>> Yes, this is preferred behavior.
> 
> So we'll want a dmi match table for the devices that need this feature,
> and only export the button for those.

Note Dmitry said that this is preferred behavior, not something which
we MUST do. And TBH I don't think that using a DMI table is a good idea,
the capacitive home-button is not only found on the HP stream 7 but also
on various cheap tablets, of which there tend to be a lot, so the DMI
table is going to be use. And the cheap tablets often have useful
model strings such as e.g. "Default String" making DMI matching a
nasty problem.

So IMHO it is better to just always report the presence of the
home button, in practice I don't think userspace is going to have
any problems with this.

Regards,

Hans

Re: [PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Sergei A. Trusov]

On Tuesday, 20 June 2017 08:32:03 +10 Bastien Nocera wrote:
> On Sat, 2017-06-17 at 19:42 +1000, Sergei A. Trusov wrote:
> > On some x86 tablets with a goodix touchscreen the windows logo on the
> > front is a capacitive home button. Touching this button results in a
> > touch
> > with bit 4 of the 0th byte set, while normally only the lower 4 bits
> > are used to indicate the number of touches.
> > 
> > Detect this and report a KEY_LEFTMETA press when this happens. Note
> > the
> > hardware might support more than one button, the number of a button
> > is
> > reported by the 'id' byte of coor_data. Now we ignore button id.
> > 
> > Signed-off-by: Sergei A. Trusov <sergei.a.trusov@ya.ru>
> > ---
> >  drivers/input/touchscreen/goodix.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/goodix.c
> > b/drivers/input/touchscreen/goodix.c
> > index 240b16f3ee97..a4c17c461792 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -76,6 +76,10 @@ struct goodix_ts_data {
> >  #define MAX_CONTACTS_LOC	5
> >  #define TRIGGER_LOC		6
> >  
> > +#define GOODIX_TOUCH_NUM_MASK		0x0f
> > +#define GOODIX_SOFTBUTTON_MASK		0x10
> > +#define GOODIX_SOFTBUTTON1		KEY_LEFTMETA
> > +
> >  static const unsigned long goodix_irq_flags[] = {
> >  	IRQ_TYPE_EDGE_RISING,
> >  	IRQ_TYPE_EDGE_FALLING,
> > @@ -208,7 +212,7 @@ static int goodix_ts_read_input_report(struct
> > goodix_ts_data *ts, u8 *data)
> >  	if (!(data[0] & 0x80))
> >  		return -EAGAIN;
> >  
> > -	touch_num = data[0] & 0x0f;
> > +	touch_num = data[0] & GOODIX_TOUCH_NUM_MASK;
> 
> This should be a separate patch.
> 
> >  	if (touch_num > ts->max_touch_num)
> >  		return -EPROTO;
> >  
> > @@ -223,7 +227,7 @@ static int goodix_ts_read_input_report(struct
> > goodix_ts_data *ts, u8 *data)
> >  			return error;
> >  	}
> >  
> > -	return touch_num;
> > +	return data[0];
> 
> I really don't like it when we change the meaning of a function's
> return value without changing the function's name, or explaining that
> we're doing that. This should be a separate patch please.

Thank you for this point. Now I see all the changes above are redundant:
we can use a value returned in the *data buffer. It is enough to add 
only two lines of code to achieve the behaviour of this entire patch.
> 
> >  }
> >  
> >  static void goodix_ts_report_touch(struct goodix_ts_data *ts, u8
> > *coor_data)
> > @@ -260,13 +264,17 @@ 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 *
> > GOODIX_MAX_CONTACTS];
> > -	int touch_num;
> > +	int touch_num, status;
> >  	int i;
> >  
> > -	touch_num = goodix_ts_read_input_report(ts, point_data);
> > -	if (touch_num < 0)
> > +	status = goodix_ts_read_input_report(ts, point_data);
> > +	if (status < 0)
> >  		return;
> >  
> > +	input_report_key(ts->input_dev, GOODIX_SOFTBUTTON1,
> > +			!!(status & GOODIX_SOFTBUTTON_MASK));

Just this one do the job:
+ input_report_key(ts->input_dev, KEY_LEFTMETA, !!(point_data[0] & 0x10));

> > +
> > +	touch_num = status & GOODIX_TOUCH_NUM_MASK;
> >  	for (i = 0; i < touch_num; i++)
> >  		goodix_ts_report_touch(ts,
> >  				&point_data[1 + GOODIX_CONTACT_SIZE
> > * i]);
> > @@ -612,6 +620,8 @@ static int goodix_request_input_dev(struct
> > goodix_ts_data *ts)
> >  	ts->input_dev->id.product = ts->id;
> >  	ts->input_dev->id.version = ts->version;
> >  
> > +	input_set_capability(ts->input_dev, EV_KEY,
> > GOODIX_SOFTBUTTON1);
> 
> I don't like this define though, please use KEY_LEFTMETA directly. We
> can change it to a constant when we have more than one softbutton.

Ok.

> General question though, should we not only export and advertise the
> button if it exists on the device?
> 
> > +
> >  	error = input_register_device(ts->input_dev);
> >  	if (error) {
> >  		dev_err(&ts->client->dev,
> > 
> --
> 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] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Bastien Nocera]

On Tue, 2017-06-20 at 19:26 +1000, Sergei A. Trusov wrote:
> 
<snip>
> Thank you for this point. Now I see all the changes above are
> redundant:
> we can use a value returned in the *data buffer. It is enough to add 
> only two lines of code to achieve the behaviour of this entire patch.

A bit more with a comment explaining the reason behind those 2 lines,
but shorter patches get my vote ;)

Re: [PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Sergei A. Trusov]

On Tuesday, 20 June 2017 19:29:01 +10 Bastien Nocera wrote:
> On Tue, 2017-06-20 at 19:26 +1000, Sergei A. Trusov wrote:
> > 
> <snip>
> > Thank you for this point. Now I see all the changes above are
> > redundant:
> > we can use a value returned in the *data buffer. It is enough to add 
> > only two lines of code to achieve the behaviour of this entire patch.
> 
> A bit more with a comment explaining the reason behind those 2 lines,
> but shorter patches get my vote ;)

Current goodix_ts_read_input_report() function puts data from hardware
into point_data buffer. Then in masks high niblle of the first byte and 
returns it as touch_num. We need this masked nibble to check, so I was 
trying to remove mask operation. After your comment I just realized that
proposed patch should be much shorter. I have included more context lines
to make cleaner my attempts to explain (as I am affraid my English is not
so good):

---
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 240b16f3ee97..0c3825c39288 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -262,16 +262,18 @@ static void goodix_process_events(struct goodix_ts_data *ts)
 	u8  point_data[1 + GOODIX_CONTACT_SIZE * GOODIX_MAX_CONTACTS];
 	int touch_num;
 	int i;
 
 	touch_num = goodix_ts_read_input_report(ts, point_data);
 	if (touch_num < 0)
 		return;
 
+	input_report_key(ts->input_dev, KEY_LEFTMETA, !!(point_data[0] & 0x10));
+
 	for (i = 0; i < touch_num; i++)
 		goodix_ts_report_touch(ts,
 				&point_data[1 + GOODIX_CONTACT_SIZE * i]);
 
 	input_mt_sync_frame(ts->input_dev);
 	input_sync(ts->input_dev);
 }
 
@@ -607,16 +609,18 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
 
 	ts->input_dev->name = "Goodix Capacitive TouchScreen";
 	ts->input_dev->phys = "input/ts";
 	ts->input_dev->id.bustype = BUS_I2C;
 	ts->input_dev->id.vendor = 0x0416;
 	ts->input_dev->id.product = ts->id;
 	ts->input_dev->id.version = ts->version;
 
+	input_set_capability(ts->input_dev, EV_KEY, KEY_LEFTMETA);
+
 	error = input_register_device(ts->input_dev);
 	if (error) {
 		dev_err(&ts->client->dev,
 			"Failed to register input device: %d", error);
 		return error;
 	}
 
 	return 0;

Re: [PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Bastien Nocera]

On Tue, 2017-06-20 at 20:11 +1000, Sergei A. Trusov wrote:
> On Tuesday, 20 June 2017 19:29:01 +10 Bastien Nocera wrote:
> > On Tue, 2017-06-20 at 19:26 +1000, Sergei A. Trusov wrote:
> > > 
> > 
> > <snip>
> > > Thank you for this point. Now I see all the changes above are
> > > redundant:
> > > we can use a value returned in the *data buffer. It is enough to add 
> > > only two lines of code to achieve the behaviour of this entire patch.
> > 
> > A bit more with a comment explaining the reason behind those 2 lines,
> > but shorter patches get my vote ;)
> 
> Current goodix_ts_read_input_report() function puts data from hardware
> into point_data buffer. Then in masks high niblle of the first byte and 
> returns it as touch_num. We need this masked nibble to check, so I was 
> trying to remove mask operation. After your comment I just realized that
> proposed patch should be much shorter. I have included more context lines
> to make cleaner my attempts to explain (as I am affraid my English is not
> so good):

Do you want to re-send with the comments added as below?

> ---
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 240b16f3ee97..0c3825c39288 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -262,16 +262,18 @@ static void goodix_process_events(struct goodix_ts_data *ts)
>  	u8  point_data[1 + GOODIX_CONTACT_SIZE * GOODIX_MAX_CONTACTS];
>  	int touch_num;
>  	int i;
>  
>  	touch_num = goodix_ts_read_input_report(ts, point_data);
>  	if (touch_num < 0)
>  		return;
>  
> +	input_report_key(ts->input_dev, KEY_LEFTMETA, !!(point_data[0] & 0x10));

You can add a comment above:
Bit 4 of the first byte reports the status of the capacitive
Windows/Home button.

>  	for (i = 0; i < touch_num; i++)
>  		goodix_ts_report_touch(ts,
>  				&point_data[1 + GOODIX_CONTACT_SIZE * i]);
>  
>  	input_mt_sync_frame(ts->input_dev);
>  	input_sync(ts->input_dev);
>  }
>  
> @@ -607,16 +609,18 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
>  
>  	ts->input_dev->name = "Goodix Capacitive TouchScreen";
>  	ts->input_dev->phys = "input/ts";
>  	ts->input_dev->id.bustype = BUS_I2C;
>  	ts->input_dev->id.vendor = 0x0416;
>  	ts->input_dev->id.product = ts->id;
>  	ts->input_dev->id.version = ts->version;
>  
> +	input_set_capability(ts->input_dev, EV_KEY, KEY_LEFTMETA);

Add a comment above:
Capacitive Windows/Home button on some devices

>  	error = input_register_device(ts->input_dev);
>  	if (error) {
>  		dev_err(&ts->client->dev,
>  			"Failed to register input device: %d", error);
>  		return error;
>  	}
>  
>  	return 0;
> 

Re: [PATCH] Input: goodix: Add support for capacitive home button found on some x86 tablets

[Sergei A. Trusov]

On Tuesday, 20 June 2017 20:20:03 +10 Bastien Nocera wrote:
> On Tue, 2017-06-20 at 20:11 +1000, Sergei A. Trusov wrote:
> > On Tuesday, 20 June 2017 19:29:01 +10 Bastien Nocera wrote:
> > > On Tue, 2017-06-20 at 19:26 +1000, Sergei A. Trusov wrote:
> > > > 
> > > 
> > > <snip>
> > > > Thank you for this point. Now I see all the changes above are
> > > > redundant:
> > > > we can use a value returned in the *data buffer. It is enough to add 
> > > > only two lines of code to achieve the behaviour of this entire patch.
> > > 
> > > A bit more with a comment explaining the reason behind those 2 lines,
> > > but shorter patches get my vote ;)
> > 
> > Current goodix_ts_read_input_report() function puts data from hardware
> > into point_data buffer. Then in masks high niblle of the first byte and 
> > returns it as touch_num. We need this masked nibble to check, so I was 
> > trying to remove mask operation. After your comment I just realized that
> > proposed patch should be much shorter. I have included more context lines
> > to make cleaner my attempts to explain (as I am affraid my English is not
> > so good):
> 
> Do you want to re-send with the comments added as below?

Yes, thank you, I'll add those and send v2.
> 
> > ---
> > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > index 240b16f3ee97..0c3825c39288 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -262,16 +262,18 @@ static void goodix_process_events(struct goodix_ts_data *ts)
> >  	u8  point_data[1 + GOODIX_CONTACT_SIZE * GOODIX_MAX_CONTACTS];
> >  	int touch_num;
> >  	int i;
> >  
> >  	touch_num = goodix_ts_read_input_report(ts, point_data);
> >  	if (touch_num < 0)
> >  		return;
> >  
> > +	input_report_key(ts->input_dev, KEY_LEFTMETA, !!(point_data[0] & 0x10));
> 
> You can add a comment above:
> Bit 4 of the first byte reports the status of the capacitive
> Windows/Home button.
> 
> >  	for (i = 0; i < touch_num; i++)
> >  		goodix_ts_report_touch(ts,
> >  				&point_data[1 + GOODIX_CONTACT_SIZE * i]);
> >  
> >  	input_mt_sync_frame(ts->input_dev);
> >  	input_sync(ts->input_dev);
> >  }
> >  
> > @@ -607,16 +609,18 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
> >  
> >  	ts->input_dev->name = "Goodix Capacitive TouchScreen";
> >  	ts->input_dev->phys = "input/ts";
> >  	ts->input_dev->id.bustype = BUS_I2C;
> >  	ts->input_dev->id.vendor = 0x0416;
> >  	ts->input_dev->id.product = ts->id;
> >  	ts->input_dev->id.version = ts->version;
> >  
> > +	input_set_capability(ts->input_dev, EV_KEY, KEY_LEFTMETA);
> 
> Add a comment above:
> Capacitive Windows/Home button on some devices
> 
> >  	error = input_register_device(ts->input_dev);
> >  	if (error) {
> >  		dev_err(&ts->client->dev,
> >  			"Failed to register input device: %d", error);
> >  		return error;
> >  	}
> >  
> >  	return 0;
> > 
> --
> 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
>