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

[Pierre-Louis Bossart]

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