Re: [PATCH 1/3] ASoC: rockchip: add support for spdifrx receiver

From: Mark Brown
Date: Mon Oct 25 2021 - 07:52:39 EST


On Sun, Oct 24, 2021 at 05:43:15PM +0800, Yunhao Tian wrote:

> --- /dev/null
> +++ b/sound/soc/rockchip/rockchip_spdifrx.c
> @@ -0,0 +1,660 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ALSA SoC Audio Layer - Rockchip SPDIF_RX Controller driver
> + *

Please make the entire comment a C++ one so things look more
intentional.

> +static int rk_spdifrx_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + return 0;
> +}

Just remove empty callbacks, if it's safe to omit a callback the
framework will let you.

> + spin_lock_irqsave(&spdifrx->irq_lock, flags);
> +
> + /* the access property of controls don't have to
> + * be volatile, as it will be notified by interrupt handler
> + */
> + spdifrx->dai = dai;
> + control = (struct snd_kcontrol_new){
> + /* Sample Rate Control */
> + .iface = SNDRV_CTL_ELEM_IFACE_PCM,
> + .name = SNDRV_CTL_NAME_IEC958("", CAPTURE, NONE) "Rate",
> + /* access don't have to be volatile, as it will be notified by intr */
> + .access = SNDRV_CTL_ELEM_ACCESS_READ,
> + .info = rk_spdifrx_rate_info,
> + .get = rk_spdifrx_rate_get,
> + };
> + spdifrx->snd_kctl_rate = snd_ctl_new1(&control, dai);

You really shouldn't be calling snd_ctl_new1() with interrupts disabled,
nor can I see a reason why you'd want to do this. I'd be surprised if
it doesn't do any allocations or other operations that are not permitted
while in atomic context.

I also don't understand why the control is declared in this way rather
than just being a normal const static struct, there's no variable data
in any of these structs that I can see.

> +static irqreturn_t rk_spdifrx_isr(int irq, void *dev_id)
> +{

> + spin_lock_irqsave(&spdifrx->irq_lock, flags);
> + if (spdifrx->dai) {

You're in the interrupt handler here so this should just be a regular
spin_lock().

> +MODULE_DESCRIPTION("ROCKCHIP SPDIF Receiver Interface");
> +MODULE_AUTHOR("Sugar Zhang <sugar.zhang@xxxxxxxxxxxxxx>");

Given that Sugar is active upstream it would be good to keep them in the
CCs.

Attachment: signature.asc
Description: PGP signature