[PATCH v2 0/3] ASoC: mediatek:mt8186: fix both the speaker and hdmi

[PATCH v2 0/3] ASoC: mediatek:mt8186: fix both the speaker and hdmi

[Jiaxin Yu]

The speaker and hdmi of mt8186 platform are shared the same port of I2S,
when connect the external display, use build-in speakers to play audio,
they both playback at the same time. So we want to manage the playback
device through DAPM events.

Change since v1:
    Support audio function for it6505 bridge to hdmi-codec

Jiaxin Yu (3):
  ASoC: hdmi-codec: Add event handler for hdmi TX
  ASoC: mediatek: mt8186: correct the HDMI widgets
  drm/bridge: it6505: Add audio support

 drivers/gpu/drm/bridge/ite-it6505.c           | 81 +++++++++++++++++--
 include/sound/hdmi-codec.h                    |  6 ++
 sound/soc/codecs/hdmi-codec.c                 | 37 ++++++++-
 .../mt8186/mt8186-mt6366-da7219-max98357.c    |  2 +-
 .../mt8186/mt8186-mt6366-rt1019-rt5682s.c     |  2 +-
 5 files changed, 116 insertions(+), 12 deletions(-)

-- 
2.18.0

[PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Jiaxin Yu]

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(-)

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;
 
-- 
2.18.0

[PATCH v2 2/3] ASoC: mediatek: mt8186: correct the HDMI widgets

[Jiaxin Yu]

Use SND_SOC_DAPM_LINE instead of SND_SOC_DAPM_OUTPUT to trigger
DAPM events to hdmi-codec when userspace control the DPAM pin.

Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
---
 sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c | 2 +-
 sound/soc/mediatek/mt8186/mt8186-mt6366-rt1019-rt5682s.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c b/sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c
