hp_pin was NULL value

hp_pin was NULL value

[Kailang]

Hi Takashi,

Could I move the alc294_hp_init(codec) to below line.
Because hp_pin = spec->gen.autocfg.hp_pins[0] was null value when alc294_hp_init(codec) at original line.
Or move alc269_parse_auto_config() upward.

BR,
Kailang
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -7514,7 +7514,6 @@ static int patch_alc269(struct hda_codec *codec)
 		spec->codec_variant = ALC269_TYPE_ALC294;
 		spec->gen.mixer_nid = 0; /* ALC2x4 does not have any loopback mixer path */
 		alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3)); /* UAJ MIC Vref control by verb */
-		alc294_hp_init(codec);
 		break;
 	case 0x10ec0300:
 		spec->codec_variant = ALC269_TYPE_ALC300;
@@ -7526,7 +7525,6 @@ static int patch_alc269(struct hda_codec *codec)
 		spec->codec_variant = ALC269_TYPE_ALC700;
 		spec->gen.mixer_nid = 0; /* ALC700 does not have any loopback mixer path */
 		alc_update_coef_idx(codec, 0x4a, 1 << 15, 0); /* Combo jack auto trigger control */
-		alc294_hp_init(codec);
 		break;
 
 	}
@@ -7553,6 +7551,16 @@ static int patch_alc269(struct hda_codec *codec)
 	if (err < 0)
 		goto error;
 
+	switch (codec->core.vendor_id) {
+	case 0x10ec0234:
+	case 0x10ec0274:
+	case 0x10ec0294:
+	case 0x10ec0700:
+	case 0x10ec0701:
+	case 0x10ec0703:
+		alc294_hp_init(codec);
+		break;
+	}
 	if (!spec->gen.no_analog && spec->gen.beep_nid && spec->gen.mixer_nid) {
 		err = set_beep_amp(spec, spec->gen.mixer_nid, 0x04, HDA_INPUT);
 		if (err < 0)

Re: hp_pin was NULL value

[Takashi Iwai]

On Wed, 09 Jan 2019 10:31:33 +0100,
Kailang wrote:
> 
> Hi Takashi,
> 
> Could I move the alc294_hp_init(codec) to below line.
> Because hp_pin = spec->gen.autocfg.hp_pins[0] was null value when alc294_hp_init(codec) at original line.
> Or move alc269_parse_auto_config() upward.

It looks OK to me.  But this made me wonder whether we don't need to
call this function at resume as well?  Currently it's called only at
probing.


thanks,

Takashi

Re: hp_pin was NULL value

[Kailang]

>>But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.
Yes, it only call at probing. Just need to call it at boot time.

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Wednesday, January 9, 2019 5:43 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Wed, 09 Jan 2019 10:31:33 +0100,
Kailang wrote:
> 
> Hi Takashi,
> 
> Could I move the alc294_hp_init(codec) to below line.
> Because hp_pin = spec->gen.autocfg.hp_pins[0] was null value when alc294_hp_init(codec) at original line.
> Or move alc269_parse_auto_config() upward.

It looks OK to me.  But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.


thanks,

Takashi

------Please consider the environment before printing this e-mail.

Re: hp_pin was NULL value

[Takashi Iwai]

On Wed, 09 Jan 2019 10:45:25 +0100,
Kailang wrote:
> 
> >>But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.
> Yes, it only call at probing. Just need to call it at boot time.
> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de> 
> Sent: Wednesday, January 9, 2019 5:43 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Wed, 09 Jan 2019 10:31:33 +0100,
> Kailang wrote:
> > 
> > Hi Takashi,
> > 
> > Could I move the alc294_hp_init(codec) to below line.
> > Because hp_pin = spec->gen.autocfg.hp_pins[0] was null value when alc294_hp_init(codec) at original line.
> > Or move alc269_parse_auto_config() upward.
> 
> It looks OK to me.  But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.

Hrm, but it modifies many COEFs, and are these all preserved with
suspend?  For example, there is also hibernation (S4), which is a
switch from the boot without the audio driver initialization.  Then
we'll resume from the uninitialized state.  I guess other COEF changes
in that area should be moved as the additional init hook as well.

Actually, if we do call this at resume, the change would be easier, a
patch like below.


thanks,

Takashi

--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -7404,6 +7404,20 @@ static void alc294_hp_init(struct hda_codec *codec)
 	msleep(50);
 }
 
+static void alc294_init(struct hda_codec *codec)
+{
+	/* UAJ MIC Vref control by verb */
+	alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3));
+	alc294_hp_init(codec);
+}
+
+static void alc700_init(struct hda_codec *codec)
+{
+	/* Combo jack auto trigger control */
+	alc_update_coef_idx(codec, 0x4a, 1 << 15, 0);
+	alc294_hp_init(codec);
+}
+
 /*
  */
 static int patch_alc269(struct hda_codec *codec)
@@ -7528,8 +7542,7 @@ static int patch_alc269(struct hda_codec *codec)
 	case 0x10ec0294:
 		spec->codec_variant = ALC269_TYPE_ALC294;
 		spec->gen.mixer_nid = 0; /* ALC2x4 does not have any loopback mixer path */
-		alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3)); /* UAJ MIC Vref control by verb */
-		alc294_hp_init(codec);
+		spec->init_hook = alc294_init;
 		break;
 	case 0x10ec0300:
 		spec->codec_variant = ALC269_TYPE_ALC300;
@@ -7540,8 +7553,7 @@ static int patch_alc269(struct hda_codec *codec)
 	case 0x10ec0703:
 		spec->codec_variant = ALC269_TYPE_ALC700;
 		spec->gen.mixer_nid = 0; /* ALC700 does not have any loopback mixer path */
-		alc_update_coef_idx(codec, 0x4a, 1 << 15, 0); /* Combo jack auto trigger control */
-		alc294_hp_init(codec);
+		spec->init_hook = alc700_init;
 		break;
 
 	}

Re: hp_pin was NULL value

[Takashi Iwai]

On Wed, 09 Jan 2019 12:19:14 +0100,
Kailang wrote:
> 
> Hi Takashi,
> 
> But spec->init_hook will call at resume back.
> alc294_hp_init() function just need to call at one time.

As already mentioned, even if it has to be called only once and does
something for the hardware, it'll be missing if the machine goes
hibernate (S4) and resume.  In S4, the machine boots from scratch and
switches immediately to the saved image that calls resume callback.
What's worse for us is that this switch happens usually in initrd,
i.e. before the sound driver gets loaded.  So, the machine goes to
resume without processing alc294_hp_init().

IOW, even if it's needed only once, you'll have to call it at resume
unless it has a significant drawback.

(Strictly speaking, we may use thaw PM callback, but the HD-audio
 codec driver doesn't use it, so there is only resume callback for
 now.)

> And it need to call it before call spec->init_hook.

But ALC294 (and ALC700) have no init_hook, so far?

> ALC225 also need to call similar procedure. I will post it later.

OK, thanks.


Takashi

> 
> BR,
> Kailang
> ________________________________________
> 從: Takashi Iwai [tiwai@suse.de]
> 寄件日期: 2019年1月9日 下午 05:56
> 至: Kailang
> 副本:  (alsa-devel@alsa-project.org)
> 主旨: Re: hp_pin was NULL value
> 
> On Wed, 09 Jan 2019 10:45:25 +0100,
> Kailang wrote:
> >
> > >>But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.
> > Yes, it only call at probing. Just need to call it at boot time.
> >
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Wednesday, January 9, 2019 5:43 PM
> > To: Kailang <kailang@realtek.com>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: Re: hp_pin was NULL value
> >
> > On Wed, 09 Jan 2019 10:31:33 +0100,
> > Kailang wrote:
> > >
> > > Hi Takashi,
> > >
> > > Could I move the alc294_hp_init(codec) to below line.
> > > Because hp_pin = spec->gen.autocfg.hp_pins[0] was null value when alc294_hp_init(codec) at original line.
> > > Or move alc269_parse_auto_config() upward.
> >
> > It looks OK to me.  But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.
> 
> Hrm, but it modifies many COEFs, and are these all preserved with
> suspend?  For example, there is also hibernation (S4), which is a
> switch from the boot without the audio driver initialization.  Then
> we'll resume from the uninitialized state.  I guess other COEF changes
> in that area should be moved as the additional init hook as well.
> 
> Actually, if we do call this at resume, the change would be easier, a
> patch like below.
> 
> 
> thanks,
> 
> Takashi
> 
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -7404,6 +7404,20 @@ static void alc294_hp_init(struct hda_codec *codec)
>         msleep(50);
>  }
> 
> +static void alc294_init(struct hda_codec *codec)
> +{
> +       /* UAJ MIC Vref control by verb */
> +       alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3));
> +       alc294_hp_init(codec);
> +}
> +
> +static void alc700_init(struct hda_codec *codec)
> +{
> +       /* Combo jack auto trigger control */
> +       alc_update_coef_idx(codec, 0x4a, 1 << 15, 0);
> +       alc294_hp_init(codec);
> +}
> +
>  /*
>   */
>  static int patch_alc269(struct hda_codec *codec)
> @@ -7528,8 +7542,7 @@ static int patch_alc269(struct hda_codec *codec)
>         case 0x10ec0294:
>                 spec->codec_variant = ALC269_TYPE_ALC294;
>                 spec->gen.mixer_nid = 0; /* ALC2x4 does not have any loopback mixer path */
> -               alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3)); /* UAJ MIC Vref control by verb */
> -               alc294_hp_init(codec);
> +               spec->init_hook = alc294_init;
>                 break;
>         case 0x10ec0300:
>                 spec->codec_variant = ALC269_TYPE_ALC300;
> @@ -7540,8 +7553,7 @@ static int patch_alc269(struct hda_codec *codec)
>         case 0x10ec0703:
>                 spec->codec_variant = ALC269_TYPE_ALC700;
>                 spec->gen.mixer_nid = 0; /* ALC700 does not have any loopback mixer path */
> -               alc_update_coef_idx(codec, 0x4a, 1 << 15, 0); /* Combo jack auto trigger control */
> -               alc294_hp_init(codec);
> +               spec->init_hook = alc700_init;
>                 break;
> 
>         }
> 
> 
> ------Please consider the environment before printing this e-mail.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: hp_pin was NULL value

[Takashi Iwai]

On Wed, 09 Jan 2019 13:32:51 +0100,
Kailang wrote:
> 
> 
> Hi Takashi,
> 
> 
> OK! I will test it. 
> So, if ALC225 need to do this. I will assigned it to alc225_init(). Right?
> 
> >>But ALC294 (and ALC700) have no init_hook, so far?
> 
> spec->init_hook = alc_default_init;
> It has default init_hook. I use this to do standard headphone depop function.

Ah, OK.  Then we need a call chain from alc294_init(),

static void alc294_init()
{
	coef();
	alc294_hp_init();
	alc_default_init();
}


thanks,

Takashi

> 
> BR,
> Kailang
> ________________________________________
> 從: Takashi Iwai [tiwai@suse.de]
> 寄件日期: 2019年1月9日 下午 07:29
> 至: Kailang
> 副本:  (alsa-devel@alsa-project.org)
> 主旨: Re: hp_pin was NULL value
> 
> On Wed, 09 Jan 2019 12:19:14 +0100,
> Kailang wrote:
> >
> > Hi Takashi,
> >
> > But spec->init_hook will call at resume back.
> > alc294_hp_init() function just need to call at one time.
> 
> As already mentioned, even if it has to be called only once and does
> something for the hardware, it'll be missing if the machine goes
> hibernate (S4) and resume.  In S4, the machine boots from scratch and
> switches immediately to the saved image that calls resume callback.
> What's worse for us is that this switch happens usually in initrd,
> i.e. before the sound driver gets loaded.  So, the machine goes to
> resume without processing alc294_hp_init().
> 
> IOW, even if it's needed only once, you'll have to call it at resume
> unless it has a significant drawback.
> 
> (Strictly speaking, we may use thaw PM callback, but the HD-audio
>  codec driver doesn't use it, so there is only resume callback for
>  now.)
> 
> > And it need to call it before call spec->init_hook.
> 
> But ALC294 (and ALC700) have no init_hook, so far?
> 
> > ALC225 also need to call similar procedure. I will post it later.
> 
> OK, thanks.
> 
> 
> Takashi
> 
> >
> > BR,
> > Kailang
> > ________________________________________
> > 從: Takashi Iwai [tiwai@suse.de]
> > 寄件日期: 2019年1月9日 下午 05:56
> > 至: Kailang
> > 副本:  (alsa-devel@alsa-project.org)
> > 主旨: Re: hp_pin was NULL value
> >
> > On Wed, 09 Jan 2019 10:45:25 +0100,
> > Kailang wrote:
> > >
> > > >>But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.
> > > Yes, it only call at probing. Just need to call it at boot time.
> > >
> > > -----Original Message-----
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Sent: Wednesday, January 9, 2019 5:43 PM
> > > To: Kailang <kailang@realtek.com>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: Re: hp_pin was NULL value
> > >
> > > On Wed, 09 Jan 2019 10:31:33 +0100,
> > > Kailang wrote:
> > > >
> > > > Hi Takashi,
> > > >
> > > > Could I move the alc294_hp_init(codec) to below line.
> > > > Because hp_pin = spec->gen.autocfg.hp_pins[0] was null value when alc294_hp_init(codec) at original line.
> > > > Or move alc269_parse_auto_config() upward.
> > >
> > > It looks OK to me.  But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.
> >
> > Hrm, but it modifies many COEFs, and are these all preserved with
> > suspend?  For example, there is also hibernation (S4), which is a
> > switch from the boot without the audio driver initialization.  Then
> > we'll resume from the uninitialized state.  I guess other COEF changes
> > in that area should be moved as the additional init hook as well.
> >
> > Actually, if we do call this at resume, the change would be easier, a
> > patch like below.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -7404,6 +7404,20 @@ static void alc294_hp_init(struct hda_codec *codec)
> >         msleep(50);
> >  }
> >
> > +static void alc294_init(struct hda_codec *codec)
> > +{
> > +       /* UAJ MIC Vref control by verb */
> > +       alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3));
> > +       alc294_hp_init(codec);
> > +}
> > +
> > +static void alc700_init(struct hda_codec *codec)
> > +{
> > +       /* Combo jack auto trigger control */
> > +       alc_update_coef_idx(codec, 0x4a, 1 << 15, 0);
> > +       alc294_hp_init(codec);
> > +}
> > +
> >  /*
> >   */
> >  static int patch_alc269(struct hda_codec *codec)
> > @@ -7528,8 +7542,7 @@ static int patch_alc269(struct hda_codec *codec)
> >         case 0x10ec0294:
> >                 spec->codec_variant = ALC269_TYPE_ALC294;
> >                 spec->gen.mixer_nid = 0; /* ALC2x4 does not have any loopback mixer path */
> > -               alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3)); /* UAJ MIC Vref control by verb */
> > -               alc294_hp_init(codec);
> > +               spec->init_hook = alc294_init;
> >                 break;
> >         case 0x10ec0300:
> >                 spec->codec_variant = ALC269_TYPE_ALC300;
> > @@ -7540,8 +7553,7 @@ static int patch_alc269(struct hda_codec *codec)
> >         case 0x10ec0703:
> >                 spec->codec_variant = ALC269_TYPE_ALC700;
> >                 spec->gen.mixer_nid = 0; /* ALC700 does not have any loopback mixer path */
> > -               alc_update_coef_idx(codec, 0x4a, 1 << 15, 0); /* Combo jack auto trigger control */
> > -               alc294_hp_init(codec);
> > +               spec->init_hook = alc700_init;
> >                 break;
> >
> >         }
> >
> >
> > ------Please consider the environment before printing this e-mail.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: hp_pin was NULL value

