| Message ID | 20221125094413.4940-2-jiaxin.yu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: mediatek:mt8186: fix both the speaker and hdmi | expand |
On Fri, Nov 25, 2022 at 05:44:11PM +0800, Jiaxin Yu wrote: > + /* > + * PCM trigger callback. > + * Mandatory > + */ > + int (*trigger)(struct device *dev, int cmd); > + Making this mandatory would break all existing users, though... > + switch (event) { > + case SND_SOC_DAPM_PRE_PMU: > + if (hcp->hcd.ops->trigger) > + hcp->hcd.ops->trigger(component->dev->parent, SNDRV_PCM_TRIGGER_START); ...it's not actually mandatory so it's just the comment that's wrong. I'm a little unclear why this is being implemented as a DAPM operation rather than having the driver forward the PCM trigger op if it's needed? Or alternatively if a DAPM callback is needed why not provide one directly rather than hooking into the trigger function - that's going to be called out of sequence with the rest of DAPM and be potentially confusing given the very different environments that trigger and DAPM operations run in. A quick glance at the it6505 driver suggests it'd be happier with a DAPM callback.
On Fri, 2022-11-25 at 12:18 +0000, Mark Brown wrote: > On Fri, Nov 25, 2022 at 05:44:11PM +0800, Jiaxin Yu wrote: > > > + /* > > + * PCM trigger callback. > > + * Mandatory > > + */ > > + int (*trigger)(struct device *dev, int cmd); > > + > > Making this mandatory would break all existing users, though... > Yes, it should be described as optional. > > + switch (event) { > > + case SND_SOC_DAPM_PRE_PMU: > > + if (hcp->hcd.ops->trigger) > > + hcp->hcd.ops->trigger(component->dev->parent, > > SNDRV_PCM_TRIGGER_START); > > ..it's not actually mandatory so it's just the comment that's wrong. Agreed. > I'm a little unclear why this is being implemented as a DAPM > operation > rather than having the driver forward the PCM trigger op if it's > needed? > Or alternatively if a DAPM callback is needed why not provide one > directly rather than hooking into the trigger function - that's going > to > be called out of sequence with the rest of DAPM and be potentially > confusing given the very different environments that trigger and DAPM > operations run in. A quick glance at the it6505 driver suggests it'd > be > happier with a DAPM callback. Let me describe the hardware connection about mt8186 with it6505(hdmi) and rt1015p(speakers). ==>it6505 = DL1(FE) ==>I2S3(BE) = = ==>rt1015p They shared the same one i2s port, but we'd like to control them separately. So if hdmi-codec use the PCM trigger op, whne we turn on the speaker, hdmi-codec's PCM trigger op is also executed, resulting in sound on both devices. Is there another way to control them separately? Thank you.
On Mon, Nov 28, 2022 at 03:07:22PM +0000, Jiaxin Yu (俞家鑫) wrote: > On Fri, 2022-11-25 at 12:18 +0000, Mark Brown wrote: > > On Fri, Nov 25, 2022 at 05:44:11PM +0800, Jiaxin Yu wrote: > > I'm a little unclear why this is being implemented as a DAPM > > operation > > rather than having the driver forward the PCM trigger op if it's > > needed? > > Or alternatively if a DAPM callback is needed why not provide one > > directly rather than hooking into the trigger function - that's going > > to > > be called out of sequence with the rest of DAPM and be potentially > > confusing given the very different environments that trigger and DAPM > > operations run in. A quick glance at the it6505 driver suggests it'd > > be > > happier with a DAPM callback. > Let me describe the hardware connection about mt8186 with it6505(hdmi) > and rt1015p(speakers). > ==>it6505 > = > DL1(FE) ==>I2S3(BE) = > = > ==>rt1015p > They shared the same one i2s port, but we'd like to control them > separately. So if hdmi-codec use the PCM trigger op, whne we turn on > the speaker, hdmi-codec's PCM trigger op is also executed, resulting in > sound on both devices. > Is there another way to control them separately? Thank you. If you just need power control for one or both devices then the machine driver can add a _PIN_SWITCH() on the output of the device, that'll cause DAPM to keep the device powered down when not in use. That should work well with the suggestion to provide a DAPM callback instead of a a trigger operation.
On Tue, 2022-11-29 at 17:22 +0000, Mark Brown wrote: > On Mon, Nov 28, 2022 at 03:07:22PM +0000, Jiaxin Yu (俞家鑫) wrote: > > On Fri, 2022-11-25 at 12:18 +0000, Mark Brown wrote: > > > On Fri, Nov 25, 2022 at 05:44:11PM +0800, Jiaxin Yu wrote: > > > I'm a little unclear why this is being implemented as a DAPM > > > operation > > > rather than having the driver forward the PCM trigger op if it's > > > needed? > > > Or alternatively if a DAPM callback is needed why not provide one > > > directly rather than hooking into the trigger function - that's > > > going > > > to > > > be called out of sequence with the rest of DAPM and be > > > potentially > > > confusing given the very different environments that trigger and > > > DAPM > > > operations run in. A quick glance at the it6505 driver suggests > > > it'd > > > be > > > happier with a DAPM callback. > > Let me describe the hardware connection about mt8186 with > > it6505(hdmi) > > and rt1015p(speakers). > > ==>it6505 > > = > > DL1(FE) ==>I2S3(BE) = > > = > > ==>rt1015p > > They shared the same one i2s port, but we'd like to control them > > separately. So if hdmi-codec use the PCM trigger op, whne we turn > > on > > the speaker, hdmi-codec's PCM trigger op is also executed, > > resulting in > > sound on both devices. > > Is there another way to control them separately? Thank you. > > If you just need power control for one or both devices then the > machine > driver can add a _PIN_SWITCH() on the output of the device, that'll > cause DAPM to keep the device powered down when not in use. That > should > work well with the suggestion to provide a DAPM callback instead of a > a > trigger operation. Yes, we do use a _PIN_SWITCH() on the outout of the device: > static const struct snd_kcontrol_new > mt8186_mt6366_rt1019_rt5682s_controls[] = { > SOC_DAPM_PIN_SWITCH("Speakers"), > SOC_DAPM_PIN_SWITCH("Headphone"), > SOC_DAPM_PIN_SWITCH("Headset Mic"), > SOC_DAPM_PIN_SWITCH("HDMI1"), > }; Which operation should I use to inform bridge driver to control audio on or off? I'm curious why I don't see .trigger in the structure hdmi_codec_ops compared to the structure snd_soc_dai_ops?
On Thu, Dec 01, 2022 at 03:06:04PM +0000, Jiaxin Yu (俞家鑫) wrote: > On Tue, 2022-11-29 at 17:22 +0000, Mark Brown wrote: > > static const struct snd_kcontrol_new > > mt8186_mt6366_rt1019_rt5682s_controls[] = { > > SOC_DAPM_PIN_SWITCH("Speakers"), > > SOC_DAPM_PIN_SWITCH("Headphone"), > > SOC_DAPM_PIN_SWITCH("Headset Mic"), > > SOC_DAPM_PIN_SWITCH("HDMI1"), > > }; > Which operation should I use to inform bridge driver to control audio > on or off? I'm curious why I don't see .trigger in the structure > hdmi_codec_ops compared to the structure snd_soc_dai_ops? You'd need to add a callback that the user of hdmi-codec passes in which would be triggered by an event on a DAPM widget added in the audio path rather than trying to shoehorn this into a PCM operation - a big part of the problem here is that you're trying to add something that doesn't fit into a PCM operation.
On Thu, 2022-12-01 at 15:23 +0000, Mark Brown wrote: > On Thu, Dec 01, 2022 at 03:06:04PM +0000, Jiaxin Yu (俞家鑫) wrote: > > On Tue, 2022-11-29 at 17:22 +0000, Mark Brown wrote: > > > static const struct snd_kcontrol_new > > > mt8186_mt6366_rt1019_rt5682s_controls[] = { > > > SOC_DAPM_PIN_SWITCH("Speakers"), > > > SOC_DAPM_PIN_SWITCH("Headphone"), > > > SOC_DAPM_PIN_SWITCH("Headset Mic"), > > > SOC_DAPM_PIN_SWITCH("HDMI1"), > > > }; > > Which operation should I use to inform bridge driver to control > > audio > > on or off? I'm curious why I don't see .trigger in the structure > > hdmi_codec_ops compared to the structure snd_soc_dai_ops? > > You'd need to add a callback that the user of hdmi-codec passes in > which > would be triggered by an event on a DAPM widget added in the audio > path > rather than trying to shoehorn this into a PCM operation - a big part > of > the problem here is that you're trying to add something that doesn't > fit > into a PCM operation. Dear Mark, 1. I have added a DAPM widget that is "SDB", when we open or close HDMI PIN_SWITCH, the callback 'hdmi_tx_event' registered in the widget will be triggered. Maybe you mean I shouldn't use SNDRV_PCM_TRIGGER_START and SNDRV_PCM_TRIGGER_STOP? 2. If I don't use hcd.ops->trigger notifies the bridge ic driver to switch the audio, which ops should I use? I actually want to know hdmi-codec.c and it6505.c except hdmi_codec_ops, is there any other way to communicate? My understanding is not deep enough, so please help explain more in detail, thank you very much! +static int hdmi_tx_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); + struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component); + + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + if (hcp->hcd.ops->trigger) + hcp->hcd.ops->trigger(component->dev->parent, SNDRV_PCM_TRIGGER_START); + break; + case SND_SOC_DAPM_POST_PMD: + if (hcp->hcd.ops->trigger) + hcp->hcd.ops->trigger(component->dev->parent, SNDRV_PCM_TRIGGER_STOP); + break; + default: + break; + } + + return 0; +} + static const struct snd_soc_dapm_widget hdmi_widgets[] = { + SND_SOC_DAPM_OUT_DRV_E("SDB", SND_SOC_NOPM, 0, 0, NULL, 0, hdmi_tx_event, + SND_SOC_DAPM_POST_PMD | SND_SOC_DAPM_PRE_PMU), SND_SOC_DAPM_OUTPUT("TX"), SND_SOC_DAPM_OUTPUT("RX"), };
On Mon, Dec 05, 2022 at 09:34:17AM +0000, Jiaxin Yu (俞家鑫) wrote: > 1. I have added a DAPM widget that is "SDB", when we open or close HDMI > PIN_SWITCH, the callback 'hdmi_tx_event' registered in the widget will > be triggered. Maybe you mean I shouldn't use SNDRV_PCM_TRIGGER_START > and SNDRV_PCM_TRIGGER_STOP? No, I mean that if you want to control the enable and disable of the output path you should implement a DAPM widget. > 2. If I don't use hcd.ops->trigger notifies the bridge ic driver to > switch the audio, which ops should I use? > I actually want to know hdmi-codec.c and it6505.c except > hdmi_codec_ops, is there any other way to communicate? Like I said you should use the event on the DAPM widget. This will require providing operations for the events to the drivers.
On Mon, 2022-12-05 at 12:07 +0000, Mark Brown wrote: > On Mon, Dec 05, 2022 at 09:34:17AM +0000, Jiaxin Yu (俞家鑫) wrote: > > > 1. I have added a DAPM widget that is "SDB", when we open or close > > HDMI > > PIN_SWITCH, the callback 'hdmi_tx_event' registered in the widget > > will > > be triggered. Maybe you mean I shouldn't use > > SNDRV_PCM_TRIGGER_START > > and SNDRV_PCM_TRIGGER_STOP? > > No, I mean that if you want to control the enable and disable of the > output path you should implement a DAPM widget. > May I ask which driver file to add a new DAPM widget? Is it the bridge ic driver like it6505.c? Or is it linke the "SDB" added in this patch? > > 2. If I don't use hcd.ops->trigger notifies the bridge ic driver to > > switch the audio, which ops should I use? > > I actually want to know hdmi-codec.c and it6505.c except > > hdmi_codec_ops, is there any other way to communicate? > > Like I said you should use the event on the DAPM widget. This will > require providing operations for the events to the drivers. Yes, I should add a new set of events, such as: enum { HDMI_CODEC_TRIGGER_EVENT_STOP, HDMI_CODEC_TRIGGER_EVENT_START, HDMI_CODEC_TRIGGER_EVENT_SUSPEND, HDMI_CODEC_TRIGGER_EVENT_RESUME, } Then provide handles for these events in the it6505 driver. Am I right? Thanks, Jiaxin.Yu
On Tue, Dec 13, 2022 at 02:23:32PM +0000, Jiaxin Yu (俞家鑫) wrote: > On Mon, 2022-12-05 at 12:07 +0000, Mark Brown wrote: > > On Mon, Dec 05, 2022 at 09:34:17AM +0000, Jiaxin Yu (俞家鑫) wrote: > > No, I mean that if you want to control the enable and disable of the > > output path you should implement a DAPM widget. > May I ask which driver file to add a new DAPM widget? Is it the bridge > ic driver like it6505.c? Or is it linke the "SDB" added in this patch? I would expect this to follow a similar pattern to everything else with hdmi-codec.c and have the actual ASoC stuff in there with a callback exposed to the rest of the world. > Yes, I should add a new set of events, such as: > enum { > HDMI_CODEC_TRIGGER_EVENT_STOP, > HDMI_CODEC_TRIGGER_EVENT_START, > HDMI_CODEC_TRIGGER_EVENT_SUSPEND, > HDMI_CODEC_TRIGGER_EVENT_RESUME, > } > Then provide handles for these events in the it6505 driver. Am I right? I'd expect more like on/off for a DAPM widget (the DAPM callbacks are pre/post on/off) but yes.
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 48ad33aba393..dcbc09254144 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -81,6 +81,12 @@ struct hdmi_codec_ops { struct hdmi_codec_daifmt *fmt, struct hdmi_codec_params *hparms); + /* + * PCM trigger callback. + * Mandatory + */ + int (*trigger)(struct device *dev, int cmd); + /* * Shuts down the audio stream. * Mandatory diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index 0b1cdb2d6049..cb4479372e09 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -276,7 +276,31 @@ struct hdmi_codec_priv { u8 iec_status[AES_IEC958_STATUS_SIZE]; }; +static int hdmi_tx_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); + struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component); + + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + if (hcp->hcd.ops->trigger) + hcp->hcd.ops->trigger(component->dev->parent, SNDRV_PCM_TRIGGER_START); + break; + case SND_SOC_DAPM_POST_PMD: + if (hcp->hcd.ops->trigger) + hcp->hcd.ops->trigger(component->dev->parent, SNDRV_PCM_TRIGGER_STOP); + break; + default: + break; + } + + return 0; +} + static const struct snd_soc_dapm_widget hdmi_widgets[] = { + SND_SOC_DAPM_OUT_DRV_E("SDB", SND_SOC_NOPM, 0, 0, NULL, 0, hdmi_tx_event, + SND_SOC_DAPM_POST_PMD | SND_SOC_DAPM_PRE_PMU), SND_SOC_DAPM_OUTPUT("TX"), SND_SOC_DAPM_OUTPUT("RX"), }; @@ -808,18 +832,23 @@ static int hdmi_dai_probe(struct snd_soc_dai *dai) struct hdmi_codec_daifmt *daifmt; struct snd_soc_dapm_route route[] = { { - .sink = "TX", + .sink = dai->driver->capture.stream_name, + .source = "RX", + }, + { + .sink = "SDB", .source = dai->driver->playback.stream_name, }, { - .sink = dai->driver->capture.stream_name, - .source = "RX", + .sink = "TX", + .source = "SDB", }, + }; int ret; dapm = snd_soc_component_get_dapm(dai->component); - ret = snd_soc_dapm_add_routes(dapm, route, 2); + ret = snd_soc_dapm_add_routes(dapm, route, ARRAY_SIZE(route)); if (ret) return ret;
If the speaker and hdmi are connect to the same port of I2S, when try to switch to speaker playback, we will find that hdmi is always turned on automatically. The way of switching is through SOC_DAPM_PIN_SWITCH, however, such events can not be handled in hdmi-codec driver. So add event handler for hdmi TX to solve the above issue. Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com> --- include/sound/hdmi-codec.h | 6 ++++++ sound/soc/codecs/hdmi-codec.c | 37 +++++++++++++++++++++++++++++++---- 2 files changed, 39 insertions(+), 4 deletions(-)