index d8c93d66b4e6..f4960b0c8298 100644
--- a/sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c
+++ b/sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c
@@ -965,7 +965,7 @@ mt8186_mt6366_da7219_max98357_widgets[] = {
 	SND_SOC_DAPM_SPK("Speakers", NULL),
 	SND_SOC_DAPM_HP("Headphones", NULL),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
-	SND_SOC_DAPM_OUTPUT("HDMI1"),
+	SND_SOC_DAPM_LINE("HDMI1", NULL),
 	SND_SOC_DAPM_MIXER(SOF_DMA_DL1, SND_SOC_NOPM, 0, 0, NULL, 0),
 	SND_SOC_DAPM_MIXER(SOF_DMA_DL2, SND_SOC_NOPM, 0, 0, NULL, 0),
 	SND_SOC_DAPM_MIXER(SOF_DMA_UL1, SND_SOC_NOPM, 0, 0, NULL, 0),
diff --git a/sound/soc/mediatek/mt8186/mt8186-mt6366-rt1019-rt5682s.c b/sound/soc/mediatek/mt8186/mt8186-mt6366-rt1019-rt5682s.c
index e59d92e2afa3..c61e9bb419d4 100644
--- a/sound/soc/mediatek/mt8186/mt8186-mt6366-rt1019-rt5682s.c
+++ b/sound/soc/mediatek/mt8186/mt8186-mt6366-rt1019-rt5682s.c
@@ -1033,7 +1033,7 @@ mt8186_mt6366_rt1019_rt5682s_widgets[] = {
 	SND_SOC_DAPM_SPK("Speakers", NULL),
 	SND_SOC_DAPM_HP("Headphone", NULL),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
-	SND_SOC_DAPM_OUTPUT("HDMI1"),
+	SND_SOC_DAPM_LINE("HDMI1", NULL),
 	SND_SOC_DAPM_MIXER(SOF_DMA_DL1, SND_SOC_NOPM, 0, 0, NULL, 0),
 	SND_SOC_DAPM_MIXER(SOF_DMA_DL2, SND_SOC_NOPM, 0, 0, NULL, 0),
 	SND_SOC_DAPM_MIXER(SOF_DMA_UL1, SND_SOC_NOPM, 0, 0, NULL, 0),
-- 
2.18.0

[PATCH v2 3/3] drm/bridge: it6505: Add audio support

[Jiaxin Yu]

Add audio support for it6505

1. Bridge to hdmi-codec to support audio feature. At the same time,
   the function of automatically detecting audio is removed.
2. It is observed that some DP-to-HDMI dongles will get into bad
   states if sending InfoFrame without audio data. Defer to enable
   it6505's audio feature when PCM triggers START or RESUME.

Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 81 ++++++++++++++++++++++++++---
 1 file changed, 75 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index dfe4351c9bdd..1f1f37b3df83 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2159,7 +2159,6 @@ static void it6505_stop_link_train(struct it6505 *it6505)
 
 static void it6505_link_train_ok(struct it6505 *it6505)
 {
-	struct device *dev = &it6505->client->dev;
 
 	it6505->link_state = LINK_OK;
 	/* disalbe mute enable avi info frame */
@@ -2167,11 +2166,6 @@ static void it6505_link_train_ok(struct it6505 *it6505)
 	it6505_set_bits(it6505, REG_INFOFRAME_CTRL,
 			EN_VID_CTRL_PKT, EN_VID_CTRL_PKT);
 
-	if (it6505_audio_input(it6505)) {
-		DRM_DEV_DEBUG_DRIVER(dev, "Enable audio!");
-		it6505_enable_audio(it6505);
-	}
-
 	if (it6505->hdcp_desired)
 		it6505_start_hdcp(it6505);
 }
@@ -2823,6 +2817,45 @@ static void __maybe_unused it6505_audio_shutdown(struct device *dev, void *data)
 		it6505_disable_audio(it6505);
 }
 
+static int it6505_audio_hw_params(struct device *dev, void *data,
+				  struct hdmi_codec_daifmt *daifmt,
+				  struct hdmi_codec_params *params)
+{
+	struct it6505 *it6505 = dev_get_drvdata(dev);
+
+	return it6505_audio_setup_hw_params(it6505, params);
+}
+
+static int it6505_audio_setup_trigger(struct it6505 *it6505, int cmd)
+{
+	struct device *dev = &it6505->client->dev;
+
+	DRM_DEV_DEBUG_DRIVER(dev, "event: %d", cmd);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		queue_delayed_work(system_wq, &it6505->delayed_audio,
+				   msecs_to_jiffies(180));
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+		cancel_delayed_work(&it6505->delayed_audio);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int it6505_audio_trigger(struct device *dev, int cmd)
+{
+	struct it6505 *it6505 = dev_get_drvdata(dev);
+
+	return it6505_audio_setup_trigger(it6505, cmd);
+}
+
 static int __maybe_unused it6505_audio_hook_plugged_cb(struct device *dev,
 						       void *data,
 						       hdmi_codec_plugged_cb fn,
@@ -2837,6 +2870,36 @@ static int __maybe_unused it6505_audio_hook_plugged_cb(struct device *dev,
 	return 0;
 }
 
+static const struct hdmi_codec_ops it6505_audio_codec_ops = {
+	.hw_params = it6505_audio_hw_params,
+	.trigger = it6505_audio_trigger,
+	.audio_shutdown = it6505_audio_shutdown,
+	.hook_plugged_cb = it6505_audio_hook_plugged_cb,
+};
+
+static int it6505_register_audio_driver(struct device *dev)
+{
+	struct it6505 *it6505 = dev_get_drvdata(dev);
+	struct hdmi_codec_pdata codec_data = {
+		.ops = &it6505_audio_codec_ops,
+		.max_i2s_channels = 8,
+		.i2s = 1,
+		.data = it6505,
+	};
+	struct platform_device *pdev;
+
+	pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME,
+					     PLATFORM_DEVID_AUTO, &codec_data,
+					     sizeof(codec_data));
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	INIT_DELAYED_WORK(&it6505->delayed_audio, it6505_delayed_audio);
+	DRM_DEV_DEBUG_DRIVER(dev, "bound to %s", HDMI_CODEC_DRV_NAME);
+
+	return 0;
+}
+
 static inline struct it6505 *bridge_to_it6505(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct it6505, bridge);
@@ -3302,6 +3365,12 @@ static int it6505_i2c_probe(struct i2c_client *client,
 		return err;
 	}
 
+	err = it6505_register_audio_driver(dev);
+	if (err < 0) {
+		DRM_DEV_ERROR(dev, "Failed to register audio driver: %d", err);
+		return err;
+	}
+
 	INIT_WORK(&it6505->link_works, it6505_link_training_work);
 	INIT_WORK(&it6505->hdcp_wait_ksv_list, it6505_hdcp_wait_ksv_list);
 	INIT_DELAYED_WORK(&it6505->hdcp_work, it6505_hdcp_work);
-- 
2.18.0

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Jiaxin Yu (俞家鑫)]

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.




************* MEDIATEK Confidentiality Notice
 ********************
The information contained in this e-mail message (including any
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be
conveyed only to the designated recipient(s). Any use, dissemination,
distribution, printing, retaining or copying of this e-mail (including its
attachments) by unintended recipient(s) is strictly prohibited and may
be unlawful. If you are not an intended recipient of this e-mail, or believe

that you have received this e-mail in error, please notify the sender
immediately (by replying to this e-mail), delete any and all copies of
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Jiaxin Yu (俞家鑫)]

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?







************* MEDIATEK Confidentiality Notice
 ********************
The information contained in this e-mail message (including any
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be
conveyed only to the designated recipient(s). Any use, dissemination,
distribution, printing, retaining or copying of this e-mail (including its
attachments) by unintended recipient(s) is strictly prohibited and may
be unlawful. If you are not an intended recipient of this e-mail, or believe

that you have received this e-mail in error, please notify the sender
immediately (by replying to this e-mail), delete any and all copies of
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Jiaxin Yu (俞家鑫)]

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"),
 };

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Jiaxin Yu (俞家鑫)]

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

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Jiaxin Yu (俞家鑫)]

