Re: [PATCH v6 1/3] include: uapi: counter: Add microchip-tcb-capture.h

[William Breathitt Gray <wbg@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2247 bytes --]

On Thu, Feb 27, 2025 at 03:40:18PM +0100, Bence Csókás wrote:
> Add UAPI header for the microchip-tcb-capture.c driver.
> This header will hold the various event channels, component numbers etc.
> used by this driver.
> 
> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>

Oops, I almost missed this one! Make sure I'm included in the To field
for the next revision. ;-)

By the way, b4 is a nifty tool that can save you some work and help you
prep patch series for submission.[^1]

> +/*
> + * The driver defines the following components:
> + *
> + * Count 0
> + * \__  Synapse 0 -- Signal 0 (Channel A, i.e. TIOA)
> + * \__  Synapse 1 -- Signal 1 (Channel B, i.e. TIOB)
> + */
> +
> +enum counter_mchp_signals {
> +	COUNTER_MCHP_SIG_TIOA,
> +	COUNTER_MCHP_SIG_TIOB,
> +};

Are these meant to be used to identify the Signals in the
microchip-tcb-capture.c file. You should set the the counter_signal id
members to these enum constants then. However, this enum doesn't need to
be exposed to userspace in that case.

Or is the purpose of this to match the parent ID of the Signal when
you create Counter watches? That won't work safely the way you intend
because the Counter subsystem creates the userspace parent IDs
independent of the kernelspace counter_signal struct id member.

Right now the Counter subsystem just happens to create these parent IDs
sequentially from 0 because it was a simple way to implement it at the
time we introduced the feature. However, there is nothing that ensures
this will stay that way in the future, and in fact the design intention
was exactly to allow the possibility of a future change to this area of
code.

In other words, there's no gurantee the parent ID in userspace will
remain the same even between driver reloads. The intended way for
userspace to behave is to first identify the desired Signals at startup
based on their "name" sysfs attribute and then set Counter watches and
such accordingly thereafter as desired.

If that is the only purpose of enum counter_mchp_signals, then we can
omit this patch from the series and you won't need to send it in the
next revision.

William Breathitt Gray

[^1] https://b4.docs.kernel.org/en/latest/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]