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
