* [PATCH v3] media: radio: Critical interrupt bugfix for si470x over i2c
@ 2018-02-26 2:27 Douglas Fischer
2018-02-26 11:57 ` Hans Verkuil
0 siblings, 1 reply; 4+ messages in thread
From: Douglas Fischer @ 2018-02-26 2:27 UTC (permalink / raw)
To: hverkuil, linux-media
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@gmail.com>
---
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 */
+ if (de)
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
retval = si470x_set_register(radio, SYSCONFIG1);
if (retval < 0)
goto done;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] media: radio: Critical interrupt bugfix for si470x over i2c
2018-02-26 2:27 [PATCH v3] media: radio: Critical interrupt bugfix for si470x over i2c Douglas Fischer
@ 2018-02-26 11:57 ` Hans Verkuil
2018-02-26 23:39 ` Douglas Fischer
0 siblings, 1 reply; 4+ messages in thread
From: Hans Verkuil @ 2018-02-26 11:57 UTC (permalink / raw)
To: Douglas Fischer, linux-media
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@gmail.com>
> ---
>
> 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.
Regards,
Hans
> + if (de)
> + radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
> retval = si470x_set_register(radio, SYSCONFIG1);
> if (retval < 0)
> goto done;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] media: radio: Critical interrupt bugfix for si470x over i2c
2018-02-26 11:57 ` Hans Verkuil
@ 2018-02-26 23:39 ` Douglas Fischer
2018-02-27 7:38 ` Hans Verkuil
0 siblings, 1 reply; 4+ messages in thread
From: Douglas Fischer @ 2018-02-26 23:39 UTC (permalink / raw)
To: Hans Verkuil, linux-media
Hans,
See comments below. Thank you for the quick response on this and all
your patience and help in general with these patches.
On Mon, 26 Feb 2018 12:57:26 +0100
Hans Verkuil <hverkuil@xs4all.nl> 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@gmail.com>
> > ---
> >
> > 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 */"?
> Regards,
>
> Hans
>
> > + if (de)
> > + radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
> > retval = si470x_set_register(radio, SYSCONFIG1);
> > if (retval < 0)
> > goto done;
> >
>
Thank you,
Doug
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] media: radio: Critical interrupt bugfix for si470x over i2c
2018-02-26 23:39 ` Douglas Fischer
@ 2018-02-27 7:38 ` Hans Verkuil
0 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2018-02-27 7:38 UTC (permalink / raw)
To: Douglas Fischer, linux-media
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@xs4all.nl> 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@gmail.com>
>>> ---
>>>
>>> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-27 7:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-26 2:27 [PATCH v3] media: radio: Critical interrupt bugfix for si470x over i2c Douglas Fischer
2018-02-26 11:57 ` Hans Verkuil
2018-02-26 23:39 ` Douglas Fischer
2018-02-27 7:38 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox