Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren <swarren@wwwdotorg.org>]

On 03/26/2013 07:15 PM, Mark Brown wrote:
> On Tue, Mar 26, 2013 at 05:35:38PM -0600, Stephen Warren wrote:
> 
>> Mark, a couple questions: * Is the custom index_reg device attr
>> file OK, or should I remove that? * Do new CODEC drivers have to
>> use regmap directly, or is using the ASoC IO system still OK?
> 
> Convert to regmap please - it looks like this ought to be using
> the paging support from a quick glance at what the register I/O
> stuff is doing.  The device file appears to duplicate the register
> dump stuff and isn't suitable for sysfs anyway as it's not one
> value per file, it ought to be in debugfs.

Ah, I hadn't seen the ranges stuff before. I think that should work.

>> +/* IN1/IN2 Input Type */ +static const char * const
>> rt5640_input_mode[] = { +	"Single ended", "Differential"};
> 
> This looks like platform data.

I thought the idea was to expose all the CODEC's configuration/routing
through DAPM, and then let everything get set up through controls at
run-time? Looking at the WM8903 driver, it seems like it has the exact
same kind of option.

>> +static const char * const rt5640_data_select[] = { +	"Normal",
>> "left copy to right", "right copy to left", "Swap"};
> 
> DAPM?
> 
>> +/* DMIC */ +static const char * const rt5640_dmic_mode[] =
>> {"Disable", "DMIC1", "DMIC2"}; + +static const
>> SOC_ENUM_SINGLE_DECL(rt5640_dmic_enum, 0, 0, rt5640_dmic_mode);
> 
> DAPM?

I'm not sure what change you're asking for here; isn't this defining a
control that influences the DAPM routing?

Perhaps the issue is that the enum above feeds into a snd_kcontrol,
rather than being an snd_soc_dapm_route that's conditional upon one of
the values in that list?

If so, that's going to be a lot of stuff to change in the driver
considering that I can't actually test any of the DMIC (or even analog
mic) support yet since I can't get it working. Perhaps I should just
rip out all the widgets/controls I can't test?

>> +static const struct snd_soc_dapm_route rt5640_dapm_routes[] = { 
>> +	{"IN1P", NULL, "LDO2"}, +	{"IN2P", NULL, "LDO2"}, + +	{"IN1P",
>> NULL, "MIC1"}, +	{"IN1N", NULL, "MIC1"}, +	{"IN2P", NULL,
>> "MIC2"}, +	{"IN2N", NULL, "MIC2"},
> 
> Are MIC1 and MIC2 pins on the device?

Hmmm. I see SND_SOC_DAPM_INPUT()s for MIC1 and MIC2, but they don't
appear to be pins on the device. Rather, IN1P/N and IN2P/N are the
pins, and also are declared as SND_SOC_DAPM_INPUT().

I'm not really sure what MIC1/MIC2 are meant to represent...

>> +	{"DMIC L1", NULL, "DMIC CLK"}, +	{"DMIC L2", NULL, "DMIC
>> CLK"},
> 
> Shouldn't the DMIC CLK widget be handling the clock enables/muxes
> from the DMIC event above?

Oh right, so you mean:

* Add a SND_SOC_DAPM_OUTPUT() for the "DMIC CLK".

* Change the "DMIC L*" from SND_SOC_DAPM_PGA_E() to
SND_SOC_DAPM_PGA(), and move the event to the "DMIC CLK" widget.

* Add routing table entries where "DMIC1 *" are each fed from "DMIC
CLK", with "conditions" based on which DMIC is "on":

{"DMIC L1" "DMIC1" "DMIC CLK"},
{"DMIC R1" "DMIC1" "DMIC CLK"},
{"DMIC L2" "DMIC2" "DMIC CLK"},
{"DMIC R2" "DMIC2" "DMIC CLK"},

or something like that?

>> +static int get_sdp_info(struct snd_soc_codec *codec, int
>> dai_id)
...
>> +	bclk_ms = frame_size > 32 ? 1 : 0;
> 
> Grumble.

Sorry, I don't understand the issue here. (through lack of familiarity
with the issue; I'm not saying there isn't one).

>> +static int rt5640_prepare(struct snd_pcm_substream *substream, +
>> struct snd_soc_dai *dai) +{ +	struct snd_soc_pcm_runtime *rtd =
>> substream->private_data; +	struct snd_soc_codec *codec =
>> rtd->codec; +	struct rt5640_priv *rt5640 =
>> snd_soc_codec_get_drvdata(codec); + +	rt5640->aif_pu = dai->id; +
>> return 0; +}
> 
> This looks like only one DAI can be active at once, shouldn't there
> be some sort of checking for busy here?

I haven't read the spec through in explicit detail, but I didn't see
anything to indicate that only one DAI could be active at once. In
fact the diagrams in the documentation make it look at least somewhat
cross-bar-like.

It looks like rt5640->aif_pu is used solely by set_dmic_clk() to
somehow derive the DMIC CLK from the DAI LRCLK based on which DAI is
actually active. I assume "which DAI is actually active" should be
implemented as "which DAI is routed from the DMICs". I guess this
should be implemented in a very different way then?

> 
>> +	dai_sel = get_sdp_info(codec, dai->id); +	if (dai_sel < 0) { +
>> dev_err(codec->dev, "Failed to get sdp info: %d\n", dai_sel); +
>> return -EINVAL; +	}
> 
> I suspect using dai->base (or just picking a suitable value for
> dai->id even) would be simpler.

So there is something dynamic here; there are 2 physical DAIs on the
chip package, but somehow I think there are 3 digital channels within
the device that can be routed to/from the physical DAIs in various
(sometimes broadcast?) configurations based on the rt5640_dai_iis_map.
Hence, the implementation of get_sdp_info() looks up the value of the
register that configures that routing, and returns data that isn't
static...

I wonder if this all shouldn't be explicitly represented by DAPM
widgets/routes, but the diagrams and text in the documentation aren't
particularly clear on what this third thing is that the code seems to
support:-(

>> +static int rt5640_resume(struct snd_soc_codec *codec) +{ +	int
>> ret = 0 ; + +	codec->cache_sync = 1; +	ret =
>> snd_soc_cache_sync(codec); +	if (ret) { +		dev_err(codec->dev,
>> "Failed to sync cache: %d\n", ret); +		return ret; +	} +
>> rt5640_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> 
> We also sync the cache in the bias management code.

In other words, I should just remove the call to
snd_soc_cache_sync(codec) here, since it's redundant?