Re: [alsa-devel] [PATCH RESEND 0/2] ALSA SOC driver for uda134x codec

On Thu, Nov 13, 2008 at 04:41:30PM +0100, Christian Pellegrin wrote:

This patch adds support for the UDA1341 codec. It is *heavily* based on the one made by Zoltan Devai but:

since the UDA is the only use of the L3 protocol I can find I just embedded a stripped-down L3 bit-banging algorithm from the original RMK work. It is really small.

Generated on 20081113 against f7e1798a78a30bae1cf3ebc3e1b6f7c1bac57756

A couple of minor procedural points: - Normally the patches in a series are numbered starting from 1 with mail 0 being a cover letter ("This series does..." or similar). - Information like the "Generated on" should go after the --- with the diffstat so that it doesn't go in the commit message.

+++ b/include/sound/uda134x.h

+extern struct snd_soc_dai uda134x_dai; +extern struct snd_soc_codec_device soc_codec_dev_uda134x;

At least this should be in a header sound/soc/codecs - as I said previously that is the idiom used by codec drivers. However...

+struct uda134x_platform_data {

• struct l3_pins l3;
• void (*power) (int);
• int model;

+#define UDA134X_UDA1340 0 +#define UDA134X_UDA1341 1 +#define UDA134X_UDA1344 2 +};

...putting this in a separate file in include/sound is a good idea.

Is having UDA1340 as zero a good idea - that'll mean that if someone forgets to initialise the model it'll come out as UDA1340? It might be better to start the model numbers from 1 so that it'll be obvious if that happens. Might also be an idea to use an enum rather than the series of #defines?

+config SND_SOC_UDA134X

•   tristate
  

•   select SND_SOC_L3
  

Note that the select here won't actually do any good but it's useful for documentation.

diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index 3b9b58a..9af993e 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -8,6 +8,8 @@ snd-soc-tlv320aic23-objs := tlv320aic23.o snd-soc-tlv320aic26-objs := tlv320aic26.o snd-soc-tlv320aic3x-objs := tlv320aic3x.o snd-soc-twl4030-objs := twl4030.o +snd-soc-l3-objs := l3.o +snd-soc-uda134x-objs := uda134x.o

Please keep this and the other build stuff sorted (it helps merges).

• if (reg >= UDA134X_REGS_NUM) {

• printk(KERN_ERR "%s unkown register: reg: %d",
  

•        __func__, reg);
  

Could use pr_error() here and elsewhere but doesn't make much difference either way - up to you.

+static int uda134x_startup(struct snd_pcm_substream *substream) +{

• struct snd_soc_pcm_runtime *rtd = substream->private_data;
• struct snd_soc_device *socdev = rtd->socdev;
• struct snd_soc_codec *codec = socdev->codec;
• struct uda134x_priv *uda134x = codec->private_data;
• struct snd_pcm_runtime *master_runtime;
• if (uda134x->master_substream) {

• master_runtime = uda134x->master_substream->runtime;
  

• pr_debug("%s costrining to %d bits at %d\n", __func__,
  

constraining.

+static int uda134x_set_bias_level(struct snd_soc_codec *codec,

• 		  enum snd_soc_bias_level level)
  

+{

• u8 reg;
• struct uda134x_platform_data *pd = codec->control_data;
• int i;
• u8 *cache = codec->reg_cache;
• pr_debug("%s bias level %d\n", __func__, level);
• switch (level) {
• case SND_SOC_BIAS_ON:

• /* power on */
  

• if (pd->power)
  

• 	pd->power(1);
  

• /* Sync reg_cache with the hardware */
  

• for (i = 0; i < ARRAY_SIZE(uda134x_reg); i++)
  

• 	codec->write(codec, i, *cache++);
  

• /* ADC, DAC on */
  

• reg = uda134x_read_reg_cache(codec, UDA134X_STATUS1);
  

• uda134x_write(codec, UDA134X_STATUS1, reg | 0x03);
  

• break;
  

This should probably be done when going into SND_SOC_BIAS_STANDBY or at least _PREPARE. The codec will be brought up to _ON whenever a playback or record is active, spending most of the rest of the time at _STANDBY. The result will be that you're syncing the register cache to the codec every time playback starts, even if the codec is already on. _ON is expected to be very quick.

It might be worth implementing DAPM for the ADC and DAC power - the savings are probably very small but it should be very straightforward to implement. Not essential, though - doing it in bias_level() is also OK.

+EXPORT_SYMBOL(uda134x_dai);

Note that since ASoC core is all EXPORT_SYMBOL_GPL() it'll be difficult for anyone to actually use this from non-GPLed code.

• pd = codec_setup_data;
• switch (pd->model) {
• case UDA134X_UDA1340:
• case UDA134X_UDA1341:
• case UDA134X_UDA1344:

• break;
  

• default:

• printk(KERN_ERR "UDA134X SoC codec: "
  

•        "unsupported model\n");
  

• return -EINVAL;
  

Probably worth printing out what the model was set to if it's not recognised.

+struct snd_soc_codec_device soc_codec_dev_uda134x = {

• .probe = uda134x_soc_probe,
• .remove = uda134x_soc_remove,

+#if defined(CONFIG_PM)

• .suspend = uda134x_soc_suspend,
• .resume = uda134x_soc_resume,

+#else

• .suspend = NULL,
• .resume = NULL,

+#endif /* CONFIG_PM */

The #else should be with the definition of the functions so there's no need for a conditional here.