Add support for Counter array components

RFC PATCH Superseded William Breathitt Gray 2 participants 5 messages Aug 29, 2022 – Sep 10, 2022

Subsystems (2)

IIO Counter

lore

Share & tools

lore

William Breathitt Gray

author

2022-08-29 15:17 UTC #0 lore raw

cover Add support for Counter array components

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

Signature · 1 line

2.37.2

William Breathitt Gray

author

2022-08-29 15:17 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/counter/counter-sysfs.c | 98 ++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 49 deletions(-)

drivers/counter/counter-sysfs.c +49 ~~-49~~

index 04eac41dad33..026ea094d68e 100644
@@ -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;
 }

Signature · 1 line

2.37.2

William Breathitt Gray

author

2022-08-29 15:17 UTC #2 lore raw

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

In reply to William Breathitt Gray

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:

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 <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

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

drivers/counter/counter-sysfs.c +114 -2

index 026ea094d68e..464bc4750787 100644
@@ -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;
 	}

include/linux/counter.h +78 -0

114 lines not on this page · raw message

Signature · 1 line

2.37.2

Julien Panis

2022-09-06 15:31 UTC #3 lore raw

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

Quoted 7 lines · 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.

Quoted 5 lines

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

William Breathitt Gray

author

2022-09-10 23:40 UTC #4 lore raw

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

Quoted 33 lines · Julien Panis

Quoted 7 lines · 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.
Quoted 5 lines
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).

Quoted 1 line

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