Hi,
On Mon, Nov 10, 2008 at 2:34 PM, Mark Brown <broonie@xxxxxxxxxxxxx> wrote:
>
> Ideally this should be submitted as multiple patches - at least one for
> the codec and one for the board, probably also one for the l3 interface.
>
ack
>
> Please generate patches against current git - the topic/asoc branch of:
>
ack.
> You should also add this to SND_SOC_ALL_CODECS. There's a bunch of
> other things like this that have changed between 2.6.27 and the current
> code - for example, the bias level configuration.
>
ack.
>
> There doesn't seem to be much benefit to adding a subdirectory for two
> source files here.
>
ack
>
> Please use the standard pr_debug() macro rather than defining custom
> ones.
>
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.
>
> These should be inline functions for type safety.
>
ack
>
> That should probably print the error unconditionally.
>
ack
>> + switch (uda1341->sysclk / params_rate(params)) {
>
>> + default:
>> + return -EINVAL;
>> + break;
>> + }
>
> No need for the break here. It's probably best to log an explicit error
> saying why the failure occurred - otherwise it'll be a bit obscure to
> users what's going on. A similar comment applies to the other error
> cases.
>
ack
>> +static int uda1341_set_dai_sysclk(struct snd_soc_dai *codec_dai,
>> + int clk_id, unsigned int freq, int dir)
>> +{
>
>> + /* Anything between 256fs*8Khz and 512fs*48Khz should be acceptable
>> + we'll error out on set_hw_params if it's not OK */
>
> This implies rather more flexibility than actually exists - hw_params()
> requires an exact multiplier here. You should probably add a note
> explaining that it's up to the machine driver to make sure that the rate
> is OK.
>
ack
>
> There's a typo here. Also, this and many of the other control names
> don't fit in with the ALSA control name spec so won't be displayed
> correctly by applications. For example, these should be "Volume" rather
> than "gain", and "switch" should be "Switch". There's documentation of
> the standard
> names in:
>
> Documentation/sound/alsa/ControlNames.txt
>
ack. I have to admit that I didn't check the names. I will read this
document and do the changes accordingly
>> +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.
>> + pd = (struct uda1341_platform_data *) codec->control_data;
>> + if (pd->power)
>> + pd->power(0);
>
> I'd expect that dapm_event() (which is now called set_bias_level())
> would be handling the power callback too.
>
ack
>> +struct snd_soc_codec_device soc_codec_dev_uda1341 = {
>> + .probe = uda1341_soc_probe,
>> + .remove = uda1341_soc_remove,
>> +#if defined(CONFIG_PM)
>> + .suspend = uda1341_soc_suspend,
>> + .resume = uda1341_soc_resume,
>> +#endif /* CONFIG_PM */
>
> The standard idiom for this is to have an else defining the functions to
> NULL pointers above and then no ifdef here.
>
ack
>> + void (*power) (int);
>
> I'd really expect to see this being passed an argument specifying what
> it's interacting with.
>
ack
>> -
>> +
>> config SND_S3C24XX_SOC_NEO1973_WM8753
>> tristate "SoC I2S Audio support for NEO1973 - WM8753"
>> depends on SND_S3C24XX_SOC && MACH_NEO1973_GTA01
>
> Random whitespace change here...
>
ack, sorry for that
>
>> +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
> It also looks like this should be imposing constraints which prevent the
> configuration of different rates for the DAC and ADC - several existing
> codec drivers like the wm8903 provide examples of doing this.
>
ack
>> + 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
>> +static void setdat(struct uda1341_platform_data *p, int v)
>> +{
>> + s3c2410_gpio_setpin(s3c24xx_uda1341_l3_pins->l3_data, v > 0);
>> + s3c2410_gpio_cfgpin(s3c24xx_uda1341_l3_pins->l3_data,
>> + S3C2410_GPIO_OUTPUT);
>> +}
>
> Is there any reason for setting the pins to output mode each time? It
> looks like you could just configure the mode once at startup and then
> only set the pin status here.
no, I'll clean this up too.
>
>> +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.
>> + ret = platform_device_add(s3c24xx_uda1341_snd_device);
>
>> + xtal = clk_get(NULL, "xtal");
>> + pclk = clk_get(NULL, "pclk");
>
> This should be done in the init function for the device and should
> really check the return value in case it can't get the clock for some
> reason. Ideally there'd be a dev there, but I'd need to check if the
> s3c24xx stuff does that.
>
I'll looka at some other s3c24xx clock user and do the changes.
Thanks,
--
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
