On Wed, Dec 30, 2020 at 02:37:19PM +0000, Jonathan Cameron wrote:

On Fri, 25 Dec 2020 19:15:34 -0500
William Breathitt Gray [off-list ref] wrote:

quoted

This is a reimplementation of the Generic Counter driver interface.
There are no modifications to the Counter subsystem userspace interface,
so existing userspace applications should continue to run seamlessly.

Hi William

Been a while since I looked at this series.  Its rather big and I'm lazy
(or busy depending on who I'm talking to :)

Hmm. I'm a bit in two minds about how you should handle the huge amount of
description here.  Some of it clearly belongs in the kernel docs (and some
is I think put there in a later patch).  Other parts are specific to
this series, but I'm not 100% sure this much detail is really useful in the
git log.   Note that we now have links to the threads for all patches applied
using b4 (which this will be) so it's fine to have really detailed stuff
in cover letters rather than the individual patches.

I'll simplify the description here to something more succinct.

One thing that would be handy for review, might be if you put up a tree
somewhere with this applied and included a link.

This is such a large set of changes that having a tree to checkout for
review would be convenient. I typically push to my personal tree, so you
can check out the changes there in the counter_chrdev_v* branches:
https://gitlab.com/vilhelmgray/iio

I'll include a link to it again in the cover letter for v8 when it's
ready.

Mind you I don't feel that strongly about it if it you do want to maintain
it in the individual patch descriptions.

I've been a bit lazy and not cropped this down as much as I ideally should
have done (to include only bits I'm commenting on).

Anyhow, various minor things inline but this fundamentally looks fine to me.

Jonathan

quoted

Overview
========

The purpose of this patch is to internalize the sysfs interface code
among the various counter drivers into a shared module. Counter drivers
pass and take data natively (i.e. u8, u64, etc.) and the shared counter
module handles the translation between the sysfs interface.

Confusing statement.  Between the sysfs interface and what?
Perhaps "handles the translation to/from the sysfs interface."

Looks like I cut that line short by accident; it should read: "between
the sysfs interface and the device drivers". I'll fix this up.

quoted

This
guarantees a standard userspace interface for all counter drivers, and
helps generalize the Generic Counter driver ABI in order to support the
Generic Counter chrdev interface (introduced in a subsequent patch)
without significant changes to the existing counter drivers.

A high-level view of how a count value is passed down from a counter
driver is exemplified by the following. The driver callbacks are first
registered to the Counter core component for use by the Counter
userspace interface components:

                        +----------------------------+
	                | Counter device driver      |

Looks like something snuck a tab in amongst your spaces.

Ack.

quoted

 static int quad8_signal_read(struct counter_device *counter,
-	struct counter_signal *signal, enum counter_signal_value *val)
+			     struct counter_signal *signal,
+			     enum counter_signal_level *level)
 {
 	const struct quad8_iio *const priv = counter->priv;
 	unsigned int state;

@@ -633,13 +634,13 @@ static int quad8_signal_read(struct counter_device *counter,
 	state = inb(priv->base + QUAD8_REG_INDEX_INPUT_LEVELS)
 		& BIT(signal->id - 16);
 
-	*val = (state) ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
+	*level = (state) ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW;

This bit of refactoring / renaming could have been split out as a precursor patch
I think.  There may be other opportunities. 

Ack. I'll look around for additional changes I can pull out as precursor
patches too.

quoted

 
 	return 0;
 }
 
 static int quad8_count_read(struct counter_device *counter,
-	struct counter_count *count, unsigned long *val)
+			    struct counter_count *count, u64 *val)

Could the type change for val have been done as a precursor?

I don't think we can pull this one out as a precursor unfortunately.
Since unsigned long is passed in as pointer, we could get a type
mismatch if we're on a 32-bit system. For this to work, it requires the
u64 change in struct counter_ops and subsequent dependent code, so we'll
have to keep it as part of this patch for now.

quoted

@@ -785,18 +782,21 @@ static int quad8_function_set(struct counter_device *counter,
 		*quadrature_mode = 1;
 
 		switch (function) {
-		case QUAD8_COUNT_FUNCTION_QUADRATURE_X1:
+		case COUNTER_FUNCTION_QUADRATURE_X1_A:
 			*scale = 0;
 			mode_cfg |= QUAD8_CMR_QUADRATURE_X1;
 			break;
-		case QUAD8_COUNT_FUNCTION_QUADRATURE_X2:
+		case COUNTER_FUNCTION_QUADRATURE_X2_A:
 			*scale = 1;
 			mode_cfg |= QUAD8_CMR_QUADRATURE_X2;
 			break;
-		case QUAD8_COUNT_FUNCTION_QUADRATURE_X4:
+		case COUNTER_FUNCTION_QUADRATURE_X4:
 			*scale = 2;
 			mode_cfg |= QUAD8_CMR_QUADRATURE_X4;
 			break;
+		default:
+			mutex_unlock(&priv->lock);
+			return -EINVAL;

This looks like a sensible precaution / cleanup but could have been
done separately to the rest of the patch I think?

Ack.

quoted

@@ -1229,30 +1194,28 @@ static ssize_t quad8_count_ceiling_write(struct counter_device *counter,
 
 	mutex_unlock(&priv->lock);
 
-	return len;
+	return -EINVAL;

?  That looks like the good exit path to me.

You're right, this should be a return 0.

quoted

+/**
+ * counter_register - register Counter to the system
+ * @counter:	pointer to Counter to register
+ *
+ * This function registers a Counter to the system. A sysfs "counter" directory
+ * will be created and populated with sysfs attributes correlating with the
+ * Counter Signals, Synapses, and Counts respectively.

Where easy to do it's worth documenting return values.

Ack.

quoted

+static void devm_counter_unregister(struct device *dev, void *res)
+{
+	counter_unregister(*(struct counter_device **)res);

Rename this. It looks like it's a generic way of unwinding
devm_counter_register which it is definitely not...

Ack.

quoted

+/**
+ * struct counter_attribute - Counter sysfs attribute
+ * @dev_attr:	device attribute for sysfs
+ * @l:		node to add Counter attribute to attribute group list
+ * @comp:	Counter component callbacks and data
+ * @scope:	Counter scope of the attribute
+ * @parent:	pointer to the parent component
+ */
+struct counter_attribute {
+	struct device_attribute dev_attr;
+	struct list_head l;
+
+	struct counter_comp comp;
+	__u8 scope;

Why not an enum?

This should be enum; I missed it from the previous revision.

quoted

+	switch (a->comp.type) {
+	case COUNTER_COMP_FUNCTION:
+		return sprintf(buf, "%s\n", counter_function_str[data]);
+	case COUNTER_COMP_SIGNAL_LEVEL:
+		return sprintf(buf, "%s\n", counter_signal_value_str[data]);
+	case COUNTER_COMP_SYNAPSE_ACTION:
+		return sprintf(buf, "%s\n", counter_synapse_action_str[data]);
+	case COUNTER_COMP_ENUM:
+		return sprintf(buf, "%s\n", avail->strs[data]);
+	case COUNTER_COMP_COUNT_DIRECTION:
+		return sprintf(buf, "%s\n", counter_count_direction_str[data]);
+	case COUNTER_COMP_COUNT_MODE:
+		return sprintf(buf, "%s\n", counter_count_mode_str[data]);
+	default:

Perhaps move the below return sprintf() up here?

Ack.

quoted

+		break;
+	}
+
+	return sprintf(buf, "%u\n", (unsigned int)data);
+}
+
+static int find_in_string_array(u32 *const enum_item, const u32 *const enums,
+				const size_t num_enums, const char *const buf,
+				const char *const string_array[])

Please avoid defining such generically named functions.  High chance of a clash
with something that turns up in generic headers sometime in the future.

Ack.

quoted

+static ssize_t enums_available_show(const u32 *const enums,
+				    const size_t num_enums,
+				    const char *const strs[], char *buf)
+{
+	size_t len = 0;
+	size_t index;
+
+	for (index = 0; index < num_enums; index++)
+		len += sprintf(buf + len, "%s\n", strs[enums[index]]);

Probably better to add protections on overrunning the buffer to this.
Sure it won't actually happen but that may not be obvious to someone reading
this code in future.

Look at new sysfs_emit * family for this.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2efc459d06f1630001e3984854848a5647086232

Ack.

quoted

+static ssize_t counter_comp_available_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	const struct counter_attribute *const a = to_counter_attribute(attr);
+	const struct counter_count *const count = a->parent;
+	const struct counter_synapse *const synapse = a->comp.priv;
+	const struct counter_available *const avail = a->comp.priv;
+
+	switch (a->comp.type) {
+	case COUNTER_COMP_FUNCTION:
+		return enums_available_show(count->functions_list,
+					    count->num_functions,
+					    counter_function_str, buf);
+	case COUNTER_COMP_SYNAPSE_ACTION:
+		return enums_available_show(synapse->actions_list,
+					    synapse->num_actions,
+					    counter_synapse_action_str, buf);
+	case COUNTER_COMP_ENUM:
+		return strs_available_show(avail, buf);
+	case COUNTER_COMP_COUNT_MODE:
+		return enums_available_show(avail->enums, avail->num_items,
+					    counter_count_mode_str, buf);
+	default:
+		break;

Might as well return -EINVAL; here

Ack.

quoted

+	/* Store list node */
+	list_add(&counter_attr->l, &group->attr_list);
+	group->num_attr++;
+
+	switch (comp->type) {
+	case COUNTER_COMP_FUNCTION:
+	case COUNTER_COMP_SYNAPSE_ACTION:
+	case COUNTER_COMP_ENUM:
+	case COUNTER_COMP_COUNT_MODE:
+		return counter_avail_attr_create(dev, group, comp, parent);
+	default:
+		break;

return 0 in here.  Also add a comment on why it isn't an error.

Ack.

quoted

+static int counter_sysfs_synapses_add(struct counter_device *const counter,
+	struct counter_attribute_group *const group,
+	struct counter_count *const count)
+{
+	const __u8 scope = COUNTER_SCOPE_COUNT;
+	struct device *const dev = &counter->dev;
+	size_t i;
+	struct counter_synapse *synapse;
+	size_t id;
+	struct counter_comp comp;
+	int err;
+
+	/* Add each Synapse */
+	for (i = 0; i < count->num_synapses; i++) {

Could reduce scope and make code a bit more readable by
pulling

struct counter_synapse *synapse;
struct counter_comp comp;
size_t id;

and maybe other things in here.  Makes it clear their scope
is just within this loop.

Ack.

quoted

 /**
  * struct counter_synapse - Counter Synapse node
- * @action:		index of current action mode
  * @actions_list:	array of available action modes
  * @num_actions:	number of action modes specified in @actions_list
- * @signal:		pointer to associated signal
+ * @signal:		pointer to the associated Signal

Might have been nice to pull the cases that were purely capitalization out as
a separate patch immediately following this one. There aren't
a huge number, but from a review point of view it's a noop patch
so doesn't need the reviewer to be awake :)

Ack.

William Breathitt Gray