[Sound-open-firmware] [PATCH 1/5] dai: add lbm status for dai ssp

[Pan, Xiuli]

On 6/21/2018 03:58, Pierre-Louis Bossart wrote:
>
>
> On 06/20/2018 02:03 PM, Liam Girdwood wrote:
>> On Wed, 2018-06-20 at 11:40 -0500, Pierre-Louis Bossart wrote:
>>> On 6/20/18 5:43 AM, Liam Girdwood wrote:
>>>> On Wed, 2018-06-20 at 13:55 +0800, Pan, Xiuli wrote:
>>>>> ipc_dai_config is a shared structure from host to DSP, it should 
>>>>> not be
>>>>> used to save some SSP only stauts and it may also not be generic.
>>>>> This change is made for this reason as your last two comments about
>>>>> adding flag in
>>>>>
>>>>> struct sof_ipc_dai_config and struct dai with this SSP only status.
>>>> but loopback is not just for SSP, other non Intel DAIs also support 
>>>> it.
>>> Humm, not sure I agree here with the logic.
>>>
>>> Some non-Intel DAIs support LBM.
>>> One Intel DAI out of 4 - the SSP - supports LBM
>> McASP, McBSP have loopback and probably plenty of others too.
> ipc_dai_config is mostly a union today:
>
> /* general purpose DAI configuration */
> struct sof_ipc_dai_config {
>     struct sof_ipc_hdr hdr;
>     enum sof_ipc_dai_type type;
>     uint32_t id;    /* physical number if more than 1 of this type */
>
>     /* physical protocol and clocking */
>     uint16_t format;    /* SOF_DAI_FMT_ */
>     uint16_t reserved;    /* alignment */
>
>     /* HW specific data */
>     union {
>         struct sof_ipc_dai_ssp_params ssp;
>         struct sof_ipc_dai_hda_params hda;
>         struct sof_ipc_dai_dmic_params dmic;
>     };
> };
>
> we discussed earlier that the format was SSP specific, and if there 
> are additional interfaces they will have to be dealt with an extension 
> of a union.  So now I am surprised to see you add a new generic field 
> which isn't really applicable to all DAI types.
>
>>
>>> -> so what is the reason for making this field generic?
>>> It creates a case where LBM would be configurable for Intel DAIs who
>>> don't support it...
>>>
>> Not really, if the LBM flag was set for HDA in topology then it would be
>> rejected by the HDA DAI driver and passed as an error back up the stack.
> So we need an error check in the driver which parses the topology AND 
> in the firmware?
>
> I am not going to lay on the tracks but I don't see where this is 
> going. I find it simpler to just add this field when it's supported. 
> Not to mention that the addition of such generic parameters is very 
> unlikely moving forward if we have to deal with backwards compatibility.
Hi Liam/Pierre

I am quite confused now, let me show the current status and the planning:
Current:

SOF:
1.COMP DAI CMD will handle the kcontrol IO callback IPC message
2.COMP DAI CMD will call the DAI SSP ops function to set the register.
3.SSP status is stored in DAI SSP private date

SOFT:
1. Add token for dai_in widget to create kcontrol bind with COMP DAI.
2. Create loopback specific topology file for loopback usage.

KERNEL:
1. Add debugmode debugfs
2. Parse token form
3. Add kcontrol if there is config
4. only send IPC if we are in debugmode for put IO callback handler.

 From the discuss above the question now is focus on the topology tokens 
and the status store:
1. the token should be some more generic flag
2. The M4 macro should have some parameter for loopback topology
3. The SOF status should align with flag

So here are some questions:
1. What is the final exception for this feature? What other features are 
like this loopback mode?
2. What tplg files should we have? Should we enable the kcontrol as 
default if the platform support, or have some separate tplg file for 
loopback usage.
3. Where should the SSP Loopback mode kcontrol status be stored? In 
private data or in the sof_ipc_dai_config?
The sof_ipc_dai_config is some structure used for DAI config and I did 
not think it should store some firmware only status.

Thanks
Xiuli


>
>>
>> Liam
>>
>> _______________________________________________
>> Sound-open-firmware mailing list
>> Sound-open-firmware at alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
>
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware