January 2011 - Alsa-devel - mailman.alsa-project.org

[alsa-devel] unsupported variant of snd_hda_intel
by Alexandre 04 Jul '24

04 Jul '24

Hello, I have an Acer Aspire 4736G laptop. The soundcard that is shipped in this laptop is intel-hda based as shown (actually it's a realtek): cat /proc/asound/cards 0 [Intel ]: HDA-Intel - HDA Intel HDA Intel at 0x9b300000 irq 46 Basic sound output is correct but few feature are missing: - internal mic seems to be found but do not works. - sound output stop working when trying to send it using HDMI output - heads do not work when plugged neither when forcing pulseaudio to use it. I have an ubuntu 10.10 which is shipped with alsa 1.0.23. I have googled around and tried loading the snd-hda-intel with several parameters () as found in HD-Audio-Models.txt to no avail. cat /proc/asound/pcm 00-00: ALC888 Analog : ALC888 Analog : playback 1 : capture 1 00-01: ALC888 Digital : ALC888 Digital : playback 1 00-03: NVIDIA HDMI : NVIDIA HDMI : playback 1 Where can I begin to help implement the support for this sound card? I have very skills in developement in general but am ready to try if it is not to hard (I supose I can use previous code for similar models).. I just need a shepherd. Thanks

4 4

[alsa-devel] hda-intel missing mixer channels
by Orm Finnendahl 26 Aug '13

26 Aug '13

Hi, I sent this to alsa-user a week ago but didn't get any reply. Does anybody know, what is responsible for the missing faders? Did the driver change or is it some module loading mechanism in the distro which prevents the faders from showing up in alsamixer? This is so strange. Please help! -- Orm -------------------------------------------------------------------- Hi everybody, after the last update of Ubuntu (intrepid to jaunty) on my Lenovo Ideapad my microphone input doesn't work anymore (neither internal nor external). I found out that alsamixer lacks quite a lot of faders in the newer kernel/module compared to the older one. Some of the faders now unavailable were crucial for Mic input to work on the older system so I'd very much like to get them back ;-) Some more infos: Soundcard: Intel 82801G (ICH7 Family) Chip: Realtek ALC269 Module: snd_hda_intel (in both kernels) Older kernel: Ubuntu stock 2.6.27-14-generic Newer kernel: Ubuntu stock 2.6.28-13-rt Is there any way to make the missing controls available or did the driver change and I have to compile the older driver for the new kernel? Yours, Orm

2 3

[alsa-devel] Problem with MX27 ASoC audio
by Randolph Chung 11 Jul '13

11 Jul '13

I have a target that uses iMX27(L) + tlv320aic3x. The kernel tree is HEAD of linux-arm from about a week ago. Simple playback and recording (using aplay and arecord) work fine. However, I've run into a strange problem when both playback and capture streams are opened at the same time. Consider the attached test program. The capture device is setup with a period time of 10ms, and the program reads samples in a loop. The expected result is that I get a sample every 10ms. This works fine if the playback device is *not* opened (more specifically, if the hw_params are not set). However if the playback device is opened (#if 1 code) then I get samples every 20ms, which seems very wrong. I have tried using both DMA and FIQ ssi-pcm interfaces and both give the same strange result. With the FIQ driver, if I instrument the timer callback, the timer triggers every 10ms but there is only enough data every 20ms. The codec is being driven with a 13MHz MCLK (output from MX27) and the BCLK and WCLK come from the codec. They seem to have the correct rates. Any ideas? Can anybody else with MX27 hardware verify if my test program also fails on their target? thanks, randolph /* Test program */ #include <alsa/asoundlib.h> #include <stdio.h> #include <stdlib.h> #include <assert.h> #include <sys/time.h> int main(int argc, char **argv) { int err = -1; char buf[4096]; unsigned long period_size; snd_pcm_t *in_handle = NULL, *out_handle; snd_pcm_hw_params_t *hwparams; if (snd_pcm_open(&in_handle, "hw:0,0", SND_PCM_STREAM_CAPTURE, 0) < 0) { printf("Cannot open capture device\n"); return -1; } snd_pcm_hw_params_malloc(&hwparams); snd_pcm_hw_params_any(in_handle, hwparams); snd_pcm_hw_params_set_access(in_handle, hwparams, SND_PCM_ACCESS_RW_INTERLEAVED); snd_pcm_hw_params_set_format(in_handle, hwparams, SND_PCM_FORMAT_S16_LE); snd_pcm_hw_params_set_channels(in_handle, hwparams, 1); snd_pcm_hw_params_set_rate(in_handle, hwparams, 8000, 0); snd_pcm_hw_params_set_period_time(in_handle, hwparams, 10000, 0); snd_pcm_hw_params_get_period_size(hwparams, &period_size, 0); snd_pcm_hw_params(in_handle, hwparams); snd_pcm_hw_params_free(hwparams); if (snd_pcm_open(&out_handle, "hw:0,0", SND_PCM_STREAM_PLAYBACK, 0) < 0) { printf("Cannot open playback device\n"); return -1; } #if 1 snd_pcm_hw_params_malloc(&hwparams); snd_pcm_hw_params_any(out_handle, hwparams); snd_pcm_hw_params_set_access(out_handle, hwparams, SND_PCM_ACCESS_RW_INTERLEAVED); snd_pcm_hw_params_set_format(out_handle, hwparams, SND_PCM_FORMAT_S16_LE); snd_pcm_hw_params_set_channels(out_handle, hwparams, 1); snd_pcm_hw_params_set_rate(out_handle, hwparams, 8000, 0); snd_pcm_hw_params_set_period_time(out_handle, hwparams, 10000, 0); snd_pcm_hw_params(out_handle, hwparams); snd_pcm_hw_params_free(hwparams); #endif printf("period size = %u\n", period_size); printf("Config done, start loop\n"); while (1) { struct timeval tv; int ret = snd_pcm_readi(in_handle, buf, period_size); if (ret < 0) snd_pcm_prepare(in_handle); gettimeofday(&tv, NULL); printf("[%u.%06u] snd_pcm_readi() return %d\n", tv.tv_sec, tv.tv_usec, ret); } return 0; }

5 11

[alsa-devel] sound: usb-audio: full capture/playback/spdif support for Digidesign Mbox 2
by Damien Zammit 23 Dec '12

23 Dec '12

Hi folks, I have put together full support for Digidesign Mbox 2, but it needs someone to help me merge it into alsa. I haven't seen the latest snd-usb-audio code since last year, it seems to have branched into multiple files. I have worked out how the Digidesign Mbox 2 needs to be configured, it supports SPDIF i/o simultaneously with analogue but can be toggled on and off by using different altsettings. Also the sample clock rate must be configured via a special method. Therefore I have provided for this in a device_setup parameter with 4 settings: 48Khz Spdif+analog = 0 44.1Khz Spdif+analog = 1 48Khz Analog only = 2 44.1Khz Analog only = 3 ###### audio /* Digidesign Mbox 2 workaround: * supports S24_3BE BIG ENDIAN in an unusual way */ if (chip->usb_id == USB_ID(0x0dba, 0x3000)) pcm_format = SNDRV_PCM_FORMAT_S24_3BE; static int mbox2_skip_setting_quirk(struct snd_usb_audio *chip, int iface, int altno); static int parse_audio_endpoints(struct snd_usb_audio *chip, int iface_no) { ..... ..... /* Digidesign Mbox 2: skip altsets incompatible with device_setup */ if (chip->usb_id == USB_ID(0x0dba, 0x3000) && mbox2_skip_setting_quirk(chip, iface_no, altno)) continue; ..... } #define MBOX2_SET_48K_SPDIF 0 #define MBOX2_SET_44K_SPDIF 1 #define MBOX2_SET_48K_ANALOG 2 #define MBOX2_SET_44K_ANALOG 3 static int mbox2_skip_setting_quirk(struct snd_usb_audio *chip, int iface, int altno) { u8 srate[3]; /* Reset ifaces 2-5 to 0 altsetting. */ usb_set_interface(chip->dev, iface, 0); if (device_setup[chip->index] == MBOX2_SET_48K_SPDIF || device_setup[chip->index] == MBOX2_SET_48K_ANALOG) { //setup 48k srate[0]=0x80; srate[1]=0xbb; srate[2]=0x00; /* Send the magic! */ snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), 0x01, 0x22, 0x0100, 0x0085, &srate, 0x0003, 1000); snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), 0x81, 0xa2, 0x0100, 0x0085, &srate, 0x0003, 1000); snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), 0x81, 0xa2, 0x0100, 0x0086, &srate, 0x0003, 1000); snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), 0x81, 0xa2, 0x0100, 0x0003, &srate, 0x0003, 1000); snd_printk(KERN_INFO "Digidesign Mbox 2: 24 Bit 48000Hz"); if (device_setup[chip->index] == MBOX2_SET_48K_SPDIF) { //setup spdif snd_printk(KERN_INFO ", SPDIF\n"); if (altno != 2) return 1; } else { //setup analog snd_printk(KERN_INFO ", ANALOGUE\n"); if (altno != 3) return 1; } } else { //setup 44k srate[0]=0x44; srate[1]=0xac; srate[2]=0x00; /* Send the magic! */ snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), 0x01, 0x22, 0x0100, 0x0085, &srate, 0x0003, 1000); snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), 0x81, 0xa2, 0x0100, 0x0085, &srate, 0x0003, 1000); snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), 0x81, 0xa2, 0x0100, 0x0086, &srate, 0x0003, 1000); snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), 0x81, 0xa2, 0x0100, 0x0003, &srate, 0x0003, 1000); snd_printk(KERN_INFO "Digidesign Mbox 2: 24 Bit 44100Hz"); if (device_setup[chip->index] == MBOX2_SET_44K_SPDIF || device_setup[chip->index] == MBOX2_SET_44K_ANALOG) { //setup spdif snd_printk(KERN_INFO ", SPDIF\n"); if (altno != 2) return 1; } else { //setup analog snd_printk(KERN_INFO ", ANALOGUE\n"); if (altno != 3) return 1; } } return 0; } #define MBOX2_FIRMWARE_SIZE 646 #define MBOX2_BOOT_LOADING 0x01 /* Hard coded into the device */ #define MBOX2_BOOT_READY 0x02 /* Hard coded into the device */ static int snd_usb_mbox2_boot_quirk(struct usb_device *dev) { struct usb_host_config *config = dev->actconfig; int err; u8 enablemagic[3]; u8 bootresponse; int fwsize; if ((fwsize = le16_to_cpu(get_cfg_desc(config)->wTotalLength)) == MBOX2_FIRMWARE_SIZE) { snd_printdd("Sending Digidesign Mbox 2 boot sequence...\n"); /* From USB Snoop, * * SetupPacket = RT RQ VHVL INDX SIZE * RT = 0xRT Request Type * RQ = 0xRQ Request * VHVL = 0xVLVH Value in reverse byte order * INDX = 0xDXIN Index in reverse byte order * SIZE = 0xZESI Size in reverse byte order * * Magic boot code setup packet: c0 85 01 00 00 00 12 00 * RQ RT VLVH DXIN ZESI * becomes 0x85, 0xc0, 0x0001, 0x0000, &RETURNDATA, 0x0012, TIMEOUT * for snd_usb_ctl_msg() */ mbox2_reboot: snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), 0x85, 0xc0, 0x0001, 0x0000, &bootresponse, 0x0012, 1000); if (bootresponse == MBOX2_BOOT_LOADING) { snd_printdd("device not ready, resending boot sequence...\n"); goto mbox2_reboot; } if (bootresponse == MBOX2_BOOT_READY) { snd_printdd("device initialised!\n"); err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, &dev->descriptor, sizeof(dev->descriptor)); config = dev->actconfig; if (err < 0) snd_printdd("error usb_get_descriptor: %d\n", err); err = usb_reset_configuration(dev); if (err < 0) snd_printdd("error usb_reset_configuration: %d\n", err); snd_printdd("mbox2_boot: new boot length = %d\n", le16_to_cpu(get_cfg_desc(config)->wTotalLength)); /* Successful boot, (should reset all altsettings to 0 like windows does, but works without it) * but now we will try sending capture/playback enable magic * * 80 bb 00 = 24bit mode - S24_3BE * 44 ac 00 = 16bit mode? * */ /* We want 48khz mode for now */ enablemagic[0]=0x80; enablemagic[1]=0xbb; enablemagic[2]=0x00; /* Send the magic! */ snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), 0x01, 0x22, 0x0100, 0x0085, &enablemagic, 0x0003, 1000); snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), 0x81, 0xa2, 0x0100, 0x0085, &enablemagic, 0x0003, 1000); snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), 0x81, 0xa2, 0x0100, 0x0086, &enablemagic, 0x0003, 1000); snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), 0x81, 0xa2, 0x0100, 0x0003, &enablemagic, 0x0003, 1000); return 0; /* Succesful boot */ } snd_printdd("unknown bootresponse, ignoring device: %d\n",bootresponse); return -ENODEV; } snd_printk("Invalid firmware size: %d\n",fwsize); return -ENODEV; } ####### quirk: { USB_DEVICE(0x0dba, 0x3000), .driver_info = (unsigned long) & (const struct snd_usb_audio_quirk) { .vendor_name = "Digidesign", .product_name = "Mbox 2", .ifnum = QUIRK_ANY_INTERFACE, .type = QUIRK_COMPOSITE, .data = (const struct snd_usb_audio_quirk[]) { { .ifnum = 0, .type = QUIRK_IGNORE_INTERFACE }, { .ifnum = 1, .type = QUIRK_AUDIO_STANDARD_INTERFACE }, { .ifnum = 2, .type = QUIRK_AUDIO_STANDARD_INTERFACE }, { .ifnum = 3, .type = QUIRK_AUDIO_STANDARD_INTERFACE }, { .ifnum = 4, .type = QUIRK_AUDIO_STANDARD_INTERFACE }, { .ifnum = 5, .type = QUIRK_AUDIO_STANDARD_INTERFACE }, { .ifnum = 6, .type = QUIRK_MIDI_MBOX2, .data = & (const struct snd_usb_midi_endpoint_info) { .out_ep = 0x02, .out_cables = 0x0001, .in_ep = 0x81, .in_interval = 0x01, .in_cables = 0x0001 } }, { .ifnum = -1 } } } }, ##### midi case QUIRK_MIDI_MBOX2: /* Digidesign Mbox 2 uses MIDIMAN MIDI protocol */ umidi->usb_protocol_ops = &snd_usbmidi_midiman_ops; /* * We have to make sure that the USB core looks * again at interface 6 by calling usb_set_interface() on it. */ usb_set_interface(umidi->chip->dev, 6, 0); memcpy(&endpoints[0], quirk->data, sizeof(struct snd_usb_midi_endpoint_info)); err = 0; break; Damien

4 26

[alsa-devel] [PATCH] snd-usb-us122l: Fix missing NULL checks
by Karsten Wiese 27 Apr '12

27 Apr '12

Fix missing NULL checks in usb_stream_hwdep_poll() and usb_stream_hwdep_ioctl(). Wake up poll waiters before returning from usb_stream_hwdep_ioctl(). Signed-off-by: Karsten Wiese <fzu(a)wemgehoertderstaat.de> Cc: stable(a)kernel.org --- sound/usb/usx2y/us122l.c | 41 ++++++++++++++++++++--------------------- 1 files changed, 20 insertions(+), 21 deletions(-) diff --git a/sound/usb/usx2y/us122l.c b/sound/usb/usx2y/us122l.c index 91bb296..7abda8d 100644 --- a/sound/usb/usx2y/us122l.c +++ b/sound/usb/usx2y/us122l.c @@ -269,29 +269,26 @@ static unsigned int usb_stream_hwdep_poll(struct snd_hwdep *hw, struct file *file, poll_table *wait) { struct us122l *us122l = hw->private_data; - struct usb_stream *s = us122l->sk.s; unsigned *polled; unsigned int mask; poll_wait(file, &us122l->sk.sleep, wait); - switch (s->state) { - case usb_stream_ready: - if (us122l->first == file) - polled = &s->periods_polled; - else - polled = &us122l->second_periods_polled; - if (*polled != s->periods_done) { - *polled = s->periods_done; - mask = POLLIN | POLLOUT | POLLWRNORM; - break; + mask = POLLIN | POLLOUT | POLLWRNORM | POLLERR; + if (mutex_trylock(&us122l->mutex)) { + struct usb_stream *s = us122l->sk.s; + if (s && s->state == usb_stream_ready) { + if (us122l->first == file) + polled = &s->periods_polled; + else + polled = &us122l->second_periods_polled; + if (*polled != s->periods_done) { + *polled = s->periods_done; + mask = POLLIN | POLLOUT | POLLWRNORM; + } else + mask = 0; } - /* Fall through */ - mask = 0; - break; - default: - mask = POLLIN | POLLOUT | POLLWRNORM | POLLERR; - break; + mutex_unlock(&us122l->mutex); } return mask; } @@ -377,6 +374,7 @@ static int usb_stream_hwdep_ioctl(struct snd_hwdep *hw, struct file *file, { struct usb_stream_config *cfg; struct us122l *us122l = hw->private_data; + struct usb_stream *s; unsigned min_period_frames; int err = 0; bool high_speed; @@ -422,18 +420,18 @@ static int usb_stream_hwdep_ioctl(struct snd_hwdep *hw, struct file *file, snd_power_wait(hw->card, SNDRV_CTL_POWER_D0); mutex_lock(&us122l->mutex); + s = us122l->sk.s; if (!us122l->master) us122l->master = file; else if (us122l->master != file) { - if (memcmp(cfg, &us122l->sk.s->cfg, sizeof(*cfg))) { + if (!s || memcmp(cfg, &s->cfg, sizeof(*cfg))) { err = -EIO; goto unlock; } us122l->slave = file; } - if (!us122l->sk.s || - memcmp(cfg, &us122l->sk.s->cfg, sizeof(*cfg)) || - us122l->sk.s->state == usb_stream_xrun) { + if (!s || memcmp(cfg, &s->cfg, sizeof(*cfg)) || + s->state == usb_stream_xrun) { us122l_stop(us122l); if (!us122l_start(us122l, cfg->sample_rate, cfg->period_frames)) err = -EIO; @@ -444,6 +442,7 @@ unlock: mutex_unlock(&us122l->mutex); free: kfree(cfg); + wake_up_all(&us122l->sk.sleep); return err; } -- 1.7.2.3

2 5

[alsa-devel] [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI
by David Henningsson 01 Feb '12

01 Feb '12

Since I'm a little new in HDMI land and the patch is non-trivial, I wouldn't mind some comments on this one. Also thanks to Jaroslav who cleared a few things out about the logical and physical devices which helped me figure out what I should do about it :-) Anyway, here's the patch. Some newer chips have more than one HDMI output, but usually not all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway. Signed-off-by: David Henningsson <david.henningsson(a)canonical.com> -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic

4 14

[alsa-devel] [ASoc] TDM over one SSC channel on AT91 possible?
by Friedrich Lobenstock 24 Jun '11

24 Jun '11

Hello! The background: I'm just designing an ATMEL AT91RM9200 based board which got two TLV320AIC33 on board. It is either possible to hook up the two TLV320AIC33 to a separate SSC (synchronous serial control) channel each or use one SSC channel and configure it for TDM (time division multiplex) mode. There is already a driver for TLV320AIC14k/TLV320AIC2k in the development tree, so adding support for the TLV320AIC33 should be feasible. What I'm not sure about is the fact if ASOC is prepared for using channels, especially SSC, in TDM mode. Is the core ASOC structure prepared to handle that? Would we be better off instantiate one drive twice each time with different parameters for the SSC channel? Thanks for any feedback. -- MfG / Regards Friedrich Lobenstock _________________________________________________________ SCOTTY Group Austria GmbH a SCOTTY Group plc company, http://www.scottygroup.com/ _________________________________________________________

4 3

Re: [alsa-devel] [PATCH] ASoC: Add support for Cirrus Logic CS42L52 Lowpower Stereo Codec
by Mark Brown 22 Jun '11

22 Jun '11

On Thu, Feb 25, 2010 at 06:07:35PM -0600, Brian wrote: This is a quick first pass review only. It'd be best if you could include a Signed-off-by line in submissions so that they can be merged if they're OK, even if you expect problems you may be surprised! Overall this looks fairly good - there's a lot of comments below but the vast majority of them are stylistic issues or needing to update to current APIs rather than major structural problems with the code. The main thing I don't really follow is the register access code, there's quite a bit of stuff going on in there. > + * published by the Free Software Foundation. > + * Revision history > + * Nov 2007 Initial version. > + * Oct 2008 Updated to 2.6.26 > + * Feb 2010 Updated to latest asoc git tree Please remove the changelog, once things are merged git takes care of that. > + */ > +#define DEBUG Not by default in mainline :) > +#ifdef DEBUG > +#define SOCDBG(fmt, arg...) printk(KERN_ERR "%s: %s() " fmt, SOC_CS42L52_NAME, __FUNCTION__, ##arg) > +#else > +#define SOCDBG(fmt, arg...) > +#endif > +#define SOCINF(fmt, args...) printk(KERN_INFO "%s: " fmt, SOC_CS42L52_NAME, ##args) > +#define SOCERR(fmt, args...) printk(KERN_ERR "%s: " fmt, SOC_CS42L52_NAME, ##args) Please convert the driver to use the standard dev_ and pr_ macros for this (where possible the dev_ ones are better). > +static int soc_cs42l52_set_bias_level(struct snd_soc_codec *codec, > + enum snd_soc_bias_level level); > +static int soc_cs42l52_pcm_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai); > +static int soc_cs42l52_set_sysclk(struct snd_soc_dai *codec_dai, > + int clk_id, u_int freq, int dir); > +static int soc_cs42l52_digital_mute(struct snd_soc_dai *dai, int mute); > +static int soc_cs42l52_set_fmt(struct snd_soc_dai *codec_dai, > + u_int fmt); > + > +static unsigned int soc_cs42l52_read(struct snd_soc_codec *codec, > + u_int reg); Please use 'unsigned int' rather than 'u_int' for consistency with the rest of ASoC. It's a bit surprising that you need the forward declarations at all but they do no harm. > +/** > + * snd_soc_get_volsw - single mixer get callback > + * @kcontrol: mixer control > + * @uinfo: control element information > + * > + * Callback to get the value of a single mixer control. > + * > + * Returns 0 for success. > + */ > +int snd_soc_cs42l5x_get_volsw(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); > + int reg = kcontrol->private_value & 0xff; The indentation looks wrong - should be tabs for indenting the code blocks. scripts/checkpatch.pl is a pretty good check for this sort of stuff, I'll not mention other issues. > + int shift = (kcontrol->private_value >> 8) & 0x0f; > + int rshift = (kcontrol->private_value >> 12) & 0x0f; > + int max = (kcontrol->private_value >> 16) & 0xff; > + int mask = (1 << fls(max)) - 1; > + int min = (kcontrol->private_value >> 24) & 0xff; > + > + ucontrol->value.integer.value[0] = > + ((snd_soc_read(codec, reg) >> shift) - min) & mask; > + if (shift != rshift) > + ucontrol->value.integer.value[1] = > + ((snd_soc_read(codec, reg) >> rshift) - min) & mask; This and most of the other custom control stuff looks an awful lot like it should be done by adding a SND_SOC_DOUBLE_SIGNED() or something along the lines of the existing _S8() but I didn't read entirely closely. I was speaking to someone on IRC only the other day who needed some of those types for a different device. These should also be updated to use struct soc_mixer_control for the private value rather than shifting everything into the private value, it allows wider ranges and is less error prone. > +static const u8 soc_cs42l52_reg_default[] = { > + 0x00, 0xE0, 0x01, 0x07, 0x05, /*4*/ > + 0xa0, 0x00, 0x00, 0x81, /*8*/ Very nitpicky but you're using a mix of upper and lower case for the letters in the hex digits. > +static inline int soc_cs42l52_read_reg_cache(struct snd_soc_codec *codec, > + u_int reg) > +{ > + u8 *cache = codec->reg_cache; > + > + return reg > SOC_CS42L52_REG_NUM ? -EINVAL : cache[reg]; > +} You should be able to replace all your register I/O code with use of soc-cache.c with 8 bit data, 8 bit register. Except... > + if(info->flags & SOC_CS42L52_ALL_IN_ONE) > + { > + for(i = 0; i < codec->num_dai; i++) > + { > + if(info->machine_handler) > + info->machine_handler(i); ...I'm not sure what's going on here but it could really use an explanation? I'm not sure what ALL_IN_ONE is supposed to do, or what the machine_handler() function is for. > + if(info->flags & SOC_CS42L52_CHIP_SWICTH) > + { > + num = info->flags & SOC_CS42L52_CHIP_MASK; > + if(info->machine_handler) > + info->machine_handler(num); > + } This too. There's a lot of unusual code using this machine handler function. I suspect this can all use ASoC standard stuff. > +static inline int soc_cs42l52_get_revison(struct snd_soc_codec *codec) > +{ > + u8 data; > + u8 addr; > + int num, ret = 0; > + struct soc_codec_cs42l52 *info = (struct soc_codec_cs42l52*)codec->private_data; No need to cast away from void (quite a few examples of this in the code). > + return ret < 0 ? ret : data; I'm not a big fan of the ternery operator... > +static unsigned int soc_cs42l52_read(struct snd_soc_codec *codec, > + u_int reg) > +{ > + u8 data; > + u8 addr; > + int i, ret = 0; > + struct soc_codec_cs42l52 *info = (struct soc_codec_cs42l52*)codec->private_data; > +#ifndef CONFIG_CS42L52_DEBUG > + if(reg == CODEC_CS42L52_SPK_STATUS) > + { > +#endif This really shouldn't be ifdefed. If you need to handle registers that report status from the device with the soc-cache stuff use the volatile_register() callback in the CODEC, any registers that need a CODEC read will call through to the hardware rather than using the cache. > +static const char *cs42l52_adc_mux[] = {"AIN1", "AIN2", "AIN3", "AIN4", "PGA"}; > +static const char *cs42l52_mic_mux[] = {"MIC1", "MIC2"}; > +static const char *cs42l52_stereo_mux[] = {"Mono", "Stereo"}; > +static const char *cs42l52_off[] = {"On", "Off"}; > +static const char *cs42l52_hpmux[] = {"Off", "On"}; > + > +static const struct soc_enum soc_cs42l52_enum[] = { > +SOC_ENUM_DOUBLE(CODEC_CS42L52_ANALOG_HPF_CTL, 4, 6, 2, cs42l52_hpf_freeze), /*0*/ > +SOC_ENUM_SINGLE(CODEC_CS42L52_ADC_HPF_FREQ, 0, 4, cs42l52_hpf_corner_freq), Don't use an array of enums, define individual variables for them. Some drivers use the arrays for historical reasons - they're quite painful to work with if you ever want to change anything in the middle and we've found a few off by one errors in the referencing into the arrays before :/. > +static const struct snd_kcontrol_new soc_cs42l52_controls[] = { > + > +SOC_ENUM("Mic VA Capture Switch", soc_cs42l52_enum[18]), /*0*/ enums shouldn't be called Switch - Switch has a specific meaning to ALSA (see Documentation/sound/alsa/ControlNames.txt, there's quite a few other controls could do with having the components of the name reordered and various other things following that). Normally just omit the ' Switch' from the name. > +SOC_DOUBLE("Analog SR Capture Switch", CODEC_CS42L52_ANALOG_HPF_CTL, 1, 3, 1, 1), > +SOC_DOUBLE("Analog ZC Capture Switch", CODEC_CS42L52_ANALOG_HPF_CTL, 0, 2, 1, 1), This'd normally be 'Capture ZC Switch' if it's for zero cross. > +SOC_ENUM("HPF corner freq Capture Switch", soc_cs42l52_enum[1]), /*5*/ > + > +SOC_SINGLE("Ganged Ctl Capture Switch", CODEC_CS42L52_ADC_MISC_CTL, 7, 1, 1), /* should be enabled init */ What does the comment mean here? > +SOC_SINGLE("HP Analog Gain Playback Volume", CODEC_CS42L52_PB_CTL1, 5, 7, 0), Just "Headphone Volume"? > +SOC_SINGLE("Playback B=A Volume Playback Switch", CODEC_CS42L52_PB_CTL1, 4, 1, 0), /*10*/ /*should be enabled init*/ If this is the mute for the headphone volume it should have the same name but with Switch instead of Volume so applications like alsamixer can display the two together. > +static int soc_cs42l52_add_controls(struct snd_soc_codec *codec) > +{ > + int i,ret = 0; > + > + for(i = 0; i < ARRAY_SIZE(soc_cs42l52_controls); i++) > + { > + ret = snd_ctl_add(codec->card, > + snd_soc_cnew(&soc_cs42l52_controls[i], codec, NULL)); > + if(ret < 0) > + { > + SOCDBG("add cs42l52 controls failed\n"); > + break; Replace this with snd_soc_add_controls(). > + /* Sum switches */ > + SND_SOC_DAPM_PGA("AIN1A Switch", CODEC_CS42L52_ADC_PGA_A, 0, 0, NULL, 0), Without having checked the datasheet I *suspect* most of these switches should be modelled as inputs to a mixer (possibly one with no power control of its own)? The "Switch" in the name certainly looks very out of place in the name of a DAPM control. > + /* Output path */ > + SND_SOC_DAPM_MUX("Passthrough Left Playback Switch", SND_SOC_NOPM, 0, 0, &cs42l52_hpa_mux), Normally "Bypass" instead of "Passthrough". > + /* Mic Bias */ > + {"Mic Bias Capture Switch", "On", "PGA MICA"}, > + {"Mic Bias Capture Switch", "On", "PGA MICB"}, > + {"Mic-Bias", NULL, "Mic Bias Capture Switch"}, > + {"Mic-Bias", NULL, "Mic Bias Capture Switch"}, > + {"ADC Mux Left Capture Switch", "PGA", "Mic-Bias"}, > + {"ADC Mux Right Capture Switch", "PGA", "Mic-Bias"}, > + {"Passthrough Left Playback Switch", "On", "Mic-Bias"}, > + {"Passthrough Right Playback Switch", "On", "Mic-Bias"}, This looks odd - normally the microphone bias would be connected to the microphone input externally and would not be hooked up as an input wihtin the CODEC? > + /* terminator */ > + {NULL, NULL, NULL}, Not needed any more (I'm vaugely surprised this doesn't generate an error). > +#define SOC_CS42L52_RATES ( SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 | \ > + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | \ > + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \ > + SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 | \ > + SNDRV_PCM_RATE_96000 ) /*refer to cs42l52 datasheet page35*/ This is equivalent to SNDRV_PCM_RATE_8000_96000 > +/* #define CONFIG_MANUAL_CLK */ This might use some comments (or should possibly be deleted)? > + soc_cs42l52_set_bias_level(soc_codec, SND_SOC_BIAS_OFF); > + soc_codec->bias_level = SND_SOC_BIAS_STANDBY; > + schedule_delayed_work(&soc_codec->delayed_work, msecs_to_jiffies(1000)); > + soc_cs42l52_required_setup(soc_codec); A comment about what the delayed work is up to wouldn't hurt? > + /*set hp default volume*/ > + soc_cs42l52_write(soc_codec, CODEC_CS42L52_HPA_VOL, DEFAULT_HP_VOL); > + soc_cs42l52_write(soc_codec, CODEC_CS42L52_HPB_VOL, DEFAULT_HP_VOL); For pretty much all the configuration you're doing here just let userspace set things up, there's no telling what's an appropriate setting on any given system and presumably the chip designers made a reasonable effort to ensure that the default state of the chip is at least not harmful. > +#ifdef CONFIG_MANUAL_CLK > + soc_cs42l52_write(soc_codec, CODEC_CS42L52_CLK_CTL, > + (soc_cs42l52_read(soc_codec, CODEC_CS42L52_CLK_CTL) > + & ~CLK_CTL_AUTODECT_ENABLE)); Pass this configuration in as platform data, this will allow multiple boards to build from the same kernel. > + ret = snd_soc_init_card(soc_dev); > + if(ret) > + { > + SOCERR("add snd card failed\n"); > + goto card_err; > + } Please develop against git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git for-2.6.35 (or whatever the latest kernel version branch is at any given time.) You should also be calling snd_soc_register_codec() and snd_soc_register_dai() from this function. > +#if defined (CONFIG_I2C) || defined (CONFIG_I2C_MODULE) No need for the ifdefs if you only support I2C, the ifdefs are used by drivers that have multiple control interfaces (typically I2C plus SPI). > + } > + > + > + return ret; > +} > + > +static int cs42l52_i2c_remove(struct i2c_client *client) > +{ > + struct snd_soc_codec *soc_codec; > + if(client) > + { > + soc_codec = i2c_get_clientdata(client); > + if(soc_codec) > + kfree(soc_codec->reg_cache); > + } > + return 0; > +} This should be unregistering from ASoC as well, or refusing to remove if not implemented. > + case SND_SOC_DAIFMT_DSP_B: > + SOCINF("unsupported format\n"); > + ret = -EINVAL; > + goto done; > + default: > + SOCINF("invaild format\n"); > + ret = -EINVAL; > + goto done; Could just squash these together. > + case SND_SOC_DAIFMT_IB_IF: > + SOCDBG("codec dai fmt inversed sclk\n"); > + iface |= IFACE_CTL1_INV_SCLK; > + break; > + case SND_SOC_DAIFMT_IB_NF: > + iface |= IFACE_CTL1_INV_SCLK; > + break; It looks like the hardware only supports the inversion of one of the clocks so only two of the clock polarity combinations should be supported? > + info->format = iface; I'd really expect to see interaction with the hardware here? > + /* 96k */ > + {12288000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K, CLK_CTL_NOT_27M, CLK_CTL_RATIO_128, 0}, > + {18432000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K, CLK_CTL_NOT_27M, CLK_CTL_RATIO_128, 0},/*29*/ > + {12000000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K, CLK_CTL_NOT_27M, CLK_CTL_RATIO_125, 0}, > + {24000000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K, CLK_CTL_NOT_27M, CLK_CTL_RATIO_125, 1}, If you're feeling adventurous you should be able to use this data to set up constraints for the DAI in the startup() callback so only sample rates supported from the system clock can be used by userspace. > +static int soc_cs42l52_pcm_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai) > +{ > + int ret = 0; > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct snd_soc_device *soc_dev = rtd->socdev; > + struct snd_soc_codec *soc_codec = soc_dev->card->codec; > + struct soc_codec_cs42l52 *info = (struct soc_codec_cs42l52*)soc_codec->private_data; > + > + u32 clk = 0; > + int index = soc_cs42l52_get_clk(info->sysclk, params_rate(params)); > +static int soc_cs42l52_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct snd_soc_device *soc_dev = (struct snd_soc_device*)platform_get_drvdata(pdev); > + struct snd_soc_codec *soc_codec = soc_dev->card->codec; > + > + soc_cs42l52_write(soc_codec, CODEC_CS42L52_PWCTL1, PWCTL1_PDN_CODEC); > + soc_cs42l52_set_bias_level(soc_codec, SND_SOC_BIAS_OFF); I'd expect that set_bias_level() would do what the explict write does? > +#if defined (CONFIG_I2C) || defined (CONFIG_I2C_MODULE) > + if(soc_codec_data->i2c_addr) > + { > + ret = i2c_add_driver(&cs42l52_i2c_drv); > + > + if(ret) > + { > + SOCERR("register i2c driver failed\n"); > + goto out; > + > + } > + SOCINF("Cirrus Logic ALSA SoC codec cs42l52 driver verison 1.2 2/2010\n"); > + return ret; > + } This should be changed to use standard device model registration - see http://git.kernel.org/?p=linux/kernel/git/broonie/sound-2.6.git;a=commit;h=… for an example of doing the conversion. > +struct soc_codec_cs42l52_data{ > + u_short i2c_addr; > +}; With the device model stuff this won't be needed any more. > +/* > + *CS42L52 internal registers > + */ > +#define CODEC_CS42L52_CHIP 0x01 > +#define CHIP_ID 0xE0 > +#define CHIP_ID_MASK 0xF8 Many of the register names and other definitions for register fields and values need to be namespace to avoid collisions.

4 5

Re: [alsa-devel] Driver for TerraTec DMX 6Fire USB
by Takashi Iwai 16 Jun '11

16 Jun '11

At Tue, 18 Jan 2011 14:41:47 +0100, torsten.schenk wrote: > > Hello, > > (to all those who already got this mail: sorry, I sent the attachment accidentally in html format so it was ignored by alsa-devel ML distributor) > > i recently got a TerraTec DMX 6Fire USB. Since it's not supported by alsa, I decided to look into communications between the windows driver and the card. Here is the result. > > What should be revised: > - I was unsure about naming conventions, so please check the name of card/midi/pcm/module. > - Also the device needs firmware to be able to run, so I included the firmware from the windows driver in the attachment. I read about that Linus Torvalds doesn't like .ihex-parsing in drivers, but I cannot convert the files easily into a binary format because the offsets are not contiguous. Of course there could be alternate ways to be thought about, which would imply that original files from TT are not supported since they are in ihex. > - I didn't manage to create a configuration in /usr/share/alsa/cards. > > What is working: Everything except SPDIF > - Hardware Master volume > - PCM 44-192kHz@24 bits, 6 channels out, 4 channels in (analog) > - MIDI in/out > - firmware loading for coldplugged devices > - phono/line switching > > If anything else comes to your attention, just let me know. Sorry for the late reply. It took some time until I have free slot. First off, there are lots of coding-style issues to be fixed. Try to run scripts/checkpatch.pl once with your patch. Not all warnings have to be fixed (e.g. 80-chars) but would be nice if fixed. Here are some review comments through a very quirk glance: > +static int __devinit probe(struct usb_interface *intf, const struct usb_device_id *usb_id) Adding a short prefix (e.g. usb6fire_probe()) is better in general. Otherwise the stack trace would be difficult to read. Of course, too long names are also annoying, so it's a matter of taste. ... > + chip = (struct sfire_chip*)card->private_data; You can avoid cast here. > + chips[regidx] = chip; > + chip->dev = device; > + chip->regidx = regidx; > + chip->intf_count = 1; > + chip->card = card; > + chip->shutdown = FALSE; > + chip->midi = NULL; > + chip->comm = NULL; > + chip->control = NULL; > + chip->pcm = NULL; NULLs are automatically filled, so these can be skipped. > + ret = snd_usb_6fire_comm_init(chip); > + if(ret < 0) > + { > + snd_card_free(card); > + return ret; > + } > + > + ret = snd_usb_6fire_midi_init(chip); > + if(ret < 0) > + { > + snd_usb_6fire_comm_destroy(chip); > + snd_card_free(card); > + return ret; > + } > + > + ret = snd_usb_6fire_pcm_init(chip); > + if(ret < 0) > + { > + snd_usb_6fire_midi_destroy(chip); > + snd_usb_6fire_comm_destroy(chip); > + snd_card_free(card); > + return ret; > + } > + > + ret = snd_usb_6fire_control_init(chip); > + if(ret < 0) > + { > + snd_usb_6fire_pcm_destroy(chip); > + snd_usb_6fire_midi_destroy(chip); > + snd_usb_6fire_comm_destroy(chip); > + snd_card_free(card); > + return ret; > + } > + > + ret = snd_card_register(card); > + if(ret < 0) > + { > + snd_usb_6fire_pcm_destroy(chip); > + snd_usb_6fire_midi_destroy(chip); > + snd_usb_6fire_comm_destroy(chip); > + snd_usb_6fire_control_destroy(chip); > + snd_card_free(card); A general way for error-handling is either using goto or using a common destructor. In either way, you can call the *_free or *_destroy with the non-NULL check of each instance. It's a cleaner in general. > +static void disconnect(struct usb_interface *intf) > +{ > + struct sfire_chip *chip; > + struct snd_card *card; > + > + chip = usb_get_intfdata(intf); > + if(chip) //if !chip, fw upload has been performed so that we don't have a chip. > + { > + card = chip->card; > + chip->intf_count--; > + if (chip->intf_count == 0) > + { > + chip->shutdown = TRUE; > + snd_card_disconnect(card); > + snd_usb_6fire_comm_destroy(chip); > + snd_usb_6fire_control_destroy(chip); > + snd_usb_6fire_pcm_destroy(chip); > + snd_usb_6fire_midi_destroy(chip); > + snd_card_free(card); > + usb_set_intfdata(intf, NULL); The same destructor as in probe can be used here. > +static struct usb_device_id device_table[] = > +{ > + { > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE, > + .idVendor = 0x0ccd, > + .idProduct = 0x0080 > + }, > + { } > +}; Usually MODULE_DEVICE_TABLE() follows just after the definition. > diff -Nur a/sound/usb/6fire/chip.h b/sound/usb/6fire/chip.h > --- a/sound/usb/6fire/chip.h 1970-01-01 01:00:00.000000000 +0100 > +++ b/sound/usb/6fire/chip.h 2011-01-18 13:58:06.000000000 +0100 > @@ -0,0 +1,30 @@ > +/* > + * Linux driver for TerraTec DMX 6Fire USB > + * > + * Author: Torsten Schenk > + * Created: Jan 01, 2011 > + * Copyright: (C) Torsten Schenk > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#pragma once Don't use prgram. Also, add #ifdef xxx for protecting the double inclusion. > +#include "common.h" > + > +struct sfire_chip > +{ > + struct usb_device *dev; > + struct snd_card *card; > + int intf_count; //number of registered interfaces > + int regidx; //index in module parameter arrays > + BOOL shutdown; Don't use own bool type. Use the generic "bool" type, and use true/false instead of TRUE/FALSE (unless the size really matters). > diff -Nur a/sound/usb/6fire/comm.c b/sound/usb/6fire/comm.c > --- a/sound/usb/6fire/comm.c 1970-01-01 01:00:00.000000000 +0100 > +++ b/sound/usb/6fire/comm.c 2011-01-18 13:58:21.000000000 +0100 ... > +static int write_02(struct comm_runtime *rt, __u8 reg, __u16 value) Use u8 and u16. > diff -Nur a/sound/usb/6fire/comm.h b/sound/usb/6fire/comm.h > --- a/sound/usb/6fire/comm.h 1970-01-01 01:00:00.000000000 +0100 > +++ b/sound/usb/6fire/comm.h 2011-01-18 13:58:29.000000000 +0100 > @@ -0,0 +1,42 @@ > +/* > + * Linux driver for TerraTec DMX 6Fire USB > + * > + * Author: Torsten Schenk > + * Created: Jan 01, 2011 > + * Copyright: (C) Torsten Schenk > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#pragma once Ditto. No pragma and add ifdef. > diff -Nur a/sound/usb/6fire/common.h b/sound/usb/6fire/common.h > --- a/sound/usb/6fire/common.h 1970-01-01 01:00:00.000000000 +0100 > +++ b/sound/usb/6fire/common.h 2011-01-18 13:58:38.000000000 +0100 > +#pragma once > + > +#include <linux/slab.h> > +#include <linux/usb.h> > +#include <sound/core.h> > + > +#define BOOL char > +#define TRUE 1 > +#define FALSE 0 Don't use these. > +#define MIN(X, Y) ((X) < (Y) ? (X) : (Y)) > +#define MAX(X, Y) ((X) > (Y) ? (X) : (Y)) Use standard min() / max() macros. > +#define ERR KERN_ERR "6fire: " > +#define WARNING KERN_WARNING "6fire: " Hmm... These are also better to be avoided. At most, define "6fire: " as PREFIX or PFX or whatever. But KERN_ERR should be put plainly. > diff -Nur a/sound/usb/6fire/control.c b/sound/usb/6fire/control.c > --- a/sound/usb/6fire/control.c 1970-01-01 01:00:00.000000000 +0100 > +++ b/sound/usb/6fire/control.c 2011-01-18 13:58:57.000000000 +0100 > diff -Nur a/sound/usb/6fire/firmware.c b/sound/usb/6fire/firmware.c > --- a/sound/usb/6fire/firmware.c 1970-01-01 01:00:00.000000000 +0100 > +++ b/sound/usb/6fire/firmware.c 2011-01-18 13:57:15.000000000 +0100 ... > +static inline int fpga_write(struct usb_device *device, char *data, int len) Avoid inline unless you really need it. Also add MODULE_FIRMWARE() definitions in this file. > diff -Nur a/sound/usb/6fire/Makefile b/sound/usb/6fire/Makefile > --- a/sound/usb/6fire/Makefile 1970-01-01 01:00:00.000000000 +0100 > +++ b/sound/usb/6fire/Makefile 2011-01-16 23:14:22.000000000 +0100 > @@ -0,0 +1,13 @@ > +obj-m += snd-usb-6fire.o > +snd-usb-6fire-objs += chip.o comm.o midi.o control.o firmware.o pcm.o > + > +all: > + make CONFIG_DEBUG_SECTION_MISMATCH=y -C /lib/modules/$(shell uname -r)/build M=$(PWD) modules > + > +clean: > + make -C /lib/modules/$(shell uname -r)/build M=$(PWD) clean > + > +install: > + mkdir -p /lib/modules/$(shell uname -r)/extras > + cp snd-usb-6fire.ko /lib/modules/$(shell uname -r)/extras Remove all, clean, install things. > diff -Nur a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c > --- a/sound/usb/6fire/pcm.c 1970-01-01 01:00:00.000000000 +0100 > +++ b/sound/usb/6fire/pcm.c 2011-01-18 13:59:57.000000000 +0100 > diff -Nur a/sound/usb/Kconfig b/sound/usb/Kconfig > --- a/sound/usb/Kconfig 2011-01-16 23:20:41.000000000 +0100 > +++ b/sound/usb/Kconfig 2011-01-16 23:14:22.000000000 +0100 > @@ -32,6 +32,17 @@ > To compile this driver as a module, choose M here: the module > will be called snd-ua101. > +config SND_USB_6FIRE > + tristate "TerraTec DMX 6Fire USB" > + depends on X86 > + select SND_HWDEP Do you really need hwdep? Rather select FW_LOADER, if any. > + select SND_PCM Missing SND_RAWMIDI dependency. > @@ -65,7 +76,6 @@ > * Native Instruments Guitar Rig Session I/O > * Native Instruments Guitar Rig mobile > * Native Instruments Traktor Kontrol X1 > - * Native Instruments Traktor Kontrol S4 > To compile this driver as a module, choose M here: the module > will be called snd-usb-caiaq. > @@ -83,7 +93,6 @@ > * Native Instruments Kore Controller > * Native Instruments Kore Controller 2 > * Native Instruments Audio Kontrol 1 > - * Native Instruments Traktor Kontrol S4 Don't bother others ;) thanks, Takashi

2 20

[alsa-devel] [RFC PATCH] Add combined clock support to Atmel SoC audio
by Ryan Mallon 07 Jun '11

07 Jun '11

The following patch is one that has been floating around in various forms in our own internal trees for a while. The Atmel SSC peripheral has seperate TX and RX clocks which use separate pins from the the micro. TF (frame) and TK (clock) for transmit and RF and RK for receive. Not all codecs have separate frame and bit clocks for transmit and receive so we want to be able to do both playback and capture using a single set of pins. This patch introduces a combined clock mode for the Atmel SSC peripheral. Which allows playback and capture to use a single set of pins. Currently combined clock is only supported on the TF/TK pins (some incomplete support exists for using RF/RK). I have tested this patch on our AT91SAM9G45 + TLV320AIC26 platform. Playback and capture work individually. Simultaneous playback and capture have been tested by connecting a loopback cable on the linein and lineout jacks and then doing: arecord -c 2 -f S16_LE -r 44100 > recording.wav & aplay 500hz_sine.wav This patch is posted as RFC since the approach is incomplete and a bit hackish. I am mostly interested in knowing if this is a sensible approach, and could be cleaned up for mainline inclusion, or if there is a better way to do this. Signed-off-by: Ryan Mallon <ryan(a)bluewatersys.com> --- diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c index 5d230ce..ee00172 100644 --- a/sound/soc/atmel/atmel_ssc_dai.c +++ b/sound/soc/atmel/atmel_ssc_dai.c @@ -43,6 +43,7 @@ #include <sound/soc.h> #include <mach/hardware.h> +#include <asm/atomic.h> #include "atmel-pcm.h" #include "atmel_ssc_dai.h" @@ -246,7 +247,18 @@ static void atmel_ssc_shutdown(struct snd_pcm_substream *substream, dma_params = ssc_p->dma_params[dir]; if (dma_params != NULL) { - ssc_writel(ssc_p->ssc->regs, CR, dma_params->mask->ssc_disable); + if (ssc_p->combined_clock) { + /* + * When using a combined clock we only disable the + * clock once all substreams have completed + */ + if (atomic_dec_and_test(&ssc_p->substreams_running)) + ssc_writel(ssc_p->ssc->regs, CR, + dma_params->mask->ssc_disable); + } else { + ssc_writel(ssc_p->ssc->regs, CR, + dma_params->mask->ssc_disable); + } pr_debug("atmel_ssc_shutdown: %s disabled SSC_SR=0x%08x\n", (dir ? "receive" : "transmit"), ssc_readl(ssc_p->ssc->regs, SR)); @@ -328,6 +340,21 @@ static int atmel_ssc_set_dai_clkdiv(struct snd_soc_dai *cpu_dai, return 0; } +void atmel_ssc_setup_combined_clock(struct atmel_ssc_info *ssc_p, + int combined_clock) +{ + int i; + + ssc_p->combined_clock = combined_clock; + for (i = 0; i < ARRAY_SIZE(ssc_dma_params); i++) { + if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_TX_ON_RX) + ssc_dma_params[i][0].mask->ssc_enable |= SSC_BIT(CR_RXEN); + else if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_RX_ON_TX) + ssc_dma_params[i][1].mask->ssc_enable |= SSC_BIT(CR_TXEN); + } +} +EXPORT_SYMBOL(atmel_ssc_setup_combined_clock); + /* * Configure the SSC. */ @@ -548,6 +575,34 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream, ssc_p->daifmt); return -EINVAL; } + + if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_RX_ON_TX) { + /* RX clock is always running */ + rcmr &= ~SSC_BF(RCMR_CKO, 0x7); + rcmr |= SSC_BF(RCMR_CKO, SSC_CKO_CONTINUOUS); + + /* TX clock is always running */ + tcmr &= ~SSC_BF(TCMR_CKO, 0x7); + tcmr |= SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS); + + /* RX clock is sourced from TK pin */ + rcmr &= ~SSC_BF(RCMR_CKS, 0x7); + rcmr |= SSC_BF(RCMR_CKS, SSC_CKS_CLOCK); + + /* Start RX on TX start */ + rcmr &= ~SSC_BF(RCMR_START, 0xf); + rcmr |= SSC_BF(RCMR_START, SSC_START_TX_RX); + + if (dir == 1) { + /* + * Set the TX clock period to the RX clock period + * FIXME - Is this okay if we are already doing TX? + */ + tcmr &= 0x00ffffff; + tcmr |= rcmr & 0xff000000; + } + } + pr_debug("atmel_ssc_hw_params: " "RCMR=%08x RFMR=%08x TCMR=%08x TFMR=%08x\n", rcmr, rfmr, tcmr, tfmr); @@ -581,6 +636,9 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream, return ret; } + if (ssc_p->combined_clock) + atomic_set(&ssc_p->substreams_running, 0); + ssc_p->initialized = 1; } @@ -616,6 +674,21 @@ static int atmel_ssc_prepare(struct snd_pcm_substream *substream, ssc_writel(ssc_p->ssc->regs, CR, dma_params->mask->ssc_enable); + if (ssc_p->combined_clock) { + atomic_inc(&ssc_p->substreams_running); + + if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_RX_ON_TX && + dir == 1) { + /* + * Jump start the dma for capture by loading the + * transmit holding register with a dummy value. + */ + pr_debug("Jump starting SSC RX DMA\n"); + if (!(ssc_readl(ssc_p->ssc->regs, SR) & (1 << 16))) + ssc_writel(ssc_p->ssc->regs, THR, 0); + } + } + pr_debug("%s enabled SSC_SR=0x%08x\n", dir ? "receive" : "transmit", ssc_readl(ssc_p->ssc->regs, SR)); diff --git a/sound/soc/atmel/atmel_ssc_dai.h b/sound/soc/atmel/atmel_ssc_dai.h index 5d4f0f9..6b20ee6 100644 --- a/sound/soc/atmel/atmel_ssc_dai.h +++ b/sound/soc/atmel/atmel_ssc_dai.h @@ -102,6 +102,8 @@ struct atmel_ssc_state { u32 ssc_imr; }; +#define ATMEL_SSC_CLOCK_RX_ON_TX 1 +#define ATMEL_SSC_CLOCK_TX_ON_RX 2 struct atmel_ssc_info { char *name; @@ -115,8 +117,14 @@ struct atmel_ssc_info { unsigned short rcmr_period; struct atmel_pcm_dma_params *dma_params[2]; struct atmel_ssc_state ssc_state; + + /* For combined clocks */ + int combined_clock; + atomic_t substreams_running; }; int atmel_ssc_set_audio(int ssc); +extern void atmel_ssc_setup_combined_clock(struct atmel_ssc_info *ssc_p, + int combined_clock); #endif /* _AT91_SSC_DAI_H */

3 6