Re: [alsa-devel] [PATCH 1/2] Xlinx ML403 AC97 Controller Reference device driver

[Takashi Iwai <tiwai@suse.de>]

Hi,

thanks for the patch.  Here the quick review at a first glance...

At Thu, 09 Aug 2007 12:36:50 +0200,
Joachim Förster wrote:
> 
> --- a/sound/ppc/Makefile
> +++ b/sound/ppc/Makefile
> @@ -4,7 +4,9 @@
>  #
>  
>  snd-powermac-objs := powermac.o pmac.o awacs.o burgundy.o daca.o tumbler.o keywest.o beep.o
> +snd-ml403_ac97cr-objs := ml403_ac97cr.o
>  
>  # Toplevel Module Dependency
>  obj-$(CONFIG_SND_POWERMAC)	+= snd-powermac.o
>  obj-$(CONFIG_SND_PS3)		+= snd_ps3.o
> +obj-$(CONFIG_SND_ML403_AC97CR)	+= snd-ml403_ac97cr.o

Using both _ and - look weird.  Let's choose '-' as well as others.

> --- /dev/null
> +++ b/sound/ppc/ml403_ac97cr.c
(snip)
> +static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
> +static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
> +static int enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE;

Can the driver really support multiple instances?
If not, better to avoid these arrays.

