Re: Removing the last of IIO_COUNT (stm32-timer-trigger)

[William Breathitt Gray]

On Thu, Oct 27, 2022 at 11:41:18AM +0200, Fabrice Gasnier wrote:
> On 10/16/22 14:05, William Breathitt Gray wrote:
> > Hi all,
> > 
> > The drivers/iio/trigger/stm32-timer-trigger.c file remains the last
> > consumer if the IIO_COUNT type. The IIO_COUNT type was deprecated some
> > time ago with the introduction of the Counter subsystem. Most of the
> > previous IIO_COUNT consumers were migrated successfully to the Counter
> > subsystem, but the stm32-timer-trigger driver remains as the sole module
> > preventing the final removal of IIO_COUNT.
> > 
> > At the time we deprecated IIO_COUNT, the Counter subsystem was nascent
> > and lacked some of the functionality we have nowadays such as a
> > character device interface, timestamping, hardware buffer support, etc.
> > If I recall correctly, the decision to delay the migration of
> > stm32-timer-trigger to the Counter subsystem was a lack of some
> > functionality the Counter subsystem could not provide at the time.
> > 
> 
> Hi William,
> 
> As far as I remember, initial work on stm32-timer-counter focused only
> on porting the quadrature interface away from stm32-timer-trigger.
> Unfortunately, I've followed from afar all the progress you did in the
> framework since then.
> From the infrastructure point of view, there's probably not much to add
> to be able to move away the IIO_COUNT channel (and the IIO device) part
> of stm32-timer-trigger driver.
> Let's focus only on the modes implemented here.

Hi Fabrice,

Sorry for the delay in my response; it took me a while to think through
all this, but I believe you have the right approach in your suggestions.

