[alsa-devel] [PATCH v3] ASoC CS4270 codec device driver

[Takashi Iwai]

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:
> > >>
> > >> 1) Stand-alone and software mode
> > >> 2) Software mode via I2C only
> > >> 3) Master mode, not Slave
> > >> 4) No power management
> > >>
> > >> Signed-off-by: Timur Tabi <timur at 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