Date:   Thu, 25 Nov 2021 08:16:35 +0100
Message-ID: <s5h5ysg7lik.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Vitaly Rodionov <vitalyr@opensource.cirrus.com>
Cc:     James Schulman <james.schulman@cirrus.com>,
        David Rhodes <david.rhodes@cirrus.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, <alsa-devel@alsa-project.org>,
        <patches@opensource.cirrus.com>, <linux-kernel@vger.kernel.org>,
        Stefan Binding <sbinding@opensource.cirrus.com>
Subject: Re: [PATCH] ALSA: hda/cs8409: Schedule delayed work for jack detect on resume
In-Reply-To: <20211124181908.50672-1-vitalyr@opensource.cirrus.com>
References: <20211124181908.50672-1-vitalyr@opensource.cirrus.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 24 Nov 2021 19:19:08 +0100,
Vitaly Rodionov wrote:
> 
> From: Stefan Binding <sbinding@opensource.cirrus.com>
> 
> CS42L42 runs jack detect on resume, however this requires unsol
> events, and unsol events are ignored whilst the power state is
> not set to ON. The power state is set to ON only after the resume
> finishes.

This statement isn't clear to me: which power state and who ignores?
In the basic design of hda_jack, the all jack states get updated once
after the resume by reading the pin sense.  Doesn't this work in the
case of cs8409 because the code relies on unsol event?

> Schedule a delayed work timer to run jack detect
> after the resume call finishes.

This kind of approach is OK-ish as a last resort workaround, but I
think it'd be cleaner to rewrite the code to use directly snd_jack
stuff like HDMI codec driver without hda_jack stuff, supposing that
all jack states on cs8409 are read rather via i2c.  HDMI codec driver
re-probes jacks at its own resume callback, and issue
snd_jack_report() accordingly, as well as the same check for the
unsol_event.

Or, just faked unsol event handling at the resume callback would be
minimalistic change, I suppose.


thanks,

Takashi

