[PATCH v6 0/3] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[PATCH v6 0/3] microchip-tcb-capture: Add Capture, Compare, Overflow etc. events

[Bence Csókás]

The TCB has three R/W-able "general purpose" hardware registers:
RA, RB and RC. The hardware is capable of:
* sampling Counter Value Register (CV) to RA/RB on a trigger edge
* sending an interrupt of this change
* sending an interrupt on CV change due to trigger
* triggering an interrupt on CV compare to RC
* stop counting after sampling to RB

To enable using these features in user-space, an interrupt handler
was added, generating the necessary counter events. On top, RA/RB
registers are added as Count Extensions. To aid interoperation, a
uapi header was also added, containing the various numeral IDs of
the Extensions, Event channels etc.

Bence Csókás (3):
  include: uapi: counter: Add microchip-tcb-capture.h
  counter: microchip-tcb-capture: Add IRQ handling
  counter: microchip-tcb-capture: Add capture extensions for registers
    RA/RB

 MAINTAINERS                                   |   1 +
 drivers/counter/microchip-tcb-capture.c       | 131 ++++++++++++++++++
 .../linux/counter/microchip-tcb-capture.h     |  49 +++++++
 3 files changed, 181 insertions(+)
 create mode 100644 include/uapi/linux/counter/microchip-tcb-capture.h

-- 
2.48.1

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

[Bence Csókás]

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

[PATCH v6 2/3] counter: microchip-tcb-capture: Add IRQ handling

[Bence Csókás]

Add interrupt servicing to allow userspace to wait for the following events:
* Change-of-state caused by external trigger
* Capture of timer value into RA/RB
* Compare to RC register
* Overflow

Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
---

Notes:
    New in v2
    Changes in v3:
    * Add IRQs for Capture events (from next patch)
    * Add IRQ for RC Compare
    * Add events as bullet points to commit msg
    Changes in v4:
    * Add uapi header, names for the event channels
    * Add check for -EPROBE_DEFER from `of_irq_get()`
    Changes in v5:
    * Split out UAPI header introduction

 drivers/counter/microchip-tcb-capture.c       | 75 +++++++++++++++++++
 .../linux/counter/microchip-tcb-capture.h     | 18 +++++
 2 files changed, 93 insertions(+)

diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index 2f096a5b973d..cc12c2e2113a 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -6,18 +6,24 @@
  */
 #include <linux/clk.h>
 #include <linux/counter.h>
+#include <linux/interrupt.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <uapi/linux/counter/microchip-tcb-capture.h>
 #include <soc/at91/atmel_tcb.h>
 
 #define ATMEL_TC_CMR_MASK	(ATMEL_TC_LDRA_RISING | ATMEL_TC_LDRB_FALLING | \
 				 ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_LDBDIS | \
 				 ATMEL_TC_LDBSTOP)
 
+#define ATMEL_TC_DEF_IRQS	(ATMEL_TC_ETRGS | ATMEL_TC_COVFS | \
+				 ATMEL_TC_LDRAS | ATMEL_TC_LDRBS | ATMEL_TC_CPCS)
+
 #define ATMEL_TC_QDEN			BIT(8)
 #define ATMEL_TC_POSEN			BIT(9)
 
