Skip to contentSkip to conversation

media: imon_raw: simplify and explain bit operations

PATCH In review Sean Young 2 participants 5 messages Aug 10, 2019 – Aug 13, 2019

Subsystems (4)
Media (V4L/DVB) Kselftest RC-core / lirc framework Imon soundgraph USB IR receiver
lore
Share & tools
lore

Sean Young

author maintainer
#0 lore raw

PATCH 1/3 media: imon_raw: simplify and explain bit operations

This code needs some explanation.

Signed-off-by Sean Young <[email protected]>
---
 drivers/media/rc/imon_raw.c | 43 +++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 9 deletions(-)
drivers/media/rc/imon_raw.c +34 -9
index 25e56c5b13c0..e6723993b466 100644
@@ -14,7 +14,7 @@ struct imon {
 	struct device *dev;
 	struct urb *ir_urb;
 	struct rc_dev *rcdev;
-	u8 ir_buf[8] __aligned(__alignof__(u64));
+	__be64 ir_buf;
 	char phys[64];
 };
 

@@ -29,14 +29,35 @@ struct imon {
 static void imon_ir_data(struct imon *imon)
 {
 	struct ir_raw_event rawir = {};
-	u64 d = be64_to_cpup((__be64 *)imon->ir_buf) >> 24;
+	u64 data = be64_to_cpu(imon->ir_buf);
+	u8 packet_no = data & 0xff;
 	int offset = 40;
 	int bit;
 
-	dev_dbg(imon->dev, "data: %*ph", 8, imon->ir_buf);
+	if (packet_no == 0xff)
+		return;
+
+	dev_dbg(imon->dev, "data: %*ph", 8, &imon->ir_buf);
+
+	/*
+	 * Only the first 5 bytes contain IR data. Right shift so we move
+	 * the IR bits to the lower 40 bits.
+	 */
+	data >>= 24;
 
 	do {
-		bit = fls64(d & (BIT_ULL(offset) - 1));
+		/*
+		 * Find highest set bit which is less or equal to offset
+		 *
+		 * offset is the bit above (base 0) where we start looking.
+		 *
+		 * data & (BIT_ULL(offset) - 1) masks off any unwanted bits,
+		 * so we have just bits less than offset.
+		 *
+		 * fls will tell us the highest bit set plus 1 (or 0 if no
+		 * bits are set).
+		 */
+		bit = fls64(data & (BIT_ULL(offset) - 1));
 		if (bit < offset) {
 			dev_dbg(imon->dev, "pulse: %d bits", offset - bit);
 			rawir.pulse = true;

@@ -49,7 +70,12 @@ static void imon_ir_data(struct imon *imon)
 			offset = bit;
 		}
 
-		bit = fls64(~d & (BIT_ULL(offset) - 1));
+		/*
+		 * Find highest clear bit which is less than offset.
+		 *
+		 * Just invert the data and use same trick as above.
+		 */
+		bit = fls64(~data & (BIT_ULL(offset) - 1));
 		dev_dbg(imon->dev, "space: %d bits", offset - bit);
 
 		rawir.pulse = false;

@@ -59,7 +85,7 @@ static void imon_ir_data(struct imon *imon)
 		offset = bit;
 	} while (offset > 0);
 
-	if (imon->ir_buf[7] == 0x0a) {
+	if (packet_no == 0x0a) {
 		ir_raw_event_set_idle(imon->rcdev, true);
 		ir_raw_event_handle(imon->rcdev);
 	}

@@ -72,8 +98,7 @@ static void imon_ir_rx(struct urb *urb)
 
 	switch (urb->status) {
 	case 0:
-		if (imon->ir_buf[7] != 0xff)
-			imon_ir_data(imon);
+		imon_ir_data(imon);
 		break;
 	case -ECONNRESET:
 	case -ENOENT:

@@ -129,7 +154,7 @@ static int imon_probe(struct usb_interface *intf,
 	imon->dev = &intf->dev;
 	usb_fill_int_urb(imon->ir_urb, udev,
 			 usb_rcvintpipe(udev, ir_ep->bEndpointAddress),
-			 imon->ir_buf, sizeof(imon->ir_buf),
+			 &imon->ir_buf, sizeof(imon->ir_buf),
 			 imon_ir_rx, imon, ir_ep->bInterval);
 
 	rcdev = devm_rc_allocate_device(&intf->dev, RC_DRIVER_IR_RAW);
Signature · 1 line
2.21.0
author maintainer
#1 lore raw

PATCH 2/3 media: imon_raw: prevent "nonsensical timing event of duration 0"

Sometimes the device sends IR data which is all space, no pulses whatsoever. Add the end of this the driver will put the rc device into idle mode when it already is in idle mode. The following will be logged:

rc rc0: nonsensical timing event of duration 0
rc rc0: two consecutive events of type space

Signed-off-by Sean Young <[email protected]>
---
 drivers/media/rc/imon_raw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/rc/imon_raw.c +1 -1
index e6723993b466..d4aedcf76418 100644
@@ -85,7 +85,7 @@ static void imon_ir_data(struct imon *imon)
 		offset = bit;
 	} while (offset > 0);
 
-	if (packet_no == 0x0a) {
+	if (packet_no == 0x0a && !imon->rcdev->idle) {
 		ir_raw_event_set_idle(imon->rcdev, true);
 		ir_raw_event_handle(imon->rcdev);
 	}
Signature · 1 line
2.21.0
author maintainer
#2 lore raw

PATCH 3/3 selftests: ir: fix ir_loopback test failure

In reply to Sean Young

The decoder is called rc-mm, not rcmm. This was renamed late in the cycle so this bug crept in.

Cc Shuah Khan <[email protected]>
Signed-off-by Sean Young <[email protected]>
---
 tools/testing/selftests/ir/ir_loopback.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
tools/testing/selftests/ir/ir_loopback.c +3 -3
index e700e09e3682..af7f9c7d59bc 100644
@@ -54,9 +54,9 @@ static const struct {
 	{ RC_PROTO_RC6_MCE, "rc-6-mce", 0x00007fff, "rc-6" },
 	{ RC_PROTO_SHARP, "sharp", 0x1fff, "sharp" },
 	{ RC_PROTO_IMON, "imon", 0x7fffffff, "imon" },
-	{ RC_PROTO_RCMM12, "rcmm-12", 0x00000fff, "rcmm" },
-	{ RC_PROTO_RCMM24, "rcmm-24", 0x00ffffff, "rcmm" },
-	{ RC_PROTO_RCMM32, "rcmm-32", 0xffffffff, "rcmm" },
+	{ RC_PROTO_RCMM12, "rcmm-12", 0x00000fff, "rc-mm" },
+	{ RC_PROTO_RCMM24, "rcmm-24", 0x00ffffff, "rc-mm" },
+	{ RC_PROTO_RCMM32, "rcmm-32", 0xffffffff, "rc-mm" },
 };
 
 int lirc_open(const char *rc)
Signature · 1 line
2.21.0
maintainer Acked-by Shuah Khan on v1 3/3
#3 lore raw

Re: [PATCH 3/3] selftests: ir: fix ir_loopback test failure

Quoted 17 lines · Sean Young

The decoder is called rc-mm, not rcmm. This was renamed late in the cycle so this bug crept in.

Cc Shuah Khan <[email protected]>
Signed-off-by Sean Young <[email protected]>
---
  tools/testing/selftests/ir/ir_loopback.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
tools/testing/selftests/ir/ir_loopback.c +0 -0
index e700e09e3682..af7f9c7d59bc 100644
@@ -54,9 +54,9 @@ static const struct {
  	{ RC_PROTO_RC6_MCE, "rc-6-mce", 0x00007fff, "rc-6" },
  	{ RC_PROTO_SHARP, "sharp", 0x1fff, "sharp" },

tools/testing/selftests/ir/ir_loopback.c static const struct { · line 62

Quoted 10 lines
tools/testing/selftests/ir/ir_loopback.c +3 -3
  	{ RC_PROTO_IMON, "imon", 0x7fffffff, "imon" },
-	{ RC_PROTO_RCMM12, "rcmm-12", 0x00000fff, "rcmm" },
-	{ RC_PROTO_RCMM24, "rcmm-24", 0x00ffffff, "rcmm" },
-	{ RC_PROTO_RCMM32, "rcmm-32", 0xffffffff, "rcmm" },
+	{ RC_PROTO_RCMM12, "rcmm-12", 0x00000fff, "rc-mm" },
+	{ RC_PROTO_RCMM24, "rcmm-24", 0x00ffffff, "rc-mm" },
+	{ RC_PROTO_RCMM32, "rcmm-32", 0xffffffff, "rc-mm" },
  };
 
  int lirc_open(const char *rc)

Thanks Sean! Please cc - linux-keseltest makling list on these patches.

I can take this through my tree or here is my Ack for it go through media tree

Acked-by Shuah Khan <[email protected]>
author maintainer
#4 lore raw

Re: [PATCH 3/3] selftests: ir: fix ir_loopback test failure

Quoted 21 lines · shuah
Quoted 20 lines · Sean Young

The decoder is called rc-mm, not rcmm. This was renamed late in the cycle so this bug crept in.

Cc Shuah Khan <[email protected]>
Signed-off-by Sean Young <[email protected]>
---
  tools/testing/selftests/ir/ir_loopback.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
tools/testing/selftests/ir/ir_loopback.c +0 -3
index e700e09e3682..af7f9c7d59bc 100644
@@ -54,9 +54,9 @@ static const struct {
  	{ RC_PROTO_RC6_MCE, "rc-6-mce", 0x00007fff, "rc-6" },
  	{ RC_PROTO_SHARP, "sharp", 0x1fff, "sharp" },
  	{ RC_PROTO_IMON, "imon", 0x7fffffff, "imon" },
-	{ RC_PROTO_RCMM12, "rcmm-12", 0x00000fff, "rcmm" },
-	{ RC_PROTO_RCMM24, "rcmm-24", 0x00ffffff, "rcmm" },
-	{ RC_PROTO_RCMM32, "rcmm-32", 0xffffffff, "rcmm" },
Quoted 8 lines

tools/testing/selftests/ir/ir_loopback.c static const struct {

Quoted 6 lines
tools/testing/selftests/ir/ir_loopback.c +3 -0
+	{ RC_PROTO_RCMM12, "rcmm-12", 0x00000fff, "rc-mm" },
+	{ RC_PROTO_RCMM24, "rcmm-24", 0x00ffffff, "rc-mm" },
+	{ RC_PROTO_RCMM32, "rcmm-32", 0xffffffff, "rc-mm" },
  };
  int lirc_open(const char *rc)

Thanks Sean! Please cc - linux-keseltest makling list on these patches.

I'll do that next time, thanks.

Quoted 4 lines

I can take this through my tree or here is my Ack for it go through media tree

Acked-by Shuah Khan <[email protected]>

I'm just preparing a pull request for Mauro so I'll put it in there with your Ack.

Thank you

Sean