[patch 2/2] pcspeaker driver update

[patch 2/2] pcspeaker driver update

[Stas Sergeev]

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

Hello.

Attached patch does the following:
- changes the pcspeaker driver to
use the i8253_lock instead of i8253_beep_lock
- adds the SND_SUSPEND event that allows
to disable and re-enable the driver. It
is necessary, for example, for the PCM
PC-Speaker driver (which is currently in
an ALSA CVS), as it doesn't want to
interfere with the pcspkr, so it needs
some way to disable the thing.

Can this please be applied?

Signed-off-by: stsp@aknet.ru



[-- Attachment #2: pcspkr-2.6.12-rc6.diff --]
[-- Type: text/x-patch, Size: 3093 bytes --]

diff -urN linux-2.6.12-rc6/drivers/input/misc/pcspkr.c linux-2.6.12-rc6-spinlk/drivers/input/misc/pcspkr.c
--- linux-2.6.12-rc6/drivers/input/misc/pcspkr.c	2005-06-07 11:11:49.000000000 +0400
+++ linux-2.6.12-rc6-spinlk/drivers/input/misc/pcspkr.c	2005-06-14 12:57:59.000000000 +0400
@@ -18,6 +18,7 @@
 #include <linux/input.h>
 #include <asm/8253pit.h>
 #include <asm/io.h>
+#include <asm/timer.h>
 
 MODULE_AUTHOR("Vojtech Pavlik <vojtech@ucw.cz>");
 MODULE_DESCRIPTION("PC Speaker beeper driver");
@@ -26,27 +27,13 @@
 static char pcspkr_name[] = "PC Speaker";
 static char pcspkr_phys[] = "isa0061/input0";
 static struct input_dev pcspkr_dev;
+enum { PCSPKR_NORMAL, PCSPKR_SUSPENDED };
 
-static DEFINE_SPINLOCK(i8253_beep_lock);
-
-static int pcspkr_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
+static void pcspkr_do_sound(unsigned int count)
 {
-	unsigned int count = 0;
 	unsigned long flags;
 
-	if (type != EV_SND)
-		return -1;
-
-	switch (code) {
-		case SND_BELL: if (value) value = 1000;
-		case SND_TONE: break;
-		default: return -1;
-	}
-
-	if (value > 20 && value < 32767)
-		count = PIT_TICK_RATE / value;
-
-	spin_lock_irqsave(&i8253_beep_lock, flags);
+	spin_lock_irqsave(&i8253_lock, flags);
 
 	if (count) {
 		/* enable counter 2 */
@@ -61,7 +48,32 @@
 		outb(inb_p(0x61) & 0xFC, 0x61);
 	}
 
-	spin_unlock_irqrestore(&i8253_beep_lock, flags);
+	spin_unlock_irqrestore(&i8253_lock, flags);
+}
+
+static int pcspkr_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
+{
+	unsigned int count = 0;
+
+	if (type != EV_SND)
+		return -1;
+
+	switch (code) {
+		case SND_BELL: if (value) value = 1000;
+		case SND_TONE: break;
+		case SND_SUSPEND:
+			dev->state = value ? PCSPKR_SUSPENDED : PCSPKR_NORMAL;
+			return 0;
+		default: return -1;
+	}
+
+	if (dev->state == PCSPKR_SUSPENDED)
+		return 0;
+
+	if (value > 20 && value < 32767)
+		count = PIT_TICK_RATE / value;
+
+	pcspkr_do_sound(count);
 
 	return 0;
 }
@@ -69,7 +81,7 @@
 static int __init pcspkr_init(void)
 {
 	pcspkr_dev.evbit[0] = BIT(EV_SND);
-	pcspkr_dev.sndbit[0] = BIT(SND_BELL) | BIT(SND_TONE);
+	pcspkr_dev.sndbit[0] = BIT(SND_BELL) | BIT(SND_TONE) | BIT(SND_SUSPEND);
 	pcspkr_dev.event = pcspkr_event;
 
 	pcspkr_dev.name = pcspkr_name;
@@ -78,6 +90,7 @@
 	pcspkr_dev.id.vendor = 0x001f;
 	pcspkr_dev.id.product = 0x0001;
 	pcspkr_dev.id.version = 0x0100;
+	pcspkr_dev.state = PCSPKR_NORMAL;
 
 	input_register_device(&pcspkr_dev);
 
@@ -90,7 +103,7 @@
 {
         input_unregister_device(&pcspkr_dev);
 	/* turn off the speaker */
-	pcspkr_event(NULL, EV_SND, SND_BELL, 0);
+	pcspkr_do_sound(0);
 }
 
 module_init(pcspkr_init);
diff -urN linux-2.6.12-rc6/include/linux/input.h linux-2.6.12-rc6-spinlk/include/linux/input.h
--- linux-2.6.12-rc6/include/linux/input.h	2005-06-07 11:12:29.000000000 +0400
+++ linux-2.6.12-rc6-spinlk/include/linux/input.h	2005-06-14 12:58:41.000000000 +0400
@@ -593,6 +593,7 @@
 #define SND_CLICK		0x00
 #define SND_BELL		0x01
 #define SND_TONE		0x02
+#define SND_SUSPEND		0x03
 #define SND_MAX			0x07
 
 /*

Re: [patch 2/2] pcspeaker driver update

[Vojtech Pavlik]

On Tue, Jun 14, 2005 at 10:45:27PM +0400, Stas Sergeev wrote:

> Hello.
> 
> Attached patch does the following:
> - changes the pcspeaker driver to
> use the i8253_lock instead of i8253_beep_lock

This doesn't seem right. The driver programs an independent part of the
chip and I don't see a reason to cause the time code to wait because
we're trying to do a beep.

There is a reason for two separate locks.

> - adds the SND_SUSPEND event that allows
> to disable and re-enable the driver. It
> is necessary, for example, for the PCM
> PC-Speaker driver (which is currently in
> an ALSA CVS), as it doesn't want to
> interfere with the pcspkr, so it needs
> some way to disable the thing.

Can't you just use input_grab() for this? SND_SUSPEND really seems
inappropriate, since it's not a sound event.

> Can this please be applied?

Not yet.

> Signed-off-by: stsp@aknet.ru

> diff -urN linux-2.6.12-rc6/drivers/input/misc/pcspkr.c linux-2.6.12-rc6-spinlk/drivers/input/misc/pcspkr.c
> --- linux-2.6.12-rc6/drivers/input/misc/pcspkr.c	2005-06-07 11:11:49.000000000 +0400
> +++ linux-2.6.12-rc6-spinlk/drivers/input/misc/pcspkr.c	2005-06-14 12:57:59.000000000 +0400
> @@ -18,6 +18,7 @@
>  #include <linux/input.h>
>  #include <asm/8253pit.h>
>  #include <asm/io.h>
> +#include <asm/timer.h>
>  
>  MODULE_AUTHOR("Vojtech Pavlik <vojtech@ucw.cz>");
>  MODULE_DESCRIPTION("PC Speaker beeper driver");
> @@ -26,27 +27,13 @@
>  static char pcspkr_name[] = "PC Speaker";
>  static char pcspkr_phys[] = "isa0061/input0";
>  static struct input_dev pcspkr_dev;
> +enum { PCSPKR_NORMAL, PCSPKR_SUSPENDED };
>  
> -static DEFINE_SPINLOCK(i8253_beep_lock);
> -
> -static int pcspkr_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
> +static void pcspkr_do_sound(unsigned int count)
>  {
> -	unsigned int count = 0;
>  	unsigned long flags;
>  
> -	if (type != EV_SND)
> -		return -1;
> -
> -	switch (code) {
> -		case SND_BELL: if (value) value = 1000;
> -		case SND_TONE: break;
> -		default: return -1;
> -	}
> -
> -	if (value > 20 && value < 32767)
> -		count = PIT_TICK_RATE / value;
> -
> -	spin_lock_irqsave(&i8253_beep_lock, flags);
> +	spin_lock_irqsave(&i8253_lock, flags);
>  
>  	if (count) {
>  		/* enable counter 2 */
> @@ -61,7 +48,32 @@
>  		outb(inb_p(0x61) & 0xFC, 0x61);
>  	}
>  
> -	spin_unlock_irqrestore(&i8253_beep_lock, flags);
> +	spin_unlock_irqrestore(&i8253_lock, flags);
> +}
> +
> +static int pcspkr_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
> +{
> +	unsigned int count = 0;
> +
> +	if (type != EV_SND)
> +		return -1;
> +
> +	switch (code) {
> +		case SND_BELL: if (value) value = 1000;
> +		case SND_TONE: break;
> +		case SND_SUSPEND:
> +			dev->state = value ? PCSPKR_SUSPENDED : PCSPKR_NORMAL;
> +			return 0;
> +		default: return -1;
> +	}
> +
> +	if (dev->state == PCSPKR_SUSPENDED)
> +		return 0;
> +
> +	if (value > 20 && value < 32767)
> +		count = PIT_TICK_RATE / value;
> +
> +	pcspkr_do_sound(count);
>  
>  	return 0;
>  }
> @@ -69,7 +81,7 @@
>  static int __init pcspkr_init(void)
>  {
>  	pcspkr_dev.evbit[0] = BIT(EV_SND);
> -	pcspkr_dev.sndbit[0] = BIT(SND_BELL) | BIT(SND_TONE);
> +	pcspkr_dev.sndbit[0] = BIT(SND_BELL) | BIT(SND_TONE) | BIT(SND_SUSPEND);
>  	pcspkr_dev.event = pcspkr_event;
>  
>  	pcspkr_dev.name = pcspkr_name;
> @@ -78,6 +90,7 @@
>  	pcspkr_dev.id.vendor = 0x001f;
>  	pcspkr_dev.id.product = 0x0001;
>  	pcspkr_dev.id.version = 0x0100;
> +	pcspkr_dev.state = PCSPKR_NORMAL;
>  
>  	input_register_device(&pcspkr_dev);
>  
> @@ -90,7 +103,7 @@
>  {
>          input_unregister_device(&pcspkr_dev);
>  	/* turn off the speaker */
> -	pcspkr_event(NULL, EV_SND, SND_BELL, 0);
> +	pcspkr_do_sound(0);
>  }
>  
>  module_init(pcspkr_init);
> diff -urN linux-2.6.12-rc6/include/linux/input.h linux-2.6.12-rc6-spinlk/include/linux/input.h
> --- linux-2.6.12-rc6/include/linux/input.h	2005-06-07 11:12:29.000000000 +0400
> +++ linux-2.6.12-rc6-spinlk/include/linux/input.h	2005-06-14 12:58:41.000000000 +0400
> @@ -593,6 +593,7 @@
>  #define SND_CLICK		0x00
>  #define SND_BELL		0x01
>  #define SND_TONE		0x02
> +#define SND_SUSPEND		0x03
>  #define SND_MAX			0x07
>  
>  /*


-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [patch 2/2] pcspeaker driver update

[Stas Sergeev]

Hello.

Vojtech Pavlik wrote:
>> - changes the pcspeaker driver to
>> use the i8253_lock instead of i8253_beep_lock
> This doesn't seem right. The driver programs an independent part of the
> chip and I don't see a reason to cause the time code to wait because
> we're trying to do a beep.
Yes, you program 0x42 which is independant,
but, unless I am missing something, you also
touch 0x43 and 0x61, which are not, and so I
thought it would be better to just use the
i8253_lock alltogether. And it doesn't look
like the lock is held during the entire beep,
so it probably doesn't really make anything
to wait for too long. What am I missing?

> Can't you just use input_grab() for this?
I am not sure, I thought I can't. I looked at
the code and it seems input_event() would call
the dev->event() regardless, and only at the
bottom - dev->grab->handler->event().
While it seems like I want to prevent the
dev->event() from being called, at the first
place. And it doesn't look like the grab
functionality is described in
Documentation/input at all, and no examples
around the code that I could use. So I just
don't know what functionality is that...
Will try to play around it and maybe I'll
figure something out:)

> SND_SUSPEND really seems
> inappropriate, since it's not a sound event.
Is it just a problem of the name (i.e.
would the SND_STOP be better), or is it
conceptually wrong? (I guess for both:)

>> Can this please be applied?
> Not yet.
OK. I'll try to find the better solution.
Let's just apply the first patch then - it is a
cleanup, I don't think it could do any harm.

Re: [patch 2/2] pcspeaker driver update

[Vojtech Pavlik]

On Tue, Jun 14, 2005 at 11:29:00PM +0400, Stas Sergeev wrote:
> Hello.
> 
> Vojtech Pavlik wrote:
> >>- changes the pcspeaker driver to
> >>use the i8253_lock instead of i8253_beep_lock
> >This doesn't seem right. The driver programs an independent part of the
> >chip and I don't see a reason to cause the time code to wait because
> >we're trying to do a beep.
> Yes, you program 0x42 which is independant,
> but, unless I am missing something, you also
> touch 0x43 and 0x61, which are not, and so I
> thought it would be better to just use the
> i8253_lock alltogether. And it doesn't look
> like the lock is held during the entire beep,
> so it probably doesn't really make anything
> to wait for too long. What am I missing?

You're right, we need to serialize access to 0x43, since it selects
which of the timers will be accessed next. I had to refresh my memory on
how that piece of hardware works.

Regarding 0x61, it's rarely used elsewhere (in MCA case, and for NMI
traps). We may need to modify the other code, but I doubt there is much
worth in it. Anyway, we do a read-modify-write, so locking would be
appropriate.

So I agree with that part of the patch.

> >Can't you just use input_grab() for this?
> I am not sure, I thought I can't. I looked at
> the code and it seems input_event() would call
> the dev->event() regardless, and only at the
> bottom - dev->grab->handler->event().
> While it seems like I want to prevent the
> dev->event() from being called, at the first
> place. And it doesn't look like the grab
> functionality is described in
> Documentation/input at all, and no examples
> around the code that I could use. So I just
> don't know what functionality is that...
> Will try to play around it and maybe I'll
> figure something out:)

Indeed, input_grab() doesn't work on events _to_ the device. And
unfortunately, it probably can't be made to work, since there is no way
to figure out in input_event() which handler it was called from.

It might be desirable to separate the in/out paths in the input layer in
the future.

So indeed, you can't use it.

> >SND_SUSPEND really seems
> >inappropriate, since it's not a sound event.

> Is it just a problem of the name (i.e.
> would the SND_STOP be better), or is it
> conceptually wrong? (I guess for both:)

I don't have a problem with the naming, but indeed the concept. The
SND_* events are for generating sounds, SND_SUSPEND (or _STOP) would be
for enabling/disabling the driver. If anything, it would fall into the
EV_SYN class, but still that doesn't seem like a clean solution.

I don't want to pollute the input API with ad hoc stuff like that.

I think that it'd be best, if you need to share the PC-speaker hardware
with a different driver, to just provide an interface between the two
drivers that doesn't go through the input subsystem.

> >>Can this please be applied?
> >Not yet.

> OK. I'll try to find the better solution.
> Let's just apply the first patch then - it is a
> cleanup, I don't think it could do any harm.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [patch 2/2] pcspeaker driver update

[Stas Sergeev]

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

Hello.

Vojtech Pavlik wrote:
> It might be desirable to separate the in/out paths in the input layer in
> the future.
If it will allow to grab the events to
the device, then I think it would be the
best solution for my problem indeed.

> for enabling/disabling the driver. If anything, it would fall into the
> EV_SYN class, but still that doesn't seem like a clean solution.
> I don't want to pollute the input API with ad hoc stuff like that.
> I think that it'd be best, if you need to share the PC-speaker hardware
> with a different driver, to just provide an interface between the two
> drivers that doesn't go through the input subsystem.
I definitely agree with you that it is
not the clean solution, but the "interface"
approach has the disadvantages too. Besides
the fact that the hooks for the out-of-tree
modules are not accepted to the kernel, there
is also a technical one: if I add that interface,
then loading my module will load the pcspkr
module as a dependancy, enabling those
annoying beeps. Right now I am working
around that by adding an extra parameter
to my module, which, if enabled, will keep
the pcspkr shut up forever, but I'd like to
avoid that. So the only way looks like via
the use of the input API.
And this might be the temporary thing anyway.
As soon as the way to grab an events is
there, the problem is solved in a clean way.
So unless you feel really bad about that, I'd
like something like the attached patch to be
applied. It now uses EV_SYN.

Signed-off-by: stsp@aknet.ru


[-- Attachment #2: pcspkr-2.6.12-rc6-02.diff --]
[-- Type: text/x-patch, Size: 2527 bytes --]

diff -urN linux-2.6.12-rc6/drivers/input/misc/pcspkr.c linux-2.6.12-rc6-spinlk/drivers/input/misc/pcspkr.c
--- linux-2.6.12-rc6/drivers/input/misc/pcspkr.c	2005-06-07 11:11:49.000000000 +0400
+++ linux-2.6.12-rc6-spinlk/drivers/input/misc/pcspkr.c	2005-06-14 12:57:59.000000000 +0400
@@ -26,27 +26,13 @@
 static char pcspkr_name[] = "PC Speaker";
 static char pcspkr_phys[] = "isa0061/input0";
 static struct input_dev pcspkr_dev;
+enum { PCSPKR_NORMAL, PCSPKR_SUSPENDED };
 
-static DEFINE_SPINLOCK(i8253_beep_lock);
-
-static int pcspkr_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
+static void pcspkr_do_sound(unsigned int count)
 {
-	unsigned int count = 0;
 	unsigned long flags;
 
-	if (type != EV_SND)
-		return -1;
-
-	switch (code) {
-		case SND_BELL: if (value) value = 1000;
-		case SND_TONE: break;
-		default: return -1;
-	}
-
-	if (value > 20 && value < 32767)
-		count = PIT_TICK_RATE / value;
-
-	spin_lock_irqsave(&i8253_beep_lock, flags);
+	spin_lock_irqsave(&i8253_lock, flags);
 
 	if (count) {
 		/* enable counter 2 */
@@ -61,14 +47,48 @@
 		outb(inb_p(0x61) & 0xFC, 0x61);
 	}
 
