Re: [PATCH 4/4] snd-usb-6fire: add analog input volume control

Re: [PATCH 4/4] snd-usb-6fire: add analog input volume control

[Torsten Schenk]

> >  static struct __devinitdata snd_kcontrol_new elements[] = {
> > @@ -442,6 +497,15 @@
> >  		.get = usb6fire_control_digital_thru_get,
> >  		.put = usb6fire_control_digital_thru_put
> >  	},
> > +	{
> > +		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > +		.name = "Input 1/2 Capture Volume",
> > +		.index = 0,
> 
> If this is the only volume element, you don't need to add "1/2", no?

The problem is, that there is only an input control for 2 out of 4
analog input channels since channel 3 and 4 use a different ADC and
there doesn't seem to be a volume control on that chip. Since this
control only changes volume of channel 1 and 2, I wanted to have an
indication about that.

Thanks,
Torsten

Re: [PATCH 4/4] snd-usb-6fire: add analog input volume control

[Takashi Iwai]

At Thu, 12 Jan 2012 10:17:24 +0100,
Torsten Schenk wrote:
> 
> > >  static struct __devinitdata snd_kcontrol_new elements[] = {
> > > @@ -442,6 +497,15 @@
> > >  		.get = usb6fire_control_digital_thru_get,
> > >  		.put = usb6fire_control_digital_thru_put
> > >  	},
> > > +	{
> > > +		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > > +		.name = "Input 1/2 Capture Volume",
> > > +		.index = 0,
> > 
> > If this is the only volume element, you don't need to add "1/2", no?
> 
> The problem is, that there is only an input control for 2 out of 4
> analog input channels since channel 3 and 4 use a different ADC and
> there doesn't seem to be a volume control on that chip. Since this
> control only changes volume of channel 1 and 2, I wanted to have an
> indication about that.

Well, then the question is whether you need to give a stereo control
there.  If all I/O are supposed to be individual mono streams, it'd be
natural to provide a mono volume control with a proper index number
corresponding to each stream.


Takashi

Re: [PATCH 4/4] snd-usb-6fire: add analog input volume control

[Torsten Schenk]

On Thu, 12 Jan 2012 10:28:01 +0100
Takashi Iwai <tiwai@suse.de> wrote:

> At Thu, 12 Jan 2012 10:17:24 +0100,
> Torsten Schenk wrote:
> > 
> > > >  static struct __devinitdata snd_kcontrol_new elements[] = {
> > > > @@ -442,6 +497,15 @@
> > > >  		.get = usb6fire_control_digital_thru_get,
> > > >  		.put = usb6fire_control_digital_thru_put
> > > >  	},
> > > > +	{
> > > > +		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > > > +		.name = "Input 1/2 Capture Volume",
> > > > +		.index = 0,
> > > 
> > > If this is the only volume element, you don't need to add "1/2",
> > > no?
> > 
> > The problem is, that there is only an input control for 2 out of 4
> > analog input channels since channel 3 and 4 use a different ADC and
> > there doesn't seem to be a volume control on that chip. Since this
> > control only changes volume of channel 1 and 2, I wanted to have an
> > indication about that.
> 
> Well, then the question is whether you need to give a stereo control
> there.  If all I/O are supposed to be individual mono streams, it'd be
> natural to provide a mono volume control with a proper index number
> corresponding to each stream.

In fact they are paired up into stereo channels but there is no
explicit name since the inputs have different connectors,
depending on which plug is actually connected. Input 1/2 has two
cinch connectors, a microphone connector and a guitar/bass connector.
Input 3/4 has two cinch connectors (line) and two cinch connectors
(phono). Therefore I think it's not useful to separate these
into mono channels.

Torsten

[PATCH 4/4] snd-usb-6fire: add analog input volume control

[Torsten Schenk]

- Add a stereo volume control for analog input channel pair 1/2.
- Add missing end-of-list entry at end of list for array mute_elements
  and vol_elements

Signed-off-by: Torsten Schenk <torsten.schenk@zoho.com>

diff -Nur a/sound/usb/6fire/control.c b/sound/usb/6fire/control.c
--- a/sound/usb/6fire/control.c	2012-01-12 00:19:40.680844042 +0100
+++ b/sound/usb/6fire/control.c	2012-01-12 00:23:21.013835655 +0100
@@ -76,6 +76,17 @@
 		comm_rt->write8(comm_rt, 0x12, 0x0e, ~rt->output_mute);
 }
 
+static void usb6fire_control_input_vol_update(struct control_runtime *rt)
+{
+	struct comm_runtime *comm_rt = rt->chip->comm;
+	int i;
+
+	if (comm_rt)
+		for (i = 0; i < 2; i++)
+			comm_rt->write8(comm_rt, 0x12, 0x1c + i,
+				rt->input_vol[i] & 0x3f);
+}
+
 static void usb6fire_control_line_phono_update(struct control_runtime *rt)
 {
 	struct comm_runtime *comm_rt = rt->chip->comm;
@@ -257,6 +268,48 @@
 	return 0;
 }
 
+static int usb6fire_control_input_vol_info(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_info *uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 2;
+	uinfo->value.integer.min = -15;
+	uinfo->value.integer.max = 15;
+	return 0;
+}
+
+static int usb6fire_control_input_vol_put(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct control_runtime *rt = snd_kcontrol_chip(kcontrol);
+	int changed = 0;
+
+	if (rt->input_vol[0] != ucontrol->value.integer.value[0]) {
+		rt->input_vol[0] = ucontrol->value.integer.value[0];
+		changed = 1;
+	}
+	if (rt->input_vol[1] != ucontrol->value.integer.value[1]) {
+		rt->input_vol[1] = ucontrol->value.integer.value[1];
+		changed = 1;
+	}
+
+	if (changed)
+		usb6fire_control_input_vol_update(rt);
+
+	return changed;
+}
+
+static int usb6fire_control_input_vol_get(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct control_runtime *rt = snd_kcontrol_chip(kcontrol);
+
+	ucontrol->value.integer.value[0] = rt->input_vol[0];
+	ucontrol->value.integer.value[1] = rt->input_vol[1];
+
+	return 0;
+}
+
 static int usb6fire_control_line_phono_info(struct snd_kcontrol *kcontrol,
 		struct snd_ctl_elem_info *uinfo)
 {
@@ -378,7 +431,8 @@
 		.get = usb6fire_control_output_vol_get,
 		.put = usb6fire_control_output_vol_put,
 		.private_value = 2
-	}
+	},
+	{}
 };
 
 static struct __devinitdata snd_kcontrol_new mute_elements[] = {
@@ -411,7 +465,8 @@
 		.get = usb6fire_control_output_mute_get,
 		.put = usb6fire_control_output_mute_put,
 		.private_value = 2
-	}
+	},
+	{}
 };
 
 static struct __devinitdata snd_kcontrol_new elements[] = {
@@ -442,6 +497,15 @@
 		.get = usb6fire_control_digital_thru_get,
 		.put = usb6fire_control_digital_thru_put
 	},
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "Input 1/2 Capture Volume",
+		.index = 0,
+		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+		.info = usb6fire_control_input_vol_info,
+		.get = usb6fire_control_input_vol_get,
+		.put = usb6fire_control_input_vol_put
+	},
 	{}
 };
 
