LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jorge <jorge.sanjuan@codethink.co.uk>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org, gregkh@linuxfoundation.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion.
Date: Mon, 23 Apr 2018 17:06:54 +0100	[thread overview]
Message-ID: <61a5d852-f4b7-70fa-ae0e-2b5559e3b1f6@codethink.co.uk> (raw)
In-Reply-To: <s5hr2n6ru79.wl-tiwai@suse.de>

Hi Takashi,

Thank you very much for the reviews. I'll put a v2 patchset with the 
suggested changes for this patch and the other ones you reviewed.

Some comments below

On 23/04/18 13:19, Takashi Iwai wrote:
> On Fri, 20 Apr 2018 19:03:27 +0200,
> Jorge Sanjuan wrote:
>>
>> diff --git a/include/linux/usb/audio-v3.h b/include/linux/usb/audio-v3.h
>> index a8959aaba0ae..6cedb6d499ba 100644
>> --- a/include/linux/usb/audio-v3.h
>> +++ b/include/linux/usb/audio-v3.h
>> @@ -221,6 +221,12 @@ struct uac3_iso_endpoint_descriptor {
>>   	__le16 wLockDelay;
>>   } __attribute__((packed));
>>   
>> +/* 5.2.1.6.1 INSERTION CONTROL PARAMETER BLOCK */
>> +struct uac3_insertion_ctl_blk {
>> +	__u8 bSize;
>> +	__u8 bmConInserted[1];
> 
> Do we need to declare this as an array?

The size of bmConInserted will be determined by bSize. We could check 
how many connectors are in the device by checking the high capability 
Connectors Descriptor. If the number of connectors was greater than 8 
this block would need to get some more bytes in the request (or we could 
just request the following bytes if bSize was greater than one).

I'll remove the array and leave it as two byte block. That should be 
enough for up to 8 connectors.

> 
>>   static struct snd_kcontrol_new usb_feature_unit_ctl = {
>>   	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
>>   	.name = "", /* will be filled later manually */
>> @@ -1322,6 +1367,15 @@ static struct snd_kcontrol_new usb_bool_master_control_ctl_ro = {
>>   	.put = NULL,
>>   };
>>   
>> +static struct snd_kcontrol_new usb_connector_ctl_ro = {
> 
> Put const.

+1	

> 
> 
>> @@ -1904,16 +1966,29 @@ static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
>>   				      void *raw_desc)
>>   {
>>   	struct usb_audio_term iterm;
>> -	struct uac2_input_terminal_descriptor *d = raw_desc;
>> +	unsigned int control, bmctls, term_id;
>>   
>> -	check_input_term(state, d->bTerminalID, &iterm);
>>   	if (state->mixer->protocol == UAC_VERSION_2) {
>> -		/* Check for jack detection. */
>> -		if (uac_v2v3_control_is_readable(d->bmControls,
>> -						 UAC2_TE_CONNECTOR)) {
>> -			build_connector_control(state, &iterm, true);
>> -		}
>> -	}
>> +		struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
>> +		control = UAC2_TE_CONNECTOR;
>> +		term_id = d_v2->bTerminalID;
>> +		bmctls = d_v2->bmControls;
>> +	}
>> +	else if (state->mixer->protocol == UAC_VERSION_3) {
> 
> Put "else if" and the closing brace in the same line.

Thanks. My bad.

> 
> 
>> +		struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
>> +		control = UAC3_TE_INSERTION;
>> +		term_id = d_v3->bTerminalID;
>> +		bmctls = d_v3->bmControls;
> 
> Doesn't it need le32_to_cpu()?

Agreed.

> 
>> +	}
>> +	else /* UAC1. No Insertion control */
> 
> Ditto, put "else" and the closing brace in the same line.
> Also, use braces for the rest block, otherwise it'll look
> inconsistent.  Or rewrite with switch().
> 
>> @@ -2645,6 +2720,12 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer)
>>   			err = parse_audio_unit(&state, desc->bCSourceID);
>>   			if (err < 0 && err != -EINVAL)
>>   				return err;
>> +
>> +			if (uac_v2v3_control_is_readable(desc->bmControls,
> 
> Missing le32_to_cpu()?

Agreed.


Thanks,

Jorge

>  >
> thanks,
> 
> Takashi
> 

  reply	other threads:[~2018-04-23 16:06 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-20 17:03 [PATCH 0/4] ALSA: usb: UAC3 new features Jorge Sanjuan
2018-04-20 17:03 ` [PATCH 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit Jorge Sanjuan
2018-04-23 11:03   ` Takashi Iwai
2018-04-20 17:03 ` [PATCH 2/4] ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3 Jorge Sanjuan
2018-04-23 12:11   ` Takashi Iwai
2018-04-24  8:03   ` [alsa-devel] " Ruslan Bilovol
2018-04-20 17:03 ` [PATCH 3/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices Jorge Sanjuan
2018-04-22 20:30   ` [alsa-devel] " kbuild test robot
2018-04-20 17:03 ` [PATCH 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion Jorge Sanjuan
2018-04-22 20:55   ` kbuild test robot
2018-04-23 12:19   ` Takashi Iwai
2018-04-23 16:06     ` Jorge [this message]
2018-04-24 17:24 ` [PATCH v2 0/4] ALSA: usb: UAC3 new features Jorge Sanjuan
2018-04-24 17:24   ` [PATCH v2 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit Jorge Sanjuan
2018-04-25 22:35     ` [alsa-devel] " Ruslan Bilovol
2018-04-26 16:56       ` Jorge
2018-04-27 17:06     ` [PATCH v3 " Jorge Sanjuan
2018-05-04  0:57       ` Ruslan Bilovol
2018-05-08  9:43         ` Jorge
2018-05-09 22:11           ` Ruslan Bilovol
2018-04-24 17:24   ` [PATCH v2 2/4] ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3 Jorge Sanjuan
2018-04-24 17:55     ` Takashi Iwai
2018-04-24 17:24   ` [PATCH v2 3/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices Jorge Sanjuan
2018-04-25 22:53     ` [alsa-devel] " Ruslan Bilovol
2018-04-24 17:24   ` [PATCH v2 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion Jorge Sanjuan
2018-04-24 18:02   ` [PATCH v2 0/4] ALSA: usb: UAC3 new features Takashi Iwai
2018-04-26  9:26     ` [alsa-devel] " Ruslan Bilovol
2018-04-26 17:13       ` Jorge
2018-05-11 15:25 ` [PATCH v4 " Jorge Sanjuan
2018-05-11 15:25   ` [PATCH v4 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit Jorge Sanjuan
2018-05-14 20:54     ` Ruslan Bilovol
2018-05-11 15:25   ` [PATCH v4 2/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices Jorge Sanjuan
2018-05-14 21:00     ` Ruslan Bilovol
2018-05-11 15:25   ` [PATCH v4 3/4] ALSA: usb-audio: UAC3 Add support for connector insertion Jorge Sanjuan
2018-05-11 15:25   ` [PATCH v4 4/4] ALSA: usb-audio: UAC3: Parse Input Terminal number of channels Jorge Sanjuan
2018-05-14  8:54     ` Jorge
2018-05-14  9:36       ` Ruslan Bilovol
2018-05-14 11:03     ` [RESEND PATCH " Jorge Sanjuan
2018-05-14 21:05       ` Ruslan Bilovol
2018-05-15  5:38   ` [PATCH v4 0/4] ALSA: usb: UAC3 new features Takashi Iwai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=61a5d852-f4b7-70fa-ae0e-2b5559e3b1f6@codethink.co.uk \
    --to=jorge.sanjuan@codethink.co.uk \
    --cc=alsa-devel@alsa-project.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tiwai@suse.de \
    --subject='Re: [PATCH 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).