[PATCH 0/6] gpiolib: cdev: code cleanup following hte integration

[PATCH 0/6] gpiolib: cdev: code cleanup following hte integration

[Kent Gibson]

This patch series is a collection of improvements to simplify the
code, improve readability, and compile out unused code.
There are no functional changes.

The first patch is a cleanup for my recent linereq_free() fix. I
noted then that the edge_detector_stop() could probably be safely
moved inside the line desc check block, but wanted to keep that
change minimal just in case.  It can be safely moved, and so here
it is.

Patch 2 makes use of an existing macro to simplify a call.

Patch 3 replaces some more if-else chains with switches, which is
more readable (YMMV).

Patch 4 reorganizes the line identification code to share code
common to alternate paths.

Patch 5 consolidates a number of separate flags into one.  This
reduces code complexity, simplifies any future edge source additions,
and makes patch 6 significantly simpler.

Patch 6 totally compiles out the hte specific code when CONFIG_HTE
is not selected.

I've based this series on gpio/for-current, as it requires the fix
patch -
commit c8e27a4a5136 ("gpiolib: cdev: fix null pointer dereference in linereq_free()")
Happy to rebase if that doesn't suit.

Dipen, I don't have any HTE compatible hardware to test with, so
could you check that this still works for you?


Kent Gibson (6):
  gpiolib: cdev: simplify linereq_free
  gpiolib: cdev: simplify parameter in call to hte_edge_setup
  gpiolib: cdev: replace if-else chains with switches
  gpiolib: cdev: simplify line event identification
  gpiolib: cdev: consolidate edge detector configuration flags
  gpiolib: cdev: compile out HTE unless CONFIG_HTE selected

 drivers/gpio/gpiolib-cdev.c | 286 +++++++++++++++++++-----------------
 1 file changed, 149 insertions(+), 137 deletions(-)


base-commit: 7329b071729645e243b6207e76bca2f4951c991b
-- 
2.37.0

[PATCH 1/6] gpiolib: cdev: simplify linereq_free

[Kent Gibson]

The edge detector is only ever started after the line desc has been
determined, so move edge_detector_stop() inside the line desc check,
and merge the two checked regions into one.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 0c9a63becfef..b44526e3630e 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1460,15 +1460,15 @@ static ssize_t linereq_read(struct file *file,
 static void linereq_free(struct linereq *lr)
 {
 	unsigned int i;
-	bool hte = false;
+	bool hte;
 
 	for (i = 0; i < lr->num_lines; i++) {
-		if (lr->lines[i].desc)
+		if (lr->lines[i].desc) {
 			hte = !!test_bit(FLAG_EVENT_CLOCK_HTE,
 					 &lr->lines[i].desc->flags);
-		edge_detector_stop(&lr->lines[i], hte);
-		if (lr->lines[i].desc)
+			edge_detector_stop(&lr->lines[i], hte);
 			gpiod_free(lr->lines[i].desc);
+		}
 	}
 	kfifo_free(&lr->events);
 	kfree(lr->label);
-- 
2.37.0

[PATCH 2/6] gpiolib: cdev: simplify parameter in call to hte_edge_setup

[Kent Gibson]

Improve readability by using the GPIO_V2_LINE_FLAG_EDGE_BOTH instead
of combining the rising and falling edge flags.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index b44526e3630e..f635bbbb6a6d 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -885,9 +885,7 @@ static int debounce_setup(struct line *line,
 				return ret;
 			line->irq = irq;
 		} else {
-			ret = hte_edge_setup(line,
-					     GPIO_V2_LINE_FLAG_EDGE_RISING |
-					     GPIO_V2_LINE_FLAG_EDGE_FALLING);
+			ret = hte_edge_setup(line, GPIO_V2_LINE_FLAG_EDGE_BOTH);
 			if (ret)
 				return ret;
 		}
-- 
2.37.0

[PATCH 3/6] gpiolib: cdev: replace if-else chains with switches

[Kent Gibson]

Improve readability by replacing if-else chains with switch
statements.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index f635bbbb6a6d..bc7c8822ede0 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -588,7 +588,8 @@ static enum hte_return process_hw_ts_thread(void *p)
 	le.timestamp_ns = line->timestamp_ns;
 	eflags = READ_ONCE(line->eflags);
 
-	if (eflags == GPIO_V2_LINE_FLAG_EDGE_BOTH) {
+	switch (eflags) {
+	case GPIO_V2_LINE_FLAG_EDGE_BOTH:
 		if (line->raw_level >= 0) {
 			if (test_bit(FLAG_ACTIVE_LOW, &line->desc->flags))
 				level = !line->raw_level;
@@ -602,13 +603,16 @@ static enum hte_return process_hw_ts_thread(void *p)
 			le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
 		else
 			le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
-	} else if (eflags == GPIO_V2_LINE_FLAG_EDGE_RISING) {
+		break;
+	case GPIO_V2_LINE_FLAG_EDGE_RISING:
 		/* Emit low-to-high event */
 		le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
-	} else if (eflags == GPIO_V2_LINE_FLAG_EDGE_FALLING) {
+		break;
+	case GPIO_V2_LINE_FLAG_EDGE_FALLING:
 		/* Emit high-to-low event */
 		le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
-	} else {
+		break;
+	default:
 		return HTE_CB_HANDLED;
 	}
 	le.line_seqno = line->line_seqno;
@@ -660,7 +664,6 @@ static irqreturn_t edge_irq_thread(int irq, void *p)
 	struct line *line = p;
 	struct linereq *lr = line->req;
 	struct gpio_v2_line_event le;
-	u64 eflags;
 
 	/* Do not leak kernel stack to userspace */
 	memset(&le, 0, sizeof(le));
@@ -679,23 +682,25 @@ static irqreturn_t edge_irq_thread(int irq, void *p)
 	}
 	line->timestamp_ns = 0;
 
-	eflags = READ_ONCE(line->eflags);
-	if (eflags == GPIO_V2_LINE_FLAG_EDGE_BOTH) {
-		int level = gpiod_get_value_cansleep(line->desc);
-
-		if (level)
+	switch (READ_ONCE(line->eflags)) {
+	case GPIO_V2_LINE_FLAG_EDGE_BOTH:
+		if (gpiod_get_value_cansleep(line->desc))
 			/* Emit low-to-high event */
 			le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
 		else
 			/* Emit high-to-low event */
 			le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
-	} else if (eflags == GPIO_V2_LINE_FLAG_EDGE_RISING) {
+
+		break;
+	case GPIO_V2_LINE_FLAG_EDGE_RISING:
 		/* Emit low-to-high event */
 		le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
-	} else if (eflags == GPIO_V2_LINE_FLAG_EDGE_FALLING) {
+		break;
+	case GPIO_V2_LINE_FLAG_EDGE_FALLING:
 		/* Emit high-to-low event */
 		le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
-	} else {
+		break;
+	default:
 		return IRQ_NONE;
 	}
 	line->line_seqno++;
-- 
2.37.0

[PATCH 4/6] gpiolib: cdev: simplify line event identification

[Kent Gibson]

Reorganise line event identification code to reduce code duplication,
and replace if-else initializers with the ternary equivalent to
improve readability.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 42 ++++++++++++-------------------------
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index bc7c8822ede0..34d0bdfe5498 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -590,26 +590,20 @@ static enum hte_return process_hw_ts_thread(void *p)
 
 	switch (eflags) {
 	case GPIO_V2_LINE_FLAG_EDGE_BOTH:
-		if (line->raw_level >= 0) {
-			if (test_bit(FLAG_ACTIVE_LOW, &line->desc->flags))
-				level = !line->raw_level;
-			else
-				level = line->raw_level;
-		} else {
-			level = gpiod_get_value_cansleep(line->desc);
-		}
+		level = (line->raw_level >= 0) ?
+				line->raw_level :
+				gpiod_get_raw_value_cansleep(line->desc);
 
-		if (level)
-			le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
-		else
-			le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
+		if (test_bit(FLAG_ACTIVE_LOW, &line->desc->flags))
+			level = !level;
+
+		le.id = level ? GPIO_V2_LINE_EVENT_RISING_EDGE :
+				GPIO_V2_LINE_EVENT_FALLING_EDGE;
 		break;
 	case GPIO_V2_LINE_FLAG_EDGE_RISING:
-		/* Emit low-to-high event */
 		le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
 		break;
 	case GPIO_V2_LINE_FLAG_EDGE_FALLING:
-		/* Emit high-to-low event */
 		le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
 		break;
 	default:
@@ -684,20 +678,14 @@ static irqreturn_t edge_irq_thread(int irq, void *p)
 
 	switch (READ_ONCE(line->eflags)) {
 	case GPIO_V2_LINE_FLAG_EDGE_BOTH:
-		if (gpiod_get_value_cansleep(line->desc))
-			/* Emit low-to-high event */
-			le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
-		else
-			/* Emit high-to-low event */
-			le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
-
+		le.id = gpiod_get_value_cansleep(line->desc) ?
+				GPIO_V2_LINE_EVENT_RISING_EDGE :
+				GPIO_V2_LINE_EVENT_FALLING_EDGE;
 		break;
 	case GPIO_V2_LINE_FLAG_EDGE_RISING:
-		/* Emit low-to-high event */
 		le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
 		break;
 	case GPIO_V2_LINE_FLAG_EDGE_FALLING:
-		/* Emit high-to-low event */
 		le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
 		break;
 	default:
@@ -821,12 +809,8 @@ static void debounce_work_func(struct work_struct *work)
 			le.line_seqno : atomic_inc_return(&lr->seqno);
 	}
 
-	if (level)
-		/* Emit low-to-high event */
-		le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
-	else
-		/* Emit high-to-low event */
-		le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
+	le.id = level ? GPIO_V2_LINE_EVENT_RISING_EDGE :
+			GPIO_V2_LINE_EVENT_FALLING_EDGE;
 
 	linereq_put_event(lr, &le);
 }
-- 
2.37.0

[PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags

[Kent Gibson]

Combine the polarity_change flag, struct line eflags, and hte enable
flag into a single flag variable.

The combination of these flags describes the configuration state
of the edge detector, so formalize and clarify that by combining
them into a single variable, edflags, in struct line.

The edflags is a subset of the GPIO_V2_LINE_FLAGsb relevant to
the edge detector, and is also a superset of the eflags it replaces.
The eflags name is still used to describe the subset of edflags
corresponding to the rising/falling edge flags where edflags is
masked down to that subset.

This consolidation reduces the number of variables being passed,
simplifies state comparisons, and provides a more extensible
foundation should additional edge sources be integrated in the
future.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 126 +++++++++++++++++-------------------
 1 file changed, 60 insertions(+), 66 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 34d0bdfe5498..406b9e063374 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -430,12 +430,15 @@ struct line {
 	struct linereq *req;
 	unsigned int irq;
 	/*
-	 * eflags is set by edge_detector_setup(), edge_detector_stop() and
-	 * edge_detector_update(), which are themselves mutually exclusive,
-	 * and is accessed by edge_irq_thread() and debounce_work_func(),
-	 * which can both live with a slightly stale value.
+	 * The flags for the active edge detector configuration.
+	 *
+	 * edflags is set by linereq_create(), linereq_free(), and
+	 * linereq_set_config_unlocked(), which are themselves mutually
+	 * exclusive, and is accessed by edge_irq_thread(),
+	 * process_hw_ts_thread() and debounce_work_func(),
+	 * which can all live with a slightly stale value.
 	 */
-	u64 eflags;
+	u64 edflags;
 	/*
 	 * timestamp_ns and req_seqno are accessed only by
 	 * edge_irq_handler() and edge_irq_thread(), which are themselves
@@ -541,6 +544,12 @@ struct linereq {
 	 GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE | \
 	 GPIO_V2_LINE_BIAS_FLAGS)
 
+/* subset of flags relevant for edge detector configuration */
+#define GPIO_V2_LINE_EDGE_DETECTOR_FLAGS \
+	(GPIO_V2_LINE_FLAG_ACTIVE_LOW | \
+	 GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE | \
+	 GPIO_V2_LINE_EDGE_FLAGS)
+
 static void linereq_put_event(struct linereq *lr,
 			      struct gpio_v2_line_event *le)
 {
@@ -575,7 +584,7 @@ static enum hte_return process_hw_ts_thread(void *p)
 	struct linereq *lr;
 	struct gpio_v2_line_event le;
 	int level;
-	u64 eflags;
+	u64 edflags;
 
 	if (!p)
 		return HTE_CB_HANDLED;
@@ -586,15 +595,15 @@ static enum hte_return process_hw_ts_thread(void *p)
 	memset(&le, 0, sizeof(le));
 
 	le.timestamp_ns = line->timestamp_ns;
-	eflags = READ_ONCE(line->eflags);
+	edflags = READ_ONCE(line->edflags);
 
-	switch (eflags) {
+	switch (edflags & GPIO_V2_LINE_EDGE_FLAGS) {
 	case GPIO_V2_LINE_FLAG_EDGE_BOTH:
 		level = (line->raw_level >= 0) ?
 				line->raw_level :
 				gpiod_get_raw_value_cansleep(line->desc);
 
-		if (test_bit(FLAG_ACTIVE_LOW, &line->desc->flags))
+		if (edflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW)
 			level = !level;
 
 		le.id = level ? GPIO_V2_LINE_EVENT_RISING_EDGE :
@@ -676,7 +685,7 @@ static irqreturn_t edge_irq_thread(int irq, void *p)
 	}
 	line->timestamp_ns = 0;
 
-	switch (READ_ONCE(line->eflags)) {
+	switch (READ_ONCE(line->edflags) & GPIO_V2_LINE_EDGE_FLAGS) {
 	case GPIO_V2_LINE_FLAG_EDGE_BOTH:
 		le.id = gpiod_get_value_cansleep(line->desc) ?
 				GPIO_V2_LINE_EVENT_RISING_EDGE :
@@ -753,16 +762,13 @@ static void debounce_work_func(struct work_struct *work)
 	struct gpio_v2_line_event le;
 	struct line *line = container_of(work, struct line, work.work);
 	struct linereq *lr;
-	int level, diff_seqno;
-	u64 eflags;
+	int level = -1, diff_seqno;
+	u64 eflags, edflags = READ_ONCE(line->edflags);
 
-	if (test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)) {
+	if (edflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE)
 		level = line->raw_level;
-		if (level < 0)
-			level = gpiod_get_raw_value_cansleep(line->desc);
-	} else {
+	if (level < 0)
 		level = gpiod_get_raw_value_cansleep(line->desc);
-	}
 	if (level < 0) {
 		pr_debug_ratelimited("debouncer failed to read line value\n");
 		return;
@@ -774,12 +780,12 @@ static void debounce_work_func(struct work_struct *work)
 	WRITE_ONCE(line->level, level);
 
 	/* -- edge detection -- */
-	eflags = READ_ONCE(line->eflags);
+	eflags = edflags & GPIO_V2_LINE_EDGE_FLAGS;
 	if (!eflags)
 		return;
 
 	/* switch from physical level to logical - if they differ */
-	if (test_bit(FLAG_ACTIVE_LOW, &line->desc->flags))
+	if (edflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW)
 		level = !level;
 
 	/* ignore edges that are not being monitored */
@@ -793,7 +799,7 @@ static void debounce_work_func(struct work_struct *work)
 	lr = line->req;
 	le.timestamp_ns = line_event_timestamp(line);
 	le.offset = gpio_chip_hwgpio(line->desc);
-	if (test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)) {
+	if (edflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE) {
 		/* discard events except the last one */
 		line->total_discard_seq -= 1;
 		diff_seqno = line->last_seqno - line->total_discard_seq -
@@ -841,8 +847,7 @@ static int hte_edge_setup(struct line *line, u64 eflags)
 				 process_hw_ts_thread, line);
 }
 
-static int debounce_setup(struct line *line,
-			  unsigned int debounce_period_us, bool hte_req)
+static int debounce_setup(struct line *line, unsigned int debounce_period_us)
 {
 	unsigned long irqflags;
 	int ret, level, irq;
@@ -862,7 +867,7 @@ static int debounce_setup(struct line *line,
 		if (level < 0)
 			return level;
 
-		if (!hte_req) {
+		if (!test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)) {
 			irq = gpiod_to_irq(line->desc);
 			if (irq < 0)
 				return -ENXIO;
@@ -913,19 +918,19 @@ static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc,
 	return 0;
 }
 
-static void edge_detector_stop(struct line *line, bool hte_en)
+static void edge_detector_stop(struct line *line)
 {
-	if (line->irq && !hte_en) {
+	if (line->irq) {
 		free_irq(line->irq, line);
 		line->irq = 0;
 	}
 
-	if (hte_en)
+	if (READ_ONCE(line->edflags) & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE)
 		hte_ts_put(&line->hdesc);
 
 	cancel_delayed_work_sync(&line->work);
 	WRITE_ONCE(line->sw_debounced, 0);
-	WRITE_ONCE(line->eflags, 0);
+	WRITE_ONCE(line->edflags, 0);
 	if (line->desc)
 		WRITE_ONCE(line->desc->debounce_period_us, 0);
 	/* do not change line->level - see comment in debounced_value() */
@@ -933,23 +938,23 @@ static void edge_detector_stop(struct line *line, bool hte_en)
 
 static int edge_detector_setup(struct line *line,
 			       struct gpio_v2_line_config *lc,
-			       unsigned int line_idx,
-			       u64 eflags, bool hte_req)
+			       unsigned int line_idx, u64 edflags)
 {
 	u32 debounce_period_us;
 	unsigned long irqflags = 0;
 	int irq, ret;
+	u64 eflags;
 
+	eflags = edflags & GPIO_V2_LINE_EDGE_FLAGS;
 	if (eflags && !kfifo_initialized(&line->req->events)) {
 		ret = kfifo_alloc(&line->req->events,
 				  line->req->event_buffer_size, GFP_KERNEL);
 		if (ret)
 			return ret;
 	}
-	WRITE_ONCE(line->eflags, eflags);
 	if (gpio_v2_line_config_debounced(lc, line_idx)) {
 		debounce_period_us = gpio_v2_line_config_debounce_period(lc, line_idx);
-		ret = debounce_setup(line, debounce_period_us, hte_req);
+		ret = debounce_setup(line, debounce_period_us);
 		if (ret)
 			return ret;
 		WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
@@ -959,8 +964,8 @@ static int edge_detector_setup(struct line *line,
 	if (!eflags || READ_ONCE(line->sw_debounced))
 		return 0;
 
-	if (hte_req)
-		return hte_edge_setup(line, eflags);
+	if (edflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE)
+		return hte_edge_setup(line, edflags);
 
 	irq = gpiod_to_irq(line->desc);
 	if (irq < 0)
@@ -986,35 +991,29 @@ static int edge_detector_setup(struct line *line,
 
 static int edge_detector_update(struct line *line,
 				struct gpio_v2_line_config *lc,
-				unsigned int line_idx,
-				u64 flags, bool polarity_change,
-				bool prev_hte_flag)
+				unsigned int line_idx, u64 edflags)
 {
-	u64 eflags = flags & GPIO_V2_LINE_EDGE_FLAGS;
+	u64 active_edflags = READ_ONCE(line->edflags);
 	unsigned int debounce_period_us =
 			gpio_v2_line_config_debounce_period(lc, line_idx);
-	bool hte_change = (prev_hte_flag !=
-		      ((flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE) != 0));
 
-	if ((READ_ONCE(line->eflags) == eflags) && !polarity_change &&
-	    (READ_ONCE(line->desc->debounce_period_us) == debounce_period_us)
-	    && !hte_change)
+	if ((active_edflags == edflags) &&
+	    (READ_ONCE(line->desc->debounce_period_us) == debounce_period_us))
 		return 0;
 
 	/* sw debounced and still will be...*/
 	if (debounce_period_us && READ_ONCE(line->sw_debounced)) {
-		WRITE_ONCE(line->eflags, eflags);
 		WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
 		return 0;
 	}
 
 	/* reconfiguring edge detection or sw debounce being disabled */
-	if ((line->irq && !READ_ONCE(line->sw_debounced)) || prev_hte_flag ||
+	if ((line->irq && !READ_ONCE(line->sw_debounced)) ||
+	    (active_edflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE) ||
 	    (!debounce_period_us && READ_ONCE(line->sw_debounced)))
-		edge_detector_stop(line, prev_hte_flag);
+		edge_detector_stop(line);
 
-	return edge_detector_setup(line, lc, line_idx, eflags,
-				   flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE);
+	return edge_detector_setup(line, lc, line_idx, edflags);
 }
 
 static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc,
@@ -1283,22 +1282,17 @@ static long linereq_set_config_unlocked(struct linereq *lr,
 					struct gpio_v2_line_config *lc)
 {
 	struct gpio_desc *desc;
+	struct line *line;
 	unsigned int i;
-	u64 flags;
-	bool polarity_change;
-	bool prev_hte_flag;
+	u64 flags, edflags;
 	int ret;
 
 	for (i = 0; i < lr->num_lines; i++) {
+		line = &lr->lines[i];
 		desc = lr->lines[i].desc;
 		flags = gpio_v2_line_config_flags(lc, i);
-		polarity_change =
-			(!!test_bit(FLAG_ACTIVE_LOW, &desc->flags) !=
-			 ((flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW) != 0));
-
-		prev_hte_flag = !!test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags);
-
 		gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags);
+		edflags = flags & GPIO_V2_LINE_EDGE_DETECTOR_FLAGS;
 		/*
 		 * Lines have to be requested explicitly for input
 		 * or output, else the line will be treated "as is".
@@ -1306,7 +1300,7 @@ static long linereq_set_config_unlocked(struct linereq *lr,
 		if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
 			int val = gpio_v2_line_config_output_value(lc, i);
 
-			edge_detector_stop(&lr->lines[i], prev_hte_flag);
+			edge_detector_stop(line);
 			ret = gpiod_direction_output(desc, val);
 			if (ret)
 				return ret;
@@ -1315,12 +1309,13 @@ static long linereq_set_config_unlocked(struct linereq *lr,
 			if (ret)
 				return ret;
 
-			ret = edge_detector_update(&lr->lines[i], lc, i,
-					flags, polarity_change, prev_hte_flag);
+			ret = edge_detector_update(line, lc, i, edflags);
 			if (ret)
 				return ret;
 		}
 
+		WRITE_ONCE(line->edflags, edflags);
+
 		blocking_notifier_call_chain(&desc->gdev->notifier,
 					     GPIO_V2_LINE_CHANGED_CONFIG,
 					     desc);
@@ -1447,13 +1442,10 @@ static ssize_t linereq_read(struct file *file,
 static void linereq_free(struct linereq *lr)
 {
 	unsigned int i;
-	bool hte;
 
 	for (i = 0; i < lr->num_lines; i++) {
 		if (lr->lines[i].desc) {
-			hte = !!test_bit(FLAG_EVENT_CLOCK_HTE,
-					 &lr->lines[i].desc->flags);
-			edge_detector_stop(&lr->lines[i], hte);
+			edge_detector_stop(&lr->lines[i]);
 			gpiod_free(lr->lines[i].desc);
 		}
 	}
@@ -1489,7 +1481,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 	struct gpio_v2_line_config *lc;
 	struct linereq *lr;
 	struct file *file;
-	u64 flags;
+	u64 flags, edflags;
 	unsigned int i;
 	int fd, ret;
 
@@ -1563,6 +1555,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 		if (ret < 0)
 			goto out_free_linereq;
 
+		edflags = flags & GPIO_V2_LINE_EDGE_DETECTOR_FLAGS;
 		/*
 		 * Lines have to be requested explicitly for input
 		 * or output, else the line will be treated "as is".
@@ -1579,12 +1572,13 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 				goto out_free_linereq;
 
 			ret = edge_detector_setup(&lr->lines[i], lc, i,
-				flags & GPIO_V2_LINE_EDGE_FLAGS,
-				flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE);
+						  edflags);
 			if (ret)
 				goto out_free_linereq;
 		}
 
+		lr->lines[i].edflags = edflags;
+
 		blocking_notifier_call_chain(&desc->gdev->notifier,
 					     GPIO_V2_LINE_CHANGED_REQUESTED, desc);
 
-- 
2.37.0

[PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected

[Kent Gibson]

The majority of builds do not include HTE, so compile out hte
functionality unless CONFIG_HTE is selected.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 95 ++++++++++++++++++++++++-------------
 1 file changed, 63 insertions(+), 32 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 406b9e063374..7e7058141cd2 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -468,6 +468,7 @@ struct line {
 	 * stale value.
 	 */
 	unsigned int level;
+#ifdef CONFIG_HTE
 	/*
 	 * -- hte specific fields --
 	 */
@@ -487,6 +488,7 @@ struct line {
 	 * last sequence number before debounce period expires.
 	 */
 	u32 last_seqno;
+#endif /* CONFIG_HTE */
 };
 
 /**
@@ -572,12 +574,15 @@ static u64 line_event_timestamp(struct line *line)
 {
 	if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags))
 		return ktime_get_real_ns();
-	else if (test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags))
+	else if (IS_ENABLED(CONFIG_HTE) &&
+		 (test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)))
 		return line->timestamp_ns;
 
 	return ktime_get_ns();
 }
 
+#ifdef CONFIG_HTE
+
 static enum hte_return process_hw_ts_thread(void *p)
 {
 	struct line *line;
@@ -662,6 +667,42 @@ static enum hte_return process_hw_ts(struct hte_ts_data *ts, void *p)
 	return HTE_CB_HANDLED;
 }
 
+static int hte_edge_setup(struct line *line, u64 eflags)
+{
+	int ret;
+	unsigned long flags = 0;
+	struct hte_ts_desc *hdesc = &line->hdesc;
+
+	if (eflags & GPIO_V2_LINE_FLAG_EDGE_RISING)
+		flags |= test_bit(FLAG_ACTIVE_LOW, &line->desc->flags) ?
+				 HTE_FALLING_EDGE_TS :
+				 HTE_RISING_EDGE_TS;
+	if (eflags & GPIO_V2_LINE_FLAG_EDGE_FALLING)
+		flags |= test_bit(FLAG_ACTIVE_LOW, &line->desc->flags) ?
+				 HTE_RISING_EDGE_TS :
+				 HTE_FALLING_EDGE_TS;
+
+	line->total_discard_seq = 0;
+
+	hte_init_line_attr(hdesc, desc_to_gpio(line->desc), flags, NULL,
+			   line->desc);
+
+	ret = hte_ts_get(NULL, hdesc, 0);
+	if (ret)
+		return ret;
+
+	return hte_request_ts_ns(hdesc, process_hw_ts, process_hw_ts_thread,
+				 line);
+}
+
+#else
+
+static int hte_edge_setup(struct line *line, u64 eflags)
+{
+	return 0;
+}
+#endif /* CONFIG_HTE */
+
 static irqreturn_t edge_irq_thread(int irq, void *p)
 {
 	struct line *line = p;
@@ -762,11 +803,14 @@ static void debounce_work_func(struct work_struct *work)
 	struct gpio_v2_line_event le;
 	struct line *line = container_of(work, struct line, work.work);
 	struct linereq *lr;
-	int level = -1, diff_seqno;
+	int level = -1;
 	u64 eflags, edflags = READ_ONCE(line->edflags);
+#ifdef CONFIG_HTE
+	int diff_seqno;
 
 	if (edflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE)
 		level = line->raw_level;
+#endif
 	if (level < 0)
 		level = gpiod_get_raw_value_cansleep(line->desc);
 	if (level < 0) {
@@ -799,6 +843,7 @@ static void debounce_work_func(struct work_struct *work)
 	lr = line->req;
 	le.timestamp_ns = line_event_timestamp(line);
 	le.offset = gpio_chip_hwgpio(line->desc);
+#ifdef CONFIG_HTE
 	if (edflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE) {
 		/* discard events except the last one */
 		line->total_discard_seq -= 1;
@@ -808,7 +853,9 @@ static void debounce_work_func(struct work_struct *work)
 		le.line_seqno = line->line_seqno;
 		le.seqno = (lr->num_lines == 1) ?
 			le.line_seqno : atomic_add_return(diff_seqno, &lr->seqno);
-	} else {
+	} else
+#endif /* CONFIG_HTE */
+	{
 		line->line_seqno++;
 		le.line_seqno = line->line_seqno;
 		le.seqno = (lr->num_lines == 1) ?
@@ -821,32 +868,6 @@ static void debounce_work_func(struct work_struct *work)
 	linereq_put_event(lr, &le);
 }
 
-static int hte_edge_setup(struct line *line, u64 eflags)
-{
-	int ret;
-	unsigned long flags = 0;
-	struct hte_ts_desc *hdesc = &line->hdesc;
-
-	if (eflags & GPIO_V2_LINE_FLAG_EDGE_RISING)
-		flags |= test_bit(FLAG_ACTIVE_LOW, &line->desc->flags) ?
-				  HTE_FALLING_EDGE_TS : HTE_RISING_EDGE_TS;
-	if (eflags & GPIO_V2_LINE_FLAG_EDGE_FALLING)
-		flags |= test_bit(FLAG_ACTIVE_LOW, &line->desc->flags) ?
-				  HTE_RISING_EDGE_TS : HTE_FALLING_EDGE_TS;
-
-	line->total_discard_seq = 0;
-
-	hte_init_line_attr(hdesc, desc_to_gpio(line->desc), flags,
-			   NULL, line->desc);
-
-	ret = hte_ts_get(NULL, hdesc, 0);
-	if (ret)
-		return ret;
-
-	return hte_request_ts_ns(hdesc, process_hw_ts,
-				 process_hw_ts_thread, line);
-}
-
 static int debounce_setup(struct line *line, unsigned int debounce_period_us)
 {
 	unsigned long irqflags;
@@ -867,7 +888,8 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
 		if (level < 0)
 			return level;
 
-		if (!test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)) {
+		if (!IS_ENABLED(CONFIG_HTE) ||
+		    !test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)) {
 			irq = gpiod_to_irq(line->desc);
 			if (irq < 0)
 				return -ENXIO;
@@ -925,8 +947,10 @@ static void edge_detector_stop(struct line *line)
 		line->irq = 0;
 	}
 
+#ifdef CONFIG_HTE
 	if (READ_ONCE(line->edflags) & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE)
 		hte_ts_put(&line->hdesc);
+#endif
 
 	cancel_delayed_work_sync(&line->work);
 	WRITE_ONCE(line->sw_debounced, 0);
@@ -964,7 +988,8 @@ static int edge_detector_setup(struct line *line,
 	if (!eflags || READ_ONCE(line->sw_debounced))
 		return 0;
 
-	if (edflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE)
+	if (IS_ENABLED(CONFIG_HTE) &&
+	    (edflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE))
 		return hte_edge_setup(line, edflags);
 
 	irq = gpiod_to_irq(line->desc);
@@ -1049,6 +1074,11 @@ static int gpio_v2_line_flags_validate(u64 flags)
 	/* Return an error if an unknown flag is set */
 	if (flags & ~GPIO_V2_LINE_VALID_FLAGS)
 		return -EINVAL;
+
+	if (!IS_ENABLED(CONFIG_HTE) &&
+	    (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE))
+		return -EOPNOTSUPP;
+
 	/*
 	 * Do not allow both INPUT and OUTPUT flags to be set as they are
 	 * contradictory.
@@ -1058,7 +1088,8 @@ static int gpio_v2_line_flags_validate(u64 flags)
 		return -EINVAL;
 
 	/* Only allow one event clock source */
-	if ((flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME) &&
+	if (IS_ENABLED(CONFIG_HTE) &&
+	    (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME) &&
 	    (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE))
 		return -EINVAL;
 
-- 
2.37.0

Re: [PATCH 4/6] gpiolib: cdev: simplify line event identification

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Reorganise line event identification code to reduce code duplication,
> and replace if-else initializers with the ternary equivalent to
> improve readability.

...

> +               le.id = level ? GPIO_V2_LINE_EVENT_RISING_EDGE :
> +                               GPIO_V2_LINE_EVENT_FALLING_EDGE;

It seems several times you are doing the same, perhaps a helper?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> The majority of builds do not include HTE, so compile out hte
> functionality unless CONFIG_HTE is selected.

...

> +#ifdef CONFIG_HTE
>         /*
>          * -- hte specific fields --
>          */

Now this comment seems useless to me and it takes 3 LoCs.

...

> +       else if (IS_ENABLED(CONFIG_HTE) &&
> +                (test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)))

Too many parentheses.

...

> +               if (!IS_ENABLED(CONFIG_HTE) ||
> +                   !test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)) {

if (!(x && y)) ?

...

> +       if (!IS_ENABLED(CONFIG_HTE) &&
> +           (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE))
> +               return -EOPNOTSUPP;

Ditto for consistency?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Combine the polarity_change flag, struct line eflags, and hte enable
> flag into a single flag variable.
>
> The combination of these flags describes the configuration state
> of the edge detector, so formalize and clarify that by combining
> them into a single variable, edflags, in struct line.
>
> The edflags is a subset of the GPIO_V2_LINE_FLAGsb relevant to
> the edge detector, and is also a superset of the eflags it replaces.
> The eflags name is still used to describe the subset of edflags
> corresponding to the rising/falling edge flags where edflags is
> masked down to that subset.
>
> This consolidation reduces the number of variables being passed,
> simplifies state comparisons, and provides a more extensible
> foundation should additional edge sources be integrated in the
> future.

I believe that you have checked this from a locking perspective, so we
won't have worse lock contamination, if any.

...

>         struct linereq *lr;
>         struct gpio_v2_line_event le;
>         int level;
> -       u64 eflags;
> +       u64 edflags;

I would at the same time move it up before `int level;`.

...

> +       int level = -1, diff_seqno;
> +       u64 eflags, edflags = READ_ONCE(line->edflags);

Ditto.

...

>         u32 debounce_period_us;
>         unsigned long irqflags = 0;
>         int irq, ret;
> +       u64 eflags;

Ditto for similarity.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 3:38 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> This patch series is a collection of improvements to simplify the
> code, improve readability, and compile out unused code.
> There are no functional changes.
>
> The first patch is a cleanup for my recent linereq_free() fix. I
> noted then that the edge_detector_stop() could probably be safely
> moved inside the line desc check block, but wanted to keep that
> change minimal just in case.  It can be safely moved, and so here
> it is.
>
> Patch 2 makes use of an existing macro to simplify a call.
>
> Patch 3 replaces some more if-else chains with switches, which is
> more readable (YMMV).
>
> Patch 4 reorganizes the line identification code to share code
> common to alternate paths.
>
> Patch 5 consolidates a number of separate flags into one.  This
> reduces code complexity, simplifies any future edge source additions,
> and makes patch 6 significantly simpler.
>
> Patch 6 totally compiles out the hte specific code when CONFIG_HTE
> is not selected.
>
> I've based this series on gpio/for-current, as it requires the fix
> patch -
> commit c8e27a4a5136 ("gpiolib: cdev: fix null pointer dereference in linereq_free()")
> Happy to rebase if that doesn't suit.
>
> Dipen, I don't have any HTE compatible hardware to test with, so
> could you check that this still works for you?

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
for uncommented patches.

You may also use it if you are going to address comments as suggested.

> Kent Gibson (6):
>   gpiolib: cdev: simplify linereq_free
>   gpiolib: cdev: simplify parameter in call to hte_edge_setup
>   gpiolib: cdev: replace if-else chains with switches
>   gpiolib: cdev: simplify line event identification
>   gpiolib: cdev: consolidate edge detector configuration flags
>   gpiolib: cdev: compile out HTE unless CONFIG_HTE selected
>
>  drivers/gpio/gpiolib-cdev.c | 286 +++++++++++++++++++-----------------
>  1 file changed, 149 insertions(+), 137 deletions(-)
>
>
> base-commit: 7329b071729645e243b6207e76bca2f4951c991b
> --
> 2.37.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags

[Kent Gibson]

On Wed, Jul 13, 2022 at 12:07:29PM +0200, Andy Shevchenko wrote:
> On Wed, Jul 13, 2022 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > Combine the polarity_change flag, struct line eflags, and hte enable
> > flag into a single flag variable.
> >
> > The combination of these flags describes the configuration state
> > of the edge detector, so formalize and clarify that by combining
> > them into a single variable, edflags, in struct line.
> >
> > The edflags is a subset of the GPIO_V2_LINE_FLAGsb relevant to
> > the edge detector, and is also a superset of the eflags it replaces.
> > The eflags name is still used to describe the subset of edflags
> > corresponding to the rising/falling edge flags where edflags is
> > masked down to that subset.
> >
> > This consolidation reduces the number of variables being passed,
> > simplifies state comparisons, and provides a more extensible
> > foundation should additional edge sources be integrated in the
> > future.
> 
> I believe that you have checked this from a locking perspective, so we
> won't have worse lock contamination, if any.
> 

Yeah, they are used in the same way as the old eflags, so there is no
change in that regard.

> ...
> 
> >         struct linereq *lr;
> >         struct gpio_v2_line_event le;
> >         int level;
> > -       u64 eflags;
> > +       u64 edflags;
> 
> I would at the same time move it up before `int level;`.
> 

Ok.  What is the general rule you want applied, cos I'm not seeing it.

Cheers,
Kent.

> ...
> 
> > +       int level = -1, diff_seqno;
> > +       u64 eflags, edflags = READ_ONCE(line->edflags);
> 
> Ditto.
> 
> ...
> 
> >         u32 debounce_period_us;
> >         unsigned long irqflags = 0;
> >         int irq, ret;
> > +       u64 eflags;
> 
> Ditto for similarity.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 4/6] gpiolib: cdev: simplify line event identification

[Kent Gibson]

On Wed, Jul 13, 2022 at 11:59:10AM +0200, Andy Shevchenko wrote:
> On Wed, Jul 13, 2022 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > Reorganise line event identification code to reduce code duplication,
> > and replace if-else initializers with the ternary equivalent to
> > improve readability.
> 
> ...
> 
> > +               le.id = level ? GPIO_V2_LINE_EVENT_RISING_EDGE :
> > +                               GPIO_V2_LINE_EVENT_FALLING_EDGE;
> 
> It seems several times you are doing the same, perhaps a helper?
> 

If by several times you mean twice, then yeah.
Not sure that reaches the threshold for a helper though.

Cheers,
Kent.

Re: [PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected

[Kent Gibson]

On Wed, Jul 13, 2022 at 12:03:07PM +0200, Andy Shevchenko wrote:
> On Wed, Jul 13, 2022 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > The majority of builds do not include HTE, so compile out hte
> > functionality unless CONFIG_HTE is selected.
> 
> ...
> 
> > +#ifdef CONFIG_HTE
> >         /*
> >          * -- hte specific fields --
> >          */
> 
> Now this comment seems useless to me and it takes 3 LoCs.
> 
> ...
> 
> > +       else if (IS_ENABLED(CONFIG_HTE) &&
> > +                (test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)))
> 
> Too many parentheses.
> 
> ...
> 
> > +               if (!IS_ENABLED(CONFIG_HTE) ||
> > +                   !test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)) {
> 
> if (!(x && y)) ?
> 
> ...
> 
> > +       if (!IS_ENABLED(CONFIG_HTE) &&
> > +           (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE))
> > +               return -EOPNOTSUPP;
> 
> Ditto for consistency?
> 

Those all make sense - will do.

Thanks for the prompt review.

Cheers,
Kent.

Re: [PATCH 4/6] gpiolib: cdev: simplify line event identification

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 12:27 PM Kent Gibson <warthog618@gmail.com> wrote:
> On Wed, Jul 13, 2022 at 11:59:10AM +0200, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote:

...

> > > +               le.id = level ? GPIO_V2_LINE_EVENT_RISING_EDGE :
> > > +                               GPIO_V2_LINE_EVENT_FALLING_EDGE;
> >
> > It seems several times you are doing the same, perhaps a helper?
>
> If by several times you mean twice, then yeah.
> Not sure that reaches the threshold for a helper though.

Up to you, then!


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 12:25 PM Kent Gibson <warthog618@gmail.com> wrote:
> On Wed, Jul 13, 2022 at 12:07:29PM +0200, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote:

...

> > >         struct linereq *lr;
> > >         struct gpio_v2_line_event le;
> > >         int level;
> > > -       u64 eflags;
> > > +       u64 edflags;
> >
> > I would at the same time move it up before `int level;`.
>
> Ok.  What is the general rule you want applied, cos I'm not seeing it.

Common sense.
But here are two informal recommendations:
1) longer line first;
2) you may still group by struct, POD or other flavours (see below).

...

> > > +       int level = -1, diff_seqno;
> > > +       u64 eflags, edflags = READ_ONCE(line->edflags);
> >
> > Ditto.

Here is obviously the longer line case.

...

> > >         u32 debounce_period_us;
> > >         unsigned long irqflags = 0;
> > >         int irq, ret;
> > > +       u64 eflags;
> >
> > Ditto for similarity.

Here and in other cases above, swapping them won't interleave the
types grouping (like int-u64-int). But in some cases it's allowed when
you put the last definition in the block the definition of the
variable that keeps the returned value.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/6] gpiolib: cdev: simplify line event identification

[Kent Gibson]

On Wed, Jul 13, 2022 at 01:24:33PM +0200, Andy Shevchenko wrote:
> On Wed, Jul 13, 2022 at 12:27 PM Kent Gibson <warthog618@gmail.com> wrote:
> > On Wed, Jul 13, 2022 at 11:59:10AM +0200, Andy Shevchenko wrote:
> > > On Wed, Jul 13, 2022 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote:
> 
> ...
> 
> > > > +               le.id = level ? GPIO_V2_LINE_EVENT_RISING_EDGE :
> > > > +                               GPIO_V2_LINE_EVENT_FALLING_EDGE;
> > >
> > > It seems several times you are doing the same, perhaps a helper?
> >
> > If by several times you mean twice, then yeah.
> > Not sure that reaches the threshold for a helper though.
> 
> Up to you, then!
> 

Turns out there are three instances, so a helper it is.

Cheers,
Kent.

Re: [PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected

[Kent Gibson]

On Wed, Jul 13, 2022 at 06:30:14PM +0800, Kent Gibson wrote:
> On Wed, Jul 13, 2022 at 12:03:07PM +0200, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > The majority of builds do not include HTE, so compile out hte
> > > functionality unless CONFIG_HTE is selected.
> > 
> > ...
> > 
> > > +#ifdef CONFIG_HTE
> > >         /*
> > >          * -- hte specific fields --
> > >          */
> > 
> > Now this comment seems useless to me and it takes 3 LoCs.
> > 
> > ...
> > 
> > > +       else if (IS_ENABLED(CONFIG_HTE) &&
> > > +                (test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)))
> > 
> > Too many parentheses.
> > 
> > ...
> > 
> > > +               if (!IS_ENABLED(CONFIG_HTE) ||
> > > +                   !test_bit(FLAG_EVENT_CLOCK_HTE, &line->desc->flags)) {
> > 
> > if (!(x && y)) ?
> > 
> > ...
> > 
> > > +       if (!IS_ENABLED(CONFIG_HTE) &&
> > > +           (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE))
> > > +               return -EOPNOTSUPP;
> > 
> > Ditto for consistency?
> > 

On second thought, that last one becomes:

	if (!(IS_ENABLED(CONFIG_HTE) ||
	      !(flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE))
		return -EOPNOTSUPP;

which is MUCH less readable, so I'll leave that one as is.

Cheers,
Kent.

Re: [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration

[Dipen Patel]

On 7/12/22 6:37 PM, Kent Gibson wrote:
> This patch series is a collection of improvements to simplify the
> code, improve readability, and compile out unused code.
> There are no functional changes.
>
> The first patch is a cleanup for my recent linereq_free() fix. I
> noted then that the edge_detector_stop() could probably be safely
> moved inside the line desc check block, but wanted to keep that
> change minimal just in case.  It can be safely moved, and so here
> it is.
>
> Patch 2 makes use of an existing macro to simplify a call.
>
> Patch 3 replaces some more if-else chains with switches, which is
> more readable (YMMV).
>
> Patch 4 reorganizes the line identification code to share code
> common to alternate paths.
>
> Patch 5 consolidates a number of separate flags into one.  This
> reduces code complexity, simplifies any future edge source additions,
> and makes patch 6 significantly simpler.
>
> Patch 6 totally compiles out the hte specific code when CONFIG_HTE
> is not selected.
>
> I've based this series on gpio/for-current, as it requires the fix
> patch -
> commit c8e27a4a5136 ("gpiolib: cdev: fix null pointer dereference in linereq_free()")
> Happy to rebase if that doesn't suit.
>
> Dipen, I don't have any HTE compatible hardware to test with, so
> could you check that this still works for you?
Sure, will do it by Tuesday next week.
>
>
> Kent Gibson (6):
>   gpiolib: cdev: simplify linereq_free
>   gpiolib: cdev: simplify parameter in call to hte_edge_setup
>   gpiolib: cdev: replace if-else chains with switches
>   gpiolib: cdev: simplify line event identification
>   gpiolib: cdev: consolidate edge detector configuration flags
>   gpiolib: cdev: compile out HTE unless CONFIG_HTE selected
>
>  drivers/gpio/gpiolib-cdev.c | 286 +++++++++++++++++++-----------------
>  1 file changed, 149 insertions(+), 137 deletions(-)
>
>
> base-commit: 7329b071729645e243b6207e76bca2f4951c991b

Re: [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration

[Kent Gibson]

On Thu, Jul 14, 2022 at 07:06:18PM -0700, Dipen Patel wrote:
> On 7/12/22 6:37 PM, Kent Gibson wrote:
> >
> > I've based this series on gpio/for-current, as it requires the fix
> > patch -
> > commit c8e27a4a5136 ("gpiolib: cdev: fix null pointer dereference in linereq_free()")
> > Happy to rebase if that doesn't suit.
> >
> > Dipen, I don't have any HTE compatible hardware to test with, so
> > could you check that this still works for you?
> Sure, will do it by Tuesday next week.

Ideally with v2 of the series.

Thanks!

Kent.

Re: [PATCH 1/6] gpiolib: cdev: simplify linereq_free

[Linus Walleij]

On Wed, Jul 13, 2022 at 3:37 AM Kent Gibson <warthog618@gmail.com> wrote:

> The edge detector is only ever started after the line desc has been
> determined, so move edge_detector_stop() inside the line desc check,
> and merge the two checked regions into one.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij