pcspeaker driver update

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 [email protected]

diff -urN linux-2.6.12-rc6/drivers/input/misc/pcspkr.c linux-2.6.12-rc6-spinlk/drivers/input/misc/pcspkr.c

drivers/input/misc/pcspkr.c +33 -20

@@ -18,6 +18,7 @@
 #include <linux/input.h>
 #include <asm/8253pit.h>
 #include <asm/io.h>
+#include <asm/timer.h>
 
 MODULE_AUTHOR("Vojtech Pavlik <[email protected]>");
 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

include/linux/input.h +1 -0

@@ -593,6 +593,7 @@
 #define SND_CLICK		0x00
 #define SND_BELL		0x01
 #define SND_TONE		0x02
+#define SND_SUSPEND		0x03
 #define SND_MAX			0x07
 
 /*

Quoted 2 lines

- 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.

Can't you just use input_grab() for this?

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

Quoted 1 line

Can this please be applied?

Not yet.

Hello.

Quoted 5 lines · Vojtech Pavlik

Quoted 2 lines

- 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?

Quoted 1 line

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:)

Quoted 2 lines

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:)

Quoted 2 lines

Quoted 1 line

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.

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

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.