[Kailang]

Hi Takashi,

I simulate test your prefer function.
I put alc225_hp_init() in alc225_init().
If system boot ready, I write 1 to power_save of /sys/module/....
The print message as below. It called two times until boot ready.
If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().

[   21.497524] alc225_init hp_pin=0x21
[   21.497526] alc225_hp_init-s hp=0x21
[   22.140076] alc225_hp_init-e hp=0x21
[   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
[   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
[   22.186846] alc225_shutup hp_pin=0x21
[   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
[   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
[   22.205104] alc225_init hp_pin=0x21
[   22.205108] alc225_hp_init-s hp=0x21
[   22.852123] alc225_hp_init-e hp=0x21
[   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
############  Boot ready ##############
[  116.832084] alc225_shutup hp_pin=0x21  ==> echo 1 to power_save
[  120.002582] alc225_init hp_pin=0x21     ==> play system sound
[  120.002586] alc225_hp_init-s hp=0x21
[  120.644128] alc225_hp_init-e hp=0x21

Many thanks.
BR,
Kailang

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Wednesday, January 9, 2019 9:01 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Wed, 09 Jan 2019 13:32:51 +0100,
Kailang wrote:
> 
> 
> Hi Takashi,
> 
> 
> OK! I will test it. 
> So, if ALC225 need to do this. I will assigned it to alc225_init(). Right?
> 
> >>But ALC294 (and ALC700) have no init_hook, so far?
> 
> spec->init_hook = alc_default_init;
> It has default init_hook. I use this to do standard headphone depop function.

Ah, OK.  Then we need a call chain from alc294_init(),

static void alc294_init()
{
	coef();
	alc294_hp_init();
	alc_default_init();
}


thanks,

Takashi

> 
> BR,
> Kailang
> ________________________________________
> 從: Takashi Iwai [tiwai@suse.de]
> 寄件日期: 2019年1月9日 下午 07:29
> 至: Kailang
> 副本:  (alsa-devel@alsa-project.org)
> 主旨: Re: hp_pin was NULL value
> 
> On Wed, 09 Jan 2019 12:19:14 +0100,
> Kailang wrote:
> >
> > Hi Takashi,
> >
> > But spec->init_hook will call at resume back.
> > alc294_hp_init() function just need to call at one time.
> 
> As already mentioned, even if it has to be called only once and does 
> something for the hardware, it'll be missing if the machine goes 
> hibernate (S4) and resume.  In S4, the machine boots from scratch and 
> switches immediately to the saved image that calls resume callback.
> What's worse for us is that this switch happens usually in initrd, 
> i.e. before the sound driver gets loaded.  So, the machine goes to 
> resume without processing alc294_hp_init().
> 
> IOW, even if it's needed only once, you'll have to call it at resume 
> unless it has a significant drawback.
> 
> (Strictly speaking, we may use thaw PM callback, but the HD-audio  
> codec driver doesn't use it, so there is only resume callback for
>  now.)
> 
> > And it need to call it before call spec->init_hook.
> 
> But ALC294 (and ALC700) have no init_hook, so far?
> 
> > ALC225 also need to call similar procedure. I will post it later.
> 
> OK, thanks.
> 
> 
> Takashi
> 
> >
> > BR,
> > Kailang
> > ________________________________________
> > 從: Takashi Iwai [tiwai@suse.de]
> > 寄件日期: 2019年1月9日 下午 05:56
> > 至: Kailang
> > 副本:  (alsa-devel@alsa-project.org)
> > 主旨: Re: hp_pin was NULL value
> >
> > On Wed, 09 Jan 2019 10:45:25 +0100,
> > Kailang wrote:
> > >
> > > >>But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.
> > > Yes, it only call at probing. Just need to call it at boot time.
> > >
> > > -----Original Message-----
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Sent: Wednesday, January 9, 2019 5:43 PM
> > > To: Kailang <kailang@realtek.com>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: Re: hp_pin was NULL value
> > >
> > > On Wed, 09 Jan 2019 10:31:33 +0100, Kailang wrote:
> > > >
> > > > Hi Takashi,
> > > >
> > > > Could I move the alc294_hp_init(codec) to below line.
> > > > Because hp_pin = spec->gen.autocfg.hp_pins[0] was null value when alc294_hp_init(codec) at original line.
> > > > Or move alc269_parse_auto_config() upward.
> > >
> > > It looks OK to me.  But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.
> >
> > Hrm, but it modifies many COEFs, and are these all preserved with 
> > suspend?  For example, there is also hibernation (S4), which is a 
> > switch from the boot without the audio driver initialization.  Then 
> > we'll resume from the uninitialized state.  I guess other COEF 
> > changes in that area should be moved as the additional init hook as well.
> >
> > Actually, if we do call this at resume, the change would be easier, 
> > a patch like below.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -7404,6 +7404,20 @@ static void alc294_hp_init(struct hda_codec *codec)
> >         msleep(50);
> >  }
> >
> > +static void alc294_init(struct hda_codec *codec) {
> > +       /* UAJ MIC Vref control by verb */
> > +       alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3));
> > +       alc294_hp_init(codec);
> > +}
> > +
> > +static void alc700_init(struct hda_codec *codec) {
> > +       /* Combo jack auto trigger control */
> > +       alc_update_coef_idx(codec, 0x4a, 1 << 15, 0);
> > +       alc294_hp_init(codec);
> > +}
> > +
> >  /*
> >   */
> >  static int patch_alc269(struct hda_codec *codec) @@ -7528,8 +7542,7 
> > @@ static int patch_alc269(struct hda_codec *codec)
> >         case 0x10ec0294:
> >                 spec->codec_variant = ALC269_TYPE_ALC294;
> >                 spec->gen.mixer_nid = 0; /* ALC2x4 does not have any loopback mixer path */
> > -               alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3)); /* UAJ MIC Vref control by verb */
> > -               alc294_hp_init(codec);
> > +               spec->init_hook = alc294_init;
> >                 break;
> >         case 0x10ec0300:
> >                 spec->codec_variant = ALC269_TYPE_ALC300; @@ -7540,8 
> > +7553,7 @@ static int patch_alc269(struct hda_codec *codec)
> >         case 0x10ec0703:
> >                 spec->codec_variant = ALC269_TYPE_ALC700;
> >                 spec->gen.mixer_nid = 0; /* ALC700 does not have any loopback mixer path */
> > -               alc_update_coef_idx(codec, 0x4a, 1 << 15, 0); /* Combo jack auto trigger control */
> > -               alc294_hp_init(codec);
> > +               spec->init_hook = alc700_init;
> >                 break;
> >
> >         }
> >
> >
> > ------Please consider the environment before printing this e-mail.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: hp_pin was NULL value

[Kailang]

Hi Takashi,

Could you receive this email?

BR,
Kailang

-----Original Message-----
From: Kailang 
Sent: Thursday, January 10, 2019 11:14 AM
To: 'Takashi Iwai' <tiwai@suse.de>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: RE: hp_pin was NULL value

Hi Takashi,

I simulate test your prefer function.
I put alc225_hp_init() in alc225_init().
If system boot ready, I write 1 to power_save of /sys/module/....
The print message as below. It called two times until boot ready.
If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().

[   21.497524] alc225_init hp_pin=0x21
[   21.497526] alc225_hp_init-s hp=0x21
[   22.140076] alc225_hp_init-e hp=0x21
[   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
[   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
[   22.186846] alc225_shutup hp_pin=0x21
[   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
[   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
[   22.205104] alc225_init hp_pin=0x21
[   22.205108] alc225_hp_init-s hp=0x21
[   22.852123] alc225_hp_init-e hp=0x21
[   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
############  Boot ready ##############
[  116.832084] alc225_shutup hp_pin=0x21  ==> echo 1 to power_save
[  120.002582] alc225_init hp_pin=0x21     ==> play system sound
[  120.002586] alc225_hp_init-s hp=0x21
[  120.644128] alc225_hp_init-e hp=0x21

Many thanks.
BR,
Kailang

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de>
Sent: Wednesday, January 9, 2019 9:01 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Wed, 09 Jan 2019 13:32:51 +0100,
Kailang wrote:
> 
> 
> Hi Takashi,
> 
> 
> OK! I will test it. 
> So, if ALC225 need to do this. I will assigned it to alc225_init(). Right?
> 
> >>But ALC294 (and ALC700) have no init_hook, so far?
> 
> spec->init_hook = alc_default_init;
> It has default init_hook. I use this to do standard headphone depop function.

Ah, OK.  Then we need a call chain from alc294_init(),

static void alc294_init()
{
	coef();
	alc294_hp_init();
	alc_default_init();
}


thanks,

Takashi

> 
> BR,
> Kailang
> ________________________________________
> 從: Takashi Iwai [tiwai@suse.de]
> 寄件日期: 2019年1月9日 下午 07:29
> 至: Kailang
> 副本:  (alsa-devel@alsa-project.org)
> 主旨: Re: hp_pin was NULL value
> 
> On Wed, 09 Jan 2019 12:19:14 +0100,
> Kailang wrote:
> >
> > Hi Takashi,
> >
> > But spec->init_hook will call at resume back.
> > alc294_hp_init() function just need to call at one time.
> 
> As already mentioned, even if it has to be called only once and does 
> something for the hardware, it'll be missing if the machine goes 
> hibernate (S4) and resume.  In S4, the machine boots from scratch and 
> switches immediately to the saved image that calls resume callback.
> What's worse for us is that this switch happens usually in initrd, 
> i.e. before the sound driver gets loaded.  So, the machine goes to 
> resume without processing alc294_hp_init().
> 
> IOW, even if it's needed only once, you'll have to call it at resume 
> unless it has a significant drawback.
> 
> (Strictly speaking, we may use thaw PM callback, but the HD-audio 
> codec driver doesn't use it, so there is only resume callback for
>  now.)
> 
> > And it need to call it before call spec->init_hook.
> 
> But ALC294 (and ALC700) have no init_hook, so far?
> 
> > ALC225 also need to call similar procedure. I will post it later.
> 
> OK, thanks.
> 
> 
> Takashi
> 
> >
> > BR,
> > Kailang
> > ________________________________________
> > 從: Takashi Iwai [tiwai@suse.de]
> > 寄件日期: 2019年1月9日 下午 05:56
> > 至: Kailang
> > 副本:  (alsa-devel@alsa-project.org)
> > 主旨: Re: hp_pin was NULL value
> >
> > On Wed, 09 Jan 2019 10:45:25 +0100,
> > Kailang wrote:
> > >
> > > >>But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.
> > > Yes, it only call at probing. Just need to call it at boot time.
> > >
> > > -----Original Message-----
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Sent: Wednesday, January 9, 2019 5:43 PM
> > > To: Kailang <kailang@realtek.com>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: Re: hp_pin was NULL value
> > >
> > > On Wed, 09 Jan 2019 10:31:33 +0100, Kailang wrote:
> > > >
> > > > Hi Takashi,
> > > >
> > > > Could I move the alc294_hp_init(codec) to below line.
> > > > Because hp_pin = spec->gen.autocfg.hp_pins[0] was null value when alc294_hp_init(codec) at original line.
> > > > Or move alc269_parse_auto_config() upward.
> > >
> > > It looks OK to me.  But this made me wonder whether we don't need to call this function at resume as well?  Currently it's called only at probing.
> >
> > Hrm, but it modifies many COEFs, and are these all preserved with 
> > suspend?  For example, there is also hibernation (S4), which is a 
> > switch from the boot without the audio driver initialization.  Then 
> > we'll resume from the uninitialized state.  I guess other COEF 
> > changes in that area should be moved as the additional init hook as well.
> >
> > Actually, if we do call this at resume, the change would be easier, 
> > a patch like below.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -7404,6 +7404,20 @@ static void alc294_hp_init(struct hda_codec *codec)
> >         msleep(50);
> >  }
> >
> > +static void alc294_init(struct hda_codec *codec) {
> > +       /* UAJ MIC Vref control by verb */
> > +       alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3));
> > +       alc294_hp_init(codec);
> > +}
> > +
> > +static void alc700_init(struct hda_codec *codec) {
> > +       /* Combo jack auto trigger control */
> > +       alc_update_coef_idx(codec, 0x4a, 1 << 15, 0);
> > +       alc294_hp_init(codec);
> > +}
> > +
> >  /*
> >   */
> >  static int patch_alc269(struct hda_codec *codec) @@ -7528,8 +7542,7 
> > @@ static int patch_alc269(struct hda_codec *codec)
> >         case 0x10ec0294:
> >                 spec->codec_variant = ALC269_TYPE_ALC294;
> >                 spec->gen.mixer_nid = 0; /* ALC2x4 does not have any loopback mixer path */
> > -               alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3)); /* UAJ MIC Vref control by verb */
> > -               alc294_hp_init(codec);
> > +               spec->init_hook = alc294_init;
> >                 break;
> >         case 0x10ec0300:
> >                 spec->codec_variant = ALC269_TYPE_ALC300; @@ -7540,8
> > +7553,7 @@ static int patch_alc269(struct hda_codec *codec)
> >         case 0x10ec0703:
> >                 spec->codec_variant = ALC269_TYPE_ALC700;
> >                 spec->gen.mixer_nid = 0; /* ALC700 does not have any loopback mixer path */
> > -               alc_update_coef_idx(codec, 0x4a, 1 << 15, 0); /* Combo jack auto trigger control */
> > -               alc294_hp_init(codec);
> > +               spec->init_hook = alc700_init;
> >                 break;
> >
> >         }
> >
> >
> > ------Please consider the environment before printing this e-mail.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: hp_pin was NULL value

