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
