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@xxxxxxxxxxx
Link: https://lore.kernel.org/r/87zfuesz8y.wl-kuninori.morimoto.gx@xxxxxxxxxxx
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/?id=1e9de42f4324b91ce2e9da0939cab8fc6ae93893
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/?id=d6bead020d8f8bcaca5cdcb035250c44b21c93e7
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a90e6053baa61feed8b19a9f4cbec6b56479d1ba
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
