Message-ID: <ee85e515-1265-22d7-109f-5cef94571d76@baylibre.com>
Date:   Tue, 9 Aug 2022 08:23:53 +0200
From:   Julien Panis <jpanis@...libre.com>
To:     William Breathitt Gray <william.gray@...aro.org>
Cc:     Jonathan Cameron <jic23@...nel.org>, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, lars@...afoo.de,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, mranostay@...com
Subject: Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support
 for ECAP



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.

Powered by blists - more mailing lists