On 02/27/2018 12:39 AM, Douglas Fischer wrote: > 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! > > On Mon, 26 Feb 2018 12:57:26 +0100 > Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > >> On 02/26/2018 03:27 AM, Douglas Fischer wrote: >>> 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 <fischerdouglasc@xxxxxxxxx> >>> --- >>> >>> 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 */ >>> - 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. >> > 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 >> Regards, >> >> Hans >> >>> + if (de) >>> + radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE; >>> retval = si470x_set_register(radio, SYSCONFIG1); >>> if (retval < 0) >>> goto done; >>> >> > Thank you, > Doug >