@@ -27,6 +33,7 @@ struct mchp_tc_data {
 	int qdec_mode;
 	int num_channels;
 	int channel[2];
+	int irq;
 };
 
 static const enum counter_function mchp_tc_count_functions[] = {
@@ -294,6 +301,65 @@ static const struct of_device_id atmel_tc_of_match[] = {
 	{ /* sentinel */ }
 };
 
+static irqreturn_t mchp_tc_isr(int irq, void *dev_id)
+{
+	struct counter_device *const counter = dev_id;
+	struct mchp_tc_data *const priv = counter_priv(counter);
+	u32 sr, mask;
+
+	regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr);
+	regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], IMR), &mask);
+
+	sr &= mask;
+	if (!(sr & ATMEL_TC_ALL_IRQ))
+		return IRQ_NONE;
+
+	if (sr & ATMEL_TC_ETRGS)
+		counter_push_event(counter, COUNTER_EVENT_CHANGE_OF_STATE,
+				   COUNTER_MCHP_EVCHN_CV);
+	if (sr & ATMEL_TC_LDRAS)
+		counter_push_event(counter, COUNTER_EVENT_CAPTURE,
+				   COUNTER_MCHP_EVCHN_RA);
+	if (sr & ATMEL_TC_LDRBS)
+		counter_push_event(counter, COUNTER_EVENT_CAPTURE,
+				   COUNTER_MCHP_EVCHN_RB);
+	if (sr & ATMEL_TC_CPCS)
+		counter_push_event(counter, COUNTER_EVENT_THRESHOLD,
+				   COUNTER_MCHP_EVCHN_RC);
+	if (sr & ATMEL_TC_COVFS)
+		counter_push_event(counter, COUNTER_EVENT_OVERFLOW,
+				   COUNTER_MCHP_EVCHN_CV);
+
+	return IRQ_HANDLED;
+}
+
+static void mchp_tc_irq_remove(void *ptr)
+{
+	struct mchp_tc_data *priv = ptr;
+
+	regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], IDR), ATMEL_TC_DEF_IRQS);
+}
+
+static int mchp_tc_irq_enable(struct counter_device *const counter)
+{
+	struct mchp_tc_data *const priv = counter_priv(counter);
+	int ret = devm_request_irq(counter->parent, priv->irq, mchp_tc_isr, 0,
+				   dev_name(counter->parent), counter);
+
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], IER), ATMEL_TC_DEF_IRQS);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_add_action_or_reset(counter->parent, mchp_tc_irq_remove, priv);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static void mchp_tc_clk_remove(void *ptr)
 {
 	clk_disable_unprepare((struct clk *)ptr);
@@ -378,6 +444,15 @@ static int mchp_tc_probe(struct platform_device *pdev)
 	counter->num_signals = ARRAY_SIZE(mchp_tc_count_signals);
 	counter->signals = mchp_tc_count_signals;
 
+	priv->irq = of_irq_get(np->parent, 0);
+	if (priv->irq == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (priv->irq > 0) {
+		ret = mchp_tc_irq_enable(counter);
+		if (ret < 0)
+			return dev_err_probe(&pdev->dev, ret, "Failed to set up IRQ");
+	}
+
 	ret = devm_counter_add(&pdev->dev, counter);
 	if (ret < 0)
 		return dev_err_probe(&pdev->dev, ret, "Failed to add counter\n");
diff --git a/include/uapi/linux/counter/microchip-tcb-capture.h b/include/uapi/linux/counter/microchip-tcb-capture.h
index 7bda5fdef19b..ee72f1463594 100644
--- a/include/uapi/linux/counter/microchip-tcb-capture.h
+++ b/include/uapi/linux/counter/microchip-tcb-capture.h
@@ -12,6 +12,17 @@
  * Count 0
  * \__  Synapse 0 -- Signal 0 (Channel A, i.e. TIOA)
  * \__  Synapse 1 -- Signal 1 (Channel B, i.e. TIOB)
+ *
+ * It also supports the following events:
+ *
+ * Channel 0:
+ * - CV register changed
+ * - CV overflowed
+ * - RA captured
+ * Channel 1:
+ * - RB captured
+ * Channel 2:
+ * - RC compare triggered
  */
 
 enum counter_mchp_signals {
@@ -19,4 +30,11 @@ enum counter_mchp_signals {
 	COUNTER_MCHP_SIG_TIOB,
 };
 
+enum counter_mchp_event_channels {
+	COUNTER_MCHP_EVCHN_CV = 0,
+	COUNTER_MCHP_EVCHN_RA = 0,
+	COUNTER_MCHP_EVCHN_RB,
+	COUNTER_MCHP_EVCHN_RC,
+};
+
 #endif /* _UAPI_COUNTER_MCHP_TCB_H_ */
-- 
2.48.1

[PATCH v6 3/3] counter: microchip-tcb-capture: Add capture extensions for registers RA/RB

[Bence Csókás]

TCB hardware is capable of capturing the timer value to registers RA and
RB. Add these registers as capture extensions.

Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
---

Notes:
    Changes in v2:
    * Add IRQs
    Changes in v3:
    * Move IRQs to previous patch
    Changes in v4:
    * Return the status of the regmap_*() operations
    * Add names for the extension numbers
    Changes in v6:
    * Remove RC, as it is not a capture register

 drivers/counter/microchip-tcb-capture.c       | 56 +++++++++++++++++++
 .../linux/counter/microchip-tcb-capture.h     |  9 +++
 2 files changed, 65 insertions(+)

diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index cc12c2e2113a..4ba5e29138e7 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -254,6 +254,60 @@ static int mchp_tc_count_read(struct counter_device *counter,
 	return 0;
 }
 
+static int mchp_tc_count_cap_read(struct counter_device *counter,
+				  struct counter_count *count, size_t idx, u64 *val)
+{
+	struct mchp_tc_data *const priv = counter_priv(counter);
+	u32 cnt;
+	int ret;
+
+	switch (idx) {
+	case COUNTER_MCHP_EXCAP_RA:
+		ret = regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), &cnt);
+		break;
+	case COUNTER_MCHP_EXCAP_RB:
+		ret = regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), &cnt);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (!ret)
+		*val = cnt;
+
+	return ret;
+}
+
+static int mchp_tc_count_cap_write(struct counter_device *counter,
+				   struct counter_count *count, size_t idx, u64 val)
+{
+	struct mchp_tc_data *const priv = counter_priv(counter);
+	int ret;
+
+	if (val > U32_MAX)
+		return -ERANGE;
+
+	switch (idx) {
+	case COUNTER_MCHP_EXCAP_RA:
+		ret = regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), val);
+		break;
+	case COUNTER_MCHP_EXCAP_RB:
+		ret = regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), val);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static DEFINE_COUNTER_ARRAY_CAPTURE(mchp_tc_cnt_cap_array, 2);
+
+static struct counter_comp mchp_tc_count_ext[] = {
+	COUNTER_COMP_ARRAY_CAPTURE(mchp_tc_count_cap_read, mchp_tc_count_cap_write,
+				   mchp_tc_cnt_cap_array),
+};
+
 static struct counter_count mchp_tc_counts[] = {
 	{
 		.id = 0,
@@ -262,6 +316,8 @@ static struct counter_count mchp_tc_counts[] = {
 		.num_functions = ARRAY_SIZE(mchp_tc_count_functions),
 		.synapses = mchp_tc_count_synapses,
 		.num_synapses = ARRAY_SIZE(mchp_tc_count_synapses),
+		.ext = mchp_tc_count_ext,
+		.num_ext = ARRAY_SIZE(mchp_tc_count_ext),
 	},
 };
 
diff --git a/include/uapi/linux/counter/microchip-tcb-capture.h b/include/uapi/linux/counter/microchip-tcb-capture.h
index ee72f1463594..5c015fafe42c 100644
--- a/include/uapi/linux/counter/microchip-tcb-capture.h
+++ b/include/uapi/linux/counter/microchip-tcb-capture.h
@@ -12,6 +12,9 @@
  * Count 0
  * \__  Synapse 0 -- Signal 0 (Channel A, i.e. TIOA)
  * \__  Synapse 1 -- Signal 1 (Channel B, i.e. TIOB)
+ * \__  Extension capture0    (RA register)
+ * \__  Extension capture1    (RB register)
+ * \__  Extension capture2    (RC register)
  *
  * It also supports the following events:
  *
@@ -30,6 +33,12 @@ enum counter_mchp_signals {
 	COUNTER_MCHP_SIG_TIOB,
 };
 
+enum counter_mchp_capture_extensions {
+	COUNTER_MCHP_EXCAP_RA,
+	COUNTER_MCHP_EXCAP_RB,
+	COUNTER_MCHP_EXCAP_RC,
+};
+
 enum counter_mchp_event_channels {
 	COUNTER_MCHP_EVCHN_CV = 0,
 	COUNTER_MCHP_EVCHN_RA = 0,
-- 
2.48.1

Re: [PATCH v6 2/3] counter: microchip-tcb-capture: Add IRQ handling

[William Breathitt Gray]

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

On Thu, Feb 27, 2025 at 03:40:19PM +0100, Bence Csókás wrote:
> Add interrupt servicing to allow userspace to wait for the following events:

Hi Bence,

This is a nitpick but keep the commit description with a maximum of 75
characters per line so we don't have formatting issues with how they
wrap.

> @@ -378,6 +444,15 @@ static int mchp_tc_probe(struct platform_device *pdev)
>  	counter->num_signals = ARRAY_SIZE(mchp_tc_count_signals);
>  	counter->signals = mchp_tc_count_signals;
>  
> +	priv->irq = of_irq_get(np->parent, 0);
> +	if (priv->irq == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

In theory, the error code could be something else if of_irq_get() failed
for any other reason. Handle all those error cases at once by checking
IS_ERR(priv->irq) rather than just -EPROBE_DEFER. Then you can just
return dev_err_probe() with priv->irq for the error code.

> diff --git a/include/uapi/linux/counter/microchip-tcb-capture.h b/include/uapi/linux/counter/microchip-tcb-capture.h
> index 7bda5fdef19b..ee72f1463594 100644
> --- a/include/uapi/linux/counter/microchip-tcb-capture.h
> +++ b/include/uapi/linux/counter/microchip-tcb-capture.h
> @@ -12,6 +12,17 @@
>   * Count 0
>   * \__  Synapse 0 -- Signal 0 (Channel A, i.e. TIOA)
>   * \__  Synapse 1 -- Signal 1 (Channel B, i.e. TIOB)
> + *
> + * It also supports the following events:
> + *
> + * Channel 0:
> + * - CV register changed
> + * - CV overflowed
> + * - RA captured
> + * Channel 1:
> + * - RB captured
> + * Channel 2:
> + * - RC compare triggered
>   */
>  
>  enum counter_mchp_signals {
> @@ -19,4 +30,11 @@ enum counter_mchp_signals {
>  	COUNTER_MCHP_SIG_TIOB,
>  };
>  
> +enum counter_mchp_event_channels {
> +	COUNTER_MCHP_EVCHN_CV = 0,
> +	COUNTER_MCHP_EVCHN_RA = 0,
> +	COUNTER_MCHP_EVCHN_RB,
> +	COUNTER_MCHP_EVCHN_RC,
> +};

These would be better as preprocessor defines in case we need to
introduce new events to channel 1 or 2 in the future. That would allow
us to insert new events easily to existing channels without having to
worry about its actual position in an enum list.

One additional benefit is if we do end up introducing more Counts for
the module. In that situation we would have multiple CV and RA/RB/RC per
Counter device, but we can easily define a preprocessor macro to
calculate the channel offset given the Count index. However, with enum
structure we would have to manually add and maintain redundant defines
for each Count, which is far less ideal.

William Breathitt Gray

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

Re: [PATCH v6 3/3] counter: microchip-tcb-capture: Add capture extensions for registers RA/RB

[William Breathitt Gray]

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

On Thu, Feb 27, 2025 at 03:40:20PM +0100, Bence Csókás wrote:
> +static int mchp_tc_count_cap_read(struct counter_device *counter,
> +				  struct counter_count *count, size_t idx, u64 *val)
> +{
> +	struct mchp_tc_data *const priv = counter_priv(counter);
> +	u32 cnt;
> +	int ret;
> +
> +	switch (idx) {
> +	case COUNTER_MCHP_EXCAP_RA:
> +		ret = regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RA), &cnt);
> +		break;
> +	case COUNTER_MCHP_EXCAP_RB:
> +		ret = regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], RB), &cnt);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (!ret)
> +		*val = cnt;
> +
> +	return ret;
> +}

