Re: [PATCH v3 05/25] ASoC: qcom: qdsp6: Add support to Q6AFE

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

Thanks for the review comments,


On 02/03/18 17:54, Mark Brown wrote:
> On Fri, Mar 02, 2018 at 01:13:17PM +0000, Srinivas Kandagatla wrote:
>> On 01/03/18 20:59, Mark Brown wrote:
>>> On Tue, Feb 13, 2018 at 04:58:17PM +0000, srinivas.kandagatla@linaro.org wrote:
> 
>>>> +static struct afe_port_map port_maps[AFE_PORT_MAX] = {
>>>> +	[AFE_PORT_HDMI_RX] = { AFE_PORT_ID_MULTICHAN_HDMI_RX,
>>>> +				AFE_PORT_HDMI_RX, 1, 1},
>>>> +};
> 
>>> Is this not device specific in any way?  It looks likely to be.
> 
>> It is specific to Audio firmware build.
>> AFAIK, DSP port IDs are consistent across a given range of AVS firmware
>> builds. So far I have seen them not change in any of the B family SoCs.
>> However on older A family SOCs these are very different numbers. Which is
>> where ADSP version info would help select correct map.
> 
> Can we have some documentation of this in the code please?
> 
Sure, I will add documentation in next version.

>>>> +static struct q6afe_port *afe_find_port(struct q6afe *afe, int token)
>>>> +{
>>>> +	struct q6afe_port *p = NULL;
>>>> +
>>>> +	spin_lock(&afe->port_list_lock);
>>>> +	list_for_each_entry(p, &afe->port_list, node)
>>>> +		if (p->token == token)
>>>> +			break;
>>>> +
>>>> +	spin_unlock(&afe->port_list_lock);
> 
>>> Why do we need to lock the port list, what are we protecting it against?
> 
>> This is just to protect the list from anyone deleting this.
> 
>> Its very rare but the use case could be somelike the adsp is up and we are
>> in the middle of finding a port and then adsp went down or crashed we would
>> delete an entry in the list.
> 
> If that's what we're protecting against then this also needs to do
> something to ensure that the port we looked up doesn't get deallocated
> before or while we look at it.
Yes, I will take closer look at all possible paths before sending next 
version.
> 
>>>> +int q6afe_port_start(struct q6afe_port *port)
>>>> +{
>>>> +	return afe_port_start(port, &port->port_cfg);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(q6afe_port_start);
> 
>>> This is the third level of wrapper for the port start command in this
>>> file.  Do we *really* need all these wrappers?
> 
>> Intention here is that we have plans to support different version of ADSP,
>> on A family this command is different, so having this wrapper would help
>> tackle this use-case.
> 
> Why not just take out the level of wrapper for now then add it in when
> there's actually an abstraction in there?  The code might end up looking
> different anyway.
Okay, I can do that, will remove extra abstraction layer.

thanks,
srini
>