[RFC] make PC Speaker driver work on x86-64

[RFC] make PC Speaker driver work on x86-64

[Mikael Pettersson]

I have a pair of Athlon64 machines that dual-boot 32-bit and
64-bit kernels. One annoying difference between the kernels
is that the PC Speaker driver (CONFIG_INPUT_PCSPKR=y) only
works in the 32-bit kernels. In the 64-bit kernels it remains
inactive and doesn't even generate any boot-time initialisation
or error messages.

Today I debugged that issue, and found that the PC Speaker
driver's ->probe() routine doesn't even get called in the
64-bit kernels. The reason for that is that the arch code
apparently has to explictly add a "pcspkr" platform device
in order for the driver core to call the ->probe() routine.
arch/i386/kernel/setup.c unconditionally adds a "pcspkr"
device, but the x86_64 kernel has no code at all related to
the PC Speaker.

The patch below copies the relevant code from i386 to x86_64,
which makes the PC Speaker work for me on x86_64.

Is there a better way to do this? ACPI?

/Mikael

diff -rupN linux-2.6.17-rc3/arch/x86_64/kernel/setup.c linux-2.6.17-rc3.x86_64-pcspkr/arch/x86_64/kernel/setup.c
--- linux-2.6.17-rc3/arch/x86_64/kernel/setup.c	2006-04-28 20:54:10.000000000 +0200
+++ linux-2.6.17-rc3.x86_64-pcspkr/arch/x86_64/kernel/setup.c	2006-04-29 18:42:08.000000000 +0200
@@ -1426,3 +1426,22 @@ struct seq_operations cpuinfo_op = {
 	.show =	show_cpuinfo,
 };
 
+#ifdef CONFIG_INPUT_PCSPKR
+#include <linux/platform_device.h>
+static __init int add_pcspkr(void)
+{
+	struct platform_device *pd;
+	int ret;
+
+	pd = platform_device_alloc("pcspkr", -1);
+	if (!pd)
+		return -ENOMEM;
+
+	ret = platform_device_add(pd);
+	if (ret)
+		platform_device_put(pd);
+
+	return ret;
+}
+device_initcall(add_pcspkr);
+#endif

Re: [RFC] make PC Speaker driver work on x86-64

[Jeff Garzik]

Mikael Pettersson wrote:
> I have a pair of Athlon64 machines that dual-boot 32-bit and
> 64-bit kernels. One annoying difference between the kernels
> is that the PC Speaker driver (CONFIG_INPUT_PCSPKR=y) only
> works in the 32-bit kernels. In the 64-bit kernels it remains
> inactive and doesn't even generate any boot-time initialisation
> or error messages.
> 
> Today I debugged that issue, and found that the PC Speaker
> driver's ->probe() routine doesn't even get called in the
> 64-bit kernels. The reason for that is that the arch code
> apparently has to explictly add a "pcspkr" platform device
> in order for the driver core to call the ->probe() routine.
> arch/i386/kernel/setup.c unconditionally adds a "pcspkr"
> device, but the x86_64 kernel has no code at all related to
> the PC Speaker.

Wow, thanks.

I was wondering why my PC speaker didn't seem to work.  And here I was, 
blaming Fedora...
	
	Jeff

Re: [RFC] make PC Speaker driver work on x86-64

[Matthieu CASTET]

Hi,

Le Sat, 29 Apr 2006 20:30:10 +0200, Mikael Pettersson a écrit :


> 
> Is there a better way to do this? ACPI?
> 
Yes, I believe using PNP layer (that use ACPI with pnpacpi) with PNP0800
will be cleaner.

Matthieu

Re: [discuss] Re: [RFC] make PC Speaker driver work on x86-64

[Andi Kleen]

On Saturday 29 April 2006 23:00, Matthieu CASTET wrote:
> Hi,
> 
> Le Sat, 29 Apr 2006 20:30:10 +0200, Mikael Pettersson a écrit :
> 
> 
> > 
> > Is there a better way to do this? ACPI?
> > 
> Yes, I believe using PNP layer (that use ACPI with pnpacpi) with PNP0800
> will be cleaner.

Please do a patch then.

-Andi

