On 07/29/2016 04:24 AM, Kuninori Morimoto wrote:
Hi Lars
Thank you for your feedback
int snd_soc_runtime_set_dai_fmt(xxx) { ... /* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */ if (cpu_dai->codec) { ... } ... }
(snip)
This is for CODEC<->CODEC links where no CPU DAI is involved and the "CPU" side of the DAI link is actually a CODEC.
Wow !!
Ideally we'd make the DAI links agnostic to what is connected, e.g. it shouldn't matter what type is connected to what side. But that does not work since things are anti-symmetric between CPU and CODEC DAIs. On CODEC DAIs the capture stream is output, on CPU DAIs the capture stream is input, similar thing for playback. So we need to know whether the DAI is a CPU DAI or a CODEC DAI.
Fixing this at this point is near to impossible since it requires invasive changes to all existing drivers. So we need this code and can't drop it.
The best we can do is try to come up with a generic interface that is DAI type independent and recommend this interface for new drivers.
Hmm... OK, now, I'm investigating ALSA SoC struct connection. And yes, existing code has deep relationship each other. Thus, fixing/droping seems very dificult... I think each struct has random pointer connection which doesn't care about ALSA SoC's hierarchy.
I would like to indicate my opinion here. My strange point are...
- snd_soc_pcm_runtime has pointer to Card/Codec/Platform, but no CPU
- snd_soc_dai has pointer to [component] and [Codec] [component] is its parent, [Codec] is maybe Hack
dai->codec is something we could get rid of since it is the same as dai->component->codec. And for drivers were we know that it is a CODEC driver you can use snd_soc_component_to_codec(dai->component). For those drivers we know that this returns a valid pointer.
But there are a lot of users which all need to be updated. If you want to do it go ahead. If you do the conversion this should be done by the use of helper functions, so we can change the implementation if necessary.
- [component] has pointer to [card] and [Codec] [card] is understandable, [Codec] is maybe Hack
It seems many struct would like to access to [Codec], thus, each struct has codec pointer (as hack ?), but it is ignoring ALSA SoC hierarchy.
This is a leftover of the old hierarchy where snd_soc_codec was not a sub-struct of snd_soc_component.
It will be more clear if...
IDEA 1
Each component (= CPU/Codec/Platform) has pointer to Card now. How about Card has pointer to each component ? if so, we can access to every struct. Card -> CPU/Codec/Platform, CPU/Codec/Platform -> Card Then, we want to have component related macro/flag
#define component_is_cpu(component) (component->flag & COMPONENT_CPU) #define component_is_codec(component) (component->flag & COMPONENT_CODEC) #define component_is_platform(component) (component->flag & COMPONENT_PLATFORM)
#define component_to_cpu() container_of(xxx) #define component_to_codec() container_of(xxx) #define component_to_platorm() container_of(xxx)
I though about this when doing the refactoring of the ASoC core. But in the end it does not make too much of a difference whether we have a flag field or a CODEC pointer field. The former would have just caused more code churn so I went with the later.
You wouldn't be able to simplify the code by this. You need to replace all component->codec checks by component_is_codec(component). So the overall code complexity stays the same.
The helper macros might still make sense to hide the actual implementation.
IDEA 2
if IDEA1 was OK, snd_soc_pcm_runtime already has card pointer. This means snd_soc_pcm_runtime can access to every component struct.
snd_soc_dai has its parent component. This means snd_soc_dai can access to Card, and Card can access to every component. And then, cpu_dai->codec has no issue if we can use component_is_xxx() macro ?
- if (cpu_dai->codec) + if (component_is_codec(cpu_dai->component))If IDEA1 / IDEA2 are OK, we can cleanup each struct, especially hack-able random pointer ?
In my opinion the flags are just as much a hack as the pointer. In an ideal setup the component does not need to know what type it is. The reason why we need this in ASoC is because the framework has grown over time and we need to support legacy code.