Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

Julien Panis
2022-08-16T09:36:17Z

On 14/08/2022 19:03, William Breathitt Gray wrote:
> On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
>> ECAP hardware on AM62x SoC supports capture feature. It can be used
>> to timestamp events (falling/rising edges) detected on signal input pin.
>>
>> This commit adds capture driver support for ECAP hardware on AM62x SoC.
>>
>> In the ECAP hardware, capture pin can also be configured to be in
>> PWM mode. Current implementation only supports capture operating mode.
>> Hardware also supports timebase sync between multiple instances, but
>> this driver supports simple independent capture functionality.
>>
>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> Hi Julien,
>
> You picked up the Counter paradigm pretty quickly, good job! Some
> suggestions and comments inline below.

Hi William,
Thank you for your review.
My answers to your questions below.

>
>> ---
>> drivers/counter/Kconfig | 14 +
>> drivers/counter/Makefile | 1 +
>> drivers/counter/capture-tiecap.c | 634 +++++++++++++++++++++++++++++++
>> include/uapi/linux/counter.h | 2 +
>> 4 files changed, 651 insertions(+)
>> create mode 100644 drivers/counter/capture-tiecap.c
>>
>> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
>> index 5edd155f1911..580640807a94 100644
>> --- a/drivers/counter/Kconfig
>> +++ b/drivers/counter/Kconfig
>> @@ -101,4 +101,18 @@ config INTEL_QEP
>> To compile this driver as a module, choose M here: the module
>> will be called intel-qep.
>>
>> +config CAPTURE_TIECAP
>> + tristate "TI eCAP capture driver"
>> + depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>> + depends on HAS_IOMEM
>> + help
>> + Select this option to enable the Texas Instruments Enhanced Capture
>> + (eCAP) driver in input mode.
>> +
>> + It can be used to timestamp events (falling/rising edges) detected
>> + on signal input pin.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called capture-tiecap.
>> +
> The naming convention I typically see is to lead with the company name
> followed by device model; e.g. ti-ecap-capture. That should result in
> better grouping with the existing drivers in the drivers/counter
> directory. It's a minor benefit, so let's use that convention unless
> there's a reason why capture-tiecap is better here.

Another driver implementing ECAP PWM functionality was named "pwm-tiecap".
That's why I chose this "capture-tiecap" name. But that's OK for
ti-ecap-capture,
it's better indeed to use the same naming convention for all counter
drivers.
I will also add "select REGMAP_MMIO" to Kconfig, similarly to others drivers
(ti-eqep for instance).

