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@xxxxxxxxxxx>
>
> Reviewed-by: William Breathitt Gray <william.gray@xxxxxxxxxx>
>
> 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
