Re: [PATCH 2/2] fm801: introduce __is_ready()/__is_valid() helpers

[Takashi Iwai <tiwai@suse.de>]

At Mon, 28 Apr 2014 11:00:30 +0300,
Andy Shevchenko wrote:
> 
> The introduced functios check AC97 if it's ready for communication and
> read data is valid.
> 
> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  sound/pci/fm801.c | 86 +++++++++++++++++++++++++++----------------------------
>  1 file changed, 42 insertions(+), 44 deletions(-)
> 
> diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c
> index 8418484..4417409 100644
> --- a/sound/pci/fm801.c
> +++ b/sound/pci/fm801.c
> @@ -224,6 +224,30 @@ MODULE_DEVICE_TABLE(pci, snd_fm801_ids);
>   *  common I/O routines
>   */
>  
> +static inline bool __is_ready(struct fm801 *chip, unsigned int iterations)
> +{
> +	unsigned int idx;
> +
> +	for (idx = 0; idx < iterations; idx++) {
> +		if (!(fm801_readw(chip, AC97_CMD) & FM801_AC97_BUSY))
> +			return true;
> +		udelay(10);
> +	}
> +	return false;
> +}

The function name is a bit too ambiguous, and you don't have to use
"__" prefix unnecessarily.  Also, don't add inline unless really
needed.  Compilers are often smarter than us.

> +
> +static inline bool __is_valid(struct fm801 *chip, unsigned int iterations)
> +{
> +	unsigned int idx;
> +
> +	for (idx = 0; idx < iterations; idx++) {
> +		if (!(fm801_readw(chip, AC97_CMD) & FM801_AC97_VALID))
> +			return true;
> +		udelay(10);
> +	}
> +	return false;
> +}

Does the patch really work as expected?  It returns true when no VALID
bit is set.


thanks,

Takashi


> +
>  static int snd_fm801_update_bits(struct fm801 *chip, unsigned short reg,
>  				 unsigned short mask, unsigned short value)
>  {
> @@ -246,32 +270,19 @@ static void snd_fm801_codec_write(struct snd_ac97 *ac97,
>  				  unsigned short val)
>  {
>  	struct fm801 *chip = ac97->private_data;
> -	int idx;
>  
> -	/*
> -	 *  Wait until the codec interface is not ready..
> -	 */
> -	for (idx = 0; idx < 100; idx++) {
> -		if (!(fm801_readw(chip, AC97_CMD) & FM801_AC97_BUSY))
> -			goto ok1;
> -		udelay(10);
> +	if (!__is_ready(chip, 100)) {
> +		dev_err(chip->card->dev, "AC'97 interface is busy (1)\n");
> +		return;
>  	}
> -	dev_err(chip->card->dev, "AC'97 interface is busy (1)\n");
> -	return;
>  
> - ok1:
>  	/* write data and address */
>  	fm801_writew(val, chip, AC97_DATA);
>  	fm801_writew(reg | (ac97->addr << FM801_AC97_ADDR_SHIFT), chip, AC97_CMD);
> -	/*
> -	 *  Wait until the write command is not completed..
> -         */
> -	for (idx = 0; idx < 1000; idx++) {
> -		if (!(fm801_readw(chip, AC97_CMD) & FM801_AC97_BUSY))
> -			return;
> -		udelay(10);
> -	}
> -	dev_err(chip->card->dev, "AC'97 interface #%d is busy (2)\n", ac97->num);
> +
> +	if (!__is_ready(chip, 1000))
> +		dev_err(chip->card->dev, "AC'97 interface #%d is busy (2)\n",
> +		ac97->num);
>  }
>  
>  static unsigned short snd_fm801_codec_read(struct snd_ac97 *ac97, unsigned short reg)
> @@ -279,39 +290,26 @@ static unsigned short snd_fm801_codec_read(struct snd_ac97 *ac97, unsigned short
>  	struct fm801 *chip = ac97->private_data;
>  	int idx;
>  
> -	/*
> -	 *  Wait until the codec interface is not ready..
> -	 */
> -	for (idx = 0; idx < 100; idx++) {
> -		if (!(fm801_readw(chip, AC97_CMD) & FM801_AC97_BUSY))
> -			goto ok1;
> -		udelay(10);
> +	if (!__is_ready(chip, 100)) {
> +		dev_err(chip->card->dev, "AC'97 interface is busy (1)\n");
> +		return 0;
>  	}
> -	dev_err(chip->card->dev, "AC'97 interface is busy (1)\n");
> -	return 0;
>  
> - ok1:
>  	/* read command */
>  	fm801_writew(reg | (ac97->addr << FM801_AC97_ADDR_SHIFT) |
>  		     FM801_AC97_READ, chip, AC97_CMD);
> -	for (idx = 0; idx < 100; idx++) {
> -		if (!(fm801_readw(chip, AC97_CMD) & FM801_AC97_BUSY))
> -			goto ok2;
> -		udelay(10);
> +	if (!__is_ready(chip, 100)) {
> +		dev_err(chip->card->dev, "AC'97 interface #%d is busy (2)\n",
> +			ac97->num);
> +		return 0;
>  	}
> -	dev_err(chip->card->dev, "AC'97 interface #%d is busy (2)\n", ac97->num);
> -	return 0;
>  
> - ok2:
> -	for (idx = 0; idx < 1000; idx++) {
> -		if (fm801_readw(chip, AC97_CMD) & FM801_AC97_VALID)
> -			goto ok3;
> -		udelay(10);
> +	if (!__is_valid(chip, 1000)) {
> +		dev_err(chip->card->dev,
> +			"AC'97 interface #%d is not valid (2)\n", ac97->num);
> +		return 0;
>  	}
> -	dev_err(chip->card->dev, "AC'97 interface #%d is not valid (2)\n", ac97->num);
> -	return 0;
>  
> - ok3:
>  	return fm801_readw(chip, AC97_DATA);
>  }
>  
> -- 
> 1.8.3.101.g727a46b
>