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?
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
--
2.37.1
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]>
---
drivers/gpio/gpiolib-cdev.c | 98 ++++++++++++++++++++++++-------------
1 file changed, 63 insertions(+), 35 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 01c76aa00701..7e4670a286b2 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -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;
--
2.37.1
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(-)
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.1
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]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
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 5765379f4b54..01c76aa00701 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)
{
@@ -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);
--
2.37.1
On 7/13/22 7:03 PM, Kent Gibson 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 <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
> 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);
Acked-by: Dipen Patel <[email protected]>
On Thu, Jul 14, 2022 at 4:03 AM Kent Gibson <[email protected]> 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 <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
Ooops reviewed v1 because of too deep mailbox.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Jul 14, 2022 at 4:04 AM Kent Gibson <[email protected]> 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
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.
> 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]>
> Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Mon, Jul 18, 2022 at 11:33:48AM +0200, Linus Walleij wrote:
> On Thu, Jul 14, 2022 at 4:04 AM Kent Gibson <[email protected]> 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
>
> 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.
On Thu, Jul 14, 2022 at 4:04 AM Kent Gibson <[email protected]> wrote:
> 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.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On 7/13/22 7:03 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?
Only hte logic
Tested-by: Dipen Patel <[email protected]>
>
> 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
On Thu, Jul 14, 2022 at 4:03 AM Kent Gibson <[email protected]> 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?
>
> 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
> --
> 2.37.1
>
Excellent work as usual, all applied.
Bart