Skip to contentSkip to conversation

media: radio: Tuning bugfix for si470x over i2c

PATCH In review Douglas Fischer 2 participants 2 messages Jan 20, 2018 – Feb 15, 2018 Media (V4L/DVB)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 media: radio: Tuning bugfix for si470x over i2c

Fixed si470x_set_channel() trying to tune before chip is turned on, which causes warnings in dmesg and when probing, makes driver wait for 3s for tuning timeout. This issue did not affect USB devices because they have a different probing sequence.

---

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 17:04:59.706409128 -0500 @@ -207,29 +207,37 @@ static int

si470x_set_chan(struct si470x unsigned long time_left; bool timed_out = false;
- /* start tuning */
- radio->registers[CHANNEL] &= ~CHANNEL_CHAN;
- radio->registers[CHANNEL] |= CHANNEL_TUNE | chan;
- retval = si470x_set_register(radio, CHANNEL);
- if (retval < 0)
- goto done;
+ retval = si470x_get_register(radio, POWERCFG);
+ if (retval)
+ return retval;

- /* wait till tune operation has completed */
- reinit_completion(&radio->completion);
- time_left = wait_for_completion_timeout(&radio->completion,
-
msecs_to_jiffies(tune_timeout));
- if (time_left == 0)
- timed_out = true;
+ if ( (radio->registers[POWERCFG] & POWERCFG_ENABLE) &&
+ (radio->registers[POWERCFG] & POWERCFG_DMUTE) ) {

- if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
- dev_warn(&radio->videodev.dev, "tune does not
complete\n");
- if (timed_out)
- dev_warn(&radio->videodev.dev,
- "tune timed out after %u ms\n", tune_timeout);
+ /* start tuning */
+ radio->registers[CHANNEL] &= ~CHANNEL_CHAN;
+ radio->registers[CHANNEL] |= CHANNEL_TUNE | chan;
+ retval = si470x_set_register(radio, CHANNEL);
+ if (retval < 0)
+ goto done;

- /* stop tuning */
- radio->registers[CHANNEL] &= ~CHANNEL_TUNE;
- retval = si470x_set_register(radio, CHANNEL);
+ /* wait till tune operation has completed */
+ reinit_completion(&radio->completion);
+ time_left =
wait_for_completion_timeout(&radio->completion,
+
msecs_to_jiffies(tune_timeout));
+ if (time_left == 0)
+ timed_out = true;
+
+ if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) ==
0)
+ dev_warn(&radio->videodev.dev, "tune does not
complete\n");
+ if (timed_out)
+ dev_warn(&radio->videodev.dev,
+ "tune timed out after %u ms\n",
tune_timeout);
+
+ /* stop tuning */
+ radio->registers[CHANNEL] &= ~CHANNEL_TUNE;
+ retval = si470x_set_register(radio, CHANNEL);
+ }

done:
return retval;

#1 lore raw

Hi Douglas,

My apologies for the delay, but I have finally time to look at this.

First of all: all your patches are mangles. Your mailer probably is trying to wrap around long lines and the end result is not usable. Please check this next time.

Also, when you post newer versions of patches it is good practice to add a version number: e.g. '[PATCHv3] media: radio: Tuning bugfix for si470x over i2c'.

That helps us keeping track of different versions.

Quoted 32 lines · Douglas Fischer

Fixed si470x_set_channel() trying to tune before chip is turned on, which causes warnings in dmesg and when probing, makes driver wait for 3s for tuning timeout. This issue did not affect USB devices because they have a different probing sequence.

---

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 17:04:59.706409128 -0500 @@ -207,29 +207,37 @@ static int

si470x_set_chan(struct si470x unsigned long time_left; bool timed_out = false;

(quoted diff) +3 -10
-	/* start tuning */
-	radio->registers[CHANNEL] &= ~CHANNEL_CHAN;
-	radio->registers[CHANNEL] |= CHANNEL_TUNE | chan;
-	retval = si470x_set_register(radio, CHANNEL);
-	if (retval < 0)
-		goto done;
+	retval = si470x_get_register(radio, POWERCFG);
+	if (retval)
+		return retval;
 
-	/* wait till tune operation has completed */
-	reinit_completion(&radio->completion);
-	time_left = wait_for_completion_timeout(&radio->completion,
-

msecs_to_jiffies(tune_timeout));

Review

Quoted 4 lines
(quoted diff) +2 -2
-	if (time_left == 0)
-		timed_out = true;
+	if ( (radio->registers[POWERCFG] & POWERCFG_ENABLE) && 
+		(radio->registers[POWERCFG] & POWERCFG_DMUTE) ) { 

Just do:

	if (radio->registers[POWERCFG] & POWERCFG_ENABLE) & (POWERCFG_ENABLE | POWERCFG_DMUTE) !=
	    POWERCFG_ENABLE | POWERCFG_DMUTE)
		return 0;

And the remainder of the code can be indented one tab to the left. It's easier to read and the diff is also smaller.

Regards,

Hans

Quoted 42 lines
(quoted diff) +0 -2
-	if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
-		dev_warn(&radio->videodev.dev, "tune does not

complete\n");

(quoted diff) +9 -6
-	if (timed_out)
-		dev_warn(&radio->videodev.dev,
-			"tune timed out after %u ms\n", tune_timeout);
+		/* start tuning */
+		radio->registers[CHANNEL] &= ~CHANNEL_CHAN;
+		radio->registers[CHANNEL] |= CHANNEL_TUNE | chan;
+		retval = si470x_set_register(radio, CHANNEL);
+		if (retval < 0)
+			goto done;
 
-	/* stop tuning */
-	radio->registers[CHANNEL] &= ~CHANNEL_TUNE;
-	retval = si470x_set_register(radio, CHANNEL);
+		/* wait till tune operation has completed */
+		reinit_completion(&radio->completion);
+		time_left =

wait_for_completion_timeout(&radio->completion,
+
msecs_to_jiffies(tune_timeout));

(quoted diff) +4 -0
+		if (time_left == 0)
+			timed_out = true;
+	
+		if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) ==

0)

(quoted diff) +1 -0
+			dev_warn(&radio->videodev.dev, "tune does not

complete\n");

(quoted diff) +3 -0
+		if (timed_out)
+			dev_warn(&radio->videodev.dev,
+				"tune timed out after %u ms\n",

tune_timeout);

(quoted diff) +5 -0
+	
+		/* stop tuning */
+		radio->registers[CHANNEL] &= ~CHANNEL_TUNE;
+		retval = si470x_set_register(radio, CHANNEL);
+	}
 
 done:
 	return retval;