Hi Robin,
thanks for the patch.
On Monday 21 September 2009 20:15:19 Robin Callender wrote:
> On Fri, 2009-09-18 at 12:59 -0700, Robin Callender wrote:
> > This patch section contains (among other things) the modification to
> > composite.c/h to support routing of setup(endpoint) to the function.
> >
> >
> > Signed-off-by: Robin Callender <robin_callender@xxxxxxxxxxx>
> > ---
>
> There was a cut&paste error when the patch text was copied into the
> original email. It omitted the update to u_audio.h to include hid.h
> This updated patch fixes this problem.
>
> BTW, is there a tool for splitting up a large patch into the ~40K limit
> size? I have been manually creating the emails, which lead to this
> error. Seems there ought to be some script for handling this.
>
> Signed-off-by: Robin Callender <robin_callender@xxxxxxxxxxx>
> ---
[snip]
> diff -Naur linux-2.6.31.orig/drivers/usb/gadget/composite.c
> linux-2.6.31.new/drivers/usb/gadget/composite.c
> --- linux-2.6.31.orig/drivers/usb/gadget/composite.c 2009-09-12
> 11:51:48.000000000 -0700
> +++ linux-2.6.31.new/drivers/usb/gadget/composite.c 2009-09-18
> 10:18:54.000000000 -0700
[snip]
> @@ -810,9 +831,29 @@
> */
> if ((ctrl->bRequestType & USB_RECIP_MASK)
> == USB_RECIP_INTERFACE) {
> - f = cdev->config->interface[intf];
> - if (f && f->setup)
> - value = f->setup(f, ctrl);
> + if (intf < MAX_CONFIG_INTERFACES) {
> + f = cdev->config->interface[intf];
> + if (f && f->setup) {
> + value = f->setup(f, ctrl);
> + }
> + else
> + f = NULL;
> + }
> + else
> + f = NULL;
> +
> + }
> + if ((ctrl->bRequestType & USB_RECIP_MASK)
> + == USB_RECIP_ENDPOINT) {
> + u8 epaddr = w_index & ~USB_DIR_IN;
> + if (epaddr < MAX_CONFIG_ENDPOINTS) {
> + f = cdev->config->endpoint[epaddr];
> + if (f && f->setup) {
> + value = f->setup(f, ctrl);
> + }
> + else
> + f = NULL;
> + }
> else
> f = NULL;
> }
This issue has already been fixed in two patches I sent to the linux-usb
mailing list. Greg applied them to this tree. You should rebase your patches
on top of that and resubmit them.
> diff -Naur linux-2.6.31.orig/drivers/usb/gadget/u_audio.c
> linux-2.6.31.new/drivers/usb/gadget/u_audio.c ---
> linux-2.6.31.orig/drivers/usb/gadget/u_audio.c 2009-09-18
> 10:43:41.000000000 -0700 +++
> linux-2.6.31.new/drivers/usb/gadget/u_audio.c 2009-09-18
> 10:51:39.000000000 -0700 @@ -109,6 +109,71 @@
> /**
> * Set default hardware params
> */
> +static int capture_default_hw_params(struct gaudio_snd_dev *snd)
> +{
> + struct snd_pcm_substream *substream = snd->substream;
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct snd_pcm_hw_params *params;
> + snd_pcm_sframes_t result;
> +
> + /*
> + * SNDRV_PCM_ACCESS_RW_INTERLEAVED,
> + * SNDRV_PCM_FORMAT_S16_LE
> + * CHANNELS: 1
> + * RATE: 48000
> + */
> + snd->access = SNDRV_PCM_ACCESS_RW_INTERLEAVED;
> + snd->format = SNDRV_PCM_FORMAT_S16_LE;
> + snd->channels = 1;
> + snd->rate = 48000;
> +
> + params = kzalloc(sizeof(*params), GFP_KERNEL);
> + if (!params)
> + return -ENOMEM;
> +
> + _snd_pcm_hw_params_any(params);
> + _snd_pcm_hw_param_set(params, SNDRV_PCM_HW_PARAM_ACCESS,
> + snd->access, 0);
> + _snd_pcm_hw_param_set(params, SNDRV_PCM_HW_PARAM_FORMAT,
> + snd->format, 0);
> + _snd_pcm_hw_param_set(params, SNDRV_PCM_HW_PARAM_CHANNELS,
> + snd->channels, 0);
> + _snd_pcm_hw_param_set(params, SNDRV_PCM_HW_PARAM_RATE,
> + snd->rate, 0);
> +
> + snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DROP, NULL);
> + snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_HW_PARAMS, params);
> +
> + result = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_PREPARE,
> + NULL);
> + if (result < 0) {
> + ERROR(snd->card,
> + "Preparing sound card failed: %d\n", (int)result);
> + kfree(params);
> + return result;
> + }
> +
> + /* Store the hardware parameters */
> + snd->access = params_access(params);
> + snd->format = params_format(params);
> + snd->channels = params_channels(params);
> + snd->rate = params_rate(params);
> +
> + runtime->frame_bits = snd_pcm_format_physical_width(runtime->format);
> +
> + kfree(params);
> +
> + INFO(snd->card,
> + "Capture hardware params: access %x, format %x, "
> + "channels %d, rate %d\n",
> + snd->access, snd->format, snd->channels, snd->rate);
> +
> + return 0;
> +}
> +
> +/**
> + * Set default hardware params
> + */
> static int playback_default_hw_params(struct gaudio_snd_dev *snd)
> {
> struct snd_pcm_substream *substream = snd->substream;
> @@ -160,13 +225,55 @@
> kfree(params);
>
> INFO(snd->card,
> - "Hardware params: access %x, format %x, channels %d, rate %d\n",
> + "Playback hardware params: access %x, format %x, "
> + "channels %d, rate %d\n",
> snd->access, snd->format, snd->channels, snd->rate);
>
> return 0;
> }
>
> /**
> + * Capture audio buffer data by ALSA PCM device
> + */
> +static size_t u_audio_capture(struct gaudio *card, void *buf, size_t
> count) +{
> + ssize_t result;
> + mm_segment_t old_fs;
> + snd_pcm_sframes_t frames;
> +
> + struct gaudio_snd_dev *snd = &card->capture;
> + struct snd_pcm_substream *substream = snd->substream;
> + struct snd_pcm_runtime *runtime = substream->runtime;
> +
> +try_again:
> + if (runtime->status->state == SNDRV_PCM_STATE_XRUN ||
> + runtime->status->state == SNDRV_PCM_STATE_SUSPENDED) {
> + result = snd_pcm_kernel_ioctl(substream,
> + SNDRV_PCM_IOCTL_PREPARE, NULL);
> + if (result < 0) {
> + ERROR(card, "Preparing sound card failed: %d\n",
> + (int)result);
> + return result;
> + }
> + }
> + frames = bytes_to_frames(runtime, count);
> +
> + old_fs = get_fs();
> + set_fs(KERNEL_DS);
> +
> + result = snd_pcm_lib_read(substream, buf, frames);
> + if (result != frames) {
> + ERROR(card, "Capture error: %d\n", (int)result);
> + set_fs(old_fs);
> + goto try_again;
> + }
> +
> + set_fs(old_fs);
> +
> + return 0;
> +}
> +
> +/**
> * Playback audio buffer data by ALSA PCM device
> */
> static size_t u_audio_playback(struct gaudio *card, void *buf, size_t
> count) @@ -214,6 +321,16 @@
> return card->playback.rate;
> }
>
> +static int u_audio_get_capture_channels(struct gaudio *card)
> +{
> + return card->capture.channels;
> +}
> +
> +static int u_audio_get_capture_rate(struct gaudio *card)
> +{
> + return card->capture.rate;
> +}
> +
> /**
> * Open ALSA PCM and control device files
> * Initial the PCM or control device
> @@ -233,7 +350,6 @@
> int ret = PTR_ERR(snd->filp);
> ERROR(card, "unable to open sound control device file: %s\n",
> fn_cntl);
> - snd->filp = NULL;
> return ret;
> }
> snd->card = card;
> @@ -243,24 +359,23 @@
> snd->filp = filp_open(fn_play, O_WRONLY, 0);
> if (IS_ERR(snd->filp)) {
> ERROR(card, "No such PCM playback device: %s\n", fn_play);
> - snd->filp = NULL;
> + } else {
> + pcm_file = snd->filp->private_data;
> + snd->substream = pcm_file->substream;
> + snd->card = card;
> + playback_default_hw_params(snd);
> }
> - pcm_file = snd->filp->private_data;
> - snd->substream = pcm_file->substream;
> - snd->card = card;
> - playback_default_hw_params(snd);
>
> /* Open PCM capture device and setup substream */
> snd = &card->capture;
> snd->filp = filp_open(fn_cap, O_RDONLY, 0);
> if (IS_ERR(snd->filp)) {
> ERROR(card, "No such PCM capture device: %s\n", fn_cap);
> - snd->substream = NULL;
> - snd->card = NULL;
> } else {
> pcm_file = snd->filp->private_data;
> snd->substream = pcm_file->substream;
> snd->card = card;
> + capture_default_hw_params(snd);
> }
>
> return 0;
> diff -Naur linux-2.6.31.orig/drivers/usb/gadget/u_audio.h
> linux-2.6.31.new/drivers/usb/gadget/u_audio.h ---
> linux-2.6.31.orig/drivers/usb/gadget/u_audio.h 2009-09-09
> 15:13:59.000000000 -0700 +++
> linux-2.6.31.new/drivers/usb/gadget/u_audio.h 2009-09-21
> 10:58:22.000000000 -0700 @@ -14,6 +14,7 @@
>
> #include <linux/device.h>
> #include <linux/err.h>
> +#include <linux/hid.h>
> #include <linux/usb/audio.h>
> #include <linux/usb/composite.h>
Why is that required ? I can't see anything requiring linux/hid.h in
drivers/usb/gadget/u_audio.h.
> diff -Naur linux-2.6.31.orig/include/linux/usb/audio.h
> linux-2.6.31.new/include/linux/usb/audio.h ---
> linux-2.6.31.orig/include/linux/usb/audio.h 2009-09-12 11:51:48.000000000
> -0700 +++ linux-2.6.31.new/include/linux/usb/audio.h 2009-09-16
> 10:55:02.000000000 -0700 @@ -43,6 +43,12 @@
> /* A.8 Audio Class-Specific Endpoint Descriptor Subtypes */
> #define UAC_EP_GENERAL 0x01
>
> +/* endpoint attributes */
> +#define UAC_EP_ATTR_MASK 0x0c
> +#define UAC_EP_ATTR_ASYNC 0x04
> +#define UAC_EP_ATTR_ADAPTIVE 0x08
> +#define UAC_EP_ATTR_SYNC 0x0c
> +
linux/usb/ch9.h (2.6.32-rc1) already defines those:
#define USB_ENDPOINT_SYNC_NONE (0 << 2)
#define USB_ENDPOINT_SYNC_ASYNC (1 << 2)
#define USB_ENDPOINT_SYNC_ADAPTIVE (2 << 2)
#define USB_ENDPOINT_SYNC_SYNC (3 << 2)
> /* A.9 Audio Class-Specific Request Codes */
> #define UAC_SET_ 0x00
> #define UAC_GET_ 0x80
> @@ -154,22 +160,48 @@
> #define UAC_OUTPUT_TERMINAL_COMMUNICATION_SPEAKER 0x306
> #define UAC_OUTPUT_TERMINAL_LOW_FREQ_EFFECTS_SPEAKER 0x307
>
> +#define UAC_CHANNEL_CONFIG_L 0x0001
> +#define UAC_CHANNEL_CONFIG_R 0x0002
> +#define UAC_CHANNEL_CONFIG_C 0x0004
> +#define UAC_CHANNEL_CONFIG_LFE 0x0008
> +#define UAC_CHANNEL_CONFIG_LS 0x0010
> +#define UAC_CHANNEL_CONFIG_RS 0x0020
> +#define UAC_CHANNEL_CONFIG_LC 0x0040
> +#define UAC_CHANNEL_CONFIG_RC 0x0080
> +#define UAC_CHANNEL_CONFIG_S 0x0100
> +#define UAC_CHANNEL_CONFIG_SL 0x0200
> +#define UAC_CHANNEL_CONFIG_SR 0x0400
> +#define UAC_CHANNEL_CONFIG_T 0x0800
> +
> /* Set bControlSize = 2 as default setting */
> -#define UAC_DT_FEATURE_UNIT_SIZE(ch) (7 + ((ch) + 1) * 2)
> +#define UAC_FEATURE_UNIT_SIZE(ch,n) (7+(((ch)+1)*n))
Why do you rename the macro ?
And please add a space after the coma in the parameters list (this applies to
all other macros in the file as well).
> /* As above, but more useful for defining your own descriptors: */
> -#define DECLARE_UAC_FEATURE_UNIT_DESCRIPTOR(ch) \
> -struct uac_feature_unit_descriptor_##ch { \
> +#define DECLARE_UAC_FEATURE_UNIT_DESCRIPTOR(suffix,ch) \
> +struct uac_feature_unit_descriptor_##suffix { \
Why do you need both a suffix and the number of channels ? The same question
applies for other descriptor definitions in this file.
> __u8 bLength; \
> __u8 bDescriptorType; \
> __u8 bDescriptorSubtype; \
> __u8 bUnitID; \
> __u8 bSourceID; \
> __u8 bControlSize; \
> - __le16 bmaControls[ch + 1]; \
> + __u8 bmaControls[ch + 1]; \
That's not correct, it should be __u8 bmaControls[ch + 1][n];
> __u8 iFeature; \
> } __attribute__ ((packed))
>
> +#define UAC_SELECTOR_UNIT_SIZE(pins) (6 + pins)
That should be UAC_DT_SELECTOR_UNIT_SIZE to be consistent with other
definitions.
> +#define DECLARE_UAC_SELECTOR_UNIT_DESCRIPTOR(suffix,pins) \
> +struct uac_selector_unit_descriptor_##suffix { \
> + __u8 bLength; \
> + __u8 bDescriptorType; \
> + __u8 bDescriptorSubtype; \
> + __u8 bUnitID; \
> + __u8 bNrInPins; \
> + __u8 baSourceID[pins]; \
> + __u8 iSelector; \
> +} __attribute__ ((packed))
> +
> /* 4.5.2 Class-Specific AS Interface Descriptor */
> struct uac_as_header_descriptor {
> __u8 bLength; /* in bytes: 7 */
> @@ -238,6 +270,10 @@
> #define UAC_FORMAT_TYPE_II 0x2
> #define UAC_FORMAT_TYPE_III 0x3
>
> +#define UAS_ENDPOINT_ASYNC (1 << 2)
> +#define UAS_ENDPOINT_ADAPTIVE (2 << 2)
> +#define UAS_ENDPOINT_SYNC (3 << 2)
> +
You can reuse the linux/usb/ch9.h definitions.
> struct uac_iso_endpoint_descriptor {
> __u8 bLength; /* in bytes: 7 */
> __u8 bDescriptorType; /* USB_DT_CS_ENDPOINT */
> @@ -245,13 +281,16 @@
> __u8 bmAttributes;
> __u8 bLockDelayUnits;
> __le16 wLockDelay;
> -};
> +} __attribute__ ((packed));
> #define UAC_ISO_ENDPOINT_DESC_SIZE 7
>
> #define UAC_EP_CS_ATTR_SAMPLE_RATE 0x01
> #define UAC_EP_CS_ATTR_PITCH_CONTROL 0x02
> #define UAC_EP_CS_ATTR_FILL_MAX 0x80
>
> +#define UAC_EP_SAMPLE_FREQ (1 << (UAC_EP_CS_ATTR_SAMPLE_RATE - 1))
> +#define UAC_EP_PITCH_CONTROL (1 << (UAC_EP_CS_ATTR_PITCH_CONTROL - 1))
> +
> /* A.10.2 Feature Unit Control Selectors */
> #define UAC_FU_CONTROL_UNDEFINED 0x00
> #define UAC_MUTE_CONTROL 0x01
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
