gpiolib: cdev: code cleanup following hte integration

---
 drivers/gpio/gpiolib-cdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/gpio/gpiolib-cdev.c +4 -4

index 0c9a63becfef..b44526e3630e 100644
@@ -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);

Signature · 1 line

2.37.1

Dipen Patel

Acked-by on v2 1/6

2022-07-15 05:09 UTC #2 lore raw

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

Quoted 24 lines · 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 <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>
---
 drivers/gpio/gpiolib-cdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpio/gpiolib-cdev.c +2 -2
index 0c9a63becfef..b44526e3630e 100644
@@ -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) {

drivers/gpio/gpiolib-cdev.c static ssize_t linereq_read(struct file *file, · line 1474

Quoted 10 lines

drivers/gpio/gpiolib-cdev.c +2 -2

 			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);

Acked-by Dipen Patel <[email protected]>

Linus Walleij

Reviewed-by on v2 1/6

2022-07-18 09:28 UTC #3 lore raw

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

Quoted 6 lines · 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 <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>

Ooops reviewed v1 because of too deep mailbox.

Yours,
Linus Walleij

Kent Gibson

author reviewer

2022-07-14 02:04 UTC #4 lore raw

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

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

---
 drivers/gpio/gpiolib-cdev.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/gpio/gpiolib-cdev.c +1 -3

index b44526e3630e..f635bbbb6a6d 100644
@@ -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;
 		}

Signature · 1 line

2.37.1

Dipen Patel

Acked-by on v2 2/6

2022-07-15 05:10 UTC #5 lore raw

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

Quoted 14 lines · 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 <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>
---
 drivers/gpio/gpiolib-cdev.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/gpio/gpiolib-cdev.c +0 -0
index b44526e3630e..f635bbbb6a6d 100644

drivers/gpio/gpiolib-cdev.c static int debounce_setup(struct line *line, · line 891

Quoted 10 lines

drivers/gpio/gpiolib-cdev.c +1 -3

@@ -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;
 		}

Acked-by Dipen Patel <[email protected]>

Linus Walleij

Reviewed-by on v2 2/6

2022-07-18 09:29 UTC #6 lore raw

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

Quoted 5 lines · 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 <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>

Yours,
Linus Walleij

Kent Gibson

author reviewer

2022-07-14 02:04 UTC #7 lore raw

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

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

Signed-off-by Kent Gibson <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

drivers/gpio/gpiolib-cdev.c +18 ~~-13~~

index f635bbbb6a6d..bc7c8822ede0 100644
@@ -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++;

Signature · 1 line

2.37.1

Linus Walleij

Reviewed-by on v2 3/6

2022-07-18 09:31 UTC #8 lore raw

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

Quoted 5 lines · Kent Gibson

Improve readability by replacing if-else chains with switch statements.
Signed-off-by Kent Gibson <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>

Yep that is more readable with the switch syntactic sugar on top:

Yours,
Linus Walleij

Kent Gibson

author reviewer

2022-07-14 02:04 UTC #9 lore raw

PATCH v2 4/6 gpiolib: cdev: simplify line event identification

Reorganise line event identification code to reduce code duplication, and replace if-else initializers with a helper function to improve readability.

Signed-off-by Kent Gibson <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 44 +++++++++++++------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

drivers/gpio/gpiolib-cdev.c +15 ~~-29~~

index bc7c8822ede0..5765379f4b54 100644
@@ -569,6 +569,12 @@ static u64 line_event_timestamp(struct line *line)
 	return ktime_get_ns();
 }
 
+static u32 line_event_id(int level)
+{
+	return level ? GPIO_V2_LINE_EVENT_RISING_EDGE :
+		       GPIO_V2_LINE_EVENT_FALLING_EDGE;
+}
+
 static enum hte_return process_hw_ts_thread(void *p)
 {
 	struct line *line;

@@ -590,26 +596,19 @@ 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 = line_event_id(level);
 		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 +683,12 @@ 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 = line_event_id(gpiod_get_value_cansleep(line->desc));
 		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 +812,7 @@ 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 = line_event_id(level);
 
 	linereq_put_event(lr, &le);
 }

Signature · 1 line

2.37.1

Linus Walleij

Reviewed-by on v2 4/6

2022-07-18 09:31 UTC #10 lore raw

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

Quoted 6 lines · Kent Gibson

Reorganise line event identification code to reduce code duplication, and replace if-else initializers with a helper function to improve readability.
Signed-off-by Kent Gibson <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>

Yep more readable.

Yours,
Linus Walleij

Kent Gibson

author reviewer

2022-07-14 02:04 UTC #11 lore raw

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

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

---
 drivers/gpio/gpiolib-cdev.c | 126 +++++++++++++++++-------------------
 1 file changed, 60 insertions(+), 66 deletions(-)

drivers/gpio/gpiolib-cdev.c +60 ~~-66~~

