Received-SPF: pass (google.com: domain of linux-kernel+bounces-83758-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <599a7357-b4d6-4581-9d5c-c1d0ade3e410@foss.st.com>
Date: Tue, 27 Feb 2024 18:43:20 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 04/10] counter: stm32-timer-cnt: introduce clock signal
Content-Language: en-US
To: William Breathitt Gray <william.gray@linaro.org>
CC: <lee@kernel.org>, <alexandre.torgue@foss.st.com>,
        <linux-iio@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>,
        <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>
References: <20231220145726.640627-1-fabrice.gasnier@foss.st.com>
 <20231220145726.640627-5-fabrice.gasnier@foss.st.com>
 <ZZwm7ZyrL7vFn0Xd@ubuntu-server-vm-macos>
From: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
In-Reply-To: <ZZwm7ZyrL7vFn0Xd@ubuntu-server-vm-macos>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 1/8/24 17:46, William Breathitt Gray wrote:
> On Wed, Dec 20, 2023 at 03:57:20PM +0100, Fabrice Gasnier wrote:
>> Introduce the internal clock signal, used to count when in simple rising
>> function. Also add the "frequency" extension to the clock signal.
>>
>> With this patch, signal action reports a consistent state when "increase"
>> function is used, and the counting frequency:
>>     $ echo increase > function
>>     $ grep -H "" signal*_action
>>     signal0_action:none
>>     signal1_action:none
>>     signal2_action:rising edge
>>     $ echo 1 > enable
>>     $ cat count
>>     25425
>>     $ cat count
>>     44439
>>     $ cat ../signal2/frequency
>>     208877930
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> 
> Reviewed-by: William Breathitt Gray <william.gray@linaro.org>
> 
> The code is all right, but some minor suggestions below.
> 
>> +static struct counter_comp stm32_count_clock_ext[] = {
>> +	COUNTER_COMP_SIGNAL_U64("frequency", stm32_count_clk_get_freq, NULL),
> 
> It might be worth introducing a new COUNTER_COMP_FREQUENCY() macro now
> that we have a second driver with the 'frequency' extension
> (ti-ecap-capture also has 'frequency'). But it's up to you if you want
> to add a precursor patch to this series, or I'll introduce it separately
> myself in a independent patch.

Thanks for suggesting.

I added a precursor patch to this series.
I guess you wishes to see it used in both ti-ecap-capture and
stm32-timer-cnt. I only cared about stm32-timer-cnt in this series.

Can I let you do ti-ecap-capture change if/when you're going to apply it?

> 
>> @@ -287,7 +321,13 @@ static struct counter_signal stm32_signals[] = {
>>  	{
>>  		.id = STM32_CH2_SIG,
>>  		.name = "Channel 2"
>> -	}
>> +	},
>> +	{
>> +		.id = STM32_CLOCK_SIG,
>> +		.name = "Clock Signal",
> 
> The word "Signal" feels unnecessary to me when both the sysfs path and
> data structure will have 'signal' already. Do you think "Clock" by
> itself is clear enough?

Agreed, I updated in v4.

Best Regards,
Fabrice

> 
> William Breathitt Gray