* [PATCH] ALSA: usb-audio: Fix parameter block size for UAC2 control requests
@ 2015-08-13 11:04 Julian Scheel
2015-08-14 13:17 ` Takashi Iwai
0 siblings, 1 reply; 5+ messages in thread
From: Julian Scheel @ 2015-08-13 11:04 UTC (permalink / raw)
To: alsa-devel, tiwai; +Cc: Julian Scheel
USB Audio Class version 2.0 supports three different parameter block sizes for
CUR requests, which are 1 byte (5.2.3.1 Layout 1 Parameter Block), 2 bytes
(5.2.3.2 Layout 2 Parameter Block) and 4 bytes (5.2.3.3 Layout 3 Parameter
Block). Use the correct size according to the specific control as it was
already done for UACv1. The allocated block size for control requests is
increased to support the 4 byte worst case.
Signed-off-by: Julian Scheel <julian@jusst.de>
---
sound/usb/mixer.c | 83 +++++++++++++++++++++++++++++++++++++++++--------------
sound/usb/mixer.h | 2 ++
2 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 4a49944..675725b 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -233,6 +233,14 @@ static int convert_signed_value(struct usb_mixer_elem_info *cval, int val)
if (val >= 0x8000)
val -= 0x10000;
break;
+ case USB_MIXER_U32:
+ val &= 0xffffffff;
+ break;
+ case USB_MIXER_S32:
+ val &= 0xffffffff;
+ if (val >= 0x80000000)
+ val -= 0x100000000;
+ break;
}
return val;
}
@@ -253,6 +261,9 @@ static int convert_bytes_value(struct usb_mixer_elem_info *cval, int val)
case USB_MIXER_S16:
case USB_MIXER_U16:
return val & 0xffff;
+ case USB_MIXER_S32:
+ case USB_MIXER_U32:
+ return val & 0xffffffff;
}
return 0; /* not reached */
}
@@ -328,14 +339,26 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request,
int validx, int *value_ret)
{
struct snd_usb_audio *chip = cval->head.mixer->chip;
- unsigned char buf[2 + 3 * sizeof(__u16)]; /* enough space for one range */
+ unsigned char buf[4 + 3 * sizeof(__u32)]; /* enough space for one range */
unsigned char *val;
int idx = 0, ret, size;
__u8 bRequest;
if (request == UAC_GET_CUR) {
bRequest = UAC2_CS_CUR;
- size = sizeof(__u16);
+ switch (cval->val_type) {
+ case USB_MIXER_S32:
+ case USB_MIXER_U32:
+ size = 4;
+ break;
+ case USB_MIXER_S16:
+ case USB_MIXER_U16:
+ size = 2;
+ break;
+ default:
+ size = 1;
+ break;
+ }
} else {
bRequest = UAC2_CS_RANGE;
size = sizeof(buf);
@@ -446,7 +469,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
int request, int validx, int value_set)
{
struct snd_usb_audio *chip = cval->head.mixer->chip;
- unsigned char buf[2];
+ unsigned char buf[4];
int idx = 0, val_len, err, timeout = 10;
validx += cval->idx_off;
@@ -454,8 +477,19 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
if (cval->head.mixer->protocol == UAC_VERSION_1) {
val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1;
} else { /* UAC_VERSION_2 */
- /* audio class v2 controls are always 2 bytes in size */
- val_len = sizeof(__u16);
+ switch (cval->val_type) {
+ case USB_MIXER_S32:
+ case USB_MIXER_U32:
+ val_len = 4;
+ break;
+ case USB_MIXER_S16:
+ case USB_MIXER_U16:
+ val_len = 2;
+ break;
+ default:
+ val_len = 1;
+ break;
+ }
/* FIXME */
if (request != UAC_SET_CUR) {
@@ -469,6 +503,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
value_set = convert_bytes_value(cval, value_set);
buf[0] = value_set & 0xff;
buf[1] = (value_set >> 8) & 0xff;
+ buf[2] = (value_set >> 16) & 0xff;
+ buf[3] = (value_set >> 24) & 0xff;
err = snd_usb_autoresume(chip);
if (err < 0)
return -EIO;
@@ -799,24 +835,25 @@ static int check_input_term(struct mixer_build *state, int id,
/* feature unit control information */
struct usb_feature_control_info {
const char *name;
- unsigned int type; /* control type (mute, volume, etc.) */
+ int type; /* data type for uac1 */
+ int type_uac2; /* data type for uac2 if different from uac1, else -1 */
};
static struct usb_feature_control_info audio_feature_info[] = {
- { "Mute", USB_MIXER_INV_BOOLEAN },
- { "Volume", USB_MIXER_S16 },
- { "Tone Control - Bass", USB_MIXER_S8 },
- { "Tone Control - Mid", USB_MIXER_S8 },
- { "Tone Control - Treble", USB_MIXER_S8 },
- { "Graphic Equalizer", USB_MIXER_S8 }, /* FIXME: not implemeted yet */
- { "Auto Gain Control", USB_MIXER_BOOLEAN },
- { "Delay Control", USB_MIXER_U16 }, /* FIXME: U32 in UAC2 */
- { "Bass Boost", USB_MIXER_BOOLEAN },
- { "Loudness", USB_MIXER_BOOLEAN },
+ { "Mute", USB_MIXER_INV_BOOLEAN, -1 },
+ { "Volume", USB_MIXER_S16, -1 },
+ { "Tone Control - Bass", USB_MIXER_S8, -1 },
+ { "Tone Control - Mid", USB_MIXER_S8, -1 },
+ { "Tone Control - Treble", USB_MIXER_S8, -1 },
+ { "Graphic Equalizer", USB_MIXER_S8, -1 }, /* FIXME: not implemeted yet */
+ { "Auto Gain Control", USB_MIXER_BOOLEAN, -1 },
+ { "Delay Control", USB_MIXER_U16, USB_MIXER_U32 },
+ { "Bass Boost", USB_MIXER_BOOLEAN, -1 },
+ { "Loudness", USB_MIXER_BOOLEAN, -1 },
/* UAC2 specific */
- { "Input Gain Control", USB_MIXER_S16 },
- { "Input Gain Pad Control", USB_MIXER_S16 },
- { "Phase Inverter Control", USB_MIXER_BOOLEAN },
+ { "Input Gain Control", USB_MIXER_S16, -1 },
+ { "Input Gain Pad Control", USB_MIXER_S16, -1 },
+ { "Phase Inverter Control", USB_MIXER_BOOLEAN, -1 },
};
/* private_free callback */
@@ -1241,7 +1278,13 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc,
snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
cval->control = control;
cval->cmask = ctl_mask;
- cval->val_type = audio_feature_info[control-1].type;
+ if (state->mixer->protocol == UAC_VERSION_1)
+ cval->val_type = audio_feature_info[control-1].type;
+ else /* UAC_VERSION_2 */
+ cval->val_type = audio_feature_info[control-1].type_uac2 > -1 ?
+ audio_feature_info[control-1].type_uac2 :
+ audio_feature_info[control-1].type;
+
if (ctl_mask == 0) {
cval->channels = 1; /* master channel */
cval->master_readonly = readonly_mask;
diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
index d3268f0..3417ef3 100644
--- a/sound/usb/mixer.h
+++ b/sound/usb/mixer.h
@@ -33,6 +33,8 @@ enum {
USB_MIXER_U8,
USB_MIXER_S16,
USB_MIXER_U16,
+ USB_MIXER_S32,
+ USB_MIXER_U32,
};
typedef void (*usb_mixer_elem_dump_func_t)(struct snd_info_buffer *buffer,
--
2.5.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ALSA: usb-audio: Fix parameter block size for UAC2 control requests
2015-08-13 11:04 [PATCH] ALSA: usb-audio: Fix parameter block size for UAC2 control requests Julian Scheel
@ 2015-08-14 13:17 ` Takashi Iwai
2015-08-14 13:45 ` Julian Scheel
0 siblings, 1 reply; 5+ messages in thread
From: Takashi Iwai @ 2015-08-14 13:17 UTC (permalink / raw)
To: Julian Scheel; +Cc: alsa-devel
On Thu, 13 Aug 2015 13:04:46 +0200,
Julian Scheel wrote:
>
> USB Audio Class version 2.0 supports three different parameter block sizes for
> CUR requests, which are 1 byte (5.2.3.1 Layout 1 Parameter Block), 2 bytes
> (5.2.3.2 Layout 2 Parameter Block) and 4 bytes (5.2.3.3 Layout 3 Parameter
> Block). Use the correct size according to the specific control as it was
> already done for UACv1. The allocated block size for control requests is
> increased to support the 4 byte worst case.
>
> Signed-off-by: Julian Scheel <julian@jusst.de>
> ---
> sound/usb/mixer.c | 83 +++++++++++++++++++++++++++++++++++++++++--------------
> sound/usb/mixer.h | 2 ++
> 2 files changed, 65 insertions(+), 20 deletions(-)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 4a49944..675725b 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -233,6 +233,14 @@ static int convert_signed_value(struct usb_mixer_elem_info *cval, int val)
> if (val >= 0x8000)
> val -= 0x10000;
> break;
> + case USB_MIXER_U32:
> + val &= 0xffffffff;
> + break;
> + case USB_MIXER_S32:
> + val &= 0xffffffff;
> + if (val >= 0x80000000)
> + val -= 0x100000000;
> + break;
All these are superfluous, as int on Linux is always 32bit.
> }
> return val;
> }
> @@ -253,6 +261,9 @@ static int convert_bytes_value(struct usb_mixer_elem_info *cval, int val)
> case USB_MIXER_S16:
> case USB_MIXER_U16:
> return val & 0xffff;
> + case USB_MIXER_S32:
> + case USB_MIXER_U32:
> + return val & 0xffffffff;
Ditto.
> }
> return 0; /* not reached */
> }
> @@ -328,14 +339,26 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request,
> int validx, int *value_ret)
> {
> struct snd_usb_audio *chip = cval->head.mixer->chip;
> - unsigned char buf[2 + 3 * sizeof(__u16)]; /* enough space for one range */
> + unsigned char buf[4 + 3 * sizeof(__u32)]; /* enough space for one range */
> unsigned char *val;
> int idx = 0, ret, size;
> __u8 bRequest;
>
> if (request == UAC_GET_CUR) {
> bRequest = UAC2_CS_CUR;
> - size = sizeof(__u16);
> + switch (cval->val_type) {
> + case USB_MIXER_S32:
> + case USB_MIXER_U32:
> + size = 4;
> + break;
> + case USB_MIXER_S16:
> + case USB_MIXER_U16:
> + size = 2;
> + break;
> + default:
> + size = 1;
> + break;
Please split this into a small helper function,
e.g. uac2_ctl_value_size(). The same stuff is used in below, too.
> + }
> } else {
> bRequest = UAC2_CS_RANGE;
> size = sizeof(buf);
> @@ -446,7 +469,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
> int request, int validx, int value_set)
> {
> struct snd_usb_audio *chip = cval->head.mixer->chip;
> - unsigned char buf[2];
> + unsigned char buf[4];
> int idx = 0, val_len, err, timeout = 10;
>
> validx += cval->idx_off;
> @@ -454,8 +477,19 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
> if (cval->head.mixer->protocol == UAC_VERSION_1) {
> val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1;
> } else { /* UAC_VERSION_2 */
> - /* audio class v2 controls are always 2 bytes in size */
> - val_len = sizeof(__u16);
> + switch (cval->val_type) {
> + case USB_MIXER_S32:
> + case USB_MIXER_U32:
> + val_len = 4;
> + break;
> + case USB_MIXER_S16:
> + case USB_MIXER_U16:
> + val_len = 2;
> + break;
> + default:
> + val_len = 1;
> + break;
> + }
>
> /* FIXME */
> if (request != UAC_SET_CUR) {
> @@ -469,6 +503,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
> value_set = convert_bytes_value(cval, value_set);
> buf[0] = value_set & 0xff;
> buf[1] = (value_set >> 8) & 0xff;
> + buf[2] = (value_set >> 16) & 0xff;
> + buf[3] = (value_set >> 24) & 0xff;
It's not smart but OK, just a subtle matter...
> err = snd_usb_autoresume(chip);
> if (err < 0)
> return -EIO;
> @@ -799,24 +835,25 @@ static int check_input_term(struct mixer_build *state, int id,
> /* feature unit control information */
> struct usb_feature_control_info {
> const char *name;
> - unsigned int type; /* control type (mute, volume, etc.) */
> + int type; /* data type for uac1 */
> + int type_uac2; /* data type for uac2 if different from uac1, else -1 */
> };
>
> static struct usb_feature_control_info audio_feature_info[] = {
> - { "Mute", USB_MIXER_INV_BOOLEAN },
> - { "Volume", USB_MIXER_S16 },
> - { "Tone Control - Bass", USB_MIXER_S8 },
> - { "Tone Control - Mid", USB_MIXER_S8 },
> - { "Tone Control - Treble", USB_MIXER_S8 },
> - { "Graphic Equalizer", USB_MIXER_S8 }, /* FIXME: not implemeted yet */
> - { "Auto Gain Control", USB_MIXER_BOOLEAN },
> - { "Delay Control", USB_MIXER_U16 }, /* FIXME: U32 in UAC2 */
> - { "Bass Boost", USB_MIXER_BOOLEAN },
> - { "Loudness", USB_MIXER_BOOLEAN },
> + { "Mute", USB_MIXER_INV_BOOLEAN, -1 },
> + { "Volume", USB_MIXER_S16, -1 },
> + { "Tone Control - Bass", USB_MIXER_S8, -1 },
> + { "Tone Control - Mid", USB_MIXER_S8, -1 },
> + { "Tone Control - Treble", USB_MIXER_S8, -1 },
> + { "Graphic Equalizer", USB_MIXER_S8, -1 }, /* FIXME: not implemeted yet */
> + { "Auto Gain Control", USB_MIXER_BOOLEAN, -1 },
> + { "Delay Control", USB_MIXER_U16, USB_MIXER_U32 },
> + { "Bass Boost", USB_MIXER_BOOLEAN, -1 },
> + { "Loudness", USB_MIXER_BOOLEAN, -1 },
> /* UAC2 specific */
> - { "Input Gain Control", USB_MIXER_S16 },
> - { "Input Gain Pad Control", USB_MIXER_S16 },
> - { "Phase Inverter Control", USB_MIXER_BOOLEAN },
> + { "Input Gain Control", USB_MIXER_S16, -1 },
> + { "Input Gain Pad Control", USB_MIXER_S16, -1 },
> + { "Phase Inverter Control", USB_MIXER_BOOLEAN, -1 },
> };
>
> /* private_free callback */
> @@ -1241,7 +1278,13 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc,
> snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
> cval->control = control;
> cval->cmask = ctl_mask;
> - cval->val_type = audio_feature_info[control-1].type;
> + if (state->mixer->protocol == UAC_VERSION_1)
> + cval->val_type = audio_feature_info[control-1].type;
> + else /* UAC_VERSION_2 */
> + cval->val_type = audio_feature_info[control-1].type_uac2 > -1 ?
Usually ">= 0" is used.
> + audio_feature_info[control-1].type_uac2 :
> + audio_feature_info[control-1].type;
Since audio_feature_info[control-1] is referred so many times, it's
better to put in a temporary variable.
thanks,
Takashi
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ALSA: usb-audio: Fix parameter block size for UAC2 control requests
2015-08-14 13:17 ` Takashi Iwai
@ 2015-08-14 13:45 ` Julian Scheel
2015-08-14 13:51 ` Takashi Iwai
0 siblings, 1 reply; 5+ messages in thread
From: Julian Scheel @ 2015-08-14 13:45 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
Am 14.08.2015 um 15:17 schrieb Takashi Iwai:
> On Thu, 13 Aug 2015 13:04:46 +0200,
> Julian Scheel wrote:
>>
>> USB Audio Class version 2.0 supports three different parameter block sizes for
>> CUR requests, which are 1 byte (5.2.3.1 Layout 1 Parameter Block), 2 bytes
>> (5.2.3.2 Layout 2 Parameter Block) and 4 bytes (5.2.3.3 Layout 3 Parameter
>> Block). Use the correct size according to the specific control as it was
>> already done for UACv1. The allocated block size for control requests is
>> increased to support the 4 byte worst case.
>>
>> Signed-off-by: Julian Scheel <julian@jusst.de>
>> ---
>> sound/usb/mixer.c | 83 +++++++++++++++++++++++++++++++++++++++++--------------
>> sound/usb/mixer.h | 2 ++
>> 2 files changed, 65 insertions(+), 20 deletions(-)
>>
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 4a49944..675725b 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -233,6 +233,14 @@ static int convert_signed_value(struct usb_mixer_elem_info *cval, int val)
>> if (val >= 0x8000)
>> val -= 0x10000;
>> break;
>> + case USB_MIXER_U32:
>> + val &= 0xffffffff;
>> + break;
>> + case USB_MIXER_S32:
>> + val &= 0xffffffff;
>> + if (val >= 0x80000000)
>> + val -= 0x100000000;
>> + break;
>
> All these are superfluous, as int on Linux is always 32bit.
Of course. Thanks.
>> }
>> return val;
>> }
>> @@ -253,6 +261,9 @@ static int convert_bytes_value(struct usb_mixer_elem_info *cval, int val)
>> case USB_MIXER_S16:
>> case USB_MIXER_U16:
>> return val & 0xffff;
>> + case USB_MIXER_S32:
>> + case USB_MIXER_U32:
>> + return val & 0xffffffff;
>
> Ditto.
>
>
>> }
>> return 0; /* not reached */
>> }
>> @@ -328,14 +339,26 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request,
>> int validx, int *value_ret)
>> {
>> struct snd_usb_audio *chip = cval->head.mixer->chip;
>> - unsigned char buf[2 + 3 * sizeof(__u16)]; /* enough space for one range */
>> + unsigned char buf[4 + 3 * sizeof(__u32)]; /* enough space for one range */
>> unsigned char *val;
>> int idx = 0, ret, size;
>> __u8 bRequest;
>>
>> if (request == UAC_GET_CUR) {
>> bRequest = UAC2_CS_CUR;
>> - size = sizeof(__u16);
>> + switch (cval->val_type) {
>> + case USB_MIXER_S32:
>> + case USB_MIXER_U32:
>> + size = 4;
>> + break;
>> + case USB_MIXER_S16:
>> + case USB_MIXER_U16:
>> + size = 2;
>> + break;
>> + default:
>> + size = 1;
>> + break;
>
> Please split this into a small helper function,
> e.g. uac2_ctl_value_size(). The same stuff is used in below, too.
Ack.
>> + }
>> } else {
>> bRequest = UAC2_CS_RANGE;
>> size = sizeof(buf);
>> @@ -446,7 +469,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
>> int request, int validx, int value_set)
>> {
>> struct snd_usb_audio *chip = cval->head.mixer->chip;
>> - unsigned char buf[2];
>> + unsigned char buf[4];
>> int idx = 0, val_len, err, timeout = 10;
>>
>> validx += cval->idx_off;
>> @@ -454,8 +477,19 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
>> if (cval->head.mixer->protocol == UAC_VERSION_1) {
>> val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1;
>> } else { /* UAC_VERSION_2 */
>> - /* audio class v2 controls are always 2 bytes in size */
>> - val_len = sizeof(__u16);
>> + switch (cval->val_type) {
>> + case USB_MIXER_S32:
>> + case USB_MIXER_U32:
>> + val_len = 4;
>> + break;
>> + case USB_MIXER_S16:
>> + case USB_MIXER_U16:
>> + val_len = 2;
>> + break;
>> + default:
>> + val_len = 1;
>> + break;
>> + }
>>
>> /* FIXME */
>> if (request != UAC_SET_CUR) {
>> @@ -469,6 +503,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
>> value_set = convert_bytes_value(cval, value_set);
>> buf[0] = value_set & 0xff;
>> buf[1] = (value_set >> 8) & 0xff;
>> + buf[2] = (value_set >> 16) & 0xff;
>> + buf[3] = (value_set >> 24) & 0xff;
>
> It's not smart but OK, just a subtle matter...
Hm, what would you like better?
>> err = snd_usb_autoresume(chip);
>> if (err < 0)
>> return -EIO;
>> @@ -799,24 +835,25 @@ static int check_input_term(struct mixer_build *state, int id,
>> /* feature unit control information */
>> struct usb_feature_control_info {
>> const char *name;
>> - unsigned int type; /* control type (mute, volume, etc.) */
>> + int type; /* data type for uac1 */
>> + int type_uac2; /* data type for uac2 if different from uac1, else -1 */
>> };
>>
>> static struct usb_feature_control_info audio_feature_info[] = {
>> - { "Mute", USB_MIXER_INV_BOOLEAN },
>> - { "Volume", USB_MIXER_S16 },
>> - { "Tone Control - Bass", USB_MIXER_S8 },
>> - { "Tone Control - Mid", USB_MIXER_S8 },
>> - { "Tone Control - Treble", USB_MIXER_S8 },
>> - { "Graphic Equalizer", USB_MIXER_S8 }, /* FIXME: not implemeted yet */
>> - { "Auto Gain Control", USB_MIXER_BOOLEAN },
>> - { "Delay Control", USB_MIXER_U16 }, /* FIXME: U32 in UAC2 */
>> - { "Bass Boost", USB_MIXER_BOOLEAN },
>> - { "Loudness", USB_MIXER_BOOLEAN },
>> + { "Mute", USB_MIXER_INV_BOOLEAN, -1 },
>> + { "Volume", USB_MIXER_S16, -1 },
>> + { "Tone Control - Bass", USB_MIXER_S8, -1 },
>> + { "Tone Control - Mid", USB_MIXER_S8, -1 },
>> + { "Tone Control - Treble", USB_MIXER_S8, -1 },
>> + { "Graphic Equalizer", USB_MIXER_S8, -1 }, /* FIXME: not implemeted yet */
>> + { "Auto Gain Control", USB_MIXER_BOOLEAN, -1 },
>> + { "Delay Control", USB_MIXER_U16, USB_MIXER_U32 },
>> + { "Bass Boost", USB_MIXER_BOOLEAN, -1 },
>> + { "Loudness", USB_MIXER_BOOLEAN, -1 },
>> /* UAC2 specific */
>> - { "Input Gain Control", USB_MIXER_S16 },
>> - { "Input Gain Pad Control", USB_MIXER_S16 },
>> - { "Phase Inverter Control", USB_MIXER_BOOLEAN },
>> + { "Input Gain Control", USB_MIXER_S16, -1 },
>> + { "Input Gain Pad Control", USB_MIXER_S16, -1 },
>> + { "Phase Inverter Control", USB_MIXER_BOOLEAN, -1 },
>> };
>>
>> /* private_free callback */
>> @@ -1241,7 +1278,13 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc,
>> snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
>> cval->control = control;
>> cval->cmask = ctl_mask;
>> - cval->val_type = audio_feature_info[control-1].type;
>> + if (state->mixer->protocol == UAC_VERSION_1)
>> + cval->val_type = audio_feature_info[control-1].type;
>> + else /* UAC_VERSION_2 */
>> + cval->val_type = audio_feature_info[control-1].type_uac2 > -1 ?
>
> Usually ">= 0" is used.
Ack.
>> + audio_feature_info[control-1].type_uac2 :
>> + audio_feature_info[control-1].type;
>
> Since audio_feature_info[control-1] is referred so many times, it's
> better to put in a temporary variable.
Ack.
Thanks for reviewing!
-Julian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ALSA: usb-audio: Fix parameter block size for UAC2 control requests
2015-08-14 13:45 ` Julian Scheel
@ 2015-08-14 13:51 ` Takashi Iwai
2015-08-14 14:14 ` Julian Scheel
0 siblings, 1 reply; 5+ messages in thread
From: Takashi Iwai @ 2015-08-14 13:51 UTC (permalink / raw)
To: Julian Scheel; +Cc: alsa-devel
On Fri, 14 Aug 2015 15:45:58 +0200,
Julian Scheel wrote:
>
> Am 14.08.2015 um 15:17 schrieb Takashi Iwai:
> > On Thu, 13 Aug 2015 13:04:46 +0200,
> > Julian Scheel wrote:
> >>
> >> @@ -469,6 +503,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
> >> value_set = convert_bytes_value(cval, value_set);
> >> buf[0] = value_set & 0xff;
> >> buf[1] = (value_set >> 8) & 0xff;
> >> + buf[2] = (value_set >> 16) & 0xff;
> >> + buf[3] = (value_set >> 24) & 0xff;
> >
> > It's not smart but OK, just a subtle matter...
>
> Hm, what would you like better?
A shorter code would be to to pass __le32 value pointer directly after
calling cpu_to_le32(), for example. But it's a matter of taste, so
it's OK to keep your code as is.
Takashi
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ALSA: usb-audio: Fix parameter block size for UAC2 control requests
2015-08-14 13:51 ` Takashi Iwai
@ 2015-08-14 14:14 ` Julian Scheel
0 siblings, 0 replies; 5+ messages in thread
From: Julian Scheel @ 2015-08-14 14:14 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
Am 14.08.2015 um 15:51 schrieb Takashi Iwai:
> On Fri, 14 Aug 2015 15:45:58 +0200,
> Julian Scheel wrote:
>>
>> Am 14.08.2015 um 15:17 schrieb Takashi Iwai:
>>> On Thu, 13 Aug 2015 13:04:46 +0200,
>>> Julian Scheel wrote:
>>>>
>>>> @@ -469,6 +503,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
>>>> value_set = convert_bytes_value(cval, value_set);
>>>> buf[0] = value_set & 0xff;
>>>> buf[1] = (value_set >> 8) & 0xff;
>>>> + buf[2] = (value_set >> 16) & 0xff;
>>>> + buf[3] = (value_set >> 24) & 0xff;
>>>
>>> It's not smart but OK, just a subtle matter...
>>
>> Hm, what would you like better?
>
> A shorter code would be to to pass __le32 value pointer directly after
> calling cpu_to_le32(), for example. But it's a matter of taste, so
> it's OK to keep your code as is.
Thanks, I'll leave it as is for now and keep the le32 approach in mind
for the future.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-08-14 14:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-13 11:04 [PATCH] ALSA: usb-audio: Fix parameter block size for UAC2 control requests Julian Scheel
2015-08-14 13:17 ` Takashi Iwai
2015-08-14 13:45 ` Julian Scheel
2015-08-14 13:51 ` Takashi Iwai
2015-08-14 14:14 ` Julian Scheel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox