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

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

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.