[Takashi Iwai]

On Tue, 15 Jan 2019 03:31:33 +0100,
Kailang wrote:
> 
> Hi Takashi,
> 
> Could you receive this email?
> 
> BR,
> Kailang
> 
> -----Original Message-----
> From: Kailang 
> Sent: Thursday, January 10, 2019 11:14 AM
> To: 'Takashi Iwai' <tiwai@suse.de>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: RE: hp_pin was NULL value
> 
> Hi Takashi,
> 
> I simulate test your prefer function.
> I put alc225_hp_init() in alc225_init().
> If system boot ready, I write 1 to power_save of /sys/module/....
> The print message as below. It called two times until boot ready.
> If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> 
> [   21.497524] alc225_init hp_pin=0x21
> [   21.497526] alc225_hp_init-s hp=0x21
> [   22.140076] alc225_hp_init-e hp=0x21
> [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> [   22.186846] alc225_shutup hp_pin=0x21
> [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> [   22.205104] alc225_init hp_pin=0x21
> [   22.205108] alc225_hp_init-s hp=0x21
> [   22.852123] alc225_hp_init-e hp=0x21
> [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> ############  Boot ready ##############
> [  116.832084] alc225_shutup hp_pin=0x21  ==> echo 1 to power_save
> [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> [  120.002586] alc225_hp_init-s hp=0x21
> [  120.644128] alc225_hp_init-e hp=0x21

It wasn't clear to me whether you meant this as a success or a
failure...  Did the patch work as expected, no?


thanks,

Takashi

Re: hp_pin was NULL value

[Kailang]

No, it not expected.
It will enter two times after boot ready.
I think it could add a flag in alc225_hp_init(). 
To make sure it only excute one time. How do you think about?

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Tuesday, January 15, 2019 2:37 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Tue, 15 Jan 2019 03:31:33 +0100,
Kailang wrote:
> 
> Hi Takashi,
> 
> Could you receive this email?
> 
> BR,
> Kailang
> 
> -----Original Message-----
> From: Kailang
> Sent: Thursday, January 10, 2019 11:14 AM
> To: 'Takashi Iwai' <tiwai@suse.de>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: RE: hp_pin was NULL value
> 
> Hi Takashi,
> 
> I simulate test your prefer function.
> I put alc225_hp_init() in alc225_init().
> If system boot ready, I write 1 to power_save of /sys/module/....
> The print message as below. It called two times until boot ready.
> If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> 
> [   21.497524] alc225_init hp_pin=0x21
> [   21.497526] alc225_hp_init-s hp=0x21
> [   22.140076] alc225_hp_init-e hp=0x21
> [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> [   22.186846] alc225_shutup hp_pin=0x21
> [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> [   22.205104] alc225_init hp_pin=0x21
> [   22.205108] alc225_hp_init-s hp=0x21
> [   22.852123] alc225_hp_init-e hp=0x21
> [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> ############  Boot ready ############## [  116.832084] alc225_shutup 
> hp_pin=0x21  ==> echo 1 to power_save
> [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> alc225_hp_init-e hp=0x21

It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?


thanks,

Takashi

------Please consider the environment before printing this e-mail.

Re: hp_pin was NULL value

[Takashi Iwai]

On Tue, 15 Jan 2019 08:43:40 +0100,
Kailang wrote:
> 
> 
> No, it not expected.
> It will enter two times after boot ready.
> I think it could add a flag in alc225_hp_init(). 
> To make sure it only excute one time. How do you think about?

Where was it called, actually?  The call of patch_ops.init is found in
several places: once in snd_hda_codec_build_controls() just before
creating controls (which is the boot time init), and at resume,
alc269_resume().

And, as already explained, we have to call this at resume, because
it's not guaranteed that it's been already called in the case of
hibernation.


thanks,

Takashi

> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de> 
> Sent: Tuesday, January 15, 2019 2:37 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 15 Jan 2019 03:31:33 +0100,
> Kailang wrote:
> > 
> > Hi Takashi,
> > 
> > Could you receive this email?
> > 
> > BR,
> > Kailang
> > 
> > -----Original Message-----
> > From: Kailang
> > Sent: Thursday, January 10, 2019 11:14 AM
> > To: 'Takashi Iwai' <tiwai@suse.de>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: RE: hp_pin was NULL value
> > 
> > Hi Takashi,
> > 
> > I simulate test your prefer function.
> > I put alc225_hp_init() in alc225_init().
> > If system boot ready, I write 1 to power_save of /sys/module/....
> > The print message as below. It called two times until boot ready.
> > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > 
> > [   21.497524] alc225_init hp_pin=0x21
> > [   21.497526] alc225_hp_init-s hp=0x21
> > [   22.140076] alc225_hp_init-e hp=0x21
> > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > [   22.186846] alc225_shutup hp_pin=0x21
> > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > [   22.205104] alc225_init hp_pin=0x21
> > [   22.205108] alc225_hp_init-s hp=0x21
> > [   22.852123] alc225_hp_init-e hp=0x21
> > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > ############  Boot ready ############## [  116.832084] alc225_shutup 
> > hp_pin=0x21  ==> echo 1 to power_save
> > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > alc225_hp_init-e hp=0x21
> 
> It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> 
> 
> thanks,
> 
> Takashi
> 
> ------Please consider the environment before printing this e-mail.
> 

Re: hp_pin was NULL value

[Kailang]

Hi Takashi,

Sorry!! Maybe I don't understand what you mean.	
Please see below.

--- patch_realtek.c.orig	2019-01-11 16:28:43.745957854 +0800
+++ patch_realtek.c	2019-01-15 16:09:56.017873567 +0800
@@ -3221,12 +3221,29 @@ static void alc256_shutup(struct hda_cod
 	snd_hda_shutup_pins(codec);
 }
 
+static void alc225_hp_init(struct hda_codec *codec)
+{
+	struct alc_spec *spec = codec->spec;
+	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
+	int i, val;
+	int coef38, coef0d, coef36;
+	printk("%s-start hp=0x%x\n",__func__,hp_pin);
+	if (!hp_pin)
+		return;
+
+	alc_update_coef_idx(codec, 0x4a, 1<<15, 1<<15);
+	msleep(50);
+	alc_update_coef_idx(codec, 0x4a, 1<<15, 0);
+	printk("%s-end hp=0x%x\n",__func__,hp_pin);
+}
+
 static void alc225_init(struct hda_codec *codec)
 {
 	struct alc_spec *spec = codec->spec;
 	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
 	bool hp1_pin_sense, hp2_pin_sense;
 
+	alc225_hp_init(codec);
 	if (!hp_pin)
 		return;
 
@@ -3262,6 +3279,7 @@ static void alc225_init(struct hda_codec
 
 	alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
 	alc_update_coefex_idx(codec, 0x57, 0x04, 0x0007, 0x4); /* Hight power */
+	printk("%s hp=0x%x\n",__func__,hp_pin);
 }

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Tuesday, January 15, 2019 3:54 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Tue, 15 Jan 2019 08:43:40 +0100,
Kailang wrote:
> 
> 
> No, it not expected.
> It will enter two times after boot ready.
> I think it could add a flag in alc225_hp_init(). 
> To make sure it only excute one time. How do you think about?

Where was it called, actually?  The call of patch_ops.init is found in several places: once in snd_hda_codec_build_controls() just before creating controls (which is the boot time init), and at resume, alc269_resume().

And, as already explained, we have to call this at resume, because it's not guaranteed that it's been already called in the case of hibernation.


thanks,

Takashi

> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Tuesday, January 15, 2019 2:37 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 15 Jan 2019 03:31:33 +0100,
> Kailang wrote:
> > 
> > Hi Takashi,
> > 
> > Could you receive this email?
> > 
> > BR,
> > Kailang
> > 
> > -----Original Message-----
> > From: Kailang
> > Sent: Thursday, January 10, 2019 11:14 AM
> > To: 'Takashi Iwai' <tiwai@suse.de>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: RE: hp_pin was NULL value
> > 
> > Hi Takashi,
> > 
> > I simulate test your prefer function.
> > I put alc225_hp_init() in alc225_init().
> > If system boot ready, I write 1 to power_save of /sys/module/....
> > The print message as below. It called two times until boot ready.
> > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > 
> > [   21.497524] alc225_init hp_pin=0x21
> > [   21.497526] alc225_hp_init-s hp=0x21
> > [   22.140076] alc225_hp_init-e hp=0x21
> > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > [   22.186846] alc225_shutup hp_pin=0x21
> > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > [   22.205104] alc225_init hp_pin=0x21
> > [   22.205108] alc225_hp_init-s hp=0x21
> > [   22.852123] alc225_hp_init-e hp=0x21
> > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > ############  Boot ready ############## [  116.832084] alc225_shutup
> > hp_pin=0x21  ==> echo 1 to power_save
> > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > alc225_hp_init-e hp=0x21
> 
> It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> 
> 
> thanks,
> 
> Takashi
> 
> ------Please consider the environment before printing this e-mail.
> 

Re: hp_pin was NULL value

[Takashi Iwai]

On Tue, 15 Jan 2019 09:17:24 +0100,
Kailang wrote:
> 
> 
> Hi Takashi,
> 
> Sorry!! Maybe I don't understand what you mean.	
> Please see below.
> 
> --- patch_realtek.c.orig	2019-01-11 16:28:43.745957854 +0800
> +++ patch_realtek.c	2019-01-15 16:09:56.017873567 +0800
> @@ -3221,12 +3221,29 @@ static void alc256_shutup(struct hda_cod
>  	snd_hda_shutup_pins(codec);
>  }
>  
> +static void alc225_hp_init(struct hda_codec *codec)
> +{
> +	struct alc_spec *spec = codec->spec;
> +	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> +	int i, val;
> +	int coef38, coef0d, coef36;
> +	printk("%s-start hp=0x%x\n",__func__,hp_pin);
> +	if (!hp_pin)
> +		return;
> +
> +	alc_update_coef_idx(codec, 0x4a, 1<<15, 1<<15);
> +	msleep(50);
> +	alc_update_coef_idx(codec, 0x4a, 1<<15, 0);
> +	printk("%s-end hp=0x%x\n",__func__,hp_pin);
> +}
> +
>  static void alc225_init(struct hda_codec *codec)
>  {
>  	struct alc_spec *spec = codec->spec;
>  	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
>  	bool hp1_pin_sense, hp2_pin_sense;
>  
> +	alc225_hp_init(codec);
>  	if (!hp_pin)
>  		return;
>  
> @@ -3262,6 +3279,7 @@ static void alc225_init(struct hda_codec
>  
>  	alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
>  	alc_update_coefex_idx(codec, 0x57, 0x04, 0x0007, 0x4); /* Hight power */
> +	printk("%s hp=0x%x\n",__func__,hp_pin);
>  }

... and I don't understand what's your problem.

Is it about that alc255_hp_init() gets called at resume in addition to
the boot time?  If so, this is the intended behavior.


thanks,

Takashi

> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de> 
> Sent: Tuesday, January 15, 2019 3:54 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 15 Jan 2019 08:43:40 +0100,
> Kailang wrote:
> > 
> > 
> > No, it not expected.
> > It will enter two times after boot ready.
> > I think it could add a flag in alc225_hp_init(). 
> > To make sure it only excute one time. How do you think about?
> 
> Where was it called, actually?  The call of patch_ops.init is found in several places: once in snd_hda_codec_build_controls() just before creating controls (which is the boot time init), and at resume, alc269_resume().
> 
> And, as already explained, we have to call this at resume, because it's not guaranteed that it's been already called in the case of hibernation.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Tuesday, January 15, 2019 2:37 PM
> > To: Kailang <kailang@realtek.com>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: Re: hp_pin was NULL value
> > 
> > On Tue, 15 Jan 2019 03:31:33 +0100,
> > Kailang wrote:
> > > 
> > > Hi Takashi,
> > > 
> > > Could you receive this email?
> > > 
> > > BR,
> > > Kailang
> > > 
> > > -----Original Message-----
> > > From: Kailang
> > > Sent: Thursday, January 10, 2019 11:14 AM
> > > To: 'Takashi Iwai' <tiwai@suse.de>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: RE: hp_pin was NULL value
> > > 
> > > Hi Takashi,
> > > 
> > > I simulate test your prefer function.
> > > I put alc225_hp_init() in alc225_init().
> > > If system boot ready, I write 1 to power_save of /sys/module/....
> > > The print message as below. It called two times until boot ready.
> > > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > > 
> > > [   21.497524] alc225_init hp_pin=0x21
> > > [   21.497526] alc225_hp_init-s hp=0x21
> > > [   22.140076] alc225_hp_init-e hp=0x21
> > > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > [   22.186846] alc225_shutup hp_pin=0x21
> > > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > > [   22.205104] alc225_init hp_pin=0x21
> > > [   22.205108] alc225_hp_init-s hp=0x21
> > > [   22.852123] alc225_hp_init-e hp=0x21
> > > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > ############  Boot ready ############## [  116.832084] alc225_shutup
> > > hp_pin=0x21  ==> echo 1 to power_save
> > > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > > alc225_hp_init-e hp=0x21
> > 
> > It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > ------Please consider the environment before printing this e-mail.
> > 
> 

Re: hp_pin was NULL value

[Kailang]

>>>Is it about that alc225_hp_init() gets called at resume in addition to the boot time?

alc225_hp_init() ==> it only need to call one time at boot time.
This HP initial code doesn't need to call at resume.
If system boot, HP initial code just need to call it at boot time.
S3 state was not need to call this function.

Sorry!! Maybe I describe it confuse you.

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Tuesday, January 15, 2019 4:58 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Tue, 15 Jan 2019 09:17:24 +0100,
Kailang wrote:
> 
> 
> Hi Takashi,
> 
> Sorry!! Maybe I don't understand what you mean.	
> Please see below.
> 
> --- patch_realtek.c.orig	2019-01-11 16:28:43.745957854 +0800
> +++ patch_realtek.c	2019-01-15 16:09:56.017873567 +0800
> @@ -3221,12 +3221,29 @@ static void alc256_shutup(struct hda_cod
>  	snd_hda_shutup_pins(codec);
>  }
>  
> +static void alc225_hp_init(struct hda_codec *codec) {
> +	struct alc_spec *spec = codec->spec;
> +	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> +	int i, val;
> +	int coef38, coef0d, coef36;
> +	printk("%s-start hp=0x%x\n",__func__,hp_pin);
> +	if (!hp_pin)
> +		return;
> +
> +	alc_update_coef_idx(codec, 0x4a, 1<<15, 1<<15);
> +	msleep(50);
> +	alc_update_coef_idx(codec, 0x4a, 1<<15, 0);
> +	printk("%s-end hp=0x%x\n",__func__,hp_pin); }
> +
>  static void alc225_init(struct hda_codec *codec)  {
>  	struct alc_spec *spec = codec->spec;
>  	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
>  	bool hp1_pin_sense, hp2_pin_sense;
>  
> +	alc225_hp_init(codec);
>  	if (!hp_pin)
>  		return;
>  
> @@ -3262,6 +3279,7 @@ static void alc225_init(struct hda_codec
>  
>  	alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
>  	alc_update_coefex_idx(codec, 0x57, 0x04, 0x0007, 0x4); /* Hight 
> power */
> +	printk("%s hp=0x%x\n",__func__,hp_pin);
>  }

... and I don't understand what's your problem.

Is it about that alc255_hp_init() gets called at resume in addition to the boot time?  If so, this is the intended behavior.


thanks,

Takashi

> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Tuesday, January 15, 2019 3:54 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 15 Jan 2019 08:43:40 +0100,
> Kailang wrote:
> > 
> > 
> > No, it not expected.
> > It will enter two times after boot ready.
> > I think it could add a flag in alc225_hp_init(). 
> > To make sure it only excute one time. How do you think about?
> 
> Where was it called, actually?  The call of patch_ops.init is found in several places: once in snd_hda_codec_build_controls() just before creating controls (which is the boot time init), and at resume, alc269_resume().
> 
> And, as already explained, we have to call this at resume, because it's not guaranteed that it's been already called in the case of hibernation.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Tuesday, January 15, 2019 2:37 PM
> > To: Kailang <kailang@realtek.com>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: Re: hp_pin was NULL value
> > 
> > On Tue, 15 Jan 2019 03:31:33 +0100,
> > Kailang wrote:
> > > 
> > > Hi Takashi,
> > > 
> > > Could you receive this email?
> > > 
> > > BR,
> > > Kailang
> > > 
> > > -----Original Message-----
> > > From: Kailang
> > > Sent: Thursday, January 10, 2019 11:14 AM
> > > To: 'Takashi Iwai' <tiwai@suse.de>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: RE: hp_pin was NULL value
> > > 
> > > Hi Takashi,
> > > 
> > > I simulate test your prefer function.
> > > I put alc225_hp_init() in alc225_init().
> > > If system boot ready, I write 1 to power_save of /sys/module/....
> > > The print message as below. It called two times until boot ready.
> > > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > > 
> > > [   21.497524] alc225_init hp_pin=0x21
> > > [   21.497526] alc225_hp_init-s hp=0x21
> > > [   22.140076] alc225_hp_init-e hp=0x21
> > > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > [   22.186846] alc225_shutup hp_pin=0x21
> > > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > > [   22.205104] alc225_init hp_pin=0x21
> > > [   22.205108] alc225_hp_init-s hp=0x21
> > > [   22.852123] alc225_hp_init-e hp=0x21
> > > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > ############  Boot ready ############## [  116.832084] 
> > > alc225_shutup
> > > hp_pin=0x21  ==> echo 1 to power_save
> > > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > > alc225_hp_init-e hp=0x21
> > 
> > It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > ------Please consider the environment before printing this e-mail.
> > 
> 

Re: hp_pin was NULL value

[Takashi Iwai]

On Tue, 15 Jan 2019 10:06:58 +0100,
Kailang wrote:
> 
> >>>Is it about that alc225_hp_init() gets called at resume in addition to the boot time?
> 
> alc225_hp_init() ==> it only need to call one time at boot time.
> This HP initial code doesn't need to call at resume.
> If system boot, HP initial code just need to call it at boot time.
> S3 state was not need to call this function.

But S4 resume must call this function: that's my point.

So the question is whether calling alc225_hp_init() at S3 resume is
significantly harmful or not.  If not, we can move it there.  If it's
harmful, we need to distinguish S3 and S4, and introduce either some
flag or a new callback, which will be a much more work than simply
patching the codec code.


thanks,

Takashi

> Sorry!! Maybe I describe it confuse you.
> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de> 
> Sent: Tuesday, January 15, 2019 4:58 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 15 Jan 2019 09:17:24 +0100,
> Kailang wrote:
> > 
> > 
> > Hi Takashi,
> > 
> > Sorry!! Maybe I don't understand what you mean.	
> > Please see below.
> > 
> > --- patch_realtek.c.orig	2019-01-11 16:28:43.745957854 +0800
> > +++ patch_realtek.c	2019-01-15 16:09:56.017873567 +0800
> > @@ -3221,12 +3221,29 @@ static void alc256_shutup(struct hda_cod
> >  	snd_hda_shutup_pins(codec);
> >  }
> >  
> > +static void alc225_hp_init(struct hda_codec *codec) {
> > +	struct alc_spec *spec = codec->spec;
> > +	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > +	int i, val;
> > +	int coef38, coef0d, coef36;
> > +	printk("%s-start hp=0x%x\n",__func__,hp_pin);
> > +	if (!hp_pin)
> > +		return;
> > +
> > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 1<<15);
> > +	msleep(50);
> > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 0);
> > +	printk("%s-end hp=0x%x\n",__func__,hp_pin); }
> > +
> >  static void alc225_init(struct hda_codec *codec)  {
> >  	struct alc_spec *spec = codec->spec;
> >  	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> >  	bool hp1_pin_sense, hp2_pin_sense;
> >  
> > +	alc225_hp_init(codec);
> >  	if (!hp_pin)
> >  		return;
> >  
> > @@ -3262,6 +3279,7 @@ static void alc225_init(struct hda_codec
> >  
> >  	alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
> >  	alc_update_coefex_idx(codec, 0x57, 0x04, 0x0007, 0x4); /* Hight 
> > power */
> > +	printk("%s hp=0x%x\n",__func__,hp_pin);
> >  }
> 
> ... and I don't understand what's your problem.
> 
> Is it about that alc255_hp_init() gets called at resume in addition to the boot time?  If so, this is the intended behavior.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Tuesday, January 15, 2019 3:54 PM
> > To: Kailang <kailang@realtek.com>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: Re: hp_pin was NULL value
> > 
> > On Tue, 15 Jan 2019 08:43:40 +0100,
> > Kailang wrote:
> > > 
> > > 
> > > No, it not expected.
> > > It will enter two times after boot ready.
> > > I think it could add a flag in alc225_hp_init(). 
> > > To make sure it only excute one time. How do you think about?
> > 
> > Where was it called, actually?  The call of patch_ops.init is found in several places: once in snd_hda_codec_build_controls() just before creating controls (which is the boot time init), and at resume, alc269_resume().
> > 
> > And, as already explained, we have to call this at resume, because it's not guaranteed that it's been already called in the case of hibernation.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > 
> > > -----Original Message-----
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Sent: Tuesday, January 15, 2019 2:37 PM
> > > To: Kailang <kailang@realtek.com>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: Re: hp_pin was NULL value
> > > 
> > > On Tue, 15 Jan 2019 03:31:33 +0100,
> > > Kailang wrote:
> > > > 
> > > > Hi Takashi,
> > > > 
> > > > Could you receive this email?
> > > > 
> > > > BR,
> > > > Kailang
> > > > 
> > > > -----Original Message-----
> > > > From: Kailang
> > > > Sent: Thursday, January 10, 2019 11:14 AM
> > > > To: 'Takashi Iwai' <tiwai@suse.de>
> > > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > > Subject: RE: hp_pin was NULL value
> > > > 
> > > > Hi Takashi,
> > > > 
> > > > I simulate test your prefer function.
> > > > I put alc225_hp_init() in alc225_init().
> > > > If system boot ready, I write 1 to power_save of /sys/module/....
> > > > The print message as below. It called two times until boot ready.
> > > > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > > > 
> > > > [   21.497524] alc225_init hp_pin=0x21
> > > > [   21.497526] alc225_hp_init-s hp=0x21
> > > > [   22.140076] alc225_hp_init-e hp=0x21
> > > > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > [   22.186846] alc225_shutup hp_pin=0x21
> > > > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > > > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > > > [   22.205104] alc225_init hp_pin=0x21
> > > > [   22.205108] alc225_hp_init-s hp=0x21
> > > > [   22.852123] alc225_hp_init-e hp=0x21
> > > > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > ############  Boot ready ############## [  116.832084] 
> > > > alc225_shutup
> > > > hp_pin=0x21  ==> echo 1 to power_save
> > > > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > > > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > > > alc225_hp_init-e hp=0x21
> > > 
> > > It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > ------Please consider the environment before printing this e-mail.
> > > 
> > 
> 

Re: hp_pin was NULL value

[Kailang]

Our Windows code just called it at boot time only. SD's programing guide point just need to call one time.
So, I don't know it's harmful or not. I will check with our AE and SD.
I could also test it with S3 resume state.

Many Thanks. Please wait.

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Tuesday, January 15, 2019 5:17 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Tue, 15 Jan 2019 10:06:58 +0100,
Kailang wrote:
> 
> >>>Is it about that alc225_hp_init() gets called at resume in addition to the boot time?
> 
> alc225_hp_init() ==> it only need to call one time at boot time.
> This HP initial code doesn't need to call at resume.
> If system boot, HP initial code just need to call it at boot time.
> S3 state was not need to call this function.

But S4 resume must call this function: that's my point.

So the question is whether calling alc225_hp_init() at S3 resume is significantly harmful or not.  If not, we can move it there.  If it's harmful, we need to distinguish S3 and S4, and introduce either some flag or a new callback, which will be a much more work than simply patching the codec code.


thanks,

Takashi

> Sorry!! Maybe I describe it confuse you.
> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Tuesday, January 15, 2019 4:58 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 15 Jan 2019 09:17:24 +0100,
> Kailang wrote:
> > 
> > 
> > Hi Takashi,
> > 
> > Sorry!! Maybe I don't understand what you mean.	
> > Please see below.
> > 
> > --- patch_realtek.c.orig	2019-01-11 16:28:43.745957854 +0800
> > +++ patch_realtek.c	2019-01-15 16:09:56.017873567 +0800
> > @@ -3221,12 +3221,29 @@ static void alc256_shutup(struct hda_cod
> >  	snd_hda_shutup_pins(codec);
> >  }
> >  
> > +static void alc225_hp_init(struct hda_codec *codec) {
> > +	struct alc_spec *spec = codec->spec;
> > +	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > +	int i, val;
> > +	int coef38, coef0d, coef36;
> > +	printk("%s-start hp=0x%x\n",__func__,hp_pin);
> > +	if (!hp_pin)
> > +		return;
> > +
> > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 1<<15);
> > +	msleep(50);
> > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 0);
> > +	printk("%s-end hp=0x%x\n",__func__,hp_pin); }
> > +
> >  static void alc225_init(struct hda_codec *codec)  {
> >  	struct alc_spec *spec = codec->spec;
> >  	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> >  	bool hp1_pin_sense, hp2_pin_sense;
> >  
> > +	alc225_hp_init(codec);
> >  	if (!hp_pin)
> >  		return;
> >  
> > @@ -3262,6 +3279,7 @@ static void alc225_init(struct hda_codec
> >  
> >  	alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
> >  	alc_update_coefex_idx(codec, 0x57, 0x04, 0x0007, 0x4); /* Hight 
> > power */
> > +	printk("%s hp=0x%x\n",__func__,hp_pin);
> >  }
> 
> ... and I don't understand what's your problem.
> 
> Is it about that alc255_hp_init() gets called at resume in addition to the boot time?  If so, this is the intended behavior.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Tuesday, January 15, 2019 3:54 PM
> > To: Kailang <kailang@realtek.com>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: Re: hp_pin was NULL value
> > 
> > On Tue, 15 Jan 2019 08:43:40 +0100,
> > Kailang wrote:
> > > 
> > > 
> > > No, it not expected.
> > > It will enter two times after boot ready.
> > > I think it could add a flag in alc225_hp_init(). 
> > > To make sure it only excute one time. How do you think about?
> > 
> > Where was it called, actually?  The call of patch_ops.init is found in several places: once in snd_hda_codec_build_controls() just before creating controls (which is the boot time init), and at resume, alc269_resume().
> > 
> > And, as already explained, we have to call this at resume, because it's not guaranteed that it's been already called in the case of hibernation.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > 
> > > -----Original Message-----
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Sent: Tuesday, January 15, 2019 2:37 PM
> > > To: Kailang <kailang@realtek.com>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: Re: hp_pin was NULL value
> > > 
> > > On Tue, 15 Jan 2019 03:31:33 +0100, Kailang wrote:
> > > > 
> > > > Hi Takashi,
> > > > 
> > > > Could you receive this email?
> > > > 
> > > > BR,
> > > > Kailang
> > > > 
> > > > -----Original Message-----
> > > > From: Kailang
> > > > Sent: Thursday, January 10, 2019 11:14 AM
> > > > To: 'Takashi Iwai' <tiwai@suse.de>
> > > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > > Subject: RE: hp_pin was NULL value
> > > > 
> > > > Hi Takashi,
> > > > 
> > > > I simulate test your prefer function.
> > > > I put alc225_hp_init() in alc225_init().
> > > > If system boot ready, I write 1 to power_save of /sys/module/....
> > > > The print message as below. It called two times until boot ready.
> > > > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > > > 
> > > > [   21.497524] alc225_init hp_pin=0x21
> > > > [   21.497526] alc225_hp_init-s hp=0x21
> > > > [   22.140076] alc225_hp_init-e hp=0x21
> > > > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > [   22.186846] alc225_shutup hp_pin=0x21
> > > > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > > > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > > > [   22.205104] alc225_init hp_pin=0x21
> > > > [   22.205108] alc225_hp_init-s hp=0x21
> > > > [   22.852123] alc225_hp_init-e hp=0x21
> > > > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > ############  Boot ready ############## [  116.832084] 
> > > > alc225_shutup
> > > > hp_pin=0x21  ==> echo 1 to power_save
> > > > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > > > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > > > alc225_hp_init-e hp=0x21
> > > 
> > > It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > ------Please consider the environment before printing this e-mail.
> > > 
> > 
> 

Re: hp_pin was NULL value

[Kailang]

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

Hi Takashi,

I ask our AE for this function.
We also prefer to run it only one time on boot time. S3 resume back not need to run this function.
Because it will take about 650ms times to initial.

Attach was the new patch.
Reject commit or use attach patch to commit. Which is better?

BR,
Kailang

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Tuesday, January 15, 2019 5:17 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Tue, 15 Jan 2019 10:06:58 +0100,
Kailang wrote:
> 
> >>>Is it about that alc225_hp_init() gets called at resume in addition to the boot time?
> 
> alc225_hp_init() ==> it only need to call one time at boot time.
> This HP initial code doesn't need to call at resume.
> If system boot, HP initial code just need to call it at boot time.
> S3 state was not need to call this function.

But S4 resume must call this function: that's my point.

So the question is whether calling alc225_hp_init() at S3 resume is significantly harmful or not.  If not, we can move it there.  If it's harmful, we need to distinguish S3 and S4, and introduce either some flag or a new callback, which will be a much more work than simply patching the codec code.


thanks,

Takashi

> Sorry!! Maybe I describe it confuse you.
> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Tuesday, January 15, 2019 4:58 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 15 Jan 2019 09:17:24 +0100,
> Kailang wrote:
> > 
> > 
> > Hi Takashi,
> > 
> > Sorry!! Maybe I don't understand what you mean.	
> > Please see below.
> > 
> > --- patch_realtek.c.orig	2019-01-11 16:28:43.745957854 +0800
> > +++ patch_realtek.c	2019-01-15 16:09:56.017873567 +0800
> > @@ -3221,12 +3221,29 @@ static void alc256_shutup(struct hda_cod
> >  	snd_hda_shutup_pins(codec);
> >  }
> >  
> > +static void alc225_hp_init(struct hda_codec *codec) {
> > +	struct alc_spec *spec = codec->spec;
> > +	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > +	int i, val;
> > +	int coef38, coef0d, coef36;
> > +	printk("%s-start hp=0x%x\n",__func__,hp_pin);
> > +	if (!hp_pin)
> > +		return;
> > +
> > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 1<<15);
> > +	msleep(50);
> > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 0);
> > +	printk("%s-end hp=0x%x\n",__func__,hp_pin); }
> > +
> >  static void alc225_init(struct hda_codec *codec)  {
> >  	struct alc_spec *spec = codec->spec;
> >  	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> >  	bool hp1_pin_sense, hp2_pin_sense;
> >  
> > +	alc225_hp_init(codec);
> >  	if (!hp_pin)
> >  		return;
> >  
> > @@ -3262,6 +3279,7 @@ static void alc225_init(struct hda_codec
> >  
> >  	alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
> >  	alc_update_coefex_idx(codec, 0x57, 0x04, 0x0007, 0x4); /* Hight 
> > power */
> > +	printk("%s hp=0x%x\n",__func__,hp_pin);
> >  }
> 
> ... and I don't understand what's your problem.
> 
> Is it about that alc255_hp_init() gets called at resume in addition to the boot time?  If so, this is the intended behavior.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Tuesday, January 15, 2019 3:54 PM
> > To: Kailang <kailang@realtek.com>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: Re: hp_pin was NULL value
> > 
> > On Tue, 15 Jan 2019 08:43:40 +0100,
> > Kailang wrote:
> > > 
> > > 
> > > No, it not expected.
> > > It will enter two times after boot ready.
> > > I think it could add a flag in alc225_hp_init(). 
> > > To make sure it only excute one time. How do you think about?
> > 
> > Where was it called, actually?  The call of patch_ops.init is found in several places: once in snd_hda_codec_build_controls() just before creating controls (which is the boot time init), and at resume, alc269_resume().
> > 
> > And, as already explained, we have to call this at resume, because it's not guaranteed that it's been already called in the case of hibernation.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > 
> > > -----Original Message-----
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Sent: Tuesday, January 15, 2019 2:37 PM
> > > To: Kailang <kailang@realtek.com>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: Re: hp_pin was NULL value
> > > 
> > > On Tue, 15 Jan 2019 03:31:33 +0100, Kailang wrote:
> > > > 
> > > > Hi Takashi,
> > > > 
> > > > Could you receive this email?
> > > > 
> > > > BR,
> > > > Kailang
> > > > 
> > > > -----Original Message-----
> > > > From: Kailang
> > > > Sent: Thursday, January 10, 2019 11:14 AM
> > > > To: 'Takashi Iwai' <tiwai@suse.de>
> > > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > > Subject: RE: hp_pin was NULL value
> > > > 
> > > > Hi Takashi,
> > > > 
> > > > I simulate test your prefer function.
> > > > I put alc225_hp_init() in alc225_init().
> > > > If system boot ready, I write 1 to power_save of /sys/module/....
> > > > The print message as below. It called two times until boot ready.
> > > > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > > > 
> > > > [   21.497524] alc225_init hp_pin=0x21
> > > > [   21.497526] alc225_hp_init-s hp=0x21
> > > > [   22.140076] alc225_hp_init-e hp=0x21
> > > > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > [   22.186846] alc225_shutup hp_pin=0x21
> > > > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > > > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > > > [   22.205104] alc225_init hp_pin=0x21
> > > > [   22.205108] alc225_hp_init-s hp=0x21
> > > > [   22.852123] alc225_hp_init-e hp=0x21
> > > > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > ############  Boot ready ############## [  116.832084] 
> > > > alc225_shutup
> > > > hp_pin=0x21  ==> echo 1 to power_save
> > > > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > > > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > > > alc225_hp_init-e hp=0x21
> > > 
> > > It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > ------Please consider the environment before printing this e-mail.
> > > 
> > 
> 

[-- Attachment #2: 0000-alc294-hp-init-again.patch --]
[-- Type: application/octet-stream, Size: 3944 bytes --]

From fbb94a345004844ad8858a1caece000713a4f9cf Mon Sep 17 00:00:00 2001
From: Kailang Yang <kailang@realtek.com>
Date: Tue, 29 Jan 2019 15:38:21 +0800
Subject: [PATCH] ALSA: hda/realtek - Fixed hp_pin no value

Fix hp_pin always no value.

Fixes: bde1a7459623a66c2abec4d0a841e4b06cc88d9a ('ALSA: hda/realtek - Fixed headphone issue for ALC700')
Signed-off-by: Kailang Yang <kailang@realtek.com>

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index b4f4721..4139ace 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -117,6 +117,7 @@ struct alc_spec {
 	int codec_variant;	/* flag for other variants */
 	unsigned int has_alc5505_dsp:1;
 	unsigned int no_depop_delay:1;
+	unsigned int done_hp_init:1;
 
 	/* for PLL fix */
 	hda_nid_t pll_nid;
@@ -3372,6 +3373,48 @@ static void alc_default_shutup(struct hda_codec *codec)
 	snd_hda_shutup_pins(codec);
 }
 
+static void alc294_hp_init(struct hda_codec *codec)
+{
+	struct alc_spec *spec = codec->spec;
+	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
+	int i, val;
+
+	if (!hp_pin)
+		return;
+
+	snd_hda_codec_write(codec, hp_pin, 0,
+			    AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_MUTE);
+
+	msleep(100);
+
+	snd_hda_codec_write(codec, hp_pin, 0,
+			    AC_VERB_SET_PIN_WIDGET_CONTROL, 0x0);
+
+	alc_update_coef_idx(codec, 0x6f, 0x000f, 0);/* Set HP depop to manual mode */
+	alc_update_coefex_idx(codec, 0x58, 0x00, 0x8000, 0x8000); /* HP depop procedure start */
+
+	/* Wait for depop procedure finish  */
+	val = alc_read_coefex_idx(codec, 0x58, 0x01);
+	for (i = 0; i < 20 && val & 0x0080; i++) {
+		msleep(50);
+		val = alc_read_coefex_idx(codec, 0x58, 0x01);
+	}
+	/* Set HP depop to auto mode */
+	alc_update_coef_idx(codec, 0x6f, 0x000f, 0x000b);
+	msleep(50);
+}
+
+static void alc294_init(struct hda_codec *codec)
+{
+	struct alc_spec *spec = codec->spec;
+
+	if (!spec->done_hp_init) {
+		alc294_hp_init(codec);
+		spec->done_hp_init = true;
+	}
+	alc_default_init(codec);
+}
+
 static void alc5505_coef_set(struct hda_codec *codec, unsigned int index_reg,
 			     unsigned int val)
 {
@@ -7373,37 +7416,6 @@ static void alc269_fill_coef(struct hda_codec *codec)
 	alc_update_coef_idx(codec, 0x4, 0, 1<<11);
 }
 
-static void alc294_hp_init(struct hda_codec *codec)
-{
-	struct alc_spec *spec = codec->spec;
-	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
-	int i, val;
-
-	if (!hp_pin)
-		return;
-
-	snd_hda_codec_write(codec, hp_pin, 0,
-			    AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_MUTE);
-
-	msleep(100);
-
-	snd_hda_codec_write(codec, hp_pin, 0,
-			    AC_VERB_SET_PIN_WIDGET_CONTROL, 0x0);
-
-	alc_update_coef_idx(codec, 0x6f, 0x000f, 0);/* Set HP depop to manual mode */
-	alc_update_coefex_idx(codec, 0x58, 0x00, 0x8000, 0x8000); /* HP depop procedure start */
-
-	/* Wait for depop procedure finish  */
-	val = alc_read_coefex_idx(codec, 0x58, 0x01);
-	for (i = 0; i < 20 && val & 0x0080; i++) {
-		msleep(50);
-		val = alc_read_coefex_idx(codec, 0x58, 0x01);
-	}
-	/* Set HP depop to auto mode */
-	alc_update_coef_idx(codec, 0x6f, 0x000f, 0x000b);
-	msleep(50);
-}
-
 /*
  */
 static int patch_alc269(struct hda_codec *codec)
@@ -7529,7 +7541,7 @@ static int patch_alc269(struct hda_codec *codec)
 		spec->codec_variant = ALC269_TYPE_ALC294;
 		spec->gen.mixer_nid = 0; /* ALC2x4 does not have any loopback mixer path */
 		alc_update_coef_idx(codec, 0x6b, 0x0018, (1<<4) | (1<<3)); /* UAJ MIC Vref control by verb */
-		alc294_hp_init(codec);
+		spec->init_hook = alc294_init;
 		break;
 	case 0x10ec0300:
 		spec->codec_variant = ALC269_TYPE_ALC300;
@@ -7541,7 +7553,7 @@ static int patch_alc269(struct hda_codec *codec)
 		spec->codec_variant = ALC269_TYPE_ALC700;
 		spec->gen.mixer_nid = 0; /* ALC700 does not have any loopback mixer path */
 		alc_update_coef_idx(codec, 0x4a, 1 << 15, 0); /* Combo jack auto trigger control */
-		alc294_hp_init(codec);
+		spec->init_hook = alc294_init;
 		break;
 
 	}

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: hp_pin was NULL value

[Takashi Iwai]

On Tue, 29 Jan 2019 09:05:48 +0100,
Kailang wrote:
> 
> Hi Takashi,
> 
> I ask our AE for this function.
> We also prefer to run it only one time on boot time. S3 resume back not need to run this function.
> Because it will take about 650ms times to initial.
> 
> Attach was the new patch.
> Reject commit or use attach patch to commit. Which is better?

This won't work with S4, unfortunately.

The point of S4 problem is that the switch happens from the
uninitialized state to the already running system via a single resume
callback.  That is, the driver was already initialized in the restored
image although the chip is still uninitialized on the booted image
before the switch.  Then switch happens, and since it keeps
done_hp_init=1 at S4 resume call, the whole trick won't be applied at
all.

I'll check whether we have some indicator showing that the call is in
thaw/restore, or we'd have to implement a separate thaw/restore
callback.


thanks,

Takashi

> 
> BR,
> Kailang
> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de> 
> Sent: Tuesday, January 15, 2019 5:17 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 15 Jan 2019 10:06:58 +0100,
> Kailang wrote:
> > 
> > >>>Is it about that alc225_hp_init() gets called at resume in addition to the boot time?
> > 
> > alc225_hp_init() ==> it only need to call one time at boot time.
> > This HP initial code doesn't need to call at resume.
> > If system boot, HP initial code just need to call it at boot time.
> > S3 state was not need to call this function.
> 
> But S4 resume must call this function: that's my point.
> 
> So the question is whether calling alc225_hp_init() at S3 resume is significantly harmful or not.  If not, we can move it there.  If it's harmful, we need to distinguish S3 and S4, and introduce either some flag or a new callback, which will be a much more work than simply patching the codec code.
> 
> 
> thanks,
> 
> Takashi
> 
> > Sorry!! Maybe I describe it confuse you.
> > 
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Tuesday, January 15, 2019 4:58 PM
> > To: Kailang <kailang@realtek.com>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: Re: hp_pin was NULL value
> > 
> > On Tue, 15 Jan 2019 09:17:24 +0100,
> > Kailang wrote:
> > > 
> > > 
> > > Hi Takashi,
> > > 
> > > Sorry!! Maybe I don't understand what you mean.	
> > > Please see below.
> > > 
> > > --- patch_realtek.c.orig	2019-01-11 16:28:43.745957854 +0800
> > > +++ patch_realtek.c	2019-01-15 16:09:56.017873567 +0800
> > > @@ -3221,12 +3221,29 @@ static void alc256_shutup(struct hda_cod
> > >  	snd_hda_shutup_pins(codec);
> > >  }
> > >  
> > > +static void alc225_hp_init(struct hda_codec *codec) {
> > > +	struct alc_spec *spec = codec->spec;
> > > +	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > > +	int i, val;
> > > +	int coef38, coef0d, coef36;
> > > +	printk("%s-start hp=0x%x\n",__func__,hp_pin);
> > > +	if (!hp_pin)
> > > +		return;
> > > +
> > > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 1<<15);
> > > +	msleep(50);
> > > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 0);
> > > +	printk("%s-end hp=0x%x\n",__func__,hp_pin); }
> > > +
> > >  static void alc225_init(struct hda_codec *codec)  {
> > >  	struct alc_spec *spec = codec->spec;
> > >  	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > >  	bool hp1_pin_sense, hp2_pin_sense;
> > >  
> > > +	alc225_hp_init(codec);
> > >  	if (!hp_pin)
> > >  		return;
> > >  
> > > @@ -3262,6 +3279,7 @@ static void alc225_init(struct hda_codec
> > >  
> > >  	alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
> > >  	alc_update_coefex_idx(codec, 0x57, 0x04, 0x0007, 0x4); /* Hight 
> > > power */
> > > +	printk("%s hp=0x%x\n",__func__,hp_pin);
> > >  }
> > 
> > ... and I don't understand what's your problem.
> > 
> > Is it about that alc255_hp_init() gets called at resume in addition to the boot time?  If so, this is the intended behavior.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > 
> > > -----Original Message-----
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Sent: Tuesday, January 15, 2019 3:54 PM
> > > To: Kailang <kailang@realtek.com>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: Re: hp_pin was NULL value
> > > 
> > > On Tue, 15 Jan 2019 08:43:40 +0100,
> > > Kailang wrote:
> > > > 
> > > > 
> > > > No, it not expected.
> > > > It will enter two times after boot ready.
> > > > I think it could add a flag in alc225_hp_init(). 
> > > > To make sure it only excute one time. How do you think about?
> > > 
> > > Where was it called, actually?  The call of patch_ops.init is found in several places: once in snd_hda_codec_build_controls() just before creating controls (which is the boot time init), and at resume, alc269_resume().
> > > 
> > > And, as already explained, we have to call this at resume, because it's not guaranteed that it's been already called in the case of hibernation.
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > > 
> > > > -----Original Message-----
> > > > From: Takashi Iwai <tiwai@suse.de>
> > > > Sent: Tuesday, January 15, 2019 2:37 PM
> > > > To: Kailang <kailang@realtek.com>
> > > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > > Subject: Re: hp_pin was NULL value
> > > > 
> > > > On Tue, 15 Jan 2019 03:31:33 +0100, Kailang wrote:
> > > > > 
> > > > > Hi Takashi,
> > > > > 
> > > > > Could you receive this email?
> > > > > 
> > > > > BR,
> > > > > Kailang
> > > > > 
> > > > > -----Original Message-----
> > > > > From: Kailang
> > > > > Sent: Thursday, January 10, 2019 11:14 AM
> > > > > To: 'Takashi Iwai' <tiwai@suse.de>
> > > > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > > > Subject: RE: hp_pin was NULL value
> > > > > 
> > > > > Hi Takashi,
> > > > > 
> > > > > I simulate test your prefer function.
> > > > > I put alc225_hp_init() in alc225_init().
> > > > > If system boot ready, I write 1 to power_save of /sys/module/....
> > > > > The print message as below. It called two times until boot ready.
> > > > > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > > > > 
> > > > > [   21.497524] alc225_init hp_pin=0x21
> > > > > [   21.497526] alc225_hp_init-s hp=0x21
> > > > > [   22.140076] alc225_hp_init-e hp=0x21
> > > > > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > [   22.186846] alc225_shutup hp_pin=0x21
> > > > > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > > > > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > > > > [   22.205104] alc225_init hp_pin=0x21
> > > > > [   22.205108] alc225_hp_init-s hp=0x21
> > > > > [   22.852123] alc225_hp_init-e hp=0x21
> > > > > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > ############  Boot ready ############## [  116.832084] 
> > > > > alc225_shutup
> > > > > hp_pin=0x21  ==> echo 1 to power_save
> > > > > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > > > > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > > > > alc225_hp_init-e hp=0x21
> > > > 
> > > > It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> > > > 
> > > > 
> > > > thanks,
> > > > 
> > > > Takashi
> > > > 
> > > > ------Please consider the environment before printing this e-mail.
> > > > 
> > > 
> > 
> [2 0000-alc294-hp-init-again.patch <application/octet-stream (base64)>]
> 

Re: hp_pin was NULL value

[Kailang]

I use Ubuntu to do the test.
I can't find S4 function in Ubuntu OS.
Does Yours OS support the S4 function?

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Tuesday, January 29, 2019 4:29 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Tue, 29 Jan 2019 09:05:48 +0100,
Kailang wrote:
> 
> Hi Takashi,
> 
> I ask our AE for this function.
> We also prefer to run it only one time on boot time. S3 resume back not need to run this function.
> Because it will take about 650ms times to initial.
> 
> Attach was the new patch.
> Reject commit or use attach patch to commit. Which is better?

This won't work with S4, unfortunately.

The point of S4 problem is that the switch happens from the uninitialized state to the already running system via a single resume callback.  That is, the driver was already initialized in the restored image although the chip is still uninitialized on the booted image before the switch.  Then switch happens, and since it keeps
done_hp_init=1 at S4 resume call, the whole trick won't be applied at all.

I'll check whether we have some indicator showing that the call is in thaw/restore, or we'd have to implement a separate thaw/restore callback.


thanks,

Takashi

> 
> BR,
> Kailang
> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Tuesday, January 15, 2019 5:17 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 15 Jan 2019 10:06:58 +0100,
> Kailang wrote:
> > 
> > >>>Is it about that alc225_hp_init() gets called at resume in addition to the boot time?
> > 
> > alc225_hp_init() ==> it only need to call one time at boot time.
> > This HP initial code doesn't need to call at resume.
> > If system boot, HP initial code just need to call it at boot time.
> > S3 state was not need to call this function.
> 
> But S4 resume must call this function: that's my point.
> 
> So the question is whether calling alc225_hp_init() at S3 resume is significantly harmful or not.  If not, we can move it there.  If it's harmful, we need to distinguish S3 and S4, and introduce either some flag or a new callback, which will be a much more work than simply patching the codec code.
> 
> 
> thanks,
> 
> Takashi
> 
> > Sorry!! Maybe I describe it confuse you.
> > 
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Tuesday, January 15, 2019 4:58 PM
> > To: Kailang <kailang@realtek.com>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: Re: hp_pin was NULL value
> > 
> > On Tue, 15 Jan 2019 09:17:24 +0100,
> > Kailang wrote:
> > > 
> > > 
> > > Hi Takashi,
> > > 
> > > Sorry!! Maybe I don't understand what you mean.	
> > > Please see below.
> > > 
> > > --- patch_realtek.c.orig	2019-01-11 16:28:43.745957854 +0800
> > > +++ patch_realtek.c	2019-01-15 16:09:56.017873567 +0800
> > > @@ -3221,12 +3221,29 @@ static void alc256_shutup(struct hda_cod
> > >  	snd_hda_shutup_pins(codec);
> > >  }
> > >  
> > > +static void alc225_hp_init(struct hda_codec *codec) {
> > > +	struct alc_spec *spec = codec->spec;
> > > +	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > > +	int i, val;
> > > +	int coef38, coef0d, coef36;
> > > +	printk("%s-start hp=0x%x\n",__func__,hp_pin);
> > > +	if (!hp_pin)
> > > +		return;
> > > +
> > > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 1<<15);
> > > +	msleep(50);
> > > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 0);
> > > +	printk("%s-end hp=0x%x\n",__func__,hp_pin); }
> > > +
> > >  static void alc225_init(struct hda_codec *codec)  {
> > >  	struct alc_spec *spec = codec->spec;
> > >  	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > >  	bool hp1_pin_sense, hp2_pin_sense;
> > >  
> > > +	alc225_hp_init(codec);
> > >  	if (!hp_pin)
> > >  		return;
> > >  
> > > @@ -3262,6 +3279,7 @@ static void alc225_init(struct hda_codec
> > >  
> > >  	alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
> > >  	alc_update_coefex_idx(codec, 0x57, 0x04, 0x0007, 0x4); /* Hight 
> > > power */
> > > +	printk("%s hp=0x%x\n",__func__,hp_pin);
> > >  }
> > 
> > ... and I don't understand what's your problem.
> > 
> > Is it about that alc255_hp_init() gets called at resume in addition to the boot time?  If so, this is the intended behavior.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > 
> > > -----Original Message-----
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Sent: Tuesday, January 15, 2019 3:54 PM
> > > To: Kailang <kailang@realtek.com>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: Re: hp_pin was NULL value
> > > 
> > > On Tue, 15 Jan 2019 08:43:40 +0100, Kailang wrote:
> > > > 
> > > > 
> > > > No, it not expected.
> > > > It will enter two times after boot ready.
> > > > I think it could add a flag in alc225_hp_init(). 
> > > > To make sure it only excute one time. How do you think about?
> > > 
> > > Where was it called, actually?  The call of patch_ops.init is found in several places: once in snd_hda_codec_build_controls() just before creating controls (which is the boot time init), and at resume, alc269_resume().
> > > 
> > > And, as already explained, we have to call this at resume, because it's not guaranteed that it's been already called in the case of hibernation.
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > > 
> > > > -----Original Message-----
> > > > From: Takashi Iwai <tiwai@suse.de>
> > > > Sent: Tuesday, January 15, 2019 2:37 PM
> > > > To: Kailang <kailang@realtek.com>
> > > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > > Subject: Re: hp_pin was NULL value
> > > > 
> > > > On Tue, 15 Jan 2019 03:31:33 +0100, Kailang wrote:
> > > > > 
> > > > > Hi Takashi,
> > > > > 
> > > > > Could you receive this email?
> > > > > 
> > > > > BR,
> > > > > Kailang
> > > > > 
> > > > > -----Original Message-----
> > > > > From: Kailang
> > > > > Sent: Thursday, January 10, 2019 11:14 AM
> > > > > To: 'Takashi Iwai' <tiwai@suse.de>
> > > > > Cc: (alsa-devel@alsa-project.org) 
> > > > > <alsa-devel@alsa-project.org>
> > > > > Subject: RE: hp_pin was NULL value
> > > > > 
> > > > > Hi Takashi,
> > > > > 
> > > > > I simulate test your prefer function.
> > > > > I put alc225_hp_init() in alc225_init().
> > > > > If system boot ready, I write 1 to power_save of /sys/module/....
> > > > > The print message as below. It called two times until boot ready.
> > > > > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > > > > 
> > > > > [   21.497524] alc225_init hp_pin=0x21
> > > > > [   21.497526] alc225_hp_init-s hp=0x21
> > > > > [   22.140076] alc225_hp_init-e hp=0x21
> > > > > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > [   22.186846] alc225_shutup hp_pin=0x21
> > > > > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > > > > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > > > > [   22.205104] alc225_init hp_pin=0x21
> > > > > [   22.205108] alc225_hp_init-s hp=0x21
> > > > > [   22.852123] alc225_hp_init-e hp=0x21
> > > > > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > ############  Boot ready ############## [  116.832084] 
> > > > > alc225_shutup
> > > > > hp_pin=0x21  ==> echo 1 to power_save
> > > > > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > > > > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > > > > alc225_hp_init-e hp=0x21
> > > > 
> > > > It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> > > > 
> > > > 
> > > > thanks,
> > > > 
> > > > Takashi
> > > > 
> > > > ------Please consider the environment before printing this e-mail.
> > > > 
> > > 
> > 
> [2 0000-alc294-hp-init-again.patch <application/octet-stream 
> (base64)>]
> 

Re: hp_pin was NULL value

[Kailang]

Hi Takashi,

So I think put it under alc269 parser. Maybe it is the quickly method.

	err = alc269_parse_auto_config(codec);
	if (err < 0)
		goto error;
+   .....
+   .....

BR,
kailang
-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Tuesday, January 29, 2019 4:29 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Tue, 29 Jan 2019 09:05:48 +0100,
Kailang wrote:
> 
> Hi Takashi,
> 
> I ask our AE for this function.
> We also prefer to run it only one time on boot time. S3 resume back not need to run this function.
> Because it will take about 650ms times to initial.
> 
> Attach was the new patch.
> Reject commit or use attach patch to commit. Which is better?

This won't work with S4, unfortunately.

The point of S4 problem is that the switch happens from the uninitialized state to the already running system via a single resume callback.  That is, the driver was already initialized in the restored image although the chip is still uninitialized on the booted image before the switch.  Then switch happens, and since it keeps
done_hp_init=1 at S4 resume call, the whole trick won't be applied at all.

I'll check whether we have some indicator showing that the call is in thaw/restore, or we'd have to implement a separate thaw/restore callback.


thanks,

Takashi

> 
> BR,
> Kailang
> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Tuesday, January 15, 2019 5:17 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 15 Jan 2019 10:06:58 +0100,
> Kailang wrote:
> > 
> > >>>Is it about that alc225_hp_init() gets called at resume in addition to the boot time?
> > 
> > alc225_hp_init() ==> it only need to call one time at boot time.
> > This HP initial code doesn't need to call at resume.
> > If system boot, HP initial code just need to call it at boot time.
> > S3 state was not need to call this function.
> 
> But S4 resume must call this function: that's my point.
> 
> So the question is whether calling alc225_hp_init() at S3 resume is significantly harmful or not.  If not, we can move it there.  If it's harmful, we need to distinguish S3 and S4, and introduce either some flag or a new callback, which will be a much more work than simply patching the codec code.
> 
> 
> thanks,
> 
> Takashi
> 
> > Sorry!! Maybe I describe it confuse you.
> > 
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Tuesday, January 15, 2019 4:58 PM
> > To: Kailang <kailang@realtek.com>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: Re: hp_pin was NULL value
> > 
> > On Tue, 15 Jan 2019 09:17:24 +0100,
> > Kailang wrote:
> > > 
> > > 
> > > Hi Takashi,
> > > 
> > > Sorry!! Maybe I don't understand what you mean.	
> > > Please see below.
> > > 
> > > --- patch_realtek.c.orig	2019-01-11 16:28:43.745957854 +0800
> > > +++ patch_realtek.c	2019-01-15 16:09:56.017873567 +0800
> > > @@ -3221,12 +3221,29 @@ static void alc256_shutup(struct hda_cod
> > >  	snd_hda_shutup_pins(codec);
> > >  }
> > >  
> > > +static void alc225_hp_init(struct hda_codec *codec) {
> > > +	struct alc_spec *spec = codec->spec;
> > > +	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > > +	int i, val;
> > > +	int coef38, coef0d, coef36;
> > > +	printk("%s-start hp=0x%x\n",__func__,hp_pin);
> > > +	if (!hp_pin)
> > > +		return;
> > > +
> > > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 1<<15);
> > > +	msleep(50);
> > > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 0);
> > > +	printk("%s-end hp=0x%x\n",__func__,hp_pin); }
> > > +
> > >  static void alc225_init(struct hda_codec *codec)  {
> > >  	struct alc_spec *spec = codec->spec;
> > >  	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > >  	bool hp1_pin_sense, hp2_pin_sense;
> > >  
> > > +	alc225_hp_init(codec);
> > >  	if (!hp_pin)
> > >  		return;
> > >  
> > > @@ -3262,6 +3279,7 @@ static void alc225_init(struct hda_codec
> > >  
> > >  	alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
> > >  	alc_update_coefex_idx(codec, 0x57, 0x04, 0x0007, 0x4); /* Hight 
> > > power */
> > > +	printk("%s hp=0x%x\n",__func__,hp_pin);
> > >  }
> > 
> > ... and I don't understand what's your problem.
> > 
> > Is it about that alc255_hp_init() gets called at resume in addition to the boot time?  If so, this is the intended behavior.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > 
> > > -----Original Message-----
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Sent: Tuesday, January 15, 2019 3:54 PM
> > > To: Kailang <kailang@realtek.com>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: Re: hp_pin was NULL value
> > > 
> > > On Tue, 15 Jan 2019 08:43:40 +0100, Kailang wrote:
> > > > 
> > > > 
> > > > No, it not expected.
> > > > It will enter two times after boot ready.
> > > > I think it could add a flag in alc225_hp_init(). 
> > > > To make sure it only excute one time. How do you think about?
> > > 
> > > Where was it called, actually?  The call of patch_ops.init is found in several places: once in snd_hda_codec_build_controls() just before creating controls (which is the boot time init), and at resume, alc269_resume().
> > > 
> > > And, as already explained, we have to call this at resume, because it's not guaranteed that it's been already called in the case of hibernation.
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > > 
> > > > -----Original Message-----
> > > > From: Takashi Iwai <tiwai@suse.de>
> > > > Sent: Tuesday, January 15, 2019 2:37 PM
> > > > To: Kailang <kailang@realtek.com>
> > > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > > Subject: Re: hp_pin was NULL value
> > > > 
> > > > On Tue, 15 Jan 2019 03:31:33 +0100, Kailang wrote:
> > > > > 
> > > > > Hi Takashi,
> > > > > 
> > > > > Could you receive this email?
> > > > > 
> > > > > BR,
> > > > > Kailang
> > > > > 
> > > > > -----Original Message-----
> > > > > From: Kailang
> > > > > Sent: Thursday, January 10, 2019 11:14 AM
> > > > > To: 'Takashi Iwai' <tiwai@suse.de>
> > > > > Cc: (alsa-devel@alsa-project.org) 
> > > > > <alsa-devel@alsa-project.org>
> > > > > Subject: RE: hp_pin was NULL value
> > > > > 
> > > > > Hi Takashi,
> > > > > 
> > > > > I simulate test your prefer function.
> > > > > I put alc225_hp_init() in alc225_init().
> > > > > If system boot ready, I write 1 to power_save of /sys/module/....
> > > > > The print message as below. It called two times until boot ready.
> > > > > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > > > > 
> > > > > [   21.497524] alc225_init hp_pin=0x21
> > > > > [   21.497526] alc225_hp_init-s hp=0x21
> > > > > [   22.140076] alc225_hp_init-e hp=0x21
> > > > > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > [   22.186846] alc225_shutup hp_pin=0x21
> > > > > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > > > > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > > > > [   22.205104] alc225_init hp_pin=0x21
> > > > > [   22.205108] alc225_hp_init-s hp=0x21
> > > > > [   22.852123] alc225_hp_init-e hp=0x21
> > > > > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > ############  Boot ready ############## [  116.832084] 
> > > > > alc225_shutup
> > > > > hp_pin=0x21  ==> echo 1 to power_save
> > > > > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > > > > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > > > > alc225_hp_init-e hp=0x21
> > > > 
> > > > It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> > > > 
> > > > 
> > > > thanks,
> > > > 
> > > > Takashi
> > > > 
> > > > ------Please consider the environment before printing this e-mail.
> > > > 
> > > 
> > 
> [2 0000-alc294-hp-init-again.patch <application/octet-stream 
> (base64)>]
> 

Re: hp_pin was NULL value

[Takashi Iwai]

On Tue, 29 Jan 2019 09:32:47 +0100,
Kailang wrote:
> 
> I use Ubuntu to do the test.
> I can't find S4 function in Ubuntu OS.
> Does Yours OS support the S4 function?

Yes.  And I'm surprised that Ubuntu doesn't support hibernate.
Doesn't "systemctl hibernate" work?


Takashi

> 
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de> 
> Sent: Tuesday, January 29, 2019 4:29 PM
> To: Kailang <kailang@realtek.com>
> Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> Subject: Re: hp_pin was NULL value
> 
> On Tue, 29 Jan 2019 09:05:48 +0100,
> Kailang wrote:
> > 
> > Hi Takashi,
> > 
> > I ask our AE for this function.
> > We also prefer to run it only one time on boot time. S3 resume back not need to run this function.
> > Because it will take about 650ms times to initial.
> > 
> > Attach was the new patch.
> > Reject commit or use attach patch to commit. Which is better?
> 
> This won't work with S4, unfortunately.
> 
> The point of S4 problem is that the switch happens from the uninitialized state to the already running system via a single resume callback.  That is, the driver was already initialized in the restored image although the chip is still uninitialized on the booted image before the switch.  Then switch happens, and since it keeps
> done_hp_init=1 at S4 resume call, the whole trick won't be applied at all.
> 
> I'll check whether we have some indicator showing that the call is in thaw/restore, or we'd have to implement a separate thaw/restore callback.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > BR,
> > Kailang
> > 
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Tuesday, January 15, 2019 5:17 PM
> > To: Kailang <kailang@realtek.com>
> > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > Subject: Re: hp_pin was NULL value
> > 
> > On Tue, 15 Jan 2019 10:06:58 +0100,
> > Kailang wrote:
> > > 
> > > >>>Is it about that alc225_hp_init() gets called at resume in addition to the boot time?
> > > 
> > > alc225_hp_init() ==> it only need to call one time at boot time.
> > > This HP initial code doesn't need to call at resume.
> > > If system boot, HP initial code just need to call it at boot time.
> > > S3 state was not need to call this function.
> > 
> > But S4 resume must call this function: that's my point.
> > 
> > So the question is whether calling alc225_hp_init() at S3 resume is significantly harmful or not.  If not, we can move it there.  If it's harmful, we need to distinguish S3 and S4, and introduce either some flag or a new callback, which will be a much more work than simply patching the codec code.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > Sorry!! Maybe I describe it confuse you.
> > > 
> > > -----Original Message-----
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Sent: Tuesday, January 15, 2019 4:58 PM
> > > To: Kailang <kailang@realtek.com>
> > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > Subject: Re: hp_pin was NULL value
> > > 
> > > On Tue, 15 Jan 2019 09:17:24 +0100,
> > > Kailang wrote:
> > > > 
> > > > 
> > > > Hi Takashi,
> > > > 
> > > > Sorry!! Maybe I don't understand what you mean.	
> > > > Please see below.
> > > > 
> > > > --- patch_realtek.c.orig	2019-01-11 16:28:43.745957854 +0800
> > > > +++ patch_realtek.c	2019-01-15 16:09:56.017873567 +0800
> > > > @@ -3221,12 +3221,29 @@ static void alc256_shutup(struct hda_cod
> > > >  	snd_hda_shutup_pins(codec);
> > > >  }
> > > >  
> > > > +static void alc225_hp_init(struct hda_codec *codec) {
> > > > +	struct alc_spec *spec = codec->spec;
> > > > +	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > > > +	int i, val;
> > > > +	int coef38, coef0d, coef36;
> > > > +	printk("%s-start hp=0x%x\n",__func__,hp_pin);
> > > > +	if (!hp_pin)
> > > > +		return;
> > > > +
> > > > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 1<<15);
> > > > +	msleep(50);
> > > > +	alc_update_coef_idx(codec, 0x4a, 1<<15, 0);
> > > > +	printk("%s-end hp=0x%x\n",__func__,hp_pin); }
> > > > +
> > > >  static void alc225_init(struct hda_codec *codec)  {
> > > >  	struct alc_spec *spec = codec->spec;
> > > >  	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> > > >  	bool hp1_pin_sense, hp2_pin_sense;
> > > >  
> > > > +	alc225_hp_init(codec);
> > > >  	if (!hp_pin)
> > > >  		return;
> > > >  
> > > > @@ -3262,6 +3279,7 @@ static void alc225_init(struct hda_codec
> > > >  
> > > >  	alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
> > > >  	alc_update_coefex_idx(codec, 0x57, 0x04, 0x0007, 0x4); /* Hight 
> > > > power */
> > > > +	printk("%s hp=0x%x\n",__func__,hp_pin);
> > > >  }
> > > 
> > > ... and I don't understand what's your problem.
> > > 
> > > Is it about that alc255_hp_init() gets called at resume in addition to the boot time?  If so, this is the intended behavior.
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > > 
> > > > -----Original Message-----
> > > > From: Takashi Iwai <tiwai@suse.de>
> > > > Sent: Tuesday, January 15, 2019 3:54 PM
> > > > To: Kailang <kailang@realtek.com>
> > > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > > Subject: Re: hp_pin was NULL value
> > > > 
> > > > On Tue, 15 Jan 2019 08:43:40 +0100, Kailang wrote:
> > > > > 
> > > > > 
> > > > > No, it not expected.
> > > > > It will enter two times after boot ready.
> > > > > I think it could add a flag in alc225_hp_init(). 
> > > > > To make sure it only excute one time. How do you think about?
> > > > 
> > > > Where was it called, actually?  The call of patch_ops.init is found in several places: once in snd_hda_codec_build_controls() just before creating controls (which is the boot time init), and at resume, alc269_resume().
> > > > 
> > > > And, as already explained, we have to call this at resume, because it's not guaranteed that it's been already called in the case of hibernation.
> > > > 
> > > > 
> > > > thanks,
> > > > 
> > > > Takashi
> > > > 
> > > > > 
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai <tiwai@suse.de>
> > > > > Sent: Tuesday, January 15, 2019 2:37 PM
> > > > > To: Kailang <kailang@realtek.com>
> > > > > Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
> > > > > Subject: Re: hp_pin was NULL value
> > > > > 
> > > > > On Tue, 15 Jan 2019 03:31:33 +0100, Kailang wrote:
> > > > > > 
> > > > > > Hi Takashi,
> > > > > > 
> > > > > > Could you receive this email?
> > > > > > 
> > > > > > BR,
> > > > > > Kailang
> > > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Kailang
> > > > > > Sent: Thursday, January 10, 2019 11:14 AM
> > > > > > To: 'Takashi Iwai' <tiwai@suse.de>
> > > > > > Cc: (alsa-devel@alsa-project.org) 
> > > > > > <alsa-devel@alsa-project.org>
> > > > > > Subject: RE: hp_pin was NULL value
> > > > > > 
> > > > > > Hi Takashi,
> > > > > > 
> > > > > > I simulate test your prefer function.
> > > > > > I put alc225_hp_init() in alc225_init().
> > > > > > If system boot ready, I write 1 to power_save of /sys/module/....
> > > > > > The print message as below. It called two times until boot ready.
> > > > > > If System enter to power save, play stream will wake up codec. It also call alc225_hp_init().
> > > > > > 
> > > > > > [   21.497524] alc225_init hp_pin=0x21
> > > > > > [   21.497526] alc225_hp_init-s hp=0x21
> > > > > > [   22.140076] alc225_hp_init-e hp=0x21
> > > > > > [   22.183496] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > > [   22.184681] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > > [   22.186846] alc225_shutup hp_pin=0x21
> > > > > > [   22.187831] input: HDA Intel Line Out as /devices/pci0000:00/0000:00:1b.0/sound/card0/input5
> > > > > > [   22.188055] input: HDA Intel Headphone as /devices/pci0000:00/0000:00:1b.0/sound/card0/input6
> > > > > > [   22.205104] alc225_init hp_pin=0x21
> > > > > > [   22.205108] alc225_hp_init-s hp=0x21
> > > > > > [   22.852123] alc225_hp_init-e hp=0x21
> > > > > > [   22.894772] snd_hda_codec_realtek hdaudioC0D2: Headset jack set to unplugged mode.
> > > > > > ############  Boot ready ############## [  116.832084] 
> > > > > > alc225_shutup
> > > > > > hp_pin=0x21  ==> echo 1 to power_save
> > > > > > [  120.002582] alc225_init hp_pin=0x21     ==> play system sound
> > > > > > [  120.002586] alc225_hp_init-s hp=0x21 [  120.644128] 
> > > > > > alc225_hp_init-e hp=0x21
> > > > > 
> > > > > It wasn't clear to me whether you meant this as a success or a failure...  Did the patch work as expected, no?
> > > > > 
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > Takashi
> > > > > 
> > > > > ------Please consider the environment before printing this e-mail.
> > > > > 
> > > > 
> > > 
> > [2 0000-alc294-hp-init-again.patch <application/octet-stream 
> > (base64)>]
> > 
> 

Re: hp_pin was NULL value

[Takashi Iwai]

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

On Tue, 29 Jan 2019 09:39:56 +0100,
Kailang wrote:
> 
> Hi Takashi,
> 
> So I think put it under alc269 parser. Maybe it is the quickly method.
> 
> 	err = alc269_parse_auto_config(codec);
> 	if (err < 0)
> 		goto error;
> +   .....
> +   .....

Not really...  The init sequence needs to be applied in two different
places: once in the init callback, and another in the resume callback
but only for the hibernation restore.

The patches below are applied on top of yours, and this should make
things working.

The first one lets the HD-audio core recording the currently processed
PM event, and the second one evaluates it and applies the missing init
sequence also for the hibernation resume.

This isn't quite sexy, but it has the minimal change in the codec
driver side.  If this requirement is more common, we can think of
splitting / reorganizing the codec callbacks to be more directly
called from the device pm ops.


thanks,

Takashi


[-- Attachment #2: 0001-ALSA-hda-Record-the-current-power-state-before-suspe.patch --]
[-- Type: application/octet-stream, Size: 2624 bytes --]

>From 8028c19c7349737bfb002265b75af75c87d7c7a4 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Tue, 29 Jan 2019 14:03:33 +0100
Subject: [PATCH 1/2] ALSA: hda - Record the current power state before
 suspend/resume calls

Currently we deal with single codec and suspend codec callbacks for
all S3, S4 and runtime PM handling.  But it turned out that we want
distinguish the call patterns sometimes, e.g. for applying some init
sequence only at probing and restoring from hibernate.

This patch slightly modifies the common PM callbacks for HD-audio
codec and stores the currently processed PM event in power_state of
the codec's device.power field, which is currently unused.  The codec
callback can take a look at this event value and judges which purpose
it's being called.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_codec.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index e4704f5729d3..43bcc6f1ea8c 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2917,6 +2917,7 @@ static void hda_call_codec_resume(struct hda_codec *codec)
 		hda_jackpoll_work(&codec->jackpoll_work.work);
 	else
 		snd_hda_jack_report_sync(codec);
+	dev->power.power_state = PMSG_ON;
 	snd_hdac_leave_pm(&codec->core);
 }
 
@@ -2947,10 +2948,42 @@ static int hda_codec_runtime_resume(struct device *dev)
 }
 #endif /* CONFIG_PM */
 
+#ifdef CONFIG_PM_SLEEP
+static int hda_codec_pm_suspend(struct device *dev)
+{
+	dev->power.power_state = PMSG_SUSPEND;
+	return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_resume(struct device *dev)
+{
+	dev->power.power_state = PMSG_RESUME;
+	return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_thaw(struct device *dev)
+{
+	dev->power.power_state = PMSG_THAW;
+	return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_restore(struct device *dev)
+{
+	dev->power.power_state = PMSG_RESTORE;
+	return pm_runtime_force_resume(dev);
+}
+#endif /* CONFIG_PM_SLEEP */
+
 /* referred in hda_bind.c */
 const struct dev_pm_ops hda_codec_driver_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				pm_runtime_force_resume)
+#ifdef CONFIG_PM_SLEEP
+	.suspend = hda_codec_pm_suspend,
+	.resume = hda_codec_pm_resume,
+	.freeze = hda_codec_pm_freeze,
+	.thaw = hda_codec_pm_thaw,
+	.poweroff = hda_codec_pm_suspend,
+	.restore = hda_codec_pm_restore,
+#endif /* CONFIG_PM_SLEEP */
 	SET_RUNTIME_PM_OPS(hda_codec_runtime_suspend, hda_codec_runtime_resume,
 			   NULL)
 };
-- 
2.16.4


[-- Attachment #3: 0002-ALSA-hda-realtek-Apply-ALC294-hp-init-also-for-S4-re.patch --]
[-- Type: application/octet-stream, Size: 1276 bytes --]

>From 259f064043a264d6b45b3e9738947a24814d9ab0 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Tue, 29 Jan 2019 14:14:51 +0100
Subject: [PATCH 2/2] ALSA: hda/realtek - Apply ALC294 hp init also for S4
 resume

The init sequence for ALC294 headphone stuff is needed not only for
the boot up time but also for the resume from hibernation, where the
device is switched from the boot kernel without sound driver to the
suspended image.  Since we record the PM event in the device
power_state field, we can now recognize the call pattern and apply the
sequence conditionally.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/patch_realtek.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 4139aced63f8..e9dc9408d9bc 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -3408,7 +3408,9 @@ static void alc294_init(struct hda_codec *codec)
 {
 	struct alc_spec *spec = codec->spec;
 
-	if (!spec->done_hp_init) {
+	/* required only at boot or S4 resume time */
+	if (!spec->done_hp_init ||
+	    codec->core.dev.power.power_state.event == PM_EVENT_RESTORE) {
 		alc294_hp_init(codec);
 		spec->done_hp_init = true;
 	}
-- 
2.16.4


[-- Attachment #4: Type: text/plain, Size: 0 bytes --]

Re: hp_pin was NULL value

[Takashi Iwai]

On Tue, 29 Jan 2019 14:22:45 +0100,
Takashi Iwai wrote:
> 
> On Tue, 29 Jan 2019 09:39:56 +0100,
> Kailang wrote:
> > 
> > Hi Takashi,
> > 
> > So I think put it under alc269 parser. Maybe it is the quickly method.
> > 
> > 	err = alc269_parse_auto_config(codec);
> > 	if (err < 0)
> > 		goto error;
> > +   .....
> > +   .....
> 
> Not really...  The init sequence needs to be applied in two different
> places: once in the init callback, and another in the resume callback
> but only for the hibernation restore.
> 
> The patches below are applied on top of yours, and this should make
> things working.
> 
> The first one lets the HD-audio core recording the currently processed
> PM event, and the second one evaluates it and applies the missing init
> sequence also for the hibernation resume.
> 
> This isn't quite sexy, but it has the minimal change in the codec
> driver side.  If this requirement is more common, we can think of
> splitting / reorganizing the codec callbacks to be more directly
> called from the device pm ops.

I did quick tests and the test result looks positive, at least, about
the PM power_state change tracking.  So I'm going to push your fix to
for-linus for 5.0, while other two to for-next, for 5.1, as the S4
resume issue isn't any urgent bug and it's no regression, either.


thanks,

Takashi

Re: hp_pin was NULL value

[Kailang]

Many Thanks. ^^

-----Original Message-----
From: Takashi Iwai <tiwai@suse.de> 
Sent: Tuesday, January 29, 2019 11:37 PM
To: Kailang <kailang@realtek.com>
Cc: (alsa-devel@alsa-project.org) <alsa-devel@alsa-project.org>
Subject: Re: hp_pin was NULL value

On Tue, 29 Jan 2019 14:22:45 +0100,
Takashi Iwai wrote:
> 
> On Tue, 29 Jan 2019 09:39:56 +0100,
> Kailang wrote:
> > 
> > Hi Takashi,
> > 
> > So I think put it under alc269 parser. Maybe it is the quickly method.
> > 
> > 	err = alc269_parse_auto_config(codec);
> > 	if (err < 0)
> > 		goto error;
> > +   .....
> > +   .....
> 
> Not really...  The init sequence needs to be applied in two different
> places: once in the init callback, and another in the resume callback 
> but only for the hibernation restore.
> 
> The patches below are applied on top of yours, and this should make 
> things working.
> 
> The first one lets the HD-audio core recording the currently processed 
> PM event, and the second one evaluates it and applies the missing init 
> sequence also for the hibernation resume.
> 
> This isn't quite sexy, but it has the minimal change in the codec 
> driver side.  If this requirement is more common, we can think of 
> splitting / reorganizing the codec callbacks to be more directly 
> called from the device pm ops.

I did quick tests and the test result looks positive, at least, about the PM power_state change tracking.  So I'm going to push your fix to for-linus for 5.0, while other two to for-next, for 5.1, as the S4 resume issue isn't any urgent bug and it's no regression, either.


thanks,

Takashi

------Please consider the environment before printing this e-mail.