Re: [PATCH v2 7/9] ASoC: Codec: Add sti platform codec

[Arnaud Pouliquen <arnaud.pouliquen@st.com>]

On 05/25/2015 03:01 PM, Mark Brown wrote:
>> +static int  stih407_sas_dac_supply(struct snd_soc_dapm_widget *w,
>> +				   struct snd_kcontrol *kcontrol, int event) {
>> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
>> +	struct sti_sas_data *drvdata = dev_get_drvdata(codec->dev);
>> +	struct sti_dac_audio *dac = &drvdata->dac;
>> +	int ret = 0;
>> +
>> +	switch (event) {
>> +	case SND_SOC_DAPM_PRE_PMU:
>> +		/* Enable analog */
>> +		ret = regmap_field_write(dac->field[STIH407_DAC_STANDBY_ANA],
>> +					 0);
>> +		/* Disable standby */
>> +		if (!ret)
>> +			ret = regmap_field_write(
>> +				dac->field[STIH407_DAC_STANDBY], 0);
>> +		break;
>
> These event functions all look very similar and I can't help but think
> that they look awfully like the sort of register write sequences that
> DAPM normally generates anyway.  Is there any great reason for not doing
> these by registering multiple widgets with routes between them rather
> than with custom code?
>
I need to respect the sequence for power up /power down. for sure I 
could manage it using 3 DAPMs with routes, but how to ensure sequence?
Moreover, i will need to implement a cpu dai DAPM to manage 
uniperipheral clock. This clock need to be enabled/disabled before/after 
DAC to avoid plop. So would prefer to manage the DAC sequence in a 
single function...

>> +static int sti_sas_hw_params(struct snd_pcm_substream *substream,
>> +			     struct snd_pcm_hw_params *params,
>> +			     struct snd_soc_dai *dai)
>> +{
>> +	struct snd_soc_codec *codec = dai->codec;
>> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> +	int div;
>> +
>> +	div = sti_sas_dai_clk_div[dai->id];
>> +	if (cpu_dai->driver->ops->set_clkdiv)
>> +		return cpu_dai->driver->ops->set_clkdiv(cpu_dai,
>> +							SND_SOC_CLOCK_OUT, div);
>> +	dev_warn(codec->dev, "WARN: CPU DAI not support sysclk div");
>
> This is worrying, we shouldn't be peering inside the CPU DAI like this.
> I'd expect this to either be done autonomously by the CPU DAI or handled
> in a machine driver.
>
I think i misunderstand your remark in V1...but i still not understand 
how you want that i implement it, if i can't neither use 
snd_soc_dai_set_clkdiv (except implement it in simple_card).
Please could you precise how you would like that i implement the feature?

I'm kind surprise that nobody else have this kind of setup. I know at 
least one codec that should need this kind of service: AK4628.
For this codec, depending on runtime frequency, a division value should 
be apply between MCLK and SCLK...