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

[Takashi Iwai]

On Fri, 30 Jul 2021 17:18:30 +0200,
Vitaly Rodionov wrote:
> 
> From: Lucas Tanure <tanureal@xxxxxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Vitaly Rodionov <vitalyr@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Stefan Binding <sbinding@xxxxxxxxxxxxxxxxxxxxx>
> ---
> 
> 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