[alsa-devel] [PATCH] ALSA SOC driver for s3c24xx with uda1341

[Mark Brown]

On Mon, Nov 10, 2008 at 08:04:49PM +0100, chri wrote:
> On Mon, Nov 10, 2008 at 2:34 PM, Mark Brown <broonie at sirena.org.uk> wrote:

> ack. I was tempted to use pr_debug (and even better dev_dbg) but I
> went for the old printk method for uniformity with the rest of the
> drivers.

If you work with current git you'll see that the other drivers have been
converted to pr_debug().

> >> +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.

> they turn on/off an amplifier with 6db gain in the record and playback
> respectively.

Should be "ADC +6dB Switch" or similar then.

> >> +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.

> unfortunately the 2 clock sources are handled differently: PCLK can be
> divided, MPLL_in no. So I don't see much convenience in using a
> (complicated) table

A pointer to a table of dividers would handle it.  In any case, it needs
more comments.

> >> +     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.

> I already did it

I'd have expected checkpatch to catch the extra space you've got after
clk_source there...

> >> +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.

> It's not clear to me what you mean here.

This function is going to be used in a vtable where IIRC it can be
optional but it needs to check the value of power in s3c24xx_uda1341_l3_pins.
It feels like you've got too many layers of indirection here and that,
for example, the l3_pins power should be assignable directly to the
vtable.