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. > +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. > + 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.
Attachment:
signature.asc
Description: PGP signature