Re: [discuss] Re: [RFC] make PC Speaker driver work on x86-64

[matthieu castet]

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

Hi,

Andi Kleen wrote:
> On Saturday 29 April 2006 23:00, Matthieu CASTET wrote:
> 
>>Hi,
>>
>>Le Sat, 29 Apr 2006 20:30:10 +0200, Mikael Pettersson a écrit :
>>
>>
>>
>>>Is there a better way to do this? ACPI?
>>>
>>
>>Yes, I believe using PNP layer (that use ACPI with pnpacpi) with PNP0800
>>will be cleaner.
> 
> 
> Please do a patch then.
> 
Is something like that is acceptable ?

Matthieu

Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>

[-- Attachment #2: pcspkr_pnp.diff --]
[-- Type: text/plain, Size: 4712 bytes --]

--- linux.old/drivers/input/misc/pcspkr.c	2006-04-30 18:16:09.000000000 +0200
+++ linux/drivers/input/misc/pcspkr.c	2006-05-02 19:35:09.000000000 +0200
@@ -17,6 +17,7 @@
 #include <linux/init.h>
 #include <linux/input.h>
 #include <linux/platform_device.h>
+#include <linux/pnp.h>
 #include <asm/8253pit.h>
 #include <asm/io.h>
 
@@ -26,6 +27,7 @@
 
 static struct platform_device *pcspkr_platform_device;
 static DEFINE_SPINLOCK(i8253_beep_lock);
+static int pnp_registered;
 
 static int pcspkr_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
 {
@@ -64,7 +66,7 @@
 	return 0;
 }
 
-static int __devinit pcspkr_probe(struct platform_device *dev)
+static int __devinit pcspkr_probe(struct device *dev, struct input_dev **pcspkr_dev_pointer)
 {
 	struct input_dev *pcspkr_dev;
 	int err;
@@ -79,7 +81,7 @@
 	pcspkr_dev->id.vendor = 0x001f;
 	pcspkr_dev->id.product = 0x0001;
 	pcspkr_dev->id.version = 0x0100;
-	pcspkr_dev->cdev.dev = &dev->dev;
+	pcspkr_dev->cdev.dev = dev;
 
 	pcspkr_dev->evbit[0] = BIT(EV_SND);
 	pcspkr_dev->sndbit[0] = BIT(SND_BELL) | BIT(SND_TONE);
@@ -90,32 +92,48 @@
 		input_free_device(pcspkr_dev);
 		return err;
 	}
+	*pcspkr_dev_pointer = pcspkr_dev;
 
-	platform_set_drvdata(dev, pcspkr_dev);
+	return 0;
+}
+
+static int __devexit pcspkr_remove(struct input_dev *pcspkr_dev)
+{
+	input_unregister_device(pcspkr_dev);
+	/* turn off the speaker */
+	pcspkr_event(NULL, EV_SND, SND_BELL, 0);
 
 	return 0;
 }
 
-static int __devexit pcspkr_remove(struct platform_device *dev)
+static int __devinit pcspkr_probe_platform(struct platform_device *dev)
+{
+	struct input_dev *pcspkr_dev;
+	int err;
+	err = pcspkr_probe(&dev->dev, &pcspkr_dev);
+	if (err == 0)
+		platform_set_drvdata(dev, pcspkr_dev);
+	return err;
+}
+
+static int __devexit pcspkr_remove_platform(struct platform_device *dev)
 {
 	struct input_dev *pcspkr_dev = platform_get_drvdata(dev);
 
-	input_unregister_device(pcspkr_dev);
+	pcspkr_remove(pcspkr_dev);
 	platform_set_drvdata(dev, NULL);
-	/* turn off the speaker */
-	pcspkr_event(NULL, EV_SND, SND_BELL, 0);
 
 	return 0;
 }
 
-static int pcspkr_suspend(struct platform_device *dev, pm_message_t state)
+static int pcspkr_suspend_platform(struct platform_device *dev, pm_message_t state)
 {
 	pcspkr_event(NULL, EV_SND, SND_BELL, 0);
 
 	return 0;
 }
 
-static void pcspkr_shutdown(struct platform_device *dev)
+static void pcspkr_shutdown_platform(struct platform_device *dev)
 {
 	/* turn off the speaker */
 	pcspkr_event(NULL, EV_SND, SND_BELL, 0);
@@ -126,17 +144,80 @@
 		.name	= "pcspkr",
 		.owner	= THIS_MODULE,
 	},
-	.probe		= pcspkr_probe,
-	.remove		= __devexit_p(pcspkr_remove),
-	.suspend	= pcspkr_suspend,
-	.shutdown	= pcspkr_shutdown,
+	.probe		= pcspkr_probe_platform,
+	.remove		= __devexit_p(pcspkr_remove_platform),
+	.suspend	= pcspkr_suspend_platform,
+	.shutdown	= pcspkr_shutdown_platform,
+};
+
+#ifdef CONFIG_PNP
+static struct pnp_device_id pcspkr_pnpids[] = {
+	{ .id = "PNP0800" },
+	{ .id = "" }
+};
+
+MODULE_DEVICE_TABLE(pnp, pcspkr_pnpids);
+
+static int __devinit pcspkr_pnp_probe(struct pnp_dev *pnp_dev,
+					  const struct pnp_device_id *id)
+{
+	struct input_dev *pcspkr_dev;
+	int err;
+	err = pcspkr_probe(&pnp_dev->dev, &pcspkr_dev);
+	if (err == 0) {
+		pnp_set_drvdata(pnp_dev, pcspkr_dev);
+		pnp_registered++;
+	}
+	return err;
+}
+
+static void __devexit pcspkr_pnp_remove(struct pnp_dev *dev)
+{
+	struct input_dev *pcspkr_dev = (struct input_dev *)pnp_get_drvdata(dev);
+
+	pcspkr_remove(pcspkr_dev);
+	pnp_set_drvdata(dev, NULL);
+
+	return;
+}
+
+#ifdef CONFIG_PM
+static int pcspkr_suspend_pnp(struct pnp_dev *pdev, pm_message_t state)
+{
+	pcspkr_event(NULL, EV_SND, SND_BELL, 0);
+
+	return 0;
+}
+#endif
+
+static struct pnp_driver pcspkr_pnp_driver = {
+	.name = "pcspkr",
+	.id_table = pcspkr_pnpids,
+	.probe = pcspkr_pnp_probe,
+	.remove = __devexit_p(pcspkr_pnp_remove),
+#ifdef CONFIG_PM
+	.suspend = pcspkr_suspend_pnp,
+#endif
 };
