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

[Biju Das]

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