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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