index 5765379f4b54..01c76aa00701 100644
@@ -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)
 {

@@ -580,8 +589,8 @@ static enum hte_return process_hw_ts_thread(void *p)
 	struct line *line;
 	struct linereq *lr;
 	struct gpio_v2_line_event le;
+	u64 edflags;
 	int level;
-	u64 eflags;
 
 	if (!p)
 		return HTE_CB_HANDLED;

@@ -592,15 +601,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 = line_event_id(level);

@@ -681,7 +690,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 = line_event_id(gpiod_get_value_cansleep(line->desc));
 		break;

@@ -756,16 +765,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;
+	u64 eflags, edflags = READ_ONCE(line->edflags);
+	int level = -1, diff_seqno;
 
-	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;

@@ -777,12 +783,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 */

@@ -796,7 +802,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 -

@@ -843,8 +849,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;

@@ -864,7 +869,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;

@@ -915,19 +920,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() */

@@ -935,23 +940,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;
+	u64 eflags;
 	int irq, ret;
 
+	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);

@@ -961,8 +966,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)

@@ -988,35 +993,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,

@@ -1285,22 +1284,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".

@@ -1308,7 +1302,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;

@@ -1317,12 +1311,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);

@@ -1449,13 +1444,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);
 		}
 	}

@@ -1491,7 +1483,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;

@@ -1565,6 +1557,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".

@@ -1581,12 +1574,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);

Signature · 1 line

2.37.1

Linus Walleij

Reviewed-by on v2 5/6

2022-07-18 09:34 UTC #12 lore raw

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

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

What is that "b" at the end of GPIO_V2_LINE_FLAGsb?
Oh well no big deal. Bart can fix when applying if it is disturbing.

Quoted 5 lines

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.

Quoted 7 lines

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 <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>

Yours,
Linus Walleij

Kent Gibson

author reviewer

2022-07-18 09:46 UTC #13 lore raw

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

Quoted 5 lines · Linus Walleij

Quoted 3 lines · Kent Gibson
Combine the polarity_change flag, struct line eflags, and hte enable flag into a single flag variable.

Quoted 9 lines

Quoted 5 lines
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
What is that "b" at the end of GPIO_V2_LINE_FLAGsb?
Oh well no big deal. Bart can fix when applying if it is disturbing.

Yeah, typo or something - no idea what that is doing there, but it has been there since v1. ¯\_(ツ)_/¯
Bart - please do fix that if you don't mind.

Thanks for the review.

Cheers,
Kent.

Kent Gibson

author reviewer

2022-07-14 02:04 UTC #14 lore raw

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

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

Signed-off-by Kent Gibson <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 98 ++++++++++++++++++++++++-------------
 1 file changed, 63 insertions(+), 35 deletions(-)

drivers/gpio/gpiolib-cdev.c +63 ~~-35~~

index 01c76aa00701..7e4670a286b2 100644
@@ -468,9 +468,7 @@ struct line {
 	 * stale value.
 	 */
 	unsigned int level;
-	/*
-	 * -- hte specific fields --
-	 */
+#ifdef CONFIG_HTE
 	struct hte_ts_desc hdesc;
 	/*
 	 * HTE provider sets line level at the time of event. The valid

@@ -487,6 +485,7 @@ struct line {
 	 * last sequence number before debounce period expires.
 	 */
 	u32 last_seqno;
+#endif /* CONFIG_HTE */
 };
 
 /**

@@ -572,7 +571,8 @@ 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();

@@ -584,6 +584,8 @@ static u32 line_event_id(int level)
 		       GPIO_V2_LINE_EVENT_FALLING_EDGE;
 }
 
+#ifdef CONFIG_HTE
+
 static enum hte_return process_hw_ts_thread(void *p)
 {
 	struct line *line;

@@ -667,6 +669,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;

@@ -766,10 +804,13 @@ static void debounce_work_func(struct work_struct *work)
 	struct line *line = container_of(work, struct line, work.work);
 	struct linereq *lr;
 	u64 eflags, edflags = READ_ONCE(line->edflags);
-	int level = -1, diff_seqno;
+	int level = -1;
+#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) {

@@ -802,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;

@@ -811,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) ?

@@ -823,32 +867,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;

@@ -869,7 +887,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;

@@ -927,8 +946,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);

@@ -966,7 +987,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);

@@ -1051,6 +1073,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.

@@ -1060,7 +1087,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;

Signature · 1 line

2.37.1

Linus Walleij

Reviewed-by on v2 6/6

2022-07-18 09:35 UTC #15 lore raw

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

Quoted 5 lines · 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 <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>

That's helpful.

Yours,
Linus Walleij

Dipen Patel

Tested-by on the v2 series

2022-07-18 21:32 UTC #16 lore raw

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

Quoted 7 lines

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?

Only hte logic

Tested-by Dipen Patel <[email protected]>

Quoted 20 lines

Changes v1 -> v2:
Address Andy's review comments, specifically
- Patch 4 move ternary initializer into a helper function.
- Patch 5 variable declaration ordering.
- Patch 6 remove obsoleted comment and tidy some if expressions.
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 | 291 +++++++++++++++++++-----------------
 1 file changed, 151 insertions(+), 140 deletions(-)
base-commit: 7329b071729645e243b6207e76bca2f4951c991b

Bartosz Golaszewski

2022-07-19 08:03 UTC #17 lore raw