Messages in this thread | | | Date | Mon, 25 Oct 2021 12:52:27 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 1/3] ASoC: rockchip: add support for spdifrx receiver |
| |
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. [unhandled content-type:application/pgp-signature] | |