| Message ID | 20200622212332.16277-3-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: handle stream at the dailink level | expand |
On 23-06-20, 05:23, Bard Liao wrote: > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > To handle streams at the dailink level, expose two helpers that will > be called from machine drivers. > > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> > --- > drivers/soundwire/stream.c | 90 +++++++++++++++++++++++++++++++++++ > include/linux/soundwire/sdw.h | 2 + > 2 files changed, 92 insertions(+) > > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > index a9a72574b34a..48daf4b48f51 100644 > --- 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? > +#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 > +{ > + 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! > + break; So on error should unset of stream pointer be done? > + } > + } > + > + return ret; > +} > + > +int sdw_startup_stream(void *sdw_substream) Can we have kernel doc style Documentation for exported APIs? > +{ > + 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; > + return 0; > + > +release_stream: > + sdw_release_stream(sdw_stream); > + set_stream(substream, NULL); > +error: > + kfree(name); > + return ret; > +} > +EXPORT_SYMBOL(sdw_startup_stream);
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);
On 30-06-20, 11:58, Pierre-Louis Bossart wrote: > > > +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. Yeah but... > > /** > * sdw_disable_stream() - Disable SoundWire stream > * > * @stream: Soundwire stream > * > * Documentation/driver-api/soundwire/stream.rst explains this API in detail it would help to have this pointer. I plan to include the kernel-doc comments in Documentation for sdw so it would great to have these in place
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index a9a72574b34a..48daf4b48f51 100644 --- 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> +#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) +{ + 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); + break; + } + } + + return ret; +} + +int sdw_startup_stream(void *sdw_substream) +{ + 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; + return 0; + +release_stream: + sdw_release_stream(sdw_stream); + set_stream(substream, NULL); +error: + kfree(name); + return ret; +} +EXPORT_SYMBOL(sdw_startup_stream); + +void sdw_shutdown_stream(void *sdw_substream) +{ + struct snd_pcm_substream *substream = sdw_substream; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct sdw_stream_runtime *sdw_stream; + struct snd_soc_dai *dai; + + /* Find stream from first CPU DAI */ + dai = asoc_rtd_to_cpu(rtd, 0); + + sdw_stream = snd_soc_dai_get_sdw_stream(dai, substream->stream); + + if (!sdw_stream) { + dev_err(rtd->dev, "no stream found for DAI %s", dai->name); + return; + } + + /* release memory */ + kfree(sdw_stream->name); + sdw_release_stream(sdw_stream); + + /* clear DAI data */ + set_stream(substream, NULL); +} +EXPORT_SYMBOL(sdw_shutdown_stream); diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 9c27a32df9bb..51ecbd8faa8c 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -952,10 +952,12 @@ int sdw_stream_remove_master(struct sdw_bus *bus, struct sdw_stream_runtime *stream); int sdw_stream_remove_slave(struct sdw_slave *slave, struct sdw_stream_runtime *stream); +int sdw_startup_stream(void *sdw_substream); int sdw_prepare_stream(struct sdw_stream_runtime *stream); int sdw_enable_stream(struct sdw_stream_runtime *stream); int sdw_disable_stream(struct sdw_stream_runtime *stream); int sdw_deprepare_stream(struct sdw_stream_runtime *stream); +void sdw_shutdown_stream(void *sdw_substream); int sdw_bus_prep_clk_stop(struct sdw_bus *bus); int sdw_bus_clk_stop(struct sdw_bus *bus); int sdw_bus_exit_clk_stop(struct sdw_bus *bus);