[1/2,media] au0828: disable tuner links and cache tuner/decoder
diff mbox

Message ID d14f3141901856eaed358ab049f4a3aac8fe4863.1457711514.git.mchehab@osg.samsung.com
State New
Headers show

Commit Message

Mauro Carvalho Chehab March 11, 2016, 3:55 p.m. UTC
For au0828_enable_source() to work, the tuner links should be
disabled and the tuner/decoder should be cached at au0828 struct.

Fixes: 9822f4173f84 ('[media] au0828: use v4l2_mc_create_media_graph()')
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/usb/au0828/au0828-core.c | 42 ++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 15 deletions(-)

Comments

Shuah Khan March 11, 2016, 4:27 p.m. UTC | #1
On 03/11/2016 08:55 AM, Mauro Carvalho Chehab wrote:
> For au0828_enable_source() to work, the tuner links should be
> disabled and the tuner/decoder should be cached at au0828 struct.

hmm. are you sure about needing to cache decoder in au0828 struct.
It gets cached in au0828_card_analog_fe_setup() which is called
from au0828_card_setup() - this step happens before
au0828_media_device_register()

#ifdef CONFIG_MEDIA_CONTROLLER
                if (sd)
                        dev->decoder = &sd->entity;
#endif

thanks,
-- Shuah

