Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ee85e515-1265-22d7-109f-5cef94571d76@baylibre.com>
Date:   Tue, 9 Aug 2022 08:23:53 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.1.0
Subject: Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support
 for ECAP
Content-Language: en-US
From:   Julien Panis <jpanis@baylibre.com>
To:     William Breathitt Gray <william.gray@linaro.org>
Cc:     Jonathan Cameron <jic23@kernel.org>, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, lars@metafoo.de,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, mranostay@ti.com
References: <20220728175124.468461-1-jpanis@baylibre.com>
 <20220728175124.468461-3-jpanis@baylibre.com>
 <20220731164116.30e91f34@jic23-huawei>
 <11b7436b-5c31-671e-ba77-435fe8e3b767@baylibre.com>
 <98d17617-72b5-6330-d4f5-1bece928ceab@baylibre.com> <YvBZQsiAm6f3yxME@fedora>
 <d47c842b-579d-b119-73f9-e8c95984b30b@baylibre.com> <YvE3m+jLPuLvXXdS@fedora>
 <733242ec-2f17-e843-5e36-2f804c040a9d@baylibre.com>
In-Reply-To: <733242ec-2f17-e843-5e36-2f804c040a9d@baylibre.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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.