mbox series

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

Message ID 20221125094413.4940-1-jiaxin.yu@mediatek.com
Headers show
Series ASoC: mediatek:mt8186: fix both the speaker and hdmi | expand

Message

Jiaxin Yu (俞家鑫) Nov. 25, 2022, 9:44 a.m. UTC
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(-)

Comments

Jiaxin Yu (俞家鑫) Nov. 28, 2022, 3:07 p.m. UTC | #1
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!
Mark Brown Nov. 29, 2022, 5:22 p.m. UTC | #2
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.
Jiaxin Yu (俞家鑫) Dec. 1, 2022, 3:06 p.m. UTC | #3
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!
Mark Brown Dec. 1, 2022, 3:23 p.m. UTC | #4
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.
Jiaxin Yu (俞家鑫) Dec. 5, 2022, 9:34 a.m. UTC | #5
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"),
 };
Mark Brown Dec. 5, 2022, 12:07 p.m. UTC | #6
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.
Jiaxin Yu (俞家鑫) Dec. 13, 2022, 2:23 p.m. UTC | #7
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
Mark Brown Dec. 13, 2022, 4:35 p.m. UTC | #8
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.