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@xxxxxxxxx> 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@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-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
