Re: [PATCH] counter: stm32-timer-cnt: fix ceiling write max value

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

On 3/3/21 12:42 AM, William Breathitt Gray wrote:
> On Tue, Mar 02, 2021 at 06:03:25PM +0100, Fabrice Gasnier wrote:
>> On 3/2/21 3:56 PM, William Breathitt Gray wrote:
>>> Side question: if priv->ceiling is tracking the current ceiling
>>> configuration, would it make sense to change stm32_count_ceiling_read()
>>> to print the value of priv->ceiling instead of doing a regmap_read()
>>> call?
>>
>> Hi William,
>>
>> Thanks for reviewing.
>>
>> I'd be fine either way. So no objection to move to the priv->ceiling
>> (cached) value. It could also here here.
>> By looking at this, I figured out there's probably another thing to fix
>> here, for initial conditions.
>>
>> At probe time priv->ceiling is initialized to max value (ex 65535 for a
>> 16 bits counter). But the register content is 0 (clear by mfd driver at
>> probe time).
>>
>> - So, reading ceiling from sysfs currently reports 0 (regmap_read())
>> after booting and probing.
>>
>> I see two cases at this point:
>> - In case the counter gets enabled without any prior configuration, it
>> won't count: ceiling value (e.g. 65535) should be written to register
>> before it is enabled, so the counter will actually count. So there's
>> room for a fix here.
>>
>> - In case function gets set (ex: quadrature x4), priv->ceiling (e.g.
>> 65535) gets written to the register (although it's been read earlier as
>> 0 from sysfs).
>> This could be fixed by reading the priv->ceiling in
>> stm32_count_ceiling_read() as you're asking (provided 1st case has been
>> fixed as well)
>>
>> I'll probably prepare one or two patches for the above cases, if you agree ?
>>
>> Best Regards,
>> Fabrice
> 
> Looking through the driver, it doesn't seem like priv->ceiling is used
> in any performance critical code, just the callbacks for count_write()
> and function_set(). It might make more sense to remove priv->ceiling and
> replace it with the regmap_read() calls where necessary so that we
> always get the most current ceiling value from the device when needed.

Hi William,

Ok, I'll look into this.

> 
> As for the default ceiling value for the device at probe time, this
> should probably be set to the max value because that is what a normal
> user would expect when loading a Counter driver (a ceiling value of 0 at
> startup is somewhat unintuitive).

Yes, I agree. In fact, the default (reset) value is the maximum. The 0
value is forced in ARR register by the mfd driver [1], after reading the
max_arr value. I see no rationale for this.
I think the fix should probably be done there: I'd prefer to backup and
restore ARR value in the mfd, instead of unconditionally clearing it.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/mfd/stm32-timers.c?h=v5.11#n167

> 
> If you prepare those two patches, then that should resolve this.

I'll prepare this.

Thanks for your advices,
Best Regards,
Fabrice

> 
> William Breathitt Gray
>