It's cleaner to exit early on an error than to carry it to the end.
Instead of if (!ret), perform an if (ret) return ret to exit early on an
error, then simply return 0 at the end of the funtion.

> diff --git a/include/uapi/linux/counter/microchip-tcb-capture.h b/include/uapi/linux/counter/microchip-tcb-capture.h
> index ee72f1463594..5c015fafe42c 100644
> --- a/include/uapi/linux/counter/microchip-tcb-capture.h
> +++ b/include/uapi/linux/counter/microchip-tcb-capture.h
> @@ -12,6 +12,9 @@
>   * Count 0
>   * \__  Synapse 0 -- Signal 0 (Channel A, i.e. TIOA)
>   * \__  Synapse 1 -- Signal 1 (Channel B, i.e. TIOB)
> + * \__  Extension capture0    (RA register)
> + * \__  Extension capture1    (RB register)
> + * \__  Extension capture2    (RC register)

The capture2 extension doesn't exist in this patch so remove this
comment line.

> @@ -30,6 +33,12 @@ enum counter_mchp_signals {
>  	COUNTER_MCHP_SIG_TIOB,
>  };
>  
> +enum counter_mchp_capture_extensions {
> +	COUNTER_MCHP_EXCAP_RA,
> +	COUNTER_MCHP_EXCAP_RB,
> +	COUNTER_MCHP_EXCAP_RC,
> +};