>
>> endif # COUNTER
>> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
>> index 8fde6c100ebc..7fac3e0985b5 100644
>> --- a/drivers/counter/Makefile
>> +++ b/drivers/counter/Makefile
>> @@ -14,3 +14,4 @@ obj-$(CONFIG_TI_EQEP) += ti-eqep.o
>> obj-$(CONFIG_FTM_QUADDEC) += ftm-quaddec.o
>> obj-$(CONFIG_MICROCHIP_TCB_CAPTURE) += microchip-tcb-capture.o
>> obj-$(CONFIG_INTEL_QEP) += intel-qep.o
>> +obj-$(CONFIG_CAPTURE_TIECAP) += capture-tiecap.o
>> diff --git a/drivers/counter/capture-tiecap.c b/drivers/counter/capture-tiecap.c
>> new file mode 100644
>> index 000000000000..0601c65ef203
>> --- /dev/null
>> +++ b/drivers/counter/capture-tiecap.c
>> @@ -0,0 +1,634 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * ECAP Capture driver
>> + *
>> + * Copyright (C) 2022 Julien Panis <jpanis@baylibre.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/counter.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +
>> +#define ECAP_DRV_NAME "ecap"
>> +
>> +/* Registers */
>> +#define ECAP_NB_CAP 4
>> +
>> +#define ECAP_TSCNT_REG 0x00
>> +
>> +#define ECAP_CAP_REG(i) (((i) << 2) + 0x08)
>> +
>> +#define ECAP_ECCTL_REG 0x28
>> +#define ECAP_CAPPOL_BIT(i) BIT((i) << 1)
>> +#define ECAP_EV_MODE_MASK GENMASK(7, 0)
>> +#define ECAP_CAPLDEN_BIT BIT(8)
>> +#define ECAP_CONT_ONESHT_BIT BIT(16)
>> +#define ECAP_STOPVALUE_MASK GENMASK(18, 17)
>> +#define ECAP_REARM_RESET_BIT BIT(19)
>> +#define ECAP_TSCNTSTP_BIT BIT(20)
>> +#define ECAP_SYNCO_DIS_MASK GENMASK(23, 22)
>> +#define ECAP_CAP_APWM_BIT BIT(25)
>> +#define ECAP_ECCTL_EN_MASK (ECAP_CAPLDEN_BIT | ECAP_TSCNTSTP_BIT)
>> +#define ECAP_ECCTL_CFG_MASK (ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK \
>> + | ECAP_ECCTL_EN_MASK | ECAP_REARM_RESET_BIT \
>> + | ECAP_CAP_APWM_BIT | ECAP_CONT_ONESHT_BIT)
>> +
>> +#define ECAP_ECINT_EN_FLG_REG 0x2c
>> +#define ECAP_NB_CEVT (ECAP_NB_CAP + 1)
>> +#define ECAP_CEVT_EN_MASK GENMASK(ECAP_NB_CEVT, ECAP_NB_CAP)
>> +#define ECAP_CEVT_FLG_BIT(i) BIT((i) + 17)
>> +
>> +#define ECAP_ECINT_CLR_FRC_REG 0x30
>> +#define ECAP_INT_CLR_BIT BIT(0)
>> +#define ECAP_CEVT_CLR_BIT(i) BIT((i) + 1)
>> +#define ECAP_CEVT_CLR_MASK GENMASK(ECAP_NB_CEVT, 0)
>> +
>> +#define ECAP_PID_REG 0x5c
>> +
>> +/*
>> + * Event modes
>> + * One bit for each CAPx register : 1 = falling edge / 0 = rising edge
>> + * e.g. mode = 13 = 0xd = 0b1101
>> + * -> falling edge for CAP1-3-4 / rising edge for CAP2
>> + */
>> +#define ECAP_EV_MODE_BIT(i) BIT(i)
>> +
>> +/* ECAP input */
>> +#define ECAP_SIGNAL 0
>> +
>> +static const struct regmap_config ecap_cnt_regmap_config = {
>> + .reg_bits = 32,
>> + .reg_stride = 4,
>> + .val_bits = 32,
>> + .max_register = ECAP_PID_REG,
>> +};
>> +
>> +/**
>> + * struct ecap_cnt_dev - device private data structure
>> + * @enabled: device state
>> + * @clk: device clock
>> + * @clk_rate: device clock rate
>> + * @regmap: device register map
>> + * @elapsed_time: elapsed time since capture start
>> + * @lock: synchronization lock to prevent race conditions when computing times
>> + * @pm_ctx: device context for PM operations
>> + */
>> +struct ecap_cnt_dev {
>> + bool enabled;
>> + struct clk *clk;
>> + unsigned long clk_rate;
>> + struct regmap *regmap;
>> + __aligned_u64 elapsed_time;
>> + spinlock_t lock;
>> + struct {
>> + u8 ev_mode;
>> + unsigned int time_cntr;
>> + } pm_ctx;
>> +};
>> +
>> +static u8 ecap_cnt_capture_get_evmode(struct counter_device *counter)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> + u8 ev_mode = 0;
>> + unsigned int regval;
>> + int i;
>> +
>> + pm_runtime_get_sync(counter->parent);
>> + regmap_read(ecap_dev->regmap, ECAP_ECCTL_REG, &regval);
>> + pm_runtime_put_sync(counter->parent);
>> +
>> + for (i = 0 ; i < ECAP_NB_CAP ; i++) {
>> + if (regval & ECAP_CAPPOL_BIT(i))
>> + ev_mode |= ECAP_EV_MODE_BIT(i);
>> + }
>> +
>> + return ev_mode;
>> +}
>> +
>> +static void ecap_cnt_capture_set_evmode(struct counter_device *counter, u8 ev_mode)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> + unsigned int regval = 0;
>> + int i;
>> +
>> + for (i = 0 ; i < ECAP_NB_CAP ; i++) {
>> + if (ev_mode & ECAP_EV_MODE_BIT(i))
>> + regval |= ECAP_CAPPOL_BIT(i);
>> + }
>> +
>> + pm_runtime_get_sync(counter->parent);
>> + regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_EV_MODE_MASK, regval);
>> + pm_runtime_put_sync(counter->parent);
>> +}
>> +
>> +static void ecap_cnt_capture_enable(struct counter_device *counter, bool rearm)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> + unsigned int regval;
>> +
>> + pm_runtime_get_sync(counter->parent);
>> +
>> + /* Enable interrupts on events */
>> + regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
>> + ECAP_CEVT_EN_MASK, ECAP_CEVT_EN_MASK);
>> +
>> + /* Run counter */
>> + regval = ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK;
>> + if (rearm) {
>> + ecap_dev->elapsed_time = 0;
>> + regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, 0);
>> + regval |= ECAP_REARM_RESET_BIT;
>> + }
>> + regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MASK, regval);
>> +}
>> +
>> +static void ecap_cnt_capture_disable(struct counter_device *counter)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + /* Disable interrupts on events */
>> + regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, ECAP_CEVT_EN_MASK, 0);
>> +
>> + /* Stop counter */
>> + regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK, 0);
>> +
>> + pm_runtime_put_sync(counter->parent);
>> +}
>> +
>> +static int ecap_cnt_count_get_ns(struct counter_device *counter, unsigned int reg, u64 *val)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> + unsigned int regval;
>> + unsigned long flags;
>> +
>> + pm_runtime_get_sync(counter->parent);
>> + spin_lock_irqsave(&ecap_dev->lock, flags);
>> +
>> + /* time_ns = 10^9 * (time_cycles + time_cumul) / clk_rate */
>> + regmap_read(ecap_dev->regmap, reg, &regval);
>> + *val = (regval + ecap_dev->elapsed_time) * NSEC_PER_SEC;
>> + do_div(*val, ecap_dev->clk_rate);
>> +
>> + spin_unlock_irqrestore(&ecap_dev->lock, flags);
>> + pm_runtime_put_sync(counter->parent);
>> +
>> + return 0;
>> +}
>> +
>> +static inline int ecap_cnt_count_read(struct counter_device *counter,
>> + struct counter_count *count,
>> + u64 *val)
>> +{
>> + return ecap_cnt_count_get_ns(counter, ECAP_TSCNT_REG, val);
>> +}
> This conversion to nanoseconds is an interpretation of the count value
> that belongs in userspace. Instead of converting to nanoseconds here,
> simply expose the count value directly and add extensions to expose the
> elapsed time (a Count extension) and clock rate (a Signal extension) as
> well; with these three values you can derive nanoseconds in userspace
> using the time_ns equation in your ecap_cnt_count_get_ns() comment.
>
>> +static int ecap_cnt_function_read(struct counter_device *counter,
>> + struct counter_count *count,
>> + enum counter_function *function)
>> +{
>> + *function = COUNTER_FUNCTION_INCREASE;
>> +
>> + return 0;
>> +}
>> +
>> +static int ecap_cnt_action_read(struct counter_device *counter,
>> + struct counter_count *count,
>> + struct counter_synapse *synapse,
>> + enum counter_synapse_action *action)
>> +{
>> + *action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
>> +
>> + return 0;
>> +}
> Right now you have a Signal defined for the ECAPSIG line, but there is
> at least one more relevant Signal to define: the clock updating ECAPCNT.
> The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
> clock Signal, but for the ECAPSIG Signal you will need to report a
> Synapse action based on the polarity of the next capture (i.e. whether
> high or low).

