Message-ID: <20200323124636.GN72691@vkoul-mobl>
Date:   Mon, 23 Mar 2020 18:16:36 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        tiwai@...e.de, broonie@...nel.org, gregkh@...uxfoundation.org,
        jank@...ence.com, srinivas.kandagatla@...aro.org,
        slawomir.blauciak@...el.com,
        Bard liao <yung-chuan.liao@...ux.intel.com>,
        Rander Wang <rander.wang@...ux.intel.com>,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        Hui Wang <hui.wang@...onical.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>
Subject: Re: [PATCH] soundwire: stream: only change state if needed

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

Powered by blists - more mailing lists