[PATCH 1/2] Input: edt-ft5x06 - use get_unaligned_be16()

[PATCH 1/2] Input: edt-ft5x06 - use get_unaligned_be16()

[Dmitry Torokhov]

Instead of doing conversion by hand, let's use the proper accessors.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index c639ebce914c..ec770226e119 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -27,6 +27,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/input/mt.h>
 #include <linux/input/touchscreen.h>
+#include <asm/unaligned.h>
 
 #define WORK_REGISTER_THRESHOLD		0x00
 #define WORK_REGISTER_REPORT_RATE	0x08
@@ -239,8 +240,8 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
 		if (tsdata->version == EDT_M06 && type == TOUCH_EVENT_DOWN)
 			continue;
 
-		x = ((buf[0] << 8) | buf[1]) & 0x0fff;
-		y = ((buf[2] << 8) | buf[3]) & 0x0fff;
+		x = get_unaligned_be16(buf) & 0x0fff;
+		y = get_unaligned_be16(buf + 2) & 0x0fff;
 		/* The FT5x26 send the y coordinate first */
 		if (tsdata->version == EV_FT)
 			swap(x, y);
-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH 2/2] Input: edt-ft5x06 - simplify event reporting code

[Dmitry Torokhov]

Now that input_mt_report_slot_state() returns true if slot is active we no
longer need a temporary for the slot state.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index ec770226e119..3cc4341bbdff 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -229,7 +229,6 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
 
 	for (i = 0; i < tsdata->max_support_points; i++) {
 		u8 *buf = &rdbuf[i * tplen + offset];
-		bool down;
 
 		type = buf[0] >> 6;
 		/* ignore Reserved events */
@@ -247,16 +246,12 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
 			swap(x, y);
 
 		id = (buf[2] >> 4) & 0x0f;
-		down = type != TOUCH_EVENT_UP;
 
 		input_mt_slot(tsdata->input, id);
-		input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, down);
-
-		if (!down)
-			continue;
-
-		touchscreen_report_pos(tsdata->input, &tsdata->prop, x, y,
-				       true);
+		if (input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER,
+					       type != TOUCH_EVENT_UP))
+			touchscreen_report_pos(tsdata->input, &tsdata->prop,
+					       x, y, true);
 	}
 
 	input_mt_report_pointer_emulation(tsdata->input, true);
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH 2/2] Input: edt-ft5x06 - simplify event reporting code

[Andy Shevchenko]

On Sun, Jun 23, 2019 at 9:31 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Now that input_mt_report_slot_state() returns true if slot is active we no
> longer need a temporary for the slot state.


> -               down = type != TOUCH_EVENT_UP;
>
>                 input_mt_slot(tsdata->input, id);
> -               input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, down);

> +               if (input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER,
> +                                              type != TOUCH_EVENT_UP))

Can't we simple do somethink like
-               down = type != TOUCH_EVENT_UP;
+               down = input_mt_report_slot_state(tsdata->input,
MT_TOOL_FINGER, type != TOUCH_EVENT_UP);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] Input: edt-ft5x06 - use get_unaligned_be16()

[Andy Shevchenko]

On Sun, Jun 23, 2019 at 9:31 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Instead of doing conversion by hand, let's use the proper accessors.
>

The code looks fine to me,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

I can test it later next week (Wednesday or so).

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index c639ebce914c..ec770226e119 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -27,6 +27,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/input/mt.h>
>  #include <linux/input/touchscreen.h>
> +#include <asm/unaligned.h>
>
>  #define WORK_REGISTER_THRESHOLD                0x00
>  #define WORK_REGISTER_REPORT_RATE      0x08
> @@ -239,8 +240,8 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
>                 if (tsdata->version == EDT_M06 && type == TOUCH_EVENT_DOWN)
>                         continue;
>
> -               x = ((buf[0] << 8) | buf[1]) & 0x0fff;
> -               y = ((buf[2] << 8) | buf[3]) & 0x0fff;
> +               x = get_unaligned_be16(buf) & 0x0fff;
> +               y = get_unaligned_be16(buf + 2) & 0x0fff;
>                 /* The FT5x26 send the y coordinate first */
>                 if (tsdata->version == EV_FT)
>                         swap(x, y);
> --
> 2.22.0.410.gd8fdbe21b5-goog
>


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/2] Input: edt-ft5x06 - use get_unaligned_be16()

[David Laight]

