At Tue, 31 Jul 2007 18:17:33 +0200, I wrote:
At Tue, 31 Jul 2007 11:12:44 -0500, Timur Tabi wrote:
Takashi Iwai wrote:
At Tue, 31 Jul 2007 10:46:52 -0500, Timur Tabi wrote:
This patch adds ALSA SoC support for the Cirrus Logic CS4270 codec. The following features are suppored:
- Stand-alone and software mode
- Software mode via I2C only
- Master mode, not Slave
- No power management
Signed-off-by: Timur Tabi timur@freescale.com
Thanks, it can be now applied fine.
The only thing I noticed is below (oh I should have mentioned it before...):
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index e5fb437..7824880 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -17,3 +17,23 @@ config SND_SOC_WM8753 config SND_SOC_WM9712 tristate depends on SND_SOC
+# Cirrus Logic CS4270 Codec +config SND_SOC_CS4270
- tristate
- depends on SND_SOC
+# Cirrus Logic CS4270 Codec Hardware Mute Support +# Select if you have external muting circuitry attached to your CS4270. +config SND_SOC_CS4270_HWMUTE
- bool
- depends on SND_SOC_CS4270
+# Cirrus Logic CS4270 Codec VD = 3.3V Errata +# Select if you are affected by the errata where the part will not function +# if MCLK divide-by-1.5 is selected and VD is set to 3.3V. The driver will +# not select any sample rates that require MCLK to be divided by 1.5. +config SND_SOC_CS4270_VD33_ERRATA
- bool
- depends on SND_SOC_CS4270
I'd suggest to convert these comments to help texts. Also, usually the items to be chosen via menuconfig have tristate "XXXX" or bool "XXX"
Could you fix them, then I'll finally merge it to upstream?
I am following the model of the other Kconfig options. If I add the "XXXX" and the help text, then they become selectable from "make menuconfig", but they would be the only ones to show up like that.
I think the reason why they are like this is because they're supposed to be selected from the machine driver that uses them, like this:
Doh, I see, these are codecs only for reverse selections. Sorry for confusion. Then I'll merge it as it is.
OK, now found another problem. The driver seems not built without CONFIG_I2C.
In file included from sound/soc/codecs/cs4270.c:1: sound/soc/codecs/cs4270.c:694: error: ‘cs4270_set_dai_sysclk’ undeclared here (not in a function) sound/soc/codecs/cs4270.c:695: error: ‘cs4270_set_dai_fmt’ undeclared here (not in a function)
Shouldn't the driver be dependent on I2C?
Also,
#ifdef CONFIG_I2C
isn't enough. It should be
#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
since I2C can be a module.
I already merged V3 to HG tree, so please post the additional patch against it for fixing these issues.
thanks,
Takashi