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;
Subject: Re: [PATCH v3 13/27] ALSA: hda/cs8409: Dont disable I2C clock between
 consecutive accesses
To:     Takashi Iwai <tiwai@suse.de>
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>
References: <20210730151844.7873-1-vitalyr@opensource.cirrus.com>
 <20210730151844.7873-14-vitalyr@opensource.cirrus.com>
 <s5h7dh51thw.wl-tiwai@suse.de>
From:   Vitaly Rodionov <vitalyr@opensource.cirrus.com>
Message-ID: <499860c1-bf6f-969a-a987-3302820b66af@opensource.cirrus.com>
Date:   Wed, 11 Aug 2021 19:33:07 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <s5h7dh51thw.wl-tiwai@suse.de>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

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