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

[Liam Girdwood]

On Thu, 2018-06-21 at 15:06 +0800, Pan, Xiuli wrote:
> 
> 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?

tristate is similar, SSP supports it.

> 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.

default. 

1) apl.m4 can include ssp.m4. 

2) ssp.m4 will define a macro to build the kcontrol for LBM. 

3) This macro can then be invoked by dai.m4, if it's undefined it will not
create a kcontrol.

4) This means all Intel platforms that have SSP will include ssp.m4 and all SSP
topology users will get this kcontrol for each SSP port. 

> 3. Where should the SSP Loopback mode kcontrol status be stored? In 
> private data or in the sof_ipc_dai_config?

status is private data, topology data should not b used for runtime status.

> The sof_ipc_dai_config is some structure used for DAI config and I did 
> not think it should store some firmware only status.

Liam
> 
> 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
> 
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware