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:   Fri, 25 Dec 2020 12:30:04 -0500
From:   William Breathitt Gray <vilhelm.gray@gmail.com>
To:     David Lechner <david@lechnology.com>
Cc:     jic23@kernel.org, kernel@pengutronix.de,
        linux-stm32@st-md-mailman.stormreply.com, a.fatoum@pengutronix.de,
        kamel.bouhara@bootlin.com, gwendal@chromium.org,
        alexandre.belloni@bootlin.com, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, 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, Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [PATCH v6 3/5] counter: Add character device interface
Message-ID: <X+YhnDqLZ7Ad9b2O@shinobu>
References: <cover.1606075915.git.vilhelm.gray@gmail.com>
 <b52a62196399d33221f78a1689276ac193c10229.1606075915.git.vilhelm.gray@gmail.com>
 <b5d49a3a-99ab-e5c0-3f0b-601eed9b54f5@lechnology.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="Bi9bW/svyG+8ZjzL"
Content-Disposition: inline
In-Reply-To: <b5d49a3a-99ab-e5c0-3f0b-601eed9b54f5@lechnology.com>
Precedence: bulk


--Bi9bW/svyG+8ZjzL
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi David,

I agree with your suggested changes -- just a couple select comments
following below.

On Sun, Dec 13, 2020 at 05:58:26PM -0600, David Lechner wrote:
> > +static int counter_add_watch(struct counter_device *const counter,
> > +			     const unsigned long arg)
> > +{

[...]

> > +
> > +dummy_component:
> > +	comp_node.component =3D watch.component;
>=20
>=20
> In my experiments, I added a events_validate driver callback here to
> validate each event as it is added. This way the user can know exactly
> which event caused the problem rather than waiting for the event_config
> callback.

Yes, this is a good idea and I have use for this in the 104-quad-8
driver as well. I'm going to name this "watch_validate" however, because
I need to validate the requested channel as well as the requested event
here (both part of the struct counter_watch).

> > diff --git a/include/linux/counter.h b/include/linux/counter.h
> > index 3f3f8ba6c1b4..98cd7c035968 100644
> > --- a/include/linux/counter.h
>=20
>=20
> >=20
> > +/**
> > + * struct counter_event_node - Counter Event node
> > + * @l:		list of current watching Counter events
> > + * @event:	event that triggers
> > + * @channel:	event channel
> > + * @comp_list:	list of components to watch when event triggers
> > + */
> > +struct counter_event_node {
> > +	struct list_head l;
> > +	u8 event;
> > +	u8 channel;
> > +	struct list_head comp_list;
> > +};
> > +
>=20
>=20
> Unless this is needed outside of the drivers/counter/ directory, I
> would suggest putting it in drivers/counter/counter-chrdev.h instead
> of include/linux/counter.h.

The "events_list" member of the struct counter_device is a list of
struct counter_event_node. The events_configure() callback should parse
through this list to determine the current events configuration request.
As such, driver authors will need this structure available via
include/linux/counter.h so they can parse "events_list".

William Breathitt Gray

--Bi9bW/svyG+8ZjzL
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEk5I4PDJ2w1cDf/bghvpINdm7VJIFAl/mIZwACgkQhvpINdm7
VJIZxxAAhAKczPADq8u05OkOAmgVD7IeT9cUBIB/XnT0VVSVX46G473g78mZwlsx
qnfK1l4uDvwjAY+YFFKanUztxg/j60BzJOt9BK4NqWR4ic7J0FPj9i4YgN9J+9oo
JmFSjqT8Abo7rEdB426yT9Fof9cBU/oPdOpNSufDNidrxXnTIOKnjA9odG/vgOzl
mlXTQHs2glKeLrHIvJOnK7mwy3l11Ew2xhRvBEsMfbzXIYoOkVC8Ax146tHpqBVl
hN/vCEU49+JCbZrSOk9EmtjRZ7DiF8AoV7GFUjqGWxkECKx1ov2WeR62DFhUJMvI
qC/boNu0BZW2N8HgIvgsgKP0r/v0E+RuRFzcahpS/0YhsNh8fgEVfeIWS2ZaVXWT
7e4FZIOw4Vuk0E2DXw0rWRzvNCCstRsqY9fQ7ee1/WruCHRvZEeiifJADx7E7LBv
XwQynYXOgFe1hxXFejwSTgTCMl5y25CaT8JVU8GLYgHYnPKkIVSsOJ5dUgTbRCOu
RBbMgZbcBuB1tUy45SP6Rx8in4joA+AGAt6oj4kxrd+/+O2+uvUoNILqoYHAMyV6
EBP78O/sAHNObYnwVJNaM1Bc1VYIF7+pMjotOTtHmkZhs5Ac8qxXih4r+mIZgItu
yGeJ9cG82fkziDp+TtNuYXsfLQQhxtdicKmAqzZeg5VSyGQWkQY=
=vH3L
-----END PGP SIGNATURE-----

--Bi9bW/svyG+8ZjzL--