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 v7 3/5] counter: Add character device interface
Date: Sat, 30 Jan 2021 13:59:31 +0900 [thread overview]
Message-ID: <YBTns6IzaNUrMSsq@shinobu> (raw)
In-Reply-To: <e9102ed7-d4e1-0c81-96f3-8d3c297d037f@lechnology.com>
[-- Attachment #1: Type: text/plain, Size: 4024 bytes --]
On Wed, Dec 30, 2020 at 03:36:35PM -0600, David Lechner wrote:
> On 12/25/20 6:15 PM, William Breathitt Gray wrote:
>
> > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> > new file mode 100644
> > index 000000000000..7585dc9db19d
> > --- /dev/null
> > +++ b/include/uapi/linux/counter.h
> > @@ -0,0 +1,123 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Userspace ABI for Counter character devices
> > + * Copyright (C) 2020 William Breathitt Gray
> > + */
> > +#ifndef _UAPI_COUNTER_H_
> > +#define _UAPI_COUNTER_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +/* Component type definitions */
> > +enum counter_component_type {
> > + COUNTER_COMPONENT_NONE,
> > + COUNTER_COMPONENT_SIGNAL,
> > + COUNTER_COMPONENT_COUNT,
> > + COUNTER_COMPONENT_FUNCTION,
> > + COUNTER_COMPONENT_SYNAPSE_ACTION,
> > + COUNTER_COMPONENT_EXTENSION,
> > +};
> > +
> > +/* Component scope definitions */
> > +enum counter_scope {
>
> Do we need COUNTER_SCOPE_NONE to go with COUNTER_COMPONENT_NONE?
COUNTER_COMPONENT_NONE alone should be fine because it already indicates
that the 'component' member of the struct counter_watch is to be ignored
(i.e. type, scope, etc. will not be evaluated and that section of code
is bypassed).
> > + COUNTER_SCOPE_DEVICE,
> > + COUNTER_SCOPE_SIGNAL,
> > + COUNTER_SCOPE_COUNT,
> > +};
> > +
> > +/**
> > + * struct counter_component - Counter component identification
> > + * @type: component type (Count, extension, etc.)
>
> Instead of "Count, extension, etc.", it could be more helpful
> to say one of enum counter_component_type.
Ack.
> > + * @scope: component scope (Device, Count, or Signal)
>
> Same here. @scope must be one of enum counter_scope.
Ack.
> > + * @parent: parent component identification number
> > + * @id: component identification number
>
> It could be helpful to say that these id numbers match
> the X/Y/Z in the sysfs paths as described in the sysfs
> ABI.
Ack.
> > + */
> > +struct counter_component {
> > + __u8 type;
> > + __u8 scope;
> > + __u8 parent;
> > + __u8 id;
> > +};
> > +
> > +/* Event type definitions */
> > +enum counter_event_type {
> > + COUNTER_EVENT_OVERFLOW,
> > + COUNTER_EVENT_UNDERFLOW,
> > + COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> > + COUNTER_EVENT_THRESHOLD,
> > + COUNTER_EVENT_INDEX,
> > +};
> > +
> > +/**
> > + * struct counter_watch - Counter component watch configuration
> > + * @component: component to watch when event triggers
> > + * @event: event that triggers
>
> It would be helpful to say that @event must be one of
> enum counter_event_type.
Ack.
> > + * @channel: event channel
>
> It would be useful to say that @channel should be 0 unless
> a component has more than one event of the same type.
I'll make this clearer.
> > + */
> > +struct counter_watch {
> > + struct counter_component component;
> > + __u8 event;
> > + __u8 channel;
> > +};
> > +
> > +/* ioctl commands */
> > +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
> > +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
> > +#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
> > +
> > +/**
> > + * struct counter_event - Counter event data
> > + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> > + * @value: component value
> > + * @watch: component watch configuration
> > + * @errno: system error number
> > + */
> > +struct counter_event {
> > + __aligned_u64 timestamp;
> > + __aligned_u64 value;
> > + struct counter_watch watch;
> > + __u8 errno;
>
> There are error codes larger than 255. Probably better
> make this __u32.
Are error codes larger than 255 actually useful in this case? I noticed
the exit() function will only return the least significant byte of
status to the parent: https://man7.org/linux/man-pages/man3/exit.3.html
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2021-01-30 9:01 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-26 0:15 [PATCH v7 0/5] Introduce the Counter character device interface William Breathitt Gray
2020-12-26 0:15 ` [PATCH v7 2/5] docs: counter: Update to reflect sysfs internalization William Breathitt Gray
2020-12-30 14:41 ` Jonathan Cameron
2020-12-26 0:15 ` [PATCH v7 3/5] counter: Add character device interface William Breathitt Gray
2020-12-30 15:04 ` Jonathan Cameron
2021-02-12 6:32 ` William Breathitt Gray
2020-12-30 21:36 ` David Lechner
2021-01-30 4:59 ` William Breathitt Gray [this message]
2021-01-28 9:01 ` Oleksij Rempel
2021-01-30 5:15 ` William Breathitt Gray
2020-12-26 0:15 ` [PATCH v7 4/5] docs: counter: Document " William Breathitt Gray
2020-12-30 14:47 ` Jonathan Cameron
2020-12-30 18:18 ` David Lechner
2020-12-26 0:15 ` [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 William Breathitt Gray
2020-12-30 15:31 ` Jonathan Cameron
2021-02-12 6:04 ` William Breathitt Gray
2020-12-30 17:36 ` David Lechner
2021-02-11 23:56 ` William Breathitt Gray
2021-02-12 1:10 ` David Lechner
[not found] ` <fc40ab7f4a38e80d86715daa5eaf744dd645a75b.1608935587.git.vilhelm.gray@gmail.com>
2020-12-30 23:24 ` [PATCH v7 1/5] counter: Internalize sysfs interface code David Lechner
2021-01-06 5:30 ` William Breathitt Gray
[not found] ` <20201230143719.28a90914@archlinux>
2021-01-06 5:29 ` William Breathitt Gray
2020-12-30 23:34 ` [PATCH v7 0/5] Introduce the Counter character device interface David Lechner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YBTns6IzaNUrMSsq@shinobu \
--to=vilhelm.gray@gmail.com \
--cc=a.fatoum@pengutronix.de \
--cc=alexandre.belloni@bootlin.com \
--cc=alexandre.torgue@st.com \
--cc=dan.carpenter@oracle.com \
--cc=david@lechnology.com \
--cc=fabrice.gasnier@st.com \
--cc=gwendal@chromium.org \
--cc=jic23@kernel.org \
--cc=kamel.bouhara@bootlin.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=mcoquelin.stm32@gmail.com \
--cc=patrick.havelange@essensium.com \
--cc=syednwaris@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).