[57/98] sony-laptop: Add support for extra keyboard events

Message ID	200903291838.14723.matze@welwarsky.de (mailing list archive)
State	Superseded, archived
Headers	show Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n2TGdKIc003733 for <patchwork-acpi@patchwork.kernel.org>; Sun, 29 Mar 2009 16:39:20 GMT From: Matthias Welwarsky <matze@welwarsky.de> To: Matthew Garrett <mjg59@srcf.ucam.org> Subject: Re: [PATCH 57/98] sony-laptop: Add support for extra keyboard events Date: Sun, 29 Mar 2009 18:38:14 +0200 Cc: Len Brown <lenb@kernel.org>, linux-acpi@vger.kernel.org, Mattia Dongili <malattia@linux.it>, Len Brown <len.brown@intel.com> References: <1238214733-8285-1-git-send-email-lenb@kernel.org> <200903291751.45455.matze@welwarsky.de> <20090329160212.GA9129@srcf.ucam.org> In-Reply-To: <20090329160212.GA9129@srcf.ucam.org> MIME-Version: 1.0 Content-Disposition: inline Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <200903291838.14723.matze@welwarsky.de> Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk

Message ID

200903291838.14723.matze@welwarsky.de (mailing list archive)

State

Superseded, archived

Headers

From: Matthias Welwarsky <matze@welwarsky.de>
To: Matthew Garrett <mjg59@srcf.ucam.org>
Subject: Re: [PATCH 57/98] sony-laptop: Add support for extra keyboard events
Date: Sun, 29 Mar 2009 18:38:14 +0200
Cc: Len Brown <lenb@kernel.org>, linux-acpi@vger.kernel.org,
	Mattia Dongili <malattia@linux.it>, Len Brown <len.brown@intel.com>
References: <1238214733-8285-1-git-send-email-lenb@kernel.org>
	<200903291751.45455.matze@welwarsky.de>
	<20090329160212.GA9129@srcf.ucam.org>
In-Reply-To: <20090329160212.GA9129@srcf.ucam.org>
MIME-Version: 1.0
Content-Disposition: inline
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Message-Id: <200903291838.14723.matze@welwarsky.de>
Sender: linux-acpi-owner@vger.kernel.org
Precedence: bulk

Commit Message

Matthias Welwarsky March 29, 2009, 4:38 p.m. UTC

On Sunday 29 March 2009 18:02:12 Matthew Garrett wrote:
> On Sun, Mar 29, 2009 at 05:51:45PM +0200, Matthias Welwarsky wrote:
> > before the call to acpi_bus_generate_proc_event(). It was called
> > unconditionally, with whatever value ev happened to have at that time,
> > i.e. also unprocessed events < 0x90 or events where the translation
> > failed. Moving the call makes sure that only properly processed events
> > are reported. Without, I was seeing a lot of bogus events reported, e.g.
> > volume-down when the stamina switch was flipped, or when the rf kill
> > switch was operated.
>
> Oh, I see. No, you can't just remove it from there - that'll break
> older-style events. I'd add it there without removing the original, but
> then make the rest of the block conditional on the original event not
> being >= 0x90.

Whoops, just noticed that there was a bug in my patch. No wonder it confused 
you.  Should untranslated events >= 0x90 still generate ACPI events? In that 
case, the following patch would fix it. If not, the acpi event reporting must 
be moved into the else path, too.


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

Comments

Matthew Garrett March 29, 2009, 4:41 p.m. UTC | #1

On Sun, Mar 29, 2009 at 06:38:14PM +0200, Matthias Welwarsky wrote:

> Whoops, just noticed that there was a bug in my patch. No wonder it confused 
> you.  Should untranslated events >= 0x90 still generate ACPI events? In that 
> case, the following patch would fix it. If not, the acpi event reporting must 
> be moved into the else path, too.

I don't think it's likely to be useful - if we ever implement support 
for them then we'll generate different events and generate user 
confusion.

> --- sony-laptop.c.orig	2009-03-29 12:41:44.000000000 +0200
> +++ sony-laptop.c	2009-03-29 18:36:31.000000000 +0200
> @@ -865,7 +865,7 @@
>  static struct sony_nc_event sony_100_events[] = {
>  	{ 0x90, SONYPI_EVENT_PKEY_P1 },
>  	{ 0x10, SONYPI_EVENT_ANYBUTTON_RELEASED },
> -	{ 0x91, SONYPI_EVENT_PKEY_P1 },
> +	{ 0x91, SONYPI_EVENT_PKEY_P2 },
>  	{ 0x11, SONYPI_EVENT_ANYBUTTON_RELEASED },
>  	{ 0x81, SONYPI_EVENT_FNKEY_F1 },
>  	{ 0x01, SONYPI_EVENT_FNKEY_RELEASED },
> @@ -929,7 +929,7 @@
>  		if (sony_find_snc_handle(0x127) == ev)
>  			key_handle = 0x127;
>  
> -		if (handle) {
> +		if (key_handle) {
>  			struct sony_nc_event *key_event;
>  
>  			if (sony_call_snc_handle(key_handle, 0x200, &result))
> @@ -955,15 +955,16 @@
>  				printk(KERN_INFO DRV_PFX
>  				       "Unknown event: 0x%x 0x%x\n", key_handle,
>  				       ev);
> -			}
> +			} else
> +				sony_laptop_report_input_event(ev);
>  		} else if (sony_find_snc_handle(0x124) == ev) {
>  			sony_nc_rfkill_update();
>  			return;
>  		}
> -	}
> +	} else
> +		sony_laptop_report_input_event(ev);
>  
>  	dprintk("sony_acpi_notify, event: 0x%.2x\n", ev);
> -	sony_laptop_report_input_event(ev);
>  	acpi_bus_generate_proc_event(sony_nc_acpi_device, 1, ev);
>  }

Yes, that looks good, just move the acpi_bus_generate_proc_event into 
the same blocks as sony_laptop_report_input_event.

Acked-by: Matthew Garrett <mjg@redhat.com>

Mattia Dongili March 30, 2009, 3:52 a.m. UTC | #2

On Sun, Mar 29, 2009 at 05:41:07PM +0100, Matthew Garrett wrote:
> On Sun, Mar 29, 2009 at 06:38:14PM +0200, Matthias Welwarsky wrote:
> 
> > Whoops, just noticed that there was a bug in my patch. No wonder it confused 
> > you.  Should untranslated events >= 0x90 still generate ACPI events? In that 
> > case, the following patch would fix it. If not, the acpi event reporting must 
> > be moved into the else path, too.
> 
> I don't think it's likely to be useful - if we ever implement support 
> for them then we'll generate different events and generate user 
> confusion.

yes, but up to 2.6.29 they are being reported for all the models
supported by the former SNC initialization code. I'd rather keep them
until /proc/acpi/event goes away.

> > --- sony-laptop.c.orig	2009-03-29 12:41:44.000000000 +0200
> > +++ sony-laptop.c	2009-03-29 18:36:31.000000000 +0200
> > @@ -865,7 +865,7 @@
> >  static struct sony_nc_event sony_100_events[] = {
> >  	{ 0x90, SONYPI_EVENT_PKEY_P1 },
> >  	{ 0x10, SONYPI_EVENT_ANYBUTTON_RELEASED },
> > -	{ 0x91, SONYPI_EVENT_PKEY_P1 },
> > +	{ 0x91, SONYPI_EVENT_PKEY_P2 },
> >  	{ 0x11, SONYPI_EVENT_ANYBUTTON_RELEASED },
> >  	{ 0x81, SONYPI_EVENT_FNKEY_F1 },
> >  	{ 0x01, SONYPI_EVENT_FNKEY_RELEASED },
> > @@ -929,7 +929,7 @@
> >  		if (sony_find_snc_handle(0x127) == ev)
> >  			key_handle = 0x127;
> >  
> > -		if (handle) {
> > +		if (key_handle) {
> >  			struct sony_nc_event *key_event;
> >  
> >  			if (sony_call_snc_handle(key_handle, 0x200, &result))
> > @@ -955,15 +955,16 @@
> >  				printk(KERN_INFO DRV_PFX
> >  				       "Unknown event: 0x%x 0x%x\n", key_handle,
> >  				       ev);
> > -			}
> > +			} else
> > +				sony_laptop_report_input_event(ev);
> >  		} else if (sony_find_snc_handle(0x124) == ev) {
> >  			sony_nc_rfkill_update();
> >  			return;
> >  		}
> > -	}
> > +	} else
> > +		sony_laptop_report_input_event(ev);
> >  
> >  	dprintk("sony_acpi_notify, event: 0x%.2x\n", ev);
> > -	sony_laptop_report_input_event(ev);
> >  	acpi_bus_generate_proc_event(sony_nc_acpi_device, 1, ev);
> >  }
> 
> Yes, that looks good, just move the acpi_bus_generate_proc_event into 
> the same blocks as sony_laptop_report_input_event.
> 
> Acked-by: Matthew Garrett <mjg@redhat.com>

I'll take this version of the patch.

Len, this patch isn't upstream yet, would you prefer me to resend
only this one with the fixes from Matthias merged in or do you want it
as a separate fix?

Also, Matthias can we have your signoff line?
Thanks

Matthias Welwarsky March 30, 2009, 6:20 a.m. UTC | #3

On Monday 30 March 2009 05:52:39 Mattia Dongili wrote:
> On Sun, Mar 29, 2009 at 05:41:07PM +0100, Matthew Garrett wrote:
> > On Sun, Mar 29, 2009 at 06:38:14PM +0200, Matthias Welwarsky wrote:
> > > Whoops, just noticed that there was a bug in my patch. No wonder it
> > > confused you.  Should untranslated events >= 0x90 still generate ACPI
> > > events? In that case, the following patch would fix it. If not, the
> > > acpi event reporting must be moved into the else path, too.
> >
> > I don't think it's likely to be useful - if we ever implement support
> > for them then we'll generate different events and generate user
> > confusion.
>
> yes, but up to 2.6.29 they are being reported for all the models
> supported by the former SNC initialization code. I'd rather keep them
> until /proc/acpi/event goes away.

Hm, but if you keep them, there would be the possibility of ambiguous events. 
Events >= 0x90 are being translated and possibly (mostly) become < 0x90, so 
there's something like a namespace clash here. I think there should be a 
marker for translated events.

> > > --- sony-laptop.c.orig	2009-03-29 12:41:44.000000000 +0200
> > > +++ sony-laptop.c	2009-03-29 18:36:31.000000000 +0200
> > > @@ -865,7 +865,7 @@
> > >  static struct sony_nc_event sony_100_events[] = {
> > >  	{ 0x90, SONYPI_EVENT_PKEY_P1 },
> > >  	{ 0x10, SONYPI_EVENT_ANYBUTTON_RELEASED },
> > > -	{ 0x91, SONYPI_EVENT_PKEY_P1 },
> > > +	{ 0x91, SONYPI_EVENT_PKEY_P2 },
> > >  	{ 0x11, SONYPI_EVENT_ANYBUTTON_RELEASED },
> > >  	{ 0x81, SONYPI_EVENT_FNKEY_F1 },
> > >  	{ 0x01, SONYPI_EVENT_FNKEY_RELEASED },
> > > @@ -929,7 +929,7 @@
> > >  		if (sony_find_snc_handle(0x127) == ev)
> > >  			key_handle = 0x127;
> > >
> > > -		if (handle) {
> > > +		if (key_handle) {
> > >  			struct sony_nc_event *key_event;
> > >
> > >  			if (sony_call_snc_handle(key_handle, 0x200, &result))
> > > @@ -955,15 +955,16 @@
> > >  				printk(KERN_INFO DRV_PFX
> > >  				       "Unknown event: 0x%x 0x%x\n", key_handle,
> > >  				       ev);
> > > -			}
> > > +			} else
> > > +				sony_laptop_report_input_event(ev);
> > >  		} else if (sony_find_snc_handle(0x124) == ev) {
> > >  			sony_nc_rfkill_update();
> > >  			return;
> > >  		}
> > > -	}
> > > +	} else
> > > +		sony_laptop_report_input_event(ev);
> > >
> > >  	dprintk("sony_acpi_notify, event: 0x%.2x\n", ev);
> > > -	sony_laptop_report_input_event(ev);
> > >  	acpi_bus_generate_proc_event(sony_nc_acpi_device, 1, ev);
> > >  }
> >
> > Yes, that looks good, just move the acpi_bus_generate_proc_event into
> > the same blocks as sony_laptop_report_input_event.
> >
> > Acked-by: Matthew Garrett <mjg@redhat.com>
>
> I'll take this version of the patch.

What's about rfkill then? Should rfkill hardware switch activations also send 
an acpi event in addition? Currently they don't.

> Len, this patch isn't upstream yet, would you prefer me to resend
> only this one with the fixes from Matthias merged in or do you want it
> as a separate fix?
>
> Also, Matthias can we have your signoff line?

I'll resend the patch with a proper sign-off once we're clear about these 
questions, ok?

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

--- sony-laptop.c.orig	2009-03-29 12:41:44.000000000 +0200
+++ sony-laptop.c	2009-03-29 18:36:31.000000000 +0200
@@ -865,7 +865,7 @@ 
 static struct sony_nc_event sony_100_events[] = {
 	{ 0x90, SONYPI_EVENT_PKEY_P1 },
 	{ 0x10, SONYPI_EVENT_ANYBUTTON_RELEASED },
-	{ 0x91, SONYPI_EVENT_PKEY_P1 },
+	{ 0x91, SONYPI_EVENT_PKEY_P2 },
 	{ 0x11, SONYPI_EVENT_ANYBUTTON_RELEASED },
 	{ 0x81, SONYPI_EVENT_FNKEY_F1 },
 	{ 0x01, SONYPI_EVENT_FNKEY_RELEASED },
@@ -929,7 +929,7 @@ 
 		if (sony_find_snc_handle(0x127) == ev)
 			key_handle = 0x127;
 
-		if (handle) {
+		if (key_handle) {
 			struct sony_nc_event *key_event;
 
 			if (sony_call_snc_handle(key_handle, 0x200, &result))
@@ -955,15 +955,16 @@ 
 				printk(KERN_INFO DRV_PFX
 				       "Unknown event: 0x%x 0x%x\n", key_handle,
 				       ev);
-			}
+			} else
+				sony_laptop_report_input_event(ev);
 		} else if (sony_find_snc_handle(0x124) == ev) {
 			sony_nc_rfkill_update();
 			return;
 		}
-	}
+	} else
+		sony_laptop_report_input_event(ev);
 
 	dprintk("sony_acpi_notify, event: 0x%.2x\n", ev);
-	sony_laptop_report_input_event(ev);
 	acpi_bus_generate_proc_event(sony_nc_acpi_device, 1, ev);
 }

[57/98] sony-laptop: Add support for extra keyboard events

Commit Message

Comments

Patch