From: Dmitry Torokhov
> Sent: 23 June 2019 07:32
> 
> Instead of doing conversion by hand, let's use the proper accessors.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index c639ebce914c..ec770226e119 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -27,6 +27,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/input/mt.h>
>  #include <linux/input/touchscreen.h>
> +#include <asm/unaligned.h>
> 
>  #define WORK_REGISTER_THRESHOLD		0x00
>  #define WORK_REGISTER_REPORT_RATE	0x08
> @@ -239,8 +240,8 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
>  		if (tsdata->version == EDT_M06 && type == TOUCH_EVENT_DOWN)
>  			continue;
> 
> -		x = ((buf[0] << 8) | buf[1]) & 0x0fff;
> -		y = ((buf[2] << 8) | buf[3]) & 0x0fff;
> +		x = get_unaligned_be16(buf) & 0x0fff;
> +		y = get_unaligned_be16(buf + 2) & 0x0fff;

You might as well delete the pointless masking with 0xff.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/2] Input: edt-ft5x06 - use get_unaligned_be16()

[Andy Shevchenko]

On Tue, Jun 25, 2019 at 11:44 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Dmitry Torokhov
> > Sent: 23 June 2019 07:32
> >
> > Instead of doing conversion by hand, let's use the proper accessors.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/touchscreen/edt-ft5x06.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index c639ebce914c..ec770226e119 100644
> > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/input/mt.h>
> >  #include <linux/input/touchscreen.h>
> > +#include <asm/unaligned.h>
> >
> >  #define WORK_REGISTER_THRESHOLD              0x00
> >  #define WORK_REGISTER_REPORT_RATE    0x08
> > @@ -239,8 +240,8 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
> >               if (tsdata->version == EDT_M06 && type == TOUCH_EVENT_DOWN)
> >                       continue;
> >
> > -             x = ((buf[0] << 8) | buf[1]) & 0x0fff;
> > -             y = ((buf[2] << 8) | buf[3]) & 0x0fff;
> > +             x = get_unaligned_be16(buf) & 0x0fff;
> > +             y = get_unaligned_be16(buf + 2) & 0x0fff;
>
> You might as well delete the pointless masking with 0xff.

Hmm... Does it guarantee the most significant nibble to be always 0?
(Note 16-bit value and three f:s in the mask)


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/2] Input: edt-ft5x06 - use get_unaligned_be16()

[David Laight]

From: Andy Shevchenko
> Sent: 25 June 2019 11:50
> To: David Laight
> Cc: Dmitry Torokhov; linux-input@vger.kernel.org; Marco Felsch; Benoit Parrot; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] Input: edt-ft5x06 - use get_unaligned_be16()
> 
> On Tue, Jun 25, 2019 at 11:44 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Dmitry Torokhov
> > > Sent: 23 June 2019 07:32
> > >
> > > Instead of doing conversion by hand, let's use the proper accessors.
> > >
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/input/touchscreen/edt-ft5x06.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > index c639ebce914c..ec770226e119 100644
> > > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > > @@ -27,6 +27,7 @@
> > >  #include <linux/gpio/consumer.h>
> > >  #include <linux/input/mt.h>
> > >  #include <linux/input/touchscreen.h>
> > > +#include <asm/unaligned.h>
> > >
> > >  #define WORK_REGISTER_THRESHOLD              0x00
> > >  #define WORK_REGISTER_REPORT_RATE    0x08
> > > @@ -239,8 +240,8 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
> > >               if (tsdata->version == EDT_M06 && type == TOUCH_EVENT_DOWN)
> > >                       continue;
> > >
> > > -             x = ((buf[0] << 8) | buf[1]) & 0x0fff;
> > > -             y = ((buf[2] << 8) | buf[3]) & 0x0fff;
> > > +             x = get_unaligned_be16(buf) & 0x0fff;
> > > +             y = get_unaligned_be16(buf + 2) & 0x0fff;
> >
> > You might as well delete the pointless masking with 0xff.
> 
> Hmm... Does it guarantee the most significant nibble to be always 0?
> (Note 16-bit value and three f:s in the mask)

Sorry, I misread it :-(

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/2] Input: edt-ft5x06 - use get_unaligned_be16()

