Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup

[Jerome Brunet <jbrunet@xxxxxxxxxxxx>]

On Mon 13 May 2024 at 00:11, Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx> wrote:

> Hi Jerome
>
> Thank you for your feedback and analysis !
>
>>  - 1st problem: I see that following your removal of
>>    snd_soc_dai_link_set_capabilities(), the dpcm_playback/capture flags
>>    are no longer properly initialised in the amlogic card drivers.
>>    That will need fixing.
> (snip)
>> This codec is not meant to have capture channels.
>> I think DT description and the card driver settings (before the removal of
>> snd_soc_dai_link_set_capabilities()) are correct.
>
> OK, I see. Thank you for your analysis.
>
> The problem was my patch checks CPU direction vs Codec direction only,
> thus, it will indicates unexpected warnings, like this case.
>
> Thank you for finding it, I hope v2 patch should be OK for you.
>

I'll check

>> IMO, those check become too restrictive. We are adding a lot of code I'm
>> not sure I understand what we stand by going so far in the
>> consistency checks.
>> 
>> Initially those dpcm_playback/capture flag could be used to just
>> forcefully disable a link direction, regardless of the CPUs or codecs present
>> on the link. It was just another setting and it was not required to be consistent
>> with anything. It just declared whether the direction was allowed on the
>> link, or not.
>> 
>> It changed this commit, and the flags suddenly needed to be consistent
>> with whatever was on link:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc?h=v6.9-rc7&id=b73287f0b074
>> 
>> I complained back then and I still don't think this change was good.
>> 
>> If the flags needs to consistent with what is on the link, and we have
>> the ability to check it, why let card drivers set it and then complain
>> about it in ASoC checks ? Why not deal with it in the framework directly ?
>> 
>> I think the simplest solution would to just go back to the initial
>> meaning of these flags which was just the card driver declaring the
>> direction allowed/disallowed on a link. Then ASoC can mix that
>> information with whatever it finds with DAI drivers and figure out what
>> is actually possible.
>> 
>> It would give a clear and separate semantic meaning to those flags
>> instead something redundant with the DAI driver info.
>> 
>> What we have currently is not straight forward to set and check.
>> It makes the code unnecessarily complicated and difficult to maintain. I
>> think the difficulties with this patchset are a good illustration of
>> that, unfortunately.
>
> By this patch-set, it will be handled automatically via CPU and Codec
> driver settings, Card driver will no longer need to consider about
> direction (like non-DPCM), and I hope people (including you) will be
> happy about that.
>

If it makes things simpler, I am happy for sure :)

However, I'm a bit confused. If it is handled automatically by the CPUs
and Codecs settings, does it mean dpcm_playback/capture flags are
no-ops from now on ?

Should I update my card drivers to ditch those flags completely ?

May I still disable a direction on a link from the card driver, like in
the case I described above, when a TDM link has no slots for a direction ?

> Thank you for your help !!
>
> Best regards
> ---
> Renesas Electronics
> Ph.D. Kuninori Morimoto


-- 
Jerome