[RFC PATCH 0/2] Add support for Counter array components

[RFC PATCH 0/2] Add support for Counter array components

[William Breathitt Gray]

The COUNTER_COMP_ARRAY_U64 Counter component type is introduced to
enable support for Counter array components. With Counter array
components, exposure for buffers on counter devices can be defined via
new Counter array component macros. This should simplify code for driver
authors who would otherwise need to define individual Counter components
for each array element.

For example, suppose a driver wants to expose a Count's read-only
capture buffer of four elements using a callback `foobar_read()`::

        COUNTER_COMP_COUNT_ARRAY_U64("capture", foobar_read, NULL, 4)

Respective sysfs attributes for each array element would appear for the
respective Count:

* /sys/bus/counter/devices/counterX/countY/capture0
* /sys/bus/counter/devices/counterX/countY/capture1
* /sys/bus/counter/devices/counterX/countY/capture2
* /sys/bus/counter/devices/counterX/countY/capture3

If a user tries to read _capture2_ for example, `idx` will be `2` when
passed to the `foobar_read()` callback, and thus the driver knows which
array element to handle.

Currently, only u64 arrays are supported. One thing to consider is
whether it would make sense to support arrays of other types. This would
allow support for arrays that don't necessarily correlate to a buffer on
the device, but rather serve as logical groupings of components.

For example, if a Signal has four polarity modes, a driver author can
define a Counter array component to handle the exposure of all four
together rather than defining individual components for each one. This
is the reason I've chosen the more generic "array" terminology for this
feature rather than "buffer".

William Breathitt Gray (2):
  counter: Consolidate Counter extension sysfs attribute creation
  counter: Introduce the COUNTER_COMP_ARRAY_U64 component type

 drivers/counter/counter-sysfs.c | 210 ++++++++++++++++++++++++--------
 include/linux/counter.h         |  78 ++++++++++++
 2 files changed, 239 insertions(+), 49 deletions(-)


base-commit: a12224997bec72d231a8dd642876e6364decdc45
-- 
2.37.2

[RFC PATCH 1/2] counter: Consolidate Counter extension sysfs attribute creation

[William Breathitt Gray]

Counter extensions are handled for the Device, Counts, and Signals. The
code loops through each Counter extension and creates the expected sysfs
attributes. This patch consolidates that code into functions to reduce
redundancy and make the intention of the code clearer.

Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/counter/counter-sysfs.c | 98 ++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 49 deletions(-)

diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index 04eac41dad33..026ea094d68e 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
@@ -580,6 +580,46 @@ static int counter_comp_id_attr_create(struct device *const dev,
 	return 0;
 }
 
+static int counter_ext_attrs_create(struct device *const dev,
+				    struct counter_attribute_group *const group,
+				    const struct counter_comp *const ext,
+				    const enum counter_scope scope,
+				    void *const parent, const size_t id)
+{
+	int err;
+
+	/* Create main extension attribute */
+	err = counter_attr_create(dev, group, ext, scope, parent);
+	if (err < 0)
+		return err;
+
+	/* Create extension id attribute */
+	return counter_comp_id_attr_create(dev, group, ext->name, id);
+}
+
+static int counter_sysfs_exts_add(struct device *const dev,
+				  struct counter_attribute_group *const group,
+				  const struct counter_comp *const exts,
+				  const size_t num_ext,
+				  const enum counter_scope scope,
+				  void *const parent)
+{
+	size_t i;
+	const struct counter_comp *ext;
+	int err;
+
+	/* Create attributes for each extension */
+	for (i = 0; i < num_ext; i++) {
+		ext = &exts[i];
+		err = counter_ext_attrs_create(dev, group, ext, scope, parent,
+					       i);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
 static struct counter_comp counter_signal_comp = {
 	.type = COUNTER_COMP_SIGNAL_LEVEL,
 	.name = "signal",
@@ -593,8 +633,6 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
 	struct device *const dev = &counter->dev;
 	int err;
 	struct counter_comp comp;
-	size_t i;
-	struct counter_comp *ext;
 
 	/* Create main Signal attribute */
 	comp = counter_signal_comp;
@@ -608,21 +646,9 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
 	if (err < 0)
 		return err;
 
-	/* Create an attribute for each extension */
-	for (i = 0; i < signal->num_ext; i++) {
-		ext = &signal->ext[i];
-
-		err = counter_attr_create(dev, cattr_group, ext, scope, signal);
-		if (err < 0)
-			return err;
-
-		err = counter_comp_id_attr_create(dev, cattr_group, ext->name,
-						  i);
-		if (err < 0)
-			return err;
-	}
-
-	return 0;
+	/* Add Signal extensions */
+	return counter_sysfs_exts_add(dev, cattr_group, signal->ext,
+				      signal->num_ext, scope, signal);
 }
 
 static int counter_sysfs_signals_add(struct counter_device *const counter,
@@ -707,8 +733,6 @@ static int counter_count_attrs_create(struct counter_device *const counter,
 	struct device *const dev = &counter->dev;
 	int err;
 	struct counter_comp comp;
-	size_t i;
-	struct counter_comp *ext;
 
 	/* Create main Count attribute */
 	comp = counter_count_comp;
@@ -731,21 +755,9 @@ static int counter_count_attrs_create(struct counter_device *const counter,
 	if (err < 0)
 		return err;
 
-	/* Create an attribute for each extension */
-	for (i = 0; i < count->num_ext; i++) {
-		ext = &count->ext[i];
-
-		err = counter_attr_create(dev, cattr_group, ext, scope, count);
-		if (err < 0)
-			return err;
-
-		err = counter_comp_id_attr_create(dev, cattr_group, ext->name,
-						  i);
-		if (err < 0)
-			return err;
-	}
-
-	return 0;
+	/* Add Count extensions */
+	return counter_sysfs_exts_add(dev, cattr_group, count->ext,
+				      count->num_ext, scope, count);
 }
 
 static int counter_sysfs_counts_add(struct counter_device *const counter,
@@ -838,8 +850,6 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
 	const enum counter_scope scope = COUNTER_SCOPE_DEVICE;
 	struct device *const dev = &counter->dev;
 	int err;
-	size_t i;
-	struct counter_comp *ext;
 
 	/* Add Signals sysfs attributes */
 	err = counter_sysfs_signals_add(counter, cattr_group);
@@ -876,19 +886,9 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
 	if (err < 0)
 		return err;
 
-	/* Create an attribute for each extension */
-	for (i = 0; i < counter->num_ext; i++) {
-		ext = &counter->ext[i];
-
-		err = counter_attr_create(dev, cattr_group, ext, scope, NULL);
-		if (err < 0)
-			return err;
-
-		err = counter_comp_id_attr_create(dev, cattr_group, ext->name,
-						  i);
-		if (err < 0)
-			return err;
-	}
+	/* Add device extensions */
+	return counter_sysfs_exts_add(dev, cattr_group, counter->ext,
+				      counter->num_ext, scope, NULL);
 
 	return 0;
 }
-- 
2.37.2

[RFC PATCH 2/2] counter: Introduce the COUNTER_COMP_ARRAY_U64 component type

[William Breathitt Gray]

The COUNTER_COMP_ARRAY_U64 Counter component type is introduced to
enable support for Counter array components. With Counter array
components, exposure for buffers on counter devices can be defined via
new Counter array component macros. This should simplify code for driver
authors who would otherwise need to define individual Counter components
for each array element.

Three Counter array component macros are introduced::

        COUNTER_COMP_DEVICE_ARRAY_U64(_name, _read, _write, _length)
        COUNTER_COMP_COUNT_ARRAY_U64(_name, _read, _write, _length)
        COUNTER_COMP_SIGNAL_ARRAY_U64(_name, _read, _write, _length)

Six respective callbacks are introduced as well::

        int (*device_array_u64_read)(struct counter_device *counter,
                                     size_t idx, u64 *val);
        int (*count_array_u64_read)(struct counter_device *counter,
                                    struct counter_count *count,
                                    size_t idx, u64 *val);
        int (*signal_array_u64_read)(struct counter_device *counter,
                                     struct counter_signal *signal,
                                     size_t idx, u64 *val);
        int (*device_array_u64_write)(struct counter_device *counter,
                                      size_t idx, u64 val);
        int (*count_array_u64_write)(struct counter_device *counter,
                                     struct counter_count *count,
                                     size_t idx, u64 val);
        int (*signal_array_u64_write)(struct counter_device *counter,
                                      struct counter_signal *signal,
                                      size_t idx, u64 val);

Driver authors can handle reads/writes for an array component by
receiving an element index via the `idx` parameter and processing the
respective value via the `val` parameter.

For example, suppose a driver wants to expose a Count's read-only
capture buffer of four elements using a callback `foobar_read()`::

        COUNTER_COMP_COUNT_ARRAY_U64("capture", foobar_read, NULL, 4)

Respective sysfs attributes for each array element would appear for the
respective Count:

* /sys/bus/counter/devices/counterX/countY/capture0
* /sys/bus/counter/devices/counterX/countY/capture1
* /sys/bus/counter/devices/counterX/countY/capture2
* /sys/bus/counter/devices/counterX/countY/capture3

If a user tries to read _capture2_ for example, `idx` will be `2` when
passed to the `foobar_read()` callback, and thus the driver knows which
array element to handle.

Cc: Julien Panis <jpanis@baylibre.com>
Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/counter/counter-sysfs.c | 116 +++++++++++++++++++++++++++++++-
 include/linux/counter.h         |  78 +++++++++++++++++++++
 2 files changed, 192 insertions(+), 2 deletions(-)

diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index 026ea094d68e..464bc4750787 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
@@ -341,6 +341,72 @@ static ssize_t counter_comp_u64_store(struct device *dev,
 	return len;
 }
 
+static ssize_t counter_comp_array_u64_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	const struct counter_attribute *const a = to_counter_attribute(attr);
+	struct counter_device *const counter = counter_from_dev(dev);
+	const size_t idx = (size_t)a->comp.priv;
+	int err;
+	u64 data = 0;
+
+	switch (a->scope) {
+	case COUNTER_SCOPE_DEVICE:
+		err = a->comp.device_array_u64_read(counter, idx, &data);
+		break;
+	case COUNTER_SCOPE_SIGNAL:
+		err = a->comp.signal_array_u64_read(counter, a->parent, idx,
+						    &data);
+		break;
+	case COUNTER_SCOPE_COUNT:
+		err = a->comp.count_array_u64_read(counter, a->parent, idx,
+						   &data);
+		break;
+	default:
+		return -EINVAL;
+	}
+	if (err < 0)
+		return err;
+
+	return sysfs_emit(buf, "%llu\n", (unsigned long long)data);
+}
+
+static ssize_t counter_comp_array_u64_store(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
+{
+	const struct counter_attribute *const a = to_counter_attribute(attr);
+	struct counter_device *const counter = counter_from_dev(dev);
+	const size_t idx = (size_t)a->comp.priv;
+	int err;
+	u64 data = 0;
+
+	err = kstrtou64(buf, 0, &data);
+	if (err < 0)
+		return err;
+
+	switch (a->scope) {
+	case COUNTER_SCOPE_DEVICE:
+		err = a->comp.device_array_u64_write(counter, idx, data);
+		break;
+	case COUNTER_SCOPE_SIGNAL:
+		err = a->comp.signal_array_u64_write(counter, a->parent, idx,
+						     data);
+		break;
+	case COUNTER_SCOPE_COUNT:
+		err = a->comp.count_array_u64_write(counter, a->parent, idx,
+						    data);
+		break;
+	default:
+		return -EINVAL;
+	}
+	if (err < 0)
+		return err;
+
+	return len;
+}
+
 static ssize_t enums_available_show(const u32 *const enums,
 				    const size_t num_enums,
 				    const char *const strs[], char *buf)
@@ -488,6 +554,16 @@ static int counter_attr_create(struct device *const dev,
 			dev_attr->store = counter_comp_u64_store;
 		}
 		break;
+	case COUNTER_COMP_ARRAY_U64:
+		if (comp->count_array_u64_read) {
+			dev_attr->attr.mode |= 0444;
+			dev_attr->show = counter_comp_array_u64_show;
+		}
+		if (comp->count_array_u64_write) {
+			dev_attr->attr.mode |= 0200;
+			dev_attr->store = counter_comp_array_u64_store;
+		}
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -597,6 +673,38 @@ static int counter_ext_attrs_create(struct device *const dev,
 	return counter_comp_id_attr_create(dev, group, ext->name, id);
 }
 
+static int counter_array_attrs_create(struct device *const dev,
+				      struct counter_attribute_group *const group,
+				      const struct counter_comp *const comp,
+				      const enum counter_scope scope,
+				      void *const parent, const size_t id)
+{
+	const size_t length = (size_t)comp->priv;
+	struct counter_comp ext = *comp;
+	size_t i;
+	int err;
+
+	/* Create an attribute for each array element */
+	for (i = 0; i < length; i++) {
+		/* Set index for array element extension */
+		ext.priv = (void *)i;
+
+		/* Generate array element attribute name */
+		ext.name = devm_kasprintf(dev, GFP_KERNEL, "%s%zu", comp->name,
+					  i);
+		if (!ext.name)
+			return -ENOMEM;
+
+		/* Create all attributes associated with the array element */
+		err = counter_ext_attrs_create(dev, group, &ext, scope, parent,
+					       id + i);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
 static int counter_sysfs_exts_add(struct device *const dev,
 				  struct counter_attribute_group *const group,
 				  const struct counter_comp *const exts,
@@ -611,8 +719,12 @@ static int counter_sysfs_exts_add(struct device *const dev,
 	/* Create attributes for each extension */
 	for (i = 0; i < num_ext; i++) {
 		ext = &exts[i];
-		err = counter_ext_attrs_create(dev, group, ext, scope, parent,
-					       i);
+		if (ext->type == COUNTER_COMP_ARRAY_U64)
+			err = counter_array_attrs_create(dev, group, ext, scope,
+							 parent, i);
+		else
+			err = counter_ext_attrs_create(dev, group, ext, scope,
+						       parent, i);
 		if (err < 0)
 			return err;
 	}
diff --git a/include/linux/counter.h b/include/linux/counter.h
index 1fe17f5adb09..8ef4dd9d73dc 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -31,6 +31,7 @@ enum counter_comp_type {
 	COUNTER_COMP_ENUM,
 	COUNTER_COMP_COUNT_DIRECTION,
 	COUNTER_COMP_COUNT_MODE,
+	COUNTER_COMP_ARRAY_U64,
 };
 
 /**
@@ -68,6 +69,24 @@ enum counter_comp_type {
  * @signal_u64_read:		Signal u64 component read callback. The read value of
  *			the respective Signal u64 component should be passed
  *			back via the val parameter.
+ * @device_array_u64_read:	Device u64 array component read callback. The
+ *				index of the respective Device u64 array
+ *				component element is passed via the idx
+ *				parameter. The read value of the respective
+ *				Device u64 array component element should be
+ *				passed back via the val parameter.
+ * @count_array_u64_read:	Count u64 array component read callback. The
+ *				index of the respective Count u64 array
+ *				component element is passed via the idx
+ *				parameter. The read value of the respective
+ *				Count u64 array component element should be
+ *				passed back via the val parameter.
+ * @signal_array_u64_read:	Signal u64 array component read callback. The
+ *				index of the respective Count u64 array
+ *				component element is passed via the idx
+ *				parameter. The read value of the respective
+ *				Count u64 array component element should be
+ *				passed back via the val parameter.
  * @action_write:		Synapse action mode write callback. The write value of
  *			the respective Synapse action mode is passed via the
  *			action parameter.
@@ -98,6 +117,24 @@ enum counter_comp_type {
  * @signal_u64_write:		Signal u64 component write callback. The write value of
  *			the respective Signal u64 component is passed via the
  *			val parameter.
+ * @device_array_u64_write:	Device u64 array component write callback. The
+ *				index of the respective Device u64 array
+ *				component element is passed via the idx
+ *				parameter. The write value of the respective
+ *				Device u64 array component element is passed via
+ *				the val parameter.
+ * @count_array_u64_write:	Count u64 array component write callback. The
+ *				index of the respective Count u64 array
+ *				component element is passed via the idx
+ *				parameter. The write value of the respective
+ *				Count u64 array component element is passed via
+ *				the val parameter.
+ * @signal_array_u64_write:	Signal u64 array component write callback. The
+ *				index of the respective Signal u64 array
+ *				component element is passed via the idx
+ *				parameter. The write value of the respective
+ *				Signal u64 array component element is passed via
+ *				the val parameter.
  */
 struct counter_comp {
 	enum counter_comp_type type;
@@ -125,6 +162,14 @@ struct counter_comp {
 				      struct counter_count *count, u64 *val);
 		int (*signal_u64_read)(struct counter_device *counter,
 				       struct counter_signal *signal, u64 *val);
+		int (*device_array_u64_read)(struct counter_device *counter,
+					     size_t idx, u64 *val);
+		int (*count_array_u64_read)(struct counter_device *counter,
+					    struct counter_count *count,
+					    size_t idx, u64 *val);
+		int (*signal_array_u64_read)(struct counter_device *counter,
+					     struct counter_signal *signal,
+					     size_t idx, u64 *val);
 	};
 	union {
 		int (*action_write)(struct counter_device *counter,
@@ -148,6 +193,14 @@ struct counter_comp {
 				       struct counter_count *count, u64 val);
 		int (*signal_u64_write)(struct counter_device *counter,
 					struct counter_signal *signal, u64 val);
+		int (*device_array_u64_write)(struct counter_device *counter,
+					      size_t idx, u64 val);
+		int (*count_array_u64_write)(struct counter_device *counter,
+					     struct counter_count *count,
+					     size_t idx, u64 val);
+		int (*signal_array_u64_write)(struct counter_device *counter,
+					      struct counter_signal *signal,
+					      size_t idx, u64 val);
 	};
 };
 
@@ -452,6 +505,31 @@ struct counter_available {
 	.priv = &(_available), \
 }
 
+#define COUNTER_COMP_DEVICE_ARRAY_U64(_name, _read, _write, _length) \
+{ \
+	.type = COUNTER_COMP_ARRAY_U64, \
+	.name = (_name), \
+	.device_array_u64_read = (_read), \
+	.device_array_u64_write = (_write), \
+	.priv = (void *)(_length), \
+}
+#define COUNTER_COMP_COUNT_ARRAY_U64(_name, _read, _write, _length) \
+{ \
+	.type = COUNTER_COMP_ARRAY_U64, \
+	.name = (_name), \
+	.count_array_u64_read = (_read), \
+	.count_array_u64_write = (_write), \
+	.priv = (void *)(_length), \
+}
+#define COUNTER_COMP_SIGNAL_ARRAY_U64(_name, _read, _write, _length) \
+{ \
+	.type = COUNTER_COMP_ARRAY_U64, \
+	.name = (_name), \
+	.signal_array_u64_read = (_read), \
+	.signal_array_u64_write = (_write), \
+	.priv = (void *)(_length), \
+}
+
 #define COUNTER_COMP_CEILING(_read, _write) \
 	COUNTER_COMP_COUNT_U64("ceiling", _read, _write)
 
-- 
2.37.2

Re: [RFC PATCH 2/2] counter: Introduce the COUNTER_COMP_ARRAY_U64 component type

[Julien Panis]

On 29/08/2022 17:17, William Breathitt Gray wrote:
> The COUNTER_COMP_ARRAY_U64 Counter component type is introduced to
> enable support for Counter array components. With Counter array
> components, exposure for buffers on counter devices can be defined via
> new Counter array component macros. This should simplify code for driver
> authors who would otherwise need to define individual Counter components
> for each array element.
>
> Three Counter array component macros are introduced::
>
>          COUNTER_COMP_DEVICE_ARRAY_U64(_name, _read, _write, _length)
>          COUNTER_COMP_COUNT_ARRAY_U64(_name, _read, _write, _length)
>          COUNTER_COMP_SIGNAL_ARRAY_U64(_name, _read, _write, _length)

Hi William,

I have 2 comments :

1) What about ENUM ? I guess that it will not be possible to handle 
ARRAY of ENUM ?
     That would be useful for polarity0/1/2/3 in my ECAP driver
     (something like COUNTER_COMP_SIGNAL_ARRAY_ENUM for instance)

2) I made some tests with COUNTER_COMP_COUNT_ARRAY_U64
         COUNTER_COMP_COUNT_ARRAY_U64("capture", ecap_cnt_cap_read, 
NULL, ECAP_NB_CEVT)
     I can get consistent data value when using 'cat captureX' linux 
commands.
     But I get weird values when using watches from userspace 
application code (I will send my test results to you).

Julien Panis

Re: [RFC PATCH 2/2] counter: Introduce the COUNTER_COMP_ARRAY_U64 component type

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

On Tue, Sep 06, 2022 at 04:41:16PM +0200, Julien Panis wrote:
> 
> 
> On 29/08/2022 17:17, William Breathitt Gray wrote:
> > The COUNTER_COMP_ARRAY_U64 Counter component type is introduced to
> > enable support for Counter array components. With Counter array
> > components, exposure for buffers on counter devices can be defined via
> > new Counter array component macros. This should simplify code for driver
> > authors who would otherwise need to define individual Counter components
> > for each array element.
> > 
> > Three Counter array component macros are introduced::
> > 
> >          COUNTER_COMP_DEVICE_ARRAY_U64(_name, _read, _write, _length)
> >          COUNTER_COMP_COUNT_ARRAY_U64(_name, _read, _write, _length)
> >          COUNTER_COMP_SIGNAL_ARRAY_U64(_name, _read, _write, _length)
> 
> Hi William,
> 
> I have 2 comments :
> 
> 1) What about ENUM ? I guess that it will not be possible to handle ARRAY of
> ENUM ?
>     That would be useful for polarity0/1/2/3 in my ECAP driver
>     (something like COUNTER_COMP_SIGNAL_ARRAY_ENUM for instance)
> 
> 2) I made some tests with COUNTER_COMP_COUNT_ARRAY_U64
>         COUNTER_COMP_COUNT_ARRAY_U64("capture", ecap_cnt_cap_read, NULL,
> ECAP_NB_CEVT)
>     I can get consistent data value when using 'cat captureX' linux
> commands.
>     But I get weird values when using watches from userspace application
> code (I will send my test results to you).
> 
> Julien Panis

I have an idea that might work for supporting enum types, so I'll try to
implement support for that in v2. I was also able to track down the bug
you found so I'll have that fixed as well.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]