| Message ID | 20250227144023.64530-2-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | microchip-tcb-capture: Add Capture, Compare, Overflow etc. events | expand |
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/
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
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
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_ */
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