[RFC PATCH 1/2] bttv: fix audio mute on device close for the video device node

[RFC PATCH 1/2] bttv: fix audio mute on device close for the video device node

[Frank Schäfer]

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/pci/bt8xx/bttv-driver.c |   22 +++++++++++-----------
 1 Datei geändert, 11 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)

diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
index 8610b6a..2c09bc5 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -992,21 +992,20 @@ static char *audio_modes[] = {
 static int
 audio_mux(struct bttv *btv, int input, int mute)
 {
-	int gpio_val, signal;
+	int gpio_val, signal, mute_gpio;
 	struct v4l2_ctrl *ctrl;
 
 	gpio_inout(bttv_tvcards[btv->c.type].gpiomask,
 		   bttv_tvcards[btv->c.type].gpiomask);
 	signal = btread(BT848_DSTATUS) & BT848_DSTATUS_HLOC;
 
-	btv->mute = mute;
 	btv->audio = input;
 
 	/* automute */
-	mute = mute || (btv->opt_automute && (!signal || !btv->users)
+	mute_gpio = mute || (btv->opt_automute && (!signal || !btv->users)
 				&& !btv->has_radio_tuner);
 
-	if (mute)
+	if (mute_gpio)
 		gpio_val = bttv_tvcards[btv->c.type].gpiomute;
 	else
 		gpio_val = bttv_tvcards[btv->c.type].gpiomux[input];
@@ -1022,7 +1021,7 @@ audio_mux(struct bttv *btv, int input, int mute)
 	}
 
 	if (bttv_gpio)
-		bttv_gpio_tracking(btv, audio_modes[mute ? 4 : input]);
+		bttv_gpio_tracking(btv, audio_modes[mute_gpio ? 4 : input]);
 	if (in_interrupt())
 		return 0;
 
@@ -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);
@@ -3062,8 +3062,7 @@ static int bttv_open(struct file *file)
 			    sizeof(struct bttv_buffer),
 			    fh, &btv->lock);
 	set_tvnorm(btv,btv->tvnorm);
