Re: [PATCH RFC v2 02/11] ASoC: meson: aiu-encoder-i2s: use gx_iface and gx_stream structures

[Jerome Brunet <jbrunet@baylibre.com>]

On mar. 14 avril 2026 at 17:13, Mark Brown <broonie@kernel.org> wrote:

> On Sat, Apr 11, 2026 at 04:57:27PM +0200, Valerio Setti wrote:
>
>> @@ -200,13 +200,17 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
>
>> -	aiu_encoder_i2s_divider_enable(component, true);
>> +	ret = gx_stream_set_cont_clocks(ts, iface->fmt);
>> +	if (ret)
>> +		dev_err(dai->dev, "failed to apply continuous clock setting\n");
>> +
>> +	aiu_encoder_i2s_divider_enable(component, 1);
>
> If we're checking the error here we should probably return it as well.
> Including the error code in the log message is also generally helpful.
>
>> @@ -214,16 +218,20 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
>>  static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream,
>>  				   struct snd_soc_dai *dai)
>>  {
>> +	struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
>>  	struct snd_soc_component *component = dai->component;
>>  
>> -	aiu_encoder_i2s_divider_enable(component, false);
>> -
>> -	return 0;
>> +	/* This is the last substream open and that is going to be closed. */
>> +	if (snd_soc_dai_active(dai) <= 1)
>> +		aiu_encoder_i2s_divider_enable(component, 0);
>> +	return gx_stream_set_cont_clocks(ts, 0);
>>  }
>
> Note that we only hw_free() if we preprared, but we enable in
> hw_params().

Huh interresting, I had not thought of that. Valerio and I discussed the
clock part a lot for this rework. It is the crux since since the
interface and clock setting lives in the AIU subsys but serves both the
AIU and AUDIN subsys.

Valerio maybe you could keep function above just to set the rate, but
enabling the clocks through a DAPM supply widget ? This is kind of what
the AXG is doing.

what do you think ?

(actually in the AXG the each formatter widget call CCF
clk_prepare_enable() but a supply widget poking the register would do
the same thing)

>
>> @@ -284,6 +295,8 @@ static int aiu_encoder_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
>>  	if (ret)
>>  		dev_err(dai->dev, "Failed to set sysclk to %uHz", freq);
>>  
>> +	aiu->i2s.iface.mclk_rate = freq;
>> +
>>  	return ret;
>>  }
>
> This means we store the new rate even if the set above failed.

-- 
Jerome