LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vitaly Rodionov <vitalyr@opensource.cirrus.com>
To: Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>
Cc: <alsa-devel@alsa-project.org>, <patches@opensource.cirrus.com>,
	<linux-kernel@vger.kernel.org>,
	Lucas Tanure <tanureal@opensource.cirrus.com>,
	Stefan Binding <sbinding@opensource.cirrus.com>
Subject: [PATCH v3 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses
Date: Fri, 30 Jul 2021 16:18:30 +0100	[thread overview]
Message-ID: <20210730151844.7873-14-vitalyr@opensource.cirrus.com> (raw)
In-Reply-To: <20210730151844.7873-1-vitalyr@opensource.cirrus.com>

From: Lucas Tanure <tanureal@opensource.cirrus.com>

Only disable I2C clock 25 ms after not being used.

The current implementation enables and disables the I2C clock for each
I2C transaction. Each enable/disable call requires two verb transactions.
This means each I2C transaction requires a total of four verb transactions
to enable and disable the clock.
However, if there are multiple consecutive I2C transactions, it is not
necessary to enable and disable the clock each time, instead it is more
efficient to enable the clock for the first transaction, and disable it
after the final transaction, which would improve performance.
This is achieved by using a timeout which disables the clock if no request
to enable the clock has occurred for 25 ms.

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com>
Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---

Changes in v2:
- Improved delayed work start/cancel implementation, and re-worked commit message
 adding more explanation why this was required. 

Changes in v3:
- Cancel the disable timer, but do not wait for any running disable functions to finish.
 If the disable timer runs out before cancel, the delayed work thread will be blocked,
 waiting for the mutex to become unlocked. This mutex will be locked for the duration of
 any i2c transaction, so the disable function will run to completion immediately
 afterwards in the scenario. The next enable call will re-enable the clock, regardless.

 sound/pci/hda/patch_cs8409.c | 64 ++++++++++++++++++++++++++----------
 sound/pci/hda/patch_cs8409.h |  4 +++
 2 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/sound/pci/hda/patch_cs8409.c b/sound/pci/hda/patch_cs8409.c
index 08205c19698c..1c75aa262ebc 100644
--- a/sound/pci/hda/patch_cs8409.c
+++ b/sound/pci/hda/patch_cs8409.c
@@ -53,7 +53,9 @@ static struct cs8409_spec *cs8409_alloc_spec(struct hda_codec *codec)
 	if (!spec)
 		return NULL;
 	codec->spec = spec;
+	spec->codec = codec;
 	codec->power_save_node = 1;
+	INIT_DELAYED_WORK(&spec->i2c_clk_work, cs8409_disable_i2c_clock);
 	snd_hda_gen_spec_init(&spec->gen);
 
 	return spec;
@@ -72,21 +74,45 @@ static inline void cs8409_vendor_coef_set(struct hda_codec *codec, unsigned int
 	snd_hda_codec_write(codec, CS8409_PIN_VENDOR_WIDGET, 0, AC_VERB_SET_PROC_COEF, coef);
 }
 
-/**
+/*
+ * cs8409_disable_i2c_clock - Worker that disable the I2C Clock after 25ms without use
+ */
+static void cs8409_disable_i2c_clock(struct work_struct *work)
+{
+	struct cs8409_spec *spec = container_of(work, struct cs8409_spec, i2c_clk_work.work);
+
+	mutex_lock(&spec->cs8409_i2c_mux);
+	if (spec->i2c_clck_enabled) {
+		cs8409_vendor_coef_set(spec->codec, 0x0,
+			       cs8409_vendor_coef_get(spec->codec, 0x0) & 0xfffffff7);
+		spec->i2c_clck_enabled = 0;
+	}
+	mutex_unlock(&spec->cs8409_i2c_mux);
+}
+
+/*
  * cs8409_enable_i2c_clock - Enable I2C clocks
  * @codec: the codec instance
- * @enable: Enable or disable I2C clocks
- *
  * Enable or Disable I2C clocks.
+ * This must be called when the i2c mutex is locked.
  */
-static void cs8409_enable_i2c_clock(struct hda_codec *codec, unsigned int enable)
+static void cs8409_enable_i2c_clock(struct hda_codec *codec)
 {
-	unsigned int retval;
-	unsigned int newval;
+	struct cs8409_spec *spec = codec->spec;
+
+	/* Cancel the disable timer, but do not wait for any running disable functions to finish.
+	 * If the disable timer runs out before cancel, the delayed work thread will be blocked,
+	 * waiting for the mutex to become unlocked. This mutex will be locked for the duration of
+	 * any i2c transaction, so the disable function will run to completion immediately
+	 * afterwards in the scenario. The next enable call will re-enable the clock, regardless.
+	 */
+	cancel_delayed_work(&spec->i2c_clk_work);
 
-	retval = cs8409_vendor_coef_get(codec, 0x0);
-	newval = (enable) ? (retval | 0x8) : (retval & 0xfffffff7);
-	cs8409_vendor_coef_set(codec, 0x0, newval);
+	if (!spec->i2c_clck_enabled) {
+		cs8409_vendor_coef_set(codec, 0x0, cs8409_vendor_coef_get(codec, 0x0) | 0x8);
+		spec->i2c_clck_enabled = 1;
+	}
+	queue_delayed_work(system_power_efficient_wq, &spec->i2c_clk_work, msecs_to_jiffies(25));
 }
 
 /**
@@ -134,7 +160,7 @@ static int cs8409_i2c_read(struct hda_codec *codec, unsigned int i2c_address, un
 	if (spec->cs42l42_suspended)
 		return -EPERM;
 
-	cs8409_enable_i2c_clock(codec, 1);
+	cs8409_enable_i2c_clock(codec);
 	cs8409_vendor_coef_set(codec, CS8409_I2C_ADDR, i2c_address);
 
 	if (paged) {
@@ -157,8 +183,6 @@ static int cs8409_i2c_read(struct hda_codec *codec, unsigned int i2c_address, un
 	/* Register in bits 15-8 and the data in 7-0 */
 	read_data = cs8409_vendor_coef_get(codec, CS8409_I2C_QREAD);
 
-	cs8409_enable_i2c_clock(codec, 0);
-
 	return read_data & 0x0ff;
 }
 