> 
> Fixes: 9822f4173f84 ('[media] au0828: use v4l2_mc_create_media_graph()')
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/usb/au0828/au0828-core.c | 42 ++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index 5dc82e8c8670..ecfa18939663 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -456,7 +456,8 @@ static int au0828_media_device_register(struct au0828_dev *dev,
>  {
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	int ret;
> -	struct media_entity *entity, *demod = NULL, *tuner = NULL;
> +	struct media_entity *entity, *demod = NULL;
> +	struct media_link *link;
>  
>  	if (!dev->media_dev)
>  		return 0;
> @@ -482,26 +483,37 @@ static int au0828_media_device_register(struct au0828_dev *dev,
>  	}
>  
>  	/*
> -	 * Find tuner and demod to disable the link between
> -	 * the two to avoid disable step when tuner is requested
> -	 * by video or audio. Note that this step can't be done
> -	 * until dvb graph is created during dvb register.
> +	 * Find tuner, decoder and demod.
> +	 *
> +	 * The tuner and decoder should be cached, as they'll be used by
> +	 *	au0828_enable_source.
> +	 *
> +	 * It also needs to disable the link between tuner and
> +	 * decoder/demod, to avoid disable step when tuner is requested
> +	 * by video or audio. Note that this step can't be done until dvb
> +	 * graph is created during dvb register.
>  	*/
>  	media_device_for_each_entity(entity, dev->media_dev) {
> -		if (entity->function == MEDIA_ENT_F_DTV_DEMOD)
> +		switch (entity->function) {
> +		case MEDIA_ENT_F_TUNER:
> +			dev->tuner = entity;
> +			break;
> +		case MEDIA_ENT_F_ATV_DECODER:
> +			dev->decoder = entity;
> +			break;
> +		case MEDIA_ENT_F_DTV_DEMOD:
>  			demod = entity;
> -		else if (entity->function == MEDIA_ENT_F_TUNER)
> -			tuner = entity;
> +			break;
> +		}
>  	}
> -	/* Disable link between tuner and demod */
> -	if (tuner && demod) {
> -		struct media_link *link;
>  
> -		list_for_each_entry(link, &demod->links, list) {
> -			if (link->sink->entity == demod &&
> -			    link->source->entity == tuner) {
> +	/* Disable link between tuner->demod and/or tuner->decoder */
> +	if (dev->tuner) {
> +		list_for_each_entry(link, &dev->tuner->links, list) {
> +			if (demod && link->sink->entity == demod)
> +				media_entity_setup_link(link, 0);
> +			if (dev->decoder && link->sink->entity == dev->decoder)
>  				media_entity_setup_link(link, 0);
> -			}
>  		}
>  	}
>  
>
Mauro Carvalho Chehab March 11, 2016, 4:34 p.m. UTC | #2
Em Fri, 11 Mar 2016 09:27:40 -0700
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 03/11/2016 08:55 AM, Mauro Carvalho Chehab wrote:
> > For au0828_enable_source() to work, the tuner links should be
> > disabled and the tuner/decoder should be cached at au0828 struct.  
> 
> hmm. are you sure about needing to cache decoder in au0828 struct.
> It gets cached in au0828_card_analog_fe_setup() which is called
> from au0828_card_setup() - this step happens before
> au0828_media_device_register()
> 
> #ifdef CONFIG_MEDIA_CONTROLLER
>                 if (sd)
>                         dev->decoder = &sd->entity;
> #endif

I haven't check it. Yet, I guess the best would be to put those
caches all at au0828_media_device_register(). This way, it would be
easier to remember removing them, once we move au0828_enable_source
to the core.

(my plan is to take a look on it after the merge window, doing some
tests on more complex drivers, like em28xx and saa7134).

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
Shuah Khan March 11, 2016, 4:52 p.m. UTC | #3
On 03/11/2016 09:34 AM, Mauro Carvalho Chehab wrote:
> Em Fri, 11 Mar 2016 09:27:40 -0700
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> On 03/11/2016 08:55 AM, Mauro Carvalho Chehab wrote:
>>> For au0828_enable_source() to work, the tuner links should be
>>> disabled and the tuner/decoder should be cached at au0828 struct.  
>>
>> hmm. are you sure about needing to cache decoder in au0828 struct.
>> It gets cached in au0828_card_analog_fe_setup() which is called
>> from au0828_card_setup() - this step happens before
>> au0828_media_device_register()
>>
>> #ifdef CONFIG_MEDIA_CONTROLLER
>>                 if (sd)
>>                         dev->decoder = &sd->entity;
>> #endif
> 
> I haven't check it. Yet, I guess the best would be to put those
> caches all at au0828_media_device_register(). This way, it would be
> easier to remember removing them, once we move au0828_enable_source
> to the core.

Yes. Consolidating caches in one place is a good idea. One thing we
have to keep in mind when we do that is the !MEDIA_CONTROLLER cases.
The above cached value could be used even when MEDIA_CONTROLLER isn't
enabled.

> 
> (my plan is to take a look on it after the merge window, doing some
> tests on more complex drivers, like em28xx and saa7134).

Sounds good.

thanks,
-- Shuah
Shuah Khan March 11, 2016, 4:55 p.m. UTC | #4
On 03/11/2016 09:52 AM, Shuah Khan wrote:
> On 03/11/2016 09:34 AM, Mauro Carvalho Chehab wrote:
>> Em Fri, 11 Mar 2016 09:27:40 -0700
>> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
>>
>>> On 03/11/2016 08:55 AM, Mauro Carvalho Chehab wrote:
>>>> For au0828_enable_source() to work, the tuner links should be
>>>> disabled and the tuner/decoder should be cached at au0828 struct.  
>>>
>>> hmm. are you sure about needing to cache decoder in au0828 struct.
>>> It gets cached in au0828_card_analog_fe_setup() which is called
>>> from au0828_card_setup() - this step happens before
>>> au0828_media_device_register()
>>>
>>> #ifdef CONFIG_MEDIA_CONTROLLER
>>>                 if (sd)
>>>                         dev->decoder = &sd->entity;
>>> #endif
>>
>> I haven't check it. Yet, I guess the best would be to put those
>> caches all at au0828_media_device_register(). This way, it would be
>> easier to remember removing them, once we move au0828_enable_source
>> to the core.
> 
> Yes. Consolidating caches in one place is a good idea. One thing we
> have to keep in mind when we do that is the !MEDIA_CONTROLLER cases.
> The above cached value could be used even when MEDIA_CONTROLLER isn't
> enabled.

Never mind - the second comment isn't valid! We are setting this
in ifdef CONFIG_MEDIA_CONTROLLER :)

thanks,
-- Shuah

Patch
diff mbox

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index 5dc82e8c8670..ecfa18939663 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -456,7 +456,8 @@  static int au0828_media_device_register(struct au0828_dev *dev,
 {
 #ifdef CONFIG_MEDIA_CONTROLLER
 	int ret;
-	struct media_entity *entity, *demod = NULL, *tuner = NULL;
+	struct media_entity *entity, *demod = NULL;
+	struct media_link *link;
 
 	if (!dev->media_dev)
 		return 0;
@@ -482,26 +483,37 @@  static int au0828_media_device_register(struct au0828_dev *dev,
 	}
 
 	/*
-	 * Find tuner and demod to disable the link between
-	 * the two to avoid disable step when tuner is requested
-	 * by video or audio. Note that this step can't be done
-	 * until dvb graph is created during dvb register.
+	 * Find tuner, decoder and demod.
+	 *
+	 * The tuner and decoder should be cached, as they'll be used by
+	 *	au0828_enable_source.
+	 *
+	 * It also needs to disable the link between tuner and
+	 * decoder/demod, to avoid disable step when tuner is requested
+	 * by video or audio. Note that this step can't be done until dvb
+	 * graph is created during dvb register.
 	*/
 	media_device_for_each_entity(entity, dev->media_dev) {
-		if (entity->function == MEDIA_ENT_F_DTV_DEMOD)
+		switch (entity->function) {
+		case MEDIA_ENT_F_TUNER:
+			dev->tuner = entity;
+			break;
+		case MEDIA_ENT_F_ATV_DECODER:
+			dev->decoder = entity;
+			break;
+		case MEDIA_ENT_F_DTV_DEMOD:
 			demod = entity;
-		else if (entity->function == MEDIA_ENT_F_TUNER)
-			tuner = entity;
+			break;
+		}
 	}
-	/* Disable link between tuner and demod */
-	if (tuner && demod) {
-		struct media_link *link;
 
-		list_for_each_entry(link, &demod->links, list) {
-			if (link->sink->entity == demod &&
-			    link->source->entity == tuner) {
+	/* Disable link between tuner->demod and/or tuner->decoder */
+	if (dev->tuner) {
+		list_for_each_entry(link, &dev->tuner->links, list) {
+			if (demod && link->sink->entity == demod)
+				media_entity_setup_link(link, 0);
+			if (dev->decoder && link->sink->entity == dev->decoder)
 				media_entity_setup_link(link, 0);
-			}
 		}
 	}