+#else
+static struct pnp_driver pcspkr_pnp_driver;
+#endif
 
 
 static int __init pcspkr_init(void)
 {
 	int err;
 
+	err = pnp_register_driver(&pcspkr_pnp_driver);
+	if (err < 0)
+		pnp_registered = 0;
+	if (err == 0 && pnp_registered == 0)
+		pnp_unregister_driver(&pcspkr_pnp_driver);
+
+	if (pnp_registered)
+		return 0;
+
+	/* Try platform driver if pnp failed or found nothing */
 	err = platform_driver_register(&pcspkr_platform_driver);
 	if (err)
 		return err;
@@ -163,6 +244,10 @@
 
 static void __exit pcspkr_exit(void)
 {
+	if (pnp_registered) {
+		pnp_unregister_driver(&pcspkr_pnp_driver);
+		return;
+	}
 	platform_device_unregister(pcspkr_platform_device);
 	platform_driver_unregister(&pcspkr_platform_driver);
 }

Re: [discuss] Re: [RFC] make PC Speaker driver work on x86-64

[Andi Kleen]

On Tuesday 02 May 2006 19:37, matthieu castet wrote:
> Hi,
> 
> Andi Kleen wrote:
> > On Saturday 29 April 2006 23:00, Matthieu CASTET wrote:
> > 
> >>Hi,
> >>
> >>Le Sat, 29 Apr 2006 20:30:10 +0200, Mikael Pettersson a écrit :
> >>
> >>
> >>
> >>>Is there a better way to do this? ACPI?
> >>>
> >>
> >>Yes, I believe using PNP layer (that use ACPI with pnpacpi) with PNP0800
> >>will be cleaner.
> > 
> > 
> > Please do a patch then.
> > 
> Is something like that is acceptable ?

Does it work? 

Also I have no idea if all x86 systems report the PC speaker in ACPI - a small
survey of that would be probably a good idea. I guess just most of them reporting it would be 
reasonable.

-Andi

Re: [discuss] Re: [RFC] make PC Speaker driver work on x86-64

[matthieu castet]

Andi Kleen wrote:

> Does it work? 
> 
of course (at least on x86) !

> Also I have no idea if all x86 systems report the PC speaker in ACPI - a small
> survey of that would be probably a good idea. I guess just most of them reporting it would be 
> reasonable.
That's why I keep the platform driver :
the logic is try with ACPI in order to discover if there is a speaker. 
If we failed, let's try the platform driver.

Matthieu

PS : even system without ACPI should report the speaker with pnpbios.

Re: [discuss] Re: [RFC] make PC Speaker driver work on x86-64

[Andi Kleen]

On Tuesday 02 May 2006 22:33, matthieu castet wrote:
> Andi Kleen wrote:
> 
> > Does it work? 
> > 
> of course (at least on x86) !
> 
> > Also I have no idea if all x86 systems report the PC speaker in ACPI - a small
> > survey of that would be probably a good idea. I guess just most of them reporting it would be 
> > reasonable.
> That's why I keep the platform driver :
> the logic is try with ACPI in order to discover if there is a speaker. 
> If we failed, let's try the platform driver.

Ok - you mean just keeping the old code for that as fallback That makes sense.
 
> Matthieu
> 
> PS : even system without ACPI should report the speaker with pnpbios.

PNP BIOS doesn't work on 64bit.

-Andi

Re: [discuss] Re: [RFC] make PC Speaker driver work on x86-64

[Andi Kleen]

On Tuesday 02 May 2006 22:33, matthieu castet wrote:

> > Also I have no idea if all x86 systems report the PC speaker in ACPI - a small
> > survey of that would be probably a good idea. I guess just most of them reporting it would be 
> > reasonable.
> That's why I keep the platform driver :
> the logic is try with ACPI in order to discover if there is a speaker. 
> If we failed, let's try the platform driver.

The patch doesn't apply to 2.6.17rc3-git2. For what tree did you do it?

Also can you please include a full changelog in the patch?

Thanks,
-Andi

Re: [discuss] [RFC] make PC Speaker driver work on x86-64

[Andi Kleen]

On Saturday 29 April 2006 20:30, Mikael Pettersson wrote:
> I have a pair of Athlon64 machines that dual-boot 32-bit and
> 64-bit kernels. One annoying difference between the kernels
> is that the PC Speaker driver (CONFIG_INPUT_PCSPKR=y) only
> works in the 32-bit kernels. 

Ah, I would consider this more a feature than a bug but ok :)

> In the 64-bit kernels it remains 
> inactive and doesn't even generate any boot-time initialisation
> or error messages.
> 
> Today I debugged that issue, and found that the PC Speaker
> driver's ->probe() routine doesn't even get called in the
> 64-bit kernels. The reason for that is that the arch code
> apparently has to explictly add a "pcspkr" platform device
> in order for the driver core to call the ->probe() routine.
> arch/i386/kernel/setup.c unconditionally adds a "pcspkr"
> device, but the x86_64 kernel has no code at all related to
> the PC Speaker.
> 
> The patch below copies the relevant code from i386 to x86_64,
> which makes the PC Speaker work for me on x86_64.

Ok thanks. Applied.

> Is there a better way to do this? ACPI?

Maybe. ACPI folks, any opinion? 

-Andi (known to rip out the speaker cables in new machines) 

