On Sun, Feb 14, 2021 at 06:06:12PM +0000, Jonathan Cameron wrote:

On Fri, 12 Feb 2021 21:13:41 +0900
William Breathitt Gray [off-list ref] wrote:

quoted

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 <redacted>
Cc: Dan Carpenter <redacted>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>

Hi William,

A few minor comments.  Mostly seems to have come together well and
makes sense to me.

Jonathan

quoted

---
 drivers/counter/Makefile         |   2 +-
 drivers/counter/counter-chrdev.c | 496 +++++++++++++++++++++++++++++++
 drivers/counter/counter-chrdev.h |  16 +
 drivers/counter/counter-core.c   |  37 ++-
 include/linux/counter.h          |  45 +++
 include/uapi/linux/counter.h     |  70 +++++
 6 files changed, 661 insertions(+), 5 deletions(-)
 create mode 100644 drivers/counter/counter-chrdev.c
 create mode 100644 drivers/counter/counter-chrdev.h

...

quoted

diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
index bcf672e1fc0d..c137fcb97d9c 100644
--- a/drivers/counter/counter-core.c
+++ b/drivers/counter/counter-core.c

@@ -5,12 +5,16 @@
  */
 #include <linux/counter.h>
 #include <linux/device.h>
+#include <linux/device/bus.h>
 #include <linux/export.h>
+#include <linux/fs.h>
 #include <linux/gfp.h>
 #include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/types.h>
 
+#include "counter-chrdev.h"
 #include "counter-sysfs.h"
 
 /* Provides a unique ID for each counter device */

@@ -33,6 +37,8 @@ static struct bus_type counter_bus_type = {
 	.name = "counter"
 };
 
+static dev_t counter_devt;
+
 /**
  * counter_register - register Counter to the system
  * @counter:	pointer to Counter to register

@@ -54,7 +60,6 @@ int counter_register(struct counter_device *const counter)
 	if (counter->id < 0)
 		return counter->id;
 
-	/* Configure device structure for Counter */

Not sure why this comment gets removed here.

This comment wasn't suppose to be removed. I'll revert this.

quoted

 	dev->type = &counter_device_type;
 	dev->bus = &counter_bus_type;
 	if (counter->parent) {

@@ -65,18 +70,25 @@ int counter_register(struct counter_device *const counter)
 	device_initialize(dev);
 	dev_set_drvdata(dev, counter);
 
+	/* Add Counter character device */
+	err = counter_chrdev_add(counter, counter_devt);
+	if (err < 0)
+		goto err_free_id;
+
 	/* Add Counter sysfs attributes */
 	err = counter_sysfs_add(counter);
 	if (err < 0)
-		goto err_free_id;
+		goto err_remove_chrdev;
 
 	/* Add device to system */
 	err = device_add(dev);
 	if (err < 0)
-		goto err_free_id;
+		goto err_remove_chrdev;

It might be worth thinking about using cdev_device_add()
though will require a slightly different order of adding.

I think using cdev_device_add() should be possible. I'll adjust
counter_chrdev_add() accordingly to account for this.
 

quoted

 
 	return 0;
 
+err_remove_chrdev:
+	counter_chrdev_remove(counter);
 err_free_id:
 	put_device(dev);
 	return err;

@@ -138,13 +150,30 @@ int devm_counter_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_counter_register);
 
+#define COUNTER_DEV_MAX 256
+
 static int __init counter_init(void)
 {
-	return bus_register(&counter_bus_type);
+	int err;
+
+	err = bus_register(&counter_bus_type);
+	if (err < 0)
+		return err;
+
+	err = alloc_chrdev_region(&counter_devt, 0, COUNTER_DEV_MAX, "counter");
+	if (err < 0)
+		goto err_unregister_bus;
+
+	return 0;
+
+err_unregister_bus:
+	bus_unregister(&counter_bus_type);
+	return err;
 }
 
 static void __exit counter_exit(void)
 {
+	unregister_chrdev_region(counter_devt, COUNTER_DEV_MAX);
 	bus_unregister(&counter_bus_type);
 }
 

...

quoted

diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
index 6113938a6044..3d647a5383b8 100644
--- a/include/uapi/linux/counter.h
+++ b/include/uapi/linux/counter.h

@@ -6,6 +6,19 @@
 #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 {
 	COUNTER_SCOPE_DEVICE,

@@ -13,6 +26,63 @@ enum counter_scope {
 	COUNTER_SCOPE_COUNT,
 };
 
+/**
+ * struct counter_component - Counter component identification
+ * @type: component type (one of enum counter_component_type)
+ * @scope: component scope (one of enum counter_scope)
+ * @parent: parent component ID (matching the Y/Z suffix of the respective sysfs
+ *	    path as described in Documentation/ABI/testing/sysfs-bus-counter)

Probably good to give an example here as well as the cross reference.

Ack.

William Breathitt Gray