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:   Mon, 22 Feb 2021 10:43:00 +0900
From:   William Breathitt Gray <vilhelm.gray@gmail.com>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Ahmad Fatoum <a.fatoum@pengutronix.de>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        David Jander <david@protonic.nl>,
        Robin van der Gracht <robin@protonic.nl>,
        linux-iio@vger.kernel.org,
        Linus Walleij <linus.walleij@linaro.org>,
        Jonathan Cameron <jic23@kernel.org>
Subject: Re: [PATCH v5 2/2] counter: add IRQ or GPIO based event counter
Message-ID: <YDMMJJ985Zq9oEOv@shinobu>
References: <20210208135347.18494-1-o.rempel@pengutronix.de>
 <20210208135347.18494-3-o.rempel@pengutronix.de>
 <YCjlPhEtyH+vfSi4@shinobu>
 <20210215091737.fx6dwiz7tt56wbkr@pengutronix.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="wC8Tf9mEZxh/iA1u"
Content-Disposition: inline
In-Reply-To: <20210215091737.fx6dwiz7tt56wbkr@pengutronix.de>
Precedence: bulk


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

On Mon, Feb 15, 2021 at 10:17:37AM +0100, Oleksij Rempel wrote:
> > > +static irqreturn_t event_cnt_isr(int irq, void *dev_id)
> > > +{
> > > +	struct event_cnt_priv *priv =3D dev_id;
> > > +
> > > +	atomic_inc(&priv->count);
> >=20
> > This is just used to count the number of interrupts right? I wonder if
> > we can do this smarter. For example, the kernel already keeps track of
> > number of interrupts that has occurred for any particular IRQ line on a
> > CPU (see the 'kstat_irqs' member of struct irq_desc, and the
> > show_interrupts() function in kernel/irq/proc.c). Would it make sense to
> > simply store the initial interrupt count on driver load or enablement,
> > and then return the difference during a count_read() callback?
>=20
> This driver do not makes a lot of sense without your chardev patches. As
> soon as this patches go mainline, this driver will be able to send
> event with a timestamp and counter state to the user space.
>=20
> With other words, we will need an irq handler anyway. In this case we
> can't save more RAM or CPU cycles by using system irq counters.

It's true that this driver will need an IRQ handler when the timestamp
functionality is added, but deriving the count value is different matter
regardless. There's already code in the kernel to retrieve the number of
interrupts, so it makes sense that we use that rather than rolling our
own -- at the very least to ensure the value we provide to users is
consistent with the ones already provided by other areas of the kernel.

To that end, I'd like to see your cnt_isr() function removed for this
patchset (you can bring it back once timestamp support is added).
Reimplement your cnt_read/cnt_write() functions to instead use
kstat_irqs_usr() from <linux/kernel_stat.h> to get the current number of
interrupts the IRQ line and use it to derive your count value for this
driver.

> > > +static struct counter_signal event_cnt_signals[] =3D {
> > > +	{
> > > +		.id =3D 0,
> > > +		.name =3D "Channel 0 signal",
> >=20
> > You should choose a more description name for this Signal;
> > "Channel 0 signal" isn't very useful information for the user. Is this
> > signal the respective GPIO line state?
>=20
> Sounds plausible. How about "Channel 0, GPIO line state"?

Ideally, this would match the GPIO name (or I suppose the IRQ number if
not a GPIO line). So in your probe() function you can do something like
this I believe:

	cnt_signals[0].name =3D priv->gpio->name;

Of course, you should first check whether this is a GPIO line or IRQ
line and set the name accordingly.

William Breathitt Gray

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

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

iQIzBAABCgAdFiEEk5I4PDJ2w1cDf/bghvpINdm7VJIFAmAzDBkACgkQhvpINdm7
VJIqnw//X6S/JF8NIH9UZTbsm1n4Yu5N6OuQoLch0ADAHI6Ei6/ntVwFPUPG0Kaz
8DoefFjwbu7ixJA/aCLo8c0dCzrztQxcz4gTB+50uoxGNk6YqvYGHtsfcalN2vae
u9ku4i3j2oDk9KZwlFs49QhxZjBIZ6BBmz/R2HbYcurhU4pa/7Y22otyPUwKhXVB
pvDTKNrQmN2PRYS1CQcwup+D0/skW/JKiG76MPCtJ1HWt+bj+Aih+7wIwqsQ1m6p
W0jyQc4Mt+tbPZ30IratIJJiO1Vsw/D9TTHCkErP7x7BEE2dIY4SJ5fJe1L8up1f
vHo2G8gFO8JdoZT4eFvEbmWKC6Lv76ClKjSDqDKgr5SYtyZOzbtqPaxjDD3GB3eY
BPU8eekZoPjdDuktFnaLP6wXVewGP87Thr7NXCwRkA7boSe+6s3IYTFAzqhtgdKw
QdE+C3154koHmyyd5kwkVm3Hzur4xCWj2wQ7fze6tjwnSiAdCMFrz6myMtj7Exg7
GBDbLVQWvv0F6uA3G8vzzT2o//hb8AD1x859o1Bxmcbsrjz3yb4/s5QwKRxq7MFK
IZYz9VAMgNUUYlD/hs21UBl2PmjEH6NGySuGvXOMTGU5EAdTdL8SopJ38l9XuzHS
ZjlHqraYQvaP9p64s7vB8taS6LnZudVgivmf/0spF5PlL5K7R1o=
=zZCZ
-----END PGP SIGNATURE-----

--wC8Tf9mEZxh/iA1u--