Messages in this thread | | | Subject | Re: [PATCH 2/5] soundwire: stream: add helper to startup/shutdown streams | From | Pierre-Louis Bossart <> | Date | Tue, 30 Jun 2020 11:58:41 -0500 |
| |
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);
| |