Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver

["Marko Mäkelä" <marko.makela@iki.fi>]

[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]

Sun, Jun 26, 2022 at 03:33:48PM +0300, Marko Mäkelä wrote:
>How about the following improvement? If IR_RX_BC is a simple cursor to 
>the 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending 
>refresh_tab[] but simply remember where the previous call left off. We 
>could always read the 128 bytes at IR_RX_BUF, and process everything 
>between the previous position reported by IR_RX_BC and the current 
>position reported by IR_RX_BC, and treat buf[] as a ring buffer.

I experimented with this on the 5.19.0-rc3 kernel. With the attached 
patch applied on top of this patch series, "ir-keytables -t" reported 
only one RC5 encoded key-down event. I had to unplug and plug in the 
adapter in order to receive another RC5 event. The refresh command seems 
to be necessary for the device to store and forward further IR data.

>Last time I tested it, the patch was a significant improvement. I think 
>that "perfect" is the enemy of "good enough", and the patch should be 
>included in the kernel.

The remaining problem definitely is a limitation of the interface. There 
is little that can be done to work around it.

	Marko

[-- Attachment #2: rc_bc_ringbuffer.patch --]
[-- Type: text/x-diff, Size: 2783 bytes --]

diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
index a83b1107fc7f..04670cec727c 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
@@ -1711,16 +1711,10 @@ static int rtl2831u_get_rc_config(struct dvb_usb_device *d,
 
 static int rtl2832u_rc_query(struct dvb_usb_device *d)
 {
-	int ret, i, len;
+	int ret, i, end;
 	struct rtl28xxu_dev *dev = d->priv;
 	struct ir_raw_event ev = {};
 	u8 buf[128];
-	static const struct rtl28xxu_reg_val_mask refresh_tab[] = {
-		{IR_RX_IF,               0x03, 0xff},
-		{IR_RX_BUF_CTRL,         0x80, 0xff},
-		{IR_RX_CTRL,             0x80, 0xff},
-	};
-	u32 idle_length;
 
 	/* init remote controller */
 	if (!dev->rc_active) {
@@ -1761,48 +1755,22 @@ static int rtl2832u_rc_query(struct dvb_usb_device *d)
 		goto exit;
 
 	ret = rtl28xxu_rd_reg(d, IR_RX_BC, &buf[0]);
-	if (ret || buf[0] > sizeof(buf))
+	if (ret)
 		goto err;
 
-	len = buf[0];
+	i = dev->rc_bc;
+	end = dev->rc_bc = buf[0] & 0x7f;
 
 	/* read raw code from hw */
-	ret = rtl28xxu_rd_regs(d, IR_RX_BUF, buf, len);
+	ret = rtl28xxu_rd_regs(d, IR_RX_BUF, buf, sizeof buf);
 	if (ret)
 		goto err;
 
-	dev_dbg(&d->intf->dev, "IR_RX_BUF=%*ph\n", len, buf);
-
-	/* if the receiver is not idle yet, do not process */
-	idle_length = 0;
-	if (len > 2) {
-		if (!(buf[len - 1] & 0x80))
-			idle_length += buf[len - 1];
-		if (!(buf[len - 2] & 0x80))
-			idle_length += buf[len - 2];
-	}
-
-	if (idle_length < 0xbf) {
-		/*
-		 * If the IR does not end with a space equal to the idle
-		 * length, then the IR is not complete yet and more is to
-		 * arrive shortly. If we process it and flush the buffer now,
-		 * we end up missing IR.
-		 */
-		dev_dbg(&d->intf->dev, "ignoring idle=%x\n", idle_length);
-		return 0;
-	}
-
-	/* let hw receive new code */
-	for (i = 0; i < ARRAY_SIZE(refresh_tab); i++) {
-		ret = rtl28xxu_wr_reg_mask(d, refresh_tab[i].reg,
-				refresh_tab[i].val, refresh_tab[i].mask);
-		if (ret)
-			goto err;
-	}
+	dev_dbg(&d->intf->dev, "IR_RX_BUF=%d,%*ph\n", end,
+		(int) sizeof buf, buf);
 
 	/* pass data to Kernel IR decoder */
-	for (i = 0; i < len; i++) {
+	for (; i != end; i++, i &= 0x7f) {
 		ev.pulse = buf[i] >> 7;
 		ev.duration = 51 * (buf[i] & 0x7f);
 		ir_raw_event_store_with_filter(d->rc_dev, &ev);
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
index d5e207baa05d..b1abd73a3020 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
@@ -64,6 +64,7 @@ struct rtl28xxu_dev {
 	u8 tuner;
 	char *tuner_name;
 	u8 page; /* integrated demod active register page */
+	u8 rc_bc;
 	struct i2c_adapter *demod_i2c_adapter;
 	bool rc_active;
 	bool new_i2c_write;