Re: [PATCH] Fujitsu Amilo PA 1510 key-release events quirk

Re: [PATCH] Fujitsu Amilo PA 1510 key-release events quirk

[Andrew Morton]

On Sat, 29 Nov 2008 22:59:36 +0100 Daniel Mierswa <impulze@impulze.org> wrote:

> heya,
> the fujitsu amilo pa 1510 laptop needs a little lovin' regarding the 
> release event of the volume up and down keys. I attached a format-patch 
> (against torvalds/linux-2.6.git ed313489badef16d7) which separates the 
> generation of release events for certain scancodes into a macro since 3 
> dmi matches are already using it and generation of the missing release 
> events for that particular laptop. Please post back with any advice and 
> thoughts for that patch.
> 

Please don't send more than one patch per email.  I shall comment on
the two patches in two separate replies.

> From a197dfb0eed23b87418c0a7fe6a562910046576e Mon Sep 17 00:00:00 2001
> From: Daniel Mierswa <impulze@impulze.org>
> Date: Sat, 29 Nov 2008 22:39:34 +0100
> Subject: [PATCH] Separate macro for generation of keyrelease events
> 
> ---
>  drivers/input/keyboard/atkbd.c |   23 +++++++++++++----------
>  1 files changed, 13 insertions(+), 10 deletions(-)

Please also cc linux-input@vger.kernel.org on input-related patches.

> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 22016ca..99ef522 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -834,6 +834,17 @@ static void atkbd_disconnect(struct serio *serio)
>  }
>  
>  /*
> + * generate release events for the keycodes given in forced_release_keys[]
> + */
> +#define GEN_RELEASE_EVENT \
> +	int i; \
> +\
> +	if (atkbd->set == 2) \
> +		for (i = 0; i < ARRAY_SIZE(forced_release_keys); i++) \
> +			__set_bit(forced_release_keys[i], \
> +				atkbd->force_release_mask);
> +
> +/*
>   * Most special keys (Fn+F?) on Dell laptops do not generate release
>   * events so we have to do it ourselves.
>   */
> @@ -842,12 +853,8 @@ static void atkbd_dell_laptop_keymap_fixup(struct atkbd *atkbd)
>  	const unsigned int forced_release_keys[] = {
>  		0x85, 0x86, 0x87, 0x88, 0x89, 0x8a, 0x8b, 0x8f, 0x93,
>  	};
> -	int i;
>  
> -	if (atkbd->set == 2)
> -		for (i = 0; i < ARRAY_SIZE(forced_release_keys); i++)
> -			__set_bit(forced_release_keys[i],
> -				  atkbd->force_release_mask);
> +	GEN_RELEASE_EVENT
>  }
>  
>  /*
> @@ -859,12 +866,8 @@ static void atkbd_hp_keymap_fixup(struct atkbd *atkbd)
>  	const unsigned int forced_release_keys[] = {
>  		0x94,
>  	};
> -	int i;
>  
> -	if (atkbd->set == 2)
> -		for (i = 0; i < ARRAY_SIZE(forced_release_keys); i++)
> -			__set_bit(forced_release_keys[i],
> -					atkbd->force_release_mask);
> +	GEN_RELEASE_EVENT
>  }

oh my.  Please, no.  Just write a C function:

void atkbd_gen_release_event(unsigned int *keys, unsigned nr_keys)
{
	...
}

Re: [PATCH] Fujitsu Amilo PA 1510 key-release events quirk

[Andrew Morton]

On Sat, 29 Nov 2008 22:59:36 +0100 Daniel Mierswa <impulze@impulze.org> wrote:

> From: Daniel Mierswa <impulze@impulze.org>
> Date: Sat, 29 Nov 2008 22:40:20 +0100
> Subject: [PATCH] Fujitsu Siemens Amilo PA 1510 quirks
> 
> The volume up and down keys on the Fujitsu Siemens Amilo PA 1510 laptop
> won't generate release events, so we have to do that. Use the same
> way that is already used with other models.
> ---
>  drivers/input/keyboard/atkbd.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 99ef522..1250242 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -845,6 +845,19 @@ static void atkbd_disconnect(struct serio *serio)
>  				atkbd->force_release_mask);
>  
>  /*
> + * The volume up and volume down special keys on a Fujitsu Amilo PA 1510 laptop
> + * do not generate release events so we have to do it ourselves.
> + */
> +static void atkbd_amilopa1510_keymap_fixup(struct atkbd *atkbd)
> +{
> +	const unsigned int forced_release_keys[] = {
> +		0xb0, 0xae,
> +	};
> +
> +	GEN_RELEASE_EVENT
> +}

It's best to make forced_release_keys static as well - there is no need
to build this array on the stack at runtime.  The compiler _could_
perform this optimisation itself andallegedly it will sometimes do so. 
Last time I checked it did not.

