diff mbox series

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

Message ID 20220713013721.68879-7-warthog618@gmail.com
State New
Headers show
Series gpiolib: cdev: code cleanup following hte integration | expand

Commit Message

Kent Gibson July 13, 2022, 1:37 a.m. UTC 
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(-)

Comments

Andy Shevchenko July 13, 2022, 10:03 a.m. UTC | #1 
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?
Kent Gibson July 13, 2022, 10:30 a.m. UTC | #2 
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.
Kent Gibson July 14, 2022, 1:08 a.m. UTC | #3 
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.
diff mbox series

Patch

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;