> +#define PDEBUG(fac, fmt, args...) if (fac & PDEBUG_FACILITIES) \
> +		snd_printd(KERN_DEBUG SND_ML403_AC97CR_DRIVER ": " fmt, ##args)

This should be wrapped with do { } while (0)

> +struct lm4550_reg lm4550_regfile[64] = {
> +	{.reg = 0x00,
> +	 .flag = LM4550_REG_NOSAVE | LM4550_REG_FAKEREAD,
> +	 .def = 0x0D50},

Better to use C99 style initialization here, e.g.

	[0x00] = { .... },
	[0x02] = { .... },
	[0x7e] = { .... },

so you can avoid writing empty items.
The index value could be also AC97_XXX, such as [AC97_MASTER] =
{...}.

What is the purpose of reg field at all, BTW?  I guess it's
superfluous.

> +#define LM4550_RF_OK(reg)    ((lm4550_regfile[reg / 2].reg != 0) \
> +			      || (lm4550_regfile[reg / 2].flag != 0))
> +#define LM4550_RF_FLAG(reg)  lm4550_regfile[reg / 2].flag
> +#define LM4550_RF_VAL(reg)   lm4550_regfile[reg / 2].value
> +#define LM4550_RF_DEF(reg)   lm4550_regfile[reg / 2].def
> +#define LM4550_RF_WMASK(reg) lm4550_regfile[reg / 2].wmask
> +
> +static void lm4550_regfile_init(void)
> +{
> +	int i;
> +	for (i = 0; i < 128; i = i + 2)
> +		if (LM4550_RF_FLAG(i) & LM4550_REG_FAKEPROBE)
> +			LM4550_RF_VAL(i) = LM4550_RF_DEF(i);

"MACRO(x) = XXX" looks a bit strange to me.

> +static struct snd_pcm_hardware snd_ml403_ac97cr_playback = {
> +	.info =	            (SNDRV_PCM_INFO_MMAP |
> +		             SNDRV_PCM_INFO_INTERLEAVED |
> +		             SNDRV_PCM_INFO_MMAP_VALID),
> +	.formats =          SNDRV_PCM_FMTBIT_S16_BE,
> +	.rates =	    (SNDRV_PCM_RATE_CONTINUOUS |
> +			     SNDRV_PCM_RATE_8000_48000 |
> +			     SNDRV_PCM_RATE_KNOT),

RATE_CONTINUOUS and RATE_KNOW are usually exclusive.

> +static int
> +snd_ml403_ac97cr_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct snd_ml403_ac97cr *ml403_ac97cr;
> +	int err = 0;
> +
> +	ml403_ac97cr = snd_pcm_substream_chip(substream);
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +		if (substream == ml403_ac97cr->playback_substream) {
> +			PDEBUG(WORK_INFO, "trigger(playback): START\n");
> +			ml403_ac97cr->ind_rec.hw_ready = 1;
> +
> +			/* clear play FIFO */
> +			out_be32(CR_REG(ml403_ac97cr, RESETFIFO), CR_PLAYRESET);
> +
> +			/* enable play irq */
> +			ml403_ac97cr->enable_irq = 1;
> +			enable_irq(ml403_ac97cr->irq);
> +		}

Maybe better to create two functions, one for playback and one for
capture, instead of many if's in a function?

> +static int snd_ml403_ac97cr_pcm_prepare(struct snd_pcm_substream *substream)
> +{
> +	struct snd_ml403_ac97cr *ml403_ac97cr;
> +	struct snd_pcm_runtime *runtime;
> +
> +	ml403_ac97cr = snd_pcm_substream_chip(substream);
> +	runtime = substream->runtime;
> +
> +	if (substream == ml403_ac97cr->playback_substream) {

Ditto.

> +static int
> +snd_ml403_ac97cr_hw_params(struct snd_pcm_substream *substream,
> +			   struct snd_pcm_hw_params *hw_params)
> +{
> +	PDEBUG(WORK_INFO, "hw_params(): desired buffer bytes=%d, desired "
> +	       "period bytes=%d\n",
> +	       params_buffer_bytes(hw_params), params_period_bytes(hw_params));
> +	/* check period bytes, has to be multiple of CR_FIFO_SIZE / 2, don't
> +	 * know if ALSA takes multiples of period_bytes_min _only_ ...?!
> +	 */

This should be done via an additional hw_constraint at open,

	snd_pcm_hw_constraint_step(runtime, 0,
		SNDRV_PCM_HW_PARAM_PERIOD_BYTES, CR_FIFO_SIZE / 2);

> +struct snd_pcm_ops snd_ml403_ac97cr_playback_ops = {

Missing static.

> +irqreturn_t snd_ml403_ac97cr_irq(int irq, void *dev_id, struct pt_regs *regs)

Hm, this looks like an old style irq handler.
Did you test the driver with the recent kernel?

Also, missing static there.

> +static unsigned short
> +snd_ml403_ac97cr_codec_read(struct snd_ac97 *ac97, unsigned short reg)
....
> +	/* if we are here, we _have_ to access the codec really, no faking */
> +	spin_lock(&ml403_ac97cr->reg_lock);
....
> +	do {
....
> +		schedule_timeout_uninterruptible(1);
> +	} while (time_after(end_time, jiffies));

Sleep in spinlock is bad.

> +static int __init
> +snd_ml403_ac97cr_create(struct snd_card *card, struct platform_device *pfdev,
> +			struct snd_ml403_ac97cr **rml403_ac97cr)

It's no longer __init as long as you use platform_device.
It should be __devinit instead.


> --- /dev/null
> +++ b/sound/ppc/pcm-indirect2.h
(snip)
> +#ifdef SND_PCM_INDIRECT2_STAT
> +static inline void snd_pcm_indirect2_stat(struct snd_pcm_substream *substream,
> +					  struct snd_pcm_indirect2 *rec)

Remove inline from the functions in this file.  They are too lengthy.

sound/pcm-indirect.h contain inline functions becuase they are
relatively small, and I didn't want to add them in the core module
unconditionally.


Last but not least, the whole patch appears not following strictly the
standard kernel coding style.  For example, the style

	if ((err = foo()) < 0)
		....

should be

	err = foo();
	if (err < 0)
		...

Also, many lines are over 80 chars.  Fold them appropriately.
At least the new patches should be like that.

In doubt, you can try $KERNEL/scripts/checkpatch.pl script, which was
added recently to linux kernel tree.


Takashi