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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

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?

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]