[PATCH net-next] ieee802154: use helper function to get rid of redundancy

In some call sites of at86rf230_write_subreg(), they are rather
redundant. Use a helper function to replace those call sites for
efficiency.

With at86rf230_write_subreg_by_page(), now ->rssi_base_val is
assigned before at86rf230_write_subreg() is evaluated. Looking at
at86rf230_write_subreg(), that change could possibly affect

		__at86rf230_read_subreg()
and
		__at86rf230_write()

only or vice versa. Yet by the definition of

		struct at86rf230_local

changing ->rssi_base_val will in no way affect those two functions
above and evaluation of those two has no impact on ->rssi_base_val.
Thus, at86rf230_write_subreg_by_page() itself is safe.

Additionally at86rf230_write_subreg_by_channel() is simple and safe.

Signed-off-by: Jean Sacren <redacted>
---
 drivers/net/ieee802154/at86rf230.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 4517b149ed07..5aecbd1c402c 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c

@@ -597,25 +597,37 @@ at86rf230_set_channel(struct at86rf230_local *lp, int page, int channel)
 	return at86rf230_write_subreg(lp, SR_CHANNEL, channel);
 }
 
+static int at86rf230_write_subreg_by_channel(struct at86rf230_local *lp,
+					     int channel)
+{
+	if (channel)
+		channel = 1;
+
+	return at86rf230_write_subreg(lp, SR_SUB_MODE, channel);
+}
+
+static int at86rf230_write_subreg_by_page(struct at86rf230_local *lp, int page)
+{
+	if (page) {
+		lp->rssi_base_val = -98;
+		page = 1;
+	} else {
+		lp->rssi_base_val = -100;
+	}
+
+	return at86rf230_write_subreg(lp, SR_BPSK_QPSK, page);
+}
+
 static int
 at86rf212_set_channel(struct at86rf230_local *lp, int page, int channel)
 {
 	int rc;
 
-	if (channel == 0)
-		rc = at86rf230_write_subreg(lp, SR_SUB_MODE, 0);
-	else
-		rc = at86rf230_write_subreg(lp, SR_SUB_MODE, 1);
+	rc = at86rf230_write_subreg_by_channel(lp, channel);
 	if (rc < 0)
 		return rc;
 
-	if (page == 0) {
-		rc = at86rf230_write_subreg(lp, SR_BPSK_QPSK, 0);
-		lp->rssi_base_val = -100;
-	} else {
-		rc = at86rf230_write_subreg(lp, SR_BPSK_QPSK, 1);
-		lp->rssi_base_val = -98;
-	}
+	rc = at86rf230_write_subreg_by_page(lp, page);
 	if (rc < 0)
 		return rc;
 

Hi Jean,

the at86rf230 driver supports several at86rf2xx chips. You split the
at86rf212_set_channel which is at86rf212 specific in two function which
are named at86rf230_foo.

Sorry, but I think we should not do this. One reason is that the code is
much easier to read when we have one channel_set callback for at86rf23x
and at86rf212 chips.



Btw. I preparing a big cleanup for this driver which supports regmap and
asychronous handling for rx/tx paths.

- Alex

From: Alexander Aring <alex.aring@gmail.com>
Date: Sun, 01 Jun 2014 09:26:57 +0200

Hi Alex,

Thank you very much for the feedback.

the at86rf230 driver supports several at86rf2xx chips. You split the
at86rf212_set_channel which is at86rf212 specific in two function which
are named at86rf230_foo.

I didn't "split" at86rf212_set_channel() in two functions. I spliced
those two sections of code and made at86rf212_set_channel() far
succinct and easy to read.

Sorry, but I think we should not do this. One reason is that the code is
much easier to read when we have one channel_set callback for at86rf23x
and at86rf212 chips.

If you use one channel_set callback as before the change, how would you
overcome the redundancy?

Btw. I preparing a big cleanup for this driver which supports regmap and
asychronous handling for rx/tx paths.

Thank you for letting me know. I'm sure I will learn a lot.