At Fri, 13 Jul 2012 16:44:28 -0700, ian_minett@creativelabs.com wrote:
From: Ian Minett ian_minett@creativelabs.com
Thanks very much for the feedback on the last submission, apologies for the delayed response. Please find attached the new set of patches for updating the CA0132 codec, which takes many of the suggestions into account.
- Ian
ALSA: Update Creative CA0132 HDA codec to introduce DSP features.
patch_ca0132.c:
- Add DSP support to codec.
- 3 capture subdevices supported: Analog Mic1/Digital Mic,
Analog Mic2 and What U Hear.
- Add jack detection kcontrol
- Use request_firmware() for loading DSP binaries.
Ideally, these should be split to each patch. Though I see it's not so trivial in this case...
But please provide more technical details. I can only guess what's VNID and what's effect module ID. Give more descriptions.
hda_intel.c:
- in azx_pcm_trigger(), change spin_lock()/spin_unlock() pair to
spin_lock_irqsave()/spin_unlock_irqrestore(). This still seems to be necessary - if an interrupt occurs after azx_stream_start() and before spin_unlock(), it will cause deadlock as azx_interrupt() also acquires chip->reg_lock.
Oh, this is basically because you are doing pretty bad things: calling PCM trigger callback from the codec driver. What's the reason behind it?
Since the whole patch lacks the big and small pictures, it's hard to judge...
ctefx.bin:
- DSP binary for firmware repository, for driver to load from
/lib/firmware directory.
Please submit the patch for adding this, too. Or, at least, give a kernel config to build without the firmware. Otherwise you'll break the driver.
Looking though the code, the biggest problem is that you are using float in the code. This is now allowed in the kernel at all. You have to rewrite without float type -- i.e. never use "float" in the code, for both fields/arguments and computations.
Also, some other review comments below:
diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index d0d3540..c7cdfe1 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c @@ -1,5 +1,5 @@ /*
- HD audio interface patch for Creative CA0132 chip
- HD audio interface patch for Creative Sound Core3D chip
- Copyright (c) 2011, Creative Technology Ltd.
@@ -25,18 +25,470 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/pci.h> -#include <linux/mutex.h> #include <linux/module.h> +#include <linux/firmware.h> #include <sound/core.h> +#include <sound/tlv.h> #include "hda_codec.h" #include "hda_local.h" +#include "hda_jack.h" #include "hda_auto_parser.h"
+#include "ca0132_regs.h"
+/* Enable this to see more debug messages. */ +/*#define ENABLE_CA0132_DEBUG*/ +/* Enable this to see controls for tuning purpose. */ +/*#define ENABLE_TUNING_CONTROLS*/
+#define FLOAT_ZERO 0x00000000 +#define FLOAT_ONE 0x3f800000 +#define FLOAT_TWO 0x40000000 +#define FLOAT_MINUS_5 0xc0a00000
+#define UNSOL_TAG_HP 0x10 +#define UNSOL_TAG_AMIC1 0x12 +#define UNSOL_TAG_DSP 0x16
+#define SUCCEEDED(_x) (((int)(_x)) >= 0) +#define FAILED(_x) (((int)(_x)) < 0) +#define CT_OK (0) +#define CT_FAIL (-1)
Please avoid home-baked macros like this. Usually a function returns zero for succcess and a negative error code. No need to define special CT_XX macros.
+#define MIN(x, y) (((x) < (y)) ? (x) : (y)) +#define MAX(x, y) (((x) > (y)) ? (x) : (y))
Use the standard min() and max() macros.
+#define DSP_DMA_WRITE_BUFLEN_INIT (1UL<<18) +#define DSP_DMA_WRITE_BUFLEN_OVLY (1UL<<15)
+#define DMA_TRANSFER_FRAME_SIZE_NWORDS 8 +#define DMA_TRANSFER_MAX_FRAME_SIZE_NWORDS 32 +#define DMA_OVERLAY_FRAME_SIZE_NWORDS 2
+#define MASTERCONTROL 0x80 +#define MASTERCONTROL_ALLOC_DMA_CHAN 9 +#define MASTERCONTROL_QUERY_SPEAKER_EQ_ADDRESS 22
#define WIDGET_CHIP_CTRL 0x15 #define WIDGET_DSP_CTRL 0x16
-#define WUH_MEM_CONNID 10 -#define DSP_MEM_CONNID 16 +#define MEM_CONNID_MICIN1 3 +#define MEM_CONNID_MICIN2 5 +#define MEM_CONNID_MICOUT1 12 +#define MEM_CONNID_MICOUT2 14 +#define MEM_CONNID_WUH 10 +#define MEM_CONNID_DSP 16 +#define MEM_CONNID_DMIC 100
+#define SCP_SET 0 +#define SCP_GET 1
+#define SPEQ_FILE "ctspeq.bin" +#define SPEQ_SIZE 0x2040 +#define EFX_FILE "ctefx.bin" +#define EFX_SIZE 0xA003C
Don't forget to add MODULE_FIRMWARE() with these files, too.
I stop at this point... Too big to review...
thanks,
Takashi