Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 01 Aug 2021 10:03:55 +0200
Message-ID: <s5h7dh51thw.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Vitaly Rodionov <vitalyr@opensource.cirrus.com>
Cc:     Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        <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: Re: [PATCH v3 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses
In-Reply-To: <20210730151844.7873-14-vitalyr@opensource.cirrus.com>
References: <20210730151844.7873-1-vitalyr@opensource.cirrus.com>
        <20210730151844.7873-14-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 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