> +/*
>   * Most special keys (Fn+F?) on Dell laptops do not generate release
>   * events so we have to do it ourselves.
>   */
> @@ -1463,6 +1476,15 @@ static struct dmi_system_id atkbd_dmi_quirk_table[] __initdata = {
>  		.driver_data = atkbd_dell_laptop_keymap_fixup,
>  	},
>  	{
> +		.ident = "Fujitsu Amilo PA 1510",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"), 
> +			DMI_MATCH(DMI_PRODUCT_NAME, "AMILO Pa 1510"),
> +		},
> +		.callback = atkbd_setup_fixup,
> +		.driver_data = atkbd_amilopa1510_keymap_fixup,
> +	},
> +	{
>  		.ident = "HP 2133",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),

Re: [PATCH] Fujitsu Amilo PA 1510 key-release events quirk

[Andrew Morton]

On Sat, 29 Nov 2008 23:38:26 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:

> Just write a C function:
> 
> void atkbd_gen_release_event(unsigned int *keys, unsigned nr_keys)

                               ^ const

> {
> 	...
> }
> 
> 

Re: [PATCH] Fujitsu Amilo PA 1510 key-release events quirk (PATCH 1/3)

[Daniel Mierswa]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

Split patches as requested in "[PATCH] Fujitsu Amilo PA 1510 key-release
events quirk"
Here's the first out of 3.


On 30.11.2008 08:38, Andrew Morton wrote:
> Please don't send more than one patch per email.  I shall comment on
> the two patches in two separate replies.
>   
done, except that they are three now :)


On 30.11.2008 08:38, Andrew Morton wrote:
> Please also cc linux-input@vger.kernel.org on input-related patches.
>
>   
done


On 30.11.2008 08:38, Andrew Morton wrote:
> oh my.  Please, no.  Just write a C function:
>
> void atkbd_gen_release_event(unsigned int *keys, unsigned nr_keys)
> {
> 	...
> }
>
>   
done, I prefer that myself, although I first thought it might be a bad
idea to pass around pointers there, since
other developers might expect ARRAY_SIZE to work in gen_release_events.

-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply 
know better than you do.
                --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22



[-- Attachment #2: atkbd_gen_release_events.patch --]
[-- Type: text/plain, Size: 1910 bytes --]

>From f4889ca0b6895abdd1888cb1b56dd411045780be Mon Sep 17 00:00:00 2001
From: Daniel Mierswa <impulze@impulze.org>
Date: Sun, 30 Nov 2008 13:14:00 +0100
Subject: [PATCH] use function for generation of keyrelease events

---
 drivers/input/keyboard/atkbd.c |   27 +++++++++++++++++----------
 1 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 22016ca..c239745 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -834,6 +834,19 @@ static void atkbd_disconnect(struct serio *serio)
 }
 
 /*
+ * generate release events for the keycodes given in keys[]
+ */
+static void atkbd_gen_release_event(struct atkbd* atkbd, const unsigned int *keys,
+			unsigned int nr_keys)
+{
+	unsigned int i;
+
+	if (atkbd->set == 2)
+		for (i = 0; i < nr_keys; i++)
+			__set_bit(keys[i], atkbd->force_release_mask);
+}
+
+/*
  * Most special keys (Fn+F?) on Dell laptops do not generate release
  * events so we have to do it ourselves.
  */
@@ -842,12 +855,9 @@ static void atkbd_dell_laptop_keymap_fixup(struct atkbd *atkbd)
 	const unsigned int forced_release_keys[] = {
 		0x85, 0x86, 0x87, 0x88, 0x89, 0x8a, 0x8b, 0x8f, 0x93,
 	};
-	int i;
 