-	set_input(btv, btv->input, btv->tvnorm);
-
+	set_input(btv, btv->input, btv->tvnorm); /* also (un)mutes audio */
 
 	/* The V4L2 spec requires one global set of cropping parameters
 	   which only change on request. These are stored in btv->crop[1].
@@ -3124,7 +3123,7 @@ static int bttv_release(struct file *file)
 	bttv_field_count(btv);
 
 	if (!btv->users)
-		audio_mute(btv, btv->mute);
+		audio_mute(btv, 1);
 
 	v4l2_fh_del(&fh->fh);
 	v4l2_fh_exit(&fh->fh);
@@ -4209,6 +4208,7 @@ static int bttv_probe(struct pci_dev *dev, const struct pci_device_id *pci_id)
 	btv->std = V4L2_STD_PAL;
 	init_irqreg(btv);
 	v4l2_ctrl_handler_setup(hdl);
+	audio_mute(btv, 1);
 
 	if (hdl->error) {
 		result = hdl->error;
-- 
1.7.10.4

[RFC PATCH 2/2] bttv: fix audio mute on device close for the radio device node

[Frank Schäfer]

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/pci/bt8xx/bttv-driver.c |    5 ++++-
 1 Datei geändert, 4 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)

diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
index 2c09bc5..74977f7 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -3227,6 +3227,7 @@ static int radio_open(struct file *file)
 	v4l2_fh_init(&fh->fh, vdev);
 
 	btv->radio_user++;
+	audio_mute(btv, btv->mute);
 
 	v4l2_fh_add(&fh->fh);
 
@@ -3248,8 +3249,10 @@ static int radio_release(struct file *file)
 
 	bttv_call_all(btv, core, ioctl, SAA6588_CMD_CLOSE, &cmd);
 
-	if (btv->radio_user == 0)
+	if (btv->radio_user == 0) {
 		btv->has_radio_tuner = 0;
+		audio_mute(btv, 1);
+	}
 	return 0;
 }
 
-- 
1.7.10.4

Re: [RFC PATCH 2/2] bttv: fix audio mute on device close for the radio device node

[Frank Schäfer]

Hans, in one of your previous comments you mentioned that radio devices
have to be handled differently, so I'm not sure if this is the right
thing to do...

Am 10.03.2013 12:40, schrieb Frank Schäfer:
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/pci/bt8xx/bttv-driver.c |    5 ++++-
>  1 Datei geändert, 4 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
>
> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> index 2c09bc5..74977f7 100644
> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> @@ -3227,6 +3227,7 @@ static int radio_open(struct file *file)
>  	v4l2_fh_init(&fh->fh, vdev);
>  
>  	btv->radio_user++;
> +	audio_mute(btv, btv->mute);
>  
>  	v4l2_fh_add(&fh->fh);
>  
> @@ -3248,8 +3249,10 @@ static int radio_release(struct file *file)
>  
>  	bttv_call_all(btv, core, ioctl, SAA6588_CMD_CLOSE, &cmd);
>  
> -	if (btv->radio_user == 0)
> +	if (btv->radio_user == 0) {
>  		btv->has_radio_tuner = 0;
> +		audio_mute(btv, 1);
> +	}

The same here, change to

            if (!btv->users && !btv->radio_user)        ?

Regards,
Frank

>  	return 0;
>  }
>  

Re: [RFC PATCH 2/2] bttv: fix audio mute on device close for the radio device node

[Hans Verkuil]

On Sun March 10 2013 12:40:35 Frank Schäfer wrote:
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/pci/bt8xx/bttv-driver.c |    5 ++++-
>  1 Datei geändert, 4 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
> 
> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> index 2c09bc5..74977f7 100644
> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> @@ -3227,6 +3227,7 @@ static int radio_open(struct file *file)
>  	v4l2_fh_init(&fh->fh, vdev);
>  
>  	btv->radio_user++;
> +	audio_mute(btv, btv->mute);
>  
>  	v4l2_fh_add(&fh->fh);
>  
> @@ -3248,8 +3249,10 @@ static int radio_release(struct file *file)
>  
>  	bttv_call_all(btv, core, ioctl, SAA6588_CMD_CLOSE, &cmd);
>  
> -	if (btv->radio_user == 0)
> +	if (btv->radio_user == 0) {
>  		btv->has_radio_tuner = 0;
> +		audio_mute(btv, 1);
> +	}
>  	return 0;
>  }
>  
> 

Sorry, but this isn't right.

You should be able to just set the radio to a frequency and then exit. Since
most cards have an audio out that loops to an audio input you don't want to
have to keep the radio device open.

Audio should be muted when the module is unloaded, though.

The relationship between TV and radio tuners was discussed last year. The
following proposal was accepted:

------- start -----------
How to handle tuner ownership if both a video and radio node share the same
tuner?

Calling S_FREQ, S_TUNER, S_MODULATOR or S_HW_FREQ_SEEK will make the filehandle
the owner if possible. EBUSY is returned if someone else owns the tuner and you
would need to switch the tuner mode.

Ditto for ioctls that expect a valid tuner configuration like QUERYSTD. This is
likely to be driver dependent, though. Just opening a device node should not
switch ownership.

G_FREQUENCY: should just return the last set frequency for radio or TV: requires
that that is remembered when switching ownership. This is what happens today, so
G_FREQUENCY does not have to switch ownership.

G_TUNER: the rxsubchans, signal and afc fields all require ownership of the tuner.
So in principle you would want to switch ownership when G_TUNER is called. On the
other hand, that would mean that calling v4l2-ctl --all -d /dev/radio0 would change
tuner ownership to radio for /dev/video0. That's rather unexpected.

So just set rxsubchans, signal and afc to 0 if the device node doesn't own the tuner.

Closing a device node should not switch ownership. E.g. if nobody has a radio device
open, should the tuner switch back to TV mode automatically? The answer is that it
shouldn't.

How about hybrid tuners? The code to handle tuner ownership should be shared between
DVB and V4L2.
----------- end --------------

All very nice, but nobody had the chance to actually work on this.

But this is how it should work.

Regards,

	Hans

Re: [RFC PATCH 2/2] bttv: fix audio mute on device close for the radio device node

[Frank Schäfer]

Am 10.03.2013 12:59, schrieb Hans Verkuil:
> On Sun March 10 2013 12:40:35 Frank Schäfer wrote:
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>  drivers/media/pci/bt8xx/bttv-driver.c |    5 ++++-
>>  1 Datei geändert, 4 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
>>
>> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
>> index 2c09bc5..74977f7 100644
>> --- a/drivers/media/pci/bt8xx/bttv-driver.c
>> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
>> @@ -3227,6 +3227,7 @@ static int radio_open(struct file *file)
>>  	v4l2_fh_init(&fh->fh, vdev);
>>  
>>  	btv->radio_user++;
>> +	audio_mute(btv, btv->mute);
>>  
>>  	v4l2_fh_add(&fh->fh);
>>  
>> @@ -3248,8 +3249,10 @@ static int radio_release(struct file *file)
>>  
>>  	bttv_call_all(btv, core, ioctl, SAA6588_CMD_CLOSE, &cmd);
>>  
>> -	if (btv->radio_user == 0)
>> +	if (btv->radio_user == 0) {
>>  		btv->has_radio_tuner = 0;
>> +		audio_mute(btv, 1);
>> +	}
>>  	return 0;
>>  }
>>  
>>
> Sorry, but this isn't right.
>
> You should be able to just set the radio to a frequency and then exit. Since
> most cards have an audio out that loops to an audio input you don't want to
> have to keep the radio device open.

Ok, so I will drop this patch.

AFAICS the above said also applies to the video device part, so it's
still not clear to me why both devices should be handled differently.
Anyway, I will regard it as a kind of "tradition".

>
> Audio should be muted when the module is unloaded, though.
>
> The relationship between TV and radio tuners was discussed last year. The
> following proposal was accepted:
>
> ------- start -----------
> How to handle tuner ownership if both a video and radio node share the same
> tuner?
>
> Calling S_FREQ, S_TUNER, S_MODULATOR or S_HW_FREQ_SEEK will make the filehandle
> the owner if possible. EBUSY is returned if someone else owns the tuner and you
> would need to switch the tuner mode.
>
> Ditto for ioctls that expect a valid tuner configuration like QUERYSTD. This is
> likely to be driver dependent, though. Just opening a device node should not
> switch ownership.
>
> G_FREQUENCY: should just return the last set frequency for radio or TV: requires
> that that is remembered when switching ownership. This is what happens today, so
> G_FREQUENCY does not have to switch ownership.
>
> G_TUNER: the rxsubchans, signal and afc fields all require ownership of the tuner.
> So in principle you would want to switch ownership when G_TUNER is called. On the
> other hand, that would mean that calling v4l2-ctl --all -d /dev/radio0 would change
> tuner ownership to radio for /dev/video0. That's rather unexpected.
>
> So just set rxsubchans, signal and afc to 0 if the device node doesn't own the tuner.
>
> Closing a device node should not switch ownership. E.g. if nobody has a radio device
> open, should the tuner switch back to TV mode automatically? The answer is that it
> shouldn't.
>
> How about hybrid tuners? The code to handle tuner ownership should be shared between
> DVB and V4L2.
> ----------- end --------------
>
> All very nice, but nobody had the chance to actually work on this.
>
> But this is how it should work.

Interesting, thanks !

Regards,
Frank

>
> Regards,
>
> 	Hans

Re: [RFC PATCH 2/2] bttv: fix audio mute on device close for the radio device node

[Hans Verkuil]

On Sun March 10 2013 14:47:48 Frank Schäfer wrote:
> Am 10.03.2013 12:59, schrieb Hans Verkuil:
> > On Sun March 10 2013 12:40:35 Frank Schäfer wrote:
> >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >> ---
> >>  drivers/media/pci/bt8xx/bttv-driver.c |    5 ++++-
> >>  1 Datei geändert, 4 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
> >>
> >> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> >> index 2c09bc5..74977f7 100644
> >> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> >> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> >> @@ -3227,6 +3227,7 @@ static int radio_open(struct file *file)
> >>  	v4l2_fh_init(&fh->fh, vdev);
> >>  
> >>  	btv->radio_user++;
> >> +	audio_mute(btv, btv->mute);
> >>  
> >>  	v4l2_fh_add(&fh->fh);
> >>  
> >> @@ -3248,8 +3249,10 @@ static int radio_release(struct file *file)
> >>  
> >>  	bttv_call_all(btv, core, ioctl, SAA6588_CMD_CLOSE, &cmd);
> >>  
> >> -	if (btv->radio_user == 0)
> >> +	if (btv->radio_user == 0) {
> >>  		btv->has_radio_tuner = 0;
> >> +		audio_mute(btv, 1);
> >> +	}
> >>  	return 0;
> >>  }
> >>  
> >>
> > Sorry, but this isn't right.
> >
> > You should be able to just set the radio to a frequency and then exit. Since
> > most cards have an audio out that loops to an audio input you don't want to
> > have to keep the radio device open.
> 
> Ok, so I will drop this patch.
> 
> AFAICS the above said also applies to the video device part, so it's
> still not clear to me why both devices should be handled differently.
> Anyway, I will regard it as a kind of "tradition".

It is legacy. I doubt we would design it like that today. Also note that
there is generally little point in just listening to TV without actually
watching it (although some people do :-) ), so it makes sense to mute the
audio when you stop watching TV.

But radio has traditionally been implemented this way and we have to keep
that.

Regards,

	Hans

> 
> >
> > Audio should be muted when the module is unloaded, though.
> >
> > The relationship between TV and radio tuners was discussed last year. The
> > following proposal was accepted:
> >
> > ------- start -----------
> > How to handle tuner ownership if both a video and radio node share the same
> > tuner?
> >
> > Calling S_FREQ, S_TUNER, S_MODULATOR or S_HW_FREQ_SEEK will make the filehandle
> > the owner if possible. EBUSY is returned if someone else owns the tuner and you
> > would need to switch the tuner mode.
> >
> > Ditto for ioctls that expect a valid tuner configuration like QUERYSTD. This is
> > likely to be driver dependent, though. Just opening a device node should not
> > switch ownership.
> >
> > G_FREQUENCY: should just return the last set frequency for radio or TV: requires
> > that that is remembered when switching ownership. This is what happens today, so
> > G_FREQUENCY does not have to switch ownership.
> >
> > G_TUNER: the rxsubchans, signal and afc fields all require ownership of the tuner.
> > So in principle you would want to switch ownership when G_TUNER is called. On the
> > other hand, that would mean that calling v4l2-ctl --all -d /dev/radio0 would change
> > tuner ownership to radio for /dev/video0. That's rather unexpected.
> >
> > So just set rxsubchans, signal and afc to 0 if the device node doesn't own the tuner.
> >
> > Closing a device node should not switch ownership. E.g. if nobody has a radio device
> > open, should the tuner switch back to TV mode automatically? The answer is that it
> > shouldn't.
> >
> > How about hybrid tuners? The code to handle tuner ownership should be shared between
> > DVB and V4L2.
> > ----------- end --------------
> >
> > All very nice, but nobody had the chance to actually work on this.
> >
> > But this is how it should work.
> 
> Interesting, thanks !
> 
> Regards,
> Frank
> 
> >
> > Regards,
> >
> > 	Hans
> 
> --
> 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
> 

Re: [RFC PATCH 2/2] bttv: fix audio mute on device close for the radio device node

[Frank Schäfer]

Am 10.03.2013 14:56, schrieb Hans Verkuil:
> On Sun March 10 2013 14:47:48 Frank Schäfer wrote:
>> Am 10.03.2013 12:59, schrieb Hans Verkuil:
>>> On Sun March 10 2013 12:40:35 Frank Schäfer wrote:
>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>> ---
>>>>  drivers/media/pci/bt8xx/bttv-driver.c |    5 ++++-
>>>>  1 Datei geändert, 4 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
>>>>
>>>> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
>>>> index 2c09bc5..74977f7 100644
>>>> --- a/drivers/media/pci/bt8xx/bttv-driver.c
>>>> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
>>>> @@ -3227,6 +3227,7 @@ static int radio_open(struct file *file)
>>>>  	v4l2_fh_init(&fh->fh, vdev);
>>>>  
>>>>  	btv->radio_user++;
>>>> +	audio_mute(btv, btv->mute);
>>>>  
>>>>  	v4l2_fh_add(&fh->fh);
>>>>  
>>>> @@ -3248,8 +3249,10 @@ static int radio_release(struct file *file)
>>>>  
>>>>  	bttv_call_all(btv, core, ioctl, SAA6588_CMD_CLOSE, &cmd);
>>>>  
>>>> -	if (btv->radio_user == 0)
>>>> +	if (btv->radio_user == 0) {
>>>>  		btv->has_radio_tuner = 0;
>>>> +		audio_mute(btv, 1);
>>>> +	}
>>>>  	return 0;
>>>>  }
>>>>  
>>>>
>>> Sorry, but this isn't right.
>>>
>>> You should be able to just set the radio to a frequency and then exit. Since
>>> most cards have an audio out that loops to an audio input you don't want to
>>> have to keep the radio device open.
>> Ok, so I will drop this patch.
>>
>> AFAICS the above said also applies to the video device part, so it's
>> still not clear to me why both devices should be handled differently.
>> Anyway, I will regard it as a kind of "tradition".
> It is legacy. I doubt we would design it like that today. Also note that
> there is generally little point in just listening to TV without actually
> watching it (although some people do :-) ), so it makes sense to mute the
> audio when you stop watching TV.
>
> But radio has traditionally been implemented this way and we have to keep
> that.
>
> Regards,
>
> 	Hans
>

[...]

Am 10.03.2013 14:58, schrieb Mauro Carvalho Chehab:
...
> It is not tradition. Changing it would break userspace. For example, xawtv's
> "radio" program expects this behavior (see the "-q" and "-m" parameters).
>
> A typical radio usage is to do:
>
> 	$ radio -f 93.7 -q
>
> And when user is done listening to radio:
>
> 	$ radio -m
>
> Of course, for the above to work, the user needs to have a board wired into
> the audio device, or with an speaker directly connected into it.
>
> Regards,
> Mauro

Fair enough, thanks for your explanations.
Please excuse my questions about issues like this.
Without knowing the history and the reasons for the initial design
decisions it's sometimes difficult to understand why things are working
as they are and if they need to be fixed.

Regards,
Frank

Re: [RFC PATCH 2/2] bttv: fix audio mute on device close for the radio device node

[Hans Verkuil]

On Sun March 10 2013 22:49:33 Frank Schäfer wrote:
> Am 10.03.2013 14:56, schrieb Hans Verkuil:
> > On Sun March 10 2013 14:47:48 Frank Schäfer wrote:
> >> Am 10.03.2013 12:59, schrieb Hans Verkuil:
> >>> On Sun March 10 2013 12:40:35 Frank Schäfer wrote:
> >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >>>> ---
> >>>>  drivers/media/pci/bt8xx/bttv-driver.c |    5 ++++-
> >>>>  1 Datei geändert, 4 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
> >>>>
> >>>> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> >>>> index 2c09bc5..74977f7 100644
> >>>> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> >>>> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> >>>> @@ -3227,6 +3227,7 @@ static int radio_open(struct file *file)
> >>>>  	v4l2_fh_init(&fh->fh, vdev);
> >>>>  
> >>>>  	btv->radio_user++;
> >>>> +	audio_mute(btv, btv->mute);
> >>>>  
> >>>>  	v4l2_fh_add(&fh->fh);
> >>>>  
> >>>> @@ -3248,8 +3249,10 @@ static int radio_release(struct file *file)
> >>>>  
> >>>>  	bttv_call_all(btv, core, ioctl, SAA6588_CMD_CLOSE, &cmd);
> >>>>  
> >>>> -	if (btv->radio_user == 0)
> >>>> +	if (btv->radio_user == 0) {
> >>>>  		btv->has_radio_tuner = 0;
> >>>> +		audio_mute(btv, 1);
> >>>> +	}
> >>>>  	return 0;
> >>>>  }
> >>>>  
> >>>>
> >>> Sorry, but this isn't right.
> >>>
> >>> You should be able to just set the radio to a frequency and then exit. Since
> >>> most cards have an audio out that loops to an audio input you don't want to
> >>> have to keep the radio device open.
> >> Ok, so I will drop this patch.
> >>
> >> AFAICS the above said also applies to the video device part, so it's
> >> still not clear to me why both devices should be handled differently.
> >> Anyway, I will regard it as a kind of "tradition".
> > It is legacy. I doubt we would design it like that today. Also note that
> > there is generally little point in just listening to TV without actually
> > watching it (although some people do :-) ), so it makes sense to mute the
> > audio when you stop watching TV.
> >
> > But radio has traditionally been implemented this way and we have to keep
> > that.
> >
> > Regards,
> >
> > 	Hans
> >
> 
> [...]
> 
> Am 10.03.2013 14:58, schrieb Mauro Carvalho Chehab:
> ...
> > It is not tradition. Changing it would break userspace. For example, xawtv's
> > "radio" program expects this behavior (see the "-q" and "-m" parameters).
> >
> > A typical radio usage is to do:
> >
> > 	$ radio -f 93.7 -q
> >
> > And when user is done listening to radio:
> >
> > 	$ radio -m
> >
> > Of course, for the above to work, the user needs to have a board wired into
> > the audio device, or with an speaker directly connected into it.
> >
> > Regards,
> > Mauro
> 
> Fair enough, thanks for your explanations.
> Please excuse my questions about issues like this.

No problem, just ask!

> Without knowing the history and the reasons for the initial design
> decisions it's sometimes difficult to understand why things are working
> as they are and if they need to be fixed.

It's not helped in the case of bttv by particularly obscure and convoluted
code with regards to audio muting and volume handling, as you have discovered
by now :-)

I'll see if I can review your patch series this week. If possible I'd like
to do some testing as well.

Regards,

	Hans

Re: [RFC PATCH 2/2] bttv: fix audio mute on device close for the radio device node

[Mauro Carvalho Chehab]

Em Sun, 10 Mar 2013 14:47:48 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 10.03.2013 12:59, schrieb Hans Verkuil:
> > On Sun March 10 2013 12:40:35 Frank Schäfer wrote:
> >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >> ---
> >>  drivers/media/pci/bt8xx/bttv-driver.c |    5 ++++-
> >>  1 Datei geändert, 4 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
> >>
> >> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> >> index 2c09bc5..74977f7 100644
> >> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> >> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> >> @@ -3227,6 +3227,7 @@ static int radio_open(struct file *file)
> >>  	v4l2_fh_init(&fh->fh, vdev);
> >>  
> >>  	btv->radio_user++;
> >> +	audio_mute(btv, btv->mute);
> >>  
> >>  	v4l2_fh_add(&fh->fh);
> >>  
> >> @@ -3248,8 +3249,10 @@ static int radio_release(struct file *file)
> >>  
> >>  	bttv_call_all(btv, core, ioctl, SAA6588_CMD_CLOSE, &cmd);
> >>  
> >> -	if (btv->radio_user == 0)
> >> +	if (btv->radio_user == 0) {
> >>  		btv->has_radio_tuner = 0;
> >> +		audio_mute(btv, 1);
> >> +	}
> >>  	return 0;
> >>  }
> >>  
> >>
> > Sorry, but this isn't right.
> >
> > You should be able to just set the radio to a frequency and then exit. Since
> > most cards have an audio out that loops to an audio input you don't want to
> > have to keep the radio device open.
> 
> Ok, so I will drop this patch.
> 
> AFAICS the above said also applies to the video device part, so it's
> still not clear to me why both devices should be handled differently.
> Anyway, I will regard it as a kind of "tradition".

It is not tradition. Changing it would break userspace. For example, xawtv's
"radio" program expects this behavior (see the "-q" and "-m" parameters).

A typical radio usage is to do:

	$ radio -f 93.7 -q

And when user is done listening to radio:

	$ radio -m

Of course, for the above to work, the user needs to have a board wired into
the audio device, or with an speaker directly connected into it.

Regards,
Mauro

Re: [RFC PATCH 1/2] bttv: fix audio mute on device close for the video device node

[Frank Schäfer]

Am 10.03.2013 12:40, schrieb Frank Schäfer:
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/pci/bt8xx/bttv-driver.c |   22 +++++++++++-----------
>  1 Datei geändert, 11 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
>
> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> index 8610b6a..2c09bc5 100644
> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> @@ -992,21 +992,20 @@ static char *audio_modes[] = {
>  static int
>  audio_mux(struct bttv *btv, int input, int mute)
>  {
> -	int gpio_val, signal;
> +	int gpio_val, signal, mute_gpio;
>  	struct v4l2_ctrl *ctrl;
>  
>  	gpio_inout(bttv_tvcards[btv->c.type].gpiomask,
>  		   bttv_tvcards[btv->c.type].gpiomask);
>  	signal = btread(BT848_DSTATUS) & BT848_DSTATUS_HLOC;
>  
> -	btv->mute = mute;
>  	btv->audio = input;
>  
>  	/* automute */
> -	mute = mute || (btv->opt_automute && (!signal || !btv->users)
> +	mute_gpio = mute || (btv->opt_automute && (!signal || !btv->users)
>  				&& !btv->has_radio_tuner);
>  
> -	if (mute)
> +	if (mute_gpio)
>  		gpio_val = bttv_tvcards[btv->c.type].gpiomute;
>  	else
>  		gpio_val = bttv_tvcards[btv->c.type].gpiomux[input];
> @@ -1022,7 +1021,7 @@ audio_mux(struct bttv *btv, int input, int mute)
>  	}
>  
>  	if (bttv_gpio)
> -		bttv_gpio_tracking(btv, audio_modes[mute ? 4 : input]);
> +		bttv_gpio_tracking(btv, audio_modes[mute_gpio ? 4 : input]);
>  	if (in_interrupt())
>  		return 0;
>  
> @@ -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);
> @@ -3062,8 +3062,7 @@ static int bttv_open(struct file *file)
>  			    sizeof(struct bttv_buffer),
>  			    fh, &btv->lock);
>  	set_tvnorm(btv,btv->tvnorm);
> -	set_input(btv, btv->input, btv->tvnorm);
> -
> +	set_input(btv, btv->input, btv->tvnorm); /* also (un)mutes audio */
>  
>  	/* The V4L2 spec requires one global set of cropping parameters
>  	   which only change on request. These are stored in btv->crop[1].
> @@ -3124,7 +3123,7 @@ static int bttv_release(struct file *file)
>  	bttv_field_count(btv);
>  
>  	if (!btv->users)
> -		audio_mute(btv, btv->mute);
> +		audio_mute(btv, 1);
>  

Btw, what about the interaction between the video and the radio device
node ?
With the current code it is possible to open the video and the radio
device node at the same time, so I wonder if we need an additional patch
changing this to

                if (!btv->users && !btv->radio_user)


Regards,
Frank

>  	v4l2_fh_del(&fh->fh);
>  	v4l2_fh_exit(&fh->fh);
> @@ -4209,6 +4208,7 @@ static int bttv_probe(struct pci_dev *dev, const struct pci_device_id *pci_id)
>  	btv->std = V4L2_STD_PAL;
>  	init_irqreg(btv);
>  	v4l2_ctrl_handler_setup(hdl);
> +	audio_mute(btv, 1);
>  
>  	if (hdl->error) {
>  		result = hdl->error;

Re: [RFC PATCH 1/2] bttv: fix audio mute on device close for the video device node

[Hans Verkuil]

On Sun March 10 2013 12:40:34 Frank Schäfer wrote:
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>

Could you describe exactly what bug is fixed? I tested it a fair amount
when I created my bttv patches, so I'd like to know what I missed.

Regards,

	Hans

> ---
>  drivers/media/pci/bt8xx/bttv-driver.c |   22 +++++++++++-----------
>  1 Datei geändert, 11 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
> 
> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> index 8610b6a..2c09bc5 100644
> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> @@ -992,21 +992,20 @@ static char *audio_modes[] = {
>  static int
>  audio_mux(struct bttv *btv, int input, int mute)
>  {
> -	int gpio_val, signal;
> +	int gpio_val, signal, mute_gpio;
>  	struct v4l2_ctrl *ctrl;
>  
>  	gpio_inout(bttv_tvcards[btv->c.type].gpiomask,
>  		   bttv_tvcards[btv->c.type].gpiomask);
>  	signal = btread(BT848_DSTATUS) & BT848_DSTATUS_HLOC;
>  
> -	btv->mute = mute;
>  	btv->audio = input;
>  
>  	/* automute */
> -	mute = mute || (btv->opt_automute && (!signal || !btv->users)
> +	mute_gpio = mute || (btv->opt_automute && (!signal || !btv->users)
>  				&& !btv->has_radio_tuner);
>  
> -	if (mute)
> +	if (mute_gpio)
>  		gpio_val = bttv_tvcards[btv->c.type].gpiomute;
>  	else
>  		gpio_val = bttv_tvcards[btv->c.type].gpiomux[input];
> @@ -1022,7 +1021,7 @@ audio_mux(struct bttv *btv, int input, int mute)
>  	}
>  
>  	if (bttv_gpio)
> -		bttv_gpio_tracking(btv, audio_modes[mute ? 4 : input]);
> +		bttv_gpio_tracking(btv, audio_modes[mute_gpio ? 4 : input]);
>  	if (in_interrupt())
>  		return 0;
>  
> @@ -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);
> @@ -3062,8 +3062,7 @@ static int bttv_open(struct file *file)
>  			    sizeof(struct bttv_buffer),
>  			    fh, &btv->lock);
>  	set_tvnorm(btv,btv->tvnorm);
> -	set_input(btv, btv->input, btv->tvnorm);
> -
> +	set_input(btv, btv->input, btv->tvnorm); /* also (un)mutes audio */
>  
>  	/* The V4L2 spec requires one global set of cropping parameters
>  	   which only change on request. These are stored in btv->crop[1].
> @@ -3124,7 +3123,7 @@ static int bttv_release(struct file *file)
>  	bttv_field_count(btv);
>  
>  	if (!btv->users)
> -		audio_mute(btv, btv->mute);
> +		audio_mute(btv, 1);
>  
>  	v4l2_fh_del(&fh->fh);
>  	v4l2_fh_exit(&fh->fh);
> @@ -4209,6 +4208,7 @@ static int bttv_probe(struct pci_dev *dev, const struct pci_device_id *pci_id)
>  	btv->std = V4L2_STD_PAL;
>  	init_irqreg(btv);
>  	v4l2_ctrl_handler_setup(hdl);
> +	audio_mute(btv, 1);
>  
>  	if (hdl->error) {
>  		result = hdl->error;
> 

Re: [RFC PATCH 1/2] bttv: fix audio mute on device close for the video device node

[Frank Schäfer]

Am 10.03.2013 13:00, schrieb Hans Verkuil:
> On Sun March 10 2013 12:40:34 Frank Schäfer wrote:
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> Could you describe exactly what bug is fixed? I tested it a fair amount
> when I created my bttv patches, so I'd like to know what I missed.

Yeah, sure. The whole thing is a bit tricky and I have to admit that the
patch isn't self-explaining enough.

The main issue it fixes is, that audio_mux() is called with the current
value of the mute control instead of mute=1.
So the device is muted on last close only if the mute control was 1.
Fixing this needs changing some other parts of the code, too.

What makes the patch bit complicated is the fact, that there are
actually two types of "mute" handled in function audio_mux():
1) gpio mute
2) subdevice muting
The "automute" feature also does its best to make things complicating.

And finally a follow-up fix is needed to avoid device unmuting already
when the drivers probing function is called (should not happen before
the first open).

Hmm... sorry... I should really have split this into two patches.
I will send an updated patch series with better explanations.

Regards,
Frank

>
> Regards,
>
> 	Hans
>
>> ---
>>  drivers/media/pci/bt8xx/bttv-driver.c |   22 +++++++++++-----------
>>  1 Datei geändert, 11 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
>> index 8610b6a..2c09bc5 100644
>> --- a/drivers/media/pci/bt8xx/bttv-driver.c
>> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
>> @@ -992,21 +992,20 @@ static char *audio_modes[] = {
>>  static int
>>  audio_mux(struct bttv *btv, int input, int mute)
>>  {
>> -	int gpio_val, signal;
>> +	int gpio_val, signal, mute_gpio;
>>  	struct v4l2_ctrl *ctrl;
>>  
>>  	gpio_inout(bttv_tvcards[btv->c.type].gpiomask,
>>  		   bttv_tvcards[btv->c.type].gpiomask);
>>  	signal = btread(BT848_DSTATUS) & BT848_DSTATUS_HLOC;
>>  
>> -	btv->mute = mute;
>>  	btv->audio = input;
>>  
>>  	/* automute */
>> -	mute = mute || (btv->opt_automute && (!signal || !btv->users)
>> +	mute_gpio = mute || (btv->opt_automute && (!signal || !btv->users)
>>  				&& !btv->has_radio_tuner);
>>  
>> -	if (mute)
>> +	if (mute_gpio)
>>  		gpio_val = bttv_tvcards[btv->c.type].gpiomute;
>>  	else
>>  		gpio_val = bttv_tvcards[btv->c.type].gpiomux[input];
>> @@ -1022,7 +1021,7 @@ audio_mux(struct bttv *btv, int input, int mute)
>>  	}
>>  
>>  	if (bttv_gpio)
>> -		bttv_gpio_tracking(btv, audio_modes[mute ? 4 : input]);
>> +		bttv_gpio_tracking(btv, audio_modes[mute_gpio ? 4 : input]);
>>  	if (in_interrupt())
>>  		return 0;
>>  
>> @@ -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);
>> @@ -3062,8 +3062,7 @@ static int bttv_open(struct file *file)
>>  			    sizeof(struct bttv_buffer),
>>  			    fh, &btv->lock);
>>  	set_tvnorm(btv,btv->tvnorm);
>> -	set_input(btv, btv->input, btv->tvnorm);
>> -
>> +	set_input(btv, btv->input, btv->tvnorm); /* also (un)mutes audio */
>>  
>>  	/* The V4L2 spec requires one global set of cropping parameters
>>  	   which only change on request. These are stored in btv->crop[1].
>> @@ -3124,7 +3123,7 @@ static int bttv_release(struct file *file)
>>  	bttv_field_count(btv);
>>  
>>  	if (!btv->users)
>> -		audio_mute(btv, btv->mute);
>> +		audio_mute(btv, 1);
>>  
>>  	v4l2_fh_del(&fh->fh);
>>  	v4l2_fh_exit(&fh->fh);
>> @@ -4209,6 +4208,7 @@ static int bttv_probe(struct pci_dev *dev, const struct pci_device_id *pci_id)
>>  	btv->std = V4L2_STD_PAL;
>>  	init_irqreg(btv);
>>  	v4l2_ctrl_handler_setup(hdl);
>> +	audio_mute(btv, 1);
>>  
>>  	if (hdl->error) {
>>  		result = hdl->error;
>>