[alsa-devel] [PATCH] PCI168 snd-azt3328 Linux driver: another huge update

[Takashi Iwai]

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