-	spin_unlock_irqrestore(&i8253_beep_lock, flags);
+	spin_unlock_irqrestore(&i8253_lock, flags);
+}
+
+static int pcspkr_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
+{
+	unsigned int count = 0;
+
+	switch (type) {
+		case EV_SND:
+			switch (code) {
+				case SND_BELL: if (value) value = 1000;
+				case SND_TONE: break;
+				default: return -1;
+			}
+			break;
+
+		case EV_SYN:
+			switch (code) {
+				case SYN_CONFIG:
+					dev->state = value ? PCSPKR_SUSPENDED : PCSPKR_NORMAL;
+					return 0;
+				default: return -1;
+			}
+			break;
+
+		default: return -1;
+	}
+
+	if (dev->state == PCSPKR_SUSPENDED)
+		return 0;
+
+	if (value > 20 && value < 32767)
+		count = PIT_TICK_RATE / value;
+
+	pcspkr_do_sound(count);
 
 	return 0;
 }
 
 static int __init pcspkr_init(void)
 {
-	pcspkr_dev.evbit[0] = BIT(EV_SND);
+	pcspkr_dev.evbit[0] = BIT(EV_SND) | BIT(EV_SYN);
 	pcspkr_dev.sndbit[0] = BIT(SND_BELL) | BIT(SND_TONE);
 	pcspkr_dev.event = pcspkr_event;
 
@@ -78,6 +98,7 @@
 	pcspkr_dev.id.vendor = 0x001f;
 	pcspkr_dev.id.product = 0x0001;
 	pcspkr_dev.id.version = 0x0100;
+	pcspkr_dev.state = PCSPKR_NORMAL;
 
 	input_register_device(&pcspkr_dev);
 
@@ -90,7 +111,7 @@
 {
         input_unregister_device(&pcspkr_dev);
 	/* turn off the speaker */
-	pcspkr_event(NULL, EV_SND, SND_BELL, 0);
+	pcspkr_do_sound(0);
 }
 
 module_init(pcspkr_init);