| Message ID | 20230523021933.3422-3-trevor.wu@mediatek.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: mt8188: add new board support | expand |
On 24/05/2023 04:25, Trevor Wu (吳文良) wrote: > On Tue, 2023-05-23 at 18:26 +0200, Alexandre Mergnat wrote: >> On 23/05/2023 04:19, Trevor Wu wrote: >>> ADDA_BE is used to connect to mt6359. For machine mt8188-mt6359, >>> codec >>> for ADDA_BE must be mt6359 which are configured on the machine >>> driver. >>> Besides, ADDA_BE is divided into two dais, UL_SRC_BE and DL_SRC_BE. >>> As a result, remove ADDA_BE from items of link-name. >>> >>> Signed-off-by: Trevor Wu<trevor.wu@mediatek.com> >> >> I don't understand how "DL_SRC_BE" and "UL_SRC_BE" links are done. >> Why these dais don't replace "ADDA_BE" in this binding ? >> >> Regards, >> Alexandre >> > > Hi Alexandre, > > Because the sound card is mt8188-mt6359, the codec for these two links > must be mt6359. Thus, I specifiy the codec in machine driver directly. > If the codec is changed, there will be a new sound card and binding > file. In conclusion, the codec won't be updated via dts, and that's why > I don't just replace ADDA_BE in this binding. > > Do you suggest me add some information in the commit message? No it's fine, I'm just trying to understand. When you say "I specifiy the codec in machine driver directly", you are talking about this change ? + } else if (strcmp(dai_link->name, "DL_SRC_BE") == 0 || + strcmp(dai_link->name, "UL_SRC_BE") == 0) { + if (!init_mt6359) { + dai_link->init = mt8188_mt6359_init; I'm asking because the equivalent was done here: - [DAI_LINK_ADDA_BE] = { - .name = "ADDA_BE", + [DAI_LINK_DL_SRC_BE] = { + .name = "DL_SRC_BE", .no_pcm = 1, .dpcm_playback = 1, - .dpcm_capture = 1, - .init = mt8188_mt6359_init, - SND_SOC_DAILINK_REG(adda), + SND_SOC_DAILINK_REG(dl_src), So I'm wondering why "ADDA_BE" & "DPTX_BE" & "ETDM3_OUT_BE" are in the enum list of the binding since the codec is already specified in machine driver too. I probably miss something but I don't know what.
On Wed, 2023-05-24 at 15:28 +0200, Alexandre Mergnat wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > On 24/05/2023 04:25, Trevor Wu (吳文良) wrote: > > On Tue, 2023-05-23 at 18:26 +0200, Alexandre Mergnat wrote: > > > On 23/05/2023 04:19, Trevor Wu wrote: > > > > ADDA_BE is used to connect to mt6359. For machine mt8188- > > > > mt6359, > > > > codec > > > > for ADDA_BE must be mt6359 which are configured on the machine > > > > driver. > > > > Besides, ADDA_BE is divided into two dais, UL_SRC_BE and > > > > DL_SRC_BE. > > > > As a result, remove ADDA_BE from items of link-name. > > > > > > > > Signed-off-by: Trevor Wu<trevor.wu@mediatek.com> > > > > > > I don't understand how "DL_SRC_BE" and "UL_SRC_BE" links are > > > done. > > > Why these dais don't replace "ADDA_BE" in this binding ? > > > > > > Regards, > > > Alexandre > > > > > > > Hi Alexandre, > > > > Because the sound card is mt8188-mt6359, the codec for these two > > links > > must be mt6359. Thus, I specifiy the codec in machine driver > > directly. > > If the codec is changed, there will be a new sound card and binding > > file. In conclusion, the codec won't be updated via dts, and that's > > why > > I don't just replace ADDA_BE in this binding. > > > > Do you suggest me add some information in the commit message? > > No it's fine, I'm just trying to understand. > > When you say "I specifiy the codec in machine driver directly", you > are talking about this change ? > > + } else if (strcmp(dai_link->name, "DL_SRC_BE") == 0 > || > + strcmp(dai_link->name, "UL_SRC_BE") == 0) > { > + if (!init_mt6359) { > + dai_link->init = mt8188_mt6359_init; > > I'm asking because the equivalent was done here: > > - [DAI_LINK_ADDA_BE] = { > - .name = "ADDA_BE", > + [DAI_LINK_DL_SRC_BE] = { > + .name = "DL_SRC_BE", > .no_pcm = 1, > .dpcm_playback = 1, > - .dpcm_capture = 1, > - .init = mt8188_mt6359_init, > - SND_SOC_DAILINK_REG(adda), > + SND_SOC_DAILINK_REG(dl_src), > > So I'm wondering why "ADDA_BE" & "DPTX_BE" & "ETDM3_OUT_BE" are in > the > enum list of the binding since the codec is already specified in > machine driver too. I probably miss something but I don't know what. > > The following code snippet is cut from [PATCH v2 1/7]. /* BE */ -SND_SOC_DAILINK_DEFS(adda, - DAILINK_COMP_ARRAY(COMP_CPU("ADDA")), +SND_SOC_DAILINK_DEFS(dl_src, + DAILINK_COMP_ARRAY(COMP_CPU("DL_SRC")), DAILINK_COMP_ARRAY(COMP_CODEC("mt6359-sound", "mt6359-snd-codec- aif1")), DAILINK_COMP_ARRAY(COMP_EMPTY())); @@ -140,6 +140,12 @@ SND_SOC_DAILINK_DEFS(pcm1, DAILINK_COMP_ARRAY(COMP_DUMMY()), DAILINK_COMP_ARRAY(COMP_EMPTY())); +SND_SOC_DAILINK_DEFS(ul_src, + DAILINK_COMP_ARRAY(COMP_CPU("UL_SRC")), + DAILINK_COMP_ARRAY(COMP_CODEC("mt6359-sound", + "mt6359-snd-codec- aif1")), + DAILINK_COMP_ARRAY(COMP_EMPTY())); This is why I talk about specifying the codec it connects in the machine driver. If you check other dai-links, you would see COMP_DUMMY() in the COMP_CODEC() field. Thanks, Trevor >
On 24/05/2023 15:45, Trevor Wu (吳文良) wrote: > On Wed, 2023-05-24 at 15:28 +0200, Alexandre Mergnat wrote: >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> >> >> On 24/05/2023 04:25, Trevor Wu (吳文良) wrote: >>> On Tue, 2023-05-23 at 18:26 +0200, Alexandre Mergnat wrote: >>>> On 23/05/2023 04:19, Trevor Wu wrote: >>>>> ADDA_BE is used to connect to mt6359. For machine mt8188- >>>>> mt6359, >>>>> codec >>>>> for ADDA_BE must be mt6359 which are configured on the machine >>>>> driver. >>>>> Besides, ADDA_BE is divided into two dais, UL_SRC_BE and >>>>> DL_SRC_BE. >>>>> As a result, remove ADDA_BE from items of link-name. >>>>> >>>>> Signed-off-by: Trevor Wu<trevor.wu@mediatek.com> >>>> >>>> I don't understand how "DL_SRC_BE" and "UL_SRC_BE" links are >>>> done. >>>> Why these dais don't replace "ADDA_BE" in this binding ? >>>> >>>> Regards, >>>> Alexandre >>>> >>> >>> Hi Alexandre, >>> >>> Because the sound card is mt8188-mt6359, the codec for these two >>> links >>> must be mt6359. Thus, I specifiy the codec in machine driver >>> directly. >>> If the codec is changed, there will be a new sound card and binding >>> file. In conclusion, the codec won't be updated via dts, and that's >>> why >>> I don't just replace ADDA_BE in this binding. >>> >>> Do you suggest me add some information in the commit message? >> >> No it's fine, I'm just trying to understand. >> >> When you say "I specifiy the codec in machine driver directly", you >> are talking about this change ? >> >> + } else if (strcmp(dai_link->name, "DL_SRC_BE") == 0 >> || >> + strcmp(dai_link->name, "UL_SRC_BE") == 0) >> { >> + if (!init_mt6359) { >> + dai_link->init = mt8188_mt6359_init; >> >> I'm asking because the equivalent was done here: >> >> - [DAI_LINK_ADDA_BE] = { >> - .name = "ADDA_BE", >> + [DAI_LINK_DL_SRC_BE] = { >> + .name = "DL_SRC_BE", >> .no_pcm = 1, >> .dpcm_playback = 1, >> - .dpcm_capture = 1, >> - .init = mt8188_mt6359_init, >> - SND_SOC_DAILINK_REG(adda), >> + SND_SOC_DAILINK_REG(dl_src), >> >> So I'm wondering why "ADDA_BE" & "DPTX_BE" & "ETDM3_OUT_BE" are in >> the >> enum list of the binding since the codec is already specified in >> machine driver too. I probably miss something but I don't know what. >> >> > > > The following code snippet is cut from [PATCH v2 1/7]. > > /* BE */ > -SND_SOC_DAILINK_DEFS(adda, > - DAILINK_COMP_ARRAY(COMP_CPU("ADDA")), > +SND_SOC_DAILINK_DEFS(dl_src, > + DAILINK_COMP_ARRAY(COMP_CPU("DL_SRC")), > DAILINK_COMP_ARRAY(COMP_CODEC("mt6359-sound", > "mt6359-snd-codec- > aif1")), > DAILINK_COMP_ARRAY(COMP_EMPTY())); > @@ -140,6 +140,12 @@ SND_SOC_DAILINK_DEFS(pcm1, > DAILINK_COMP_ARRAY(COMP_DUMMY()), > DAILINK_COMP_ARRAY(COMP_EMPTY())); > > +SND_SOC_DAILINK_DEFS(ul_src, > + DAILINK_COMP_ARRAY(COMP_CPU("UL_SRC")), > + DAILINK_COMP_ARRAY(COMP_CODEC("mt6359-sound", > + "mt6359-snd-codec- > aif1")), > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > > > This is why I talk about specifying the codec it connects in the > machine driver. > If you check other dai-links, you would see COMP_DUMMY() in the > COMP_CODEC() field. Ok thanks for the explanation. If I understand well, ADDA_BE could have been removed from the enum list before your serie because the codec was already specified for ADDA_BE. Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
On Wed, 2023-05-24 at 16:46 +0200, Alexandre Mergnat wrote: > On 24/05/2023 15:45, Trevor Wu (吳文良) wrote: > > On Wed, 2023-05-24 at 15:28 +0200, Alexandre Mergnat wrote: > > > External email : Please do not click links or open attachments > > > until > > > you have verified the sender or the content. > > > > > > > > > On 24/05/2023 04:25, Trevor Wu (吳文良) wrote: > > > > On Tue, 2023-05-23 at 18:26 +0200, Alexandre Mergnat wrote: > > > > > On 23/05/2023 04:19, Trevor Wu wrote: > > > > > > ADDA_BE is used to connect to mt6359. For machine mt8188- > > > > > > mt6359, > > > > > > codec > > > > > > for ADDA_BE must be mt6359 which are configured on the > > > > > > machine > > > > > > driver. > > > > > > Besides, ADDA_BE is divided into two dais, UL_SRC_BE and > > > > > > DL_SRC_BE. > > > > > > As a result, remove ADDA_BE from items of link-name. > > > > > > > > > > > > Signed-off-by: Trevor Wu<trevor.wu@mediatek.com> > > > > > > > > > > I don't understand how "DL_SRC_BE" and "UL_SRC_BE" links are > > > > > done. > > > > > Why these dais don't replace "ADDA_BE" in this binding ? > > > > > > > > > > Regards, > > > > > Alexandre > > > > > > > > > > > > > Hi Alexandre, > > > > > > > > Because the sound card is mt8188-mt6359, the codec for these > > > > two > > > > links > > > > must be mt6359. Thus, I specifiy the codec in machine driver > > > > directly. > > > > If the codec is changed, there will be a new sound card and > > > > binding > > > > file. In conclusion, the codec won't be updated via dts, and > > > > that's > > > > why > > > > I don't just replace ADDA_BE in this binding. > > > > > > > > Do you suggest me add some information in the commit message? > > > > > > No it's fine, I'm just trying to understand. > > > > > > When you say "I specifiy the codec in machine driver directly", > > > you > > > are talking about this change ? > > > > > > + } else if (strcmp(dai_link->name, "DL_SRC_BE") == > > > 0 > > > > > > > > > > > + strcmp(dai_link->name, "UL_SRC_BE") == > > > 0) > > > { > > > + if (!init_mt6359) { > > > + dai_link->init = > > > mt8188_mt6359_init; > > > > > > I'm asking because the equivalent was done here: > > > > > > - [DAI_LINK_ADDA_BE] = { > > > - .name = "ADDA_BE", > > > + [DAI_LINK_DL_SRC_BE] = { > > > + .name = "DL_SRC_BE", > > > .no_pcm = 1, > > > .dpcm_playback = 1, > > > - .dpcm_capture = 1, > > > - .init = mt8188_mt6359_init, > > > - SND_SOC_DAILINK_REG(adda), > > > + SND_SOC_DAILINK_REG(dl_src), > > > > > > So I'm wondering why "ADDA_BE" & "DPTX_BE" & "ETDM3_OUT_BE" are > > > in > > > the > > > enum list of the binding since the codec is already specified in > > > machine driver too. I probably miss something but I don't know > > > what. > > > > > > > > > > > > The following code snippet is cut from [PATCH v2 1/7]. > > > > /* BE */ > > -SND_SOC_DAILINK_DEFS(adda, > > - DAILINK_COMP_ARRAY(COMP_CPU("ADDA")), > > +SND_SOC_DAILINK_DEFS(dl_src, > > + DAILINK_COMP_ARRAY(COMP_CPU("DL_SRC")), > > DAILINK_COMP_ARRAY(COMP_CODEC("mt6359-sound", > > "mt6359-snd- > > codec- > > aif1")), > > DAILINK_COMP_ARRAY(COMP_EMPTY())); > > @@ -140,6 +140,12 @@ SND_SOC_DAILINK_DEFS(pcm1, > > DAILINK_COMP_ARRAY(COMP_DUMMY()), > > DAILINK_COMP_ARRAY(COMP_EMPTY())); > > > > +SND_SOC_DAILINK_DEFS(ul_src, > > + DAILINK_COMP_ARRAY(COMP_CPU("UL_SRC")), > > + DAILINK_COMP_ARRAY(COMP_CODEC("mt6359-sound", > > + "mt6359-snd- > > codec- > > aif1")), > > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > > > > > > This is why I talk about specifying the codec it connects in the > > machine driver. > > If you check other dai-links, you would see COMP_DUMMY() in the > > COMP_CODEC() field. > > Ok thanks for the explanation. If I understand well, ADDA_BE could > have > been removed from the enum list before your serie because the codec > was > already specified for ADDA_BE. > > Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com> > Actually, it's allowed to be overwritten by DTS configuration. That's why I listed ADDA_BE in the enum list before. However, I found the binding and machine driver was implemented for mt6359, so I think it's more reasonable to remove it from the list. Thanks, Trevor
diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml index 6640272b3f4f..3d2c01b693be 100644 --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml @@ -42,7 +42,6 @@ patternProperties: we are going to update parameters in this node. items: enum: - - ADDA_BE - DPTX_BE - ETDM1_IN_BE - ETDM2_IN_BE
ADDA_BE is used to connect to mt6359. For machine mt8188-mt6359, codec for ADDA_BE must be mt6359 which are configured on the machine driver. Besides, ADDA_BE is divided into two dais, UL_SRC_BE and DL_SRC_BE. As a result, remove ADDA_BE from items of link-name. Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> --- .../devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml | 1 - 1 file changed, 1 deletion(-)