@@ -182,7 +206,7 @@ static int cs8409_i2c_write(struct hda_codec *codec, unsigned int i2c_address, u
 	if (spec->cs42l42_suspended)
 		return -EPERM;
 
-	cs8409_enable_i2c_clock(codec, 1);
+	cs8409_enable_i2c_clock(codec);
 	cs8409_vendor_coef_set(codec, CS8409_I2C_ADDR, i2c_address);
 
 	if (paged) {
@@ -203,8 +227,6 @@ static int cs8409_i2c_write(struct hda_codec *codec, unsigned int i2c_address, u
 		return -EIO;
 	}
 
-	cs8409_enable_i2c_clock(codec, 0);
-
 	return 0;
 }
 
@@ -551,6 +573,14 @@ static int cs8409_suspend(struct hda_codec *codec)
 }
 #endif
 
+static void cs8409_free(struct hda_codec *codec)
+{
+	struct cs8409_spec *spec = codec->spec;
+
+	cancel_delayed_work_sync(&spec->i2c_clk_work);
+	snd_hda_gen_free(codec);
+}
+
 /* Vendor specific HW configuration
  * PLL, ASP, I2C, SPI, GPIOs, DMIC etc...
  */
@@ -633,7 +663,7 @@ static const struct hda_codec_ops cs8409_cs42l42_patch_ops = {
 	.build_controls = cs8409_build_controls,
 	.build_pcms = snd_hda_gen_build_pcms,
 	.init = cs8409_cs42l42_init,
-	.free = snd_hda_gen_free,
+	.free = cs8409_free,
 	.unsol_event = cs8409_jack_unsol_event,
 #ifdef CONFIG_PM
 	.suspend = cs8409_suspend,
@@ -785,7 +815,7 @@ static int patch_cs8409(struct hda_codec *codec)
 
 	err = cs8409_parse_auto_config(codec);
 	if (err < 0) {
-		snd_hda_gen_free(codec);
+		cs8409_free(codec);
 		return err;
 	}
 
diff --git a/sound/pci/hda/patch_cs8409.h b/sound/pci/hda/patch_cs8409.h
index bf0e8a4cc4cc..542582c213d2 100644
--- a/sound/pci/hda/patch_cs8409.h
+++ b/sound/pci/hda/patch_cs8409.h
@@ -11,6 +11,7 @@
 
 #include <linux/pci.h>
 #include <sound/tlv.h>
+#include <linux/workqueue.h>
 #include <sound/hda_codec.h>
 #include "hda_local.h"
 #include "hda_auto_parser.h"
@@ -267,6 +268,7 @@ struct cs8409_cir_param {
 
 struct cs8409_spec {
 	struct hda_gen_spec gen;
+	struct hda_codec *codec;
 
 	unsigned int gpio_mask;
 	unsigned int gpio_dir;
@@ -278,6 +280,8 @@ struct cs8409_spec {
 	s8 vol[CS42L42_VOLUMES];
 
 	struct mutex cs8409_i2c_mux;
+	unsigned int i2c_clck_enabled;
+	struct delayed_work i2c_clk_work;
 
 	/* verb exec op override */
 	int (*exec_verb)(struct hdac_device *dev, unsigned int cmd, unsigned int flags,
-- 
2.25.1


  parent reply	other threads:[~2021-07-30 15:19 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-30 15:18 [PATCH v3 00/27] ALSA: hda/cirrus: Split generic cirrus HDA codecs and CS8490 bridge into separate modules Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 01/27] ALSA: hda/cirrus: Move CS8409 HDA bridge to separate module Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 02/27] ALSA: hda/cs8409: Move arrays of configuration to a new file Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 03/27] ALSA: hda/cs8409: Use enums for register names and coefficients Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 04/27] ALSA: hda/cs8409: Mask all CS42L42 interrupts on initialization Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 05/27] ALSA: hda/cs8409: Reduce HS pops/clicks for Cyborg Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 06/27] ALSA: hda/cs8409: Disable unnecessary Ring Sense for Cyborg/Warlock/Bullseye Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 07/27] ALSA: hda/cs8409: Disable unsolicited responses during suspend Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 08/27] ALSA: hda/cs8409: Disable unsolicited response for the first boot Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 09/27] ALSA: hda/cs8409: Mask CS42L42 wake events Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 10/27] ALSA: hda/cs8409: Simplify CS42L42 jack detect Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 11/27] ALSA: hda/cs8409: Prevent I2C access during suspend time Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 12/27] ALSA: hda/cs8409: Generalize volume controls Vitaly Rodionov
