All of lore.kernel.org
 help / color / mirror / Atom feed
* [REVIEW PATCH] tuner-core fix for au0828 g_tuner bug
@ 2013-03-25 11:32 Hans Verkuil
  2013-03-25 12:11 ` Mauro Carvalho Chehab
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Hans Verkuil @ 2013-03-25 11:32 UTC (permalink / raw)
  To: linux-media; +Cc: Devin Heitmueller, Mauro Carvalho Chehab

While testing the au0828 driver overhaul patch series:

http://patchwork.linuxtv.org/patch/17576/

I discovered that the signal strength as reported by VIDIOC_G_TUNER was
always 0. Initially I thought it was related to the i2c gate, but after
testing some more that turned out to be wrong, although it did gave me the
clue that it was related to the order in which subdevs were called. If
the g_tuner op of au8522 was called before that of tuner-core, then it would
fail, if the order was the other way around then it would work.

Some digging in tuner-core clarified it: if the has_signal callback is set,
then tuner-core would call 'vt->signal = analog_ops->has_signal(&t->fe);'.
But has_signal is always filled in, even if the frontend doesn't implement
get_rf_strength, as is the case for the xc5000. And in that case vt->signal
is overwritten with 0.

Solution: don't set the has_signal callback if get_rf_strength is not
supported. Ditto for get_afc.

Regards,

	Hans

-------- patch ----------
If the tuner frontend does not support get_rf_strength, then don't set
the has_signal callback. Ditto for get_afc.

Both callbacks overwrite the signal and afc fields of struct v4l2_tuner
but that should only happen if the tuner can actually detect this. If
it can't, then it should leave those fields alone so other subdevices
can try and detect the signal/afc.

This fixes the bug where the au8522 detected a signal and then tuner-core
overwrote it with 0 since the xc5000 tuner does not support get_rf_strength.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/tuner-core.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
index f775768..dd8a803 100644
--- a/drivers/media/v4l2-core/tuner-core.c
+++ b/drivers/media/v4l2-core/tuner-core.c
@@ -253,7 +253,7 @@ static int fe_set_config(struct dvb_frontend *fe, void *priv_cfg)
 
 static void tuner_status(struct dvb_frontend *fe);
 
-static struct analog_demod_ops tuner_analog_ops = {
+static const struct analog_demod_ops tuner_analog_ops = {
 	.set_params     = fe_set_params,
 	.standby        = fe_standby,
 	.has_signal     = fe_has_signal,
@@ -453,6 +453,11 @@ static void set_type(struct i2c_client *c, unsigned int type,
 		memcpy(analog_ops, &tuner_analog_ops,
 		       sizeof(struct analog_demod_ops));
 
+		if (fe_tuner_ops->get_rf_strength == NULL)
+			analog_ops->has_signal = NULL;
+		if (fe_tuner_ops->get_afc == NULL)
+			analog_ops->get_afc = NULL;
+
 	} else {
 		t->name = analog_ops->info.name;
 	}
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [REVIEW PATCH] tuner-core fix for au0828 g_tuner bug
  2013-03-25 11:32 [REVIEW PATCH] tuner-core fix for au0828 g_tuner bug Hans Verkuil
@ 2013-03-25 12:11 ` Mauro Carvalho Chehab
  2013-03-25 12:55 ` [PATCH 0/3] Do a few more fixes/cleanups for has_signal/get_afc Mauro Carvalho Chehab
  2013-03-25 14:38 ` [REVIEW PATCH] tuner-core fix for au0828 g_tuner bug Devin Heitmueller
  2 siblings, 0 replies; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2013-03-25 12:11 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media, Devin Heitmueller

Em Mon, 25 Mar 2013 12:32:31 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> While testing the au0828 driver overhaul patch series:
> 
> http://patchwork.linuxtv.org/patch/17576/
> 
> I discovered that the signal strength as reported by VIDIOC_G_TUNER was
> always 0. Initially I thought it was related to the i2c gate, but after
> testing some more that turned out to be wrong, although it did gave me the
> clue that it was related to the order in which subdevs were called. If
> the g_tuner op of au8522 was called before that of tuner-core, then it would
> fail, if the order was the other way around then it would work.
> 
> Some digging in tuner-core clarified it: if the has_signal callback is set,
> then tuner-core would call 'vt->signal = analog_ops->has_signal(&t->fe);'.
> But has_signal is always filled in, even if the frontend doesn't implement
> get_rf_strength, as is the case for the xc5000. And in that case vt->signal
> is overwritten with 0.
> 
> Solution: don't set the has_signal callback if get_rf_strength is not
> supported. Ditto for get_afc.

It looks OK to me.

> 
> Regards,
> 
> 	Hans
> 
> -------- patch ----------
> If the tuner frontend does not support get_rf_strength, then don't set
> the has_signal callback. Ditto for get_afc.
> 
> Both callbacks overwrite the signal and afc fields of struct v4l2_tuner
> but that should only happen if the tuner can actually detect this. If
> it can't, then it should leave those fields alone so other subdevices
> can try and detect the signal/afc.
> 
> This fixes the bug where the au8522 detected a signal and then tuner-core
> overwrote it with 0 since the xc5000 tuner does not support get_rf_strength.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/tuner-core.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> index f775768..dd8a803 100644
> --- a/drivers/media/v4l2-core/tuner-core.c
> +++ b/drivers/media/v4l2-core/tuner-core.c
> @@ -253,7 +253,7 @@ static int fe_set_config(struct dvb_frontend *fe, void *priv_cfg)
>  
>  static void tuner_status(struct dvb_frontend *fe);
>  
> -static struct analog_demod_ops tuner_analog_ops = {
> +static const struct analog_demod_ops tuner_analog_ops = {
>  	.set_params     = fe_set_params,
>  	.standby        = fe_standby,
>  	.has_signal     = fe_has_signal,
> @@ -453,6 +453,11 @@ static void set_type(struct i2c_client *c, unsigned int type,
>  		memcpy(analog_ops, &tuner_analog_ops,
>  		       sizeof(struct analog_demod_ops));
>  
> +		if (fe_tuner_ops->get_rf_strength == NULL)
> +			analog_ops->has_signal = NULL;
> +		if (fe_tuner_ops->get_afc == NULL)
> +			analog_ops->get_afc = NULL;
> +
>  	} else {
>  		t->name = analog_ops->info.name;
>  	}


-- 

Cheers,
Mauro

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 0/3] Do a few more fixes/cleanups for has_signal/get_afc
  2013-03-25 11:32 [REVIEW PATCH] tuner-core fix for au0828 g_tuner bug Hans Verkuil
  2013-03-25 12:11 ` Mauro Carvalho Chehab
@ 2013-03-25 12:55 ` Mauro Carvalho Chehab
  2013-03-25 12:55   ` [PATCH 1/3] tuner-core: return afc instead of zero Mauro Carvalho Chehab
                     ` (2 more replies)
  2013-03-25 14:38 ` [REVIEW PATCH] tuner-core fix for au0828 g_tuner bug Devin Heitmueller
  2 siblings, 3 replies; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2013-03-25 12:55 UTC (permalink / raw)
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List

While reviewing "tuner-core fix for au0828 g_tuner bug" patch, I noticed
a few other related issues with tuner. This series addresses it.

Hans, please add on your git tree when sending me the au0828 g_tuner bug
patch.

Mauro Carvalho Chehab (3):
  tuner-core: return afc instead of zero
  tuner-core: Remove the now uneeded checks at fe_has_signal/get_afc
  tuner-core: handle errors when getting signal strength/afc

 drivers/media/v4l2-core/tuner-core.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

-- 
1.8.1.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] tuner-core: return afc instead of zero
  2013-03-25 12:55 ` [PATCH 0/3] Do a few more fixes/cleanups for has_signal/get_afc Mauro Carvalho Chehab
@ 2013-03-25 12:55   ` Mauro Carvalho Chehab
  2013-03-25 12:55   ` [PATCH 2/3] tuner-core: Remove the now uneeded checks at fe_has_signal/get_afc Mauro Carvalho Chehab
  2013-03-25 12:55   ` [PATCH 3/3] tuner-core: handle errors when getting signal strength/afc Mauro Carvalho Chehab
  2 siblings, 0 replies; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2013-03-25 12:55 UTC (permalink / raw)
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List

While the driver gets AFC from the tuner, it doesn't return it
back via V4L2 API due to a mistake at the return. fix it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/v4l2-core/tuner-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
index dd8a803..5e18f44 100644
--- a/drivers/media/v4l2-core/tuner-core.c
+++ b/drivers/media/v4l2-core/tuner-core.c
@@ -235,7 +235,7 @@ static int fe_get_afc(struct dvb_frontend *fe)
 	if (fe->ops.tuner_ops.get_afc)
 		fe->ops.tuner_ops.get_afc(fe, &afc);
 
-	return 0;
+	return afc;
 }
 
 static int fe_set_config(struct dvb_frontend *fe, void *priv_cfg)
-- 
1.8.1.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] tuner-core: Remove the now uneeded checks at fe_has_signal/get_afc
  2013-03-25 12:55 ` [PATCH 0/3] Do a few more fixes/cleanups for has_signal/get_afc Mauro Carvalho Chehab
  2013-03-25 12:55   ` [PATCH 1/3] tuner-core: return afc instead of zero Mauro Carvalho Chehab
@ 2013-03-25 12:55   ` Mauro Carvalho Chehab
  2013-03-25 12:55   ` [PATCH 3/3] tuner-core: handle errors when getting signal strength/afc Mauro Carvalho Chehab
  2 siblings, 0 replies; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2013-03-25 12:55 UTC (permalink / raw)
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List

Now that those functions are only used when the corresponding
function calls are defined, we don't need to check if those
function calls are present at the structure before using it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/v4l2-core/tuner-core.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
index 5e18f44..f1e8b40 100644
--- a/drivers/media/v4l2-core/tuner-core.c
+++ b/drivers/media/v4l2-core/tuner-core.c
@@ -222,8 +222,7 @@ static int fe_has_signal(struct dvb_frontend *fe)
 {
 	u16 strength = 0;
 
-	if (fe->ops.tuner_ops.get_rf_strength)
-		fe->ops.tuner_ops.get_rf_strength(fe, &strength);
+	fe->ops.tuner_ops.get_rf_strength(fe, &strength);
 
 	return strength;
 }
@@ -232,8 +231,7 @@ static int fe_get_afc(struct dvb_frontend *fe)
 {
 	s32 afc = 0;
 
-	if (fe->ops.tuner_ops.get_afc)
-		fe->ops.tuner_ops.get_afc(fe, &afc);
+	fe->ops.tuner_ops.get_afc(fe, &afc);
 
 	return afc;
 }
@@ -256,8 +254,6 @@ static void tuner_status(struct dvb_frontend *fe);
 static const struct analog_demod_ops tuner_analog_ops = {
 	.set_params     = fe_set_params,
 	.standby        = fe_standby,
-	.has_signal     = fe_has_signal,
-	.get_afc        = fe_get_afc,
 	.set_config     = fe_set_config,
 	.tuner_status   = tuner_status
 };
@@ -453,10 +449,10 @@ static void set_type(struct i2c_client *c, unsigned int type,
 		memcpy(analog_ops, &tuner_analog_ops,
 		       sizeof(struct analog_demod_ops));
 
-		if (fe_tuner_ops->get_rf_strength == NULL)
-			analog_ops->has_signal = NULL;
-		if (fe_tuner_ops->get_afc == NULL)
-			analog_ops->get_afc = NULL;
+		if (fe_tuner_ops->get_rf_strength)
+			analog_ops->has_signal = fe_has_signal;
+		if (fe_tuner_ops->get_afc)
+			analog_ops->get_afc = fe_get_afc;
 
 	} else {
 		t->name = analog_ops->info.name;
-- 
1.8.1.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] tuner-core: handle errors when getting signal strength/afc
  2013-03-25 12:55 ` [PATCH 0/3] Do a few more fixes/cleanups for has_signal/get_afc Mauro Carvalho Chehab
  2013-03-25 12:55   ` [PATCH 1/3] tuner-core: return afc instead of zero Mauro Carvalho Chehab
  2013-03-25 12:55   ` [PATCH 2/3] tuner-core: Remove the now uneeded checks at fe_has_signal/get_afc Mauro Carvalho Chehab
@ 2013-03-25 12:55   ` Mauro Carvalho Chehab
  2 siblings, 0 replies; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2013-03-25 12:55 UTC (permalink / raw)
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List

If those callbacks fail, it should return zero, and not a random
value.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/v4l2-core/tuner-core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
index f1e8b40..cf9a9af 100644
--- a/drivers/media/v4l2-core/tuner-core.c
+++ b/drivers/media/v4l2-core/tuner-core.c
@@ -220,18 +220,20 @@ static void fe_standby(struct dvb_frontend *fe)
 
 static int fe_has_signal(struct dvb_frontend *fe)
 {
-	u16 strength = 0;
+	u16 strength;
 
-	fe->ops.tuner_ops.get_rf_strength(fe, &strength);
+	if (fe->ops.tuner_ops.get_rf_strength(fe, &strength) < 0)
+		return 0;
 
 	return strength;
 }
 
 static int fe_get_afc(struct dvb_frontend *fe)
 {
-	s32 afc = 0;
+	s32 afc;
 
-	fe->ops.tuner_ops.get_afc(fe, &afc);
+	if (fe->ops.tuner_ops.get_afc(fe, &afc) < 0)
+		return 0;
 
 	return afc;
 }
-- 
1.8.1.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [REVIEW PATCH] tuner-core fix for au0828 g_tuner bug
  2013-03-25 11:32 [REVIEW PATCH] tuner-core fix for au0828 g_tuner bug Hans Verkuil
  2013-03-25 12:11 ` Mauro Carvalho Chehab
  2013-03-25 12:55 ` [PATCH 0/3] Do a few more fixes/cleanups for has_signal/get_afc Mauro Carvalho Chehab
@ 2013-03-25 14:38 ` Devin Heitmueller
  2 siblings, 0 replies; 7+ messages in thread
From: Devin Heitmueller @ 2013-03-25 14:38 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media, Mauro Carvalho Chehab

On Mon, Mar 25, 2013 at 7:32 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> This fixes the bug where the au8522 detected a signal and then tuner-core
> overwrote it with 0 since the xc5000 tuner does not support get_rf_strength.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Nice find.  That makes much more sense.

Cheers,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2013-03-25 14:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-25 11:32 [REVIEW PATCH] tuner-core fix for au0828 g_tuner bug Hans Verkuil
2013-03-25 12:11 ` Mauro Carvalho Chehab
2013-03-25 12:55 ` [PATCH 0/3] Do a few more fixes/cleanups for has_signal/get_afc Mauro Carvalho Chehab
2013-03-25 12:55   ` [PATCH 1/3] tuner-core: return afc instead of zero Mauro Carvalho Chehab
2013-03-25 12:55   ` [PATCH 2/3] tuner-core: Remove the now uneeded checks at fe_has_signal/get_afc Mauro Carvalho Chehab
2013-03-25 12:55   ` [PATCH 3/3] tuner-core: handle errors when getting signal strength/afc Mauro Carvalho Chehab
2013-03-25 14:38 ` [REVIEW PATCH] tuner-core fix for au0828 g_tuner bug Devin Heitmueller

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.