From: William Breathitt Gray <vilhelm.gray@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: 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,
david@lechnology.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: Fri, 12 Feb 2021 15:32:12 +0900 [thread overview]
Message-ID: <YCYg7Eiu4u1t9VxE@shinobu> (raw)
In-Reply-To: <20201230150440.0723cab9@archlinux>
[-- Attachment #1: Type: text/plain, Size: 7511 bytes --]
On Wed, Dec 30, 2020 at 03:04:40PM +0000, Jonathan Cameron wrote:
> On Fri, 25 Dec 2020 19:15:36 -0500
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>
> > This patch introduces a character device interface for the Counter
> > subsystem. Device data is exposed through standard character device read
> > operations. Device data is gathered when a Counter event is pushed by
> > the respective Counter device driver. Configuration is handled via ioctl
> > operations on the respective Counter character device node.
> >
> > Cc: David Lechner <david@lechnology.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>
> There are a few things in here that could profitably be pulled out as precursor
> patches. I don't really understand the connection of extension_name to the
> addition of a chardev for example. Might be needed to provide enough
> info to actually use the chardev, but does it have meaning without that?
> Either way, definitely feels like it can be done in a separate patch.
The extension_name attributes are needed so chrdev users have enough
info to identify which extension number corresponds to which extension.
I'll move this to change to a separate patch and provide an appropriate
explanation there to make things clearer.
> > +static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + struct counter_device *const counter = filp->private_data;
> > + unsigned long flags;
> > + int err = 0;
> > +
> > + switch (cmd) {
> > + case COUNTER_CLEAR_WATCHES_IOCTL:
> > + return counter_clear_watches(counter);
> > + case COUNTER_ADD_WATCH_IOCTL:
> > + return counter_add_watch(counter, arg);
> > + case COUNTER_LOAD_WATCHES_IOCTL:
> > + raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> > +
> > + counter_events_list_free(&counter->events_list);
> > + list_replace_init(&counter->next_events_list,
> > + &counter->events_list);
> > +
> > + if (counter->ops->events_configure)
> > + err = counter->ops->events_configure(counter);
> > +
> > + raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> > + break;
>
> return here.
Ack.
> > +static int counter_get_data(struct counter_device *const counter,
> > + const struct counter_comp_node *const comp_node,
> > + u64 *const value)
> > +{
> > + const struct counter_comp *const comp = &comp_node->comp;
> > + void *const parent = comp_node->parent;
> > + int err = 0;
> > + u8 value_u8 = 0;
> > + u32 value_u32 = 0;
> > +
> > + if (comp_node->component.type == COUNTER_COMPONENT_NONE)
> > + return 0;
> > +
> > + switch (comp->type) {
> > + case COUNTER_COMP_U8:
> > + case COUNTER_COMP_BOOL:
> > + switch (comp_node->component.scope) {
> > + case COUNTER_SCOPE_DEVICE:
> > + err = comp->device_u8_read(counter, &value_u8);
> > + break;
> > + case COUNTER_SCOPE_SIGNAL:
> > + err = comp->signal_u8_read(counter, parent, &value_u8);
> > + break;
> > + case COUNTER_SCOPE_COUNT:
> > + err = comp->count_u8_read(counter, parent, &value_u8);
> > + break;
> > + }
> > + *value = value_u8;
> > + break;
> > + case COUNTER_COMP_SIGNAL_LEVEL:
> > + case COUNTER_COMP_FUNCTION:
> > + case COUNTER_COMP_ENUM:
> > + case COUNTER_COMP_COUNT_DIRECTION:
> > + case COUNTER_COMP_COUNT_MODE:
> > + switch (comp_node->component.scope) {
> > + case COUNTER_SCOPE_DEVICE:
> > + err = comp->device_u32_read(counter, &value_u32);
> > + break;
> > + case COUNTER_SCOPE_SIGNAL:
> > + err = comp->signal_u32_read(counter, parent,
> > + &value_u32);
> > + break;
> > + case COUNTER_SCOPE_COUNT:
> > + err = comp->count_u32_read(counter, parent, &value_u32);
> > + break;
> > + }
> > + *value = value_u32;
>
> Seems like a return here would make more sense as no shared stuff to do at
> end of the switch. Same in other similar cases.
Ack.
> > + break;
> > + case COUNTER_COMP_U64:
> > + switch (comp_node->component.scope) {
> > + case COUNTER_SCOPE_DEVICE:
> > + return comp->device_u64_read(counter, value);
> > + case COUNTER_SCOPE_SIGNAL:
> > + return comp->signal_u64_read(counter, parent, value);
> > + case COUNTER_SCOPE_COUNT:
> > + return comp->count_u64_read(counter, parent, value);
> > + }
> > + break;
> > + case COUNTER_COMP_SYNAPSE_ACTION:
> > + err = comp->action_read(counter, parent, comp->priv,
> > + &value_u32);
> > + *value = value_u32;
> > + break;
> > + }
> > +
> > + return err;
> > +}
> > +
> > +/**
> > + * counter_push_event - queue event for userspace reading
> > + * @counter: pointer to Counter structure
> > + * @event: triggered event
> > + * @channel: event channel
> > + *
> > + * Note: If no one is watching for the respective event, it is silently
> > + * discarded.
> > + */
> > +void counter_push_event(struct counter_device *const counter, const u8 event,
> > + const u8 channel)
> > +{
> > + struct counter_event ev = {0};
> > + unsigned int copied = 0;
> > + unsigned long flags;
> > + struct counter_event_node *event_node;
> > + struct counter_comp_node *comp_node;
> > +
> > + ev.timestamp = ktime_get_ns();
> > + ev.watch.event = event;
> > + ev.watch.channel = channel;
> > +
> > + raw_spin_lock_irqsave(&counter->events_list_lock, flags);
>
> For a raw spin lock, we definitely want to see comments on why it
> is necessary.
Ack.
> > @@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
> > return err;
> >
> > /* Create Count name attribute */
> > - err = counter_name_attr_create(dev, group, count->name);
> > + err = counter_name_attr_create(dev, group, "name", count->name);
>
> This refactoring could also be pulled out to a precusor patch.
Ack. This will be part of the extension_name patch.
> > @@ -319,12 +315,21 @@ struct counter_device {
> >
> > int id;
> > struct device dev;
> > + struct cdev chrdev;
> > + struct list_head events_list;
> > + raw_spinlock_t events_list_lock;
> > + struct list_head next_events_list;
> > + DECLARE_KFIFO(events, struct counter_event, 64);
>
> Why 64? Probably want that to be somewhat dynamic, even if only at build time.
Ack. This will be dynamically configurable via sysfs attribute in v8.
> > + wait_queue_head_t events_wait;
> > + struct mutex events_lock;
> > };
> >
> > int counter_register(struct counter_device *const counter);
> > void counter_unregister(struct counter_device *const counter);
> > int devm_counter_register(struct device *dev,
> > struct counter_device *const counter);
> > +void counter_push_event(struct counter_device *const counter, const u8 event,
> > + const u8 channel);
> >
> > #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
> > { \
> > 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
> Small thing but I would have been tempted to do a precursor patch to the
> main change simply putting in place the userspace header.
>
> Classic Nop patch that makes it easier to focus on the real stuff in this
> patch by getting that noise out of the way!
>
> Jonathan
Ack.
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2021-02-12 6:34 UTC|newest]
Thread overview: 25+ 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 1/5] counter: Internalize sysfs interface code William Breathitt Gray
2020-12-30 14:37 ` Jonathan Cameron
2021-01-06 5:29 ` William Breathitt Gray
2020-12-30 23:24 ` David Lechner
2021-01-06 5:30 ` 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 [this message]
2020-12-30 21:36 ` David Lechner
2021-01-30 4:59 ` William Breathitt Gray
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
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=YCYg7Eiu4u1t9VxE@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).