Messages in this thread | | | Date | Sun, 01 Aug 2021 10:03:55 +0200 | From | Takashi Iwai <> | Subject | Re: [PATCH v3 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses |
| |
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
| |