diff mbox

[bug] Volume at maximum when track with different frequency is played with my RME sound card

Message ID s5h4mfy1mwm.wl-tiwai@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Iwai Dec. 4, 2015, 11:19 a.m. UTC
On Fri, 04 Dec 2015 11:44:02 +0100,
Maeda wrote:
> 
> Hi !
> 
> Well done ! It plays without max loud volume, and no errors on output.
> I tried with the following test :
> 
>     `aplay -fcd -d3 /dev/zero ;aplay -r96000 -c2 -d3 /dev/zero
>     ;speaker-test  -c2 -twav`
> 
> And it's OK. Then I tried with mplayer and some flac files (44.1 / 48 
> and 96 KHz sample) : no problem.
> 
> I think you found the solution. If there are some future problem this 
> patch generates, I'll only see when using "it" each day. Then, for now, 
> you can commit it ;)

Good to hear.  Could you try the revised patch below instead?
It does restore DAC volume at the end, after the spinlock, so that we
can avoid the ugly delay.


thanks,

Takashi

---

Comments

Maeda Dec. 4, 2015, 12:12 p.m. UTC | #1
I've problem with compiling rme96.c (see attachment).
Maybe I forgot something, i double checked, but my rme96.c seems to be good.

Any clue ?

Le 04/12/2015 12:19, Takashi Iwai a écrit :
> On Fri, 04 Dec 2015 11:44:02 +0100,
> Maeda wrote:
>> Hi !
>>
>> Well done ! It plays without max loud volume, and no errors on output.
>> I tried with the following test :
>>
>>      `aplay -fcd -d3 /dev/zero ;aplay -r96000 -c2 -d3 /dev/zero
>>      ;speaker-test  -c2 -twav`
>>
>> And it's OK. Then I tried with mplayer and some flac files (44.1 / 48
>> and 96 KHz sample) : no problem.
>>
>> I think you found the solution. If there are some future problem this
>> patch generates, I'll only see when using "it" each day. Then, for now,
>> you can commit it ;)
> Good to hear.  Could you try the revised patch below instead?
> It does restore DAC volume at the end, after the spinlock, so that we
> can avoid the ugly delay.
>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
> index 714df906249e..e4229d01cf6a 100644
> --- a/sound/pci/rme96.c
> +++ b/sound/pci/rme96.c
> @@ -741,10 +741,11 @@ snd_rme96_playback_setrate(struct rme96 *rme96,
>   	{
>   		/* change to/from double-speed: reset the DAC (if available) */
>   		snd_rme96_reset_dac(rme96);
> +		return 1; /* need to restore volume */
>   	} else {
>   		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> +		return 0;
>   	}
> -	return 0;
>   }
>   
>   static int
> @@ -980,6 +981,7 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>   	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
>   	struct snd_pcm_runtime *runtime = substream->runtime;
>   	int err, rate, dummy;
> +	bool apply_dac_volume = false;
>   
>   	runtime->dma_area = (void __force *)(rme96->iobase +
>   					     RME96_IO_PLAY_BUFFER);
> @@ -993,24 +995,24 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>   	{
>                   /* slave clock */
>                   if ((int)params_rate(params) != rate) {
> -			spin_unlock_irq(&rme96->lock);
> -			return -EIO;
> -                }
> -	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
> -		spin_unlock_irq(&rme96->lock);
> -		return err;
> -	}
> -	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
> -		spin_unlock_irq(&rme96->lock);
> -		return err;
> +			err = -EIO;
> +			goto error;
> +		}
> +	} else {
> +		err = snd_rme96_playback_setrate(rme96, params_rate(params));
> +		if (err < 0)
> +			goto error;
> +		apply_dac_volume = err > 0; /* need to restore volume later? */
>   	}
> +	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
> +		goto error;
>   	snd_rme96_setframelog(rme96, params_channels(params), 1);
>   	if (rme96->capture_periodsize != 0) {
>   		if (params_period_size(params) << rme96->playback_frlog !=
>   		    rme96->capture_periodsize)
>   		{
> -			spin_unlock_irq(&rme96->lock);
> -			return -EBUSY;
> +			err = -EBUSY;
> +			goto error;
>   		}
>   	}
>   	rme96->playback_periodsize =
> @@ -1022,8 +1024,15 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>   		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>   	}
>   	spin_unlock_irq(&rme96->lock);
> +	err = 0;
>   		
> -	return 0;
> + error:
> +	if (apply_dac_volume) {
> +		usleep_range(3000, 10000);
> +		snd_rme96_apply_dac_volume(rme96);
> +	}
> +
> +	return err;
>   }
>   
>   static int
CC [M]  sound/pci/rme96.o
sound/pci/rme96.c: Dans la fonction ‘snd_rme96_playback_hw_params’:
sound/pci/rme96.c:1008:3: erreur: label ‘error’ used but not defined
   goto error;
   ^
sound/pci/rme96.c:1009:2: attention : « return » manquant dans une fonction devant retourner une valeur [-Wreturn-type]
  }
  ^
sound/pci/rme96.c: Hors de toute fonction :
sound/pci/rme96.c:1010:46: erreur: expected ‘)’ before ‘(’ token
  snd_rme96_setframelog(rme96, params_channels(params), 1);
                                              ^
sound/pci/rme96.c:1011:2: erreur: expected identifier or ‘(’ before ‘if’
  if (rme96->capture_periodsize != 0) {
  ^
sound/pci/rme96.c:1019:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
  rme96->playback_periodsize =
       ^
sound/pci/rme96.c:1021:46: erreur: expected ‘)’ before ‘->’ token
  snd_rme96_set_period_properties(rme96, rme96->playback_periodsize);
                                              ^
sound/pci/rme96.c:1023:2: erreur: expected identifier or ‘(’ before ‘if’
  if ((rme96->wcreg & RME96_WCR_ADAT) == 0) {
  ^
sound/pci/rme96.c:1027:18: erreur: expected declaration specifiers or ‘...’ before ‘&’ token
  spin_unlock_irq(&rme96->lock);
                  ^
sound/pci/rme96.c:1028:2: attention : la définition de données n'a pas de type ni de classe de stockage
  err = 0;  
  ^
sound/pci/rme96.c:1028:2: erreur: type defaults to ‘int’ in declaration of ‘err’ [-Werror=implicit-int]
sound/pci/rme96.c:1029:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘:’ token
  error:
       ^
sound/pci/rme96.c:1035:2: erreur: expected identifier or ‘(’ before ‘return’
  return err;
  ^
sound/pci/rme96.c:1036:1: erreur: expected identifier or ‘(’ before ‘}’ token
 }
Takashi Iwai Dec. 4, 2015, 12:47 p.m. UTC | #2
On Fri, 04 Dec 2015 13:12:41 +0100,
Maeda wrote:
> 
> I've problem with compiling rme96.c (see attachment).
> Maybe I forgot something, i double checked, but my rme96.c seems to be good.
> 
> Any clue ?

You must have patched wrongly.
You reverted the former patches, right?


Takashi

> 
> Le 04/12/2015 12:19, Takashi Iwai a écrit :
> > On Fri, 04 Dec 2015 11:44:02 +0100,
> > Maeda wrote:
> >> Hi !
> >>
> >> Well done ! It plays without max loud volume, and no errors on output.
> >> I tried with the following test :
> >>
> >>      `aplay -fcd -d3 /dev/zero ;aplay -r96000 -c2 -d3 /dev/zero
> >>      ;speaker-test  -c2 -twav`
> >>
> >> And it's OK. Then I tried with mplayer and some flac files (44.1 / 48
> >> and 96 KHz sample) : no problem.
> >>
> >> I think you found the solution. If there are some future problem this
> >> patch generates, I'll only see when using "it" each day. Then, for now,
> >> you can commit it ;)
> > Good to hear.  Could you try the revised patch below instead?
> > It does restore DAC volume at the end, after the spinlock, so that we
> > can avoid the ugly delay.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > ---
> > diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
> > index 714df906249e..e4229d01cf6a 100644
> > --- a/sound/pci/rme96.c
> > +++ b/sound/pci/rme96.c
> > @@ -741,10 +741,11 @@ snd_rme96_playback_setrate(struct rme96 *rme96,
> >   	{
> >   		/* change to/from double-speed: reset the DAC (if available) */
> >   		snd_rme96_reset_dac(rme96);
> > +		return 1; /* need to restore volume */
> >   	} else {
> >   		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> > +		return 0;
> >   	}
> > -	return 0;
> >   }
> >   
> >   static int
> > @@ -980,6 +981,7 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
> >   	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
> >   	struct snd_pcm_runtime *runtime = substream->runtime;
> >   	int err, rate, dummy;
> > +	bool apply_dac_volume = false;
> >   
> >   	runtime->dma_area = (void __force *)(rme96->iobase +
> >   					     RME96_IO_PLAY_BUFFER);
> > @@ -993,24 +995,24 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
> >   	{
> >                   /* slave clock */
> >                   if ((int)params_rate(params) != rate) {
> > -			spin_unlock_irq(&rme96->lock);
> > -			return -EIO;
> > -                }
> > -	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
> > -		spin_unlock_irq(&rme96->lock);
> > -		return err;
> > -	}
> > -	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
> > -		spin_unlock_irq(&rme96->lock);
> > -		return err;
> > +			err = -EIO;
> > +			goto error;
> > +		}
> > +	} else {
> > +		err = snd_rme96_playback_setrate(rme96, params_rate(params));
> > +		if (err < 0)
> > +			goto error;
> > +		apply_dac_volume = err > 0; /* need to restore volume later? */
> >   	}
> > +	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
> > +		goto error;
> >   	snd_rme96_setframelog(rme96, params_channels(params), 1);
> >   	if (rme96->capture_periodsize != 0) {
> >   		if (params_period_size(params) << rme96->playback_frlog !=
> >   		    rme96->capture_periodsize)
> >   		{
> > -			spin_unlock_irq(&rme96->lock);
> > -			return -EBUSY;
> > +			err = -EBUSY;
> > +			goto error;
> >   		}
> >   	}
> >   	rme96->playback_periodsize =
> > @@ -1022,8 +1024,15 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
> >   		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> >   	}
> >   	spin_unlock_irq(&rme96->lock);
> > +	err = 0;
> >   		
> > -	return 0;
> > + error:
> > +	if (apply_dac_volume) {
> > +		usleep_range(3000, 10000);
> > +		snd_rme96_apply_dac_volume(rme96);
> > +	}
> > +
> > +	return err;
> >   }
> >   
> >   static int
> 
>   CC [M]  sound/pci/rme96.o
> sound/pci/rme96.c: Dans la fonction ‘snd_rme96_playback_hw_params’:
> sound/pci/rme96.c:1008:3: erreur: label ‘error’ used but not defined
>    goto error;
>    ^
> sound/pci/rme96.c:1009:2: attention : « return » manquant dans une fonction devant retourner une valeur [-Wreturn-type]
>   }
>   ^
> sound/pci/rme96.c: Hors de toute fonction :
> sound/pci/rme96.c:1010:46: erreur: expected ‘)’ before ‘(’ token
>   snd_rme96_setframelog(rme96, params_channels(params), 1);
>                                               ^
> sound/pci/rme96.c:1011:2: erreur: expected identifier or ‘(’ before ‘if’
>   if (rme96->capture_periodsize != 0) {
>   ^
> sound/pci/rme96.c:1019:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
>   rme96->playback_periodsize =
>        ^
> sound/pci/rme96.c:1021:46: erreur: expected ‘)’ before ‘->’ token
>   snd_rme96_set_period_properties(rme96, rme96->playback_periodsize);
>                                               ^
> sound/pci/rme96.c:1023:2: erreur: expected identifier or ‘(’ before ‘if’
>   if ((rme96->wcreg & RME96_WCR_ADAT) == 0) {
>   ^
> sound/pci/rme96.c:1027:18: erreur: expected declaration specifiers or ‘...’ before ‘&’ token
>   spin_unlock_irq(&rme96->lock);
>                   ^
> sound/pci/rme96.c:1028:2: attention : la définition de données n'a pas de type ni de classe de stockage
>   err = 0;  
>   ^
> sound/pci/rme96.c:1028:2: erreur: type defaults to ‘int’ in declaration of ‘err’ [-Werror=implicit-int]
> sound/pci/rme96.c:1029:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘:’ token
>   error:
>        ^
> sound/pci/rme96.c:1035:2: erreur: expected identifier or ‘(’ before ‘return’
>   return err;
>   ^
> sound/pci/rme96.c:1036:1: erreur: expected identifier or ‘(’ before ‘}’ token
>  }
Maeda Dec. 4, 2015, 12:54 p.m. UTC | #3
Oops. Forgot to do that. I'll take back the rme96.c from kernel source 
and apply your last patch again.

Le 04/12/2015 13:47, Takashi Iwai a écrit :
> On Fri, 04 Dec 2015 13:12:41 +0100,
> Maeda wrote:
>> I've problem with compiling rme96.c (see attachment).
>> Maybe I forgot something, i double checked, but my rme96.c seems to be good.
>>
>> Any clue ?
> You must have patched wrongly.
> You reverted the former patches, right?
>
>
> Takashi
>
>> Le 04/12/2015 12:19, Takashi Iwai a écrit :
>>> On Fri, 04 Dec 2015 11:44:02 +0100,
>>> Maeda wrote:
>>>> Hi !
>>>>
>>>> Well done ! It plays without max loud volume, and no errors on output.
>>>> I tried with the following test :
>>>>
>>>>       `aplay -fcd -d3 /dev/zero ;aplay -r96000 -c2 -d3 /dev/zero
>>>>       ;speaker-test  -c2 -twav`
>>>>
>>>> And it's OK. Then I tried with mplayer and some flac files (44.1 / 48
>>>> and 96 KHz sample) : no problem.
>>>>
>>>> I think you found the solution. If there are some future problem this
>>>> patch generates, I'll only see when using "it" each day. Then, for now,
>>>> you can commit it ;)
>>> Good to hear.  Could you try the revised patch below instead?
>>> It does restore DAC volume at the end, after the spinlock, so that we
>>> can avoid the ugly delay.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>> ---
>>> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
>>> index 714df906249e..e4229d01cf6a 100644
>>> --- a/sound/pci/rme96.c
>>> +++ b/sound/pci/rme96.c
>>> @@ -741,10 +741,11 @@ snd_rme96_playback_setrate(struct rme96 *rme96,
>>>    	{
>>>    		/* change to/from double-speed: reset the DAC (if available) */
>>>    		snd_rme96_reset_dac(rme96);
>>> +		return 1; /* need to restore volume */
>>>    	} else {
>>>    		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>>> +		return 0;
>>>    	}
>>> -	return 0;
>>>    }
>>>    
>>>    static int
>>> @@ -980,6 +981,7 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>    	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
>>>    	struct snd_pcm_runtime *runtime = substream->runtime;
>>>    	int err, rate, dummy;
>>> +	bool apply_dac_volume = false;
>>>    
>>>    	runtime->dma_area = (void __force *)(rme96->iobase +
>>>    					     RME96_IO_PLAY_BUFFER);
>>> @@ -993,24 +995,24 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>    	{
>>>                    /* slave clock */
>>>                    if ((int)params_rate(params) != rate) {
>>> -			spin_unlock_irq(&rme96->lock);
>>> -			return -EIO;
>>> -                }
>>> -	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
>>> -		spin_unlock_irq(&rme96->lock);
>>> -		return err;
>>> -	}
>>> -	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
>>> -		spin_unlock_irq(&rme96->lock);
>>> -		return err;
>>> +			err = -EIO;
>>> +			goto error;
>>> +		}
>>> +	} else {
>>> +		err = snd_rme96_playback_setrate(rme96, params_rate(params));
>>> +		if (err < 0)
>>> +			goto error;
>>> +		apply_dac_volume = err > 0; /* need to restore volume later? */
>>>    	}
>>> +	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
>>> +		goto error;
>>>    	snd_rme96_setframelog(rme96, params_channels(params), 1);
>>>    	if (rme96->capture_periodsize != 0) {
>>>    		if (params_period_size(params) << rme96->playback_frlog !=
>>>    		    rme96->capture_periodsize)
>>>    		{
>>> -			spin_unlock_irq(&rme96->lock);
>>> -			return -EBUSY;
>>> +			err = -EBUSY;
>>> +			goto error;
>>>    		}
>>>    	}
>>>    	rme96->playback_periodsize =
>>> @@ -1022,8 +1024,15 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>    		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>>>    	}
>>>    	spin_unlock_irq(&rme96->lock);
>>> +	err = 0;
>>>    		
>>> -	return 0;
>>> + error:
>>> +	if (apply_dac_volume) {
>>> +		usleep_range(3000, 10000);
>>> +		snd_rme96_apply_dac_volume(rme96);
>>> +	}
>>> +
>>> +	return err;
>>>    }
>>>    
>>>    static int
>>    CC [M]  sound/pci/rme96.o
>> sound/pci/rme96.c: Dans la fonction ‘snd_rme96_playback_hw_params’:
>> sound/pci/rme96.c:1008:3: erreur: label ‘error’ used but not defined
>>     goto error;
>>     ^
>> sound/pci/rme96.c:1009:2: attention : « return » manquant dans une fonction devant retourner une valeur [-Wreturn-type]
>>    }
>>    ^
>> sound/pci/rme96.c: Hors de toute fonction :
>> sound/pci/rme96.c:1010:46: erreur: expected ‘)’ before ‘(’ token
>>    snd_rme96_setframelog(rme96, params_channels(params), 1);
>>                                                ^
>> sound/pci/rme96.c:1011:2: erreur: expected identifier or ‘(’ before ‘if’
>>    if (rme96->capture_periodsize != 0) {
>>    ^
>> sound/pci/rme96.c:1019:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
>>    rme96->playback_periodsize =
>>         ^
>> sound/pci/rme96.c:1021:46: erreur: expected ‘)’ before ‘->’ token
>>    snd_rme96_set_period_properties(rme96, rme96->playback_periodsize);
>>                                                ^
>> sound/pci/rme96.c:1023:2: erreur: expected identifier or ‘(’ before ‘if’
>>    if ((rme96->wcreg & RME96_WCR_ADAT) == 0) {
>>    ^
>> sound/pci/rme96.c:1027:18: erreur: expected declaration specifiers or ‘...’ before ‘&’ token
>>    spin_unlock_irq(&rme96->lock);
>>                    ^
>> sound/pci/rme96.c:1028:2: attention : la définition de données n'a pas de type ni de classe de stockage
>>    err = 0;
>>    ^
>> sound/pci/rme96.c:1028:2: erreur: type defaults to ‘int’ in declaration of ‘err’ [-Werror=implicit-int]
>> sound/pci/rme96.c:1029:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘:’ token
>>    error:
>>         ^
>> sound/pci/rme96.c:1035:2: erreur: expected identifier or ‘(’ before ‘return’
>>    return err;
>>    ^
>> sound/pci/rme96.c:1036:1: erreur: expected identifier or ‘(’ before ‘}’ token
>>   }
Maeda Dec. 4, 2015, 2:24 p.m. UTC | #4
Last patch applied and all's right. What next ?

Thanks.

Le 04/12/2015 13:47, Takashi Iwai a écrit :
> On Fri, 04 Dec 2015 13:12:41 +0100,
> Maeda wrote:
>> I've problem with compiling rme96.c (see attachment).
>> Maybe I forgot something, i double checked, but my rme96.c seems to be good.
>>
>> Any clue ?
> You must have patched wrongly.
> You reverted the former patches, right?
>
>
> Takashi
>
>> Le 04/12/2015 12:19, Takashi Iwai a écrit :
>>> On Fri, 04 Dec 2015 11:44:02 +0100,
>>> Maeda wrote:
>>>> Hi !
>>>>
>>>> Well done ! It plays without max loud volume, and no errors on output.
>>>> I tried with the following test :
>>>>
>>>>       `aplay -fcd -d3 /dev/zero ;aplay -r96000 -c2 -d3 /dev/zero
>>>>       ;speaker-test  -c2 -twav`
>>>>
>>>> And it's OK. Then I tried with mplayer and some flac files (44.1 / 48
>>>> and 96 KHz sample) : no problem.
>>>>
>>>> I think you found the solution. If there are some future problem this
>>>> patch generates, I'll only see when using "it" each day. Then, for now,
>>>> you can commit it ;)
>>> Good to hear.  Could you try the revised patch below instead?
>>> It does restore DAC volume at the end, after the spinlock, so that we
>>> can avoid the ugly delay.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>> ---
>>> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
>>> index 714df906249e..e4229d01cf6a 100644
>>> --- a/sound/pci/rme96.c
>>> +++ b/sound/pci/rme96.c
>>> @@ -741,10 +741,11 @@ snd_rme96_playback_setrate(struct rme96 *rme96,
>>>    	{
>>>    		/* change to/from double-speed: reset the DAC (if available) */
>>>    		snd_rme96_reset_dac(rme96);
>>> +		return 1; /* need to restore volume */
>>>    	} else {
>>>    		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>>> +		return 0;
>>>    	}
>>> -	return 0;
>>>    }
>>>    
>>>    static int
>>> @@ -980,6 +981,7 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>    	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
>>>    	struct snd_pcm_runtime *runtime = substream->runtime;
>>>    	int err, rate, dummy;
>>> +	bool apply_dac_volume = false;
>>>    
>>>    	runtime->dma_area = (void __force *)(rme96->iobase +
>>>    					     RME96_IO_PLAY_BUFFER);
>>> @@ -993,24 +995,24 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>    	{
>>>                    /* slave clock */
>>>                    if ((int)params_rate(params) != rate) {
>>> -			spin_unlock_irq(&rme96->lock);
>>> -			return -EIO;
>>> -                }
>>> -	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
>>> -		spin_unlock_irq(&rme96->lock);
>>> -		return err;
>>> -	}
>>> -	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
>>> -		spin_unlock_irq(&rme96->lock);
>>> -		return err;
>>> +			err = -EIO;
>>> +			goto error;
>>> +		}
>>> +	} else {
>>> +		err = snd_rme96_playback_setrate(rme96, params_rate(params));
>>> +		if (err < 0)
>>> +			goto error;
>>> +		apply_dac_volume = err > 0; /* need to restore volume later? */
>>>    	}
>>> +	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
>>> +		goto error;
>>>    	snd_rme96_setframelog(rme96, params_channels(params), 1);
>>>    	if (rme96->capture_periodsize != 0) {
>>>    		if (params_period_size(params) << rme96->playback_frlog !=
>>>    		    rme96->capture_periodsize)
>>>    		{
>>> -			spin_unlock_irq(&rme96->lock);
>>> -			return -EBUSY;
>>> +			err = -EBUSY;
>>> +			goto error;
>>>    		}
>>>    	}
>>>    	rme96->playback_periodsize =
>>> @@ -1022,8 +1024,15 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>    		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>>>    	}
>>>    	spin_unlock_irq(&rme96->lock);
>>> +	err = 0;
>>>    		
>>> -	return 0;
>>> + error:
>>> +	if (apply_dac_volume) {
>>> +		usleep_range(3000, 10000);
>>> +		snd_rme96_apply_dac_volume(rme96);
>>> +	}
>>> +
>>> +	return err;
>>>    }
>>>    
>>>    static int
>>    CC [M]  sound/pci/rme96.o
>> sound/pci/rme96.c: Dans la fonction ‘snd_rme96_playback_hw_params’:
>> sound/pci/rme96.c:1008:3: erreur: label ‘error’ used but not defined
>>     goto error;
>>     ^
>> sound/pci/rme96.c:1009:2: attention : « return » manquant dans une fonction devant retourner une valeur [-Wreturn-type]
>>    }
>>    ^
>> sound/pci/rme96.c: Hors de toute fonction :
>> sound/pci/rme96.c:1010:46: erreur: expected ‘)’ before ‘(’ token
>>    snd_rme96_setframelog(rme96, params_channels(params), 1);
>>                                                ^
>> sound/pci/rme96.c:1011:2: erreur: expected identifier or ‘(’ before ‘if’
>>    if (rme96->capture_periodsize != 0) {
>>    ^
>> sound/pci/rme96.c:1019:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
>>    rme96->playback_periodsize =
>>         ^
>> sound/pci/rme96.c:1021:46: erreur: expected ‘)’ before ‘->’ token
>>    snd_rme96_set_period_properties(rme96, rme96->playback_periodsize);
>>                                                ^
>> sound/pci/rme96.c:1023:2: erreur: expected identifier or ‘(’ before ‘if’
>>    if ((rme96->wcreg & RME96_WCR_ADAT) == 0) {
>>    ^
>> sound/pci/rme96.c:1027:18: erreur: expected declaration specifiers or ‘...’ before ‘&’ token
>>    spin_unlock_irq(&rme96->lock);
>>                    ^
>> sound/pci/rme96.c:1028:2: attention : la définition de données n'a pas de type ni de classe de stockage
>>    err = 0;
>>    ^
>> sound/pci/rme96.c:1028:2: erreur: type defaults to ‘int’ in declaration of ‘err’ [-Werror=implicit-int]
>> sound/pci/rme96.c:1029:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘:’ token
>>    error:
>>         ^
>> sound/pci/rme96.c:1035:2: erreur: expected identifier or ‘(’ before ‘return’
>>    return err;
>>    ^
>> sound/pci/rme96.c:1036:1: erreur: expected identifier or ‘(’ before ‘}’ token
>>   }
Takashi Iwai Dec. 4, 2015, 2:36 p.m. UTC | #5
On Fri, 04 Dec 2015 15:24:44 +0100,
Maeda wrote:
> 
> Last patch applied and all's right. What next ?

At best, give your proper tested-by tag.  It's like

Tested-by: Your Real Name <mail@address>

It's included in the git commit log as your contribution.
Then I'll merge the fix into the upstream.


thanks,

Takashi

> 
> Thanks.
> 
> Le 04/12/2015 13:47, Takashi Iwai a écrit :
> > On Fri, 04 Dec 2015 13:12:41 +0100,
> > Maeda wrote:
> >> I've problem with compiling rme96.c (see attachment).
> >> Maybe I forgot something, i double checked, but my rme96.c seems to be good.
> >>
> >> Any clue ?
> > You must have patched wrongly.
> > You reverted the former patches, right?
> >
> >
> > Takashi
> >
> >> Le 04/12/2015 12:19, Takashi Iwai a écrit :
> >>> On Fri, 04 Dec 2015 11:44:02 +0100,
> >>> Maeda wrote:
> >>>> Hi !
> >>>>
> >>>> Well done ! It plays without max loud volume, and no errors on output.
> >>>> I tried with the following test :
> >>>>
> >>>>       `aplay -fcd -d3 /dev/zero ;aplay -r96000 -c2 -d3 /dev/zero
> >>>>       ;speaker-test  -c2 -twav`
> >>>>
> >>>> And it's OK. Then I tried with mplayer and some flac files (44.1 / 48
> >>>> and 96 KHz sample) : no problem.
> >>>>
> >>>> I think you found the solution. If there are some future problem this
> >>>> patch generates, I'll only see when using "it" each day. Then, for now,
> >>>> you can commit it ;)
> >>> Good to hear.  Could you try the revised patch below instead?
> >>> It does restore DAC volume at the end, after the spinlock, so that we
> >>> can avoid the ugly delay.
> >>>
> >>>
> >>> thanks,
> >>>
> >>> Takashi
> >>>
> >>> ---
> >>> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
> >>> index 714df906249e..e4229d01cf6a 100644
> >>> --- a/sound/pci/rme96.c
> >>> +++ b/sound/pci/rme96.c
> >>> @@ -741,10 +741,11 @@ snd_rme96_playback_setrate(struct rme96 *rme96,
> >>>    	{
> >>>    		/* change to/from double-speed: reset the DAC (if available) */
> >>>    		snd_rme96_reset_dac(rme96);
> >>> +		return 1; /* need to restore volume */
> >>>    	} else {
> >>>    		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> >>> +		return 0;
> >>>    	}
> >>> -	return 0;
> >>>    }
> >>>    
> >>>    static int
> >>> @@ -980,6 +981,7 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
> >>>    	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
> >>>    	struct snd_pcm_runtime *runtime = substream->runtime;
> >>>    	int err, rate, dummy;
> >>> +	bool apply_dac_volume = false;
> >>>    
> >>>    	runtime->dma_area = (void __force *)(rme96->iobase +
> >>>    					     RME96_IO_PLAY_BUFFER);
> >>> @@ -993,24 +995,24 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
> >>>    	{
> >>>                    /* slave clock */
> >>>                    if ((int)params_rate(params) != rate) {
> >>> -			spin_unlock_irq(&rme96->lock);
> >>> -			return -EIO;
> >>> -                }
> >>> -	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
> >>> -		spin_unlock_irq(&rme96->lock);
> >>> -		return err;
> >>> -	}
> >>> -	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
> >>> -		spin_unlock_irq(&rme96->lock);
> >>> -		return err;
> >>> +			err = -EIO;
> >>> +			goto error;
> >>> +		}
> >>> +	} else {
> >>> +		err = snd_rme96_playback_setrate(rme96, params_rate(params));
> >>> +		if (err < 0)
> >>> +			goto error;
> >>> +		apply_dac_volume = err > 0; /* need to restore volume later? */
> >>>    	}
> >>> +	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
> >>> +		goto error;
> >>>    	snd_rme96_setframelog(rme96, params_channels(params), 1);
> >>>    	if (rme96->capture_periodsize != 0) {
> >>>    		if (params_period_size(params) << rme96->playback_frlog !=
> >>>    		    rme96->capture_periodsize)
> >>>    		{
> >>> -			spin_unlock_irq(&rme96->lock);
> >>> -			return -EBUSY;
> >>> +			err = -EBUSY;
> >>> +			goto error;
> >>>    		}
> >>>    	}
> >>>    	rme96->playback_periodsize =
> >>> @@ -1022,8 +1024,15 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
> >>>    		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> >>>    	}
> >>>    	spin_unlock_irq(&rme96->lock);
> >>> +	err = 0;
> >>>    		
> >>> -	return 0;
> >>> + error:
> >>> +	if (apply_dac_volume) {
> >>> +		usleep_range(3000, 10000);
> >>> +		snd_rme96_apply_dac_volume(rme96);
> >>> +	}
> >>> +
> >>> +	return err;
> >>>    }
> >>>    
> >>>    static int
> >>    CC [M]  sound/pci/rme96.o
> >> sound/pci/rme96.c: Dans la fonction ‘snd_rme96_playback_hw_params’:
> >> sound/pci/rme96.c:1008:3: erreur: label ‘error’ used but not defined
> >>     goto error;
> >>     ^
> >> sound/pci/rme96.c:1009:2: attention : « return » manquant dans une fonction devant retourner une valeur [-Wreturn-type]
> >>    }
> >>    ^
> >> sound/pci/rme96.c: Hors de toute fonction :
> >> sound/pci/rme96.c:1010:46: erreur: expected ‘)’ before ‘(’ token
> >>    snd_rme96_setframelog(rme96, params_channels(params), 1);
> >>                                                ^
> >> sound/pci/rme96.c:1011:2: erreur: expected identifier or ‘(’ before ‘if’
> >>    if (rme96->capture_periodsize != 0) {
> >>    ^
> >> sound/pci/rme96.c:1019:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
> >>    rme96->playback_periodsize =
> >>         ^
> >> sound/pci/rme96.c:1021:46: erreur: expected ‘)’ before ‘->’ token
> >>    snd_rme96_set_period_properties(rme96, rme96->playback_periodsize);
> >>                                                ^
> >> sound/pci/rme96.c:1023:2: erreur: expected identifier or ‘(’ before ‘if’
> >>    if ((rme96->wcreg & RME96_WCR_ADAT) == 0) {
> >>    ^
> >> sound/pci/rme96.c:1027:18: erreur: expected declaration specifiers or ‘...’ before ‘&’ token
> >>    spin_unlock_irq(&rme96->lock);
> >>                    ^
> >> sound/pci/rme96.c:1028:2: attention : la définition de données n'a pas de type ni de classe de stockage
> >>    err = 0;
> >>    ^
> >> sound/pci/rme96.c:1028:2: erreur: type defaults to ‘int’ in declaration of ‘err’ [-Werror=implicit-int]
> >> sound/pci/rme96.c:1029:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘:’ token
> >>    error:
> >>         ^
> >> sound/pci/rme96.c:1035:2: erreur: expected identifier or ‘(’ before ‘return’
> >>    return err;
> >>    ^
> >> sound/pci/rme96.c:1036:1: erreur: expected identifier or ‘(’ before ‘}’ token
> >>   }
>
Maeda Dec. 4, 2015, 3:01 p.m. UTC | #6
I'm not use to do that. Do you mean adding a tags in the bug I opened 
(https://bugzilla.kernel.org/show_bug.cgi?id=105771) ?

Le 04/12/2015 15:36, Takashi Iwai a écrit :
> On Fri, 04 Dec 2015 15:24:44 +0100,
> Maeda wrote:
>> Last patch applied and all's right. What next ?
> At best, give your proper tested-by tag.  It's like
>
> Tested-by: Your Real Name <mail@address>
>
> It's included in the git commit log as your contribution.
> Then I'll merge the fix into the upstream.
>
>
> thanks,
>
> Takashi
>
>> Thanks.
>>
>> Le 04/12/2015 13:47, Takashi Iwai a écrit :
>>> On Fri, 04 Dec 2015 13:12:41 +0100,
>>> Maeda wrote:
>>>> I've problem with compiling rme96.c (see attachment).
>>>> Maybe I forgot something, i double checked, but my rme96.c seems to be good.
>>>>
>>>> Any clue ?
>>> You must have patched wrongly.
>>> You reverted the former patches, right?
>>>
>>>
>>> Takashi
>>>
>>>> Le 04/12/2015 12:19, Takashi Iwai a écrit :
>>>>> On Fri, 04 Dec 2015 11:44:02 +0100,
>>>>> Maeda wrote:
>>>>>> Hi !
>>>>>>
>>>>>> Well done ! It plays without max loud volume, and no errors on output.
>>>>>> I tried with the following test :
>>>>>>
>>>>>>        `aplay -fcd -d3 /dev/zero ;aplay -r96000 -c2 -d3 /dev/zero
>>>>>>        ;speaker-test  -c2 -twav`
>>>>>>
>>>>>> And it's OK. Then I tried with mplayer and some flac files (44.1 / 48
>>>>>> and 96 KHz sample) : no problem.
>>>>>>
>>>>>> I think you found the solution. If there are some future problem this
>>>>>> patch generates, I'll only see when using "it" each day. Then, for now,
>>>>>> you can commit it ;)
>>>>> Good to hear.  Could you try the revised patch below instead?
>>>>> It does restore DAC volume at the end, after the spinlock, so that we
>>>>> can avoid the ugly delay.
>>>>>
>>>>>
>>>>> thanks,
>>>>>
>>>>> Takashi
>>>>>
>>>>> ---
>>>>> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
>>>>> index 714df906249e..e4229d01cf6a 100644
>>>>> --- a/sound/pci/rme96.c
>>>>> +++ b/sound/pci/rme96.c
>>>>> @@ -741,10 +741,11 @@ snd_rme96_playback_setrate(struct rme96 *rme96,
>>>>>     	{
>>>>>     		/* change to/from double-speed: reset the DAC (if available) */
>>>>>     		snd_rme96_reset_dac(rme96);
>>>>> +		return 1; /* need to restore volume */
>>>>>     	} else {
>>>>>     		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>>>>> +		return 0;
>>>>>     	}
>>>>> -	return 0;
>>>>>     }
>>>>>     
>>>>>     static int
>>>>> @@ -980,6 +981,7 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>>>     	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
>>>>>     	struct snd_pcm_runtime *runtime = substream->runtime;
>>>>>     	int err, rate, dummy;
>>>>> +	bool apply_dac_volume = false;
>>>>>     
>>>>>     	runtime->dma_area = (void __force *)(rme96->iobase +
>>>>>     					     RME96_IO_PLAY_BUFFER);
>>>>> @@ -993,24 +995,24 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>>>     	{
>>>>>                     /* slave clock */
>>>>>                     if ((int)params_rate(params) != rate) {
>>>>> -			spin_unlock_irq(&rme96->lock);
>>>>> -			return -EIO;
>>>>> -                }
>>>>> -	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
>>>>> -		spin_unlock_irq(&rme96->lock);
>>>>> -		return err;
>>>>> -	}
>>>>> -	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
>>>>> -		spin_unlock_irq(&rme96->lock);
>>>>> -		return err;
>>>>> +			err = -EIO;
>>>>> +			goto error;
>>>>> +		}
>>>>> +	} else {
>>>>> +		err = snd_rme96_playback_setrate(rme96, params_rate(params));
>>>>> +		if (err < 0)
>>>>> +			goto error;
>>>>> +		apply_dac_volume = err > 0; /* need to restore volume later? */
>>>>>     	}
>>>>> +	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
>>>>> +		goto error;
>>>>>     	snd_rme96_setframelog(rme96, params_channels(params), 1);
>>>>>     	if (rme96->capture_periodsize != 0) {
>>>>>     		if (params_period_size(params) << rme96->playback_frlog !=
>>>>>     		    rme96->capture_periodsize)
>>>>>     		{
>>>>> -			spin_unlock_irq(&rme96->lock);
>>>>> -			return -EBUSY;
>>>>> +			err = -EBUSY;
>>>>> +			goto error;
>>>>>     		}
>>>>>     	}
>>>>>     	rme96->playback_periodsize =
>>>>> @@ -1022,8 +1024,15 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>>>     		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>>>>>     	}
>>>>>     	spin_unlock_irq(&rme96->lock);
>>>>> +	err = 0;
>>>>>     		
>>>>> -	return 0;
>>>>> + error:
>>>>> +	if (apply_dac_volume) {
>>>>> +		usleep_range(3000, 10000);
>>>>> +		snd_rme96_apply_dac_volume(rme96);
>>>>> +	}
>>>>> +
>>>>> +	return err;
>>>>>     }
>>>>>     
>>>>>     static int
>>>>     CC [M]  sound/pci/rme96.o
>>>> sound/pci/rme96.c: Dans la fonction ‘snd_rme96_playback_hw_params’:
>>>> sound/pci/rme96.c:1008:3: erreur: label ‘error’ used but not defined
>>>>      goto error;
>>>>      ^
>>>> sound/pci/rme96.c:1009:2: attention : « return » manquant dans une fonction devant retourner une valeur [-Wreturn-type]
>>>>     }
>>>>     ^
>>>> sound/pci/rme96.c: Hors de toute fonction :
>>>> sound/pci/rme96.c:1010:46: erreur: expected ‘)’ before ‘(’ token
>>>>     snd_rme96_setframelog(rme96, params_channels(params), 1);
>>>>                                                 ^
>>>> sound/pci/rme96.c:1011:2: erreur: expected identifier or ‘(’ before ‘if’
>>>>     if (rme96->capture_periodsize != 0) {
>>>>     ^
>>>> sound/pci/rme96.c:1019:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
>>>>     rme96->playback_periodsize =
>>>>          ^
>>>> sound/pci/rme96.c:1021:46: erreur: expected ‘)’ before ‘->’ token
>>>>     snd_rme96_set_period_properties(rme96, rme96->playback_periodsize);
>>>>                                                 ^
>>>> sound/pci/rme96.c:1023:2: erreur: expected identifier or ‘(’ before ‘if’
>>>>     if ((rme96->wcreg & RME96_WCR_ADAT) == 0) {
>>>>     ^
>>>> sound/pci/rme96.c:1027:18: erreur: expected declaration specifiers or ‘...’ before ‘&’ token
>>>>     spin_unlock_irq(&rme96->lock);
>>>>                     ^
>>>> sound/pci/rme96.c:1028:2: attention : la définition de données n'a pas de type ni de classe de stockage
>>>>     err = 0;
>>>>     ^
>>>> sound/pci/rme96.c:1028:2: erreur: type defaults to ‘int’ in declaration of ‘err’ [-Werror=implicit-int]
>>>> sound/pci/rme96.c:1029:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘:’ token
>>>>     error:
>>>>          ^
>>>> sound/pci/rme96.c:1035:2: erreur: expected identifier or ‘(’ before ‘return’
>>>>     return err;
>>>>     ^
>>>> sound/pci/rme96.c:1036:1: erreur: expected identifier or ‘(’ before ‘}’ token
>>>>    }
Takashi Iwai Dec. 4, 2015, 3:08 p.m. UTC | #7
On Fri, 04 Dec 2015 16:01:59 +0100,
Maeda wrote:
> 
> I'm not use to do that. Do you mean adding a tags in the bug I opened 
> (https://bugzilla.kernel.org/show_bug.cgi?id=105771) ?

Much simpler.  Just drop me a mail with the line including your
Tested-by tag text.


Takashi

> 
> Le 04/12/2015 15:36, Takashi Iwai a écrit :
> > On Fri, 04 Dec 2015 15:24:44 +0100,
> > Maeda wrote:
> >> Last patch applied and all's right. What next ?
> > At best, give your proper tested-by tag.  It's like
> >
> > Tested-by: Your Real Name <mail@address>
> >
> > It's included in the git commit log as your contribution.
> > Then I'll merge the fix into the upstream.
> >
> >
> > thanks,
> >
> > Takashi
> >
> >> Thanks.
> >>
> >> Le 04/12/2015 13:47, Takashi Iwai a écrit :
> >>> On Fri, 04 Dec 2015 13:12:41 +0100,
> >>> Maeda wrote:
> >>>> I've problem with compiling rme96.c (see attachment).
> >>>> Maybe I forgot something, i double checked, but my rme96.c seems to be good.
> >>>>
> >>>> Any clue ?
> >>> You must have patched wrongly.
> >>> You reverted the former patches, right?
> >>>
> >>>
> >>> Takashi
> >>>
> >>>> Le 04/12/2015 12:19, Takashi Iwai a écrit :
> >>>>> On Fri, 04 Dec 2015 11:44:02 +0100,
> >>>>> Maeda wrote:
> >>>>>> Hi !
> >>>>>>
> >>>>>> Well done ! It plays without max loud volume, and no errors on output.
> >>>>>> I tried with the following test :
> >>>>>>
> >>>>>>        `aplay -fcd -d3 /dev/zero ;aplay -r96000 -c2 -d3 /dev/zero
> >>>>>>        ;speaker-test  -c2 -twav`
> >>>>>>
> >>>>>> And it's OK. Then I tried with mplayer and some flac files (44.1 / 48
> >>>>>> and 96 KHz sample) : no problem.
> >>>>>>
> >>>>>> I think you found the solution. If there are some future problem this
> >>>>>> patch generates, I'll only see when using "it" each day. Then, for now,
> >>>>>> you can commit it ;)
> >>>>> Good to hear.  Could you try the revised patch below instead?
> >>>>> It does restore DAC volume at the end, after the spinlock, so that we
> >>>>> can avoid the ugly delay.
> >>>>>
> >>>>>
> >>>>> thanks,
> >>>>>
> >>>>> Takashi
> >>>>>
> >>>>> ---
> >>>>> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
> >>>>> index 714df906249e..e4229d01cf6a 100644
> >>>>> --- a/sound/pci/rme96.c
> >>>>> +++ b/sound/pci/rme96.c
> >>>>> @@ -741,10 +741,11 @@ snd_rme96_playback_setrate(struct rme96 *rme96,
> >>>>>     	{
> >>>>>     		/* change to/from double-speed: reset the DAC (if available) */
> >>>>>     		snd_rme96_reset_dac(rme96);
> >>>>> +		return 1; /* need to restore volume */
> >>>>>     	} else {
> >>>>>     		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> >>>>> +		return 0;
> >>>>>     	}
> >>>>> -	return 0;
> >>>>>     }
> >>>>>     
> >>>>>     static int
> >>>>> @@ -980,6 +981,7 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
> >>>>>     	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
> >>>>>     	struct snd_pcm_runtime *runtime = substream->runtime;
> >>>>>     	int err, rate, dummy;
> >>>>> +	bool apply_dac_volume = false;
> >>>>>     
> >>>>>     	runtime->dma_area = (void __force *)(rme96->iobase +
> >>>>>     					     RME96_IO_PLAY_BUFFER);
> >>>>> @@ -993,24 +995,24 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
> >>>>>     	{
> >>>>>                     /* slave clock */
> >>>>>                     if ((int)params_rate(params) != rate) {
> >>>>> -			spin_unlock_irq(&rme96->lock);
> >>>>> -			return -EIO;
> >>>>> -                }
> >>>>> -	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
> >>>>> -		spin_unlock_irq(&rme96->lock);
> >>>>> -		return err;
> >>>>> -	}
> >>>>> -	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
> >>>>> -		spin_unlock_irq(&rme96->lock);
> >>>>> -		return err;
> >>>>> +			err = -EIO;
> >>>>> +			goto error;
> >>>>> +		}
> >>>>> +	} else {
> >>>>> +		err = snd_rme96_playback_setrate(rme96, params_rate(params));
> >>>>> +		if (err < 0)
> >>>>> +			goto error;
> >>>>> +		apply_dac_volume = err > 0; /* need to restore volume later? */
> >>>>>     	}
> >>>>> +	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
> >>>>> +		goto error;
> >>>>>     	snd_rme96_setframelog(rme96, params_channels(params), 1);
> >>>>>     	if (rme96->capture_periodsize != 0) {
> >>>>>     		if (params_period_size(params) << rme96->playback_frlog !=
> >>>>>     		    rme96->capture_periodsize)
> >>>>>     		{
> >>>>> -			spin_unlock_irq(&rme96->lock);
> >>>>> -			return -EBUSY;
> >>>>> +			err = -EBUSY;
> >>>>> +			goto error;
> >>>>>     		}
> >>>>>     	}
> >>>>>     	rme96->playback_periodsize =
> >>>>> @@ -1022,8 +1024,15 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
> >>>>>     		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> >>>>>     	}
> >>>>>     	spin_unlock_irq(&rme96->lock);
> >>>>> +	err = 0;
> >>>>>     		
> >>>>> -	return 0;
> >>>>> + error:
> >>>>> +	if (apply_dac_volume) {
> >>>>> +		usleep_range(3000, 10000);
> >>>>> +		snd_rme96_apply_dac_volume(rme96);
> >>>>> +	}
> >>>>> +
> >>>>> +	return err;
> >>>>>     }
> >>>>>     
> >>>>>     static int
> >>>>     CC [M]  sound/pci/rme96.o
> >>>> sound/pci/rme96.c: Dans la fonction ‘snd_rme96_playback_hw_params’:
> >>>> sound/pci/rme96.c:1008:3: erreur: label ‘error’ used but not defined
> >>>>      goto error;
> >>>>      ^
> >>>> sound/pci/rme96.c:1009:2: attention : « return » manquant dans une fonction devant retourner une valeur [-Wreturn-type]
> >>>>     }
> >>>>     ^
> >>>> sound/pci/rme96.c: Hors de toute fonction :
> >>>> sound/pci/rme96.c:1010:46: erreur: expected ‘)’ before ‘(’ token
> >>>>     snd_rme96_setframelog(rme96, params_channels(params), 1);
> >>>>                                                 ^
> >>>> sound/pci/rme96.c:1011:2: erreur: expected identifier or ‘(’ before ‘if’
> >>>>     if (rme96->capture_periodsize != 0) {
> >>>>     ^
> >>>> sound/pci/rme96.c:1019:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
> >>>>     rme96->playback_periodsize =
> >>>>          ^
> >>>> sound/pci/rme96.c:1021:46: erreur: expected ‘)’ before ‘->’ token
> >>>>     snd_rme96_set_period_properties(rme96, rme96->playback_periodsize);
> >>>>                                                 ^
> >>>> sound/pci/rme96.c:1023:2: erreur: expected identifier or ‘(’ before ‘if’
> >>>>     if ((rme96->wcreg & RME96_WCR_ADAT) == 0) {
> >>>>     ^
> >>>> sound/pci/rme96.c:1027:18: erreur: expected declaration specifiers or ‘...’ before ‘&’ token
> >>>>     spin_unlock_irq(&rme96->lock);
> >>>>                     ^
> >>>> sound/pci/rme96.c:1028:2: attention : la définition de données n'a pas de type ni de classe de stockage
> >>>>     err = 0;
> >>>>     ^
> >>>> sound/pci/rme96.c:1028:2: erreur: type defaults to ‘int’ in declaration of ‘err’ [-Werror=implicit-int]
> >>>> sound/pci/rme96.c:1029:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘:’ token
> >>>>     error:
> >>>>          ^
> >>>> sound/pci/rme96.c:1035:2: erreur: expected identifier or ‘(’ before ‘return’
> >>>>     return err;
> >>>>     ^
> >>>> sound/pci/rme96.c:1036:1: erreur: expected identifier or ‘(’ before ‘}’ token
> >>>>    }
>
Maeda Dec. 4, 2015, 3:14 p.m. UTC | #8
OK :)

Tested-by: Sylvain LABOISNE <maeda1@free.fr>


Le 04/12/2015 16:08, Takashi Iwai a écrit :
> On Fri, 04 Dec 2015 16:01:59 +0100,
> Maeda wrote:
>> I'm not use to do that. Do you mean adding a tags in the bug I opened
>> (https://bugzilla.kernel.org/show_bug.cgi?id=105771) ?
> Much simpler.  Just drop me a mail with the line including your
> Tested-by tag text.
>
>
> Takashi
>
>> Le 04/12/2015 15:36, Takashi Iwai a écrit :
>>> On Fri, 04 Dec 2015 15:24:44 +0100,
>>> Maeda wrote:
>>>> Last patch applied and all's right. What next ?
>>> At best, give your proper tested-by tag.  It's like
>>>
>>> Tested-by: Your Real Name <mail@address>
>>>
>>> It's included in the git commit log as your contribution.
>>> Then I'll merge the fix into the upstream.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>>> Thanks.
>>>>
>>>> Le 04/12/2015 13:47, Takashi Iwai a écrit :
>>>>> On Fri, 04 Dec 2015 13:12:41 +0100,
>>>>> Maeda wrote:
>>>>>> I've problem with compiling rme96.c (see attachment).
>>>>>> Maybe I forgot something, i double checked, but my rme96.c seems to be good.
>>>>>>
>>>>>> Any clue ?
>>>>> You must have patched wrongly.
>>>>> You reverted the former patches, right?
>>>>>
>>>>>
>>>>> Takashi
>>>>>
>>>>>> Le 04/12/2015 12:19, Takashi Iwai a écrit :
>>>>>>> On Fri, 04 Dec 2015 11:44:02 +0100,
>>>>>>> Maeda wrote:
>>>>>>>> Hi !
>>>>>>>>
>>>>>>>> Well done ! It plays without max loud volume, and no errors on output.
>>>>>>>> I tried with the following test :
>>>>>>>>
>>>>>>>>         `aplay -fcd -d3 /dev/zero ;aplay -r96000 -c2 -d3 /dev/zero
>>>>>>>>         ;speaker-test  -c2 -twav`
>>>>>>>>
>>>>>>>> And it's OK. Then I tried with mplayer and some flac files (44.1 / 48
>>>>>>>> and 96 KHz sample) : no problem.
>>>>>>>>
>>>>>>>> I think you found the solution. If there are some future problem this
>>>>>>>> patch generates, I'll only see when using "it" each day. Then, for now,
>>>>>>>> you can commit it ;)
>>>>>>> Good to hear.  Could you try the revised patch below instead?
>>>>>>> It does restore DAC volume at the end, after the spinlock, so that we
>>>>>>> can avoid the ugly delay.
>>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Takashi
>>>>>>>
>>>>>>> ---
>>>>>>> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
>>>>>>> index 714df906249e..e4229d01cf6a 100644
>>>>>>> --- a/sound/pci/rme96.c
>>>>>>> +++ b/sound/pci/rme96.c
>>>>>>> @@ -741,10 +741,11 @@ snd_rme96_playback_setrate(struct rme96 *rme96,
>>>>>>>      	{
>>>>>>>      		/* change to/from double-speed: reset the DAC (if available) */
>>>>>>>      		snd_rme96_reset_dac(rme96);
>>>>>>> +		return 1; /* need to restore volume */
>>>>>>>      	} else {
>>>>>>>      		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>>>>>>> +		return 0;
>>>>>>>      	}
>>>>>>> -	return 0;
>>>>>>>      }
>>>>>>>      
>>>>>>>      static int
>>>>>>> @@ -980,6 +981,7 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>>>>>      	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
>>>>>>>      	struct snd_pcm_runtime *runtime = substream->runtime;
>>>>>>>      	int err, rate, dummy;
>>>>>>> +	bool apply_dac_volume = false;
>>>>>>>      
>>>>>>>      	runtime->dma_area = (void __force *)(rme96->iobase +
>>>>>>>      					     RME96_IO_PLAY_BUFFER);
>>>>>>> @@ -993,24 +995,24 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>>>>>      	{
>>>>>>>                      /* slave clock */
>>>>>>>                      if ((int)params_rate(params) != rate) {
>>>>>>> -			spin_unlock_irq(&rme96->lock);
>>>>>>> -			return -EIO;
>>>>>>> -                }
>>>>>>> -	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
>>>>>>> -		spin_unlock_irq(&rme96->lock);
>>>>>>> -		return err;
>>>>>>> -	}
>>>>>>> -	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
>>>>>>> -		spin_unlock_irq(&rme96->lock);
>>>>>>> -		return err;
>>>>>>> +			err = -EIO;
>>>>>>> +			goto error;
>>>>>>> +		}
>>>>>>> +	} else {
>>>>>>> +		err = snd_rme96_playback_setrate(rme96, params_rate(params));
>>>>>>> +		if (err < 0)
>>>>>>> +			goto error;
>>>>>>> +		apply_dac_volume = err > 0; /* need to restore volume later? */
>>>>>>>      	}
>>>>>>> +	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
>>>>>>> +		goto error;
>>>>>>>      	snd_rme96_setframelog(rme96, params_channels(params), 1);
>>>>>>>      	if (rme96->capture_periodsize != 0) {
>>>>>>>      		if (params_period_size(params) << rme96->playback_frlog !=
>>>>>>>      		    rme96->capture_periodsize)
>>>>>>>      		{
>>>>>>> -			spin_unlock_irq(&rme96->lock);
>>>>>>> -			return -EBUSY;
>>>>>>> +			err = -EBUSY;
>>>>>>> +			goto error;
>>>>>>>      		}
>>>>>>>      	}
>>>>>>>      	rme96->playback_periodsize =
>>>>>>> @@ -1022,8 +1024,15 @@ snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
>>>>>>>      		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>>>>>>>      	}
>>>>>>>      	spin_unlock_irq(&rme96->lock);
>>>>>>> +	err = 0;
>>>>>>>      		
>>>>>>> -	return 0;
>>>>>>> + error:
>>>>>>> +	if (apply_dac_volume) {
>>>>>>> +		usleep_range(3000, 10000);
>>>>>>> +		snd_rme96_apply_dac_volume(rme96);
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return err;
>>>>>>>      }
>>>>>>>      
>>>>>>>      static int
>>>>>>      CC [M]  sound/pci/rme96.o
>>>>>> sound/pci/rme96.c: Dans la fonction ‘snd_rme96_playback_hw_params’:
>>>>>> sound/pci/rme96.c:1008:3: erreur: label ‘error’ used but not defined
>>>>>>       goto error;
>>>>>>       ^
>>>>>> sound/pci/rme96.c:1009:2: attention : « return » manquant dans une fonction devant retourner une valeur [-Wreturn-type]
>>>>>>      }
>>>>>>      ^
>>>>>> sound/pci/rme96.c: Hors de toute fonction :
>>>>>> sound/pci/rme96.c:1010:46: erreur: expected ‘)’ before ‘(’ token
>>>>>>      snd_rme96_setframelog(rme96, params_channels(params), 1);
>>>>>>                                                  ^
>>>>>> sound/pci/rme96.c:1011:2: erreur: expected identifier or ‘(’ before ‘if’
>>>>>>      if (rme96->capture_periodsize != 0) {
>>>>>>      ^
>>>>>> sound/pci/rme96.c:1019:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘->’ token
>>>>>>      rme96->playback_periodsize =
>>>>>>           ^
>>>>>> sound/pci/rme96.c:1021:46: erreur: expected ‘)’ before ‘->’ token
>>>>>>      snd_rme96_set_period_properties(rme96, rme96->playback_periodsize);
>>>>>>                                                  ^
>>>>>> sound/pci/rme96.c:1023:2: erreur: expected identifier or ‘(’ before ‘if’
>>>>>>      if ((rme96->wcreg & RME96_WCR_ADAT) == 0) {
>>>>>>      ^
>>>>>> sound/pci/rme96.c:1027:18: erreur: expected declaration specifiers or ‘...’ before ‘&’ token
>>>>>>      spin_unlock_irq(&rme96->lock);
>>>>>>                      ^
>>>>>> sound/pci/rme96.c:1028:2: attention : la définition de données n'a pas de type ni de classe de stockage
>>>>>>      err = 0;
>>>>>>      ^
>>>>>> sound/pci/rme96.c:1028:2: erreur: type defaults to ‘int’ in declaration of ‘err’ [-Werror=implicit-int]
>>>>>> sound/pci/rme96.c:1029:7: erreur: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘:’ token
>>>>>>      error:
>>>>>>           ^
>>>>>> sound/pci/rme96.c:1035:2: erreur: expected identifier or ‘(’ before ‘return’
>>>>>>      return err;
>>>>>>      ^
>>>>>> sound/pci/rme96.c:1036:1: erreur: expected identifier or ‘(’ before ‘}’ token
>>>>>>     }
diff mbox

Patch

diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
index 714df906249e..e4229d01cf6a 100644
--- a/sound/pci/rme96.c
+++ b/sound/pci/rme96.c
@@ -741,10 +741,11 @@  snd_rme96_playback_setrate(struct rme96 *rme96,
 	{
 		/* change to/from double-speed: reset the DAC (if available) */
 		snd_rme96_reset_dac(rme96);
+		return 1; /* need to restore volume */
 	} else {
 		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
+		return 0;
 	}
-	return 0;
 }
 
 static int
@@ -980,6 +981,7 @@  snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	int err, rate, dummy;
+	bool apply_dac_volume = false;
 
 	runtime->dma_area = (void __force *)(rme96->iobase +
 					     RME96_IO_PLAY_BUFFER);
@@ -993,24 +995,24 @@  snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
 	{
                 /* slave clock */
                 if ((int)params_rate(params) != rate) {
-			spin_unlock_irq(&rme96->lock);
-			return -EIO;                    
-                }
-	} else if ((err = snd_rme96_playback_setrate(rme96, params_rate(params))) < 0) {
-		spin_unlock_irq(&rme96->lock);
-		return err;
-	}
-	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0) {
-		spin_unlock_irq(&rme96->lock);
-		return err;
+			err = -EIO;
+			goto error;
+		}
+	} else {
+		err = snd_rme96_playback_setrate(rme96, params_rate(params));
+		if (err < 0)
+			goto error;
+		apply_dac_volume = err > 0; /* need to restore volume later? */
 	}
+	if ((err = snd_rme96_playback_setformat(rme96, params_format(params))) < 0)
+		goto error;
 	snd_rme96_setframelog(rme96, params_channels(params), 1);
 	if (rme96->capture_periodsize != 0) {
 		if (params_period_size(params) << rme96->playback_frlog !=
 		    rme96->capture_periodsize)
 		{
-			spin_unlock_irq(&rme96->lock);
-			return -EBUSY;
+			err = -EBUSY;
+			goto error;
 		}
 	}
 	rme96->playback_periodsize =
@@ -1022,8 +1024,15 @@  snd_rme96_playback_hw_params(struct snd_pcm_substream *substream,
 		writel(rme96->wcreg |= rme96->wcreg_spdif_stream, rme96->iobase + RME96_IO_CONTROL_REGISTER);
 	}
 	spin_unlock_irq(&rme96->lock);
+	err = 0;
 		
-	return 0;
+ error:
+	if (apply_dac_volume) {
+		usleep_range(3000, 10000);
+		snd_rme96_apply_dac_volume(rme96);
+	}
+
+	return err;
 }
 
 static int