lkml.org 
[lkml]   [2020]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/5] soundwire: stream: add helper to startup/shutdown streams
From
Date
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);

\
 
 \ /
  Last update: 2020-06-30 19:27    [W:0.072 / U:7.900 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site