[Benoit Parrot]

Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote on Sat [2019-Jun-22 23:31:52 -0700]:
> Instead of doing conversion by hand, let's use the proper accessors.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index c639ebce914c..ec770226e119 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -27,6 +27,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/input/mt.h>
>  #include <linux/input/touchscreen.h>
> +#include <asm/unaligned.h>
>  
>  #define WORK_REGISTER_THRESHOLD		0x00
>  #define WORK_REGISTER_REPORT_RATE	0x08
> @@ -239,8 +240,8 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
>  		if (tsdata->version == EDT_M06 && type == TOUCH_EVENT_DOWN)
>  			continue;
>  
> -		x = ((buf[0] << 8) | buf[1]) & 0x0fff;
> -		y = ((buf[2] << 8) | buf[3]) & 0x0fff;
> +		x = get_unaligned_be16(buf) & 0x0fff;
> +		y = get_unaligned_be16(buf + 2) & 0x0fff;

Appears to work fine in my test:

Tested-by: Benoit Parrot <bparrot@ti.com>

>  		/* The FT5x26 send the y coordinate first */
>  		if (tsdata->version == EV_FT)
>  			swap(x, y);
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
> 

Re: [PATCH 2/2] Input: edt-ft5x06 - simplify event reporting code

[Benoit Parrot]

Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote on Sat [2019-Jun-22 23:31:53 -0700]:
> Now that input_mt_report_slot_state() returns true if slot is active we no
> longer need a temporary for the slot state.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Appears to work fine in my test:

Tested-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index ec770226e119..3cc4341bbdff 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -229,7 +229,6 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
>  
>  	for (i = 0; i < tsdata->max_support_points; i++) {
>  		u8 *buf = &rdbuf[i * tplen + offset];
> -		bool down;
>  
>  		type = buf[0] >> 6;
>  		/* ignore Reserved events */
> @@ -247,16 +246,12 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
>  			swap(x, y);
>  
>  		id = (buf[2] >> 4) & 0x0f;
> -		down = type != TOUCH_EVENT_UP;
>  
>  		input_mt_slot(tsdata->input, id);
> -		input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, down);
> -
> -		if (!down)
> -			continue;
> -
> -		touchscreen_report_pos(tsdata->input, &tsdata->prop, x, y,
> -				       true);
> +		if (input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER,
> +					       type != TOUCH_EVENT_UP))
> +			touchscreen_report_pos(tsdata->input, &tsdata->prop,
> +					       x, y, true);
>  	}
>  
>  	input_mt_report_pointer_emulation(tsdata->input, true);
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
> 

Re: [PATCH 2/2] Input: edt-ft5x06 - simplify event reporting code

[Dmitry Torokhov]

On Sun, Jun 23, 2019 at 10:59:18AM +0300, Andy Shevchenko wrote:
> On Sun, Jun 23, 2019 at 9:31 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Now that input_mt_report_slot_state() returns true if slot is active we no
> > longer need a temporary for the slot state.
> 
> 
> > -               down = type != TOUCH_EVENT_UP;
> >
> >                 input_mt_slot(tsdata->input, id);
> > -               input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, down);
> 
> > +               if (input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER,
> > +                                              type != TOUCH_EVENT_UP))
> 
> Can't we simple do somethink like
> -               down = type != TOUCH_EVENT_UP;
> +               down = input_mt_report_slot_state(tsdata->input,
> MT_TOOL_FINGER, type != TOUCH_EVENT_UP);

Why though? The temporary was needed so we did not have to repeat the
expression for "contact down" condition, and now we do not need it. The
whole change was done so that we cab remove the temporary...

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: edt-ft5x06 - simplify event reporting code

[Andy Shevchenko]

On Sun, Jun 30, 2019 at 10:05 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Sun, Jun 23, 2019 at 10:59:18AM +0300, Andy Shevchenko wrote:
> > On Sun, Jun 23, 2019 at 9:31 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Now that input_mt_report_slot_state() returns true if slot is active we no
> > > longer need a temporary for the slot state.

> > > -               down = type != TOUCH_EVENT_UP;
> > >
> > >                 input_mt_slot(tsdata->input, id);
> > > -               input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, down);
> >
> > > +               if (input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER,
> > > +                                              type != TOUCH_EVENT_UP))
> >
> > Can't we simple do somethink like
> > -               down = type != TOUCH_EVENT_UP;
> > +               down = input_mt_report_slot_state(tsdata->input,
> > MT_TOOL_FINGER, type != TOUCH_EVENT_UP);
>
> Why though? The temporary was needed so we did not have to repeat the
> expression for "contact down" condition, and now we do not need it. The
> whole change was done so that we cab remove the temporary...

I see. Thanks for explanation.

-- 
With Best Regards,
Andy Shevchenko