Just to be sure : by using the word ECAPCNT, I guess that you speak
about the
Mod4 counter (0->1->2->3->0...), don't you ? (2 bits)
Or do you speak about ECAP_TSCNT_REG register content ? (32 bits)

>
>> +
>> +static int ecap_cnt_watch_validate(struct counter_device *counter,
>> + const struct counter_watch *watch)
>> +{
>> + struct counter_event_node *event_node;
>> +
>> + if (watch->channel || watch->event != COUNTER_EVENT_CAPTURE)
>> + return -EINVAL;
> The intention of this conditional is more obvious if you explicitly
> check for watch->channel != 0.
>
>> +
>> + list_for_each_entry(event_node, &counter->next_events_list, l)
>> + if (watch->component.type != COUNTER_COMPONENT_EXTENSION ||
>> + watch->component.scope != COUNTER_SCOPE_COUNT ||
>> + watch->component.parent ||
>> + watch->component.id >= ECAP_NB_CAP)
>> + return -EINVAL;
> You don't need this list_for_each_entry loop at all. The purpose of the
> watch_validate() callback is to make sure a watch isn't added with a
> channel or event configuration that conflicts with existing watches in
> the next_events_list list.
>
> For example, the 104-QUAD-8 device only allows one particular event to
> occur on any given channel. The quad8_watch_validate() function makes
> sure that all watches for a particular channel are set for the same
> event; if a watch for that channel is set for a different event, then
> that watch is invalid because the 104-QUAD-8 device cannot handle such
> a request for two different events on the same channel.
>
> Regarding the TI ECAP device, it looks like it only supports that one
> COUNTER_EVENT_CAPTURE event so you don't need to check the other watches
> in the next_events_list list. Your conditional check above for
> watch->channel and watch->event should be enough by itself.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int ecap_cnt_enable_read(struct counter_device *counter,
>> + struct counter_count *count,
>> + u8 *enable)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + *enable = ecap_dev->enabled;
>> +
>> + return 0;
>> +}
>> +
>> +static int ecap_cnt_enable_write(struct counter_device *counter,
>> + struct counter_count *count,
>> + u8 enable)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + if (enable > 1)
>> + return -EINVAL;
>> + if (enable == ecap_dev->enabled)
>> + return 0;
>> + if (enable)
>> + ecap_cnt_capture_enable(counter, true);
>> + else
>> + ecap_cnt_capture_disable(counter);
>> + ecap_dev->enabled = enable;
>> +
>> + return 0;
>> +}
>> +
>> +static int ecap_cnt_cap_get_pol(struct counter_device *counter,
>> + struct counter_signal *signal,
>> + u32 *pol, u8 inst)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + pm_runtime_get_sync(counter->parent);
>> + *pol = regmap_test_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
>> + pm_runtime_put_sync(counter->parent);
>> +
>> + return 0;
>> +}
>> +
>> +static int ecap_cnt_cap_set_pol(struct counter_device *counter,
>> + struct counter_signal *signal,
>> + u32 pol, u8 inst)
>> +{
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> + if (ecap_dev->enabled)
>> + return -EBUSY;
>> + if (pol > 1)
>> + return -EINVAL;
>> +
>> + pm_runtime_get_sync(counter->parent);
>> + if (pol)
>> + regmap_set_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
>> + else
>> + regmap_clear_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
>> + pm_runtime_put_sync(counter->parent);
>> +
>> + return 0;
>> +}
>> +
>> +static inline int ecap_cnt_cap_read(struct counter_device *counter,
>> + struct counter_count *count,
>> + u64 *cap, u8 inst)
>> +{
>> + return ecap_cnt_count_get_ns(counter, ECAP_CAP_REG(inst), cap);
>> +}
> Return the captured count value directly here; userspace can handle the
> nanosecond interpretation for the same reason explained earlier for the
> ECAPCNT count value.
>
>> +static inline int ecap_cnt_cap1_get_pol(struct counter_device *counter,
>> + struct counter_signal *signal, u32 *pol)
>> +{
>> + return ecap_cnt_cap_get_pol(counter, signal, pol, 0);
>> +}
>> +
>> +static inline int ecap_cnt_cap2_get_pol(struct counter_device *counter,
>> + struct counter_signal *signal, u32 *pol)
>> +{
>> + return ecap_cnt_cap_get_pol(counter, signal, pol, 1);
>> +}
>> +
>> +static inline int ecap_cnt_cap3_get_pol(struct counter_device *counter,
>> + struct counter_signal *signal, u32 *pol)
>> +{
>> + return ecap_cnt_cap_get_pol(counter, signal, pol, 2);
>> +}
>> +
>> +static inline int ecap_cnt_cap4_get_pol(struct counter_device *counter,
>> + struct counter_signal *signal, u32 *pol)
>> +{
>> + return ecap_cnt_cap_get_pol(counter, signal, pol, 3);
>> +}
>> +
>> +static inline int ecap_cnt_cap1_set_pol(struct counter_device *counter,
>> + struct counter_signal *signal, u32 pol)
>> +{
>> + return ecap_cnt_cap_set_pol(counter, signal, pol, 0);
>> +}
>> +
>> +static inline int ecap_cnt_cap2_set_pol(struct counter_device *counter,
>> + struct counter_signal *signal, u32 pol)
>> +{
>> + return ecap_cnt_cap_set_pol(counter, signal, pol, 1);
>> +}
>> +
>> +static inline int ecap_cnt_cap3_set_pol(struct counter_device *counter,
>> + struct counter_signal *signal, u32 pol)
>> +{
>> + return ecap_cnt_cap_set_pol(counter, signal, pol, 2);
>> +}
>> +
>> +static inline int ecap_cnt_cap4_set_pol(struct counter_device *counter,
>> + struct counter_signal *signal, u32 pol)
>> +{
>> + return ecap_cnt_cap_set_pol(counter, signal, pol, 3);
>> +}
>> +
>> +static inline int ecap_cnt_cap1_read(struct counter_device *counter,
>> + struct counter_count *count, u64 *cap)
>> +{
>> + return ecap_cnt_cap_read(counter, count, cap, 0);
>> +}
>> +
>> +static inline int ecap_cnt_cap2_read(struct counter_device *counter,
>> + struct counter_count *count, u64 *cap)
>> +{
>> + return ecap_cnt_cap_read(counter, count, cap, 1);
>> +}
>> +
>> +static inline int ecap_cnt_cap3_read(struct counter_device *counter,
>> + struct counter_count *count, u64 *cap)
>> +{
>> + return ecap_cnt_cap_read(counter, count, cap, 2);
>> +}
>> +
>> +static inline int ecap_cnt_cap4_read(struct counter_device *counter,
>> + struct counter_count *count, u64 *cap)
>> +{
>> + return ecap_cnt_cap_read(counter, count, cap, 3);
>> +}
> There's a lot of boilerplate code here for each ECAP register read/write
> callback. Try defining a macro to build these so you can define each
> read/write callback pair more succinctly.

