On 16/08/2022 17:12, William Breathitt Gray wrote:
On Tue, Aug 16, 2022 at 09:58:10AM +0200, Julien Panis wrote:
On 14/08/2022 19:03, William Breathitt Gray wrote:
On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
+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)
Sorry for the confusion, I'm talking about ECAP_TSCNT_REG (32-bit) here.
You should rename this Count in your ecap_cnt_counts array from
"ECAPCNT" to "Time-Stamp Counter" to make it clearer to users as well;
it would be prudent to rename "ECAPSIG" too.
I didn't know that there was a register exposing the Mod4 counter value.
If that's true, then define a Count for the Mod4 counter in your
ecap_cnt_counts array.
There is no dedicated register for that, but it would be possible to
expose this value
(when interruptions are triggered, we would just need to parse flags
1/2/3/4 to determine
mod4 counter current state). That would not be very useful, though.
+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.
Okay that's good. By the way, COUNTER_COMP_ENABLE ensures the enable
value passed to ecap_cnt_enable_write() is either 0 or 1, so you don't
need the enable > 1 check in your callback.
+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.
Instead of limiting the event push to just the 4th capture, I'd push
COUNTER_EVENT_CAPTURE on every capture but delegate them to their own
channels::
counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, i);
I prefer using only 1 signal if you don't mind. I think it's less
confusing for the user.
The captures operate as a circular buffer, so the user can determine the
current capture index based on the watch channel reported and perform a
modulo to read the buffers in right sequence. Alternatively, they can
watch just channel 3 if they want to process only four captures at a
time.
William Breathitt Gray
