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

Bence Csókás posted 3 patches 11 months, 2 weeks ago
There is a newer version of this series
[PATCH v6 1/3] include: uapi: counter: Add microchip-tcb-capture.h
Posted by Bence Csókás 11 months, 2 weeks ago
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>
---

Notes:
    New in v5

 MAINTAINERS                                   |  1 +
 .../linux/counter/microchip-tcb-capture.h     | 22 +++++++++++++++++++
 2 files changed, 23 insertions(+)
 create mode 100644 include/uapi/linux/counter/microchip-tcb-capture.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e047e20fbd8..d1d264210690 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15579,6 +15579,7 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	drivers/counter/microchip-tcb-capture.c
+F:	include/uapi/linux/counter/microchip-tcb-capture.h
 
 MICROCHIP USB251XB DRIVER
 M:	Richard Leitner <richard.leitner@skidata.com>
diff --git a/include/uapi/linux/counter/microchip-tcb-capture.h b/include/uapi/linux/counter/microchip-tcb-capture.h
new file mode 100644
index 000000000000..7bda5fdef19b
--- /dev/null
+++ b/include/uapi/linux/counter/microchip-tcb-capture.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Channel numbers used by the microchip-tcb-capture driver
+ * Copyright (C) 2025 Bence Csókás
+ */
+#ifndef _UAPI_COUNTER_MCHP_TCB_H_
+#define _UAPI_COUNTER_MCHP_TCB_H_
+
+/*
+ * 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,
+};
+
+#endif /* _UAPI_COUNTER_MCHP_TCB_H_ */
-- 
2.48.1


Re: [PATCH v6 1/3] include: uapi: counter: Add microchip-tcb-capture.h
Posted by William Breathitt Gray 11 months, 1 week ago
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/
Re: [PATCH v6 1/3] include: uapi: counter: Add microchip-tcb-capture.h
Posted by Csókás Bence 11 months, 1 week ago
Hi,

On 2025. 03. 04. 10:51, William Breathitt Gray wrote:
> 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]

Yes, I have considered it, but unfortunately it still has quite a few 
bugs, for example [1], which has mangled my tags before, when a 
maintainer using it tried to apply one of my patches with it.

[1] https://github.com/mricon/b4/issues/52

>> +/*
>> + * 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.

The thought was to let userspace figure out which 
`signal%d_action_component_id` to read, but now I see that this is not 
the way to go.

> 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.

Alright, I'll drop this enum. Then this header will be empty at the 
start, save for the block comment. I hope that will be alright.

> William Breathitt Gray
> 
> [^1] https://b4.docs.kernel.org/en/latest/

Bence

Re: [PATCH v6 1/3] include: uapi: counter: Add microchip-tcb-capture.h
Posted by William Breathitt Gray 11 months, 1 week ago
On Tue, Mar 04, 2025 at 12:14:04PM +0100, Csókás Bence wrote:
> Hi,
> 
> On 2025. 03. 04. 10:51, William Breathitt Gray wrote:
> > 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]
> 
> Yes, I have considered it, but unfortunately it still has quite a few bugs,
> for example [1], which has mangled my tags before, when a maintainer using
> it tried to apply one of my patches with it.
> 
> [1] https://github.com/mricon/b4/issues/52

Oh that is unfortunate, you'll have to continue submitting manually for
now until those bugs are fixed. :-(

> > > +/*
> > > + * 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.
> 
> The thought was to let userspace figure out which
> `signal%d_action_component_id` to read, but now I see that this is not the
> way to go.
> 
> > 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.
> 
> Alright, I'll drop this enum. Then this header will be empty at the start,
> save for the block comment. I hope that will be alright.

No, it'll better to drop this patch and introduce the header in the IRQ
handling patch. You can merge the MAINTAINERS change there as well.

William Breathitt Gray