From: Greg KH <gregkh@linuxfoundation.org>
To: William Breathitt Gray <vilhelm.gray@gmail.com>
Cc: jic23@kernel.org, linux-arm-kernel@lists.infradead.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-iio@vger.kernel.org, fabrice.gasnier@st.com,
benjamin.gaignard@st.com, robh+dt@kernel.org, knaack.h@gmx.de,
lars@metafoo.de, pmeerw@pmeerw.net, mark.rutland@arm.com
Subject: Re: [PATCH v7 01/10] counter: Introduce the Generic Counter interface
Date: Sat, 7 Jul 2018 17:16:22 +0200 [thread overview]
Message-ID: <20180707151622.GB410@kroah.com> (raw)
In-Reply-To: <51b75b2b4495d4ad7ed173d91a726379bdae2353.1529607879.git.vilhelm.gray@gmail.com>
On Thu, Jun 21, 2018 at 05:07:08PM -0400, William Breathitt Gray wrote:
> This patch introduces the Generic Counter interface for supporting
> counter devices.
>
> In the context of the Generic Counter interface, a counter is defined as
> a device that reports one or more "counts" based on the state changes of
> one or more "signals" as evaluated by a defined "count function."
>
> Driver callbacks should be provided to communicate with the device: to
> read and write various Signals and Counts, and to set and get the
> "action mode" and "count function" for various Synapses and Counts
> respectively.
>
> To support a counter device, a driver must first allocate the available
> Counter Signals via counter_signal structures. These Signals should
> be stored as an array and set to the signals array member of an
> allocated counter_device structure before the Counter is registered to
> the system.
>
> Counter Counts may be allocated via counter_count structures, and
> respective Counter Signal associations (Synapses) made via
> counter_synapse structures. Associated counter_synapse structures are
> stored as an array and set to the the synapses array member of the
> respective counter_count structure. These counter_count structures are
> set to the counts array member of an allocated counter_device structure
> before the Counter is registered to the system.
>
> A counter device is registered to the system by passing the respective
> initialized counter_device structure to the counter_register function;
> similarly, the counter_unregister function unregisters the respective
> Counter. The devm_counter_register and devm_counter_unregister functions
> serve as device memory-managed versions of the counter_register and
> counter_unregister functions respectively.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
> MAINTAINERS | 7 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/counter/Kconfig | 18 +
> drivers/counter/Makefile | 8 +
> drivers/counter/generic-counter.c | 1519 +++++++++++++++++++++++++++++
> include/linux/counter.h | 534 ++++++++++
> 7 files changed, 2089 insertions(+)
> create mode 100644 drivers/counter/Kconfig
> create mode 100644 drivers/counter/Makefile
> create mode 100644 drivers/counter/generic-counter.c
> create mode 100644 include/linux/counter.h
First cut of review, does counter.h really have to be that big? It
feels like some of the "internal" functions and structures could be
local to drivers/counter/ right?
> +menuconfig COUNTER
> + tristate "Counter support"
> + help
> + Provides Generic Counter interface support for counter devices.
> +
> + Counter devices are prevalent within a diverse spectrum of industries.
> + The ubiquitous presence of these devices necessitates a common
> + interface and standard of interaction and exposure. This driver API
> + attempts to resolve the issue of duplicate code found among existing
> + counter device drivers by providing a generic counter interface for
> + consumption. The Generic Counter interface enables drivers to support
> + and expose a common set of components and functionality present in
> + counter devices.
No need to explain an "API" in a Kconfig help text, which is for a user
of the kernel, not for someone writing kernel code. Odds are individual
drivers will need to select this, or are you going to depend on this
option?
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> new file mode 100644
> index 000000000000..ad1ba7109cdc
> --- /dev/null
> +++ b/drivers/counter/Makefile
> @@ -0,0 +1,8 @@
> +#
> +# Makefile for Counter devices
> +#
> +
> +# When adding new entries keep the list in alphabetical order
Why does it matter? :)
> +
> +obj-$(CONFIG_COUNTER) += counter.o
> +counter-y := generic-counter.o
Why not just call your .c file, "counter.c" to keep this simple?
> diff --git a/drivers/counter/generic-counter.c b/drivers/counter/generic-counter.c
> new file mode 100644
> index 000000000000..483826c8ce01
> --- /dev/null
> +++ b/drivers/counter/generic-counter.c
> @@ -0,0 +1,1519 @@
> +// SPDX-License-Identifier: GPL-2.0-only
Please use the normal SPDX identifiers we are using, as described in the
kernel documentation. We aren't ready to use the "new" ones just yet.
> +/*
> + * Generic Counter interface
> + * Copyright (C) 2017 William Breathitt Gray
It's 2018 now :)
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/fs.h>
> +#include <linux/gfp.h>
> +#include <linux/idr.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#include <linux/counter.h>
Why a blank line?
> +
> +const char *const count_direction_str[2] = {
> + [COUNT_DIRECTION_FORWARD] = "forward",
> + [COUNT_DIRECTION_BACKWARD] = "backward"
> +};
> +EXPORT_SYMBOL(count_direction_str);
I have to ask, for all of these, why not EXPORT_SYMBOL_GPL()?
> +
> +const char *const count_mode_str[4] = {
> + [COUNT_MODE_NORMAL] = "normal",
> + [COUNT_MODE_RANGE_LIMIT] = "range limit",
> + [COUNT_MODE_NON_RECYCLE] = "non-recycle",
> + [COUNT_MODE_MODULO_N] = "modulo-n"
> +};
> +EXPORT_SYMBOL(count_mode_str);
And why do you need to export strings?
> +
> +ssize_t counter_signal_enum_read(struct counter_device *counter,
> + struct counter_signal *signal, void *priv,
> + char *buf)
> +{
> + const struct counter_signal_enum_ext *const e = priv;
> + int err;
> + size_t index;
> +
> + if (!e->get)
> + return -EINVAL;
How can e->get not be set? Shouldn't you just not have called into this
if so?
> +
> + err = e->get(counter, signal, &index);
> + if (err)
> + return err;
> +
> + if (index >= e->num_items)
> + return -EINVAL;
> +
> + return scnprintf(buf, PAGE_SIZE, "%s\n", e->items[index]);
No need to do the scnprintf() crud, it's a sysfs file, a simple
sprintf() works just fine. Yeah, it makes people nervous, and it should :)
> +}
> +EXPORT_SYMBOL(counter_signal_enum_read);
sysfs attribute files really should be EXPORT_SYMBOL_GPL() please.
> +
> +ssize_t counter_signal_enum_write(struct counter_device *counter,
> + struct counter_signal *signal, void *priv,
> + const char *buf, size_t len)
> +{
> + const struct counter_signal_enum_ext *const e = priv;
> + ssize_t index;
> + int err;
> +
> + if (!e->set)
> + return -EINVAL;
Again, how can this be true?
> +
> + index = __sysfs_match_string(e->items, e->num_items, buf);
> + if (index < 0)
> + return index;
> +
> + err = e->set(counter, signal, index);
> + if (err)
> + return err;
> +
> + return len;
> +}
> +EXPORT_SYMBOL(counter_signal_enum_write);
> +
> +ssize_t counter_signal_enum_available_read(struct counter_device *counter,
> + struct counter_signal *signal,
> + void *priv, char *buf)
> +{
> + const struct counter_signal_enum_ext *const e = priv;
> + size_t i;
> + size_t len = 0;
> +
> + if (!e->num_items)
> + return 0;
> +
> + for (i = 0; i < e->num_items; i++)
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
> + e->items[i]);
a list of items? In sysfs? Are you _SURE_ you want to do that?
> +
> + return len;
> +}
> +EXPORT_SYMBOL(counter_signal_enum_available_read);
> +
> +ssize_t counter_count_enum_read(struct counter_device *counter,
> + struct counter_count *count, void *priv,
> + char *buf)
> +{
> + const struct counter_count_enum_ext *const e = priv;
> + int err;
> + size_t index;
> +
> + if (!e->get)
> + return -EINVAL;
Same comment everywhere for this...
let's skip lower in the files...
> +static int counter_attribute_create(
> + struct counter_device_attr_group *const group,
> + const char *const prefix,
> + const char *const name,
> + ssize_t (*show)(struct device *dev, struct device_attribute *attr,
> + char *buf),
> + ssize_t (*store)(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t len),
Typedefs for these function pointers are good to have.
> + void *const component)
That's a crazy list of parameters...
> +{
> + struct counter_device_attr *counter_attr;
> + struct device_attribute *dev_attr;
> + int err;
> + struct list_head *const attr_list = &group->attr_list;
> +
> + /* Allocate a Counter device attribute */
> + counter_attr = kzalloc(sizeof(*counter_attr), GFP_KERNEL);
> + if (!counter_attr)
> + return -ENOMEM;
> + dev_attr = &counter_attr->dev_attr;
> +
> + sysfs_attr_init(&dev_attr->attr);
> +
> + /* Configure device attribute */
> + dev_attr->attr.name = kasprintf(GFP_KERNEL, "%s%s", prefix, name);
> + if (!dev_attr->attr.name) {
> + err = -ENOMEM;
> + goto err_free_counter_attr;
> + }
> + if (show) {
> + dev_attr->attr.mode |= 0444;
> + dev_attr->show = show;
> + }
> + if (store) {
> + dev_attr->attr.mode |= 0200;
> + dev_attr->store = store;
> + }
> +
> + /* Store associated Counter component with attribute */
> + counter_attr->component = component;
> +
> + /* Keep track of the attribute for later cleanup */
> + list_add(&counter_attr->l, attr_list);
> + group->num_attr++;
So you are dynamically creating sysfs attributes? Why not just have one
big group and only enable them if needed? That would make things a lot
simpler, right?
> +struct signal_comp_t {
"_t"???
> + struct counter_signal *signal;
> +};
> +
> +static ssize_t counter_signal_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct counter_device *const counter = dev_get_drvdata(dev);
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct signal_comp_t *const component = devattr->component;
> + struct counter_signal *const signal = component->signal;
> + int err;
> + struct signal_read_value val = { .buf = buf };
> +
> + err = counter->ops->signal_read(counter, signal, &val);
> + if (err)
> + return err;
> +
> + return val.len;
> +}
> +
> +struct name_comp_t {
"_t"???
Same for the rest of this file...
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> new file mode 100644
> index 000000000000..88fc82ee30a7
> --- /dev/null
> +++ b/include/linux/counter.h
> @@ -0,0 +1,534 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
Same identifier issue.
> +/*
> + * Counter interface
> + * Copyright (C) 2017 William Breathitt Gray
2018!
And again, it looks like this file can be a lot smaller, but I don't see
a user of it just yet, so I don't really know...
thanks,
greg k-h
next prev parent reply other threads:[~2018-07-07 15:17 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-21 21:06 [PATCH v7 00/10] Introduce the Counter subsystem William Breathitt Gray
2018-06-21 21:07 ` [PATCH v7 01/10] counter: Introduce the Generic Counter interface William Breathitt Gray
2018-07-07 15:16 ` Greg KH [this message]
2018-07-09 17:40 ` William Breathitt Gray
2018-07-09 18:54 ` Greg KH
2018-07-09 18:56 ` William Breathitt Gray
2018-07-18 3:49 ` Andrew Morton
2018-07-21 16:26 ` William Breathitt Gray
2018-07-22 5:41 ` Andrew Morton
2018-06-21 21:07 ` [PATCH v7 02/10] counter: Documentation: Add Generic Counter sysfs documentation William Breathitt Gray
2018-07-02 19:11 ` [v7, " David Lechner
2018-07-03 14:04 ` William Breathitt Gray
2018-06-21 21:07 ` [PATCH v7 03/10] docs: Add Generic Counter interface documentation William Breathitt Gray
2018-06-22 16:51 ` Jonathan Cameron
2018-07-02 19:37 ` [v7,03/10] " David Lechner
2018-07-03 14:16 ` William Breathitt Gray
2018-07-04 17:23 ` Linus Walleij
2018-07-06 17:15 ` Jonathan Cameron
2018-07-06 18:25 ` David Lechner
2018-07-02 19:42 ` David Lechner
2018-07-03 14:21 ` William Breathitt Gray
2018-06-21 21:07 ` [PATCH v7 04/10] counter: 104-quad-8: Add Generic Counter interface support William Breathitt Gray
2018-06-22 16:57 ` Jonathan Cameron
2018-06-21 21:08 ` [PATCH v7 05/10] counter: 104-quad-8: Documentation: Add Generic Counter sysfs documentation William Breathitt Gray
2018-06-22 16:59 ` Jonathan Cameron
2018-06-21 21:08 ` [PATCH v7 06/10] counter: Add STM32 Timer quadrature encoder William Breathitt Gray
2018-06-22 17:03 ` Jonathan Cameron
2018-06-21 21:08 ` [PATCH v7 07/10] dt-bindings: counter: Document stm32 " William Breathitt Gray
2018-07-02 19:56 ` [v7,07/10] " David Lechner
2018-07-05 21:13 ` [PATCH v7 07/10] " Rob Herring
2018-06-21 21:08 ` [PATCH v7 08/10] counter: stm32-lptimer: add counter device William Breathitt Gray
2018-06-22 17:06 ` Jonathan Cameron
2018-06-21 21:08 ` [PATCH v7 09/10] dt-bindings: counter: Adjust dt-bindings for STM32 lptimer move William Breathitt Gray
2018-07-05 21:13 ` Rob Herring
2018-06-21 21:09 ` [PATCH v7 10/10] iio: counter: Add deprecation markings for IIO Counter attributes William Breathitt Gray
2018-06-22 17:10 ` [PATCH v7 00/10] Introduce the Counter subsystem Jonathan Cameron
2018-07-02 18:13 ` David Lechner
2018-07-03 2:48 ` William Breathitt Gray
2018-07-06 17:21 ` Jonathan Cameron
2018-07-06 18:22 ` David Lechner
2018-07-06 19:20 ` William Breathitt Gray
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=20180707151622.GB410@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=benjamin.gaignard@st.com \
--cc=devicetree@vger.kernel.org \
--cc=fabrice.gasnier@st.com \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pmeerw@pmeerw.net \
--cc=robh+dt@kernel.org \
--cc=vilhelm.gray@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).