-	if (atkbd->set == 2)
-		for (i = 0; i < ARRAY_SIZE(forced_release_keys); i++)
-			__set_bit(forced_release_keys[i],
-				  atkbd->force_release_mask);
+	atkbd_gen_release_event(atkbd, forced_release_keys,
+		ARRAY_SIZE(forced_release_keys));
 }
 
 /*
@@ -859,12 +869,9 @@ static void atkbd_hp_keymap_fixup(struct atkbd *atkbd)
 	const unsigned int forced_release_keys[] = {
 		0x94,
 	};
-	int i;
 
-	if (atkbd->set == 2)
-		for (i = 0; i < ARRAY_SIZE(forced_release_keys); i++)
-			__set_bit(forced_release_keys[i],
-					atkbd->force_release_mask);
+	atkbd_gen_release_event(atkbd, forced_release_keys,
+		ARRAY_SIZE(forced_release_keys));
 }
 
 /*
-- 
1.6.0.4

Re: [PATCH] Fujitsu Amilo PA 1510 key-release events quirk (PATCH 2/3)

[Daniel Mierswa]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

Split patches as requested in "[PATCH] Fujitsu Amilo PA 1510 key-release
events quirk"

On 30.11.2008 08:41, Andrew Morton wrote:
> It's best to make forced_release_keys static as well - there is no need
> to build this array on the stack at runtime
done.

-- 

Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply 
know better than you do.
                --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22



[-- Attachment #2: atkbd_static_arrays.patch --]
[-- Type: text/plain, Size: 1102 bytes --]

>From 3b4eeb11c3dc09005fdd10d3fb22fb2267fd0d39 Mon Sep 17 00:00:00 2001
From: Daniel Mierswa <impulze@impulze.org>
Date: Sun, 30 Nov 2008 13:14:45 +0100
Subject: [PATCH] make forced_release_keys static

---
 drivers/input/keyboard/atkbd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index c239745..11ded7e 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -852,7 +852,7 @@ static void atkbd_gen_release_event(struct atkbd* atkbd, const unsigned int *key
  */
 static void atkbd_dell_laptop_keymap_fixup(struct atkbd *atkbd)
 {
-	const unsigned int forced_release_keys[] = {
+	static const unsigned int forced_release_keys[] = {
 		0x85, 0x86, 0x87, 0x88, 0x89, 0x8a, 0x8b, 0x8f, 0x93,
 	};
 
@@ -866,7 +866,7 @@ static void atkbd_dell_laptop_keymap_fixup(struct atkbd *atkbd)
  */
 static void atkbd_hp_keymap_fixup(struct atkbd *atkbd)
 {
-	const unsigned int forced_release_keys[] = {
+	static const unsigned int forced_release_keys[] = {
 		0x94,
 	};
 
-- 
1.6.0.4

Re: [PATCH] Fujitsu Amilo PA 1510 key-release events quirk (PATCH 3/3)

[Daniel Mierswa]

[-- Attachment #1: Type: text/plain, Size: 278 bytes --]

Split patches as requested in "[PATCH] Fujitsu Amilo PA 1510 key-release
events quirk"

-- 

Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply 
know better than you do.
                --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22



[-- Attachment #2: fujitsu-siemens-amilopa1510-quirks.patch --]
[-- Type: text/plain, Size: 1736 bytes --]

>From 1a786f4d720ad0dbaf4b8a03a5237b8433753a4d Mon Sep 17 00:00:00 2001
From: Daniel Mierswa <impulze@impulze.org>
Date: Sun, 30 Nov 2008 13:17:01 +0100
Subject: [PATCH] Fujitsu Siemens Amilo PA 1510 quirks

The volume up and down keys on the Fujitsu Siemens Amilo PA 1510 laptop
won't generate release events, so we have to do that. Use the same
way that is already used with other models.
---
 drivers/input/keyboard/atkbd.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 11ded7e..7347cd4 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -875,6 +875,20 @@ static void atkbd_hp_keymap_fixup(struct atkbd *atkbd)
 }
 
 /*
+ * The volume up and volume down special keys on a Fujitsu Amilo PA 1510 laptop
+ * do not generate release events so we have to do it ourselves.
+ */
+static void atkbd_amilopa1510_keymap_fixup(struct atkbd *atkbd)
+{
+	static const unsigned int forced_release_keys[] = {
+		0xb0, 0xae,
+	};
+
+	atkbd_gen_release_event(atkbd, forced_release_keys,
+		ARRAY_SIZE(forced_release_keys));
+}
+
+/*
  * atkbd_set_keycode_table() initializes keyboard's keycode table
  * according to the selected scancode set
  */
@@ -1475,6 +1489,15 @@ static struct dmi_system_id atkbd_dmi_quirk_table[] __initdata = {
 		.callback = atkbd_setup_fixup,
 		.driver_data = atkbd_hp_keymap_fixup,
 	},
+	{
+		.ident = "Fujitsu Amilo PA 1510",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "AMILO Pa 1510"),
+		},
+		.callback = atkbd_setup_fixup,
+		.driver_data = atkbd_amilopa1510_keymap_fixup,
+	},
 	{ }
 };
 
-- 
1.6.0.4

Re: [PATCH] Fujitsu Amilo PA 1510 key-release events quirk (PATCH 3/3)

[Andrew Morton]

On Mon, 01 Dec 2008 22:01:26 +0100 Daniel Mierswa <impulze@impulze.org> wrote:

> Split patches as requested in "[PATCH] Fujitsu Amilo PA 1510 key-release
> events quirk"

Ho hum.  Please see Documentation/SubmittingPatches - I think you
managed to get everything in there wrong ;)

I unpickled the patches.  Please send a Signed-off-by: for this work.