[alsa-devel] [PATCH] S3C64XX I2S: Added machine driver for WM8580

[jassi brar]

seems my original email to alsa-devel got stuck.
lemme resend the patch with some of ur suggestions incorporated and
continue from there.

On Thu, Sep 17, 2009 at 5:00 AM, Mark Brown
<broonie at opensource.wolfsonmicro.com> wrote:
> On Wed, Sep 16, 2009 at 07:02:42PM +0900, Jassi wrote:
>
>> New machine driver for WM8580 I2S i/f on SMDK64XX.
>> By default SoC-Slave is set and WM8580 is configured to use it's
>> PLLA to generate clocks from a 12MHz crystal attached to WM8580.
>
> This looks sane as a starting point and most of the driver looks OK.
>
>> I2S controller-0 is set as default, set S3C64XX_I2S_CNTRLR to 2
>> inorder to use I2S_v4 controller of S3C6410.
>
> I'm not entirely sure what's going on in this configuration - the
> primary AIFs of the CODEC appear to be hard wired to the IISv4 interface
> in the schematics I have, IIS port 0 is only wired to the secondary AIF
> of the WM8580 so will need support for that implementing.  The end
> result should be an extra set of DAI links for the secondary AIF.  Could
> you clarify, please?
>
>> +/* SMDK64XX has a 12MHZ crystal attached to WM8580 */
>> +#define SMDK64XX_WM8580_FREQ (12000000)
>
> No need for the () here.
>
>> +     switch (params_rate(params)) {
>> +     case 8000:
>> +             pll_out = 8192000 / 4; break;
>
> These are all a bit odd - you're taking two magic numbers and performing
> an operation on them, giving another magic number.  For the most part
> the resulting magic number is always 256fs (I suspect the cases where
> they aren't should be 256fs).  Probably replacing all of this with
>
>        pll_out = params_rate(params) * 256
>
> would work.
>
>> +     /* We block CODCLK from MCLK of WM8580 */
>> +     ret = snd_soc_dai_set_sysclk(cpu_dai, S3C64XX_CODCLKSRC_EXT,
>> +                                     0, SND_SOC_CLOCK_IN);
>> +     if (ret < 0)
>> +             return ret;
>
> The code is OK (modulo the issues with the previous patch) but the
> comment is exceptionally difficult to parse.
>
>> +     /* Explicitly set WM8580-ADC/DAC to source from MCLK */
>> +     ret = snd_soc_dai_set_clkdiv(codec_dai, WM8580_ADC_CLKSEL,
>> +                                     WM8580_CLKSRC_MCLK);
>
> Obviously this will need updating for the actual patch for the WM8580
> driver.
>
>> +     if (ret < 0)
>> +             return ret;
>> +     ret = snd_soc_dai_set_clkdiv(codec_dai, WM8580_DAC_CLKSEL,
>
> Missing blank line here.
>
>> +     /* Don't output to the disconnected pin */
>> +     ret = snd_soc_dai_set_clkdiv(codec_dai, WM8580_CLKOUTSRC,
>> +                                     WM8580_CLKSRC_NONE);
>> +     if (ret < 0)
>> +             return ret;
>
> If you're going to do this do it on init, it's nothing to do with
> starting the audio stream.
>
>> +     switch (params_format(params)) {
>> +     case SNDRV_PCM_FORMAT_U8:
>> +     case SNDRV_PCM_FORMAT_S8:
>> +             bfs = 16;
>> +             rfs = 256;
>> +             break;
>> +     case SNDRV_PCM_FORMAT_U16_LE:
>> +     case SNDRV_PCM_FORMAT_S16_LE:
>> +             bfs = 32;
>> +             rfs = 256;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>
> This ought to get factored out into the CPU DAI driver, at least as a
> library function - there's nothing board specific going on here.
>
>> +     /* LineIn enabled by default */
>> +     snd_soc_dapm_disable_pin(codec, "MicIn");
>> +     snd_soc_dapm_enable_pin(codec, "LineIn");
>
> Please add a comment explaining that if this is changed it needs to be
> done with a resistor fit mod - this begs the question "How do I change
> away from the default?", especially given the many switches for things
> like this on the board.
>
> For completeness if you're going to handle the mapping to the external
> jacks there's also the possiblity that the signals will be switched out
> to the PMIC card or WM9713.  It's probably better to just not bother,
> it's not really adding anything since there's no jack detect or anything
> on the board.  All that'll happen is that people are forced to rebuild
> for DIP switch changes.
>
> The other option would be to add controls mirroring the DIP switch
> settings, but that'd not really cover the resistor fit stuff.
>
>> +     /* Stereo enabled by default */
>> +     snd_soc_dapm_enable_pin(codec, "Front-L/R");
>> +     snd_soc_dapm_disable_pin(codec, "Center/Sub");
>> +     snd_soc_dapm_disable_pin(codec, "Rear-L/R");
>
> Similar issues here - putting a fixed configuration from this in the
> driver doesn't buy anything.
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>