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

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20210806/671431eb/attachment.sig>