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;
Date:   Sun, 14 Aug 2022 14:00:26 -0400
From:   William Breathitt Gray <william.gray@linaro.org>
To:     Julien Panis <jpanis@baylibre.com>
Cc:     robh+dt@kernel.org, jic23@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        mranostay@ti.com
Subject: Re: [PATCH v4 0/3] ECAP support on TI AM62x SoC
Message-ID: <Yvk4OijjBj+Wa7fT@fedora>
References: <20220810140724.182389-1-jpanis@baylibre.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="T5liTN5OdHlrk5BP"
Content-Disposition: inline
In-Reply-To: <20220810140724.182389-1-jpanis@baylibre.com>
Precedence: bulk


--T5liTN5OdHlrk5BP
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Aug 10, 2022 at 04:07:21PM +0200, Julien Panis wrote:
> The Enhanced Capture (ECAP) module can be used to timestamp events
> detected on signal input pin. It can be used for time measurements
> of pulse train signals.
>=20
> ECAP module includes 4 timestamp capture registers. For all 4 sequenced
> timestamp capture events (1->2->3->4->1->...), edge polarity (falling/ris=
ing
> edge) can be selected.
>=20
> This driver leverages counter subsystem to :
> - select edge polarity for all 4 capture events (event mode)
> - log timestamps for each capture event
> Event polarity, and CAP1/2/3/4 timestamps give all the information
> about the input pulse train. Further information can easily be computed :
> period and/or duty cycle if frequency is constant, elapsed time between
> pulses, etc...
>=20
> Modifications since v3:
> 	- Migrate driver from IIO to Counter subsystem
> 	- Minor modification in yaml ($id) to match Counter subsystem
> 	- Add ABI documentation
>=20
> Userspace commands :
> 	### SIGNAL ###
> 	cd /sys/bus/counter/devices/counter0/signal0
>=20
> 	# Get available polarities for each capture event
> 	cat polarity1_available
> 	cat polarity2_available
> 	cat polarity3_available
> 	cat polarity4_available
>=20
> 	# Get polarity for each capture event
> 	cat polarity1
> 	cat polarity2
> 	cat polarity3
> 	cat polarity4
>=20
> 	# Set polarity for each capture event
> 	echo rising > polarity1
> 	echo falling > polarity2
> 	echo rising > polarity3
> 	echo falling > polarity4
>=20
> 	### COUNT ###
> 	cd /sys/bus/counter/devices/counter0/count0
>=20
> 	# Run ECAP
> 	echo 1 > enable
>=20
> 	# Get current timebase counter value
> 	cat count
>=20
> 	# Get captured timestamps
> 	cat capture1
> 	cat capture2
> 	cat capture3
> 	cat capture4
>=20
> 	# Note that counter watches can also be used to get
> 	# data from userspace application
> 	# -> see tools/counter/counter_example.c
>=20
> 	# Stop ECAP
> 	echo 0 > enable
>=20
> Julien Panis (3):
>   dt-binding: counter: add ti,am62-ecap-capture.yaml
>   Documentation: ABI: add sysfs-bus-counter-ecap
>   counter: capture-tiecap: capture driver support for ECAP
>=20
>  .../ABI/testing/sysfs-bus-counter-ecap        |  64 ++
>  .../counter/ti,am62-ecap-capture.yaml         |  61 ++
>  drivers/counter/Kconfig                       |  14 +
>  drivers/counter/Makefile                      |   1 +
>  drivers/counter/capture-tiecap.c              | 634 ++++++++++++++++++
>  include/uapi/linux/counter.h                  |   2 +
>  6 files changed, 776 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap
>  create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-eca=
p-capture.yaml
>  create mode 100644 drivers/counter/capture-tiecap.c
>=20
> --=20
> 2.25.1

Something that has become apparent to me is the code repetition in this
driver in order to support the capture buffer. Buffers are common
components in devices, so it'll be good for us to standardize some of
what we're exploring here into an interface that other drivers can also
use. We have two ABIs to consider: the driver interface and the sysfs
interface.

For the sysfs interface, I think we'll have to expose each element
individually (e.g. capture1, capture2, etc.) because sysfs attributes
are suppose to expose only a single datum for any given attribute.

For the driver side, we might want to introduce a new Counter component
type for buffers and respective macros to streamline some of the code
for driver authors. For example, a new COUNTER_COMP_BUFFER_U64 enum
counter_comp_type constant could be introduced to represent a u64 buffer
element; respective struct counter_comp read callbacks could be
introduced::

    int (*count_buffer_u64_read)(struct counter_device *counter,
                                 struct counter_count *count,
				 size_t index, u64 *val);

So a driver author can use the "index" parameter to locate the buffer
element and pass back its value via the "val" parameter. To define the
buffer, maybe helper macros like this could be introduced::

    COUNTER_COMP_COUNT_BUFFER_U64("capture", ecap_cnt_cap_read, 4)

This would define four u64 buffer elements each named prefixed with
"capture" and with their read callbacks set to ecap_cnt_cap_read().

One problem however is that I'm not sure if the C preprocessor would be
able to unroll the COUNTER_COMP_COUNT_BUFFER_U64 to a dynamic number of
elements based on a macro parameter (maybe there is a GCC extension).

I'm just throwing out ideas, so I'd like to hear some comments and
suggestions from others about how we should add buffer support to the
Counter subsystem.

William Breathitt Gray

--T5liTN5OdHlrk5BP
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQSNN83d4NIlKPjon7a1SFbKvhIjKwUCYvk4OgAKCRC1SFbKvhIj
K1puAQCKVF17W73GrxMBUVmEpZBsMchm0VgY1hKbIVPA69IlsQEAxjCytel3C7oB
cPBVwjGnEm0sDNveEi6Tbs+QjkxUNgI=
=X96e
-----END PGP SIGNATURE-----

--T5liTN5OdHlrk5BP--