diff mbox series

[v2,3/5] ASoC: core: add support to snd_soc_dai_get_sdw_stream()

Message ID 20190813083550.5877-4-srinivas.kandagatla@linaro.org (mailing list archive)
State New, archived
Headers show
Series soundwire: Add support to Qualcomm SoundWire master | expand

Commit Message

Srinivas Kandagatla Aug. 13, 2019, 8:35 a.m. UTC
On platforms which have smart speaker amplifiers connected via
soundwire and modeled as aux devices in ASoC, in such usecases machine
driver should be able to get sdw master stream from dai so that it can
use the runtime stream to setup slave streams.

soundwire already as a set function, get function would provide more
flexibility to above configurations.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/sound/soc-dai.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Pierre-Louis Bossart Aug. 13, 2019, 2:44 p.m. UTC | #1
On 8/13/19 3:35 AM, Srinivas Kandagatla wrote:
> On platforms which have smart speaker amplifiers connected via
> soundwire and modeled as aux devices in ASoC, in such usecases machine
> driver should be able to get sdw master stream from dai so that it can
> use the runtime stream to setup slave streams.

using the _set_sdw_stream? I don't fully get the sequence with the 
wording above.

> 
> soundwire already as a set function, get function would provide more
> flexibility to above configurations.

I am not clear if you are asking for both to be used, or get only or set 
only?

> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   include/sound/soc-dai.h | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index dc48fe081a20..1e01f4a302e0 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -202,6 +202,7 @@ struct snd_soc_dai_ops {
>   
>   	int (*set_sdw_stream)(struct snd_soc_dai *dai,
>   			void *stream, int direction);
> +	void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
>   	/*
>   	 * DAI digital mute - optional.
>   	 * Called by soc-core to minimise any pops.
> @@ -410,4 +411,13 @@ static inline int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
>   		return -ENOTSUPP;
>   }
>   
> +static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai,
> +					       int direction)
> +{
> +	if (dai->driver->ops->get_sdw_stream)
> +		return dai->driver->ops->get_sdw_stream(dai, direction);
> +	else
> +		return ERR_PTR(-ENOTSUPP);
> +}
> +
>   #endif
>
Cezary Rojewski Aug. 13, 2019, 4:03 p.m. UTC | #2
On 2019-08-13 10:35, Srinivas Kandagatla wrote:
> On platforms which have smart speaker amplifiers connected via
> soundwire and modeled as aux devices in ASoC, in such usecases machine
> driver should be able to get sdw master stream from dai so that it can
> use the runtime stream to setup slave streams.
> 
> soundwire already as a set function, get function would provide more
> flexibility to above configurations.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   include/sound/soc-dai.h | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index dc48fe081a20..1e01f4a302e0 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -202,6 +202,7 @@ struct snd_soc_dai_ops {
>   
>   	int (*set_sdw_stream)(struct snd_soc_dai *dai,
>   			void *stream, int direction);
> +	void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
>   	/*
>   	 * DAI digital mute - optional.
>   	 * Called by soc-core to minimise any pops.
> @@ -410,4 +411,13 @@ static inline int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
>   		return -ENOTSUPP;
>   }
>   
> +static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai,
> +					       int direction)
> +{
> +	if (dai->driver->ops->get_sdw_stream)
> +		return dai->driver->ops->get_sdw_stream(dai, direction);
> +	else
> +		return ERR_PTR(-ENOTSUPP);
> +}

Drop redundant else.
Srinivas Kandagatla Aug. 13, 2019, 4:50 p.m. UTC | #3
Thanks for the review,

On 13/08/2019 15:44, Pierre-Louis Bossart wrote:
> On 8/13/19 3:35 AM, Srinivas Kandagatla wrote:
>> On platforms which have smart speaker amplifiers connected via
>> soundwire and modeled as aux devices in ASoC, in such usecases machine
>> driver should be able to get sdw master stream from dai so that it can
>> use the runtime stream to setup slave streams.
> 
> using the _set_sdw_stream? I don't fully get the sequence with the 
> wording above.

Yes, using set_sdw_stream().

> 
>>
>> soundwire already as a set function, get function would provide more
>> flexibility to above configurations.
> 
> I am not clear if you are asking for both to be used, or get only or set 
> only?

It depends on the usecase, in db845c usecase  [1] as Aux device is dai 
less, machine driver is using get function to get hold of master stream 
so that it can setup slave port config.


Looks like there is a typo in above like

This was supposed to be "soundwire already has a set function, get 
function would provide more flexibility to above configurations"

[1] 
https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/tree/sound/soc/qcom/db845c.c?h=integration-linux-qcomlt

thanks,
srini

> 
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   include/sound/soc-dai.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
>> index dc48fe081a20..1e01f4a302e0 100644
>> --- a/include/sound/soc-dai.h
>> +++ b/include/sound/soc-dai.h
>> @@ -202,6 +202,7 @@ struct snd_soc_dai_ops {
>>       int (*set_sdw_stream)(struct snd_soc_dai *dai,
>>               void *stream, int direction);
>> +    void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
>>       /*
>>        * DAI digital mute - optional.
>>        * Called by soc-core to minimise any pops.
>> @@ -410,4 +411,13 @@ static inline int 
>> snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
>>           return -ENOTSUPP;
>>   }
>> +static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai,
>> +                           int direction)
>> +{
>> +    if (dai->driver->ops->get_sdw_stream)
>> +        return dai->driver->ops->get_sdw_stream(dai, direction);
>> +    else
>> +        return ERR_PTR(-ENOTSUPP);
>> +}
>> +
>>   #endif
>>
>
Srinivas Kandagatla Aug. 13, 2019, 4:52 p.m. UTC | #4
Thanks for the review,

On 13/08/2019 17:03, Cezary Rojewski wrote:
> On 2019-08-13 10:35, Srinivas Kandagatla wrote:
>> On platforms which have smart speaker amplifiers connected via
>> soundwire and modeled as aux devices in ASoC, in such usecases machine
>> driver should be able to get sdw master stream from dai so that it can
>> use the runtime stream to setup slave streams.
>>
>> soundwire already as a set function, get function would provide more
>> flexibility to above configurations.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   include/sound/soc-dai.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
>> index dc48fe081a20..1e01f4a302e0 100644
>> --- a/include/sound/soc-dai.h
>> +++ b/include/sound/soc-dai.h
>> @@ -202,6 +202,7 @@ struct snd_soc_dai_ops {
>>       int (*set_sdw_stream)(struct snd_soc_dai *dai,
>>               void *stream, int direction);
>> +    void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
>>       /*
>>        * DAI digital mute - optional.
>>        * Called by soc-core to minimise any pops.
>> @@ -410,4 +411,13 @@ static inline int 
>> snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
>>           return -ENOTSUPP;
>>   }
>> +static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai,
>> +                           int direction)
>> +{
>> +    if (dai->driver->ops->get_sdw_stream)
>> +        return dai->driver->ops->get_sdw_stream(dai, direction);
>> +    else
>> +        return ERR_PTR(-ENOTSUPP);
>> +}
> 
> Drop redundant else.
Not all the dai drivers would implement this function, I guess else is 
not redundant here!

--srini
>
Cezary Rojewski Aug. 13, 2019, 5:29 p.m. UTC | #5
On 2019-08-13 18:52, Srinivas Kandagatla wrote:
> Thanks for the review,
> 
> On 13/08/2019 17:03, Cezary Rojewski wrote:
>> On 2019-08-13 10:35, Srinivas Kandagatla wrote:
>>> On platforms which have smart speaker amplifiers connected via
>>> soundwire and modeled as aux devices in ASoC, in such usecases machine
>>> driver should be able to get sdw master stream from dai so that it can
>>> use the runtime stream to setup slave streams.
>>>
>>> soundwire already as a set function, get function would provide more
>>> flexibility to above configurations.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> ---

>>> +static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai,
>>> +                           int direction)
>>> +{
>>> +    if (dai->driver->ops->get_sdw_stream)
>>> +        return dai->driver->ops->get_sdw_stream(dai, direction);
>>> +    else
>>> +        return ERR_PTR(-ENOTSUPP);
>>> +}
>>
>> Drop redundant else.
> Not all the dai drivers would implement this function, I guess else is 
> not redundant here!
> 
> --srini
>>

Eh. By that I meant dropping "else" keyword and reducing indentation for 
"return ERR_PTR(-ENOTSUPP);"

Czarek
Pierre-Louis Bossart Aug. 13, 2019, 5:51 p.m. UTC | #6
On 8/13/19 11:50 AM, Srinivas Kandagatla wrote:
> Thanks for the review,
> 
> On 13/08/2019 15:44, Pierre-Louis Bossart wrote:
>> On 8/13/19 3:35 AM, Srinivas Kandagatla wrote:
>>> On platforms which have smart speaker amplifiers connected via
>>> soundwire and modeled as aux devices in ASoC, in such usecases machine
>>> driver should be able to get sdw master stream from dai so that it can
>>> use the runtime stream to setup slave streams.
>>
>> using the _set_sdw_stream? I don't fully get the sequence with the 
>> wording above.
> 
> Yes, using set_sdw_stream().

Maybe I am missing something here, but I don't see where the 
set_sdw_stream() is called.

Also I don't fully get the rule. set_sdw_stream() looks required, 
get_sdw_stream() is optional, is this what you are suggesting?

>>
>>>
>>> soundwire already as a set function, get function would provide more
>>> flexibility to above configurations.
>>
>> I am not clear if you are asking for both to be used, or get only or 
>> set only?
> 
> It depends on the usecase, in db845c usecase  [1] as Aux device is dai 
> less, machine driver is using get function to get hold of master stream 
> so that it can setup slave port config.
> 
> 
> Looks like there is a typo in above like
> 
> This was supposed to be "soundwire already has a set function, get 
> function would provide more flexibility to above configurations"
> 
> [1] 
> https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/tree/sound/soc/qcom/db845c.c?h=integration-linux-qcomlt 
> 
> 
> thanks,
> srini
> 
>>
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> ---
>>>   include/sound/soc-dai.h | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
>>> index dc48fe081a20..1e01f4a302e0 100644
>>> --- a/include/sound/soc-dai.h
>>> +++ b/include/sound/soc-dai.h
>>> @@ -202,6 +202,7 @@ struct snd_soc_dai_ops {
>>>       int (*set_sdw_stream)(struct snd_soc_dai *dai,
>>>               void *stream, int direction);
>>> +    void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
>>>       /*
>>>        * DAI digital mute - optional.
>>>        * Called by soc-core to minimise any pops.
>>> @@ -410,4 +411,13 @@ static inline int 
>>> snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
>>>           return -ENOTSUPP;
>>>   }
>>> +static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai,
>>> +                           int direction)
>>> +{
>>> +    if (dai->driver->ops->get_sdw_stream)
>>> +        return dai->driver->ops->get_sdw_stream(dai, direction);
>>> +    else
>>> +        return ERR_PTR(-ENOTSUPP);
>>> +}
>>> +
>>>   #endif
>>>
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Srinivas Kandagatla Aug. 13, 2019, 6:06 p.m. UTC | #7
On 13/08/2019 18:51, Pierre-Louis Bossart wrote:
> On 8/13/19 11:50 AM, Srinivas Kandagatla wrote:
>> Thanks for the review,
>>
>> On 13/08/2019 15:44, Pierre-Louis Bossart wrote:
>>> On 8/13/19 3:35 AM, Srinivas Kandagatla wrote:
>>>> On platforms which have smart speaker amplifiers connected via
>>>> soundwire and modeled as aux devices in ASoC, in such usecases machine
>>>> driver should be able to get sdw master stream from dai so that it can
>>>> use the runtime stream to setup slave streams.
>>>
>>> using the _set_sdw_stream? I don't fully get the sequence with the 
>>> wording above.
>>
>> Yes, using set_sdw_stream().
> 
> Maybe I am missing something here, but I don't see where the 
> set_sdw_stream() is called.

sorry for the confusion. It was too quick reply. :-)
I was suppose to say sdw_stream_add_slave() instead of set_sdw_stream().

As Aux device is dailess there is no way to get hold of sdw stream 
runtime for slave device associated with it.

Having snd_soc_dai_get_sdw_stream() would help machine driver to get 
hold of sdw_stream_runtime from controller dai and setup slave streams 
using sdw_stream_add_slave().


thanks,
srini


> 
> Also I don't fully get the rule. set_sdw_stream() looks required, 
> get_sdw_stream() is optional, is this what you are suggesting?
> 
>>>
>>>>
>>>> soundwire already as a set function, get function would provide more
>>>> flexibility to above configurations.
>>>
>>> I am not clear if you are asking for both to be used, or get only or 
>>> set only?
>>
>> It depends on the usecase, in db845c usecase  [1] as Aux device is dai 
>> less, machine driver is using get function to get hold of master 
>> stream so that it can setup slave port config.
>>
>>
>> Looks like there is a typo in above like
>>
>> This was supposed to be "soundwire already has a set function, get 
>> function would provide more flexibility to above configurations"
>>
>> [1] 
>> https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/tree/sound/soc/qcom/db845c.c?h=integration-linux-qcomlt 
>>
>>
>> thanks,
>> srini
>>
>>>
>>>>
>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>> ---
>>>>   include/sound/soc-dai.h | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
>>>> index dc48fe081a20..1e01f4a302e0 100644
>>>> --- a/include/sound/soc-dai.h
>>>> +++ b/include/sound/soc-dai.h
>>>> @@ -202,6 +202,7 @@ struct snd_soc_dai_ops {
>>>>       int (*set_sdw_stream)(struct snd_soc_dai *dai,
>>>>               void *stream, int direction);
>>>> +    void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
>>>>       /*
>>>>        * DAI digital mute - optional.
>>>>        * Called by soc-core to minimise any pops.
>>>> @@ -410,4 +411,13 @@ static inline int 
>>>> snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
>>>>           return -ENOTSUPP;
>>>>   }
>>>> +static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai 
>>>> *dai,
>>>> +                           int direction)
>>>> +{
>>>> +    if (dai->driver->ops->get_sdw_stream)
>>>> +        return dai->driver->ops->get_sdw_stream(dai, direction);
>>>> +    else
>>>> +        return ERR_PTR(-ENOTSUPP);
>>>> +}
>>>> +
>>>>   #endif
>>>>
>>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
Pierre-Louis Bossart Aug. 13, 2019, 7:15 p.m. UTC | #8
On 8/13/19 1:06 PM, Srinivas Kandagatla wrote:
> 
> 
> On 13/08/2019 18:51, Pierre-Louis Bossart wrote:
>> On 8/13/19 11:50 AM, Srinivas Kandagatla wrote:
>>> Thanks for the review,
>>>
>>> On 13/08/2019 15:44, Pierre-Louis Bossart wrote:
>>>> On 8/13/19 3:35 AM, Srinivas Kandagatla wrote:
>>>>> On platforms which have smart speaker amplifiers connected via
>>>>> soundwire and modeled as aux devices in ASoC, in such usecases machine
>>>>> driver should be able to get sdw master stream from dai so that it can
>>>>> use the runtime stream to setup slave streams.
>>>>
>>>> using the _set_sdw_stream? I don't fully get the sequence with the 
>>>> wording above.
>>>
>>> Yes, using set_sdw_stream().
>>
>> Maybe I am missing something here, but I don't see where the 
>> set_sdw_stream() is called.
> 
> sorry for the confusion. It was too quick reply. :-)
> I was suppose to say sdw_stream_add_slave() instead of set_sdw_stream().

ok, so get_sdw_stream() and set_sdw_stream() are not meant to be mirrors 
or both implemented. It's just a helper to respectively get a context or 
set a context but a get-modify-set type of operation is not expected.

Do I get this right?

> 
> As Aux device is dailess there is no way to get hold of sdw stream 
> runtime for slave device associated with it.
> 
> Having snd_soc_dai_get_sdw_stream() would help machine driver to get 
> hold of sdw_stream_runtime from controller dai and setup slave streams 
> using sdw_stream_add_slave().
> 
> 
> thanks,
> srini
> 
> 
>>
>> Also I don't fully get the rule. set_sdw_stream() looks required, 
>> get_sdw_stream() is optional, is this what you are suggesting?
>>
>>>>
>>>>>
>>>>> soundwire already as a set function, get function would provide more
>>>>> flexibility to above configurations.
>>>>
>>>> I am not clear if you are asking for both to be used, or get only or 
>>>> set only?
>>>
>>> It depends on the usecase, in db845c usecase  [1] as Aux device is 
>>> dai less, machine driver is using get function to get hold of master 
>>> stream so that it can setup slave port config.
>>>
>>>
>>> Looks like there is a typo in above like
>>>
>>> This was supposed to be "soundwire already has a set function, get 
>>> function would provide more flexibility to above configurations"
>>>
>>> [1] 
>>> https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/tree/sound/soc/qcom/db845c.c?h=integration-linux-qcomlt 
>>>
>>>
>>> thanks,
>>> srini
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>>> ---
>>>>>   include/sound/soc-dai.h | 10 ++++++++++
>>>>>   1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
>>>>> index dc48fe081a20..1e01f4a302e0 100644
>>>>> --- a/include/sound/soc-dai.h
>>>>> +++ b/include/sound/soc-dai.h
>>>>> @@ -202,6 +202,7 @@ struct snd_soc_dai_ops {
>>>>>       int (*set_sdw_stream)(struct snd_soc_dai *dai,
>>>>>               void *stream, int direction);
>>>>> +    void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
>>>>>       /*
>>>>>        * DAI digital mute - optional.
>>>>>        * Called by soc-core to minimise any pops.
>>>>> @@ -410,4 +411,13 @@ static inline int 
>>>>> snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
>>>>>           return -ENOTSUPP;
>>>>>   }
>>>>> +static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai 
>>>>> *dai,
>>>>> +                           int direction)
>>>>> +{
>>>>> +    if (dai->driver->ops->get_sdw_stream)
>>>>> +        return dai->driver->ops->get_sdw_stream(dai, direction);
>>>>> +    else
>>>>> +        return ERR_PTR(-ENOTSUPP);
>>>>> +}
>>>>> +
>>>>>   #endif
>>>>>
>>>>
>>> _______________________________________________
>>> Alsa-devel mailing list
>>> Alsa-devel@alsa-project.org
>>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>
Mark Brown Aug. 13, 2019, 7:18 p.m. UTC | #9
On Tue, Aug 13, 2019 at 02:15:18PM -0500, Pierre-Louis Bossart wrote:
> On 8/13/19 1:06 PM, Srinivas Kandagatla wrote:

> > sorry for the confusion. It was too quick reply. :-)
> > I was suppose to say sdw_stream_add_slave() instead of set_sdw_stream().

> ok, so get_sdw_stream() and set_sdw_stream() are not meant to be mirrors or
> both implemented. It's just a helper to respectively get a context or set a
> context but a get-modify-set type of operation is not expected.

> Do I get this right?

This seems like it's going to be confusing...
Mark Brown Aug. 13, 2019, 7:19 p.m. UTC | #10
On Tue, Aug 13, 2019 at 07:29:50PM +0200, Cezary Rojewski wrote:
> On 2019-08-13 18:52, Srinivas Kandagatla wrote:
> > On 13/08/2019 17:03, Cezary Rojewski wrote:
> > > On 2019-08-13 10:35, Srinivas Kandagatla wrote:

> > > > +    if (dai->driver->ops->get_sdw_stream)
> > > > +        return dai->driver->ops->get_sdw_stream(dai, direction);
> > > > +    else
> > > > +        return ERR_PTR(-ENOTSUPP);

> > > Drop redundant else.

> > Not all the dai drivers would implement this function, I guess else is
> > not redundant here!

> Eh. By that I meant dropping "else" keyword and reducing indentation for
> "return ERR_PTR(-ENOTSUPP);"

The above is the idiom used throughout the rest of the file.
Pierre-Louis Bossart Aug. 13, 2019, 7:38 p.m. UTC | #11
On 8/13/19 2:18 PM, Mark Brown wrote:
> On Tue, Aug 13, 2019 at 02:15:18PM -0500, Pierre-Louis Bossart wrote:
>> On 8/13/19 1:06 PM, Srinivas Kandagatla wrote:
> 
>>> sorry for the confusion. It was too quick reply. :-)
>>> I was suppose to say sdw_stream_add_slave() instead of set_sdw_stream().
> 
>> ok, so get_sdw_stream() and set_sdw_stream() are not meant to be mirrors or
>> both implemented. It's just a helper to respectively get a context or set a
>> context but a get-modify-set type of operation is not expected.
> 
>> Do I get this right?
> 
> This seems like it's going to be confusing...

Indeed. I don't have a full understanding of that part to be honest, nor 
why we need something SoundWire-specific. We already abused the 
set_tdm_slot API to store an HDaudio stream, now we have a rather 
confusing stream information for SoundWire and I have about 3 other 
'stream' contexts in SOF... I am still doing basic cleanups but this has 
been on my radar for a while.
Mark Brown Aug. 13, 2019, 7:58 p.m. UTC | #12
On Tue, Aug 13, 2019 at 02:38:53PM -0500, Pierre-Louis Bossart wrote:

> Indeed. I don't have a full understanding of that part to be honest, nor why
> we need something SoundWire-specific. We already abused the set_tdm_slot API
> to store an HDaudio stream, now we have a rather confusing stream
> information for SoundWire and I have about 3 other 'stream' contexts in
> SOF... I am still doing basic cleanups but this has been on my radar for a
> while.

There is something to be said for not abusing the TDM slot API if it can
make things clearer by using bus-idiomatic mechanisms, but it does mean
everything needs to know about each individual bus :/ .
Vinod Koul Aug. 14, 2019, 4:11 a.m. UTC | #13
On 13-08-19, 20:58, Mark Brown wrote:
> On Tue, Aug 13, 2019 at 02:38:53PM -0500, Pierre-Louis Bossart wrote:
> 
> > Indeed. I don't have a full understanding of that part to be honest, nor why
> > we need something SoundWire-specific. We already abused the set_tdm_slot API
> > to store an HDaudio stream, now we have a rather confusing stream
> > information for SoundWire and I have about 3 other 'stream' contexts in
> > SOF... I am still doing basic cleanups but this has been on my radar for a
> > while.
> 
> There is something to be said for not abusing the TDM slot API if it can
> make things clearer by using bus-idiomatic mechanisms, but it does mean
> everything needs to know about each individual bus :/ .

Here ASoC doesn't need to know about sdw bus. As Srini explained, this
helps in the case for him to get the stream context and set the stream
context from the machine driver.

Nothing else is expected to be done from this API. We already do a set
using snd_soc_dai_set_sdw_stream(). Here we add the snd_soc_dai_get_sdw_stream() to query

Thanks
Mark Brown Aug. 14, 2019, 9:08 a.m. UTC | #14
On Wed, Aug 14, 2019 at 09:41:42AM +0530, Vinod Koul wrote:
> On 13-08-19, 20:58, Mark Brown wrote:

> > There is something to be said for not abusing the TDM slot API if it can
> > make things clearer by using bus-idiomatic mechanisms, but it does mean
> > everything needs to know about each individual bus :/ .

> Here ASoC doesn't need to know about sdw bus. As Srini explained, this
> helps in the case for him to get the stream context and set the stream
> context from the machine driver.

Other drivers interoperating with the Soundwire DAI might want to do
something, it looks like that's the case for SOF.

> Nothing else is expected to be done from this API. We already do a set
> using snd_soc_dai_set_sdw_stream(). Here we add the snd_soc_dai_get_sdw_stream() to query

Well, if the API is not expected to do anything we can optimize it and
just remove it!
Pierre-Louis Bossart Aug. 14, 2019, 2:09 p.m. UTC | #15
On 8/13/19 11:11 PM, Vinod Koul wrote:
> On 13-08-19, 20:58, Mark Brown wrote:
>> On Tue, Aug 13, 2019 at 02:38:53PM -0500, Pierre-Louis Bossart wrote:
>>
>>> Indeed. I don't have a full understanding of that part to be honest, nor why
>>> we need something SoundWire-specific. We already abused the set_tdm_slot API
>>> to store an HDaudio stream, now we have a rather confusing stream
>>> information for SoundWire and I have about 3 other 'stream' contexts in
>>> SOF... I am still doing basic cleanups but this has been on my radar for a
>>> while.
>>
>> There is something to be said for not abusing the TDM slot API if it can
>> make things clearer by using bus-idiomatic mechanisms, but it does mean
>> everything needs to know about each individual bus :/ .
> 
> Here ASoC doesn't need to know about sdw bus. As Srini explained, this
> helps in the case for him to get the stream context and set the stream
> context from the machine driver.
> 
> Nothing else is expected to be done from this API. We already do a set
> using snd_soc_dai_set_sdw_stream(). Here we add the snd_soc_dai_get_sdw_stream() to query

I didn't see a call to snd_soc_dai_set_sdw_stream() in Srini's code?
Srinivas Kandagatla Oct. 9, 2019, 8:32 a.m. UTC | #16
Hi Pierre,

On 14/08/2019 15:09, Pierre-Louis Bossart wrote:
> 
> 
> On 8/13/19 11:11 PM, Vinod Koul wrote:
>> On 13-08-19, 20:58, Mark Brown wrote:
>>> On Tue, Aug 13, 2019 at 02:38:53PM -0500, Pierre-Louis Bossart wrote:
>>>
>>>> Indeed. I don't have a full understanding of that part to be honest, 
>>>> nor why
>>>> we need something SoundWire-specific. We already abused the 
>>>> set_tdm_slot API
>>>> to store an HDaudio stream, now we have a rather confusing stream
>>>> information for SoundWire and I have about 3 other 'stream' contexts in
>>>> SOF... I am still doing basic cleanups but this has been on my radar 
>>>> for a
>>>> while.
>>>
>>> There is something to be said for not abusing the TDM slot API if it can
>>> make things clearer by using bus-idiomatic mechanisms, but it does mean
>>> everything needs to know about each individual bus :/ .
>>
>> Here ASoC doesn't need to know about sdw bus. As Srini explained, this
>> helps in the case for him to get the stream context and set the stream
>> context from the machine driver.
>>
>> Nothing else is expected to be done from this API. We already do a set
>> using snd_soc_dai_set_sdw_stream(). Here we add the 
>> snd_soc_dai_get_sdw_stream() to query
> 
> I didn't see a call to snd_soc_dai_set_sdw_stream() in Srini's code?


There is a snd_soc_dai_get_sdw_stream() to get stream context and we add 
slave streams(amplifier in this case) to that context using 
sdw_stream_add_slave() in machine driver[1].

Without this helper there is no way to link slave streams to stream 
context in non dai based setup like smart speaker amplifiers.

Currently this driver is blocked on this patch, If you think there are 
other ways to do this, am happy to try them out.

Thanks,
srini

[1] 
https://git.linaro.org/people/srinivas.kandagatla/linux.git/tree/sound/soc/qcom/db845c.c?h=release/db845c/qcomlt-5.2
Pierre-Louis Bossart Oct. 9, 2019, 2:29 p.m. UTC | #17
On 10/9/19 3:32 AM, Srinivas Kandagatla wrote:
> Hi Pierre,
> 
> On 14/08/2019 15:09, Pierre-Louis Bossart wrote:
>>
>>
>> On 8/13/19 11:11 PM, Vinod Koul wrote:
>>> On 13-08-19, 20:58, Mark Brown wrote:
>>>> On Tue, Aug 13, 2019 at 02:38:53PM -0500, Pierre-Louis Bossart wrote:
>>>>
>>>>> Indeed. I don't have a full understanding of that part to be 
>>>>> honest, nor why
>>>>> we need something SoundWire-specific. We already abused the 
>>>>> set_tdm_slot API
>>>>> to store an HDaudio stream, now we have a rather confusing stream
>>>>> information for SoundWire and I have about 3 other 'stream' 
>>>>> contexts in
>>>>> SOF... I am still doing basic cleanups but this has been on my 
>>>>> radar for a
>>>>> while.
>>>>
>>>> There is something to be said for not abusing the TDM slot API if it 
>>>> can
>>>> make things clearer by using bus-idiomatic mechanisms, but it does mean
>>>> everything needs to know about each individual bus :/ .
>>>
>>> Here ASoC doesn't need to know about sdw bus. As Srini explained, this
>>> helps in the case for him to get the stream context and set the stream
>>> context from the machine driver.
>>>
>>> Nothing else is expected to be done from this API. We already do a set
>>> using snd_soc_dai_set_sdw_stream(). Here we add the 
>>> snd_soc_dai_get_sdw_stream() to query
>>
>> I didn't see a call to snd_soc_dai_set_sdw_stream() in Srini's code?
> 
> 
> There is a snd_soc_dai_get_sdw_stream() to get stream context and we add 
> slave streams(amplifier in this case) to that context using 
> sdw_stream_add_slave() in machine driver[1].
> 
> Without this helper there is no way to link slave streams to stream 
> context in non dai based setup like smart speaker amplifiers.
> 
> Currently this driver is blocked on this patch, If you think there are 
> other ways to do this, am happy to try them out.

So to be clear, you are *not* using snd_soc_dai_set_sdw_stream?
Srinivas Kandagatla Oct. 9, 2019, 4:01 p.m. UTC | #18
On 09/10/2019 15:29, Pierre-Louis Bossart wrote:
> 
> 
> On 10/9/19 3:32 AM, Srinivas Kandagatla wrote:
>> Hi Pierre,
>>
>> On 14/08/2019 15:09, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 8/13/19 11:11 PM, Vinod Koul wrote:
>>>> On 13-08-19, 20:58, Mark Brown wrote:
>>>>> On Tue, Aug 13, 2019 at 02:38:53PM -0500, Pierre-Louis Bossart wrote:
>>>>>
>>>>>> Indeed. I don't have a full understanding of that part to be 
>>>>>> honest, nor why
>>>>>> we need something SoundWire-specific. We already abused the 
>>>>>> set_tdm_slot API
>>>>>> to store an HDaudio stream, now we have a rather confusing stream
>>>>>> information for SoundWire and I have about 3 other 'stream' 
>>>>>> contexts in
>>>>>> SOF... I am still doing basic cleanups but this has been on my 
>>>>>> radar for a
>>>>>> while.
>>>>>
>>>>> There is something to be said for not abusing the TDM slot API if 
>>>>> it can
>>>>> make things clearer by using bus-idiomatic mechanisms, but it does 
>>>>> mean
>>>>> everything needs to know about each individual bus :/ .
>>>>
>>>> Here ASoC doesn't need to know about sdw bus. As Srini explained, this
>>>> helps in the case for him to get the stream context and set the stream
>>>> context from the machine driver.
>>>>
>>>> Nothing else is expected to be done from this API. We already do a set
>>>> using snd_soc_dai_set_sdw_stream(). Here we add the 
>>>> snd_soc_dai_get_sdw_stream() to query
>>>
>>> I didn't see a call to snd_soc_dai_set_sdw_stream() in Srini's code?
>>
>>
>> There is a snd_soc_dai_get_sdw_stream() to get stream context and we 
>> add slave streams(amplifier in this case) to that context using 
>> sdw_stream_add_slave() in machine driver[1].
>>
>> Without this helper there is no way to link slave streams to stream 
>> context in non dai based setup like smart speaker amplifiers.
>>
>> Currently this driver is blocked on this patch, If you think there are 
>> other ways to do this, am happy to try them out.
> 
> So to be clear, you are *not* using snd_soc_dai_set_sdw_stream?
Yes, am not using snd_soc_dai_set_sdw_stream().

--srini
> 
> 
> 
> 
>
Pierre-Louis Bossart Oct. 9, 2019, 6:53 p.m. UTC | #19
On 10/9/19 11:01 AM, Srinivas Kandagatla wrote:
> 
> 
> On 09/10/2019 15:29, Pierre-Louis Bossart wrote:
>>
>>
>> On 10/9/19 3:32 AM, Srinivas Kandagatla wrote:
>>> Hi Pierre,
>>>
>>> On 14/08/2019 15:09, Pierre-Louis Bossart wrote:
>>>>
>>>>
>>>> On 8/13/19 11:11 PM, Vinod Koul wrote:
>>>>> On 13-08-19, 20:58, Mark Brown wrote:
>>>>>> On Tue, Aug 13, 2019 at 02:38:53PM -0500, Pierre-Louis Bossart wrote:
>>>>>>
>>>>>>> Indeed. I don't have a full understanding of that part to be 
>>>>>>> honest, nor why
>>>>>>> we need something SoundWire-specific. We already abused the 
>>>>>>> set_tdm_slot API
>>>>>>> to store an HDaudio stream, now we have a rather confusing stream
>>>>>>> information for SoundWire and I have about 3 other 'stream' 
>>>>>>> contexts in
>>>>>>> SOF... I am still doing basic cleanups but this has been on my 
>>>>>>> radar for a
>>>>>>> while.
>>>>>>
>>>>>> There is something to be said for not abusing the TDM slot API if 
>>>>>> it can
>>>>>> make things clearer by using bus-idiomatic mechanisms, but it does 
>>>>>> mean
>>>>>> everything needs to know about each individual bus :/ .
>>>>>
>>>>> Here ASoC doesn't need to know about sdw bus. As Srini explained, this
>>>>> helps in the case for him to get the stream context and set the stream
>>>>> context from the machine driver.
>>>>>
>>>>> Nothing else is expected to be done from this API. We already do a set
>>>>> using snd_soc_dai_set_sdw_stream(). Here we add the 
>>>>> snd_soc_dai_get_sdw_stream() to query
>>>>
>>>> I didn't see a call to snd_soc_dai_set_sdw_stream() in Srini's code?
>>>
>>>
>>> There is a snd_soc_dai_get_sdw_stream() to get stream context and we 
>>> add slave streams(amplifier in this case) to that context using 
>>> sdw_stream_add_slave() in machine driver[1].
>>>
>>> Without this helper there is no way to link slave streams to stream 
>>> context in non dai based setup like smart speaker amplifiers.
>>>
>>> Currently this driver is blocked on this patch, If you think there 
>>> are other ways to do this, am happy to try them out.
>>
>> So to be clear, you are *not* using snd_soc_dai_set_sdw_stream?
> Yes, am not using snd_soc_dai_set_sdw_stream().

It's been a while since this thread started, and I still don't quite get 
the concepts or logic.

First, I don't understand what the problem with "aux" devices is. All 
the SoundWire stuff is based on the concept of DAI, so I guess I am 
missing why introducing the "aux" device changes anything.

Next, a 'stream' connects multiple DAIs to transmit information from 
sources to sinks. A DAI in itself is created without having any notion 
of which stream it will be associated with. This can only be done with 
machine level information.

If you query a DAI to get a stream context, then how is this stream 
context allocated in the first place? It looks like a horse and cart 
problem. Or you are assuming that a specific DAI provides the context, 
and that all other DAIs do not expose this .get_sdw_stream()? What if 
more that 1 DAI provide a stream context?

And last, I am even more lost since w/ the existing codec drivers we 
have, sdw_stream_add_slave() is called from the dai .hw_params callback, 
once you have information on formats/rates, etc. using 
sdw_stream_add_slave() from a machine driver looks like an even bigger 
stretch. I really thought the machine driver would only propagate the 
notion of stream to all DAIs that are part of the same dailink.

I am not trying to block the Qualcomm implementation, just would like to 
make sure the assumptions are clear and that we are not using the same 
API in completely different ways.
Srinivas Kandagatla Oct. 10, 2019, 8:50 a.m. UTC | #20
On 09/10/2019 19:53, Pierre-Louis Bossart wrote:
> 
> 
> On 10/9/19 11:01 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 09/10/2019 15:29, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 10/9/19 3:32 AM, Srinivas Kandagatla wrote:
>>>> Hi Pierre,
>>>>
>>>> On 14/08/2019 15:09, Pierre-Louis Bossart wrote:
>>>>>
>>>>>
>>>>> On 8/13/19 11:11 PM, Vinod Koul wrote:
>>>>>> On 13-08-19, 20:58, Mark Brown wrote:
>>>>>>> On Tue, Aug 13, 2019 at 02:38:53PM -0500, Pierre-Louis Bossart 
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Indeed. I don't have a full understanding of that part to be 
>>>>>>>> honest, nor why
>>>>>>>> we need something SoundWire-specific. We already abused the 
>>>>>>>> set_tdm_slot API
>>>>>>>> to store an HDaudio stream, now we have a rather confusing stream
>>>>>>>> information for SoundWire and I have about 3 other 'stream' 
>>>>>>>> contexts in
>>>>>>>> SOF... I am still doing basic cleanups but this has been on my 
>>>>>>>> radar for a
>>>>>>>> while.
>>>>>>>
>>>>>>> There is something to be said for not abusing the TDM slot API if 
>>>>>>> it can
>>>>>>> make things clearer by using bus-idiomatic mechanisms, but it 
>>>>>>> does mean
>>>>>>> everything needs to know about each individual bus :/ .
>>>>>>
>>>>>> Here ASoC doesn't need to know about sdw bus. As Srini explained, 
>>>>>> this
>>>>>> helps in the case for him to get the stream context and set the 
>>>>>> stream
>>>>>> context from the machine driver.
>>>>>>
>>>>>> Nothing else is expected to be done from this API. We already do a 
>>>>>> set
>>>>>> using snd_soc_dai_set_sdw_stream(). Here we add the 
>>>>>> snd_soc_dai_get_sdw_stream() to query
>>>>>
>>>>> I didn't see a call to snd_soc_dai_set_sdw_stream() in Srini's code?
>>>>
>>>>
>>>> There is a snd_soc_dai_get_sdw_stream() to get stream context and we 
>>>> add slave streams(amplifier in this case) to that context using 
>>>> sdw_stream_add_slave() in machine driver[1].
>>>>
>>>> Without this helper there is no way to link slave streams to stream 
>>>> context in non dai based setup like smart speaker amplifiers.
>>>>
>>>> Currently this driver is blocked on this patch, If you think there 
>>>> are other ways to do this, am happy to try them out.
>>>
>>> So to be clear, you are *not* using snd_soc_dai_set_sdw_stream?
>> Yes, am not using snd_soc_dai_set_sdw_stream().
> 
> It's been a while since this thread started, and I still don't quite get 
> the concepts or logic.
> 
> First, I don't understand what the problem with "aux" devices is. All 
> the SoundWire stuff is based on the concept of DAI, so I guess I am 

That is the actual problem! Some aux devices does not have dais.

> missing why introducing the "aux" device changes anything.
> 

Problem is that aux devices(amplifier) are dai less but connected via 
SoundWire. So question is how do we attach those SoundWire streams to 
SoundWire master stream?

My Idea was to get handle to the SoundWire stream from controller dai 
and adding these aux SoundWire slave devices as slave to them in machine 
driver. This was much less intrusive than other solutions.

Is there a better way to associate a dai-less codecs to SoundWire master 
stream?

> Next, a 'stream' connects multiple DAIs to transmit information from 
> sources to sinks. A DAI in itself is created without having any notion 
> of which stream it will be associated with. This can only be done with 
> machine level information.
> 
> If you query a DAI to get a stream context, then how is this stream 
> context allocated in the first place? It looks like a horse and cart 
> problem. Or you are assuming that a specific DAI provides the context, 
> and that all other DAIs do not expose this .get_sdw_stream()? What if 
> more that 1 DAI provide a stream context?

In this particular board setup. Soundwire master is allocating the 
stream runtime for each of it dais, and this runtime is used in machine 
driver to attach Auxdev Soundwire slaves.

Other setups where the codec has dai should work as expected!


> 
> And last, I am even more lost since w/ the existing codec drivers we 
> have, sdw_stream_add_slave() is called from the dai .hw_params callback, 
> once you have information on formats/rates, etc. using 
> sdw_stream_add_slave() from a machine driver looks like an even bigger 
> stretch. I really thought the machine driver would only propagate the 
> notion of stream to all DAIs that are part of the same dailink.
> 
> I am not trying to block the Qualcomm implementation, just would like to 
> make sure the assumptions are clear and that we are not using the same 
> API in completely different ways.

Am open for any suggestions on dealing with dai less setups like what we 
have on our board.

--srini
> 
> 
> 
>
Charles Keepax Oct. 10, 2019, 12:03 p.m. UTC | #21
On Thu, Oct 10, 2019 at 09:50:22AM +0100, Srinivas Kandagatla wrote:
> On 09/10/2019 19:53, Pierre-Louis Bossart wrote:
> >On 10/9/19 11:01 AM, Srinivas Kandagatla wrote:
> >>On 09/10/2019 15:29, Pierre-Louis Bossart wrote:
> >>>On 10/9/19 3:32 AM, Srinivas Kandagatla wrote:
> >>>>On 14/08/2019 15:09, Pierre-Louis Bossart wrote:
> >>>>>On 8/13/19 11:11 PM, Vinod Koul wrote:
> >>>>>>On 13-08-19, 20:58, Mark Brown wrote:
> >>>>>>>On Tue, Aug 13, 2019 at 02:38:53PM -0500, Pierre-Louis
> >>>>>>>>Indeed. I don't have a full understanding of that
> >>>>>>>>part to be honest, nor why
> >>>>>>>>we need something SoundWire-specific. We already
> >>>>>>>>abused the set_tdm_slot API
> >>>>>>>>to store an HDaudio stream, now we have a rather confusing stream
> >>>>>>>>information for SoundWire and I have about 3 other
> >>>>>>>>'stream' contexts in
> >>>>>>>>SOF... I am still doing basic cleanups but this has
> >>>>>>>>been on my radar for a
> >>>>>>>>while.
> >>>>>>>
> >>>>>>>There is something to be said for not abusing the TDM
> >>>>>>>slot API if it can
> >>>>>>>make things clearer by using bus-idiomatic mechanisms,
> >>>>>>>but it does mean
> >>>>>>>everything needs to know about each individual bus :/ .
> >>>>>>
> >>>>>>Here ASoC doesn't need to know about sdw bus. As Srini
> >>>>>>explained, this
> >>>>>>helps in the case for him to get the stream context and
> >>>>>>set the stream
> >>>>>>context from the machine driver.
> >>>>>>
> >>>>>>Nothing else is expected to be done from this API. We
> >>>>>>already do a set
> >>>>>>using snd_soc_dai_set_sdw_stream(). Here we add the
> >>>>>>snd_soc_dai_get_sdw_stream() to query
> >>>>>
> >>>>>I didn't see a call to snd_soc_dai_set_sdw_stream() in Srini's code?
> >>>>
> >>>>
> >>>>There is a snd_soc_dai_get_sdw_stream() to get stream
> >>>>context and we add slave streams(amplifier in this case) to
> >>>>that context using sdw_stream_add_slave() in machine
> >>>>driver[1].
> >>>>
> >>>>Without this helper there is no way to link slave streams to
> >>>>stream context in non dai based setup like smart speaker
> >>>>amplifiers.
> >>>>
> >>>>Currently this driver is blocked on this patch, If you think
> >>>>there are other ways to do this, am happy to try them out.
> >>>
> >>>So to be clear, you are *not* using snd_soc_dai_set_sdw_stream?
> >>Yes, am not using snd_soc_dai_set_sdw_stream().
> >
> >It's been a while since this thread started, and I still don't
> >quite get the concepts or logic.
> >
> >First, I don't understand what the problem with "aux" devices is.
> >All the SoundWire stuff is based on the concept of DAI, so I guess
> >I am
> 
> That is the actual problem! Some aux devices does not have dais.
> 

Usually aux devices are used for things like analog amplifiers that
clearly don't have a digital interface, thus it really makes no sense
to have a DAI link connecting them. So I guess my question here
would be what is the thinking on making the "smart amplifier" dailess?
It feels like having a CODEC to CODEC DAI between the CODEC and
the AMP would be a more obvious way to connect the two devices
and would presumably avoid the issues being discussed around the
patch.

Thanks,
Charles
Mark Brown Oct. 10, 2019, 1:51 p.m. UTC | #22
On Thu, Oct 10, 2019 at 12:03:37PM +0000, Charles Keepax wrote:

> Usually aux devices are used for things like analog amplifiers that
> clearly don't have a digital interface, thus it really makes no sense
> to have a DAI link connecting them. So I guess my question here
> would be what is the thinking on making the "smart amplifier" dailess?
> It feels like having a CODEC to CODEC DAI between the CODEC and
> the AMP would be a more obvious way to connect the two devices
> and would presumably avoid the issues being discussed around the
> patch.

Or is this a device where for some reason (consistency?) there
really is no DAI and someone has decided to make an analogue
amplifier with a soundwire control interface?
Pierre-Louis Bossart Oct. 10, 2019, 2:01 p.m. UTC | #23
>>> It's been a while since this thread started, and I still don't
>>> quite get the concepts or logic.
>>>
>>> First, I don't understand what the problem with "aux" devices is.
>>> All the SoundWire stuff is based on the concept of DAI, so I guess
>>> I am
>>
>> That is the actual problem! Some aux devices does not have dais.
>>
> 
> Usually aux devices are used for things like analog amplifiers that
> clearly don't have a digital interface, thus it really makes no sense
> to have a DAI link connecting them. So I guess my question here
> would be what is the thinking on making the "smart amplifier" dailess?
> It feels like having a CODEC to CODEC DAI between the CODEC and
> the AMP would be a more obvious way to connect the two devices
> and would presumably avoid the issues being discussed around the
> patch.

Ah, now I get it, I missed the point about not having DAIs for the 
amplifier.

I will second Charles' point, the code you have in the machine driver at 
[1] gets a SoundWire stream context from the SLIMbus codec dai. It's a 
bit odd to mix layers this way.


And if I look at the code below, taken from [1], if you have more than 
one codec, then your code looks problematic: data->sruntime would be 
overriden and you'd use the info from the last codec dai on the dailink...

case SLIMBUS_0_RX...SLIMBUS_6_TX:
   for (i = 0 ; i < dai_link->num_codecs; i++) {
     [snip]
     data->sruntime[cpu_dai->id] =
	snd_soc_dai_get_sdw_stream(rtd->codec_dais[i], 0); << same destination 
for multiple codec_dais...

If you keep the amp dai-less, then the stream concept should be somehow 
allocated on the master (or machine) side and passed to the codec dais 
that are associated to the same 'stream'.
	

[1] 
https://git.linaro.org/people/srinivas.kandagatla/linux.git/tree/sound/soc/qcom/db845c.c?h=release/db845c/qcomlt-5.2
Srinivas Kandagatla Oct. 10, 2019, 2:52 p.m. UTC | #24
On 10/10/2019 15:01, Pierre-Louis Bossart wrote:
> 
>>>> It's been a while since this thread started, and I still don't
>>>> quite get the concepts or logic.
>>>>
>>>> First, I don't understand what the problem with "aux" devices is.
>>>> All the SoundWire stuff is based on the concept of DAI, so I guess
>>>> I am
>>>
>>> That is the actual problem! Some aux devices does not have dais.
>>>
>>
>> Usually aux devices are used for things like analog amplifiers that
>> clearly don't have a digital interface, thus it really makes no sense
>> to have a DAI link connecting them. So I guess my question here
>> would be what is the thinking on making the "smart amplifier" dailess?
>> It feels like having a CODEC to CODEC DAI between the CODEC and
>> the AMP would be a more obvious way to connect the two devices
>> and would presumably avoid the issues being discussed around the
>> patch.
> 
> Ah, now I get it, I missed the point about not having DAIs for the 
> amplifier.
> 
> I will second Charles' point, the code you have in the machine driver at 

I agree with Charles,

WSA8810/WSA8815 is connected via SoundWire digital interface, so I can 
try to model this amplifier with dais and see how it ends up.

I still need to figure out prefixing multiple instances of this 
Amplifier controls with "Left" and "Right"

> [1] gets a SoundWire stream context from the SLIMbus codec dai. It's a 
> bit odd to mix layers this way.

Yep we have a very mixed setup on this SoC.

So it looks like this.
Main WCD934X Codec which is connected via SLIMBus which has SoundWire 
Controller block along with other analog + digital blocks.
Again the SoundWire Controller from that WCD934X codec is wired up to 
WSA881X Smart speaker amplifiers.


> 
> 
> And if I look at the code below, taken from [1], if you have more than 
> one codec, then your code looks problematic: data->sruntime would be 
> overriden and you'd use the info from the last codec dai on the dailink...

This code has been written very much specific to DB845c which has only 
one codec. But I agree with your point.

--srini

> 
> case SLIMBUS_0_RX...SLIMBUS_6_TX:
>    for (i = 0 ; i < dai_link->num_codecs; i++) {
>      [snip]
>      data->sruntime[cpu_dai->id] =
>      snd_soc_dai_get_sdw_stream(rtd->codec_dais[i], 0); << same 
> destination for multiple codec_dais...
> 
> If you keep the amp dai-less, then the stream concept should be somehow 
> allocated on the master (or machine) side and passed to the codec dais 
> that are associated to the same 'stream'.
> 
> 
> [1] 
> https://git.linaro.org/people/srinivas.kandagatla/linux.git/tree/sound/soc/qcom/db845c.c?h=release/db845c/qcomlt-5.2 
> 
>
Pierre-Louis Bossart Oct. 10, 2019, 3:49 p.m. UTC | #25
> I still need to figure out prefixing multiple instances of this 
> Amplifier controls with "Left" and "Right"

FWIW we use the "snd_codec_conf" stuff to add a prefix for each 
amplifier, so that the controls are not mixed up between instances of 
the same amp, see e.g.

	
static struct snd_soc_codec_conf codec_conf[] = {
	{
		.dev_name = "sdw:0:25d:711:0:1",
		.name_prefix = "rt711",
	},
	{
		.dev_name = "sdw:1:25d:1308:0:0",
		.name_prefix = "rt1308-1",
	},
	{
		.dev_name = "sdw:2:25d:1308:0:2",
		.name_prefix = "rt1308-2",
	},
	{
		.dev_name = "sdw:3:25d:715:0:1",
		.name_prefix = "rt715",
	},
};


https://github.com/thesofproject/linux/pull/1142/commits/9ff9cf9d8994333df2250641c95431261bc66d69#diff-892560f80d603420baec7395e0b45d81R212
Srinivas Kandagatla Oct. 11, 2019, 12:30 p.m. UTC | #26
On 10/10/2019 16:49, Pierre-Louis Bossart wrote:
> 
> 
>> I still need to figure out prefixing multiple instances of this 
>> Amplifier controls with "Left" and "Right"
> 
> FWIW we use the "snd_codec_conf" stuff to add a prefix for each 
> amplifier, so that the controls are not mixed up between instances of 
> the same amp, see e.g.
> 
Thanks Pierre for the inputs.
Am using Documentation/devicetree/bindings/sound/name-prefix.txt for dt 
and it works!

I will send new set of patches by dropping this patch!

--srini
> 
> static struct snd_soc_codec_conf codec_conf[] = {
>      {
>          .dev_name = "sdw:0:25d:711:0:1",
>          .name_prefix = "rt711",
>      },
>      {
>          .dev_name = "sdw:1:25d:1308:0:0",
>          .name_prefix = "rt1308-1",
>      },
>      {
>          .dev_name = "sdw:2:25d:1308:0:2",
>          .name_prefix = "rt1308-2",
>      },
>      {
>          .dev_name = "sdw:3:25d:715:0:1",
>          .name_prefix = "rt715",
>      },
> };
> 
> 
> https://github.com/thesofproject/linux/pull/1142/commits/9ff9cf9d8994333df2250641c95431261bc66d69#diff-892560f80d603420baec7395e0b45d81R212 
>
diff mbox series

Patch

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index dc48fe081a20..1e01f4a302e0 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -202,6 +202,7 @@  struct snd_soc_dai_ops {
 
 	int (*set_sdw_stream)(struct snd_soc_dai *dai,
 			void *stream, int direction);
+	void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
 	/*
 	 * DAI digital mute - optional.
 	 * Called by soc-core to minimise any pops.
@@ -410,4 +411,13 @@  static inline int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai,
 		return -ENOTSUPP;
 }
 
+static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai,
+					       int direction)
+{
+	if (dai->driver->ops->get_sdw_stream)
+		return dai->driver->ops->get_sdw_stream(dai, direction);
+	else
+		return ERR_PTR(-ENOTSUPP);
+}
+
 #endif