On Tue, 2022-12-13 at 16:35 +0000, Mark Brown wrote:
> 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.

Dear mark,
I apologize for the delay in responding to this issue, as we have
recently encountered the same problem again. I would like to have your
guidance on this issue once more. Let me describe the problem we
encountered again.


                     ==> "hdmi-audio-codec"(it6505 plug-in)
DL1(FE) ==> I2S1(BE) 
                     ==> "rt1019p"(Speaker Codec)

I want to independently control the switches for the speaker and hdmi,
and realize that when HDMI is plugged in, I can switch to speaker
playback, and I can also switch back to hdmi playback too. Of course,
I2S1 is used for playback at this time. 

So I want to ask if I can do it by just adding
SOC_DAPM_PIN_SWITCH("Speakers") and SOC_DAPM_PIN_SWITCH("HDMI")?
Correspondingly, dapm widget and route path need to be added. That is
"SND_SOC_DAPM_SPK("Speakers", NULL)/ SND_SOC_DAPM_LINE("HDMI1", NULL)"
and "{"Speakers", NULL, "Speaker"}/ {"HDMI1", NULL, "TX"}".

If the above is not enough, what else should I modify? hdmi-codec.c or
it6505.c? Looking forward to getting you reply, Thank you.

Jiaxin.Yu

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

On Sun, Dec 01, 2024 at 05:15:45PM +0000, Jiaxin Yu (俞家鑫) wrote:

> So I want to ask if I can do it by just adding
> SOC_DAPM_PIN_SWITCH("Speakers") and SOC_DAPM_PIN_SWITCH("HDMI")?
> Correspondingly, dapm widget and route path need to be added. That is
> "SND_SOC_DAPM_SPK("Speakers", NULL)/ SND_SOC_DAPM_LINE("HDMI1", NULL)"
> and "{"Speakers", NULL, "Speaker"}/ {"HDMI1", NULL, "TX"}".

Yes, that's what I'd expect to see.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Jiaxin Yu (俞家鑫)]

On Mon, 2024-12-02 at 13:16 +0000, Mark Brown wrote:
> On Sun, Dec 01, 2024 at 05:15:45PM +0000, Jiaxin Yu (俞家鑫) wrote:
> 
> > So I want to ask if I can do it by just adding
> > SOC_DAPM_PIN_SWITCH("Speakers") and SOC_DAPM_PIN_SWITCH("HDMI")?
> > Correspondingly, dapm widget and route path need to be added. That
> > is
> > "SND_SOC_DAPM_SPK("Speakers", NULL)/ SND_SOC_DAPM_LINE("HDMI1",
> > NULL)"
> > and "{"Speakers", NULL, "Speaker"}/ {"HDMI1", NULL, "TX"}".
> 
> Yes, that's what I'd expect to see.

Dear Mark,

So if I open the "HDMI Switch" amixer control, it will call
'hdmi_codec_startup', which in turn calls "audio_startup()"  in
'hdmi_codec_ops'. Conversely, if I close it, it will call
'hdmi_codec_shutdown', which in turn calls 'audio_shutdown' in
'hdmi_codec_ops'. Is this understanding correct?

static const struct snd_soc_dai_ops hdmi_codec_i2s_dai_ops = {
        .probe                          = hdmi_dai_probe,
        .startup                        = hdmi_codec_startup,
        .shutdown                       = hdmi_codec_shutdown,
        .hw_params                      = hdmi_codec_hw_params,
        .prepare                        = hdmi_codec_prepare,
        .set_fmt                        = hdmi_codec_i2s_set_fmt,
        .mute_stream                    = hdmi_codec_mute,
        .pcm_new                        = hdmi_codec_pcm_new,
        .auto_selectable_formats        = &hdmi_codec_formats,
        .num_auto_selectable_formats    = 1,
};

 struct hdmi_codec_ops {
         /*
          * Called when ASoC starts an audio stream setup.
          * Optional
          */
         int (*audio_startup)(struct device *dev, void *data);


         /*
          * Shuts down the audio stream.
          * Mandatory
          */
         void (*audio_shutdown)(struct device *dev, void *data);

Re: [PATCH v2 1/3] ASoC: hdmi-codec: Add event handler for hdmi TX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On Fri, Dec 06, 2024 at 03:39:15PM +0000, Jiaxin Yu (俞家鑫) wrote:
> On Mon, 2024-12-02 at 13:16 +0000, Mark Brown wrote:
> > On Sun, Dec 01, 2024 at 05:15:45PM +0000, Jiaxin Yu (俞家鑫) wrote:

> > > So I want to ask if I can do it by just adding
> > > SOC_DAPM_PIN_SWITCH("Speakers") and SOC_DAPM_PIN_SWITCH("HDMI")?
> > > Correspondingly, dapm widget and route path need to be added. That
> > > is
> > > "SND_SOC_DAPM_SPK("Speakers", NULL)/ SND_SOC_DAPM_LINE("HDMI1",
> > > NULL)"
> > > and "{"Speakers", NULL, "Speaker"}/ {"HDMI1", NULL, "TX"}".

> > Yes, that's what I'd expect to see.

> So if I open the "HDMI Switch" amixer control, it will call
> 'hdmi_codec_startup', which in turn calls "audio_startup()"  in
> 'hdmi_codec_ops'. Conversely, if I close it, it will call
> 'hdmi_codec_shutdown', which in turn calls 'audio_shutdown' in
> 'hdmi_codec_ops'. Is this understanding correct?

The audio stream will still run, the DAPM path attached to it will get
shut down.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]