On Thu, Jul 29, 2021 at 06:23:10PM +0100, Biju Das wrote:
+config SND_SOC_RZ
- tristate "RZ/G2L series SSIF-2 support"
- depends on ARCH_R9A07G044 || COMPILE_TEST
- select SND_SIMPLE_CARD
- help
Why is the DAI selecting a specific sound card, and if it must then why would it use simple-card and not the more modern audio-graph-card? The select should almost certainly just be removed entirely, this is not something DAI drivers do - cards use DAIs, not the other way around.
+++ b/sound/soc/sh/rz-ssi.c @@ -0,0 +1,871 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Renesas RZ/G2L ASoC Serial Sound Interface (SSIF-2) Driver
- Copyright (C) 2021 Renesas Electronics Corp.
Please make the entire comment a C++ one so things look more intentional.
+static int rz_ssi_stream_init(struct rz_ssi_priv *ssi,
struct rz_ssi_stream *strm,struct snd_pcm_substream *substream)+{
- struct snd_pcm_runtime *runtime = substream->runtime;
- strm->substream = substream;
- if (runtime->sample_bits != 16) {
dev_err(ssi->dev, "Unsupported sample width: %d\n",runtime->sample_bits);strm->substream = NULL;return -EINVAL;- }
- if (runtime->frame_bits != 32) {
dev_err(ssi->dev, "Unsupported frame width: %d\n",runtime->sample_bits);strm->substream = NULL;return -EINVAL;- }
- if (runtime->channels != 2) {
dev_err(ssi->dev, "Number of channels not matched\n");strm->substream = NULL;return -EINVAL;- }
Why are we only validating these here which is called from...
- switch (cmd) {
- case SNDRV_PCM_TRIGGER_START:
/* Soft Reset */rz_ssi_reg_mask_setl(ssi, SSIFCR, 0, SSIFCR_SSIRST);rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_SSIRST, 0);udelay(5);ret = rz_ssi_stream_init(ssi, strm, substream);if (ret)goto done;
...trigger. This stuff should be being validated when we set parameters in hw_params() and can usefully report the error.
+static int rz_ssi_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) +{
- struct rz_ssi_priv *ssi = snd_soc_dai_get_drvdata(dai);
- /* set master/slave audio interface */
- switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
- case SND_SOC_DAIFMT_CBS_CFS:
asm("nop"); /* codec is slave */break;
That asm("nop") looks interesting... I can't think why that'd be needed? Please also use the modern versions of these defines, consumer and provider rather than slave and master.