| Message ID | 87zfuesz8y.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: Replace dpcm_playback/capture to playback/capture_only | expand |
On Mon 01 Apr 2024 at 00:27, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > Hi Mark > > This is v2 patch-set > > When we use DPCM, we need to set dpcm_playback/capture flag. > If these flag are set, soc_get_playback_capture() will check its > availability, but non DPCM doesn't need such special flags. > > OTOH, it cares playback/capture_only flag. It is needed. > > This patch remove DPCM special flag, and replace it playback/capture_only > flag if needed. Hi Kuninori-san, Thanks for taking the time to clean the dpcm flags. While at it, I wonder if it would be worth taking it a step further. playback_only and capture_only have implication on each other. If one is set, the other can/must not be set. This leads to conditions which can be fairly hard to read and possibly bugs. I had to re-read the meson patch a few times to make sure it still had the same meaning, TBH Wouldn't it be better to replace those 2 flags with a single bitfield ? something like: unsigned int directions; #define PLAYBACK_VALID BIT(0) #define CAPTURE_VALID BIT(1) ... or something similar. I think conditions on the enabled stream would become much clearer like this. The only invalid configuation would be '!directions', which again is easier to read, instead of checking if both flags are set. It would be easy to keep playback_only/capture_only tests, where necessary, with an helper function. What do you think ? Sorry if it is a bit late to discuss this. > > 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 > > Kuninori Morimoto (16): > ASoC: soc-pcm.c: cleanup soc_get_playback_capture() > ASoC: amd: Replace dpcm_playback/capture to playback/capture_only > ASoC: fsl: Replace dpcm_playback/capture to playback/capture_only > ASoC: sof: Replace dpcm_playback/capture to playback/capture_only > ASoC: meson: Replace dpcm_playback/capture to playback/capture_only > ASoC: Intel: Replace dpcm_playback/capture to playback/capture_only > ASoC: samsung: Replace dpcm_playback/capture to playback/capture_only > ASoC: mediatek: Replace dpcm_playback/capture to playback/capture_only > ASoC: soc-core: Replace dpcm_playback/capture to playback/capture_only > ASoC: soc-topology: Replace dpcm_playback/capture to > playback/capture_only > ASoC: soc-compress: Replace dpcm_playback/capture to > playback/capture_only > ASoC: Intel: avs: boards: Replace dpcm_playback/capture to > playback/capture_only > ASoC: remove snd_soc_dai_link_set_capabilities() > ASoC: soc-pcm: remove dpcm_playback/capture > ASoC: soc-pcm: indicate warning if DPCM BE Codec has no settings > ASoC: doc: remove .dpcm_playback/capture flags > > Documentation/sound/soc/dpcm.rst | 14 ++- > include/sound/soc-dai.h | 1 - > include/sound/soc.h | 4 - > sound/soc/amd/acp-da7219-max98357a.c | 20 ++--- > sound/soc/amd/acp-es8336.c | 2 - > sound/soc/amd/acp/acp-mach-common.c | 24 ++--- > sound/soc/amd/acp3x-rt5682-max9836.c | 6 +- > sound/soc/amd/vangogh/acp5x-mach.c | 6 -- > sound/soc/fsl/fsl-asoc-card.c | 16 ++-- > sound/soc/fsl/imx-audmix.c | 6 +- > sound/soc/fsl/imx-card.c | 7 +- > sound/soc/generic/audio-graph-card.c | 2 - > sound/soc/generic/audio-graph-card2.c | 2 - > sound/soc/generic/simple-card.c | 2 - > sound/soc/intel/avs/boards/da7219.c | 2 - > sound/soc/intel/avs/boards/dmic.c | 4 +- > sound/soc/intel/avs/boards/es8336.c | 2 - > sound/soc/intel/avs/boards/hdaudio.c | 4 - > sound/soc/intel/avs/boards/i2s_test.c | 2 - > sound/soc/intel/avs/boards/max98357a.c | 2 +- > sound/soc/intel/avs/boards/max98373.c | 2 - > sound/soc/intel/avs/boards/max98927.c | 2 - > sound/soc/intel/avs/boards/nau8825.c | 2 - > sound/soc/intel/avs/boards/rt274.c | 2 - > sound/soc/intel/avs/boards/rt286.c | 2 - > sound/soc/intel/avs/boards/rt298.c | 2 - > sound/soc/intel/avs/boards/rt5514.c | 2 +- > sound/soc/intel/avs/boards/rt5663.c | 2 - > sound/soc/intel/avs/boards/rt5682.c | 2 - > sound/soc/intel/avs/boards/ssm4567.c | 2 - > sound/soc/intel/boards/bdw-rt5650.c | 4 - > sound/soc/intel/boards/bdw-rt5677.c | 4 - > sound/soc/intel/boards/bdw_rt286.c | 10 +-- > sound/soc/intel/boards/bxt_da7219_max98357a.c | 32 ++++--- > sound/soc/intel/boards/bxt_rt298.c | 26 +++--- > sound/soc/intel/boards/bytcht_cx2072x.c | 6 +- > sound/soc/intel/boards/bytcht_da7213.c | 6 +- > sound/soc/intel/boards/bytcht_es8316.c | 6 +- > sound/soc/intel/boards/bytcht_nocodec.c | 6 +- > sound/soc/intel/boards/bytcr_rt5640.c | 6 +- > sound/soc/intel/boards/bytcr_rt5651.c | 6 +- > sound/soc/intel/boards/bytcr_wm5102.c | 6 +- > sound/soc/intel/boards/cht_bsw_max98090_ti.c | 6 +- > sound/soc/intel/boards/cht_bsw_nau8824.c | 6 +- > sound/soc/intel/boards/cht_bsw_rt5645.c | 6 +- > sound/soc/intel/boards/cht_bsw_rt5672.c | 6 +- > sound/soc/intel/boards/cml_rt1011_rt5682.c | 15 ++-- > sound/soc/intel/boards/ehl_rt5660.c | 14 ++- > sound/soc/intel/boards/glk_rt5682_max98357a.c | 30 +++---- > sound/soc/intel/boards/hsw_rt5640.c | 10 +-- > sound/soc/intel/boards/kbl_da7219_max98357a.c | 26 +++--- > sound/soc/intel/boards/kbl_da7219_max98927.c | 54 +++++------- > sound/soc/intel/boards/kbl_rt5660.c | 18 ++-- > sound/soc/intel/boards/kbl_rt5663_max98927.c | 44 +++++----- > .../intel/boards/kbl_rt5663_rt5514_max98927.c | 22 ++--- > sound/soc/intel/boards/skl_hda_dsp_common.c | 14 ++- > .../soc/intel/boards/skl_nau88l25_max98357a.c | 26 +++--- > sound/soc/intel/boards/skl_nau88l25_ssm4567.c | 26 +++--- > sound/soc/intel/boards/skl_rt286.c | 26 +++--- > sound/soc/intel/boards/sof_board_helpers.c | 13 +-- > sound/soc/intel/boards/sof_es8336.c | 8 +- > sound/soc/intel/boards/sof_pcm512x.c | 8 +- > sound/soc/intel/boards/sof_sdw.c | 4 +- > sound/soc/intel/boards/sof_wm8804.c | 2 - > sound/soc/mediatek/mt2701/mt2701-cs42448.c | 20 ++--- > sound/soc/mediatek/mt2701/mt2701-wm8960.c | 6 +- > sound/soc/mediatek/mt6797/mt6797-mt6351.c | 24 ++--- > sound/soc/mediatek/mt7986/mt7986-wm8960.c | 6 +- > sound/soc/mediatek/mt8173/mt8173-max98090.c | 6 +- > .../mediatek/mt8173/mt8173-rt5650-rt5514.c | 6 +- > .../mediatek/mt8173/mt8173-rt5650-rt5676.c | 10 +-- > sound/soc/mediatek/mt8173/mt8173-rt5650.c | 10 +-- > .../mediatek/mt8183/mt8183-da7219-max98357.c | 34 +++----- > .../mt8183/mt8183-mt6358-ts3a227-max98357.c | 34 +++----- > .../mt8186/mt8186-mt6366-da7219-max98357.c | 86 +++++++----------- > .../mt8186/mt8186-mt6366-rt1019-rt5682s.c | 86 +++++++----------- > sound/soc/mediatek/mt8188/mt8188-mt6359.c | 58 ++++++------- > .../mt8192/mt8192-mt6359-rt1015-rt5682.c | 78 ++++++++--------- > sound/soc/mediatek/mt8195/mt8195-mt6359.c | 60 +++++++------ > sound/soc/meson/axg-card.c | 9 +- > sound/soc/meson/gx-card.c | 1 - > sound/soc/meson/meson-card-utils.c | 4 +- > sound/soc/qcom/common.c | 1 - > sound/soc/samsung/odroid.c | 11 ++- > sound/soc/soc-compress.c | 10 ++- > sound/soc/soc-core.c | 20 +---- > sound/soc/soc-dai.c | 38 -------- > sound/soc/soc-pcm.c | 87 ++++++++----------- > sound/soc/soc-topology-test.c | 2 - > sound/soc/soc-topology.c | 4 +- > sound/soc/sof/nocodec.c | 4 - > 91 files changed, 502 insertions(+), 863 deletions(-)
Hi Jerome Thank you for your feedback > playback_only and capture_only have implication on each other. If one is > set, the other can/must not be set. This leads to conditions which can > be fairly hard to read and possibly bugs. (snip) > Wouldn't it be better to replace those 2 flags with a single bitfield ? > > something like: > > unsigned int directions; > #define PLAYBACK_VALID BIT(0) > #define CAPTURE_VALID BIT(1) I think Amadeusz indicated similar idea, and basically I can agree about it. But in this patch-set, I want focus to removing dpcm_xxx flag as 1st step. So I'm happy to create such patch-set, but I want to handle it as another patch-set. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On Thu 04 Apr 2024 at 23:13, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > Hi Jerome > > Thank you for your feedback > >> playback_only and capture_only have implication on each other. If one is >> set, the other can/must not be set. This leads to conditions which can >> be fairly hard to read and possibly bugs. > (snip) >> Wouldn't it be better to replace those 2 flags with a single bitfield ? >> >> something like: >> >> unsigned int directions; >> #define PLAYBACK_VALID BIT(0) >> #define CAPTURE_VALID BIT(1) > > I think Amadeusz indicated similar idea, and basically I can agree about > it. I've seen it afterward. It is similar indeed but I don't think 'None' or 'Both' should have a dedicated bit. That would be yet another redundance/implication between flags/bits ... so another source of bugs/complexity IMO. > But in this patch-set, I want focus to removing dpcm_xxx flag as 1st > step. So I'm happy to create such patch-set, but I want to handle it as > another patch-set. Fine by me ... at least for the Amlogic part. > > Thank you for your help !! > Thanks for your work ! > Best regards > --- > Renesas Electronics > Ph.D. Kuninori Morimoto
Hi Jerome > >> unsigned int directions; > >> #define PLAYBACK_VALID BIT(0) > >> #define CAPTURE_VALID BIT(1) > > > > I think Amadeusz indicated similar idea, and basically I can agree about > > it. > > I've seen it afterward. It is similar indeed but I don't think 'None' or > 'Both' should have a dedicated bit. That would be yet another > redundance/implication between flags/bits ... so another source of > bugs/complexity IMO. I noticed that it is alreay using bitfield. The diff is it is possible to use "XXX | YYY" style or not. /* For unidirectional dai links */ unsigned int playback_only:1; unsigned int capture_only:1; ... /* DPCM capture and Playback support */ unsigned int dpcm_capture:1; unsigned int dpcm_playback:1; Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Mark This is v2 patch-set When we use DPCM, we need to set dpcm_playback/capture flag. If these flag are set, soc_get_playback_capture() will check its availability, but non DPCM doesn't need such special flags. OTOH, it cares playback/capture_only flag. It is needed. This patch remove DPCM special flag, and replace it playback/capture_only flag if needed. 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 Kuninori Morimoto (16): ASoC: soc-pcm.c: cleanup soc_get_playback_capture() ASoC: amd: Replace dpcm_playback/capture to playback/capture_only ASoC: fsl: Replace dpcm_playback/capture to playback/capture_only ASoC: sof: Replace dpcm_playback/capture to playback/capture_only ASoC: meson: Replace dpcm_playback/capture to playback/capture_only ASoC: Intel: Replace dpcm_playback/capture to playback/capture_only ASoC: samsung: Replace dpcm_playback/capture to playback/capture_only ASoC: mediatek: Replace dpcm_playback/capture to playback/capture_only ASoC: soc-core: Replace dpcm_playback/capture to playback/capture_only ASoC: soc-topology: Replace dpcm_playback/capture to playback/capture_only ASoC: soc-compress: Replace dpcm_playback/capture to playback/capture_only ASoC: Intel: avs: boards: Replace dpcm_playback/capture to playback/capture_only ASoC: remove snd_soc_dai_link_set_capabilities() ASoC: soc-pcm: remove dpcm_playback/capture ASoC: soc-pcm: indicate warning if DPCM BE Codec has no settings ASoC: doc: remove .dpcm_playback/capture flags Documentation/sound/soc/dpcm.rst | 14 ++- include/sound/soc-dai.h | 1 - include/sound/soc.h | 4 - sound/soc/amd/acp-da7219-max98357a.c | 20 ++--- sound/soc/amd/acp-es8336.c | 2 - sound/soc/amd/acp/acp-mach-common.c | 24 ++--- sound/soc/amd/acp3x-rt5682-max9836.c | 6 +- sound/soc/amd/vangogh/acp5x-mach.c | 6 -- sound/soc/fsl/fsl-asoc-card.c | 16 ++-- sound/soc/fsl/imx-audmix.c | 6 +- sound/soc/fsl/imx-card.c | 7 +- sound/soc/generic/audio-graph-card.c | 2 - sound/soc/generic/audio-graph-card2.c | 2 - sound/soc/generic/simple-card.c | 2 - sound/soc/intel/avs/boards/da7219.c | 2 - sound/soc/intel/avs/boards/dmic.c | 4 +- sound/soc/intel/avs/boards/es8336.c | 2 - sound/soc/intel/avs/boards/hdaudio.c | 4 - sound/soc/intel/avs/boards/i2s_test.c | 2 - sound/soc/intel/avs/boards/max98357a.c | 2 +- sound/soc/intel/avs/boards/max98373.c | 2 - sound/soc/intel/avs/boards/max98927.c | 2 - sound/soc/intel/avs/boards/nau8825.c | 2 - sound/soc/intel/avs/boards/rt274.c | 2 - sound/soc/intel/avs/boards/rt286.c | 2 - sound/soc/intel/avs/boards/rt298.c | 2 - sound/soc/intel/avs/boards/rt5514.c | 2 +- sound/soc/intel/avs/boards/rt5663.c | 2 - sound/soc/intel/avs/boards/rt5682.c | 2 - sound/soc/intel/avs/boards/ssm4567.c | 2 - sound/soc/intel/boards/bdw-rt5650.c | 4 - sound/soc/intel/boards/bdw-rt5677.c | 4 - sound/soc/intel/boards/bdw_rt286.c | 10 +-- sound/soc/intel/boards/bxt_da7219_max98357a.c | 32 ++++--- sound/soc/intel/boards/bxt_rt298.c | 26 +++--- sound/soc/intel/boards/bytcht_cx2072x.c | 6 +- sound/soc/intel/boards/bytcht_da7213.c | 6 +- sound/soc/intel/boards/bytcht_es8316.c | 6 +- sound/soc/intel/boards/bytcht_nocodec.c | 6 +- sound/soc/intel/boards/bytcr_rt5640.c | 6 +- sound/soc/intel/boards/bytcr_rt5651.c | 6 +- sound/soc/intel/boards/bytcr_wm5102.c | 6 +- sound/soc/intel/boards/cht_bsw_max98090_ti.c | 6 +- sound/soc/intel/boards/cht_bsw_nau8824.c | 6 +- sound/soc/intel/boards/cht_bsw_rt5645.c | 6 +- sound/soc/intel/boards/cht_bsw_rt5672.c | 6 +- sound/soc/intel/boards/cml_rt1011_rt5682.c | 15 ++-- sound/soc/intel/boards/ehl_rt5660.c | 14 ++- sound/soc/intel/boards/glk_rt5682_max98357a.c | 30 +++---- sound/soc/intel/boards/hsw_rt5640.c | 10 +-- sound/soc/intel/boards/kbl_da7219_max98357a.c | 26 +++--- sound/soc/intel/boards/kbl_da7219_max98927.c | 54 +++++------- sound/soc/intel/boards/kbl_rt5660.c | 18 ++-- sound/soc/intel/boards/kbl_rt5663_max98927.c | 44 +++++----- .../intel/boards/kbl_rt5663_rt5514_max98927.c | 22 ++--- sound/soc/intel/boards/skl_hda_dsp_common.c | 14 ++- .../soc/intel/boards/skl_nau88l25_max98357a.c | 26 +++--- sound/soc/intel/boards/skl_nau88l25_ssm4567.c | 26 +++--- sound/soc/intel/boards/skl_rt286.c | 26 +++--- sound/soc/intel/boards/sof_board_helpers.c | 13 +-- sound/soc/intel/boards/sof_es8336.c | 8 +- sound/soc/intel/boards/sof_pcm512x.c | 8 +- sound/soc/intel/boards/sof_sdw.c | 4 +- sound/soc/intel/boards/sof_wm8804.c | 2 - sound/soc/mediatek/mt2701/mt2701-cs42448.c | 20 ++--- sound/soc/mediatek/mt2701/mt2701-wm8960.c | 6 +- sound/soc/mediatek/mt6797/mt6797-mt6351.c | 24 ++--- sound/soc/mediatek/mt7986/mt7986-wm8960.c | 6 +- sound/soc/mediatek/mt8173/mt8173-max98090.c | 6 +- .../mediatek/mt8173/mt8173-rt5650-rt5514.c | 6 +- .../mediatek/mt8173/mt8173-rt5650-rt5676.c | 10 +-- sound/soc/mediatek/mt8173/mt8173-rt5650.c | 10 +-- .../mediatek/mt8183/mt8183-da7219-max98357.c | 34 +++----- .../mt8183/mt8183-mt6358-ts3a227-max98357.c | 34 +++----- .../mt8186/mt8186-mt6366-da7219-max98357.c | 86 +++++++----------- .../mt8186/mt8186-mt6366-rt1019-rt5682s.c | 86 +++++++----------- sound/soc/mediatek/mt8188/mt8188-mt6359.c | 58 ++++++------- .../mt8192/mt8192-mt6359-rt1015-rt5682.c | 78 ++++++++--------- sound/soc/mediatek/mt8195/mt8195-mt6359.c | 60 +++++++------ sound/soc/meson/axg-card.c | 9 +- sound/soc/meson/gx-card.c | 1 - sound/soc/meson/meson-card-utils.c | 4 +- sound/soc/qcom/common.c | 1 - sound/soc/samsung/odroid.c | 11 ++- sound/soc/soc-compress.c | 10 ++- sound/soc/soc-core.c | 20 +---- sound/soc/soc-dai.c | 38 -------- sound/soc/soc-pcm.c | 87 ++++++++----------- sound/soc/soc-topology-test.c | 2 - sound/soc/soc-topology.c | 4 +- sound/soc/sof/nocodec.c | 4 - 91 files changed, 502 insertions(+), 863 deletions(-)