> 
> /Mikael
> 
> diff -rupN linux-2.6.17-rc3/arch/x86_64/kernel/setup.c linux-2.6.17-rc3.x86_64-pcspkr/arch/x86_64/kernel/setup.c
> --- linux-2.6.17-rc3/arch/x86_64/kernel/setup.c	2006-04-28 20:54:10.000000000 +0200
> +++ linux-2.6.17-rc3.x86_64-pcspkr/arch/x86_64/kernel/setup.c	2006-04-29 18:42:08.000000000 +0200
> @@ -1426,3 +1426,22 @@ struct seq_operations cpuinfo_op = {
>  	.show =	show_cpuinfo,
>  };
>  
> +#ifdef CONFIG_INPUT_PCSPKR
> +#include <linux/platform_device.h>
> +static __init int add_pcspkr(void)
> +{
> +	struct platform_device *pd;
> +	int ret;
> +
> +	pd = platform_device_alloc("pcspkr", -1);
> +	if (!pd)
> +		return -ENOMEM;
> +
> +	ret = platform_device_add(pd);
> +	if (ret)
> +		platform_device_put(pd);
> +
> +	return ret;
> +}
> +device_initcall(add_pcspkr);
> +#endif
> 

Re: [discuss] [RFC] make PC Speaker driver work on x86-64

[Goswin von Brederlow]

Andi Kleen <ak@suse.de> writes:

> On Saturday 29 April 2006 20:30, Mikael Pettersson wrote:
>> I have a pair of Athlon64 machines that dual-boot 32-bit and
>> 64-bit kernels. One annoying difference between the kernels
>> is that the PC Speaker driver (CONFIG_INPUT_PCSPKR=y) only
>> works in the 32-bit kernels. 
>
> Ah, I would consider this more a feature than a bug but ok :)
>
>> In the 64-bit kernels it remains 
>> inactive and doesn't even generate any boot-time initialisation
>> or error messages.

That means that the system wouldn't beep on the console or when you
call "beep", right?

With 2.6.8 x86_64 that worked without problems. Since I updated to
2.6.15 the system is silent.

Could it be that this is a recent problem?

MfG
        Goswin

Re: [discuss] [RFC] make PC Speaker driver work on x86-64

[Alistair John Strachan]

On Sunday 30 April 2006 15:32, Goswin von Brederlow wrote:
> Andi Kleen <ak@suse.de> writes:
> > On Saturday 29 April 2006 20:30, Mikael Pettersson wrote:
> >> I have a pair of Athlon64 machines that dual-boot 32-bit and
> >> 64-bit kernels. One annoying difference between the kernels
> >> is that the PC Speaker driver (CONFIG_INPUT_PCSPKR=y) only
> >> works in the 32-bit kernels.
> >
> > Ah, I would consider this more a feature than a bug but ok :)
> >
> >> In the 64-bit kernels it remains
> >> inactive and doesn't even generate any boot-time initialisation
> >> or error messages.
>
> That means that the system wouldn't beep on the console or when you
> call "beep", right?
>
> With 2.6.8 x86_64 that worked without problems. Since I updated to
> 2.6.15 the system is silent.
>
> Could it be that this is a recent problem?

I think it's as described; the pcspkr driver changed in design around this 
time and needs to be plugged, now. It's probably exactly the same issue.

-- 
Cheers,
Alistair.

Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: [discuss] [RFC] make PC Speaker driver work on x86-64

[Jan Engelhardt]

>> Is there a better way to do this? ACPI?
>
>Maybe. ACPI folks, any opinion? 
>
>-Andi (known to rip out the speaker cables in new machines) 
>

Leave the cables, try this for ttyX:

diff --fast -Ndpru linux-2.6.16-rc1-git3-SUSE20060124/drivers/char/vt.c linux-2.6-AS24/drivers/char/vt.c
--- linux-2.6.16-rc1-git3-SUSE20060124/drivers/char/vt.c	2006-01-28 19:01:17.000000000 +0100
+++ linux-2.6-AS24/drivers/char/vt.c	2006-01-28 19:01:18.860985000 +0100
@@ -115,7 +115,7 @@ const struct consw *conswitchp;
  * Here is the default bell parameters: 750HZ, 1/8th of a second
  */
 #define DEFAULT_BELL_PITCH	750
-#define DEFAULT_BELL_DURATION	(HZ/8)
+#define DEFAULT_BELL_DURATION	0
 
 extern void vcs_make_devfs(struct tty_struct *tty);
 extern void vcs_remove_devfs(struct tty_struct *tty);
#<<eof>>

X seems silent by default for me, if not, you can configure it too.
But ttyX always get reset to HZ/8 when you make a tty reset, which is why I 
put 0 as a default.



Jan
--