[PATCH] soundwire: stream: only change state if needed

[PATCH] soundwire: stream: only change state if needed

[Pierre-Louis Bossart]

In a multi-cpu DAI context, the stream routines may be called from
multiple DAI callbacks. Make sure the stream state only changes for
the first call, and don't return error messages if the target state is
already reached.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/stream.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 1b43d03c79ea..3319121cd706 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
 	sdw_acquire_bus_lock(stream);
 
 	if (stream->state == SDW_STREAM_PREPARED) {
+		/* nothing to do */
 		ret = 0;
 		goto state_err;
 	}
@@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
 
 	sdw_acquire_bus_lock(stream);
 
+	if (stream->state == SDW_STREAM_ENABLED) {
+		/* nothing to do */
+		ret = 0;
+		goto state_err;
+	}
+
 	if (stream->state != SDW_STREAM_PREPARED &&
 	    stream->state != SDW_STREAM_DISABLED) {
 		pr_err("%s: %s: inconsistent state state %d\n",
@@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
 
 	sdw_acquire_bus_lock(stream);
 
+	if (stream->state == SDW_STREAM_DISABLED) {
+		/* nothing to do */
+		ret = 0;
+		goto state_err;
+	}
+
 	if (stream->state != SDW_STREAM_ENABLED) {
 		pr_err("%s: %s: inconsistent state state %d\n",
 		       __func__, stream->name, stream->state);
@@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
 
 	sdw_acquire_bus_lock(stream);
 
+	if (stream->state == SDW_STREAM_DEPREPARED) {
+		/* nothing to do */
+		ret = 0;
+		goto state_err;
+	}
+
 	if (stream->state != SDW_STREAM_PREPARED &&
 	    stream->state != SDW_STREAM_DISABLED) {
 		pr_err("%s: %s: inconsistent state state %d\n",
-- 
2.20.1

[PATCH] soundwire: stream: only change state if needed

[Pierre-Louis Bossart]

In a multi-cpu DAI context, the stream routines may be called from
multiple DAI callbacks. Make sure the stream state only changes for
the first call, and don't return error messages if the target state is
already reached.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/stream.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 1b43d03c79ea..3319121cd706 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
 	sdw_acquire_bus_lock(stream);
 
 	if (stream->state == SDW_STREAM_PREPARED) {
+		/* nothing to do */
 		ret = 0;
 		goto state_err;
 	}
@@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
 
 	sdw_acquire_bus_lock(stream);
 
+	if (stream->state == SDW_STREAM_ENABLED) {
+		/* nothing to do */
+		ret = 0;
+		goto state_err;
+	}
+
 	if (stream->state != SDW_STREAM_PREPARED &&
 	    stream->state != SDW_STREAM_DISABLED) {
 		pr_err("%s: %s: inconsistent state state %d\n",
@@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
 
 	sdw_acquire_bus_lock(stream);
 
+	if (stream->state == SDW_STREAM_DISABLED) {
+		/* nothing to do */
+		ret = 0;
+		goto state_err;
+	}
+
 	if (stream->state != SDW_STREAM_ENABLED) {
 		pr_err("%s: %s: inconsistent state state %d\n",
 		       __func__, stream->name, stream->state);
@@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
 
 	sdw_acquire_bus_lock(stream);
 
+	if (stream->state == SDW_STREAM_DEPREPARED) {
+		/* nothing to do */
+		ret = 0;
+		goto state_err;
+	}
+
 	if (stream->state != SDW_STREAM_PREPARED &&
 	    stream->state != SDW_STREAM_DISABLED) {
 		pr_err("%s: %s: inconsistent state state %d\n",
-- 
2.20.1

Re: [PATCH] soundwire: stream: only change state if needed

[Srinivas Kandagatla]

Thanks Pierre for this patch,

On 17/03/2020 10:51, Pierre-Louis Bossart wrote:
> In a multi-cpu DAI context, the stream routines may be called from
> multiple DAI callbacks. Make sure the stream state only changes for
> the first call, and don't return error messages if the target state is
> already reached.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>   drivers/soundwire/stream.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 

This patch did not work for me as it is as wsa881x codec does prepare 
and enable in one function, which breaks some of the assumptions in this 
patch.

However with below change I could get it working without moving stream 
handling to machine driver.

---------------------------->cut<-------------------------------
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index be71af4671a4..4a94ea64c1c5 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1574,7 +1574,8 @@ int sdw_prepare_stream(struct sdw_stream_runtime 
*stream)

         sdw_acquire_bus_lock(stream);

-       if (stream->state == SDW_STREAM_PREPARED) {
+       if (stream->state == SDW_STREAM_PREPARED ||
+           stream->state == SDW_STREAM_ENABLED) {
                 /* nothing to do */
                 ret = 0;
                 goto state_err;
@@ -1754,7 +1755,8 @@ int sdw_disable_stream(struct sdw_stream_runtime 
*stream)

         sdw_acquire_bus_lock(stream);

-       if (stream->state == SDW_STREAM_DISABLED) {
+       if (stream->state == SDW_STREAM_DISABLED ||
+           stream->state == SDW_STREAM_DEPREPARED) {
                 /* nothing to do */
                 ret = 0;
                 goto state_err;
---------------------------->cut<-------------------------------

--srini

> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 1b43d03c79ea..3319121cd706 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
>   	sdw_acquire_bus_lock(stream);
>   
>   	if (stream->state == SDW_STREAM_PREPARED) {
> +		/* nothing to do */
>   		ret = 0;
>   		goto state_err;
>   	}
> @@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
>   
>   	sdw_acquire_bus_lock(stream);
>   
> +	if (stream->state == SDW_STREAM_ENABLED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>   	if (stream->state != SDW_STREAM_PREPARED &&
>   	    stream->state != SDW_STREAM_DISABLED) {
>   		pr_err("%s: %s: inconsistent state state %d\n",
> @@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
>   
>   	sdw_acquire_bus_lock(stream);
>   
> +	if (stream->state == SDW_STREAM_DISABLED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>   	if (stream->state != SDW_STREAM_ENABLED) {
>   		pr_err("%s: %s: inconsistent state state %d\n",
>   		       __func__, stream->name, stream->state);
> @@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
>   
>   	sdw_acquire_bus_lock(stream);
>   
> +	if (stream->state == SDW_STREAM_DEPREPARED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>   	if (stream->state != SDW_STREAM_PREPARED &&
>   	    stream->state != SDW_STREAM_DISABLED) {
>   		pr_err("%s: %s: inconsistent state state %d\n",
> 

Re: [PATCH] soundwire: stream: only change state if needed

[Srinivas Kandagatla]

Thanks Pierre for this patch,

On 17/03/2020 10:51, Pierre-Louis Bossart wrote:
> In a multi-cpu DAI context, the stream routines may be called from
> multiple DAI callbacks. Make sure the stream state only changes for
> the first call, and don't return error messages if the target state is
> already reached.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>   drivers/soundwire/stream.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 

This patch did not work for me as it is as wsa881x codec does prepare 
and enable in one function, which breaks some of the assumptions in this 
patch.

However with below change I could get it working without moving stream 
handling to machine driver.

---------------------------->cut<-------------------------------
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index be71af4671a4..4a94ea64c1c5 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1574,7 +1574,8 @@ int sdw_prepare_stream(struct sdw_stream_runtime 
*stream)

         sdw_acquire_bus_lock(stream);

-       if (stream->state == SDW_STREAM_PREPARED) {
+       if (stream->state == SDW_STREAM_PREPARED ||
+           stream->state == SDW_STREAM_ENABLED) {
                 /* nothing to do */
                 ret = 0;
                 goto state_err;
@@ -1754,7 +1755,8 @@ int sdw_disable_stream(struct sdw_stream_runtime 
*stream)

         sdw_acquire_bus_lock(stream);

-       if (stream->state == SDW_STREAM_DISABLED) {
+       if (stream->state == SDW_STREAM_DISABLED ||
+           stream->state == SDW_STREAM_DEPREPARED) {
                 /* nothing to do */
                 ret = 0;
                 goto state_err;
---------------------------->cut<-------------------------------

--srini

> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 1b43d03c79ea..3319121cd706 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
>   	sdw_acquire_bus_lock(stream);
>   
>   	if (stream->state == SDW_STREAM_PREPARED) {
> +		/* nothing to do */
>   		ret = 0;
>   		goto state_err;
>   	}
> @@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
>   
>   	sdw_acquire_bus_lock(stream);
>   
> +	if (stream->state == SDW_STREAM_ENABLED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>   	if (stream->state != SDW_STREAM_PREPARED &&
>   	    stream->state != SDW_STREAM_DISABLED) {
>   		pr_err("%s: %s: inconsistent state state %d\n",
> @@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
>   
>   	sdw_acquire_bus_lock(stream);
>   
> +	if (stream->state == SDW_STREAM_DISABLED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>   	if (stream->state != SDW_STREAM_ENABLED) {
>   		pr_err("%s: %s: inconsistent state state %d\n",
>   		       __func__, stream->name, stream->state);
> @@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
>   
>   	sdw_acquire_bus_lock(stream);
>   
> +	if (stream->state == SDW_STREAM_DEPREPARED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>   	if (stream->state != SDW_STREAM_PREPARED &&
>   	    stream->state != SDW_STREAM_DISABLED) {
>   		pr_err("%s: %s: inconsistent state state %d\n",
> 

Re: [PATCH] soundwire: stream: only change state if needed

[Pierre-Louis Bossart]

Hi Srinivas,

> This patch did not work for me as it is as wsa881x codec does prepare 
> and enable in one function, which breaks some of the assumptions in this 
> patch.

Ah yes, if two transitions happen in the same DAI callback that wouldn't 
work indeed. We should probably add this restriction to the state 
machine documentation, the suggested mapping from ASoC DAI states to 
stream states did not account for compound cases.

> However with below change I could get it working without moving stream 
> handling to machine driver.

The change below would be an error case for Intel, so it's probably 
better if we go with your suggestion. You have a very specific state 
handling due to your power amps and it's probably better to keep it 
platform-specific.

Can you confirm though that this patch works fine if you move all the 
stream transitions to the machine driver? That should be a no-op but 
better make sure there's no misunderstanding.

Thanks
-Pierre

> ---------------------------->cut<-------------------------------
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index be71af4671a4..4a94ea64c1c5 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1574,7 +1574,8 @@ int sdw_prepare_stream(struct sdw_stream_runtime 
> *stream)
> 
>          sdw_acquire_bus_lock(stream);
> 
> -       if (stream->state == SDW_STREAM_PREPARED) {
> +       if (stream->state == SDW_STREAM_PREPARED ||
> +           stream->state == SDW_STREAM_ENABLED) {
>                  /* nothing to do */
>                  ret = 0;
>                  goto state_err;
> @@ -1754,7 +1755,8 @@ int sdw_disable_stream(struct sdw_stream_runtime 
> *stream)
> 
>          sdw_acquire_bus_lock(stream);
> 
> -       if (stream->state == SDW_STREAM_DISABLED) {
> +       if (stream->state == SDW_STREAM_DISABLED ||
> +           stream->state == SDW_STREAM_DEPREPARED) {
>                  /* nothing to do */
>                  ret = 0;
>                  goto state_err;
> ---------------------------->cut<-------------------------------
> 
> --srini
> 
>> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
>> index 1b43d03c79ea..3319121cd706 100644
>> --- a/drivers/soundwire/stream.c
>> +++ b/drivers/soundwire/stream.c
>> @@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime 
>> *stream)
>>       sdw_acquire_bus_lock(stream);
>>       if (stream->state == SDW_STREAM_PREPARED) {
>> +        /* nothing to do */
>>           ret = 0;
>>           goto state_err;
>>       }
>> @@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime 
>> *stream)
>>       sdw_acquire_bus_lock(stream);
>> +    if (stream->state == SDW_STREAM_ENABLED) {
>> +        /* nothing to do */
>> +        ret = 0;
>> +        goto state_err;
>> +    }
>> +
>>       if (stream->state != SDW_STREAM_PREPARED &&
>>           stream->state != SDW_STREAM_DISABLED) {
>>           pr_err("%s: %s: inconsistent state state %d\n",
>> @@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct 
>> sdw_stream_runtime *stream)
>>       sdw_acquire_bus_lock(stream);
>> +    if (stream->state == SDW_STREAM_DISABLED) {
>> +        /* nothing to do */
>> +        ret = 0;
>> +        goto state_err;
>> +    }
>> +
>>       if (stream->state != SDW_STREAM_ENABLED) {
>>           pr_err("%s: %s: inconsistent state state %d\n",
>>                  __func__, stream->name, stream->state);
>> @@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct 
>> sdw_stream_runtime *stream)
>>       sdw_acquire_bus_lock(stream);
>> +    if (stream->state == SDW_STREAM_DEPREPARED) {
>> +        /* nothing to do */
>> +        ret = 0;
>> +        goto state_err;
>> +    }
>> +
>>       if (stream->state != SDW_STREAM_PREPARED &&
>>           stream->state != SDW_STREAM_DISABLED) {
>>           pr_err("%s: %s: inconsistent state state %d\n",
>>

Re: [PATCH] soundwire: stream: only change state if needed

[Pierre-Louis Bossart]

Hi Srinivas,

> This patch did not work for me as it is as wsa881x codec does prepare 
> and enable in one function, which breaks some of the assumptions in this 
> patch.

Ah yes, if two transitions happen in the same DAI callback that wouldn't 
work indeed. We should probably add this restriction to the state 
machine documentation, the suggested mapping from ASoC DAI states to 
stream states did not account for compound cases.

> However with below change I could get it working without moving stream 
> handling to machine driver.

The change below would be an error case for Intel, so it's probably 
better if we go with your suggestion. You have a very specific state 
handling due to your power amps and it's probably better to keep it 
platform-specific.

Can you confirm though that this patch works fine if you move all the 
stream transitions to the machine driver? That should be a no-op but 
better make sure there's no misunderstanding.

Thanks
-Pierre

> ---------------------------->cut<-------------------------------
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index be71af4671a4..4a94ea64c1c5 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1574,7 +1574,8 @@ int sdw_prepare_stream(struct sdw_stream_runtime 
> *stream)
> 
>          sdw_acquire_bus_lock(stream);
> 
> -       if (stream->state == SDW_STREAM_PREPARED) {
> +       if (stream->state == SDW_STREAM_PREPARED ||
> +           stream->state == SDW_STREAM_ENABLED) {
>                  /* nothing to do */
>                  ret = 0;
>                  goto state_err;
> @@ -1754,7 +1755,8 @@ int sdw_disable_stream(struct sdw_stream_runtime 
> *stream)
> 
>          sdw_acquire_bus_lock(stream);
> 
> -       if (stream->state == SDW_STREAM_DISABLED) {
> +       if (stream->state == SDW_STREAM_DISABLED ||
> +           stream->state == SDW_STREAM_DEPREPARED) {
>                  /* nothing to do */
>                  ret = 0;
>                  goto state_err;
> ---------------------------->cut<-------------------------------
> 
> --srini
> 
>> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
>> index 1b43d03c79ea..3319121cd706 100644
>> --- a/drivers/soundwire/stream.c
>> +++ b/drivers/soundwire/stream.c
>> @@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime 
>> *stream)
>>       sdw_acquire_bus_lock(stream);
>>       if (stream->state == SDW_STREAM_PREPARED) {
>> +        /* nothing to do */
>>           ret = 0;
>>           goto state_err;
>>       }
>> @@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime 
>> *stream)
>>       sdw_acquire_bus_lock(stream);
>> +    if (stream->state == SDW_STREAM_ENABLED) {
>> +        /* nothing to do */
>> +        ret = 0;
>> +        goto state_err;
>> +    }
>> +
>>       if (stream->state != SDW_STREAM_PREPARED &&
>>           stream->state != SDW_STREAM_DISABLED) {
>>           pr_err("%s: %s: inconsistent state state %d\n",
>> @@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct 
>> sdw_stream_runtime *stream)
>>       sdw_acquire_bus_lock(stream);
>> +    if (stream->state == SDW_STREAM_DISABLED) {
>> +        /* nothing to do */
>> +        ret = 0;
>> +        goto state_err;
>> +    }
>> +
>>       if (stream->state != SDW_STREAM_ENABLED) {
>>           pr_err("%s: %s: inconsistent state state %d\n",
>>                  __func__, stream->name, stream->state);
>> @@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct 
>> sdw_stream_runtime *stream)
>>       sdw_acquire_bus_lock(stream);
>> +    if (stream->state == SDW_STREAM_DEPREPARED) {
>> +        /* nothing to do */
>> +        ret = 0;
>> +        goto state_err;
>> +    }
>> +
>>       if (stream->state != SDW_STREAM_PREPARED &&
>>           stream->state != SDW_STREAM_DISABLED) {
>>           pr_err("%s: %s: inconsistent state state %d\n",
>>

Re: [PATCH] soundwire: stream: only change state if needed

[Srinivas Kandagatla]

On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
> Hi Srinivas,
> 
>> This patch did not work for me as it is as wsa881x codec does prepare 
>> and enable in one function, which breaks some of the assumptions in 
>> this patch.
> 
> Ah yes, if two transitions happen in the same DAI callback that wouldn't 
> work indeed. We should probably add this restriction to the state 
> machine documentation, the suggested mapping from ASoC DAI states to 
> stream states did not account for compound cases.
> 
>> However with below change I could get it working without moving stream 
>> handling to machine driver.
> 
> The change below would be an error case for Intel, so it's probably 
> better if we go with your suggestion. You have a very specific state 
> handling due to your power amps and it's probably better to keep it 
> platform-specific.
> 
> Can you confirm though that this patch works fine if you move all the 
> stream transitions to the machine driver? That should be a no-op but 
> better make sure there's no misunderstanding.

yes, it works with this patch + moving wsa stream handing to machine driver.

--srini
> 
> Thanks
> -Pierre

Re: [PATCH] soundwire: stream: only change state if needed

[Srinivas Kandagatla]

On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
> Hi Srinivas,
> 
>> This patch did not work for me as it is as wsa881x codec does prepare 
>> and enable in one function, which breaks some of the assumptions in 
>> this patch.
> 
> Ah yes, if two transitions happen in the same DAI callback that wouldn't 
> work indeed. We should probably add this restriction to the state 
> machine documentation, the suggested mapping from ASoC DAI states to 
> stream states did not account for compound cases.
> 
>> However with below change I could get it working without moving stream 
>> handling to machine driver.
> 
> The change below would be an error case for Intel, so it's probably 
> better if we go with your suggestion. You have a very specific state 
> handling due to your power amps and it's probably better to keep it 
> platform-specific.
> 
> Can you confirm though that this patch works fine if you move all the 
> stream transitions to the machine driver? That should be a no-op but 
> better make sure there's no misunderstanding.

yes, it works with this patch + moving wsa stream handing to machine driver.

--srini
> 
> Thanks
> -Pierre

Re: [PATCH] soundwire: stream: only change state if needed

[Srinivas Kandagatla]

On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
> 
> The change below would be an error case for Intel, so it's probably 
> better if we go with your suggestion. You have a very specific state 
> handling due to your power amps and it's probably better to keep it 
> platform-specific.

Just trying to understand, why would it be error for Intel case?

IMO, If stream state is SDW_STREAM_ENABLED that also implicit that its 
prepared too. Similar thing with SDW_STREAM_DEPREPARED.
Isn't it?

--srini

Re: [PATCH] soundwire: stream: only change state if needed

[Srinivas Kandagatla]

On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
> 
> The change below would be an error case for Intel, so it's probably 
> better if we go with your suggestion. You have a very specific state 
> handling due to your power amps and it's probably better to keep it 
> platform-specific.

Just trying to understand, why would it be error for Intel case?

IMO, If stream state is SDW_STREAM_ENABLED that also implicit that its 
prepared too. Similar thing with SDW_STREAM_DEPREPARED.
Isn't it?

--srini

Re: [PATCH] soundwire: stream: only change state if needed

[Pierre-Louis Bossart]

On 3/17/20 8:04 AM, Srinivas Kandagatla wrote:
> 
> 
> On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
>>
>> The change below would be an error case for Intel, so it's probably 
>> better if we go with your suggestion. You have a very specific state 
>> handling due to your power amps and it's probably better to keep it 
>> platform-specific.
> 
> Just trying to understand, why would it be error for Intel case?
> 
> IMO, If stream state is SDW_STREAM_ENABLED that also implicit that its 
> prepared too. Similar thing with SDW_STREAM_DEPREPARED.
> Isn't it?

the stream state is a scalar value, not a mask. The state machine only 
allows transition from CONFIGURED TO PREPARED or from DEPREPARED TO 
PREPARED, or DISABLED to PREPARED.
There is no allowed transition from ENABLED TO PREPARED, you have to go 
through the DISABLED state and make sure a bank switch occurred, and 
re-do a bank switch to prepare again.

Re: [PATCH] soundwire: stream: only change state if needed

[Pierre-Louis Bossart]

On 3/17/20 8:04 AM, Srinivas Kandagatla wrote:
> 
> 
> On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
>>
>> The change below would be an error case for Intel, so it's probably 
>> better if we go with your suggestion. You have a very specific state 
>> handling due to your power amps and it's probably better to keep it 
>> platform-specific.
> 
> Just trying to understand, why would it be error for Intel case?
> 
> IMO, If stream state is SDW_STREAM_ENABLED that also implicit that its 
> prepared too. Similar thing with SDW_STREAM_DEPREPARED.
> Isn't it?

the stream state is a scalar value, not a mask. The state machine only 
allows transition from CONFIGURED TO PREPARED or from DEPREPARED TO 
PREPARED, or DISABLED to PREPARED.
There is no allowed transition from ENABLED TO PREPARED, you have to go 
through the DISABLED state and make sure a bank switch occurred, and 
re-do a bank switch to prepare again.

Re: [PATCH] soundwire: stream: only change state if needed

[Srinivas Kandagatla]

On 17/03/2020 13:19, Pierre-Louis Bossart wrote:
> 
> 
> On 3/17/20 8:04 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
>>>
>>> The change below would be an error case for Intel, so it's probably 
>>> better if we go with your suggestion. You have a very specific state 
>>> handling due to your power amps and it's probably better to keep it 
>>> platform-specific.
>>
>> Just trying to understand, why would it be error for Intel case?
>>
>> IMO, If stream state is SDW_STREAM_ENABLED that also implicit that its 
>> prepared too. Similar thing with SDW_STREAM_DEPREPARED.
>> Isn't it?
> 
> the stream state is a scalar value, not a mask. The state machine only 
> allows transition from CONFIGURED TO PREPARED or from DEPREPARED TO 
> PREPARED, or DISABLED to PREPARED.
> There is no allowed transition from ENABLED TO PREPARED, you have to go 
> through the DISABLED state and make sure a bank switch occurred, and 
> re-do a bank switch to prepare again.
I agree with you if are on single dai case. Am happy to move to stream 
handling to machine driver for now.

But this also means that in cases like multi-codec its not recommended 
to call sdw_prepare and sdw_enable in a single function from codec.
Which might be worth documenting.

--srini

Re: [PATCH] soundwire: stream: only change state if needed

[Srinivas Kandagatla]

On 17/03/2020 13:19, Pierre-Louis Bossart wrote:
> 
> 
> On 3/17/20 8:04 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
>>>
>>> The change below would be an error case for Intel, so it's probably 
>>> better if we go with your suggestion. You have a very specific state 
>>> handling due to your power amps and it's probably better to keep it 
>>> platform-specific.
>>
>> Just trying to understand, why would it be error for Intel case?
>>
>> IMO, If stream state is SDW_STREAM_ENABLED that also implicit that its 
>> prepared too. Similar thing with SDW_STREAM_DEPREPARED.
>> Isn't it?
> 
> the stream state is a scalar value, not a mask. The state machine only 
> allows transition from CONFIGURED TO PREPARED or from DEPREPARED TO 
> PREPARED, or DISABLED to PREPARED.
> There is no allowed transition from ENABLED TO PREPARED, you have to go 
> through the DISABLED state and make sure a bank switch occurred, and 
> re-do a bank switch to prepare again.
I agree with you if are on single dai case. Am happy to move to stream 
handling to machine driver for now.

But this also means that in cases like multi-codec its not recommended 
to call sdw_prepare and sdw_enable in a single function from codec.
Which might be worth documenting.

--srini

Re: [PATCH] soundwire: stream: only change state if needed

[Pierre-Louis Bossart]

>>>> The change below would be an error case for Intel, so it's probably 
>>>> better if we go with your suggestion. You have a very specific state 
>>>> handling due to your power amps and it's probably better to keep it 
>>>> platform-specific.
>>>
>>> Just trying to understand, why would it be error for Intel case?
>>>
>>> IMO, If stream state is SDW_STREAM_ENABLED that also implicit that 
>>> its prepared too. Similar thing with SDW_STREAM_DEPREPARED.
>>> Isn't it?
>>
>> the stream state is a scalar value, not a mask. The state machine only 
>> allows transition from CONFIGURED TO PREPARED or from DEPREPARED TO 
>> PREPARED, or DISABLED to PREPARED.
>> There is no allowed transition from ENABLED TO PREPARED, you have to 
>> go through the DISABLED state and make sure a bank switch occurred, 
>> and re-do a bank switch to prepare again.
> I agree with you if are on single dai case. Am happy to move to stream 
> handling to machine driver for now.
> 
> But this also means that in cases like multi-codec its not recommended 
> to call sdw_prepare and sdw_enable in a single function from codec.
> Which might be worth documenting.

Well, the bigger question is why use sdw_stream functions at the codec 
level in the first place? All other codec drivers seem to have no issue 
leaving the dais owned by the master control stream state changes.

I am not saying I object or it's bad, just that there were significant 
changes in usages of the 'stream' concept since it was introduced, as 
well as threads in MIPI circles to clarify the prepare/enable 
dependencies, so it'd be useful to have a complete picture of what 
different platforms need/want.

Re: [PATCH] soundwire: stream: only change state if needed

[Pierre-Louis Bossart]

>>>> The change below would be an error case for Intel, so it's probably 
>>>> better if we go with your suggestion. You have a very specific state 
>>>> handling due to your power amps and it's probably better to keep it 
>>>> platform-specific.
>>>
>>> Just trying to understand, why would it be error for Intel case?
>>>
>>> IMO, If stream state is SDW_STREAM_ENABLED that also implicit that 
>>> its prepared too. Similar thing with SDW_STREAM_DEPREPARED.
>>> Isn't it?
>>
>> the stream state is a scalar value, not a mask. The state machine only 
>> allows transition from CONFIGURED TO PREPARED or from DEPREPARED TO 
>> PREPARED, or DISABLED to PREPARED.
>> There is no allowed transition from ENABLED TO PREPARED, you have to 
>> go through the DISABLED state and make sure a bank switch occurred, 
>> and re-do a bank switch to prepare again.
> I agree with you if are on single dai case. Am happy to move to stream 
> handling to machine driver for now.
> 
> But this also means that in cases like multi-codec its not recommended 
> to call sdw_prepare and sdw_enable in a single function from codec.
> Which might be worth documenting.

Well, the bigger question is why use sdw_stream functions at the codec 
level in the first place? All other codec drivers seem to have no issue 
leaving the dais owned by the master control stream state changes.

I am not saying I object or it's bad, just that there were significant 
changes in usages of the 'stream' concept since it was introduced, as 
well as threads in MIPI circles to clarify the prepare/enable 
dependencies, so it'd be useful to have a complete picture of what 
different platforms need/want.

Re: [PATCH] soundwire: stream: only change state if needed

[Vinod Koul]

On 17-03-20, 05:51, Pierre-Louis Bossart wrote:
> In a multi-cpu DAI context, the stream routines may be called from
> multiple DAI callbacks. Make sure the stream state only changes for
> the first call, and don't return error messages if the target state is
> already reached.

For stream-apis we have documented explicitly in Documentation/driver-api/soundwire/stream.rst

"Bus implements below API for allocate a stream which needs to be called once
per stream. From ASoC DPCM framework, this stream state maybe linked to
.startup() operation.

.. code-block:: c

  int sdw_alloc_stream(char * stream_name); "

This is documented for all stream-apis.

This can be resolved by moving the calling of these APIs from
master-dais/slave-dais to machine-dais. They are unique in the card.

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/stream.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 1b43d03c79ea..3319121cd706 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
>  	sdw_acquire_bus_lock(stream);
>  
>  	if (stream->state == SDW_STREAM_PREPARED) {
> +		/* nothing to do */
>  		ret = 0;
>  		goto state_err;
>  	}
> @@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
>  
>  	sdw_acquire_bus_lock(stream);
>  
> +	if (stream->state == SDW_STREAM_ENABLED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>  	if (stream->state != SDW_STREAM_PREPARED &&
>  	    stream->state != SDW_STREAM_DISABLED) {
>  		pr_err("%s: %s: inconsistent state state %d\n",
> @@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
>  
>  	sdw_acquire_bus_lock(stream);
>  
> +	if (stream->state == SDW_STREAM_DISABLED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>  	if (stream->state != SDW_STREAM_ENABLED) {
>  		pr_err("%s: %s: inconsistent state state %d\n",
>  		       __func__, stream->name, stream->state);
> @@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
>  
>  	sdw_acquire_bus_lock(stream);
>  
> +	if (stream->state == SDW_STREAM_DEPREPARED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>  	if (stream->state != SDW_STREAM_PREPARED &&
>  	    stream->state != SDW_STREAM_DISABLED) {
>  		pr_err("%s: %s: inconsistent state state %d\n",
> -- 
> 2.20.1

-- 
~Vinod

Re: [PATCH] soundwire: stream: only change state if needed

[Vinod Koul]

On 17-03-20, 05:51, Pierre-Louis Bossart wrote:
> In a multi-cpu DAI context, the stream routines may be called from
> multiple DAI callbacks. Make sure the stream state only changes for
> the first call, and don't return error messages if the target state is
> already reached.

For stream-apis we have documented explicitly in Documentation/driver-api/soundwire/stream.rst

"Bus implements below API for allocate a stream which needs to be called once
per stream. From ASoC DPCM framework, this stream state maybe linked to
.startup() operation.

.. code-block:: c

  int sdw_alloc_stream(char * stream_name); "

This is documented for all stream-apis.

This can be resolved by moving the calling of these APIs from
master-dais/slave-dais to machine-dais. They are unique in the card.

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/stream.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 1b43d03c79ea..3319121cd706 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
>  	sdw_acquire_bus_lock(stream);
>  
>  	if (stream->state == SDW_STREAM_PREPARED) {
> +		/* nothing to do */
>  		ret = 0;
>  		goto state_err;
>  	}
> @@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
>  
>  	sdw_acquire_bus_lock(stream);
>  
> +	if (stream->state == SDW_STREAM_ENABLED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>  	if (stream->state != SDW_STREAM_PREPARED &&
>  	    stream->state != SDW_STREAM_DISABLED) {
>  		pr_err("%s: %s: inconsistent state state %d\n",
> @@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
>  
>  	sdw_acquire_bus_lock(stream);
>  
> +	if (stream->state == SDW_STREAM_DISABLED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>  	if (stream->state != SDW_STREAM_ENABLED) {
>  		pr_err("%s: %s: inconsistent state state %d\n",
>  		       __func__, stream->name, stream->state);
> @@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
>  
>  	sdw_acquire_bus_lock(stream);
>  
> +	if (stream->state == SDW_STREAM_DEPREPARED) {
> +		/* nothing to do */
> +		ret = 0;
> +		goto state_err;
> +	}
> +
>  	if (stream->state != SDW_STREAM_PREPARED &&
>  	    stream->state != SDW_STREAM_DISABLED) {
>  		pr_err("%s: %s: inconsistent state state %d\n",
> -- 
> 2.20.1

-- 
~Vinod

Re: [PATCH] soundwire: stream: only change state if needed

[Pierre-Louis Bossart]

On 3/20/20 9:15 AM, Vinod Koul wrote:
> On 17-03-20, 05:51, Pierre-Louis Bossart wrote:
>> In a multi-cpu DAI context, the stream routines may be called from
>> multiple DAI callbacks. Make sure the stream state only changes for
>> the first call, and don't return error messages if the target state is
>> already reached.
> 
> For stream-apis we have documented explicitly in Documentation/driver-api/soundwire/stream.rst
> 
> "Bus implements below API for allocate a stream which needs to be called once
> per stream. From ASoC DPCM framework, this stream state maybe linked to
> .startup() operation.
> 
> .. code-block:: c
> 
>    int sdw_alloc_stream(char * stream_name); "
> 
> This is documented for all stream-apis.
> 
> This can be resolved by moving the calling of these APIs from
> master-dais/slave-dais to machine-dais. They are unique in the card.

this change is about prepare/enable/disable/deprepare, not allocation or 
startup.

I see no reason to burden the machine driver with all these steps. It's 
not because QCOM needs this transition that everyone does.

As discussed earlier, QCOM cannot use this functionality because the 
prepare/enable and disable/deprepare are done in the hw_params and 
hw_free respectively. This was never the intended use, but Intel let it 
happen so I'd like you to return the favor. This change has no impact 
for QCOM and simplifies the Intel solution, so why would you object?

Seriously, your replies on all Intel contributions make me wonder if 
this is the QCOM/Linaro SoundWire subsystem, or if we are going to find 
common ground to deal with vastly different underlying architectures?

Re: [PATCH] soundwire: stream: only change state if needed

[Pierre-Louis Bossart]

On 3/20/20 9:15 AM, Vinod Koul wrote:
> On 17-03-20, 05:51, Pierre-Louis Bossart wrote:
>> In a multi-cpu DAI context, the stream routines may be called from
>> multiple DAI callbacks. Make sure the stream state only changes for
>> the first call, and don't return error messages if the target state is
>> already reached.
> 
> For stream-apis we have documented explicitly in Documentation/driver-api/soundwire/stream.rst
> 
> "Bus implements below API for allocate a stream which needs to be called once
> per stream. From ASoC DPCM framework, this stream state maybe linked to
> .startup() operation.
> 
> .. code-block:: c
> 
>    int sdw_alloc_stream(char * stream_name); "
> 
> This is documented for all stream-apis.
> 
> This can be resolved by moving the calling of these APIs from
> master-dais/slave-dais to machine-dais. They are unique in the card.

this change is about prepare/enable/disable/deprepare, not allocation or 
startup.

I see no reason to burden the machine driver with all these steps. It's 
not because QCOM needs this transition that everyone does.

As discussed earlier, QCOM cannot use this functionality because the 
prepare/enable and disable/deprepare are done in the hw_params and 
hw_free respectively. This was never the intended use, but Intel let it 
happen so I'd like you to return the favor. This change has no impact 
for QCOM and simplifies the Intel solution, so why would you object?

Seriously, your replies on all Intel contributions make me wonder if 
this is the QCOM/Linaro SoundWire subsystem, or if we are going to find 
common ground to deal with vastly different underlying architectures?

Re: [PATCH] soundwire: stream: only change state if needed

[Vinod Koul]

On 20-03-20, 09:33, Pierre-Louis Bossart wrote:
> On 3/20/20 9:15 AM, Vinod Koul wrote:
> > On 17-03-20, 05:51, Pierre-Louis Bossart wrote:
> > > In a multi-cpu DAI context, the stream routines may be called from
> > > multiple DAI callbacks. Make sure the stream state only changes for
> > > the first call, and don't return error messages if the target state is
> > > already reached.

Again I ask you to read emails carefully and not jump the gun.

> > For stream-apis we have documented explicitly in Documentation/driver-api/soundwire/stream.rst
> > 

< begins with quote from the file stream.rst>

> > "Bus implements below API for allocate a stream which needs to be called once
> > per stream. From ASoC DPCM framework, this stream state maybe linked to
> > .startup() operation.
> > 
> > .. code-block:: c
> > 
> >    int sdw_alloc_stream(char * stream_name); "

<end quote>

> > This is documented for all stream-apis.

This line is very important. But seems to have skipped

> > This can be resolved by moving the calling of these APIs from
> > master-dais/slave-dais to machine-dais. They are unique in the card.

This is suggestion, feel free to do anything as long as that conforms to
documentation laid out, which incidentally was written by Sanyog and
signed off by you. See 89634f99a83e ("Documentation: soundwire: Add more documentation")

> this change is about prepare/enable/disable/deprepare, not allocation or
> startup.

Sad to see this. Now am going to quote for all these, since you skipped
the line in above email.

For prepare:
"Bus implements below API for PREPARE state which needs to be called
once per stream. From ASoC DPCM framework, this stream state is linked
to .prepare() operation. Since the .trigger() operations may not
follow the .prepare(), a direct transition from
``SDW_STREAM_PREPARED`` to ``SDW_STREAM_DEPREPARED`` is allowed."

See the point about "once per stream".

On Enable:
"Bus implements below API for ENABLE state which needs to be called once per
stream. From ASoC DPCM framework, this stream state is linked to
.trigger() start operation."

See the point about "once per stream".

For Disable:
"Bus implements below API for DISABLED state which needs to be called once
per stream. From ASoC DPCM framework, this stream state is linked to
.trigger() stop operation."

See the point about "once per stream".

For deprepare:
"Bus implements below API for DEPREPARED state which needs to be called
once per stream. ALSA/ASoC do not have a concept of 'deprepare', and
the mapping from this stream state to ALSA/ASoC operation may be
implementation specific."

See the point about "once per stream".

> I see no reason to burden the machine driver with all these steps. It's not
> because QCOM needs this transition that everyone does.
> 
> As discussed earlier, QCOM cannot use this functionality because the
> prepare/enable and disable/deprepare are done in the hw_params and hw_free
> respectively. This was never the intended use, but Intel let it happen so
> I'd like you to return the favor. This change has no impact for QCOM and
> simplifies the Intel solution, so why would you object?
> 
> Seriously, your replies on all Intel contributions make me wonder if this is
> the QCOM/Linaro SoundWire subsystem, or if we are going to find common
> ground to deal with vastly different underlying architectures?

It is extremely sad that you chose to say this and didn't even try to
read the email and recheck the documentation you have signed off.

This has nothing to do with Qcom/Linaro. If you look at git tree in
Intel you will find that the original implementation was to use these from
machine driver. I am sure of this fact as I had written that code.

I am really getting extremely annoyed at your attitude in your
conversations and accusing me. This is not nice.
My replies are based on documentation and questioning the
implementation, which is my job here. I dont care if you feel bad about
me questions. I guess you dont like that someone is questioning, but I
dont care.

-- 
~Vinod

Re: [PATCH] soundwire: stream: only change state if needed

[Vinod Koul]

On 20-03-20, 09:33, Pierre-Louis Bossart wrote:
> On 3/20/20 9:15 AM, Vinod Koul wrote:
> > On 17-03-20, 05:51, Pierre-Louis Bossart wrote:
> > > In a multi-cpu DAI context, the stream routines may be called from
> > > multiple DAI callbacks. Make sure the stream state only changes for
> > > the first call, and don't return error messages if the target state is
> > > already reached.

Again I ask you to read emails carefully and not jump the gun.

> > For stream-apis we have documented explicitly in Documentation/driver-api/soundwire/stream.rst
> > 

< begins with quote from the file stream.rst>

> > "Bus implements below API for allocate a stream which needs to be called once
> > per stream. From ASoC DPCM framework, this stream state maybe linked to
> > .startup() operation.
> > 
> > .. code-block:: c
> > 
> >    int sdw_alloc_stream(char * stream_name); "

<end quote>

> > This is documented for all stream-apis.

This line is very important. But seems to have skipped

> > This can be resolved by moving the calling of these APIs from
> > master-dais/slave-dais to machine-dais. They are unique in the card.

This is suggestion, feel free to do anything as long as that conforms to
documentation laid out, which incidentally was written by Sanyog and
signed off by you. See 89634f99a83e ("Documentation: soundwire: Add more documentation")

> this change is about prepare/enable/disable/deprepare, not allocation or
> startup.

Sad to see this. Now am going to quote for all these, since you skipped
the line in above email.

For prepare:
"Bus implements below API for PREPARE state which needs to be called
once per stream. From ASoC DPCM framework, this stream state is linked
to .prepare() operation. Since the .trigger() operations may not
follow the .prepare(), a direct transition from
``SDW_STREAM_PREPARED`` to ``SDW_STREAM_DEPREPARED`` is allowed."

See the point about "once per stream".

On Enable:
"Bus implements below API for ENABLE state which needs to be called once per
stream. From ASoC DPCM framework, this stream state is linked to
.trigger() start operation."

See the point about "once per stream".

For Disable:
"Bus implements below API for DISABLED state which needs to be called once
per stream. From ASoC DPCM framework, this stream state is linked to
.trigger() stop operation."

See the point about "once per stream".

For deprepare:
"Bus implements below API for DEPREPARED state which needs to be called
once per stream. ALSA/ASoC do not have a concept of 'deprepare', and
the mapping from this stream state to ALSA/ASoC operation may be
implementation specific."

See the point about "once per stream".

> I see no reason to burden the machine driver with all these steps. It's not
> because QCOM needs this transition that everyone does.
> 
> As discussed earlier, QCOM cannot use this functionality because the
> prepare/enable and disable/deprepare are done in the hw_params and hw_free
> respectively. This was never the intended use, but Intel let it happen so
> I'd like you to return the favor. This change has no impact for QCOM and
> simplifies the Intel solution, so why would you object?
> 
> Seriously, your replies on all Intel contributions make me wonder if this is
> the QCOM/Linaro SoundWire subsystem, or if we are going to find common
> ground to deal with vastly different underlying architectures?

It is extremely sad that you chose to say this and didn't even try to
read the email and recheck the documentation you have signed off.

This has nothing to do with Qcom/Linaro. If you look at git tree in
Intel you will find that the original implementation was to use these from
machine driver. I am sure of this fact as I had written that code.

I am really getting extremely annoyed at your attitude in your
conversations and accusing me. This is not nice.
My replies are based on documentation and questioning the
implementation, which is my job here. I dont care if you feel bad about
me questions. I guess you dont like that someone is questioning, but I
dont care.

-- 
~Vinod