On 4/10/2024 4:54 AM, Kuninori Morimoto wrote:
Hi Mark, Amadeusz, Jerome, Pierre-Louis
This is v2.5 patch which is preparation for v3. Because full-patch-set has over 20 patches, to avoid posting huge patch-bomb, I try to post main patch only for preparation.
In v2.5, it will use xxx_assertion flag, and exising dpcm_xxx and xxx_only flags will be converted to it. I think it can keep compatibility and possible to indicate link error same as before which was conserned by Pierre-Louis. Please let me know if it still not good match.
One of my big consern is Amadeusz/Jerome's idea which uses "XXX | YYY" style. To avoid confusion, let's name each style as below here.
[Flag style] unsigned int flags; #define SND_SOC_FLAGS_ASSERTION_PLAYBACK BIT(x) #define SND_SOC_FLAGS_ASSERTION_CAPTURE BIT(x) ...
[BitField style] unsigned int playback_assertion:1; unsigned int capture_assertion:1; ...
This v2.5 patch is using [BitField style] and I think it is not a big problem from "code point of view", but I think [Flag style] is better for "human understandable point of view", because we can define like below, for example.
#define SND_SOC_FLAGS_AVAILABLE_PLAYBACK /* no flag is needed */ #define SND_SOC_FLAGS_AVAILABLE_PLAYBACK_WITH_ASSERTION SND_SOC_FLAGS_ASSERTION_PLAYBACK #define SND_SOC_FLAGS_AVAILABLE_CAPTURE /* no flag is needed */ #define SND_SOC_FLAGS_AVAILABLE_CAPTURE_WITH_ASSERTION SND_SOC_FLAGS_ASSERTION_CAPTURE #define SND_SOC_FLAGS_AVAILABLE_BIDIRECTIONAL /* no flag is needed */ #define SND_SOC_FLAGS_AVAILABLE_BIDIRECTIONAL_WITH_ASSERTION (SND_SOC_FLAGS_ASSERTION_PLAYBACK | SND_SOC_FLAGS_ASSERTION_CAPTURE) #define SND_SOC_FLAGS_AVAILABLE_PLAYBACK_ONLY SND_SOC_FLAGS_ASSERTION_PLAYBACK #define SND_SOC_FLAGS_AVAILABLE_CAPTURE_ONLY SND_SOC_FLAGS_ASSERTION_CAPTURE
Switch to [Flag style] is OK for me, but one consern is that in such case, people will wonder "why ASoC is using both [Flag style] and [BitField style] in the same time ?", because we are using [BitField style] for other flags.
So, my suggestion is that next v3 patch uses [Flag style]. And after that, post new patch-set to switch [BitField style] to [Flag style] for other flags. But I wonder is this good approach ?
v2 -> v2.5
- use xxx_assertion flag
- dpcm_xxx -> xxx_assertion
- xxx_only -> xxx_assertion
- only [01/xx] patch
v1 -> v2
- based on latest ASoC branch
- keep comment on Intel
- tidyup patch title
- tidyup DPCM BE warning output condition
- Add new patch for Document
Link: https://lore.kernel.org/r/87o7b353of.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/87zfuesz8y.wl-kuninori.morimoto.gx@renesas.com
Thank you for your help !!
Best regards
Renesas Electronics Ph.D. Kuninori Morimoto
Hi,
I've looked a bit at why the original flags were introduced.
For dpcm_playback and dpcm_capture there is https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... it feels like it works around the fact that some DAIs didn't had set channels_min properly, which will of course not work with snd_soc_dai_stream_valid(). Perhaps if we want to remove this flag we should just set channels_min everywhere where we want to have playback or capture and just remove flag?
Similar story with playback_only and capture_only, which was introduced in two patch series to remove unsupported FEs from list of available ones: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... I feel like if driver author knows that one of directions should be unused they should just set min_channels to 0 to disable it on either BE or FE DAI (personally I would prefer FE DAI in most cases).
Following above we would have no need for any flags at all. What do you think?
Thanks, Amadeusz