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@rock-chips.com");
Given that Sugar is active upstream it would be good to keep them in the CCs.