[alsa-devel] [PATCH] ASoC: add RT5670 CODEC driver

[Bard Liao]

> -----Original Message-----
> From: Mark Brown [mailto:broonie at kernel.org]
> Sent: Thursday, December 19, 2013 8:25 PM
> To: Bard Liao
> Cc: lgirdwood at gmail.com; alsa-devel at alsa-project.org; Flove; Oder Chiou
> Subject: Re: [PATCH] ASoC: add RT5670 CODEC driver
> 
> On Mon, Dec 16, 2013 at 08:11:56PM +0800, bardliao at realtek.com wrote:
> > From: Bard Liao <bardliao at realtek.com>
> 
> This patch is far too big, it really needs to be split up into a patch series to
> make it reviewable.  I'd suggest at least doing something like splitting it into
> one patch adding the CODEC functionality and then a second patch adding the
> DSP support.  Probably the header ought to be a separate patch too.

I will send the audio function part first.

> > +unsigned rt5670_pdm1_read(struct snd_soc_codec *codec, unsigned int
> > +reg) {
> 
> > +EXPORT_SYMBOL(rt5670_pdm1_read);
> 
> Why is this exported at all and why is it exported non-GPL?

If a pdm device connect to RT5670, it may need to be controlled by RT5670's pdm read/write commands.
I will export it with GPL.

> > +static int rt5670_asrc_event(struct snd_soc_dapm_widget *w,
> > +	struct snd_kcontrol *kcontrol, int event) {
> > +
> > +	switch (event) {
> > +	case SND_SOC_DAPM_POST_PMU:
> > +		snd_soc_write(w->codec, RT5670_ASRC_1, 0xffff);
> > +		snd_soc_write(w->codec, RT5670_ASRC_2, 0x1221);
> > +		snd_soc_write(w->codec, RT5670_ASRC_3, 0x0022);
> > +		break;
> > +	case SND_SOC_DAPM_PRE_PMD:
> > +		snd_soc_write(w->codec, RT5670_ASRC_1, 0);
> > +		snd_soc_write(w->codec, RT5670_ASRC_2, 0);
> > +		snd_soc_write(w->codec, RT5670_ASRC_3, 0);
> > +	default:
> > +		return 0;
> > +	}
> 
> This looks an awful lot like it might be writing some use case dependant
> parameters...  since it's just uncommented magic numbers it's hard to tell
> but there's no obvious configuration for the ASRC anywhere in the driver.

I will add control to configure ASRC's parameters.

> > +	if (rt5670->pdata.asrc_en) {
> > +		snd_soc_dapm_new_controls(&codec->dapm, rt5670_asrc_widgets,
> > +				ARRAY_SIZE(rt5670_dapm_widgets));
> > +		snd_soc_dapm_add_routes(&codec->dapm, rt5670_asrc_routes,
> > +				ARRAY_SIZE(rt5670_dapm_routes));
> > +	}
> 
> Why is this configured in platform data?

Usually, we don't enable ASRC function.
But in some platforms, they will need to enable ASRC function to get clock properly.
And I think this is not needed to configure in run time.

Thanks.