Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Julien Panis]

On 09/08/2022 08:19, Julien Panis wrote:
> On 08/08/2022 18:19, William Breathitt Gray wrote:
> > The Counter subsystem is relatively nascent so the number of existing
> > Counter device drivers to study is unfortunately sparse. If you
> > encounter any trouble along the way as you work on this, please don't
> > hestitate to reach out and I'll be happy to answer any questions you may
> > have. That said, here are some more hints that can help guide you. :-)
> >
> > Although we've been using CAPx to refer to these registers, in the
> > context of sysfs it'll be better to call the attributes "capture1",
> > "capture2", etc.; that will make their use as capture buffers more
> > obvious. Furthermore, despite my previous example, I think it's better
> > to have these exist underneath the CTR hierarchy rather than Mod4
> > because they are captures of the CTR value:
> >
> > * CTR: /sys/bus/counter/devices/counterX/count0/count
> > * CAP1: /sys/bus/counter/devices/counterX/count0/capture1
> > * CAP2: /sys/bus/counter/devices/counterX/count0/capture2
> > * CAP3: /sys/bus/counter/devices/counterX/count0/capture3
> > * CAP4: /sys/bus/counter/devices/counterX/count0/capture4
> >
> > In your device driver, you would define a struct counter_count to
> > represent CTR. In this struct counter_count there is an "ext" member
> > where you provide an array of struct count_comp. Each CAPx will have a
> > corresponding struct count_comp; it'll look something like this::
> >
> >          static struct counter_comp ctr_count_ext[] = {
> >                  COUNTER_COMP_COUNT_U64("capture1", cap1_read, NULL),
> >                  COUNTER_COMP_COUNT_U64("capture2", cap2_read, NULL),
> >                  COUNTER_COMP_COUNT_U64("capture3", cap3_read, NULL),
> >                  COUNTER_COMP_COUNT_U64("capture4", cap4_read, NULL),
> >          };
> >
> > As you already know, counter_push_event() lets you push Counter events
> > in your interrupt handler. I recommend introducing a new event type
> > under enum counter_event_type in the include/uapi/linux/counter.h header
> > file; something like COUNTER_EVENT_CAPTURE should be descriptive enough.
> >
> > The "channel" member of struct counter_watch refers to Counter event
> > channels; The purpose here is to allow us to support concurrent events
> > of the same type (e.g. two COUNTER_EVENT_OVERFLOW but for different
> > Counts). If I understand the TI ECAP device correctly, we'll be getting
> > a COUNTER_EVENT_CAPTURE event whenever a CAPx register is updated with a
> > new capture. It's up to you if you want to group them under the same
> > channel or separate channels for each CAPx; you would just pass the
> > channel in counter_push_event() to indicate which COUNTER_EVENT_CAPTURE
> > event is being handled.
>
> 2 options, indeed :
> 1) By grouping them under the same channel, the only thing I'm not a 
> great fan of is that
> I will need to use separate functions to read capture registers 
> (cap1_read/cap2_read/
> cap3_read/cap4_read), and also to set/get polarity.
> 2) By using separate channels, code will look more simple but it is 
> likely to be a little bit
> confusing for the user.
> I will probably use option 2.

[ERRATUM] : Sorry, I meant that I will probably use option 1 (not 2), to 
avoid confusion for the user.