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
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/
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
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
© 2016 - 2026 Red Hat, Inc.