On Mon, Nov 10, 2008 at 08:04:49PM +0100, chri wrote:
> On Mon, Nov 10, 2008 at 2:34 PM, Mark Brown <broonie@xxxxxxxxxxxxx> 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
