Skip to contentSkip to conversation

pcspkr: fix dependancies

PATCH Superseded Stas Sergeev 5 participants 12 messages Apr 26, 2008 – May 1, 2008

Subsystems (3)
x86 Sound Input drivers
lore
Share & tools
lore

Stas Sergeev

author
#0 lore raw

PATCH 1/1 pcspkr: fix dependancies

Hello.

linux-2.6.25 has the strange dependancies for pc-speaker, which prevents snd-pcsp from working.
Namely, the platform device (created in arch/x86/kernel/pcspeaker.c) depends on the platform driver (CONFIG_INPUT_PCSPKR). I wonder if it is a good practice to make the platform device to depend on the driver, I guess it is not.

The attached patch reverses that.
It adds the config option for the pcspkr platform device and makes the platform drivers to depend on that.
This allows snd-pcsp to work at least in some configurations.

Does that patch look reasonable?

---
fix pcspkr dependancies: make the pcspkr platform drivers to depend on a platform device, and not the other way around.

Signed-off-by Stas Sergeev <[email protected]>
CC Takashi Iwai <[email protected]>
CC Dmitry Torokhov <[email protected]>
CC Vojtech Pavlik <[email protected]>
init/Kconfig +8 -0
@@ -624,6 +624,14 @@
 	help
 	  Enable support for generating core dumps. Disabling saves about 4k.
 
+config PCSPKR_PLATFORM
+	bool "Enable PC-Speaker support" if EMBEDDED
+	depends on ALPHA || X86 || MIPS || PPC_PREP || PPC_CHRP || PPC_PSERIES
+	default y
+	help
+          This option allows to disable the internal PC-Speaker
+          support, saving some memory.
+
 config COMPAT_BRK
 	bool "Disable heap randomization"
 	default y
arch/x86/kernel/Makefile +1 -1
@@ -83,7 +83,7 @@
 obj-$(CONFIG_VMI)		+= vmi_32.o vmiclock_32.o
 obj-$(CONFIG_PARAVIRT)		+= paravirt.o paravirt_patch_$(BITS).o
 
-ifdef CONFIG_INPUT_PCSPKR
+ifdef CONFIG_PCSPKR_PLATFORM
 obj-y				+= pcspeaker.o
 endif
sound/drivers/Kconfig +2 -2
@@ -5,8 +5,8 @@
 
 
 config SND_PCSP
-	tristate "Internal PC speaker support"
-	depends on X86_PC && HIGH_RES_TIMERS
+	tristate "PC-Speaker support"
+	depends on PCSPKR_PLATFORM && X86_PC && HIGH_RES_TIMERS
 	help
 	  If you don't have a sound card in your computer, you can include a
 	  driver for the PC speaker which allows it to act like a primitive
drivers/input/misc/Kconfig +1 -1
@@ -14,7 +14,7 @@
 
 config INPUT_PCSPKR
 	tristate "PC Speaker support"
-	depends on ALPHA || X86 || MIPS || PPC_PREP || PPC_CHRP || PPC_PSERIES
+	depends on PCSPKR_PLATFORM
 	help
 	  Say Y here if you want the standard PC Speaker to be used for
 	  bells and whistles.
#1 lore raw

Hi Stas,

Quoted 12 lines · Stas Sergeev

Hello.

linux-2.6.25 has the strange dependancies for pc-speaker, which prevents snd-pcsp from working.
Namely, the platform device (created in arch/x86/kernel/pcspeaker.c) depends on the platform driver (CONFIG_INPUT_PCSPKR). I wonder if it is a good practice to make the platform device to depend on the driver, I guess it is not.

Quoted 7 lines

The attached patch reverses that.
It adds the config option for the pcspkr platform device and makes the platform drivers to depend on that.
This allows snd-pcsp to work at least in some configurations.

No, not another config option please. Either make pcspkr platform device be created unconditionally or if either pcspkr or snd-pcsp is selected.

Signature · 1 line
Dmitry
author
#2 lore raw

Hello.

Quoted 2 lines · Dmitry Torokhov

No, not another config option please. Either make pcspkr platform device be created unconditionally

I also think this would be the best
fix. But this will return us to the state of 2.6.24. Presumably the dependancy was added for the reason, even if I
fail to see one.
But in any case, the patch is attached.

Quoted 2 lines

or if either pcspkr or snd-pcsp
is selected.

I don't think it is correct, but if people think it is, then this can be done.

---
Compile pcspkr platform device unconditionally. That allows snd-pcsp to work.

