On 6/21/18 5:49 AM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 14:58 -0500, 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.
It would be a generic kcontrol flags field that will contain a flag for loopback, tristate and anything else that's non specific. I know they are not available on all, but this does save code duplication within the FW and driver since we dont have to duplicate logic around reading flags in different structures.
Part of the problem is that we bundle together DAIs that are very different in nature (SSP, DMIC, HDA, SoundWire), but indeed non-intel interfaces will be similar to SSP in terms of configuration. Similar yet different, some interfaces support different clocks for RX and TX, multiple lanes... Good luck with generic parameters...
-> 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?
No, the intention is that flags are propagated down the stack to the FW DAI driver which can then reject any incorrect request.
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.
I wont lie on the tracks either :) Please add the flags to SSP structure and if possible using some generic flag types/values would help is reusing this code for other DAIs.
You lost me here. if LBM is a generic flag it should be added in the generic part of the ipc_config, not the SSP structure.
I don't have a good feel for other generic flags, tristate maybe. Any other suggestions?
Liam
Liam
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware