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 >