@@ -505,6 +569,7 @@
 	usb6fire_control_line_phono_update(rt);
 	usb6fire_control_output_vol_update(rt);
 	usb6fire_control_output_mute_update(rt);
+	usb6fire_control_input_vol_update(rt);
 	usb6fire_control_streaming_update(rt);
 
 	ret = usb6fire_control_add_virtual(rt, chip->card,
diff -Nur a/sound/usb/6fire/control.h b/sound/usb/6fire/control.h
--- a/sound/usb/6fire/control.h	2012-01-11 19:54:50.651190007 +0100
+++ b/sound/usb/6fire/control.h	2012-01-11 23:44:43.212950616 +0100
@@ -45,6 +45,7 @@
 	bool usb_streaming;
 	u8 output_vol[6];
 	u8 output_mute;
+	s8 input_vol[2];
 };
 
 int __devinit usb6fire_control_init(struct sfire_chip *chip);

Re: [PATCH 4/4] snd-usb-6fire: add analog input volume control

[Takashi Iwai]

At Thu, 12 Jan 2012 00:26:26 +0100,
Torsten Schenk wrote:
> 
> +static int usb6fire_control_input_vol_info(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_info *uinfo)
> +{
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> +	uinfo->count = 2;
> +	uinfo->value.integer.min = -15;
> +	uinfo->value.integer.max = 15;

Some apps might not work with a negative minimum value.
If you want to be conservative, shift up to zero and adjust later.

The actual dB range should be given anyway via additional TLV, thus
you don't have to stick with the raw value too much.


>  static struct __devinitdata snd_kcontrol_new elements[] = {
> @@ -442,6 +497,15 @@
>  		.get = usb6fire_control_digital_thru_get,
>  		.put = usb6fire_control_digital_thru_put
>  	},
> +	{
> +		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +		.name = "Input 1/2 Capture Volume",
> +		.index = 0,

If this is the only volume element, you don't need to add "1/2", no?


thanks,

Takashi