Re: [PATCH 5/6 v2] sound: Add n64 driver

[Takashi Iwai <tiwai@suse.de>]

Hi,

thanks for the patch.  Now I started reviewing.  Some comments below.

On Fri, 08 Jan 2021 09:35:13 +0100,
Lauri Kasanen wrote:
> 
> This adds support for the Nintendo 64 console's sound.
> 
> The sound DMA unit has errata on certain alignments, which is why
> we can't use alsa's DMA buffer directly.

It's worth to mention this in the source code, too, before later
readers wonder it again.

> diff --git a/sound/mips/snd-n64.c b/sound/mips/snd-n64.c
> new file mode 100644
> index 0000000..0317fe2
> --- /dev/null
> +++ b/sound/mips/snd-n64.c
> @@ -0,0 +1,297 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *   Sound driver for Nintendo 64.
> + *
> + *   Copyright 2020 Lauri Kasanen

We moved forward, a happy new year :)

> +#define REG_BASE ((u32 *) CKSEG1ADDR(0x4500000))

Better to put __iomem?

> +#define MI_REG_BASE ((u32 *) CKSEG1ADDR(0x4300000))

Ditto.

> +struct n64audio_t {

We usually don't put _t suffix for a struct name.
It's only for typedefs.  So, just use struct n64audio.

> +static void n64audio_push(struct n64audio_t *priv, uint8_t irq)

This irq is a boolean flag, no?  Then use bool to be clearer.

> +static irqreturn_t n64audio_isr(int irq, void *dev_id)
> +{
> +	struct n64audio_t *priv = dev_id;
> +
> +	// Check it's ours
> +	const u32 intrs = n64mi_read_reg(MI_INTR_REG);

checkpatch would complain the blank line above (and the lack of it in
below).

> +	if (!(intrs & MI_INTR_AI))
> +		return IRQ_NONE;
> +
> +	n64audio_write_reg(AI_STATUS_REG, 1);
> +
> +	n64audio_push(priv, 1);
> +	snd_pcm_period_elapsed(priv->chan.substream);

It might be safer to check whether the stream is really present and
running.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct snd_pcm_hardware n64audio_pcm_hw = {
> +	.info = (SNDRV_PCM_INFO_MMAP |
> +		 SNDRV_PCM_INFO_MMAP_VALID |
> +		 SNDRV_PCM_INFO_INTERLEAVED |
> +		 SNDRV_PCM_INFO_BLOCK_TRANSFER),
> +	.formats =          SNDRV_PCM_FMTBIT_S16_BE,
> +	.rates =            SNDRV_PCM_RATE_8000_48000,
> +	.rate_min =         8000,
> +	.rate_max =         48000,
> +	.channels_min =     2,
> +	.channels_max =     2,
> +	.buffer_bytes_max = 32768,
> +	.period_bytes_min = 1024,
> +	.period_bytes_max = 32768,
> +	.periods_min =      1,

periods_min=1 makes little sense for this driver.

> +	.periods_max =      128,
> +};
> +
> +static int n64audio_pcm_open(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +
> +	runtime->hw = n64audio_pcm_hw;
> +	return 0;

You likely need to set up more hw constraints.
For example, unless you set the integer periods, API allows unaligned
buffer sizes, i.e. period=1024 and buffer=2500, and it screws up the
transfer for this driver implementation.

> +static int n64audio_pcm_prepare(struct snd_pcm_substream *substream)
> +{
....
> +	spin_lock_irqsave(&priv->chan.lock, flags);

The prepare callback is always non-atomic, so you can use
spin_lock_irq() here.

> +static int n64audio_pcm_close(struct snd_pcm_substream *substream)
> +{
> +	return 0; // Nothing to do, but the kernel crashes if close() doesn't exist

You may clear the substream pointer, for example.  Then ISR might be
able to avoid accessing something wrong if it were triggered
mistakenly.

> +static int __init n64audio_probe(struct platform_device *pdev)

Usually the probe callback itself shouldn't be __init.

> +{
> +	struct snd_card *card;
> +	struct snd_pcm *pcm;
> +	struct n64audio_t *priv;
> +	int err;
> +
> +	err = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1,
> +			   SNDRV_DEFAULT_STR1,
> +			   THIS_MODULE, 0, &card);
> +	if (err < 0)
> +		return err;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (priv == NULL) {
> +		err = -ENOMEM;
> +		goto fail_card;
> +	}

Note that card->private_data won't be released automatically.
For this kind of allocation, an easier way would be to pass
sizeof(*priv) in snd_card_new() call.  Then card->private_data points
to the allocated data and released altogether with snd_card_free().

> +
> +	priv->card = card;
> +	priv->ring_base = dma_alloc_coherent(card->dev, 32 * 1024,
> +					     &priv->ring_base_dma,
> +					     GFP_DMA | GFP_KERNEL);

There is no release for this?  I guess you need to define
card->private_free to do that or use devm stuff.

> +	if (!priv->ring_base)
> +		goto fail_alloc;
> +
> +	if (request_irq(RCP_IRQ, n64audio_isr,
> +				IRQF_SHARED, "N64 Audio", priv)) {
> +		err = -EBUSY;
> +		goto fail_alloc;
> +	}

Ditto, this needs the free_irq in card->private_free or devm.

> +
> +	spin_lock_init(&priv->chan.lock);

The initialization of spinlock must be done before registering the
ISR.   Do this at the very beginning.

> +static int __init n64audio_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_probe(&n64audio_driver, n64audio_probe);
> +
> +	return ret;

This can simplified.

> +fs_initcall(n64audio_init);

Does it have to be this initcall?


thanks,

Takashi