Signed-off-by Stas Sergeev <[email protected]>
arch/x86/kernel/Makefile +0 -2
@@ -83,9 +83,7 @@
 obj-$(CONFIG_VMI)		+= vmi_32.o vmiclock_32.o
 obj-$(CONFIG_PARAVIRT)		+= paravirt.o paravirt_patch_$(BITS).o
 
-ifdef CONFIG_INPUT_PCSPKR
 obj-y				+= pcspeaker.o
-endif
 
 obj-$(CONFIG_SCx200)		+= scx200.o
 scx200-y			+= scx200_32.o
maintainer
#3 lore raw
Quoted 12 lines · Stas Sergeev

Hello.

Quoted 2 lines · Dmitry Torokhov

No, not another config option please. Either make pcspkr platform device be created unconditionally

I also think this would be the best
fix. But this will return us to the state of 2.6.24. Presumably the dependancy was added for the reason, even if I
fail to see one.
But in any case, the patch is attached.

Quoted 6 lines
Quoted 2 lines

or if either pcspkr or snd-pcsp
is selected.

I don't think it is correct, but if people think it is, then this can be done.

It looks like the change was introduced by commit

67926892ef7a7fbc76de607120d44416019fdf07

I think that we should register devices even if there is no driver in the kernel for it because driver may be compiled at later time or be out of tree. Adding patch author and other people that signed off the patch to CC...

Quoted 19 lines

---
Compile pcspkr platform device unconditionally. That allows snd-pcsp to work.

Signed-off-by Stas Sergeev <[email protected]>
arch/x86/kernel/Makefile +0 -2
@@ -83,9 +83,7 @@
 obj-$(CONFIG_VMI)		+= vmi_32.o vmiclock_32.o
 obj-$(CONFIG_PARAVIRT)		+= paravirt.o paravirt_patch_$(BITS).o
 
-ifdef CONFIG_INPUT_PCSPKR
 obj-y				+= pcspeaker.o
-endif
 
 obj-$(CONFIG_SCx200)		+= scx200.o
 scx200-y			+= scx200_32.o
Signature · 1 line
Dmitry
author
#4 lore raw

Hello.

Quoted 2 lines · Dmitry Torokhov

It looks like the change was introduced by commit
67926892ef7a7fbc76de607120d44416019fdf07

Indeed. But the log message
http://www.mail-archive.com/[email protected]/msg36026.html
doesn't seem to specify the reason for the change, so this doesn't clarify a thing, unfortunately.

Quoted 4 lines

I think that we should register devices even if there is no driver in the kernel for it because driver may be compiled at later time or be out of tree. Adding patch author and other people that signed off the patch to CC...

Since there seem to be no objections, would you mind adding the patch to your git tree? Or who should that be?

maintainer
#5 lore raw
Quoted 11 lines · Stas Sergeev

Hello.

Quoted 2 lines · Dmitry Torokhov

It looks like the change was introduced by commit
67926892ef7a7fbc76de607120d44416019fdf07

Indeed. But the log message
http://www.mail-archive.com/[email protected]/msg36026.html
doesn't seem to specify the reason for the change, so this doesn't clarify a thing, unfortunately.

Quoted 7 lines
Quoted 4 lines

I think that we should register devices even if there is no driver in the kernel for it because driver may be compiled at later time or be out of tree. Adding patch author and other people that signed off the patch to CC...

Since there seem to be no objections, would you mind adding the patch to your git tree? Or who should that be?

I try to stay within drivers/input boundaries ;) Ingo I think is the person you need.

Signature · 1 line
Dmitry
#6 lore raw
Quoted 16 lines · Dmitry Torokhov
Quoted 15 lines · Stas Sergeev

Hello.

Quoted 2 lines · Dmitry Torokhov

It looks like the change was introduced by commit
67926892ef7a7fbc76de607120d44416019fdf07

Indeed. But the log message
http://www.mail-archive.com/[email protected]/msg36026.html
doesn't seem to specify the reason for the change, so this doesn't clarify a thing, unfortunately.

Quoted 4 lines

I think that we should register devices even if there is no driver in the kernel for it because driver may be compiled at later time or be out of tree. Adding patch author and other people that signed off the patch to CC...

Quoted 6 lines
Quoted 3 lines

Since there seem to be no objections, would you mind adding the patch to your git tree? Or who should that be?

I try to stay within drivers/input boundaries ;) Ingo I think is the person you need.

If you google you will find a long thread about this patch. It is all about saving memory for embedded platforms. No need to have the driver if there is no speaker on the board.

Ask Michael (author) if you need more info.

Sam

