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

[Srinivas Kandagatla]

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