[alsa-devel] [PATCH - CA0132 HDA Codec 1/2] ALSA: Update Creative CA0132 codec to add DSP features.

[Takashi Iwai]

At Fri, 13 Jul 2012 16:44:28 -0700,
ian_minett at creativelabs.com wrote:
> 
> From: Ian Minett <ian_minett at 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