Remove COUNTER_MCHP_EXCAP_RC for the same reason as above.

Also, I would argue for these to be preprocessor defines rather than
enum for the same reasons as in my other review[^1].

One final comment: is RA/RB the best way to differentiate these? One of
the benefits of abstraction layers is that users won't need to be
concerned about the hardware details, and naming the capture values
after their respective general register hardware names feels somewhat
antithetic to that end.

I imagine there are better ways to refer to these that would communicate
their relationship better, such as "primary capture" and "secondary
capture". However at that point capture0 and capture1 would seem
obvious enough, in which case you might not even need to expose these to
userspace at all.

William Breathitt Gray

[^1] https://lore.kernel.org/all/Z8alaOTjZeRuXnUI@ishi/

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

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

[William Breathitt Gray]

[-- 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 --]

Re: [PATCH v6 2/3] counter: microchip-tcb-capture: Add IRQ handling

[Csókás Bence]

Hi,

On 2025. 03. 04. 8:02, William Breathitt Gray wrote:
> In theory, the error code could be something else if of_irq_get() failed
> for any other reason. Handle all those error cases at once by checking
> IS_ERR(priv->irq) rather than just -EPROBE_DEFER. Then you can just
> return dev_err_probe() with priv->irq for the error code.

Yes, `of_irq_get()` can return an error, for example if the IRQ is not 
defined in the DT. In these cases, we just don't do IRQ, but still allow 
the device to probe. -EPROBE_DEFER is special in this case, because it 
signifies that there *is* an IRQ to set up, just not now.

>> +enum counter_mchp_event_channels {
>> +	COUNTER_MCHP_EVCHN_CV = 0,
>> +	COUNTER_MCHP_EVCHN_RA = 0,
>> +	COUNTER_MCHP_EVCHN_RB,
>> +	COUNTER_MCHP_EVCHN_RC,
>> +};
> 
> These would be better as preprocessor defines in case we need to
> introduce new events to channel 1 or 2 in the future. That would allow
> us to insert new events easily to existing channels without having to
> worry about its actual position in an enum list.

Okay. I personally like the enum interface more, as it is much cleaner, 
but if there's a good reason to use #define's, then so be it.

> One additional benefit is if we do end up introducing more Counts for
> the module. In that situation we would have multiple CV and RA/RB/RC per
> Counter device, but we can easily define a preprocessor macro to
> calculate the channel offset given the Count index. However, with enum
> structure we would have to manually add and maintain redundant defines
> for each Count, which is far less ideal.
> 
> William Breathitt Gray

Bence

Re: [PATCH v6 3/3] counter: microchip-tcb-capture: Add capture extensions for registers RA/RB

[Csókás Bence]

Hi,

On 2025. 03. 04. 8:47, William Breathitt Gray wrote:
> It's cleaner to exit early on an error than to carry it to the end.
> Instead of if (!ret), perform an if (ret) return ret to exit early on an
> error, then simply return 0 at the end of the funtion.

Ok.

> The capture2 extension doesn't exist in this patch so remove this
> comment line.
> 
>> @@ -30,6 +33,12 @@ enum counter_mchp_signals {
>>   	COUNTER_MCHP_SIG_TIOB,
>>   };
>>   
>> +enum counter_mchp_capture_extensions {
>> +	COUNTER_MCHP_EXCAP_RA,
>> +	COUNTER_MCHP_EXCAP_RB,
>> +	COUNTER_MCHP_EXCAP_RC,
>> +};
> 
> Remove COUNTER_MCHP_EXCAP_RC for the same reason as above.
> 
> Also, I would argue for these to be preprocessor defines rather than
> enum for the same reasons as in my other review[^1].

Ok.

> One final comment: is RA/RB the best way to differentiate these? One of
> the benefits of abstraction layers is that users won't need to be
> concerned about the hardware details, and naming the capture values
> after their respective general register hardware names feels somewhat
> antithetic to that end.
> 
> I imagine there are better ways to refer to these that would communicate
> their relationship better, such as "primary capture" and "secondary
> capture". However at that point capture0 and capture1 would seem
> obvious enough, in which case you might not even need to expose these to
> userspace at all.

Hmm. Well, RA and RB is what it says in the datasheet, and since we 
don't do much processing on their value, I'd say we're still closely 
coupled to the hardware. So, if one wants to understand what they do, 
they will have to read the datasheet anyways in which case I think it's 
best to be consistent with it naming-wise.

> William Breathitt Gray
> 
> [^1] https://lore.kernel.org/all/Z8alaOTjZeRuXnUI@ishi/

Bence

Re: [PATCH v6 2/3] counter: microchip-tcb-capture: Add IRQ handling

[William Breathitt Gray]

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

On Tue, Mar 04, 2025 at 10:57:05AM +0100, Csókás Bence wrote:
> On 2025. 03. 04. 8:02, William Breathitt Gray wrote:
> > In theory, the error code could be something else if of_irq_get() failed
> > for any other reason. Handle all those error cases at once by checking
> > IS_ERR(priv->irq) rather than just -EPROBE_DEFER. Then you can just
> > return dev_err_probe() with priv->irq for the error code.
> 
> Yes, `of_irq_get()` can return an error, for example if the IRQ is not
> defined in the DT. In these cases, we just don't do IRQ, but still allow the
> device to probe. -EPROBE_DEFER is special in this case, because it signifies
> that there *is* an IRQ to set up, just not now.

You're right, that makes sense. Thank you for explaining.

William Breathitt Gray

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

Re: [PATCH v6 3/3] counter: microchip-tcb-capture: Add capture extensions for registers RA/RB

[William Breathitt Gray]

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

On Tue, Mar 04, 2025 at 11:03:17AM +0100, Csókás Bence wrote:
> On 2025. 03. 04. 8:47, William Breathitt Gray wrote:
> > One final comment: is RA/RB the best way to differentiate these? One of
> > the benefits of abstraction layers is that users won't need to be
> > concerned about the hardware details, and naming the capture values
> > after their respective general register hardware names feels somewhat
> > antithetic to that end.
> > 
> > I imagine there are better ways to refer to these that would communicate
> > their relationship better, such as "primary capture" and "secondary
> > capture". However at that point capture0 and capture1 would seem
> > obvious enough, in which case you might not even need to expose these to
> > userspace at all.
> 
> Hmm. Well, RA and RB is what it says in the datasheet, and since we don't do
> much processing on their value, I'd say we're still closely coupled to the
> hardware. So, if one wants to understand what they do, they will have to
> read the datasheet anyways in which case I think it's best to be consistent
> with it naming-wise.

All right, let's keep it as RA and RA then.

William Breathitt Gray

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

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

[Csókás Bence]

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

[William Breathitt Gray]

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

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

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