[alsa-devel] [PATCH 1/2] fm801: introduce macros to access the hardware
It will help to maintain HW accessors and, for example, switch from the direct I/O to MMIO which is more convenient for PCI devices.
Signed-off-by: Andy Shevchenko andy.shevchenko@gmail.com --- sound/pci/fm801.c | 131 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 68 insertions(+), 63 deletions(-)
diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c index db18cca..8418484 100644 --- a/sound/pci/fm801.c +++ b/sound/pci/fm801.c @@ -23,6 +23,7 @@ #include <linux/delay.h> #include <linux/init.h> #include <linux/interrupt.h> +#include <linux/io.h> #include <linux/pci.h> #include <linux/slab.h> #include <linux/module.h> @@ -34,8 +35,6 @@ #include <sound/opl3.h> #include <sound/initval.h>
-#include <asm/io.h> - #ifdef CONFIG_SND_FM801_TEA575X_BOOL #include <media/tea575x.h> #endif @@ -80,7 +79,10 @@ MODULE_PARM_DESC(radio_nr, "Radio device numbers"); * Direct registers */
-#define FM801_REG(chip, reg) (chip->port + FM801_##reg) +#define fm801_writew(v,chip,reg) outw((v), chip->port + FM801_##reg) +#define fm801_readw(chip,reg) inw(chip->port + FM801_##reg) + +#define fm801_writel(v,chip,reg) outl((v), chip->port + FM801_##reg)
#define FM801_PCM_VOL 0x00 /* PCM Output Volume */ #define FM801_FM_VOL 0x02 /* FM Output Volume */ @@ -250,7 +252,7 @@ static void snd_fm801_codec_write(struct snd_ac97 *ac97, * Wait until the codec interface is not ready.. */ for (idx = 0; idx < 100; idx++) { - if (!(inw(FM801_REG(chip, AC97_CMD)) & FM801_AC97_BUSY)) + if (!(fm801_readw(chip, AC97_CMD) & FM801_AC97_BUSY)) goto ok1; udelay(10); } @@ -259,13 +261,13 @@ static void snd_fm801_codec_write(struct snd_ac97 *ac97,
ok1: /* write data and address */ - outw(val, FM801_REG(chip, AC97_DATA)); - outw(reg | (ac97->addr << FM801_AC97_ADDR_SHIFT), FM801_REG(chip, AC97_CMD)); + 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 (!(inw(FM801_REG(chip, AC97_CMD)) & FM801_AC97_BUSY)) + if (!(fm801_readw(chip, AC97_CMD) & FM801_AC97_BUSY)) return; udelay(10); } @@ -281,7 +283,7 @@ static unsigned short snd_fm801_codec_read(struct snd_ac97 *ac97, unsigned short * Wait until the codec interface is not ready.. */ for (idx = 0; idx < 100; idx++) { - if (!(inw(FM801_REG(chip, AC97_CMD)) & FM801_AC97_BUSY)) + if (!(fm801_readw(chip, AC97_CMD) & FM801_AC97_BUSY)) goto ok1; udelay(10); } @@ -290,10 +292,10 @@ static unsigned short snd_fm801_codec_read(struct snd_ac97 *ac97, unsigned short
ok1: /* read command */ - outw(reg | (ac97->addr << FM801_AC97_ADDR_SHIFT) | FM801_AC97_READ, - FM801_REG(chip, AC97_CMD)); + fm801_writew(reg | (ac97->addr << FM801_AC97_ADDR_SHIFT) | + FM801_AC97_READ, chip, AC97_CMD); for (idx = 0; idx < 100; idx++) { - if (!(inw(FM801_REG(chip, AC97_CMD)) & FM801_AC97_BUSY)) + if (!(fm801_readw(chip, AC97_CMD) & FM801_AC97_BUSY)) goto ok2; udelay(10); } @@ -302,7 +304,7 @@ static unsigned short snd_fm801_codec_read(struct snd_ac97 *ac97, unsigned short
ok2: for (idx = 0; idx < 1000; idx++) { - if (inw(FM801_REG(chip, AC97_CMD)) & FM801_AC97_VALID) + if (fm801_readw(chip, AC97_CMD) & FM801_AC97_VALID) goto ok3; udelay(10); } @@ -310,7 +312,7 @@ static unsigned short snd_fm801_codec_read(struct snd_ac97 *ac97, unsigned short return 0;
ok3: - return inw(FM801_REG(chip, AC97_DATA)); + return fm801_readw(chip, AC97_DATA); }
static unsigned int rates[] = { @@ -384,7 +386,7 @@ static int snd_fm801_playback_trigger(struct snd_pcm_substream *substream, snd_BUG(); return -EINVAL; } - outw(chip->ply_ctrl, FM801_REG(chip, PLY_CTRL)); + fm801_writew(chip->ply_ctrl, chip, PLY_CTRL); spin_unlock(&chip->reg_lock); return 0; } @@ -419,7 +421,7 @@ static int snd_fm801_capture_trigger(struct snd_pcm_substream *substream, snd_BUG(); return -EINVAL; } - outw(chip->cap_ctrl, FM801_REG(chip, CAP_CTRL)); + fm801_writew(chip->cap_ctrl, chip, CAP_CTRL); spin_unlock(&chip->reg_lock); return 0; } @@ -457,12 +459,13 @@ static int snd_fm801_playback_prepare(struct snd_pcm_substream *substream) } chip->ply_ctrl |= snd_fm801_rate_bits(runtime->rate) << FM801_RATE_SHIFT; chip->ply_buf = 0; - outw(chip->ply_ctrl, FM801_REG(chip, PLY_CTRL)); - outw(chip->ply_count - 1, FM801_REG(chip, PLY_COUNT)); + fm801_writew(chip->ply_ctrl, chip, PLY_CTRL); + fm801_writew(chip->ply_count - 1, chip, PLY_COUNT); chip->ply_buffer = runtime->dma_addr; chip->ply_pos = 0; - outl(chip->ply_buffer, FM801_REG(chip, PLY_BUF1)); - outl(chip->ply_buffer + (chip->ply_count % chip->ply_size), FM801_REG(chip, PLY_BUF2)); + fm801_writel(chip->ply_buffer, chip, PLY_BUF1); + fm801_writel(chip->ply_buffer + (chip->ply_count % chip->ply_size), + chip, PLY_BUF2); spin_unlock_irq(&chip->reg_lock); return 0; } @@ -483,12 +486,13 @@ static int snd_fm801_capture_prepare(struct snd_pcm_substream *substream) chip->cap_ctrl |= FM801_STEREO; chip->cap_ctrl |= snd_fm801_rate_bits(runtime->rate) << FM801_RATE_SHIFT; chip->cap_buf = 0; - outw(chip->cap_ctrl, FM801_REG(chip, CAP_CTRL)); - outw(chip->cap_count - 1, FM801_REG(chip, CAP_COUNT)); + fm801_writew(chip->cap_ctrl, chip, CAP_CTRL); + fm801_writew(chip->cap_count - 1, chip, CAP_COUNT); chip->cap_buffer = runtime->dma_addr; chip->cap_pos = 0; - outl(chip->cap_buffer, FM801_REG(chip, CAP_BUF1)); - outl(chip->cap_buffer + (chip->cap_count % chip->cap_size), FM801_REG(chip, CAP_BUF2)); + fm801_writel(chip->cap_buffer, chip, CAP_BUF1); + fm801_writel(chip->cap_buffer + (chip->cap_count % chip->cap_size), + chip, CAP_BUF2); spin_unlock_irq(&chip->reg_lock); return 0; } @@ -501,8 +505,8 @@ static snd_pcm_uframes_t snd_fm801_playback_pointer(struct snd_pcm_substream *su if (!(chip->ply_ctrl & FM801_START)) return 0; spin_lock(&chip->reg_lock); - ptr = chip->ply_pos + (chip->ply_count - 1) - inw(FM801_REG(chip, PLY_COUNT)); - if (inw(FM801_REG(chip, IRQ_STATUS)) & FM801_IRQ_PLAYBACK) { + ptr = chip->ply_pos + (chip->ply_count - 1) - fm801_readw(chip, PLY_COUNT); + if (fm801_readw(chip, IRQ_STATUS) & FM801_IRQ_PLAYBACK) { ptr += chip->ply_count; ptr %= chip->ply_size; } @@ -518,8 +522,8 @@ static snd_pcm_uframes_t snd_fm801_capture_pointer(struct snd_pcm_substream *sub if (!(chip->cap_ctrl & FM801_START)) return 0; spin_lock(&chip->reg_lock); - ptr = chip->cap_pos + (chip->cap_count - 1) - inw(FM801_REG(chip, CAP_COUNT)); - if (inw(FM801_REG(chip, IRQ_STATUS)) & FM801_IRQ_CAPTURE) { + ptr = chip->cap_pos + (chip->cap_count - 1) - fm801_readw(chip, CAP_COUNT); + if (fm801_readw(chip, IRQ_STATUS) & FM801_IRQ_CAPTURE) { ptr += chip->cap_count; ptr %= chip->cap_size; } @@ -533,12 +537,12 @@ static irqreturn_t snd_fm801_interrupt(int irq, void *dev_id) unsigned short status; unsigned int tmp;
- status = inw(FM801_REG(chip, IRQ_STATUS)); + status = fm801_readw(chip, IRQ_STATUS); status &= FM801_IRQ_PLAYBACK|FM801_IRQ_CAPTURE|FM801_IRQ_MPU|FM801_IRQ_VOLUME; if (! status) return IRQ_NONE; /* ack first */ - outw(status, FM801_REG(chip, IRQ_STATUS)); + fm801_writew(status, chip, IRQ_STATUS); if (chip->pcm && (status & FM801_IRQ_PLAYBACK) && chip->playback_substream) { spin_lock(&chip->reg_lock); chip->ply_buf++; @@ -546,10 +550,10 @@ static irqreturn_t snd_fm801_interrupt(int irq, void *dev_id) chip->ply_pos %= chip->ply_size; tmp = chip->ply_pos + chip->ply_count; tmp %= chip->ply_size; - outl(chip->ply_buffer + tmp, - (chip->ply_buf & 1) ? - FM801_REG(chip, PLY_BUF1) : - FM801_REG(chip, PLY_BUF2)); + if (chip->ply_buf & 1) + fm801_writel(chip->ply_buffer + tmp, chip, PLY_BUF1); + else + fm801_writel(chip->ply_buffer + tmp, chip, PLY_BUF2); spin_unlock(&chip->reg_lock); snd_pcm_period_elapsed(chip->playback_substream); } @@ -560,10 +564,10 @@ static irqreturn_t snd_fm801_interrupt(int irq, void *dev_id) chip->cap_pos %= chip->cap_size; tmp = chip->cap_pos + chip->cap_count; tmp %= chip->cap_size; - outl(chip->cap_buffer + tmp, - (chip->cap_buf & 1) ? - FM801_REG(chip, CAP_BUF1) : - FM801_REG(chip, CAP_BUF2)); + if (chip->cap_buf & 1) + fm801_writel(chip->cap_buffer + tmp, chip, CAP_BUF1); + else + fm801_writel(chip->cap_buffer + tmp, chip, CAP_BUF2); spin_unlock(&chip->reg_lock); snd_pcm_period_elapsed(chip->capture_substream); } @@ -747,7 +751,7 @@ static struct snd_fm801_tea575x_gpio snd_fm801_tea575x_gpios[] = { static void snd_fm801_tea575x_set_pins(struct snd_tea575x *tea, u8 pins) { struct fm801 *chip = tea->private_data; - unsigned short reg = inw(FM801_REG(chip, GPIO_CTRL)); + unsigned short reg = fm801_readw(chip, GPIO_CTRL); struct snd_fm801_tea575x_gpio gpio = *get_tea575x_gpio(chip);
reg &= ~(FM801_GPIO_GP(gpio.data) | @@ -759,13 +763,13 @@ static void snd_fm801_tea575x_set_pins(struct snd_tea575x *tea, u8 pins) /* WRITE_ENABLE is inverted */ reg |= (pins & TEA575X_WREN) ? 0 : FM801_GPIO_GP(gpio.wren);
- outw(reg, FM801_REG(chip, GPIO_CTRL)); + fm801_writew(reg, chip, GPIO_CTRL); }
static u8 snd_fm801_tea575x_get_pins(struct snd_tea575x *tea) { struct fm801 *chip = tea->private_data; - unsigned short reg = inw(FM801_REG(chip, GPIO_CTRL)); + unsigned short reg = fm801_readw(chip, GPIO_CTRL); struct snd_fm801_tea575x_gpio gpio = *get_tea575x_gpio(chip); u8 ret;
@@ -780,7 +784,7 @@ static u8 snd_fm801_tea575x_get_pins(struct snd_tea575x *tea) static void snd_fm801_tea575x_set_direction(struct snd_tea575x *tea, bool output) { struct fm801 *chip = tea->private_data; - unsigned short reg = inw(FM801_REG(chip, GPIO_CTRL)); + unsigned short reg = fm801_readw(chip, GPIO_CTRL); struct snd_fm801_tea575x_gpio gpio = *get_tea575x_gpio(chip);
/* use GPIO lines and set write enable bit */ @@ -811,7 +815,7 @@ static void snd_fm801_tea575x_set_direction(struct snd_tea575x *tea, bool output FM801_GPIO_GP(gpio.clk)); }
- outw(reg, FM801_REG(chip, GPIO_CTRL)); + fm801_writew(reg, chip, GPIO_CTRL); }
static struct snd_tea575x_ops snd_fm801_tea_ops = { @@ -962,7 +966,7 @@ static int snd_fm801_get_mux(struct snd_kcontrol *kcontrol, struct fm801 *chip = snd_kcontrol_chip(kcontrol); unsigned short val;
- val = inw(FM801_REG(chip, REC_SRC)) & 7; + val = fm801_readw(chip, REC_SRC) & 7; if (val > 4) val = 4; ucontrol->value.enumerated.item[0] = val; @@ -1073,12 +1077,12 @@ static int wait_for_codec(struct fm801 *chip, unsigned int codec_id, { unsigned long timeout = jiffies + waits;
- outw(FM801_AC97_READ | (codec_id << FM801_AC97_ADDR_SHIFT) | reg, - FM801_REG(chip, AC97_CMD)); + fm801_writew(reg | (codec_id << FM801_AC97_ADDR_SHIFT) | + FM801_AC97_READ, chip, AC97_CMD); udelay(5); do { - if ((inw(FM801_REG(chip, AC97_CMD)) & (FM801_AC97_VALID|FM801_AC97_BUSY)) - == FM801_AC97_VALID) + if ((fm801_readw(chip, AC97_CMD) & + (FM801_AC97_VALID | FM801_AC97_BUSY)) == FM801_AC97_VALID) return 0; schedule_timeout_uninterruptible(1); } while (time_after(timeout, jiffies)); @@ -1093,10 +1097,10 @@ static int snd_fm801_chip_init(struct fm801 *chip, int resume) goto __ac97_ok;
/* codec cold reset + AC'97 warm reset */ - outw((1<<5) | (1<<6), FM801_REG(chip, CODEC_CTRL)); - inw(FM801_REG(chip, CODEC_CTRL)); /* flush posting data */ + fm801_writew((1 << 5) | (1 << 6), chip, CODEC_CTRL); + fm801_readw(chip, CODEC_CTRL); /* flush posting data */ udelay(100); - outw(0, FM801_REG(chip, CODEC_CTRL)); + fm801_writew(0, chip, CODEC_CTRL);
if (wait_for_codec(chip, 0, AC97_RESET, msecs_to_jiffies(750)) < 0) if (!resume) { @@ -1117,7 +1121,7 @@ static int snd_fm801_chip_init(struct fm801 *chip, int resume) for (i = 3; i > 0; i--) { if (!wait_for_codec(chip, i, AC97_VENDOR_ID1, msecs_to_jiffies(50))) { - cmdw = inw(FM801_REG(chip, AC97_DATA)); + cmdw = fm801_readw(chip, AC97_DATA); if (cmdw != 0xffff && cmdw != 0) { chip->secondary = 1; chip->secondary_addr = i; @@ -1135,23 +1139,24 @@ static int snd_fm801_chip_init(struct fm801 *chip, int resume) __ac97_ok:
/* init volume */ - outw(0x0808, FM801_REG(chip, PCM_VOL)); - outw(0x9f1f, FM801_REG(chip, FM_VOL)); - outw(0x8808, FM801_REG(chip, I2S_VOL)); + fm801_writew(0x0808, chip, PCM_VOL); + fm801_writew(0x9f1f, chip, FM_VOL); + fm801_writew(0x8808, chip, I2S_VOL);
/* I2S control - I2S mode */ - outw(0x0003, FM801_REG(chip, I2S_MODE)); + fm801_writew(0x0003, chip, I2S_MODE);
/* interrupt setup */ - cmdw = inw(FM801_REG(chip, IRQ_MASK)); + cmdw = fm801_readw(chip, IRQ_MASK); if (chip->irq < 0) cmdw |= 0x00c3; /* mask everything, no PCM nor MPU */ else cmdw &= ~0x0083; /* unmask MPU, PLAYBACK & CAPTURE */ - outw(cmdw, FM801_REG(chip, IRQ_MASK)); + fm801_writew(cmdw, chip, IRQ_MASK);
/* interrupt clear */ - outw(FM801_IRQ_PLAYBACK|FM801_IRQ_CAPTURE|FM801_IRQ_MPU, FM801_REG(chip, IRQ_STATUS)); + fm801_writew(FM801_IRQ_PLAYBACK | FM801_IRQ_CAPTURE | FM801_IRQ_MPU, + chip, IRQ_STATUS);
return 0; } @@ -1165,9 +1170,9 @@ static int snd_fm801_free(struct fm801 *chip) goto __end_hw;
/* interrupt setup - mask everything */ - cmdw = inw(FM801_REG(chip, IRQ_MASK)); + cmdw = fm801_readw(chip, IRQ_MASK); cmdw |= 0x00c3; - outw(cmdw, FM801_REG(chip, IRQ_MASK)); + fm801_writew(cmdw, chip, IRQ_MASK);
__end_hw: #ifdef CONFIG_SND_FM801_TEA575X_BOOL @@ -1339,15 +1344,15 @@ static int snd_card_fm801_probe(struct pci_dev *pci, return err; } if ((err = snd_mpu401_uart_new(card, 0, MPU401_HW_FM801, - FM801_REG(chip, MPU401_DATA), + chip->port + FM801_MPU401_DATA, MPU401_INFO_INTEGRATED | MPU401_INFO_IRQ_HOOK, -1, &chip->rmidi)) < 0) { snd_card_free(card); return err; } - if ((err = snd_opl3_create(card, FM801_REG(chip, OPL3_BANK0), - FM801_REG(chip, OPL3_BANK1), + if ((err = snd_opl3_create(card, chip->port + FM801_OPL3_BANK0, + chip->port + FM801_OPL3_BANK1, OPL3_HW_OPL3_FM801, 1, &opl3)) < 0) { snd_card_free(card); return err;
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; +} + +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); }
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
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
At Mon, 28 Apr 2014 11:00:29 +0300, Andy Shevchenko wrote:
It will help to maintain HW accessors and, for example, switch from the direct I/O to MMIO which is more convenient for PCI devices.
Signed-off-by: Andy Shevchenko andy.shevchenko@gmail.com
sound/pci/fm801.c | 131 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 68 insertions(+), 63 deletions(-)
diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c index db18cca..8418484 100644 --- a/sound/pci/fm801.c +++ b/sound/pci/fm801.c @@ -23,6 +23,7 @@ #include <linux/delay.h> #include <linux/init.h> #include <linux/interrupt.h> +#include <linux/io.h> #include <linux/pci.h> #include <linux/slab.h> #include <linux/module.h> @@ -34,8 +35,6 @@ #include <sound/opl3.h> #include <sound/initval.h>
-#include <asm/io.h>
#ifdef CONFIG_SND_FM801_TEA575X_BOOL #include <media/tea575x.h> #endif @@ -80,7 +79,10 @@ MODULE_PARM_DESC(radio_nr, "Radio device numbers");
- Direct registers
*/
-#define FM801_REG(chip, reg) (chip->port + FM801_##reg) +#define fm801_writew(v,chip,reg) outw((v), chip->port + FM801_##reg) +#define fm801_readw(chip,reg) inw(chip->port + FM801_##reg)
+#define fm801_writel(v,chip,reg) outl((v), chip->port + FM801_##reg)
IMO, it's better in a form like fm801_writel(chip, reg, value)
thanks,
Takashi
On Mon, Apr 28, 2014 at 1:20 PM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 28 Apr 2014 11:00:29 +0300, Andy Shevchenko wrote:
It will help to maintain HW accessors and, for example, switch from the direct I/O to MMIO which is more convenient for PCI devices.
Signed-off-by: Andy Shevchenko andy.shevchenko@gmail.com
sound/pci/fm801.c | 131
++++++++++++++++++++++++++++--------------------------
1 file changed, 68 insertions(+), 63 deletions(-)
diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c index db18cca..8418484 100644 --- a/sound/pci/fm801.c +++ b/sound/pci/fm801.c @@ -23,6 +23,7 @@ #include <linux/delay.h> #include <linux/init.h> #include <linux/interrupt.h> +#include <linux/io.h> #include <linux/pci.h> #include <linux/slab.h> #include <linux/module.h> @@ -34,8 +35,6 @@ #include <sound/opl3.h> #include <sound/initval.h>
-#include <asm/io.h>
#ifdef CONFIG_SND_FM801_TEA575X_BOOL #include <media/tea575x.h> #endif @@ -80,7 +79,10 @@ MODULE_PARM_DESC(radio_nr, "Radio device numbers");
- Direct registers
*/
-#define FM801_REG(chip, reg) (chip->port + FM801_##reg) +#define fm801_writew(v,chip,reg) outw((v), chip->port + FM801_##reg) +#define fm801_readw(chip,reg) inw(chip->port +
FM801_##reg)
+#define fm801_writel(v,chip,reg) outl((v), chip->port + FM801_##reg)
IMO, it's better in a form like fm801_writel(chip, reg, value)
Will change this in v2.
thanks,
Takashi
participants (2)
-
Andy Shevchenko -
Takashi Iwai