diff mbox

[v4,02/12] ASoC: Intel: mrfld: set private data for cpu-dai

Message ID 1407145563-1303-3-git-send-email-subhransu.s.prusty@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Subhransu S. Prusty Aug. 4, 2014, 9:45 a.m. UTC 
We set the driver private data for media dai so that we can use in media
operations

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/sst-mfld-platform-pcm.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Mark Brown Aug. 13, 2014, 7:45 p.m. UTC | #1 
On Mon, Aug 04, 2014 at 03:15:53PM +0530, Subhransu S. Prusty wrote:

> We set the driver private data for media dai so that we can use in media
> operations

This is another one where making the changelog clearer - as far as I can
tell what this is actually doing is moving the existing private data
from being per platform to being per DAI.  That actually seems OK and I
would've applied this but it looks like it depends on patch 1.

This lack of clarity is a frequent issue with both of these DSP
serieses, it's really slowing down review since they're quite big and
hard to read.

> +static int sst_media_dai_probe(struct snd_soc_dai *cpu_dai)
> +{
> +	struct sst_data *sst = dev_get_drvdata(cpu_dai->dev);
> +
> +	snd_soc_dai_set_drvdata(cpu_dai, sst);
> +	return 0;
> +}
>  static struct snd_soc_dai_driver sst_platform_dai[] = {

Missing blank line here - this is a frequent issue with this code.
Lars-Peter Clausen Aug. 13, 2014, 7:50 p.m. UTC | #2 
[...]
> +static int sst_media_dai_probe(struct snd_soc_dai *cpu_dai)
> +{
> +	struct sst_data *sst = dev_get_drvdata(cpu_dai->dev);
> +
> +	snd_soc_dai_set_drvdata(cpu_dai, sst);

snd_soc_dai_set_drvdata does dev_set_drvdata(dai->dev, data) internally, so 
this function does:

	dev_set_drvdata(cpu_dai->dev, dev_get_drvdata(cpu_dai->dev));

That's pretty much a noop.

> +	return 0;
> +}
>   static struct snd_soc_dai_driver sst_platform_dai[] = {
>   {
> +	.probe = sst_media_dai_probe,
>   	.name = "media-cpu-dai",
>   	.ops = &sst_media_dai_ops,
>   	.playback = {
>
Subhransu S. Prusty Aug. 14, 2014, 5:36 a.m. UTC | #3 
On Wed, Aug 13, 2014 at 09:50:43PM +0200, Lars-Peter Clausen wrote:
> [...]
> >+static int sst_media_dai_probe(struct snd_soc_dai *cpu_dai)
> >+{
> >+	struct sst_data *sst = dev_get_drvdata(cpu_dai->dev);
> >+
> >+	snd_soc_dai_set_drvdata(cpu_dai, sst);
> 
> snd_soc_dai_set_drvdata does dev_set_drvdata(dai->dev, data)
> internally, so this function does:
> 
> 	dev_set_drvdata(cpu_dai->dev, dev_get_drvdata(cpu_dai->dev));
> 
> That's pretty much a noop.
> 

Agree. Intention was to use snd_soc_dai_* API. Anyway will remove and use
dev_get_drvdata(dai->dev) directly.
Lars-Peter Clausen Aug. 14, 2014, 6:16 a.m. UTC | #4 
On 08/14/2014 07:36 AM, Subhransu S. Prusty wrote:
> On Wed, Aug 13, 2014 at 09:50:43PM +0200, Lars-Peter Clausen wrote:
>> [...]
>>> +static int sst_media_dai_probe(struct snd_soc_dai *cpu_dai)
>>> +{
>>> +	struct sst_data *sst = dev_get_drvdata(cpu_dai->dev);
>>> +
>>> +	snd_soc_dai_set_drvdata(cpu_dai, sst);
>>
>> snd_soc_dai_set_drvdata does dev_set_drvdata(dai->dev, data)
>> internally, so this function does:
>>
>> 	dev_set_drvdata(cpu_dai->dev, dev_get_drvdata(cpu_dai->dev));
>>
>> That's pretty much a noop.
>>
>
> Agree. Intention was to use snd_soc_dai_* API. Anyway will remove and use
> dev_get_drvdata(dai->dev) directly.
>

It's totally fine to use snd_soc_dai_get_drvdata() through the driver. It's 
just that this probe function does nothing and can be removed.
Subhransu S. Prusty Aug. 18, 2014, 5:39 a.m. UTC | #5 
On Wed, Aug 13, 2014 at 08:45:31PM +0100, Mark Brown wrote:
> On Mon, Aug 04, 2014 at 03:15:53PM +0530, Subhransu S. Prusty wrote:
> 
> > We set the driver private data for media dai so that we can use in media
> > operations
> 
> This is another one where making the changelog clearer - as far as I can
> tell what this is actually doing is moving the existing private data
> from being per platform to being per DAI.  That actually seems OK and I
> would've applied this but it looks like it depends on patch 1.
> 
Will take care. 
> This lack of clarity is a frequent issue with both of these DSP
> serieses, it's really slowing down review since they're quite big and
> hard to read.
> 
> > +static int sst_media_dai_probe(struct snd_soc_dai *cpu_dai)
> > +{
> > +	struct sst_data *sst = dev_get_drvdata(cpu_dai->dev);
> > +
> > +	snd_soc_dai_set_drvdata(cpu_dai, sst);
> > +	return 0;
> > +}
> >  static struct snd_soc_dai_driver sst_platform_dai[] = {
> 
> Missing blank line here - this is a frequent issue with this code.
diff mbox

Patch

diff --git a/sound/soc/intel/sst-mfld-platform-pcm.c b/sound/soc/intel/sst-mfld-platform-pcm.c
index 364034024cee..821c9970548c 100644
--- a/sound/soc/intel/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/sst-mfld-platform-pcm.c
@@ -223,7 +223,7 @@  int sst_fill_stream_params(void *substream,
 }
 
 static int sst_platform_alloc_stream(struct snd_pcm_substream *substream,
-		struct snd_soc_platform *platform)
+		struct snd_soc_dai *dai)
 {
 	struct sst_runtime_stream *stream =
 			substream->runtime->private_data;
@@ -231,7 +231,7 @@  static int sst_platform_alloc_stream(struct snd_pcm_substream *substream,
 	struct snd_sst_params str_params = {0};
 	struct snd_sst_alloc_params_ext alloc_params = {0};
 	int ret_val = 0;
-	struct sst_data *ctx = snd_soc_platform_get_drvdata(platform);
+	struct sst_data *ctx = snd_soc_dai_get_drvdata(dai);
 
 	/* set codec params and inform SST driver the same */
 	sst_fill_pcm_params(substream, &param);
@@ -347,10 +347,10 @@  static void sst_media_close(struct snd_pcm_substream *substream,
 	kfree(stream);
 }
 
-static inline unsigned int get_current_pipe_id(struct snd_soc_platform *platform,
+static inline unsigned int get_current_pipe_id(struct snd_soc_dai *dai,
 					       struct snd_pcm_substream *substream)
 {
-	struct sst_data *sst = snd_soc_platform_get_drvdata(platform);
+	struct sst_data *sst = snd_soc_dai_get_drvdata(dai);
 	struct sst_dev_stream_map *map = sst->pdata->pdev_strm_map;
 	struct sst_runtime_stream *stream =
 			substream->runtime->private_data;
@@ -376,7 +376,7 @@  static int sst_media_prepare(struct snd_pcm_substream *substream,
 		return ret_val;
 	}
 
-	ret_val = sst_platform_alloc_stream(substream, dai->platform);
+	ret_val = sst_platform_alloc_stream(substream, dai);
 	if (ret_val <= 0)
 		return ret_val;
 	snprintf(substream->pcm->id, sizeof(substream->pcm->id),
@@ -412,8 +412,16 @@  static struct snd_soc_dai_ops sst_media_dai_ops = {
 	.hw_free = sst_media_hw_free,
 };
 
+static int sst_media_dai_probe(struct snd_soc_dai *cpu_dai)
+{
+	struct sst_data *sst = dev_get_drvdata(cpu_dai->dev);
+
+	snd_soc_dai_set_drvdata(cpu_dai, sst);
+	return 0;
+}
 static struct snd_soc_dai_driver sst_platform_dai[] = {
 {
+	.probe = sst_media_dai_probe,
 	.name = "media-cpu-dai",
 	.ops = &sst_media_dai_ops,
 	.playback = {