On Wed, 13 Jan 2021 13:27:21 +0100,
Lauri Kasanen wrote:
>
> This adds support for the Nintendo 64 console's sound.
>
> Signed-off-by: Lauri Kasanen <cand@xxxxxxx>
Thanks, the patch looks better.
The comments about the double buffer looks explanatory enough.
OK, let's go for some other bits:
> --- a/sound/mips/Kconfig
> +++ b/sound/mips/Kconfig
> @@ -24,5 +24,12 @@ config SND_SGI_HAL2
> help
> Sound support for the SGI Indy and Indigo2 Workstation.
>
> +config SND_N64
> + bool "N64 Audio"
> + depends on MACH_NINTENDO64
> + select SND_PCM
> + help
> + Sound support for the N64.
Does this bool work if CONFIG_SND=m is chosen beforehand?
Just to be sure.
> --- /dev/null
> +++ b/sound/mips/snd-n64.c
....
> +static void n64audio_push(struct n64audio *priv)
> +{
> + struct snd_pcm_runtime *runtime = priv->chan.substream->runtime;
> + unsigned long flags;
> + u32 count;
> +
> + spin_lock_irqsave(&priv->chan.lock, flags);
> +
> + count = priv->chan.writesize;
> + count &= ~7;
This restriction can be applied in hw_rule_period_size(), too.
Otherwise there may be discrepancy between the period size and the
actually handled size.
> +static unsigned is_pow2(const unsigned in)
> +{
> + if (in < 2) return 0;
> + return !(in & (in - 1));
There is already a helper is_power_of_2() in linux/log.h.
> +/*
> + * The target device is embedded and RAM-constrained. We save RAM
> + * by initializing in __init code that gets dropped late in boot.
> + * For the same reason there is no module or unloading support.
> + */
> +static int __init n64audio_probe(struct platform_device *pdev)
> +{
> + struct snd_card *card;
> + struct snd_pcm *pcm;
> + struct n64audio *priv;
> + struct resource *res;
> + unsigned long len;
> + 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;
It's easier to pass sizeof(*priv) to snd_card_new() for the extra_size
argument, and card->private_data is assigned for this.
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (request_irq(res->start, n64audio_isr,
> + IRQF_SHARED, "N64 Audio", priv)) {
> + err = -EBUSY;
> + goto fail_dma_alloc;
> + }
The IRQ should be assigned at the end; otherwise you may hit a bogus
access due to the shared IRQ.
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + len = res->end - res->start + 1;
> + priv->mi_reg_base = ioremap(res->start, len);
ioremap() may fail, so a NULL check is needed.
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + len = res->end - res->start + 1;
> + priv->ai_reg_base = ioremap(res->start, len);
Ditto.
thanks,
Takashi
