RE: [PATCH v3 2/3] sound: soc: sh: Add RZ/G2L SSIF-2 driver

Hi Mark,

Thanks for the feedback.

Subject: Re: [PATCH v3 2/3] sound: soc: sh: Add RZ/G2L SSIF-2 driver

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.

OK, will remove the select.

...

+++ 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.

OK.

...

+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...

OK, will move this part to hw_params and validate there using params_channel.

...

• 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?

It is not required. Will remove it.

Please also use the modern versions of these defines, consumer and provider rather than slave and master.

OK, will use consumer and provider for the above macros.

Cheers, Biju