diff mbox

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

Message ID 1398672030-29565-2-git-send-email-andy.shevchenko@gmail.com (mailing list archive)
State Superseded
Headers show

Commit Message

Andy Shevchenko April 28, 2014, 8 a.m. UTC 
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(-)

Comments

Takashi Iwai April 28, 2014, 10:25 a.m. UTC | #1 
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
>
Andy Shevchenko April 28, 2014, 11:26 a.m. UTC | #2 
On Mon, Apr 28, 2014 at 1:25 PM, Takashi Iwai <tiwai@suse.de> wrote:

> 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.
>

Understood. Will change this in v2.


>
> > +
> > +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.
>

Oops, rebase issue. Thanks for caught this up!


>
>
> 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
> >
>
diff mbox

Patch

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;
+}
+
+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;
+}
+
 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);
 }