iMon Knob driver issue

  Le 29/12/2012 13:31, Alexey Klimov a écrit :
> Hello Alexandre,
> 
> On Sat, Dec 29, 2012 at 4:08 PM, Alexandre LISSY <alexandrelissy@free.fr> wrote:
>> Hello,
>>
>> Please find attached a small patch for the iMon Knob driver. I've been
> 
> Could you please also add your Signed-off-by to the patch? It looks
> like it's missed.
> 

Sure, sorry for forgetting this, here it is !
  

Em Sat, 29 Dec 2012 14:01:57 +0100
Alexandre Lissy <lissyx+mozfr@lissyx.dyndns.org> escreveu:

> From cca7718a9902a4d5cffbf158b5853980a08ef930 Mon Sep 17 00:00:00 2001
> From: Alexandre Lissy <alexandrelissy@free.fr>
> Date: Sun, 2 Sep 2012 20:35:20 +0200
> Subject: [PATCH] fix: iMon Knob event interpretation issues
> 
> Events for the iMon Knob pad where not correctly interpreted, resulting
> in buggy mouse movements (cursor going straight out of the screen), key
> pad only generating KEY_RIGHT and KEY_DOWN events. A reproducer is:
> 
> int main(int argc, char ** argv)
> {
>         char rel_x = 0x00; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
>         rel_x = 0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
>         rel_x |= ~0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
> 
>         return 0;
> }
> 
> (running on x86 or amd64)
> $ ./test
> rel_x:0 @test.c:6
> rel_x:15 @test.c:7
> rel_x:-1 @test.c:8
> 
> (running on armv6)
> rel_x:0 @test.c:6
> rel_x:15 @test.c:7
> rel_x:255 @test.c:8
> 
> Forcing the rel_x and rel_y variables as signed char fixes the issue.
> 
> Signed-off-by: Alexandre Lissy <alexandrelissy@free.fr>
> ---
>  drivers/media/rc/imon.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> index 5dd0386..9d30ca9 100644
> --- a/drivers/media/rc/imon.c
> +++ b/drivers/media/rc/imon.c
> @@ -1225,7 +1225,7 @@ static u32 imon_panel_key_lookup(u64 code)
>  static bool imon_mouse_event(struct imon_context *ictx,
>  			     unsigned char *buf, int len)
>  {
> -	char rel_x = 0x00, rel_y = 0x00;
> +	signed char rel_x = 0x00, rel_y = 0x00;

(c/c Jarod, as he is the maintainer of this driver)

That looks weird, as, AFAIKT "char" is signed. Are you sure that this fix
is correct?

If so, maybe this could be a gcc-version-specific bug. What gcc version are
you using?

Btw, we generally use "s8" type for signed 8bit integers inside the Kernel.

>  	u8 right_shift = 1;
>  	bool mouse_input = true;
>  	int dir = 0;
> @@ -1301,7 +1301,7 @@ static void imon_touch_event(struct imon_context *ictx, unsigned char *buf)
>  static void imon_pad_to_keys(struct imon_context *ictx, unsigned char *buf)
>  {
>  	int dir = 0;
> -	char rel_x = 0x00, rel_y = 0x00;
> +	signed char rel_x = 0x00, rel_y = 0x00;
>  	u16 timeout, threshold;
>  	u32 scancode = KEY_RESERVED;
>  	unsigned long flags;
  

Hello,

I had the same opinion than you at first, yet I found this webpage while
searching for a fix :
http://www.arm.linux.org.uk/docs/faqs/signedchar.php. One line
especially: "The above code is actually buggy in that it assumes that
the type "char" is equivalent to "signed char". The C standards do say
that "char" may either be a "signed char" or "unsigned char" and it is
up to the compilers implementation or the platform which is followed."

This made me think that the fix was the correct one. Maybe the "signed
char" should be changed to s8 as you suggested, however.

Le 01/01/2013 14:12, Mauro Carvalho Chehab a écrit :
> Em Sat, 29 Dec 2012 14:01:57 +0100
> Alexandre Lissy <lissyx+mozfr@lissyx.dyndns.org> escreveu:
> 
>> From cca7718a9902a4d5cffbf158b5853980a08ef930 Mon Sep 17 00:00:00 2001
>> From: Alexandre Lissy <alexandrelissy@free.fr>
>> Date: Sun, 2 Sep 2012 20:35:20 +0200
>> Subject: [PATCH] fix: iMon Knob event interpretation issues
>>
>> Events for the iMon Knob pad where not correctly interpreted, resulting
>> in buggy mouse movements (cursor going straight out of the screen), key
>> pad only generating KEY_RIGHT and KEY_DOWN events. A reproducer is:
>>
>> int main(int argc, char ** argv)
>> {
>>         char rel_x = 0x00; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
>>         rel_x = 0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
>>         rel_x |= ~0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
>>
>>         return 0;
>> }
>>
>> (running on x86 or amd64)
>> $ ./test
>> rel_x:0 @test.c:6
>> rel_x:15 @test.c:7
>> rel_x:-1 @test.c:8
>>
>> (running on armv6)
>> rel_x:0 @test.c:6
>> rel_x:15 @test.c:7
>> rel_x:255 @test.c:8
>>
>> Forcing the rel_x and rel_y variables as signed char fixes the issue.
>>
>> Signed-off-by: Alexandre Lissy <alexandrelissy@free.fr>
>> ---
>>  drivers/media/rc/imon.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
>> index 5dd0386..9d30ca9 100644
>> --- a/drivers/media/rc/imon.c
>> +++ b/drivers/media/rc/imon.c
>> @@ -1225,7 +1225,7 @@ static u32 imon_panel_key_lookup(u64 code)
>>  static bool imon_mouse_event(struct imon_context *ictx,
>>  			     unsigned char *buf, int len)
>>  {
>> -	char rel_x = 0x00, rel_y = 0x00;
>> +	signed char rel_x = 0x00, rel_y = 0x00;
> 
> (c/c Jarod, as he is the maintainer of this driver)
> 
> That looks weird, as, AFAIKT "char" is signed. Are you sure that this fix
> is correct?
> 
> If so, maybe this could be a gcc-version-specific bug. What gcc version are
> you using?
> 
> Btw, we generally use "s8" type for signed 8bit integers inside the Kernel.
> 
>>  	u8 right_shift = 1;
>>  	bool mouse_input = true;
>>  	int dir = 0;
>> @@ -1301,7 +1301,7 @@ static void imon_touch_event(struct imon_context *ictx, unsigned char *buf)
>>  static void imon_pad_to_keys(struct imon_context *ictx, unsigned char *buf)
>>  {
>>  	int dir = 0;
>> -	char rel_x = 0x00, rel_y = 0x00;
>> +	signed char rel_x = 0x00, rel_y = 0x00;
>>  	u16 timeout, threshold;
>>  	u32 scancode = KEY_RESERVED;
>>  	unsigned long flags;
> 

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