RE: [PATCH v4 1/3] ASoC: sh: Add RZ/G2L SSIF-2 driver

Hi Mark,

Thanks for the feedback.

Subject: Re: [PATCH v4 1/3] ASoC: sh: Add RZ/G2L SSIF-2 driver

On Fri, Aug 06, 2021 at 11:29:28AM +0100, Biju Das wrote:

...

+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;
• if (runtime->sample_bits != 16) {

• dev_err(ssi->dev, "Unsupported sample width: %d\n",
  

• 	runtime->sample_bits);
  

• return -EINVAL;
  

• }
• if (runtime->frame_bits != 32) {

• dev_err(ssi->dev, "Unsupported frame width: %d\n",
  

• 	runtime->sample_bits);
  

• return -EINVAL;
  

• }

You should be doing *all* this validation at the time things are configured, not during trigger.

OK, Will move sample_bits validation to hw_params.

From core/pcm_native.c,

frame_bits = channels * sample_bits;

since we are validating both channels and sample_bits, there is no need to validate frame_bits.

So I am dropping frame_bits validation on next version.

...

+static int rz_ssi_start_stop(struct rz_ssi_priv *ssi,

• 	     struct rz_ssi_stream *strm,
  

• 	     int start)
  

+{

• struct snd_pcm_substream *substream = strm->substream;
• u32 ssicr, ssifcr;
• int timeout;
• if (start) {

• bool is_play = rz_ssi_stream_is_play(ssi, substream);
  

...

...

• } else {

• strm->running = 0;
  

...

...

• }
• return 0;

+}

This is two functions merged into one with zero shared code, just make them two separate functions and then people don't need to guess if true is start or stop.

OK. Will split this into 2 separate function.

...

• 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);
  

• spin_lock_irqsave(&ssi->lock, flags);
  

• ret = rz_ssi_stream_init(ssi, strm, substream);
  

• spin_unlock_irqrestore(&ssi->lock, flags);
  

It's not clear what this lock is intended to accomplish. It's only used in trigger like this and in _stream_is_valid(). In trigger() we're already in atomic context so don't have to worry about simultaneous calls to trigger() while in _stream_is_valid() we're just checking if there's a runtime present but since the lock is taken and held within the function the status could change before we've even returned to the caller. The two uses don't seem joined up with each other and neither seems to do much.

Thanks for pointing out. After re-checking, locking is not needed at this place. It is required only for _stream_is_valid() and _stream_quit().

Cheers, Biju