Re: [RFC PATCH v2 2/6] bttv: audio_mux(): do not change the value of the v4l2 mute control

["Frank Schäfer" <fschaefer.oss@googlemail.com>]

Hi Hans,

thank you for reviewing and sorry for the delayed reply !

Am 12.03.2013 14:41, schrieb Hans Verkuil:
> On Sun 10 March 2013 22:53:50 Frank Schäfer wrote:
>> There are cases where we want to call audio_mux() without changing the value of
>> the v4l2 mute control, for example
>> - mute mute on last close
>> - mute on device probing
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>
> I think it might be a good idea to take this one step further: pull the
> btv->audio assignment out of audio_mux as well. In other words, audio_mux
> no longer changes the bttv state.

Yes, that should be easy to do.

> I also think that the audio_mute and audio_input functions should be
> removed, just let the code call audio_mux directly. I think that is more
> transparent and one less intermediate call.

OTOH I think they improve readability.
Anyway, this will become obsolete with the next step you're suggesting:

> A next step would be to untangle the audio routing code and mute code
> from audio_mux: that's probably the core of all the confusion. The code
> relating to the input selection should be put in a separate function that
> is called only when you really want to switch inputs.

Yeah, it would be nice if we could do that, but I'm not sure if it's
entirely possible.
Mute + automute and the selected input seem to be coupled via gpio
settings. See the first third
of function audio_mux().
I will have to take a deeper look into the code to see if splitting the
function really makes sense...

> I'm not sure if you want to spend time on that last step, if not, then just
> do the first two suggestions and I'll test the result. But without really
> going to the core of the problem (one function mixing up muting and input
> selection) it remains hard to prove correctness of the code. If you have
> some time, then it would be very nice if this mess can be resolved once and
> for all.

I've put it on my TODO list, but I'm not sure if I'll find some time for
it this weekend.

Regards,
Frank

> Regards,
>
> 	Hans
>
>> ---
>>  drivers/media/pci/bt8xx/bttv-driver.c |    8 ++++----
>>  1 Datei geändert, 4 Zeilen hinzugefügt(+), 4 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
>> index a584d82..a082ab4 100644
>> --- a/drivers/media/pci/bt8xx/bttv-driver.c
>> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
>> @@ -999,7 +999,6 @@ audio_mux(struct bttv *btv, int input, int mute)
>>  		   bttv_tvcards[btv->c.type].gpiomask);
>>  	signal = btread(BT848_DSTATUS) & BT848_DSTATUS_HLOC;
>>  
>> -	btv->mute = mute;
>>  	btv->audio = input;
>>  
>>  	/* automute */
>> @@ -1031,7 +1030,7 @@ audio_mux(struct bttv *btv, int input, int mute)
>>  
>>  		ctrl = v4l2_ctrl_find(btv->sd_msp34xx->ctrl_handler, V4L2_CID_AUDIO_MUTE);
>>  		if (ctrl)
>> -			v4l2_ctrl_s_ctrl(ctrl, btv->mute);
>> +			v4l2_ctrl_s_ctrl(ctrl, mute);
>>  
>>  		/* Note: the inputs tuner/radio/extern/intern are translated
>>  		   to msp routings. This assumes common behavior for all msp3400
>> @@ -1080,7 +1079,7 @@ audio_mux(struct bttv *btv, int input, int mute)
>>  		ctrl = v4l2_ctrl_find(btv->sd_tvaudio->ctrl_handler, V4L2_CID_AUDIO_MUTE);
>>  
>>  		if (ctrl)
>> -			v4l2_ctrl_s_ctrl(ctrl, btv->mute);
>> +			v4l2_ctrl_s_ctrl(ctrl, mute);
>>  		v4l2_subdev_call(btv->sd_tvaudio, audio, s_routing,
>>  				input, 0, 0);
>>  	}
>> @@ -1088,7 +1087,7 @@ audio_mux(struct bttv *btv, int input, int mute)
>>  		ctrl = v4l2_ctrl_find(btv->sd_tda7432->ctrl_handler, V4L2_CID_AUDIO_MUTE);
>>  
>>  		if (ctrl)
>> -			v4l2_ctrl_s_ctrl(ctrl, btv->mute);
>> +			v4l2_ctrl_s_ctrl(ctrl, mute);
>>  	}
>>  	return 0;
>>  }
>> @@ -1300,6 +1299,7 @@ static int bttv_s_ctrl(struct v4l2_ctrl *c)
>>  		break;
>>  	case V4L2_CID_AUDIO_MUTE:
>>  		audio_mute(btv, c->val);
>> +		btv->mute = c->val;
>>  		break;
>>  	case V4L2_CID_AUDIO_VOLUME:
>>  		btv->volume_gpio(btv, c->val);
>>