diff mbox series

[v3,13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses

Message ID 20210730151844.7873-14-vitalyr@opensource.cirrus.com (mailing list archive)
State Superseded
Headers show
Series ALSA: hda/cirrus: Split generic cirrus HDA codecs and CS8490 bridge into separate modules. | expand

Commit Message

Vitaly Rodionov July 30, 2021, 3:18 p.m. UTC 
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(-)

Comments

Takashi Iwai Aug. 1, 2021, 8:03 a.m. UTC | #1 
On Fri, 30 Jul 2021 17:18:30 +0200,
Vitaly Rodionov wrote:
> 
> 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.

This looks almost fine, but just a couple of thoughts:

- cancel_delayed_work_sync() means to it might keep the i2c enabled
  after that point (just cancel the pending work).
  Would it cause a inconsistency afterwards?

- A similar procedure is needed for suspend callback to cancel / flush
  the work.
  The shutdown is another question, but usually it's fine to without
  any special handling as long as the resource is kept.


thanks,

Takashi
Vitaly Rodionov Aug. 11, 2021, 6:33 p.m. UTC | #2 
On 01/08/2021 9:03 am, Takashi Iwai wrote:
> On Fri, 30 Jul 2021 17:18:30 +0200,
> Vitaly Rodionov wrote:
>> 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.
> This looks almost fine, but just a couple of thoughts:
>
> - cancel_delayed_work_sync() means to it might keep the i2c enabled
>    after that point (just cancel the pending work).
>    Would it cause a inconsistency afterwards?
>
> - A similar procedure is needed for suspend callback to cancel / flush
>    the work.
>    The shutdown is another question, but usually it's fine to without
>    any special handling as long as the resource is kept.

Hi Takashi,

Thank you very much for your comments. It all make sense.

We will make further improvement and submit next version.

Thanks,

Vitaly

>
> thanks,
>
> Takashi
diff mbox series

Patch

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,