> 
> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
> Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com>
> ---
>  sound/pci/hda/patch_cs8409.c | 79 +++++++++++++++++++++++++++++-------
>  sound/pci/hda/patch_cs8409.h |  1 +
>  2 files changed, 65 insertions(+), 15 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_cs8409.c b/sound/pci/hda/patch_cs8409.c
> index 31ff11ab868e..88213e95f0b3 100644
> --- a/sound/pci/hda/patch_cs8409.c
> +++ b/sound/pci/hda/patch_cs8409.c
> @@ -634,6 +634,30 @@ static void cs42l42_run_jack_detect(struct sub_codec *cs42l42)
>  	cs8409_i2c_write(cs42l42, 0x1120, 0xc0);
>  }
>  
> +static void cs42l42_run_jack_detect_all(struct hda_codec *codec)
> +{
> +	struct cs8409_spec *spec = codec->spec;
> +	struct sub_codec *cs42l42;
> +	int i;
> +
> +	for (i = 0; i < spec->num_scodecs; i++) {
> +		cs42l42 = spec->scodecs[i];
> +		cs42l42_enable_jack_detect(cs42l42);
> +		if (!cs42l42->hp_jack_in)
> +			cs42l42_run_jack_detect(cs42l42);
> +	}
> +}
> +
> +/*
> + * cs42l42_jack_detect_worker - Worker that retries jack detect
> + */
> +static void cs42l42_jack_detect_worker(struct work_struct *work)
> +{
> +	struct cs8409_spec *spec = container_of(work, struct cs8409_spec, jack_detect_work.work);
> +
> +	cs42l42_run_jack_detect_all(spec->codec);
> +}
> +
>  static int cs42l42_handle_tip_sense(struct sub_codec *cs42l42, unsigned int reg_ts_status)
>  {
>  	int status_changed = 0;
> @@ -749,8 +773,6 @@ static void cs42l42_resume(struct sub_codec *cs42l42)
>  
>  	if (cs42l42->full_scale_vol)
>  		cs8409_i2c_write(cs42l42, 0x2001, 0x01);
> -
> -	cs42l42_enable_jack_detect(cs42l42);
>  }
>  
>  #ifdef CONFIG_PM
> @@ -800,6 +822,7 @@ static void cs8409_free(struct hda_codec *codec)
>  
>  	/* Cancel i2c clock disable timer, and disable clock if left enabled */
>  	cancel_delayed_work_sync(&spec->i2c_clk_work);
> +	cancel_delayed_work_sync(&spec->jack_detect_work);
>  	cs8409_disable_i2c_clock(codec);
>  
>  	snd_hda_gen_free(codec);
> @@ -863,6 +886,7 @@ static int cs8409_cs42l42_suspend(struct hda_codec *codec)
>  
>  	/* Cancel i2c clock disable timer, and disable clock if left enabled */
>  	cancel_delayed_work_sync(&spec->i2c_clk_work);
> +	cancel_delayed_work_sync(&spec->jack_detect_work);
>  	cs8409_disable_i2c_clock(codec);
>  
>  	snd_hda_shutup_pins(codec);
> @@ -970,6 +994,8 @@ void cs8409_cs42l42_fixups(struct hda_codec *codec, const struct hda_fixup *fix,
>  		spec->scodecs[CS8409_CODEC0]->codec = codec;
>  		codec->patch_ops = cs8409_cs42l42_patch_ops;
>  
> +		INIT_DELAYED_WORK(&spec->jack_detect_work, cs42l42_jack_detect_worker);
> +
>  		spec->gen.suppress_auto_mute = 1;
>  		spec->gen.no_primary_hp = 1;
>  		spec->gen.suppress_vmaster = 1;
> @@ -1029,9 +1055,16 @@ void cs8409_cs42l42_fixups(struct hda_codec *codec, const struct hda_fixup *fix,
>  	case HDA_FIXUP_ACT_INIT:
>  		cs8409_cs42l42_hw_init(codec);
>  		spec->init_done = 1;
> -		if (spec->init_done && spec->build_ctrl_done
> -			&& !spec->scodecs[CS8409_CODEC0]->hp_jack_in)
> -			cs42l42_run_jack_detect(spec->scodecs[CS8409_CODEC0]);
> +		if (spec->init_done && spec->build_ctrl_done) {
> +			/* No point in running jack detect until we have fully resumed */
> +			if (codec->core.dev.power.power_state.event != PM_EVENT_ON) {
> +				codec_warn(codec, "Not ready to detect jack, deferring...\n");
> +				schedule_delayed_work(&spec->jack_detect_work, msecs_to_jiffies(25));
> +				return;
> +			} else {
> +				cs42l42_run_jack_detect_all(codec);
> +			}
> +		}
>  		break;
>  	case HDA_FIXUP_ACT_BUILD:
>  		spec->build_ctrl_done = 1;
> @@ -1040,9 +1073,16 @@ void cs8409_cs42l42_fixups(struct hda_codec *codec, const struct hda_fixup *fix,
>  		 * been already plugged in.
>  		 * Run immediately after init.
>  		 */
> -		if (spec->init_done && spec->build_ctrl_done
> -			&& !spec->scodecs[CS8409_CODEC0]->hp_jack_in)
> -			cs42l42_run_jack_detect(spec->scodecs[CS8409_CODEC0]);
> +		if (spec->init_done && spec->build_ctrl_done) {
> +			/* No point in running jack detect until we have fully resumed */
> +			if (codec->core.dev.power.power_state.event != PM_EVENT_ON) {
> +				codec_warn(codec, "Not ready to detect jack, deferring...\n");
> +				schedule_delayed_work(&spec->jack_detect_work, msecs_to_jiffies(25));
> +				return;
> +			} else {
> +				cs42l42_run_jack_detect_all(codec);
> +			}
> +		}
>  		break;
>  	default:
>  		break;
> @@ -1178,7 +1218,6 @@ void dolphin_fixups(struct hda_codec *codec, const struct hda_fixup *fix, int ac
>  {
>  	struct cs8409_spec *spec = codec->spec;
>  	struct snd_kcontrol_new *kctrl;
> -	int i;
>  
>  	switch (action) {
>  	case HDA_FIXUP_ACT_PRE_PROBE:
> @@ -1193,6 +1232,8 @@ void dolphin_fixups(struct hda_codec *codec, const struct hda_fixup *fix, int ac
>  		spec->scodecs[CS8409_CODEC1]->codec = codec;
>  		spec->num_scodecs = 2;
>  
> +		INIT_DELAYED_WORK(&spec->jack_detect_work, cs42l42_jack_detect_worker);
> +
>  		codec->patch_ops = cs8409_dolphin_patch_ops;
>  
>  		/* GPIO 1,5 out, 0,4 in */
> @@ -1237,9 +1278,13 @@ void dolphin_fixups(struct hda_codec *codec, const struct hda_fixup *fix, int ac
>  		dolphin_hw_init(codec);
>  		spec->init_done = 1;
>  		if (spec->init_done && spec->build_ctrl_done) {
> -			for (i = 0; i < spec->num_scodecs; i++) {
> -				if (!spec->scodecs[i]->hp_jack_in)
> -					cs42l42_run_jack_detect(spec->scodecs[i]);
> +			/* No point in running jack detect until we have fully resumed */
> +			if (codec->core.dev.power.power_state.event != PM_EVENT_ON) {
> +				codec_warn(codec, "Not ready to detect jack, deferring...\n");
> +				schedule_delayed_work(&spec->jack_detect_work, msecs_to_jiffies(25));
> +				return;
> +			} else {
> +				cs42l42_run_jack_detect_all(codec);
>  			}
>  		}
>  		break;
> @@ -1251,9 +1296,13 @@ void dolphin_fixups(struct hda_codec *codec, const struct hda_fixup *fix, int ac
>  		 * Run immediately after init.
>  		 */
>  		if (spec->init_done && spec->build_ctrl_done) {
> -			for (i = 0; i < spec->num_scodecs; i++) {
> -				if (!spec->scodecs[i]->hp_jack_in)
> -					cs42l42_run_jack_detect(spec->scodecs[i]);
> +			/* No point in running jack detect until we have fully resumed */
> +			if (codec->core.dev.power.power_state.event != PM_EVENT_ON) {
> +				codec_warn(codec, "Not ready to detect jack, deferring...\n");
> +				schedule_delayed_work(&spec->jack_detect_work, msecs_to_jiffies(25));
> +				return;
> +			} else {
> +				cs42l42_run_jack_detect_all(codec);
>  			}
>  		}
>  		break;
> diff --git a/sound/pci/hda/patch_cs8409.h b/sound/pci/hda/patch_cs8409.h
> index ade2b838590c..632d3ec8322d 100644
> --- a/sound/pci/hda/patch_cs8409.h
> +++ b/sound/pci/hda/patch_cs8409.h
> @@ -330,6 +330,7 @@ struct cs8409_spec {
>  	unsigned int i2c_clck_enabled;
>  	unsigned int dev_addr;
>  	struct delayed_work i2c_clk_work;
> +	struct delayed_work jack_detect_work;
>  
>  	unsigned int playback_started:1;
>  	unsigned int capture_started:1;
> -- 
> 2.25.1
>