[alsa-devel] [PATCH 1/4] ALSA: hdsp - Fix detection for RME RPM/Multiface/Digiface ioboxes

[Takashi Iwai]

At Tue, 15 Jan 2013 15:18:10 +0100,
Adrian Knoth wrote:
> 
> The current iobox detection code reportedly fails for various users, so
> simply do what the Win32 driver does instead.
> 
> Patch originally by Karl Grill <kgrill at chello.at> and then modified to
> comply with kernel coding guidelines + current HEAD.
> 
> Signed-off-by: Adrian Knoth <adi at drcomp.erfurt.thur.de>
> 
> diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c
> index 4fae81f..ba190d5 100644
> --- a/sound/pci/rme9652/hdsp.c
> +++ b/sound/pci/rme9652/hdsp.c
> @@ -154,10 +154,12 @@ MODULE_FIRMWARE("digiface_firmware_rev11.bin");
>  #define HDSP_BIGENDIAN_MODE     0x200
>  #define HDSP_RD_MULTIPLE        0x400
>  #define HDSP_9652_ENABLE_MIXER  0x800
> +#define HDSP_S200		0x800
> +#define HDSP_CYCLIC_MODE	0x1000
>  #define HDSP_TDO                0x10000000
>  
> -#define HDSP_S_PROGRAM     	(HDSP_PROGRAM|HDSP_CONFIG_MODE_0)
> -#define HDSP_S_LOAD		(HDSP_PROGRAM|HDSP_CONFIG_MODE_1)
> +#define HDSP_S_PROGRAM	    (HDSP_CYCLIC_MODE|HDSP_PROGRAM|HDSP_CONFIG_MODE_0)
> +#define HDSP_S_LOAD	    (HDSP_CYCLIC_MODE|HDSP_PROGRAM|HDSP_CONFIG_MODE_1)
>  
>  /* Control Register bits */
>  
> @@ -671,13 +673,23 @@ static unsigned int hdsp_read(struct hdsp *hdsp, int reg)
>  
>  static int hdsp_check_for_iobox (struct hdsp *hdsp)
>  {
> +	int i;
> +
>  	if (hdsp->io_type == H9652 || hdsp->io_type == H9632) return 0;
> -	if (hdsp_read (hdsp, HDSP_statusRegister) & HDSP_ConfigError) {
> -		snd_printk("Hammerfall-DSP: no IO box connected!\n");
> -		hdsp->state &= ~HDSP_FirmwareLoaded;
> -		return -EIO;
> +	for (i = 0; i < 500; i++) {
> +		if (0 == (hdsp_read(hdsp, HDSP_statusRegister) &
> +					HDSP_ConfigError)) {
> +			if (i) {
> +				snd_printk("Hammerfall-DSP: IO box found after %d ms\n",
> +						(20 * i));

One more favor: please make it only as a debug print.
It's not good to print this positive result always.
That is, simply replace it with snd_printd().

> +			}
> +			return 0;
> +		}
> +		msleep(20);
>  	}
> -	return 0;
> +	snd_printk("Hammerfall-DSP: no IO box connected!\n");

Also, recommended to put KERN_ERR prefix here.

> +	hdsp->state &= ~HDSP_FirmwareLoaded;
> +	return -EIO;
>  }
>  
>  static int hdsp_wait_for_iobox(struct hdsp *hdsp, unsigned int loops,
> @@ -728,6 +740,7 @@ static int snd_hdsp_load_firmware_from_cache(struct hdsp *hdsp) {
>  
>  		if (hdsp_fifo_wait (hdsp, 0, HDSP_LONG_WAIT)) {
>  			snd_printk ("Hammerfall-DSP: timeout waiting for download preparation\n");
> +			hdsp_write(hdsp, HDSP_control2Reg, HDSP_S200);
>  			return -EIO;
>  		}
>  
> @@ -737,17 +750,15 @@ static int snd_hdsp_load_firmware_from_cache(struct hdsp *hdsp) {
>  			hdsp_write(hdsp, HDSP_fifoData, cache[i]);
>  			if (hdsp_fifo_wait (hdsp, 127, HDSP_LONG_WAIT)) {
>  				snd_printk ("Hammerfall-DSP: timeout during firmware loading\n");
> +				hdsp_write(hdsp, HDSP_control2Reg, HDSP_S200);
>  				return -EIO;
>  			}
>  		}
>  
> -		ssleep(3);
> -
> -		if (hdsp_fifo_wait (hdsp, 0, HDSP_LONG_WAIT)) {
> -			snd_printk ("Hammerfall-DSP: timeout at end of firmware loading\n");
> -		    	return -EIO;
> -		}
> +		hdsp_fifo_wait(hdsp, 3, HDSP_LONG_WAIT);
> +		hdsp_write(hdsp, HDSP_control2Reg, HDSP_S200);
>  
> +		ssleep(3);
>  #ifdef SNDRV_BIG_ENDIAN
>  		hdsp->control2_register = HDSP_BIGENDIAN_MODE;
>  #else
> @@ -773,24 +784,51 @@ static int hdsp_get_iobox_version (struct hdsp *hdsp)
>  {
>  	if ((hdsp_read (hdsp, HDSP_statusRegister) & HDSP_DllError) != 0) {
>  
> -		hdsp_write (hdsp, HDSP_control2Reg, HDSP_PROGRAM);
> -		hdsp_write (hdsp, HDSP_fifoData, 0);
> -		if (hdsp_fifo_wait (hdsp, 0, HDSP_SHORT_WAIT) < 0)
> -			return -EIO;
> +		hdsp_write(hdsp, HDSP_control2Reg, HDSP_S_LOAD);
> +		hdsp_write(hdsp, HDSP_fifoData, 0);
>  
> -		hdsp_write (hdsp, HDSP_control2Reg, HDSP_S_LOAD);
> +		if (hdsp_fifo_wait(hdsp, 0, HDSP_SHORT_WAIT) < 0) {
> +			hdsp_write(hdsp, HDSP_control2Reg, 0x100 | HDSP_S200);

0x100 isn't replaced with a literal?
Maybe worth to define (0x100 | HDSP_S200) to something meaningful, as
this appears repeatedly?


thanks,

Takashi