2021-07-30 15:18 ` Vitaly Rodionov [this message]
2021-08-01  8:03   ` [PATCH v3 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses Takashi Iwai
2021-08-11 18:33     ` Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 14/27] ALSA: hda/cs8409: Avoid setting the same I2C address for every access Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 15/27] ALSA: hda/cs8409: Avoid re-setting the same page as the last access Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 16/27] ALSA: hda/cs8409: Support i2c bulk read/write functions Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 17/27] ALSA: hda/cs8409: Separate CS8409, CS42L42 and project functions Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 18/27] ALSA: hda/cs8409: Move codec properties to its own struct Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 19/27] ALSA: hda/cs8409: Support multiple sub_codecs for Suspend/Resume/Unsol events Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 20/27] ALSA: hda/cs8409: Add Support to disable jack type detection for CS42L42 Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 21/27] ALSA: hda/cs8409: Add support for dolphin Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 22/27] ALSA: hda/cs8409: Enable Full Scale Volume for Line Out Codec on Dolphin Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 23/27] ALSA: hda/cs8409: Set fixed sample rate of 48kHz for CS42L42 Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 24/27] ALSA: hda/cs8409: Use timeout rather than retries for I2C transaction waits Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 25/27] ALSA: hda/cs8409: Remove unnecessary delays Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 26/27] ALSA: hda/cs8409: Follow correct CS42L42 power down sequence for suspend Vitaly Rodionov
2021-07-30 15:18 ` [PATCH v3 27/27] ALSA: hda/cs8409: Unmute/Mute codec when stream starts/stops Vitaly Rodionov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210730151844.7873-14-vitalyr@opensource.cirrus.com \
    --to=vitalyr@opensource.cirrus.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=patches@opensource.cirrus.com \
    --cc=perex@perex.cz \
    --cc=sbinding@opensource.cirrus.com \
    --cc=tanureal@opensource.cirrus.com \
    --cc=tiwai@suse.com \
    --subject='Re: [PATCH v3 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).