Re: [PATCH 2/5] soundwire: stream: add helper to startup/shutdown streams

[Pierre-Louis Bossart]

Thanks Vinod for the review.

> > --- a/drivers/soundwire/stream.c
> > +++ b/drivers/soundwire/stream.c
> > @@ -13,6 +13,9 @@
> >   #include <linux/slab.h>
> >   #include <linux/soundwire/sdw_registers.h>
> >   #include <linux/soundwire/sdw.h>
> > +#include <sound/core.h>
>
> Do we really need core header?

No we don't, the only thing needed in sound/soc.h it seems.

> > +#include <sound/pcm.h>
> > +#include <sound/soc.h>
> >   #include "bus.h"
> >   
> >   /*
> > @@ -1826,3 +1829,90 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
> >   	return ret;
> >   }
> >   EXPORT_SYMBOL(sdw_deprepare_stream);
> > +
> > +static int set_stream(struct snd_pcm_substream *substream,
> > +		      struct sdw_stream_runtime *sdw_stream)
>
> sdw_set_stream() please

it's an internal helper not exported, the choice of not adding a prefix 
was intentional to avoid confusion with exported ones.

> > +{
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct snd_soc_dai *dai;
> > +	int ret = 0;
> > +	int i;
> > +
> > +	/* Set stream pointer on all DAIs */
> > +	for_each_rtd_dais(rtd, i, dai) {
> > +		ret = snd_soc_dai_set_sdw_stream(dai, sdw_stream,
> > +						 substream->stream);
> > +		if (ret < 0) {
> > +			dev_err(rtd->dev,
> > +				"failed to set stream pointer on dai %s",
> > +				dai->name);
>
> lets use one line and shiny new 100 char limit, would make code read
> better!

yes

> > +			break;
>
> So on error should unset of stream pointer be done?

it's already done below, see [1]

> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +int sdw_startup_stream(void *sdw_substream)
>
> Can we have kernel doc style Documentation for exported APIs?

yes, that's a miss indeed.

Though if we follow the existing examples it's not going to be very 
informative, e.g.

/**
 * sdw_disable_stream() - Disable SoundWire stream
 *
 * @stream: Soundwire stream
 *
 * Documentation/driver-api/soundwire/stream.rst explains this API in 
detail
 */

> > +{
> > +	struct snd_pcm_substream *substream = sdw_substream;
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct sdw_stream_runtime *sdw_stream;
> > +	char *name;
> > +	int ret;
> > +
> > +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> > +		name = kasprintf(GFP_KERNEL, "%s-Playback", substream->name);
> > +	else
> > +		name = kasprintf(GFP_KERNEL, "%s-Capture", substream->name);
> > +
> > +	if (!name)
> > +		return -ENOMEM;
> > +
> > +	sdw_stream = sdw_alloc_stream(name);
> > +	if (!sdw_stream) {
> > +		dev_err(rtd->dev, "alloc stream failed for substream DAI %s",
> > +			substream->name);
> > +		ret = -ENOMEM;
> > +		goto error;
> > +	}
> > +
> > +	ret = set_stream(substream, sdw_stream);
> > +	if (ret < 0)
> > +		goto release_stream;

[1]

> > +	return 0;
> > +
> > +release_stream:
> > +	sdw_release_stream(sdw_stream);
> > +	set_stream(substream, NULL);
> > +error:
> > +	kfree(name);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(sdw_startup_stream);