I tried to use a macro but did not find any clean way to do that. I will
try again, maybe
some Zephyr code could be inspiring for that kind of static definition
using macros.

>
>> +static const struct counter_ops ecap_cnt_ops = {
>> + .count_read = ecap_cnt_count_read,
>> + .function_read = ecap_cnt_function_read,
>> + .action_read = ecap_cnt_action_read,
>> + .watch_validate = ecap_cnt_watch_validate,
>> +};
>> +
>> +static const enum counter_function ecap_cnt_functions[] = {
>> + COUNTER_FUNCTION_INCREASE,
>> +};
>> +
>> +static const enum counter_synapse_action ecap_cnt_actions[] = {
>> + COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
>> +};
>> +
>> +static const char *const ecap_cnt_cap_polarities[] = {
>> + "rising",
>> + "falling",
>> +};
>> +
>> +static DEFINE_COUNTER_ENUM(ecap_cnt_cap_avail_pol, ecap_cnt_cap_polarities);
>> +
>> +static struct counter_comp ecap_cnt_signal_ext[] = {
>> + COUNTER_COMP_SIGNAL_ENUM("polarity1", ecap_cnt_cap1_get_pol,
>> + ecap_cnt_cap1_set_pol, ecap_cnt_cap_avail_pol),
>> + COUNTER_COMP_SIGNAL_ENUM("polarity2", ecap_cnt_cap2_get_pol,
>> + ecap_cnt_cap2_set_pol, ecap_cnt_cap_avail_pol),
>> + COUNTER_COMP_SIGNAL_ENUM("polarity3", ecap_cnt_cap3_get_pol,
>> + ecap_cnt_cap3_set_pol, ecap_cnt_cap_avail_pol),
>> + COUNTER_COMP_SIGNAL_ENUM("polarity4", ecap_cnt_cap4_get_pol,
>> + ecap_cnt_cap4_set_pol, ecap_cnt_cap_avail_pol),
>> +};
>> +
>> +static struct counter_signal ecap_cnt_signals[] = {
>> + {
>> + .id = ECAP_SIGNAL,
>> + .name = "ECAPSIG",
>> + .ext = ecap_cnt_signal_ext,
>> + .num_ext = ARRAY_SIZE(ecap_cnt_signal_ext),
>> + },
>> +};
> As mentioned earlier, define another Signal here representing the clock
> feeding ECAPCNT. You should also define another extensions array for
> this new Signal that will provide the clock rate for userspace to read.
>
>> +static struct counter_synapse ecap_cnt_synapses[] = {
>> + {
>> + .actions_list = ecap_cnt_actions,
>> + .num_actions = ARRAY_SIZE(ecap_cnt_actions),
>> + .signal = &ecap_cnt_signals[ECAP_SIGNAL],
>> + },
>> +};
> You will need to define another Synapse here for the clock Signal
> mentioned above; similarly, you need to define another actions array for
> the possible edge triggers.
>
>> +static struct counter_comp ecap_cnt_count_ext[] = {
>> + COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
>> + COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
>> + COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
>> + COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),
>> + COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),
> I just want to verify: this enable extension should disable the ECAPCNT
> count itself (i.e. no more increasing count value). Is that what's
> happening here, or is this meant to disable just the captures?

