On Mon, 10 Nov 2008 13:34:52 +0000
Mark Brown <broonie@xxxxxxxxxxxxx> wrote:
> On Sat, Nov 08, 2008 at 08:43:55AM +0100, Christian Pellegrin wrote:
>
> > This patch adds support for the UDA1341 codec and a sound card
> > definition for one of these UDAs connected to the s3c24xx. It is
>
> First up, thanks for doing this work - it'd be good to see this merged
> into ASoC.
>
I'm equally interested in this patch, seeing as I
got an hp jornada 7xx with uda1344 waiting.
Have you any plans on creating an 134x seeing as there
is very little difference between chipsets?
> > *heavily* based on the one made by Zoltan Devai but:
>
> Ideally this should be submitted as multiple patches - at least one for
> the codec and one for the board, probably also one for the l3 interface.
>
> > Generated on 20081108 against v2.6.27
>
> Please generate patches against current git - the topic/asoc branch of:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
>
> In general, once the merge window is past it's best to submit patches
> against at least -rc1, submitting against the previous release makes it
> much more likely that your patch will be out of date as has happened
> here.
>
> > +++ b/sound/soc/codecs/Kconfig
> > @@ -50,3 +50,6 @@ config SND_SOC_CS4270_VD33_ERRATA
> > config SND_SOC_TLV320AIC3X
> > tristate
> > depends on I2C
> > +
> > +config SND_SOC_UDA1341
> > + tristate
>
> You should also add this to SND_SOC_ALL_CODECS. There's a bunch of
> other things like this that have changed between 2.6.27 and the current
> code - for example, the bias level configuration.
>
> > diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> > index d7b97ab..cbace60 100644
> > --- a/sound/soc/codecs/Makefile
> > +++ b/sound/soc/codecs/Makefile
> > @@ -23,3 +23,4 @@ obj-$(CONFIG_SND_SOC_WM9712) += snd-soc-wm9712.o
> > obj-$(CONFIG_SND_SOC_WM9713) += snd-soc-wm9713.o
> > obj-$(CONFIG_SND_SOC_CS4270) += snd-soc-cs4270.o
> > obj-$(CONFIG_SND_SOC_TLV320AIC3X) += snd-soc-tlv320aic3x.o
> > +obj-$(CONFIG_SND_SOC_UDA1341) += uda1341/
>
> There doesn't seem to be much benefit to adding a subdirectory for two
> source files here.
>
> > --- /dev/null
> > +++ b/sound/soc/codecs/uda1341/l3.c
>
> > +/*#define L3_DEBUG 1*/
> > +#ifdef L3_DEBUG
> > +#define DBG(format, arg...) \
> > + printk(KERN_DEBUG "L3: %s:" format "\n" , __func__, ## arg)
> > +#else
> > +#define DBG(format, arg...) do {} while (0)
> > +#endif
>
> Please use the standard pr_debug() macro rather than defining custom
> ones.
>
> > +#define setdat(adap, val) (adap->setdat(adap, val))
> > +#define setclk(adap, val) (adap->setclk(adap, val))
> > +#define setmode(adap, val) (adap->setmode(adap, val))
>
> These should be inline functions for type safety.
>
> > +/*#define UDA1341_DEBUG 1*/
> > +#ifdef UDA1341_DEBUG
> > +#define DBG(format, arg...) \
> > + printk(KERN_DEBUG "UDA1341: %s:" format "\n" , __func__, ## arg)
> > +#else
> > +#define DBG(format, arg...) do {} while (0)
> > +#endif
>
> Please use the standard pr_debug() macro.
>
> > +static int uda1341_write(struct snd_soc_codec *codec, unsigned int reg,
> > + unsigned int value)
> > +{
> > + int ret;
> > + u8 addr;
> > + u8 data = value;
> > +
> > + DBG("reg: %02X, value:%02X", reg, value);
> > +
> > + if (reg >= UDA1341_REGS_NUM) {
> > + DBG("Unkown register: reg: %d", reg);
> > + return -EINVAL;
> > + }
>
> That should probably print the error unconditionally.
>
> > +static int uda1341_hw_params(struct snd_pcm_substream *substream,
> > + struct snd_pcm_hw_params *params)
> > +{
>
> > + /* set SYSCLK / fs ratio */
> > + switch (uda1341->sysclk / params_rate(params)) {
>
> > + default:
> > + return -EINVAL;
> > + break;
> > + }
>
> No need for the break here. It's probably best to log an explicit error
> saying why the failure occurred - otherwise it'll be a bit obscure to
> users what's going on. A similar comment applies to the other error
> cases.
>
> > +static int uda1341_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> > + int clk_id, unsigned int freq, int dir)
> > +{
>
> > + /* Anything between 256fs*8Khz and 512fs*48Khz should be acceptable
> > + we'll error out on set_hw_params if it's not OK */
>
> This implies rather more flexibility than actually exists - hw_params()
> requires an exact multiplier here. You should probably add a note
> explaining that it's up to the machine driver to make sure that the rate
> is OK.
>
> > +SOC_SINGLE("Channel 1 mixer gain", UDA1341_EA000, 0, 0x1F, 1),
> > +SOC_SINGLE("Channgel 2 mixer gain", UDA1341_EA001, 0, 0x1F, 1),
>
> There's a typo here. Also, this and many of the other control names
> don't fit in with the ALSA control name spec so won't be displayed
> correctly by applications. For example, these should be "Volume" rather
> than "gain", and "switch" should be "Switch". There's documentation of
> the standard
> names in:
>
> Documentation/sound/alsa/ControlNames.txt
>
> > +SOC_SINGLE("DAC Gain switch", UDA1341_STATUS1, 6, 1, 0),
> > +SOC_SINGLE("ADC Gain switch", UDA1341_STATUS1, 5, 1, 0),
>
> What exactly do these controls do? "Gain switch" sounds wrong - I'd not
> expect the gain to be a boolean.
>
> > +static int uda1341_soc_suspend(struct platform_device *pdev,
> > + pm_message_t state)
> > +{
> > + struct snd_soc_device *socdev = platform_get_drvdata(pdev);
> > + struct snd_soc_codec *codec = socdev->codec;
> > + struct uda1341_platform_data *pd;
> > +
> > + uda1341_dapm_event(codec, SNDRV_CTL_POWER_D3cold);
> > +
> > + pd = (struct uda1341_platform_data *) codec->control_data;
> > + if (pd->power)
> > + pd->power(0);
>
> I'd expect that dapm_event() (which is now called set_bias_level())
> would be handling the power callback too.
>
> > +struct snd_soc_codec_device soc_codec_dev_uda1341 = {
> > + .probe = uda1341_soc_probe,
> > + .remove = uda1341_soc_remove,
> > +#if defined(CONFIG_PM)
> > + .suspend = uda1341_soc_suspend,
> > + .resume = uda1341_soc_resume,
> > +#endif /* CONFIG_PM */
>
> The standard idiom for this is to have an else defining the functions to
> NULL pointers above and then no ifdef here.
>
> > + void (*power) (int);
>
> I'd really expect to see this being passed an argument specifying what
> it's interacting with.
>
> > @@ -16,7 +16,7 @@ config SND_S3C2443_SOC_AC97
> > tristate
> > select AC97_BUS
> > select SND_SOC_AC97_BUS
> > -
> > +
> > config SND_S3C24XX_SOC_NEO1973_WM8753
> > tristate "SoC I2S Audio support for NEO1973 - WM8753"
> > depends on SND_S3C24XX_SOC && MACH_NEO1973_GTA01
>
> Random whitespace change here...
>
> > --- /dev/null
> > +++ b/sound/soc/s3c24xx/s3c24xx_uda1341.c
>
> > +/*#define S3C24XX_UDA1341_DEBUG 1*/
> > +#ifdef S3C24XX_UDA1341_DEBUG
> > +#define DBG(x...) printk(KERN_DEBUG "s3c24xx-i2s: " x)
> > +#else
> > +#define DBG(x...)
> > +#endif
>
> pr_debug().
>
> > +static int s3c24xx_uda1341_hw_params(struct snd_pcm_substream *substream,
> > + struct snd_pcm_hw_params *params)
> > +{
>
> I can follow this function but it'd be nice to see more comments in
> here. It looks like you could clarify it by iterating over a table of
> possible clock sources rather than writing each out in code.
>
> It also looks like this should be imposing constraints which prevent the
> configuration of different rates for the DAC and ADC - several existing
> codec drivers like the wm8903 provide examples of doing this.
>
> > + ret = cpu_dai->dai_ops.set_sysclk(cpu_dai, clk_source , clk,
> > + SND_SOC_CLOCK_IN);
> > + if (ret < 0)
> > + return ret;
>
> You want to run this through scripts/checkpatch.pl.
>
> > +static void setdat(struct uda1341_platform_data *p, int v)
> > +{
> > + s3c2410_gpio_setpin(s3c24xx_uda1341_l3_pins->l3_data, v > 0);
> > + s3c2410_gpio_cfgpin(s3c24xx_uda1341_l3_pins->l3_data,
> > + S3C2410_GPIO_OUTPUT);
> > +}
>
> Is there any reason for setting the pins to output mode each time? It
> looks like you could just configure the mode once at startup and then
> only set the pin status here.
>
> > +static void s3c24xx_uda1341_power(int en)
> > +{
> > + if (s3c24xx_uda1341_l3_pins->power)
> > + s3c24xx_uda1341_l3_pins->power(en);
> > +}
>
> This feels like it ought to be initialised conditionally rather than
> having the check for the pin it.
>
> > + ret = platform_device_add(s3c24xx_uda1341_snd_device);
>
> > + xtal = clk_get(NULL, "xtal");
> > + pclk = clk_get(NULL, "pclk");
>
> This should be done in the init function for the device and should
> really check the return value in case it can't get the clock for some
> reason. Ideally there'd be a dev there, but I'd need to check if the
> s3c24xx stuff does that.
>
> -------------------------------------------------------------------
> List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
> FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php
> Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php
--
Kristoffer Ericson <kristoffer.ericson@xxxxxxxxx>
Attachment:
pgpq9ZISz0E6Q.pgp
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
