Re: [PATCH v3 04/10] counter: stm32-timer-cnt: introduce clock signal

[Fabrice Gasnier <fabrice.gasnier@foss.st.com>]

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