At Wed, 14 May 2008 23:26:11 +0200,
Andreas Mohr wrote:
>
> +typedef struct {
> + struct snd_pcm_substream *substream;
> + int enabled;
> + int running;
> + unsigned long portbase;
> +} snd_azf3328_audio_stream_t;
> +
> +typedef enum {
> + AZF_PLAYBACK = 0,
> + AZF_CAPTURE = 1,
> +} snd_azf3328_stream_index_t;
Try to avoid unneeded typedefs.
> @@ -237,9 +292,9 @@
> /* register value containers for power management
> * Note: not always full I/O range preserved (just like Win driver!) */
> u16 saved_regs_codec [AZF_IO_SIZE_CODEC_PM / 2];
> - u16 saved_regs_io2 [AZF_IO_SIZE_IO2_PM / 2];
> + u16 saved_regs_game [AZF_IO_SIZE_GAME_PM / 2];
Don't you want to align the bracket, too? :)
> u16 saved_regs_mpu [AZF_IO_SIZE_MPU_PM / 2];
> - u16 saved_regs_synth[AZF_IO_SIZE_SYNTH_PM / 2];
> + u16 saved_regs_opl3[AZF_IO_SIZE_OPL3_PM / 2];
> u16 saved_regs_mixer[AZF_IO_SIZE_MIXER_PM / 2];
> #endif
> };
> @@ -252,126 +307,181 @@
>
> MODULE_DEVICE_TABLE(pci, snd_azf3328_ids);
>
> +
> +static int
> +snd_azf3328_io_reg_setb(unsigned reg, u8 mask, int do_set)
> +{
> + u8 prev = inb(reg), new;
> +
> + new = (do_set) ? prev|mask : prev & ~mask;
Put spaces around ops.
> + /* we need to always write the new value no matter whether it differs or not,
> + * since some register bits don't indicate their setting */
> + outb(new, reg);
> + if (new != prev)
> + return 1;
> +
> + return 0;
> +}
Use the normal indent level. (Strange that checkpatch didn't notice
it.)
> +
> +static int
> +snd_azf3328_io_reg_setw(unsigned reg, u16 mask, int do_set)
> +{
> + u16 prev = inw(reg), new;
> +
> + new = (do_set) ? prev|mask : prev & ~mask;
> + /* we need to always write the new value no matter whether it differs or not,
> + * since some register bits don't indicate their setting */
> + outw(new, reg);
> + if (new != prev)
> + return 1;
> +
> + return 0;
> +}
Ditto.
> +static void
> +snd_azf3328_codec_activity(struct snd_azf3328 *chip,
(snip)
> + /* small check to prevent shutting down the other party
> + * in case it's active */
> + if (!((!enable) && (chip->audio_stream[other].running)))
Let's reduce unneeded parentheses - or make it a simpler one like:
if (enable || !chip->audio_stream[other].running)
> @@ -930,20 +1096,22 @@
> case SNDRV_PCM_TRIGGER_START:
(snip)
> spin_lock(&chip->reg_lock);
> - /* stop playback */
> status1 = snd_azf3328_codec_inw(chip, IDX_IO_PLAY_FLAGS);
> +
> + /* stop playback */
> status1 &= ~DMA_RESUME;
> snd_azf3328_codec_outw(chip, IDX_IO_PLAY_FLAGS, status1);
> -
> +
> /* FIXME: clear interrupts or what??? */
> snd_azf3328_codec_outw(chip, IDX_IO_PLAY_IRQTYPE, 0xffff);
> spin_unlock(&chip->reg_lock);
This part can be replaced easier with the new
snd_azf3328_io_reg_setw(). Ditto for other actions.
> snd_azf3328_dbgplay("Interrupt %ld!\nIDX_IO_PLAY_FLAGS %04x, IDX_IO_PLAY_IRQTYPE %04x, IDX_IO_IRQSTATUS %04x\n",
> - irq_count,
> + irq_count++,
Don't do this in a macro, especially if it's not always compiled in.
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
