[PATCH] Input: himax_hx83112b - fix event stack size to clear irq correctly

[PATCH] Input: himax_hx83112b - fix event stack size to clear irq correctly

[Dmitry Mastykin]

The datasheet suggests that the size of the event stack is 32 x 32-bit.
Reading less bytes did not clear the interrupt output. Extra interrupts
were called with zero himax_event_get_num_points() result.
Only every second call to himax_handle_input() cleared the interrupt
output.

Tested on: HX83102J

Signed-off-by: Dmitry Mastykin <mastichi@gmail.com>
---
 drivers/input/touchscreen/himax_hx83112b.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 31f18860dbd3..fe7bdf8cec50 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -24,6 +24,7 @@
 #include <linux/regmap.h>
 
 #define HIMAX_MAX_POINTS		10
+#define HIMAX_EVENT_STACK_SIZE		128
 
 #define HIMAX_AHB_ADDR_BYTE_0			0x00
 #define HIMAX_AHB_ADDR_RDATA_BYTE_0		0x08
@@ -292,9 +293,9 @@ static int hx83100a_read_events(struct himax_ts_data *ts,
 static int himax_handle_input(struct himax_ts_data *ts)
 {
 	int error;
-	struct himax_event event;
+	u8 event[HIMAX_EVENT_STACK_SIZE];
 
-	error = ts->chip->read_events(ts, &event, sizeof(event));
+	error = ts->chip->read_events(ts, (struct himax_event *)&event, sizeof(event));
 	if (error) {
 		dev_err(&ts->client->dev, "Failed to read input event: %d\n",
 			error);
@@ -305,8 +306,8 @@ static int himax_handle_input(struct himax_ts_data *ts)
 	 * Only process the current event when it has a valid checksum but
 	 * don't consider it a fatal error when it doesn't.
 	 */
-	if (himax_verify_checksum(ts, &event))
-		himax_process_event(ts, &event);
+	if (himax_verify_checksum(ts, (struct himax_event *)&event))
+		himax_process_event(ts, (struct himax_event *)&event);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH] Input: himax_hx83112b - fix event stack size to clear irq correctly

[Dmitry Torokhov]

Hi Dmitry,

On Wed, Jan 22, 2025 at 03:14:09PM +0300, Dmitry Mastykin wrote:
> The datasheet suggests that the size of the event stack is 32 x 32-bit.
> Reading less bytes did not clear the interrupt output. Extra interrupts
> were called with zero himax_event_get_num_points() result.
> Only every second call to himax_handle_input() cleared the interrupt
> output.
> 
> Tested on: HX83102J
> 
> Signed-off-by: Dmitry Mastykin <mastichi@gmail.com>
> ---
>  drivers/input/touchscreen/himax_hx83112b.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
> index 31f18860dbd3..fe7bdf8cec50 100644
> --- a/drivers/input/touchscreen/himax_hx83112b.c
> +++ b/drivers/input/touchscreen/himax_hx83112b.c
> @@ -24,6 +24,7 @@
>  #include <linux/regmap.h>
>  
>  #define HIMAX_MAX_POINTS		10
> +#define HIMAX_EVENT_STACK_SIZE		128
>  
>  #define HIMAX_AHB_ADDR_BYTE_0			0x00
>  #define HIMAX_AHB_ADDR_RDATA_BYTE_0		0x08
> @@ -292,9 +293,9 @@ static int hx83100a_read_events(struct himax_ts_data *ts,
>  static int himax_handle_input(struct himax_ts_data *ts)
>  {
>  	int error;
> -	struct himax_event event;
> +	u8 event[HIMAX_EVENT_STACK_SIZE];

If you need this it should go into struct himax_event (wither padding or
making it a union).

Also if this change is needed to make HX83102J operate properly it
should be rolled into the previous patch adding compatibles to the
driver.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: himax_hx83112b - fix event stack size to clear irq correctly

[Dmitry Mastykin]

Thank you for the reply, Dmitry.
My explanation in the commit message was not correct.
I found that the firmware of my HX83102J has stylus support, so it's
enough to read 12 bytes of stylus data in addition to 56 bytes of
fingers to correctly clear the interrupt output. But I'm not sure if
there may also exist firmwares without stylus support for the same
chip.
I see two possibilities:
- introduce stylus presence info in the device-tree, and add
eventually stylus support to the driver;
- read 56+12 (or 128) bytes event stack for every chip. This will
require testing for all supported chips;

Kind regards
Dmitry

On Wed, Jan 22, 2025 at 9:37 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Dmitry,
>
> On Wed, Jan 22, 2025 at 03:14:09PM +0300, Dmitry Mastykin wrote:
> > The datasheet suggests that the size of the event stack is 32 x 32-bit.
> > Reading less bytes did not clear the interrupt output. Extra interrupts
> > were called with zero himax_event_get_num_points() result.
> > Only every second call to himax_handle_input() cleared the interrupt
> > output.
> >
> > Tested on: HX83102J
> >
> > Signed-off-by: Dmitry Mastykin <mastichi@gmail.com>
> > ---
> >  drivers/input/touchscreen/himax_hx83112b.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
> > index 31f18860dbd3..fe7bdf8cec50 100644
> > --- a/drivers/input/touchscreen/himax_hx83112b.c
> > +++ b/drivers/input/touchscreen/himax_hx83112b.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/regmap.h>
> >
> >  #define HIMAX_MAX_POINTS             10
> > +#define HIMAX_EVENT_STACK_SIZE               128
> >
> >  #define HIMAX_AHB_ADDR_BYTE_0                        0x00
> >  #define HIMAX_AHB_ADDR_RDATA_BYTE_0          0x08
> > @@ -292,9 +293,9 @@ static int hx83100a_read_events(struct himax_ts_data *ts,
> >  static int himax_handle_input(struct himax_ts_data *ts)
> >  {
> >       int error;
> > -     struct himax_event event;
> > +     u8 event[HIMAX_EVENT_STACK_SIZE];
>
> If you need this it should go into struct himax_event (wither padding or
> making it a union).
>
> Also if this change is needed to make HX83102J operate properly it
> should be rolled into the previous patch adding compatibles to the
> driver.
>
> Thanks.
>
> --
> Dmitry