[alsa-devel] [PATCH 2/2] fm801: introduce __is_ready()/__is_valid() helpers

[Andy Shevchenko]

On Mon, Apr 28, 2014 at 1:25 PM, Takashi Iwai <tiwai at 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 at 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
> >
>



-- 
With Best Regards,
Andy Shevchenko