[alsa-devel] [PATCH V4 1/3] ASoC: Alchemy AC97C/I2SC audio support

[Manuel Lauss]

On Mon, Jul 25, 2011 at 11:32 AM, Liam Girdwood <lrg at ti.com> wrote:
> On 24/07/11 11:11, Manuel Lauss wrote:
>> This patch adds ASoC support for the AC97 and I2S controllers
>> on the old Au1000/Au1500/Au1100 chips,
>>
>> AC97 Tested on a Db1500.  I2S untested since none of the boards
>> actually have an I2S codec wired up (just test pins).
>>
>> Signed-off-by: Manuel Lauss <manuel.lauss at googlemail.com>
>> ---
>> V4: dropped hunk which removed I2S constants in au1000.h header to avoid merge
>>     conflicts with other patches, use the context structure in psc.h since it
>>     fits really well.
>> V3: implemented feedback from Lars-Peter Clausen: src tidying, no more
>>     automatic dma device registration, split off db1000 board code.
>> V2: added untested I2S controller driver for completeness, removed the audio
>>     defines from the au1000 header as well.
>>
>
> Looks mostly OK, I just have some questions below:-
>> +#define AC97_RATES     \
>> +       SNDRV_PCM_RATE_8000_44100
>
> Just curious, is there any reason this doesn't support 48kHz ?

No reason really; fixed.


>> diff --git a/sound/soc/au1x/dma.c b/sound/soc/au1x/dma.c
>> new file mode 100644
>> index 0000000..20fedbd
>> +static int alchemy_pcm_hw_params(struct snd_pcm_substream *substream,
>> +                                struct snd_pcm_hw_params *hw_params)
>> +{
>> +       struct audio_stream *stream = ss_to_as(substream);
>> +       int err;
>> +
>> +       err = snd_pcm_lib_malloc_pages(substream,
>> +                                      params_buffer_bytes(hw_params));
>> +       if (err < 0)
>> +               return err;
>> +       return au1000_setup_dma_link(stream,
>> +                                    params_period_bytes(hw_params),
>> +                                    params_periods(hw_params));
>
>
> What happens if this fails ? You already have malloc'ed some pages.

also fixed,


>> +static int alchemy_pcm_new(struct snd_card *card,
>> +                          struct snd_soc_dai *dai,
>> +                          struct snd_pcm *pcm)
>
> This API call has been updated to only pass the rtd *

fixed,


>> diff --git a/sound/soc/au1x/i2sc.c b/sound/soc/au1x/i2sc.c
>> +static inline unsigned long RD(struct au1xpsc_audio_data *ctx, int reg)
>> +{
>> +       return __raw_readl(ctx->mmio + reg);
>> +}
>> +
>> +static inline void WR(struct au1xpsc_audio_data *ctx, int reg, unsigned long v)
>> +{
>> +       __raw_writel(v, ctx->mmio + reg);
>> +       wmb();
>> +}
>
> Btw, just wondering if arch/mips already supplies a suitable RD()/WR() for you ?

with the proper context, no.  I've used these kind of macros in other
code, though :)


>> +static unsigned long msbits_to_reg(int msbits)
>> +{
>> +       switch (msbits) {
>> +       case 8:  return CFG_SZ_8;
>> +       case 16: return CFG_SZ_16;
>> +       case 18: return CFG_SZ_18;
>> +       case 20: return CFG_SZ_20;
>> +       case 24: return CFG_SZ_24;
>
> It's best to format all the switch statements consistently throughout your code.

also fixed,


>> +static int au1xi2s_drvsuspend(struct device *dev)
>> +{
>> +       struct au1xpsc_audio_data *ctx = dev_get_drvdata(dev);
>> +
>> +       WR(ctx, I2S_ENABLE, EN_D);      /* clock off, disable */
>> +
>> +       return 0;
>> +}
>> +
>> +static int au1xi2s_drvresume(struct device *dev)
>> +{
>
> Should we not enalbe the clock here (i.e. in order to balance the clock off in suspend) ?

According to the datasheet, these bits only control internal clock
supply to the I2S module
itself.  It's reenabled in the trigger callback.  Clock for the codec
is controlled through
another register in the central clock block.   As I wrote, this is an
untested driver since none
of the boards I have actually have an I2S codec wired up.  So this is
all guesswork based
on vague datasheets ;)


>> diff --git a/sound/soc/au1x/psc.h b/sound/soc/au1x/psc.h
>> index b30eadd..c59b9e5 100644
>> --- a/sound/soc/au1x/psc.h
>> +++ b/sound/soc/au1x/psc.h
>> @@ -1,7 +1,7 @@
>>  /*
>> - * Au12x0/Au1550 PSC ALSA ASoC audio support.
>> + * Alchemy ALSA ASoC audio support.
>>   *
>> - * (c) 2007-2008 MSC Vertriebsges.m.b.H.,
>> + * (c) 2007-2011 MSC Vertriebsges.m.b.H.,
>>   *     Manuel Lauss <manuel.lauss at gmail.com>
>>   *
>>   * This program is free software; you can redistribute it and/or modify
>> @@ -13,7 +13,13 @@
>>  #ifndef _AU1X_PCM_H
>>  #define _AU1X_PCM_H
>>
>> -/* DBDMA helpers */
>> +#define PCM_TX 0
>> +#define PCM_RX 1
>
> Is there any need for these macros, SNDRV_PCM_STREAM_PLAYBACK and SNDRV_PCMP_STREAM_CAPTURE should be used for this type of logic.

As long as they both stay 0 and 1, might as well use them directly.  Fixed.
I'll remove them in a follow-up patch for the PSC drivers.

Thanks for the review!
        Manuel Lauss
--
To unsubscribe from this list: send the line "unsubscribe alsa-devel" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html