Re: [PATCH] media: radio: Critical interrupt bugfix for si470x over i2c

[Douglas Fischer <fischerdouglasc@gmail.com>]

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