Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 9 Aug 2020 13:51:45 -0400
From:   William Breathitt Gray <vilhelm.gray@gmail.com>
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     kamel.bouhara@bootlin.com, gwendal@chromium.org,
        alexandre.belloni@bootlin.com, david@lechnology.com,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        linux-arm-kernel@lists.infradead.org, syednwaris@gmail.com,
        patrick.havelange@essensium.com, fabrice.gasnier@st.com,
        mcoquelin.stm32@gmail.com, alexandre.torgue@st.com
Subject: Re: [PATCH v4 0/5] Introduce the Counter character device interface
Message-ID: <20200809175145.GB6542@shinobu>
References: <cover.1595358237.git.vilhelm.gray@gmail.com>
 <20200809144800.6b067dea@archlinux>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="0ntfKIWw70PvrIHh"
Content-Disposition: inline
In-Reply-To: <20200809144800.6b067dea@archlinux>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--0ntfKIWw70PvrIHh
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Aug 09, 2020 at 02:48:00PM +0100, Jonathan Cameron wrote:
> On Tue, 21 Jul 2020 15:35:46 -0400
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> > The following are some questions I have about this patchset:
> >=20
> > 1. Should I support multiple file descriptors for the character device
> >    in this introduction patchset?
> >=20
> >    I intend to add support for multiple file descriptors to the Counter
> >    character device, but I restricted this patchset to a single file
> >    descriptor to simplify the code logic for the sake of review. If
> >    there is enough interest, I can add support for multiple file
> >    descriptors in the next revision; I anticipate that this should be
> >    simple to implement through the allocation of a kfifo for each file
> >    descriptor during the open callback.
>=20
> What is the use case?  I can conjecture one easily enough, but I'm not
> sure how real it actually is.  We've been around this question a few
> times in IIO :)
>=20
> Certainly makes sense to design an interface that would allow you to
> add this support later if needed though.

I don't have any particular use case in mind, but I figured it would be
useful. For example, a counter device can have multiple channels with
their own events, but any particular channel might be counting the
signals of an independent device unrelated to the other channels; in
this scenario, two independent user applications might need access to
the same counter device.

Of course, supporting multiple file descriptors is something that can be
added later so perhaps it's best for us to wait until the need arises
with a real-life use case.

> >=20
> > 2. Should struct counter_event have a union for different value types,
> >    or just a value u8 array?
> >=20
> >    Currently I expose the event data value via a union containing the
> >    various possible Counter data types (value_u8 and value_u64). It is
> >    up to the user to select the right union member for the data they
> >    received. Would it make sense to return this data in a u8 array
> >    instead, with the expectation that the user will cast to the
> >    necessary data type?
>=20
> Be careful on alignment if you do that. We would need to ensure that the
> buffer is suitable aligned for a cast to work as expected.

That's a fair point. It's probably safer to continue with a union which
also has the benefit of making the possible returned types clearer to
see in the code.

> >=20
> > 3. How should errors be returned for Counter data reads performed by
> >    Counter events?
> >=20
> >    Counter events are configured with a list of Counter data read
> >    operations to perform for the user. Any one of those data reads can
> >    return an error code, but not necessarily all of them. Currently, the
> >    code exits early when an error code is returned. Should the code
> >    instead continue on, saving the error code to the struct
> >    counter_event for userspace to handle?
>=20
> I'd argue that errors are expected to be rare, so it isn't a problem
> to just fault out hard on the first one.

All right, that should help keep the error logic simple too then.

William Breathitt Gray

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

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

iQIzBAABCgAdFiEEk5I4PDJ2w1cDf/bghvpINdm7VJIFAl8wN6YACgkQhvpINdm7
VJJS2A//eaJ/jtLitKdyaRHiFbB4//bIf3sPQQxG2eJSu3eztaFphLlkMxdDijrY
OiBq5wsBkHYvcpy/z2A+XqY/PFzCpMrr/tAyuxiEeKB95WjzTUZl59CbUDOqqtwz
8wx1DcymzHfvwun1AQDfXPyMXD/Np0zdQTaY2vmjyllC5otFSfW2VfFh/RIzxQ2A
FCtP9ogxam3Pl1GZ20e76C8f+tH8M25xP9Aj3h8R7XV2NiR6xE/WuhOlsmfAHyue
cvYBfyS8YnTvZaDu1L88kqgEoXqJXqTXKTfxRYLRgEQD0DN/i7IndPgApUoya93O
kBiGMexlfBeGOMaMgs29ZoFUflyg6ZjxcgrlCGt7jGHP8cmTex6TDw3f/BSGloPq
ED5rh5rPnYzWy0mqenPgZU2V6VCZeApcGwhakCpSYFVdM4Lp4yiktdFTpzC9LkXY
jeQvOXAespYNJH1llge4DfLwctqclMZVJg0flDDCUCQA75sy6vrPE73Nb+T3XChu
TEu+P8gRFJbxi9Rg/l/7p+P30s4e2a1YKJfWdYStcHSougEHJq+9x8zUaejWAMwo
YB8+NaDRnhUi6DVgTLwrib6uvwOyGj94P2qn8Y1RhkqUzQh0vSlG0z/gyss298H8
WCSZNFOLbS9pTLd77PpNxf7HkBCkg+/M14ZmjimDQMZwRMP0yGA=
=+ft6
-----END PGP SIGNATURE-----

--0ntfKIWw70PvrIHh--