* [PATCH 1/6] gpiolib: cdev: simplify linereq_free
2022-07-13 1:37 [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration Kent Gibson
@ 2022-07-13 1:37 ` Kent Gibson
2022-07-18 9:27 ` Linus Walleij
2022-07-13 1:37 ` [PATCH 2/6] gpiolib: cdev: simplify parameter in call to hte_edge_setup Kent Gibson
` (6 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Kent Gibson @ 2022-07-13 1:37 UTC (permalink / raw)
To: linux-kernel, linux-gpio, brgl, linus.walleij, dipenp; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 2/6] gpiolib: cdev: simplify parameter in call to hte_edge_setup
2022-07-13 1:37 [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration Kent Gibson
2022-07-13 1:37 ` [PATCH 1/6] gpiolib: cdev: simplify linereq_free Kent Gibson
@ 2022-07-13 1:37 ` Kent Gibson
2022-07-13 1:37 ` [PATCH 3/6] gpiolib: cdev: replace if-else chains with switches Kent Gibson
` (5 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Kent Gibson @ 2022-07-13 1:37 UTC (permalink / raw)
To: linux-kernel, linux-gpio, brgl, linus.walleij, dipenp; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 3/6] gpiolib: cdev: replace if-else chains with switches
2022-07-13 1:37 [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration Kent Gibson
2022-07-13 1:37 ` [PATCH 1/6] gpiolib: cdev: simplify linereq_free Kent Gibson
2022-07-13 1:37 ` [PATCH 2/6] gpiolib: cdev: simplify parameter in call to hte_edge_setup Kent Gibson
@ 2022-07-13 1:37 ` Kent Gibson
2022-07-13 1:37 ` [PATCH 4/6] gpiolib: cdev: simplify line event identification Kent Gibson
` (4 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Kent Gibson @ 2022-07-13 1:37 UTC (permalink / raw)
To: linux-kernel, linux-gpio, brgl, linus.walleij, dipenp; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 4/6] gpiolib: cdev: simplify line event identification
2022-07-13 1:37 [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration Kent Gibson
` (2 preceding siblings ...)
2022-07-13 1:37 ` [PATCH 3/6] gpiolib: cdev: replace if-else chains with switches Kent Gibson
@ 2022-07-13 1:37 ` Kent Gibson
2022-07-13 9:59 ` Andy Shevchenko
2022-07-13 1:37 ` [PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags Kent Gibson
` (3 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Kent Gibson @ 2022-07-13 1:37 UTC (permalink / raw)
To: linux-kernel, linux-gpio, brgl, linus.walleij, dipenp; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/6] gpiolib: cdev: simplify line event identification
2022-07-13 1:37 ` [PATCH 4/6] gpiolib: cdev: simplify line event identification Kent Gibson
@ 2022-07-13 9:59 ` Andy Shevchenko
2022-07-13 10:27 ` Kent Gibson
0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2022-07-13 9:59 UTC (permalink / raw)
To: Kent Gibson
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/6] gpiolib: cdev: simplify line event identification
2022-07-13 9:59 ` Andy Shevchenko
@ 2022-07-13 10:27 ` Kent Gibson
2022-07-13 11:24 ` Andy Shevchenko
0 siblings, 1 reply; 21+ messages in thread
From: Kent Gibson @ 2022-07-13 10:27 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/6] gpiolib: cdev: simplify line event identification
2022-07-13 10:27 ` Kent Gibson
@ 2022-07-13 11:24 ` Andy Shevchenko
2022-07-14 0:32 ` Kent Gibson
0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2022-07-13 11:24 UTC (permalink / raw)
To: Kent Gibson
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/6] gpiolib: cdev: simplify line event identification
2022-07-13 11:24 ` Andy Shevchenko
@ 2022-07-14 0:32 ` Kent Gibson
0 siblings, 0 replies; 21+ messages in thread
From: Kent Gibson @ 2022-07-14 0:32 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags
2022-07-13 1:37 [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration Kent Gibson
` (3 preceding siblings ...)
2022-07-13 1:37 ` [PATCH 4/6] gpiolib: cdev: simplify line event identification Kent Gibson
@ 2022-07-13 1:37 ` Kent Gibson
2022-07-13 10:07 ` Andy Shevchenko
2022-07-13 1:37 ` [PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected Kent Gibson
` (2 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Kent Gibson @ 2022-07-13 1:37 UTC (permalink / raw)
To: linux-kernel, linux-gpio, brgl, linus.walleij, dipenp; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags
2022-07-13 1:37 ` [PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags Kent Gibson
@ 2022-07-13 10:07 ` Andy Shevchenko
2022-07-13 10:24 ` Kent Gibson
0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2022-07-13 10:07 UTC (permalink / raw)
To: Kent Gibson
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags
2022-07-13 10:07 ` Andy Shevchenko
@ 2022-07-13 10:24 ` Kent Gibson
2022-07-13 11:29 ` Andy Shevchenko
0 siblings, 1 reply; 21+ messages in thread
From: Kent Gibson @ 2022-07-13 10:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags
2022-07-13 10:24 ` Kent Gibson
@ 2022-07-13 11:29 ` Andy Shevchenko
0 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2022-07-13 11:29 UTC (permalink / raw)
To: Kent Gibson
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected
2022-07-13 1:37 [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration Kent Gibson
` (4 preceding siblings ...)
2022-07-13 1:37 ` [PATCH 5/6] gpiolib: cdev: consolidate edge detector configuration flags Kent Gibson
@ 2022-07-13 1:37 ` Kent Gibson
2022-07-13 10:03 ` Andy Shevchenko
2022-07-13 10:08 ` [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration Andy Shevchenko
2022-07-15 2:06 ` Dipen Patel
7 siblings, 1 reply; 21+ messages in thread
From: Kent Gibson @ 2022-07-13 1:37 UTC (permalink / raw)
To: linux-kernel, linux-gpio, brgl, linus.walleij, dipenp; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected
2022-07-13 1:37 ` [PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected Kent Gibson
@ 2022-07-13 10:03 ` Andy Shevchenko
2022-07-13 10:30 ` Kent Gibson
0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2022-07-13 10:03 UTC (permalink / raw)
To: Kent Gibson
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected
2022-07-13 10:03 ` Andy Shevchenko
@ 2022-07-13 10:30 ` Kent Gibson
2022-07-14 1:08 ` Kent Gibson
0 siblings, 1 reply; 21+ messages in thread
From: Kent Gibson @ 2022-07-13 10:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected
2022-07-13 10:30 ` Kent Gibson
@ 2022-07-14 1:08 ` Kent Gibson
0 siblings, 0 replies; 21+ messages in thread
From: Kent Gibson @ 2022-07-14 1:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration
2022-07-13 1:37 [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration Kent Gibson
` (5 preceding siblings ...)
2022-07-13 1:37 ` [PATCH 6/6] gpiolib: cdev: compile out HTE unless CONFIG_HTE selected Kent Gibson
@ 2022-07-13 10:08 ` Andy Shevchenko
2022-07-15 2:06 ` Dipen Patel
7 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2022-07-13 10:08 UTC (permalink / raw)
To: Kent Gibson
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Linus Walleij, Dipen Patel
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration
2022-07-13 1:37 [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration Kent Gibson
` (6 preceding siblings ...)
2022-07-13 10:08 ` [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration Andy Shevchenko
@ 2022-07-15 2:06 ` Dipen Patel
2022-07-15 2:08 ` Kent Gibson
7 siblings, 1 reply; 21+ messages in thread
From: Dipen Patel @ 2022-07-15 2:06 UTC (permalink / raw)
To: Kent Gibson, linux-kernel, linux-gpio, brgl, linus.walleij
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 0/6] gpiolib: cdev: code cleanup following hte integration
2022-07-15 2:06 ` Dipen Patel
@ 2022-07-15 2:08 ` Kent Gibson
0 siblings, 0 replies; 21+ messages in thread
From: Kent Gibson @ 2022-07-15 2:08 UTC (permalink / raw)
To: Dipen Patel; +Cc: linux-kernel, linux-gpio, brgl, linus.walleij
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.
^ permalink raw reply [flat|nested] 21+ messages in thread