Yes, it is what's happening here : no more increasing count value.

>
>> +};
>> +
>> +static struct counter_count ecap_cnt_counts[] = {
>> + {
>> + .id = 0,
>> + .name = "ECAPCNT",
>> + .functions_list = ecap_cnt_functions,
>> + .num_functions = ARRAY_SIZE(ecap_cnt_functions),
>> + .synapses = ecap_cnt_synapses,
>> + .num_synapses = ARRAY_SIZE(ecap_cnt_synapses),
>> + .ext = ecap_cnt_count_ext,
>> + .num_ext = ARRAY_SIZE(ecap_cnt_count_ext),
>> + },
>> +};
>> +
>> +static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
>> +{
>> + struct counter_device *counter_dev = dev_id;
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
>> + unsigned int clr = 0;
>> + unsigned int flg;
>> + int i;
>> + unsigned long flags;
>> +
>> + regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
>> +
>> + for (i = ECAP_NB_CAP - 1 ; i < ECAP_NB_CEVT ; i++) {
> Would you walk me through the logic for this loop. Is this for-loop
> intended to loop through all four capture indices? ECAP_NB_CAP and
> ECAP_NB_CEVT are defines, so your for-loop has i=3 and i<5; is this what
> you want?

In previous versions (IIO subsys), this for-loop was intended to loop
through all 4 capture indices
and overflow flag.
In this version, it has been modified to loop only for the last capture
indice (the 4th)
and overflow flag : yes, this is intentional. Only 1 event has to be
pushed so that the user
gets all 4 captured timestamps in a single-reading (using 4 watches).
But if I understand well your previous suggestion, you would like
tracking Mod4 counter value,
don't you ? So, I will change back this for-loop, so that it loops for
all capture indices (and
overflow flag) -> For all 4 capture indices, Mod4 count will be updated.
And event will still be
pushed only for the 4th capture indice.

>
>> + if (flg & ECAP_CEVT_FLG_BIT(i)) {
>> + if (i < ECAP_NB_CAP) {
>> + /* Input signal edge detected on last CAP (CAP4) */
>> + counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, 0);
>> + } else {
>> + /* Counter overflow */
>> + spin_lock_irqsave(&ecap_dev->lock, flags);
>> + ecap_dev->elapsed_time += (u64)U32_MAX + 1;
>> + spin_unlock_irqrestore(&ecap_dev->lock, flags);
> Should we push COUNTER_EVENT_OVERFLOW here? Since elapsed_time always
> increments by the same constant (U32_MAX), it would be better to replace
> this with an atomic_t "num_overflows" to track the number of overflows
> with atomic_inc() and compute the particular elapsed_time later.

I agree with this suggestion.

>
> William Breathitt Gray
>
>> + }
>> +
>> + clr |= ECAP_CEVT_CLR_BIT(i);
>> + }
>> + }
>> +
>> + clr |= ECAP_INT_CLR_BIT;
>> + regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_CLR_FRC_REG, ECAP_CEVT_CLR_MASK, clr);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static void ecap_cnt_clk_disable(void *clk)
>> +{
>> + clk_disable_unprepare(clk);
>> +}
>> +
>> +static int ecap_cnt_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct ecap_cnt_dev *ecap_dev;
>> + struct counter_device *counter_dev;
>> + void __iomem *mmio_base;
>> + int ret;
>> +
>> + counter_dev = devm_counter_alloc(dev, sizeof(*ecap_dev));
>> + if (IS_ERR(counter_dev))
>> + return PTR_ERR(counter_dev);
>> +
>> + counter_dev->name = ECAP_DRV_NAME;
>> + counter_dev->parent = dev;
>> + counter_dev->ops = &ecap_cnt_ops;
>> + counter_dev->signals = ecap_cnt_signals;
>> + counter_dev->num_signals = ARRAY_SIZE(ecap_cnt_signals);
>> + counter_dev->counts = ecap_cnt_counts;
>> + counter_dev->num_counts = ARRAY_SIZE(ecap_cnt_counts);
>> +
>> + ecap_dev = counter_priv(counter_dev);
>> +
>> + ecap_dev->clk = devm_clk_get(dev, "fck");
>> + if (IS_ERR(ecap_dev->clk))
>> + return dev_err_probe(dev, PTR_ERR(ecap_dev->clk), "failed to get clock\n");
>> +
>> + ret = clk_prepare_enable(ecap_dev->clk);
>> + if (ret) {
>> + dev_err(dev, "failed to enable clock\n");
>> + return ret;
>> + }
>> +
>> + ret = devm_add_action_or_reset(dev, ecap_cnt_clk_disable, ecap_dev->clk);
>> + if (ret) {
>> + dev_err(dev, "failed to add clock disable action\n");
>> + return ret;
>> + }
>> +
>> + ecap_dev->clk_rate = clk_get_rate(ecap_dev->clk);
>> + if (!ecap_dev->clk_rate) {
>> + dev_err(dev, "failed to get clock rate\n");
>> + return -EINVAL;
>> + }
>> +
>> + mmio_base = devm_platform_ioremap_resource(pdev, 0);
>> + if (IS_ERR(mmio_base))
>> + return PTR_ERR(mmio_base);
>> +
>> + ecap_dev->regmap = devm_regmap_init_mmio(dev, mmio_base, &ecap_cnt_regmap_config);
>> + if (IS_ERR(ecap_dev->regmap))
>> + return dev_err_probe(dev, PTR_ERR(ecap_dev->regmap), "failed to init regmap\n");
>> +
>> + spin_lock_init(&ecap_dev->lock);
>> +
>> + ret = platform_get_irq(pdev, 0);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to get irq\n");
>> + return ret;
>> + }
>> +
>> + ret = devm_request_irq(dev, ret, ecap_cnt_isr, 0, pdev->name, counter_dev);
>> + if (ret) {
>> + dev_err(dev, "failed to request irq\n");
>> + return ret;
>> + }
>> +
>> + platform_set_drvdata(pdev, counter_dev);
>> + pm_runtime_enable(dev);
>> +
>> + ecap_dev->enabled = 0;
>> + ecap_cnt_capture_set_evmode(counter_dev, 0);
>> +
>> + ret = devm_counter_add(dev, counter_dev);
>> + if (ret) {
>> + dev_err(dev, "failed to add counter\n");
>> + pm_runtime_disable(dev);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int ecap_cnt_remove(struct platform_device *pdev)
>> +{
>> + struct counter_device *counter_dev = platform_get_drvdata(pdev);
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
>> +
>> + if (ecap_dev->enabled)
>> + ecap_cnt_capture_disable(counter_dev);
>> +
>> + pm_runtime_disable(&pdev->dev);
>> +
>> + return 0;
>> +}
>> +
>> +static int ecap_cnt_suspend(struct device *dev)
>> +{
>> + struct counter_device *counter_dev = dev_get_drvdata(dev);
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
>> +
>> + /* If eCAP is running, stop capture then save timestamp counter */
>> + if (ecap_dev->enabled) {
>> + ecap_cnt_capture_disable(counter_dev);
>> +
>> + pm_runtime_get_sync(dev);
>> + regmap_read(ecap_dev->regmap, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.time_cntr);
>> + pm_runtime_put_sync(dev);
>> + }
>> +
>> + ecap_dev->pm_ctx.ev_mode = ecap_cnt_capture_get_evmode(counter_dev);
>> +
>> + return 0;
>> +}
>> +
>> +static int ecap_cnt_resume(struct device *dev)
>> +{
>> + struct counter_device *counter_dev = dev_get_drvdata(dev);
>> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
>> +
>> + ecap_cnt_capture_set_evmode(counter_dev, ecap_dev->pm_ctx.ev_mode);
>> +
>> + /* If eCAP was running, restore timestamp counter then run capture */
>> + if (ecap_dev->enabled) {
>> + pm_runtime_get_sync(dev);
>> + regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, ecap_dev->pm_ctx.time_cntr);
>> + pm_runtime_put_sync(dev);
>> +
>> + ecap_cnt_capture_enable(counter_dev, false);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(ecap_cnt_pm_ops, ecap_cnt_suspend, ecap_cnt_resume);
>> +
>> +static const struct of_device_id ecap_cnt_of_match[] = {
>> + { .compatible = "ti,am62-ecap-capture" },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, ecap_cnt_of_match);
>> +
>> +static struct platform_driver ecap_cnt_driver = {
>> + .probe = ecap_cnt_probe,
>> + .remove = ecap_cnt_remove,
>> + .driver = {
>> + .name = "ecap-capture",
>> + .of_match_table = ecap_cnt_of_match,
>> + .pm = pm_sleep_ptr(&ecap_cnt_pm_ops),
>> + },
>> +};
>> +module_platform_driver(ecap_cnt_driver);
>> +
>> +MODULE_DESCRIPTION("ECAP Capture driver");
>> +MODULE_AUTHOR("Julien Panis <jpanis@baylibre.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
>> index 96c5ffd368ad..4c5372c6f2a3 100644
>> --- a/include/uapi/linux/counter.h
>> +++ b/include/uapi/linux/counter.h
>> @@ -63,6 +63,8 @@ enum counter_event_type {
>> COUNTER_EVENT_INDEX,
>> /* State of counter is changed */
>> COUNTER_EVENT_CHANGE_OF_STATE,
>> + /* Count value is captured */
>> + COUNTER_EVENT_CAPTURE,
>> };
>>
>> /**
>> --
>> 2.25.1
>>