On Fri, Mar 02, 2018 at 01:13:17PM +0000, Srinivas Kandagatla wrote:

On 01/03/18 20:59, Mark Brown wrote:

quoted

On Tue, Feb 13, 2018 at 04:58:17PM +0000, srinivas.kandagatla at linaro.org wrote:

quoted

quoted

+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},
+};

quoted

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?

quoted

quoted

+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);

quoted

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.

quoted

quoted

+int q6afe_port_start(struct q6afe_port *port)
+{
+	return afe_port_start(port, &port->port_cfg);
+}
+EXPORT_SYMBOL_GPL(q6afe_port_start);

quoted

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180302/16cd367c/attachment.sig>