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@xxxxxxxxxx>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