maintainer
#7 lore raw
Quoted 21 lines · Sam Ravnborg
Quoted 20 lines · Dmitry Torokhov
Quoted 18 lines · Stas Sergeev

Hello.

Quoted 2 lines · Dmitry Torokhov

It looks like the change was introduced by commit
67926892ef7a7fbc76de607120d44416019fdf07

Indeed. But the log message
http://www.mail-archive.com/[email protected]/msg36026.html
doesn't seem to specify the reason for the change, so this doesn't clarify a thing, unfortunately.

Quoted 4 lines

I think that we should register devices even if there is no driver in the kernel for it because driver may be compiled at later time or be out of tree. Adding patch author and other people that signed off the patch to CC...

Since there seem to be no objections, would you mind adding the patch to your git tree? Or who should that be?

Quoted 6 lines
Quoted 2 lines

I try to stay within drivers/input boundaries ;) Ingo I think is the person you need.

If you google you will find a long thread about this patch. It is all about saving memory for embedded platforms. No need to have the driver if there is no speaker on the board.

The patch does it backwards though - it disables platform device if there is no driver. We don't stop enumerating PCI devices if some of them don't have a driver for them, do we? The arch code should not create the device if it knows that the boards does not have it.

Signature · 1 line
Dmitry
author
#8 lore raw
In reply to Sam Ravnborg

Hello.

Quoted 1 line · Sam Ravnborg

If you google you will find a long thread about this patch.

Yes, indeed, thankyou.
http://kerneltrap.org/mailarchive/linux-kernel/2008/1/17/577151

Quoted 1 line

It is all about saving memory for embedded platforms.

OK. This wasn't clear from the log message for some reason, but it is said in the above thread.

Quoted 1 line

No need to have the driver if there is no speaker on the board.

Very probably, and for that case I made the patch like this:
http://uwsg.ucs.indiana.edu/hypermail/linux/kernel/0804.3/1189.html

Quoted 1 line

Ask Michael (author) if you need more info.

Reading the thread, it actually seems like Michael wanted to submit the patch much like the aforementioned one of mine:
http://kerneltrap.org/mailarchive/linux-kernel/2008/1/18/580583
but haven't done so in time, and as the result, the wrong one stuck in. But this is never too late to correct, I hope.

So I see 2 options: either revert the patch completely, or revert the dependancies like Michael suggested initially (if I understand his suggestion right). Both patches are here, in this thread. Thoughts?

#9 lore raw
Quoted 10 lines · Stas Sergeev

Reading the thread, it actually seems like Michael wanted to submit the patch much like the aforementioned one of mine:
http://kerneltrap.org/mailarchive/linux-kernel/2008/1/18/580583
but haven't done so in time, and as the result, the wrong one stuck in.
But this is never too late to correct, I hope.

Quoted 8 lines

So I see 2 options: either revert the patch completely, or revert the dependancies like Michael suggested initially (if I understand his suggestion right). Both patches are here, in this thread. Thoughts?

Stas, Dmitry,

I agree with you... on a regular system, all the platform devices should be enumerated, even if we don't use their drivers. It's only for use in embedded devices (CONFIG_EMBEDDED) that we could omit this enumeration to reduce kernel size.

Would you post a patch doing this?

Thank you,

:-)

Michael.

Signature · 4 lines
Michael Opdenacker, Free Electrons
Free Embedded Linux Training Materials
on http://free-electrons.com/training
(More than 1500 pages!)
author
#10 lore raw

Hello.

Quoted 5 lines · Michael Opdenacker

I agree with you... on a regular system, all the platform devices should be enumerated, even if we don't use their drivers. It's only for use in embedded devices (CONFIG_EMBEDDED) that we could omit this enumeration to reduce kernel size.
Would you post a patch doing this?

Certainly, this was done.
http://uwsg.ucs.indiana.edu/hypermail/linux/kernel/0804.3/1189.html
Does this help?

author
#11 lore raw
In reply to Michael Opdenacker

Guys, there really needs to be a few more replies to make a decision.

So far only Dmitry have clearly stated his opinion regarding both patches. And that opinion was: no need for the new config option, just get the 2.6.25 patch reverted.
Dmitry, after reading the older Michael's thread, would you perhaps reconsider?

Sam and Michael said that just revering may not be a good idea, but have not commented on the patch that adds a new option under "if EMBEDDED".

That puts me in a position where both patches are effectively blocked, and in a mean time snd-pcsp remains broken.

Guys, please, give your ACK or NACK on the two posted patches! :) Namely:
http://lkml.org/lkml/2008/4/26/105
http://lkml.org/lkml/2008/4/26/283