> 
> Besides this, I may have further questions on the "hardware buffer
> support" (could you point this as I miss it for now), and also the
> capture interface.
> There has been a separate discussion here ("pwm: Make capture support
> optional"):
> https://lore.kernel.org/linux-pwm/Yz%2F4V0gH%2FvrWSS8U@orome/T/#u
> I'd be glad to get your opinion, on possibly moving the PWM input
> capture feature to the counter framework too.

I replied to the PWM discussion to provide some context about the
Counter events (capture) interface. You can find out more about the
Counter hardware buffer support in the Counter array components support
introduction patch series:
https://lore.kernel.org/all/cover.1664204990.git.william.gray@xxxxxxxxxx/

Essentially Counter arrays are a convenience tool to allow drivers to
group and handle multiple components of the same type. Hardware buffers
are thus supported by exposing each buffer element as an individual
component to the user; those components are handled by the Counter
driver via a callback with the index for the particular element passed
in as an argument.

> 
> > I hoping someone can evaluate stm32-timer-trigger to see if we are able
> > transition now to the Counter subsystem, or if that necessary
> > functionality is still missing today. Even if it turns out that we are
> > unable to migrate, it'll be useful to know what's left to implement in
> > the Counter subsystem to support stm32-timer-trigger or similar devices
> > in the future.
> 
> As you're asking, I just tried to narrow down specific things in this
> driver, and assess possible impacts. Please find some details here after
> and first as an introduction:
> 
> The IIO device registered in this driver has two specific extensions, to
> manage specific ("slave") modes: "always", "gated", "triggered".
> E.g the *enable_mode*.
> 
> The last 2 modes depends on specific hardware *triggers* being
> associated in IIO sysfs, to select the trigger (e.g. echo the source
> trigger name > trigger/current_trigger # the destination timer to trig).
> The list of triggers is specific to each timer instance in STM32.
> In other words, some timers outputs can be used as input on other
> timers. Here comes the *trigger_mode* attribute (see after).
> 
> --- enable_mode ---
> In Documentation/ABI/testing/sysfs-bus-iio-timer-stm32, this correspond to:
> - /sys/bus/iio/devices/iio:deviceX/in_count0_enable_mode
> - /sys/bus/iio/devices/iio:deviceX/in_count_enable_mode_available
> always:
> 	Counter is always ON.
> gated:
> 	Counting is enabled when connected trigger signal
> 	level is high else counting is disabled.
> triggered:
> 	Counting is enabled on rising edge of the connected
> 	trigger, and remains enabled for the duration of this
> 	selected mode.

Okay, we need to support these three modes. The "always" mode seems
straight-forward to support, but "gated" and "triggered" are what need
special consideration for this driver.

> 
> Basically, the "always" mode is already used by default in
> stm32-timer-cnt driver, and matches: COUNTER_COUNT_MODE_NORMAL, By
> referring to:
> Documentation/ABI/testing/sysfs-bus-counter
> 		normal:
> 			Counting is continuous in either direction.
> 
> Please find some thoughts/proposal here:
> - This could lead to add two counter modes to the list of "range limit",
> "non-recycle" and "modulo-n". The STM32 timer trigger inputs could be
> described as signals (list being specific for each timer instance, see
> valids_table[] arrays in stm32-timer-trigger).

It makes sense to represent these inputs as Signals because they can
directly affect the value change of the respective Count, so I think we
should organize this as such. However, "count_mode" wouldn't be the
correct place for configuration of the "gated" and "triggered" modes.
"count_mode" is for configuring how the Count value updates once a
Synapse action triggers, but "gated" and "triggered" are rather more so
determining whether those Synapse actions should trigger in the first
place.

> 
> - OR, maybe the 2 modes could be described as a specific synapse action,
> (trigger input also being described as a signal)? In both "gated" and
> "triggered" modes, the timer counts on its internal clock input (but not
> continuously). But it doesn't really match the "normal" mode.

Yes, I think this is the approach to take. Synapse actions represent
when the Count function is evaluated to update the respective Count;
"gated" and "triggered" represent modes where a trigger input serves to
determine when that update occurs, so those are naturally Synapse action
modes.

To add support for these, you would just need to introduce to
include/uapi/linux/counter.h two new enum counter_synapse_action
constants: COUNTER_SYNAPSE_ACTION_GATED and
COUNTER_SYNAPSE_ACTION_TRIGGERED. You can then use those new enums in
your action_read() callbacks.

> 
> - Last, maybe a mix of a new "trigger" count mode, and synapse action
> (gated/triggered) could be used ?
> Note: The last one may open the door to other modes that aren't
> implemented in current stm32-timer-trigger driver, by extending the
> synapse actions (like reset the counter upon trigger signal... and other
> combined modes specified in the STM32 timer spec).

Instead of "count_mode", a new Count component should be introduced to
handle this configuration. Maybe we could call it "gate_mode" (or
perhaps your might have a better name) and it can be implemented using
the COUNTER_COMP_COUNT_ENUM() macro to allow for selection between
"always", "gated", and "triggered".

> 
> --- trigger_mode ---
> In Documentation/ABI/testing/sysfs-bus-iio-timer-stm32, this correspond to:
> - /sys/bus/iio/devices/iio:deviceX/in_count_trigger_mode_available
> - /sys/bus/iio/devices/iio:deviceX/in_count0_trigger_mode
> In the STM32 timer spec this is: External Clock Mode 1 - Rising edges of
> the selected trigger (TRGI) clock the counter.

This can be implemented as a COUNTER_COMP_COUNT_ENUM() to select the
particular trigger input.

> 
> In this configuration, IMHO, this matches the "normal" counter mode.
> This lead also here to define trigger inputs as a signals. Then the
> standard increase/decrease functions suffice.
> 
> --- dt-bindings ---
> This could be a tight part. Listing/probing the triggers relies on the
> reg property defined in the trigger node:
> - Documentation/devicetree/bindings/mfd/st,stm32-timers.yaml
> 
> timers2: timer@40000000 {
>   timer@1 {
>     compatible = "st,stm32-timer-trigger";
>     reg = <1>;
>   }
>   counter {
>     compatible = "st,stm32-timer-counter";
>   };
> }
> 
> Ideally... adding the reg property to the counter node could have
> helped. But we need to enforce the backward compatibility with existing
> DT binaries. So I think that there's no choice to keep the current bindings.
> 
> This could lead to add some code to parse the trigger node for probing,
> looking for the "reg" property either from:
> - the MFD driver part drivers/mfd/stm32-timers.c
> - the counter driver, although it seems non-standard way to parse aside
> nodes.
> I have no strong opinion and I'm open to suggestions.

I don't have much of a strong opinion either, but I'd consider the MFD
driver to be the more natural location to place this. However, I'm also
open to suggestions if someone thinks it better somewhere else.

> 
> ---
> To conclude, there some open items here, but hopefully nothing blocking.
> In case we sort all these, this will allow to remove the IIO_COUNT
> channel (along with the IIO device) being registered.

I'm certain Jonathan will want some sort of deprecation schedule first
to make sure any existing users have time to migrate to the Counter
interface before we remove the IIO one, but it will give me a nice
feeling of completion to see the last of IIO_COUNT superceded by the
Counter interface. ;-)

William Breathitt Gray

> 
> There will still remain some specific attributes in the
> stm32-timer-trigger driver, related to the trigger device:
> - /sys/bus/iio/devices/triggerX/master_mode
> - /sys/bus/iio/devices/triggerX/master_mode_available
> But this shouldn't be an issue as it isn't related to the IIO_COUNT part
> of the driver.
> 
> Please advise,
> Best Regards,
> Fabrice
> 
> > 
> > Thanks,
> > 
> > William Breathitt Gray

Attachment: signature.asc
Description: PGP signature