On Mon, Nov 10, 2008 at 08:04:49PM +0100, chri wrote:
On Mon, Nov 10, 2008 at 2:34 PM, Mark Brown broonie@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.