* [PATCH] media: radio: Critical interrupt bugfix for si470x over i2c
@ 2018-01-26 23:42 Douglas Fischer
2018-02-15 14:38 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Douglas Fischer @ 2018-01-26 23:42 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.
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-01-16 16:54:23.699770645 -0500 @@ -377,8 +377,13 @@ int
si470x_start(struct si470x_device *r goto done; /* sysconfig 1 */
- radio->registers[SYSCONFIG1] =
- (de << 11) & SYSCONFIG1_DE; /* DE*/
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDSIEN;
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_STCIEN;
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDS;
+ radio->registers[SYSCONFIG1] &= ~SYSCONFIG1_GPIO2;
+ radio->registers[SYSCONFIG1] |= 0x1 << 2;
+ if (de)
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
retval = si470x_set_register(radio, SYSCONFIG1);
if (retval < 0)
goto done;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: radio: Critical interrupt bugfix for si470x over i2c
2018-01-26 23:42 [PATCH] media: radio: Critical interrupt bugfix for si470x over i2c Douglas Fischer
@ 2018-02-15 14:38 ` Hans Verkuil
2018-02-24 22:40 ` Douglas Fischer
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2018-02-15 14:38 UTC (permalink / raw)
To: Douglas Fischer, linux-media
On 27/01/18 00:42, 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.
>
> 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-01-16 16:54:23.699770645 -0500 @@ -377,8 +377,13 @@ int
> si470x_start(struct si470x_device *r goto done; /* sysconfig 1 */
> - radio->registers[SYSCONFIG1] =
> - (de << 11) & SYSCONFIG1_DE; /* DE*/
> + radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDSIEN;
> + radio->registers[SYSCONFIG1] |= SYSCONFIG1_STCIEN;
> + radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDS;
Just do:
radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDSIEN | SYSCONFIG1_STCIEN |
SYSCONFIG1_RDS;
> + radio->registers[SYSCONFIG1] &= ~SYSCONFIG1_GPIO2;
Why is this cleared?
> + radio->registers[SYSCONFIG1] |= 0x1 << 2;
What's this? It doesn't use a define, so either add one or add a comment.
> + if (de)
> + radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
> retval = si470x_set_register(radio, SYSCONFIG1);
> if (retval < 0)
> goto done;
>
Also, this is now set in si470x_start, so the same code can now be removed
in si470x_fops_open for i2c.
In general I would feel happier if you just add a 'bool is_i2c' argument to
si470x_start and only change SYSCONFIG1 for the i2c case.
Regards,
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: radio: Critical interrupt bugfix for si470x over i2c
2018-02-15 14:38 ` Hans Verkuil
@ 2018-02-24 22:40 ` Douglas Fischer
0 siblings, 0 replies; 5+ messages in thread
From: Douglas Fischer @ 2018-02-24 22:40 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media
Hans,
Sorry for the delay and thanks for getting back to me. Please see
below. Sorry for the mangles, I'll fix my email setup before I submit a
v2 for all three patches, this is the only one I have questions for you
on.
On Thu, 15 Feb 2018 15:38:55 +0100
Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 27/01/18 00:42, 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.
> >
> > 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-01-16
> > 16:54:23.699770645 -0500 @@ -377,8 +377,13 @@ int
> > si470x_start(struct si470x_device *r goto done; /* sysconfig 1 */
> > - radio->registers[SYSCONFIG1] =
> > - (de << 11) & SYSCONFIG1_DE; /* DE*/
> > + radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDSIEN;
> > + radio->registers[SYSCONFIG1] |= SYSCONFIG1_STCIEN;
> > + radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDS;
>
> Just do:
>
> radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDSIEN |
> SYSCONFIG1_STCIEN | SYSCONFIG1_RDS;
>
> > + radio->registers[SYSCONFIG1] &= ~SYSCONFIG1_GPIO2;
>
> Why is this cleared?
>
> > + radio->registers[SYSCONFIG1] |= 0x1 << 2;
>
> What's this? It doesn't use a define, so either add one or add a
> comment.
I need to set SYSCONFIG1_GPIO2 to 0x01, so clear both bits and
then set just bit 2. Is there a more elegant way to do that?
Should I just add "/* GPIO2 */" at the end of the line?
>
> > + if (de)
> > + radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
> > retval = si470x_set_register(radio, SYSCONFIG1);
> > if (retval < 0)
> > goto done;
> >
>
> Also, this is now set in si470x_start, so the same code can now be
> removed in si470x_fops_open for i2c.
>
> In general I would feel happier if you just add a 'bool is_i2c'
> argument to si470x_start and only change SYSCONFIG1 for the i2c case.
>
I can redo it that way if you would like, but to me it seems
better to write code that just works for both instead of
maintaining two different start sequences? The only difference
is that the i2c version needs GPIO2 set as an interrupt while
the USB version doesn't use GPIO2 at all. So it doesn't affect
the USB version to enable the interrupt on GPIO2.
> Regards,
>
> Hans
Thanks,
Doug
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] media: radio: Critical interrupt bugfix for si470x over i2c
@ 2018-01-20 19:18 Douglas Fischer
0 siblings, 0 replies; 5+ messages in thread
From: Douglas Fischer @ 2018-01-20 19:18 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.
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-01-16 16:54:23.699770645 -0500 @@ -377,8 +377,13 @@ int
si470x_start(struct si470x_device *r goto done; /* sysconfig 1 */
- radio->registers[SYSCONFIG1] =
- (de << 11) & SYSCONFIG1_DE; /* DE*/
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDSIEN;
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_STCIEN;
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDS;
+ radio->registers[SYSCONFIG1] &= ~SYSCONFIG1_GPIO2;
+ radio->registers[SYSCONFIG1] |= 0x1 << 2;
+ if (de)
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
retval = si470x_set_register(radio, SYSCONFIG1);
if (retval < 0)
goto done;
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] media: radio: Critical interrupt bugfix for si470x over i2c
@ 2018-01-18 20:00 Douglas Fischer
0 siblings, 0 replies; 5+ messages in thread
From: Douglas Fischer @ 2018-01-18 20:00 UTC (permalink / raw)
To: 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.
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-01-16 16:54:23.699770645 -0500 @@ -377,8 +377,13 @@ int
si470x_start(struct si470x_device *r goto done; /* sysconfig 1 */
- radio->registers[SYSCONFIG1] =
- (de << 11) & SYSCONFIG1_DE; /* DE*/
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDSIEN;
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_STCIEN;
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDS;
+ radio->registers[SYSCONFIG1] &= ~SYSCONFIG1_GPIO2;
+ radio->registers[SYSCONFIG1] |= 0x1 << 2;
+ if (de)
+ radio->registers[SYSCONFIG1] |= SYSCONFIG1_DE;
retval = si470x_set_register(radio, SYSCONFIG1);
if (retval < 0)
goto done;
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-24 22:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-26 23:42 [PATCH] media: radio: Critical interrupt bugfix for si470x over i2c Douglas Fischer
2018-02-15 14:38 ` Hans Verkuil
2018-02-24 22:40 ` Douglas Fischer
-- strict thread matches above, loose matches on Subject: below --
2018-01-20 19:18 Douglas Fischer
2018-01-18 20:00 Douglas Fischer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.