Skip to contentSkip to conversation

media: radio: Critical interrupt bugfix for si470x over i2c

PATCH Superseded Douglas Fischer 2 participants 4 messages Feb 26, 2018 – Feb 27, 2018

Subsystems (2)
Media (V4L/DVB) SI470X FM radio receiver USB driver

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Douglas Fischer

author
#0 lore raw

PATCH v3 1/1 media: radio: Critical interrupt bugfix for si470x over i2c

Fixed si470x_start() disabling the interrupt signal, causing tune operations to never complete. This does not affect USB radios because they poll the registers instead of using the IRQ line.

Stylistic and comment changes from v2.

Signed-off-by Douglas Fischer <[email protected]>

---

drivers/media/radio/si470x/radio-si470x-common.c +5 -2
diff -uprN linux.orig/drivers/media/radio/si470x/radio-si470x-common.c linux/drivers/media/radio/si470x/radio-si470x-common.c
@@ -377,8 +377,11 @@ int si470x_start(struct si470x_device *r
 		goto done;
 
 	/* sysconfig 1 */
-	radio->registers[SYSCONFIG1] =
-		(de << 11) & SYSCONFIG1_DE;		/* DE*/
+	radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDSIEN|SYSCONFIG1_STCIEN|SYSCONFIG1_RDS;
+	radio->registers[SYSCONFIG1] &= ~SYSCONFIG1_GPIO2;
+	radio->registers[SYSCONFIG1] |= (0x01 << 2); /* GPIO2 */
+	if (de)
+		radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
 	retval = si470x_set_register(radio, SYSCONFIG1);
 	if (retval < 0)
 		goto done;
#1 lore raw
Quoted 13 lines · Douglas Fischer

Fixed si470x_start() disabling the interrupt signal, causing tune operations to never complete. This does not affect USB radios because they poll the registers instead of using the IRQ line.

Stylistic and comment changes from v2.

Signed-off-by Douglas Fischer <[email protected]>

---

diff -uprN linux.orig/drivers/media/radio/si470x/radio-si470x-common.c linux/drivers/media/radio/si470x/radio-si470x-common.c

drivers/media/radio/si470x/radio-si470x-common.c int si470x_start(struct si470x_device *r · line 382

Quoted 8 lines
drivers/media/radio/si470x/radio-si470x-common.c +3 -2
@@ -377,8 +377,11 @@ int si470x_start(struct si470x_device *r
 		goto done;
 
 	/* sysconfig 1 */
-	radio->registers[SYSCONFIG1] =
-		(de << 11) & SYSCONFIG1_DE;		/* DE*/
+	radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDSIEN|SYSCONFIG1_STCIEN|SYSCONFIG1_RDS;
+	radio->registers[SYSCONFIG1] &= ~SYSCONFIG1_GPIO2;
+	radio->registers[SYSCONFIG1] |= (0x01 << 2); /* GPIO2 */

Yes, but what does this do? Enable GPIO2? The header defines two bits for GPIO1/2/3, but it doesn't say what those bits mean. So the question here is what it means to set bit 2 to 1 and bit 3 to 0? The header doesn't give any information about that, nor does this comment.

Regards,

Hans

Quoted 6 lines
drivers/media/radio/si470x/radio-si470x-common.c +2 -0
+	if (de)
+		radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
 	retval = si470x_set_register(radio, SYSCONFIG1);
 	if (retval < 0)
 		goto done;
author
#2 lore raw

Hans,

See comments below. Thank you for the quick response on this and all your patience and help in general with these patches.

Quoted 21 lines · On Mon, 26 Feb 2018 12:57:26 +0100 Hans Verkuil <[email protected]> wrote:
Quoted 20 lines · Douglas Fischer

Fixed si470x_start() disabling the interrupt signal, causing tune operations to never complete. This does not affect USB radios because they poll the registers instead of using the IRQ line.

Stylistic and comment changes from v2.

Signed-off-by Douglas Fischer <[email protected]>

---

diff -uprN linux.orig/drivers/media/radio/si470x/radio-si470x-common.c linux/drivers/media/radio/si470x/radio-si470x-common.c --- linux.orig/drivers/media/radio/si470x/radio-si470x-common.c

2018-01-15 21:58:10.675620432 -0500 +++
linux/drivers/media/radio/si470x/radio-si470x-common.c
2018-02-25 19:16:31.785934211 -0500 @@ -377,8 +377,11 @@ int

si470x_start(struct si470x_device *r goto done; /* sysconfig 1 */

drivers/media/radio/si470x/radio-si470x-common.c +1 -2
-	radio->registers[SYSCONFIG1] =
-		(de << 11) & SYSCONFIG1_DE;		/* DE*/
+	radio->registers[SYSCONFIG1] |=
Quoted 9 lines

drivers/media/radio/si470x/radio-si470x-common.c int si470x_start(struct si470x_device *r · line 382

Quoted 3 lines

SYSCONFIG1_RDSIEN|SYSCONFIG1_STCIEN|SYSCONFIG1_RDS;

drivers/media/radio/si470x/radio-si470x-common.c +2 -0
+	radio->registers[SYSCONFIG1] &= ~SYSCONFIG1_GPIO2;
+	radio->registers[SYSCONFIG1] |= (0x01 << 2); /* GPIO2 */

Yes, but what does this do? Enable GPIO2? The header defines two bits for GPIO1/2/3, but it doesn't say what those bits mean. So the question here is what it means to set bit 2 to 1 and bit 3 to 0? The header doesn't give any information about that, nor does this comment.

SYSCONFIG1_GPIO2 is bits 2 and 3, I need to clear bit 3 and set bit 2 without changing the other bits. This configures GPIO2 as "STC/RDS interrupt. A logic high will be output unless an interrupt occurs". Should I change the comment to read "/* GPIO2 STC/RDS interrupt output */"?

Quoted 11 lines

Regards,

Hans

drivers/media/radio/si470x/radio-si470x-common.c int si470x_start(struct si470x_device *r · line 387

Quoted 6 lines
drivers/media/radio/si470x/radio-si470x-common.c +2 -0
+	if (de)
+		radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
 	retval = si470x_set_register(radio, SYSCONFIG1);
 	if (retval < 0)
 		goto done;

Thank you,
Doug

#3 lore raw
Quoted 4 lines · Douglas Fischer

Hans,

See comments below. Thank you for the quick response on this and all your patience and help in general with these patches.

My pleasure!

Quoted 34 lines
Quoted 21 lines · On Mon, 26 Feb 2018 12:57:26 +0100 Hans Verkuil <[email protected]> wrote:
Quoted 20 lines · Douglas Fischer

Fixed si470x_start() disabling the interrupt signal, causing tune operations to never complete. This does not affect USB radios because they poll the registers instead of using the IRQ line.

Stylistic and comment changes from v2.

Signed-off-by Douglas Fischer <[email protected]>

---

diff -uprN linux.orig/drivers/media/radio/si470x/radio-si470x-common.c linux/drivers/media/radio/si470x/radio-si470x-common.c --- linux.orig/drivers/media/radio/si470x/radio-si470x-common.c

2018-01-15 21:58:10.675620432 -0500 +++
linux/drivers/media/radio/si470x/radio-si470x-common.c
2018-02-25 19:16:31.785934211 -0500 @@ -377,8 +377,11 @@ int

si470x_start(struct si470x_device *r goto done; /* sysconfig 1 */

drivers/media/radio/si470x/radio-si470x-common.c +1 -2
-	radio->registers[SYSCONFIG1] =
-		(de << 11) & SYSCONFIG1_DE;		/* DE*/
+	radio->registers[SYSCONFIG1] |=
Quoted 9 lines

drivers/media/radio/si470x/radio-si470x-common.c int si470x_start(struct si470x_device *r · line 382

Quoted 3 lines

SYSCONFIG1_RDSIEN|SYSCONFIG1_STCIEN|SYSCONFIG1_RDS;

drivers/media/radio/si470x/radio-si470x-common.c +2 -0
+	radio->registers[SYSCONFIG1] &= ~SYSCONFIG1_GPIO2;
+	radio->registers[SYSCONFIG1] |= (0x01 << 2); /* GPIO2 */

Yes, but what does this do? Enable GPIO2? The header defines two bits for GPIO1/2/3, but it doesn't say what those bits mean. So the question here is what it means to set bit 2 to 1 and bit 3 to 0? The header doesn't give any information about that, nor does this comment.

Quoted 1 line

SYSCONFIG1_GPIO2 is bits 2 and 3, I need to clear bit 3 and set bit 2 without changing the other bits. This configures GPIO2 as "STC/RDS interrupt. A logic high will be output unless an interrupt occurs". Should I change the comment to read "/* GPIO2 STC/RDS interrupt output */"?

It might be better to have defines in the header that make this explicit.

E.g. right now we have just:

#define SYSCONFIG1_GPIO2 0x000c /* bits 03..02: General Purpose I/O 2 */

So this could be changed to:

#define SYSCONFIG1_GPIO2_MSK     0x000c  /* bits 03..02: General Purpose I/O 2 */
#define SYSCONFIG1_GPIO2_DIS     0       /* Disable GPIO 2 interrupt */
#define SYSCONFIG1_GPIO2_STC_RDS 0x0004  /* Enable STC/RDS interrupt */

...

(I'm guessing 0 means no irq)

That way the code can become self-documenting.

Regards,

Hans

Quoted 14 lines
Quoted 11 lines

Regards,

Hans

drivers/media/radio/si470x/radio-si470x-common.c int si470x_start(struct si470x_device *r · line 387

Quoted 6 lines
drivers/media/radio/si470x/radio-si470x-common.c +2 -0
+	if (de)
+		radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
 	retval = si470x_set_register(radio, SYSCONFIG1);
 	if (retval < 0)
 		goto done;

Thank you,
Doug