| Message ID | 1341497792-6066-2-git-send-email-mchehab@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 5, 2012 at 10:16 AM, Mauro Carvalho Chehab > - /* Use both frq_lock and signal to generate the result */ > - signal = signal || ((signal & 0x07) << 12); > + /* Signal level is 3 bits only */ > + > + signal = ((1 << 12) - 1) | ((signal & 0x07) << 12); Are you sure this is correct? It's entirely possible the original code used a logical or because the signal level isn't valid unless there is a lock. The author may have been intending to say: if (signal != 0) /* There is a lock, so set the signal level */ signal = (signal & 0x07) << 12; else signal = 0 /* Leave signal level at zero since there is no lock */ I agree that the way the original code was written is confusing, but it may actually be correct. Devin
On Thu, Jul 5, 2012 at 10:25 AM, Devin Heitmueller <dheitmueller@kernellabs.com> wrote: > On Thu, Jul 5, 2012 at 10:16 AM, Mauro Carvalho Chehab >> - /* Use both frq_lock and signal to generate the result */ >> - signal = signal || ((signal & 0x07) << 12); >> + /* Signal level is 3 bits only */ >> + >> + signal = ((1 << 12) - 1) | ((signal & 0x07) << 12); > > Are you sure this is correct? It's entirely possible the original > code used a logical or because the signal level isn't valid unless > there is a lock. The author may have been intending to say: > > if (signal != 0) /* There is a lock, so set the signal level */ > signal = (signal & 0x07) << 12; > else > signal = 0 /* Leave signal level at zero since there is no lock */ > > I agree that the way the original code was written is confusing, but > it may actually be correct. On second reading, it would have needed to be a logical AND, not an OR in order for my suggestion to have been correct. That said, empirical results are definitely a stronger argument in this case. You did test this change in cases with no signal, signal lock with weak signal, and signal lock with strong signal, right? Devin
Em 05-07-2012 11:31, Devin Heitmueller escreveu: > On Thu, Jul 5, 2012 at 10:25 AM, Devin Heitmueller > <dheitmueller@kernellabs.com> wrote: >> On Thu, Jul 5, 2012 at 10:16 AM, Mauro Carvalho Chehab >>> - /* Use both frq_lock and signal to generate the result */ >>> - signal = signal || ((signal & 0x07) << 12); >>> + /* Signal level is 3 bits only */ >>> + >>> + signal = ((1 << 12) - 1) | ((signal & 0x07) << 12); >> >> Are you sure this is correct? It's entirely possible the original >> code used a logical or because the signal level isn't valid unless >> there is a lock. The author may have been intending to say: >> >> if (signal != 0) /* There is a lock, so set the signal level */ >> signal = (signal & 0x07) << 12; >> else >> signal = 0 /* Leave signal level at zero since there is no lock */ >> >> I agree that the way the original code was written is confusing, but >> it may actually be correct. No, the intention there were to do a bit OR. The idea there was that, if a lock was given, some signal would be received. The real signal level would be identified by the remaining bits. What it was happening due to the code mistake was: if (lock) return 1; else return 0; > On second reading, it would have needed to be a logical AND, not an OR > in order for my suggestion to have been correct. > > That said, empirical results are definitely a stronger argument in > this case. You did test this change in cases with no signal, signal > lock with weak signal, and signal lock with strong signal, right? Yes, it was tested and the new code works fine: it returns 0 without signal and it returns a value between 0 and 65535 depending on the signal strength. Just like the DVB API, the V4L2 API spec is not clear about what type of range should be applied for the signal (linea range? dB?). It just says that it should be between 0 and 65555. In the case of xc2028/3028, there are only 3 bits for signal strengh. The levels are in dB, with an 8dB step, where 0 means a signal weaker than 8dB and 7 means 56 dB or upper. The code should now be coherent with that. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/common/tuners/tuner-xc2028.c b/drivers/media/common/tuners/tuner-xc2028.c index 9638a69..42fdf5c 100644 --- a/drivers/media/common/tuners/tuner-xc2028.c +++ b/drivers/media/common/tuners/tuner-xc2028.c @@ -903,7 +903,7 @@ static int xc2028_signal(struct dvb_frontend *fe, u16 *strength) { struct xc2028_data *priv = fe->tuner_priv; u16 frq_lock, signal = 0; - int rc; + int rc, i; tuner_dbg("%s called\n", __func__); @@ -914,21 +914,28 @@ static int xc2028_signal(struct dvb_frontend *fe, u16 *strength) mutex_lock(&priv->lock); /* Sync Lock Indicator */ - rc = xc2028_get_reg(priv, XREG_LOCK, &frq_lock); - if (rc < 0) - goto ret; + for (i = 0; i < 3; i++) { + rc = xc2028_get_reg(priv, XREG_LOCK, &frq_lock); + if (rc < 0) + goto ret; - /* Frequency is locked */ - if (frq_lock == 1) - signal = 1 << 11; + if (frq_lock) + break; + msleep(6); + } + + /* Frequency was not locked */ + if (frq_lock == 2) + goto ret; /* Get SNR of the video signal */ rc = xc2028_get_reg(priv, XREG_SNR, &signal); if (rc < 0) goto ret; - /* Use both frq_lock and signal to generate the result */ - signal = signal || ((signal & 0x07) << 12); + /* Signal level is 3 bits only */ + + signal = ((1 << 12) - 1) | ((signal & 0x07) << 12); ret: mutex_unlock(&priv->lock);
There are several bugs at the signal strength algorithm: - It is using logical OR, instead of bit OR; - It doesn't wait up to 18 ms as it should; - the strength range is not ok. Rework on it, in order to make it work. Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> --- drivers/media/common/tuners/tuner-xc2028.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)