Re: [PATCH v3 05/25] ASoC: qcom: qdsp6: Add support to Q6AFE
Thanks for the review comments,
On 02/03/18 17:54, Mark Brown wrote:
On Fri, Mar 02, 2018 at 01:13:17PM +, Srinivas Kandagatla wrote:
On 01/03/18 20:59, Mark Brown wrote:
On Tue, Feb 13, 2018 at 04:58:17PM +, srinivas.kandaga...@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(>port_list_lock);
+ list_for_each_entry(p, >port_list, node)
+ if (p->token == token)
+ break;
+
+ spin_unlock(>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_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
Re: [PATCH v3 05/25] ASoC: qcom: qdsp6: Add support to Q6AFE
Thanks for the review comments,
On 02/03/18 17:54, Mark Brown wrote:
On Fri, Mar 02, 2018 at 01:13:17PM +, Srinivas Kandagatla wrote:
On 01/03/18 20:59, Mark Brown wrote:
On Tue, Feb 13, 2018 at 04:58:17PM +, srinivas.kandaga...@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(>port_list_lock);
+ list_for_each_entry(p, >port_list, node)
+ if (p->token == token)
+ break;
+
+ spin_unlock(>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_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
Re: [PATCH v3 05/25] ASoC: qcom: qdsp6: Add support to Q6AFE
On Fri, Mar 02, 2018 at 01:13:17PM +, Srinivas Kandagatla wrote:
> On 01/03/18 20:59, Mark Brown wrote:
> > On Tue, Feb 13, 2018 at 04:58:17PM +, srinivas.kandaga...@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(>port_list_lock);
> > > + list_for_each_entry(p, >port_list, node)
> > > + if (p->token == token)
> > > + break;
> > > +
> > > + spin_unlock(>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_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.
signature.asc
Description: PGP signature
Re: [PATCH v3 05/25] ASoC: qcom: qdsp6: Add support to Q6AFE
On Fri, Mar 02, 2018 at 01:13:17PM +, Srinivas Kandagatla wrote:
> On 01/03/18 20:59, Mark Brown wrote:
> > On Tue, Feb 13, 2018 at 04:58:17PM +, srinivas.kandaga...@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(>port_list_lock);
> > > + list_for_each_entry(p, >port_list, node)
> > > + if (p->token == token)
> > > + break;
> > > +
> > > + spin_unlock(>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_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.
signature.asc
Description: PGP signature
Re: [PATCH v3 05/25] ASoC: qcom: qdsp6: Add support to Q6AFE
Thanks for the review comments,
On 01/03/18 20:59, Mark Brown wrote:
On Tue, Feb 13, 2018 at 04:58:17PM +, srinivas.kandaga...@linaro.org wrote:
+// SPDX-License-Identifier: GPL-2.0
+#ifndef __DT_BINDINGS_Q6_AFE_H__
+#define __DT_BINDINGS_Q6_AFE_H__
+
+/* Audio Front End (AFE) Ports */
+#define AFE_PORT_HDMI_RX 8
+
+#endif /* __DT_BINDINGS_Q6_AFE_H__ */
Please use a C++ comment here as well for consistency, it looks more
intentional.
Yep, I will fix such occurrences in next version.
+config SND_SOC_QDSP6_AFE
+ tristate
+ default n
+
n is the default anyway, no need to specify it (I know some uses already
slipped through here but it'd be better to fix those).
index ..0a5af06bb50e
--- /dev/null
+++ b/sound/soc/qcom/qdsp6/q6afe.c
@@ -0,0 +1,520 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2011-2017, The Linux Foundation
+ * Copyright (c) 2018, Linaro Limited
+ */
Yep, I will fix this, I think I picked this up variant while this was
still being discussed. I will fix all such occurrences.
Same here with the comment, just make the whole comment a C++ comment
rather than having one comment using both styles. Similar things apply
elsewhere.
+/* Port map of index vs real hw port ids */
+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.
+static struct q6afe_port *afe_find_port(struct q6afe *afe, int token)
+{
+ struct q6afe_port *p = NULL;
+
+ spin_lock(>port_list_lock);
+ list_for_each_entry(p, >port_list, node)
+ if (p->token == token)
+ break;
+
+ spin_unlock(>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.
We don't write here which suggests either there's some kind of race
condition in this lookup or the lock is not doing anything.
+static int afe_callback(struct apr_device *adev,
+ struct apr_client_message *data)
+{
+ struct q6afe *afe = dev_get_drvdata(>dev);
+ struct aprv2_ibasic_rsp_result_t *res;
+ struct q6afe_port *port;
+
+ if (!data->payload_size)
+ return 0;
+
+ res = data->payload;
+ if (data->opcode == APR_BASIC_RSP_RESULT) {
+ if (res->status) {
Shouldn't we use a switch statement here in case we want to handle
something else?
Yep, I will fix this.
+ switch (res->opcode) {
+ case AFE_PORT_CMD_SET_PARAM_V2:
+ case AFE_PORT_CMD_DEVICE_STOP:
+ case AFE_PORT_CMD_DEVICE_START:
+ afe->state = AFE_CMD_RESP_AVAIL;
+ port = afe_find_port(afe, data->token);
+ if (port)
+ wake_up(>wait);
No locking here? It seems a bit odd that the AFE global state is being
set to say there's a response available but we wake a specific port.
This callback is invoked in interrupt context and sends are serialized
at afe level.
Yep, It does not make sense to have a global state, I will fix this by
making this state specific to port.
+ ret = apr_send_pkt(afe->apr, data);
+ if (ret < 0) {
+ dev_err(afe->dev, "packet not transmitted\n");
+ ret = -EINVAL;
+ goto err;
Why squash the error code here? We don't even print it.
Will fix this in next version.
+ }
+
+ ret = wait_event_timeout(*wait, (afe->state == AFE_CMD_RESP_AVAIL),
+msecs_to_jiffies(TIMEOUT_MS));
+ if (!ret) {
+ ret = -ETIMEDOUT;
+ } else if (afe->status > 0) {
+ dev_err(afe->dev, "DSP returned error[%s]\n",
+ q6dsp_strerror(afe->status));
+ ret = q6dsp_errno(afe->status);
If we time out here and the DSP delivers a response later it looks like
we'll get data corruption - the DSP will happily deliver the response
into shared state.
If I make this state specific to port, we could handle this case much
cleanly.
+static int afe_port_start(struct q6afe_port *port,
+ union afe_port_config *afe_config)
+{
+ struct afe_audioif_config_command config = {0,};
+ struct q6afe *afe =
Re: [PATCH v3 05/25] ASoC: qcom: qdsp6: Add support to Q6AFE
Thanks for the review comments,
On 01/03/18 20:59, Mark Brown wrote:
On Tue, Feb 13, 2018 at 04:58:17PM +, srinivas.kandaga...@linaro.org wrote:
+// SPDX-License-Identifier: GPL-2.0
+#ifndef __DT_BINDINGS_Q6_AFE_H__
+#define __DT_BINDINGS_Q6_AFE_H__
+
+/* Audio Front End (AFE) Ports */
+#define AFE_PORT_HDMI_RX 8
+
+#endif /* __DT_BINDINGS_Q6_AFE_H__ */
Please use a C++ comment here as well for consistency, it looks more
intentional.
Yep, I will fix such occurrences in next version.
+config SND_SOC_QDSP6_AFE
+ tristate
+ default n
+
n is the default anyway, no need to specify it (I know some uses already
slipped through here but it'd be better to fix those).
index ..0a5af06bb50e
--- /dev/null
+++ b/sound/soc/qcom/qdsp6/q6afe.c
@@ -0,0 +1,520 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2011-2017, The Linux Foundation
+ * Copyright (c) 2018, Linaro Limited
+ */
Yep, I will fix this, I think I picked this up variant while this was
still being discussed. I will fix all such occurrences.
Same here with the comment, just make the whole comment a C++ comment
rather than having one comment using both styles. Similar things apply
elsewhere.
+/* Port map of index vs real hw port ids */
+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.
+static struct q6afe_port *afe_find_port(struct q6afe *afe, int token)
+{
+ struct q6afe_port *p = NULL;
+
+ spin_lock(>port_list_lock);
+ list_for_each_entry(p, >port_list, node)
+ if (p->token == token)
+ break;
+
+ spin_unlock(>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.
We don't write here which suggests either there's some kind of race
condition in this lookup or the lock is not doing anything.
+static int afe_callback(struct apr_device *adev,
+ struct apr_client_message *data)
+{
+ struct q6afe *afe = dev_get_drvdata(>dev);
+ struct aprv2_ibasic_rsp_result_t *res;
+ struct q6afe_port *port;
+
+ if (!data->payload_size)
+ return 0;
+
+ res = data->payload;
+ if (data->opcode == APR_BASIC_RSP_RESULT) {
+ if (res->status) {
Shouldn't we use a switch statement here in case we want to handle
something else?
Yep, I will fix this.
+ switch (res->opcode) {
+ case AFE_PORT_CMD_SET_PARAM_V2:
+ case AFE_PORT_CMD_DEVICE_STOP:
+ case AFE_PORT_CMD_DEVICE_START:
+ afe->state = AFE_CMD_RESP_AVAIL;
+ port = afe_find_port(afe, data->token);
+ if (port)
+ wake_up(>wait);
No locking here? It seems a bit odd that the AFE global state is being
set to say there's a response available but we wake a specific port.
This callback is invoked in interrupt context and sends are serialized
at afe level.
Yep, It does not make sense to have a global state, I will fix this by
making this state specific to port.
+ ret = apr_send_pkt(afe->apr, data);
+ if (ret < 0) {
+ dev_err(afe->dev, "packet not transmitted\n");
+ ret = -EINVAL;
+ goto err;
Why squash the error code here? We don't even print it.
Will fix this in next version.
+ }
+
+ ret = wait_event_timeout(*wait, (afe->state == AFE_CMD_RESP_AVAIL),
+msecs_to_jiffies(TIMEOUT_MS));
+ if (!ret) {
+ ret = -ETIMEDOUT;
+ } else if (afe->status > 0) {
+ dev_err(afe->dev, "DSP returned error[%s]\n",
+ q6dsp_strerror(afe->status));
+ ret = q6dsp_errno(afe->status);
If we time out here and the DSP delivers a response later it looks like
we'll get data corruption - the DSP will happily deliver the response
into shared state.
If I make this state specific to port, we could handle this case much
cleanly.
+static int afe_port_start(struct q6afe_port *port,
+ union afe_port_config *afe_config)
+{
+ struct afe_audioif_config_command config = {0,};
+ struct q6afe *afe =
Re: [PATCH v3 05/25] ASoC: qcom: qdsp6: Add support to Q6AFE
On Tue, Feb 13, 2018 at 04:58:17PM +, srinivas.kandaga...@linaro.org wrote:
> +// SPDX-License-Identifier: GPL-2.0
> +#ifndef __DT_BINDINGS_Q6_AFE_H__
> +#define __DT_BINDINGS_Q6_AFE_H__
> +
> +/* Audio Front End (AFE) Ports */
> +#define AFE_PORT_HDMI_RX 8
> +
> +#endif /* __DT_BINDINGS_Q6_AFE_H__ */
Please use a C++ comment here as well for consistency, it looks more
intentional.
> +config SND_SOC_QDSP6_AFE
> + tristate
> + default n
> +
n is the default anyway, no need to specify it (I know some uses already
slipped through here but it'd be better to fix those).
> index ..0a5af06bb50e
> --- /dev/null
> +++ b/sound/soc/qcom/qdsp6/q6afe.c
> @@ -0,0 +1,520 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2011-2017, The Linux Foundation
> + * Copyright (c) 2018, Linaro Limited
> + */
Same here with the comment, just make the whole comment a C++ comment
rather than having one comment using both styles. Similar things apply
elsewhere.
> +/* Port map of index vs real hw port ids */
> +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.
> +static struct q6afe_port *afe_find_port(struct q6afe *afe, int token)
> +{
> + struct q6afe_port *p = NULL;
> +
> + spin_lock(>port_list_lock);
> + list_for_each_entry(p, >port_list, node)
> + if (p->token == token)
> + break;
> +
> + spin_unlock(>port_list_lock);
Why do we need to lock the port list, what are we protecting it against?
We don't write here which suggests either there's some kind of race
condition in this lookup or the lock is not doing anything.
> +static int afe_callback(struct apr_device *adev,
> + struct apr_client_message *data)
> +{
> + struct q6afe *afe = dev_get_drvdata(>dev);
> + struct aprv2_ibasic_rsp_result_t *res;
> + struct q6afe_port *port;
> +
> + if (!data->payload_size)
> + return 0;
> +
> + res = data->payload;
> + if (data->opcode == APR_BASIC_RSP_RESULT) {
> + if (res->status) {
Shouldn't we use a switch statement here in case we want to handle
something else?
> + switch (res->opcode) {
> + case AFE_PORT_CMD_SET_PARAM_V2:
> + case AFE_PORT_CMD_DEVICE_STOP:
> + case AFE_PORT_CMD_DEVICE_START:
> + afe->state = AFE_CMD_RESP_AVAIL;
> + port = afe_find_port(afe, data->token);
> + if (port)
> + wake_up(>wait);
No locking here? It seems a bit odd that the AFE global state is being
set to say there's a response available but we wake a specific port.
> + ret = apr_send_pkt(afe->apr, data);
> + if (ret < 0) {
> + dev_err(afe->dev, "packet not transmitted\n");
> + ret = -EINVAL;
> + goto err;
Why squash the error code here? We don't even print it.
> + }
> +
> + ret = wait_event_timeout(*wait, (afe->state == AFE_CMD_RESP_AVAIL),
> + msecs_to_jiffies(TIMEOUT_MS));
> + if (!ret) {
> + ret = -ETIMEDOUT;
> + } else if (afe->status > 0) {
> + dev_err(afe->dev, "DSP returned error[%s]\n",
> +q6dsp_strerror(afe->status));
> + ret = q6dsp_errno(afe->status);
If we time out here and the DSP delivers a response later it looks like
we'll get data corruption - the DSP will happily deliver the response
into shared state.
> +static int afe_port_start(struct q6afe_port *port,
> + union afe_port_config *afe_config)
> +{
> + struct afe_audioif_config_command config = {0,};
> + struct q6afe *afe = port->afe;
> + int port_id = port->id;
> + int ret, param_id = port->cfg_type;
> +
> + config.port = *afe_config;
> +
> + ret = q6afe_port_set_param_v2(port, , param_id,
> +sizeof(*afe_config));
> + if (ret) {
> + dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n",
> + port_id, ret);
> + return ret;
> + }
> + return afe_send_cmd_port_start(port);
Why not just inline this function here? It appears to have only this
user.
> + int index = 0;
We set index to 0...
> +
> + port_id = port->id;
> + index = port->token;
...the unconditionally overwrite it?
> +/**
> + * q6afe_port_start() - Start a afe port
> + *
> + * @port: Instance of port to start
> + *
> + * Return: Will be an negative on packet size on success.
> + */
> +int q6afe_port_start(struct q6afe_port *port)
> +{
> + return afe_port_start(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
Re: [PATCH v3 05/25] ASoC: qcom: qdsp6: Add support to Q6AFE
On Tue, Feb 13, 2018 at 04:58:17PM +, srinivas.kandaga...@linaro.org wrote:
> +// SPDX-License-Identifier: GPL-2.0
> +#ifndef __DT_BINDINGS_Q6_AFE_H__
> +#define __DT_BINDINGS_Q6_AFE_H__
> +
> +/* Audio Front End (AFE) Ports */
> +#define AFE_PORT_HDMI_RX 8
> +
> +#endif /* __DT_BINDINGS_Q6_AFE_H__ */
Please use a C++ comment here as well for consistency, it looks more
intentional.
> +config SND_SOC_QDSP6_AFE
> + tristate
> + default n
> +
n is the default anyway, no need to specify it (I know some uses already
slipped through here but it'd be better to fix those).
> index ..0a5af06bb50e
> --- /dev/null
> +++ b/sound/soc/qcom/qdsp6/q6afe.c
> @@ -0,0 +1,520 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2011-2017, The Linux Foundation
> + * Copyright (c) 2018, Linaro Limited
> + */
Same here with the comment, just make the whole comment a C++ comment
rather than having one comment using both styles. Similar things apply
elsewhere.
> +/* Port map of index vs real hw port ids */
> +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.
> +static struct q6afe_port *afe_find_port(struct q6afe *afe, int token)
> +{
> + struct q6afe_port *p = NULL;
> +
> + spin_lock(>port_list_lock);
> + list_for_each_entry(p, >port_list, node)
> + if (p->token == token)
> + break;
> +
> + spin_unlock(>port_list_lock);
Why do we need to lock the port list, what are we protecting it against?
We don't write here which suggests either there's some kind of race
condition in this lookup or the lock is not doing anything.
> +static int afe_callback(struct apr_device *adev,
> + struct apr_client_message *data)
> +{
> + struct q6afe *afe = dev_get_drvdata(>dev);
> + struct aprv2_ibasic_rsp_result_t *res;
> + struct q6afe_port *port;
> +
> + if (!data->payload_size)
> + return 0;
> +
> + res = data->payload;
> + if (data->opcode == APR_BASIC_RSP_RESULT) {
> + if (res->status) {
Shouldn't we use a switch statement here in case we want to handle
something else?
> + switch (res->opcode) {
> + case AFE_PORT_CMD_SET_PARAM_V2:
> + case AFE_PORT_CMD_DEVICE_STOP:
> + case AFE_PORT_CMD_DEVICE_START:
> + afe->state = AFE_CMD_RESP_AVAIL;
> + port = afe_find_port(afe, data->token);
> + if (port)
> + wake_up(>wait);
No locking here? It seems a bit odd that the AFE global state is being
set to say there's a response available but we wake a specific port.
> + ret = apr_send_pkt(afe->apr, data);
> + if (ret < 0) {
> + dev_err(afe->dev, "packet not transmitted\n");
> + ret = -EINVAL;
> + goto err;
Why squash the error code here? We don't even print it.
> + }
> +
> + ret = wait_event_timeout(*wait, (afe->state == AFE_CMD_RESP_AVAIL),
> + msecs_to_jiffies(TIMEOUT_MS));
> + if (!ret) {
> + ret = -ETIMEDOUT;
> + } else if (afe->status > 0) {
> + dev_err(afe->dev, "DSP returned error[%s]\n",
> +q6dsp_strerror(afe->status));
> + ret = q6dsp_errno(afe->status);
If we time out here and the DSP delivers a response later it looks like
we'll get data corruption - the DSP will happily deliver the response
into shared state.
> +static int afe_port_start(struct q6afe_port *port,
> + union afe_port_config *afe_config)
> +{
> + struct afe_audioif_config_command config = {0,};
> + struct q6afe *afe = port->afe;
> + int port_id = port->id;
> + int ret, param_id = port->cfg_type;
> +
> + config.port = *afe_config;
> +
> + ret = q6afe_port_set_param_v2(port, , param_id,
> +sizeof(*afe_config));
> + if (ret) {
> + dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n",
> + port_id, ret);
> + return ret;
> + }
> + return afe_send_cmd_port_start(port);
Why not just inline this function here? It appears to have only this
user.
> + int index = 0;
We set index to 0...
> +
> + port_id = port->id;
> + index = port->token;
...the unconditionally overwrite it?
> +/**
> + * q6afe_port_start() - Start a afe port
> + *
> + * @port: Instance of port to start
> + *
> + * Return: Will be an negative on packet size on success.
> + */
> +int q6afe_port_start(struct q6afe_port *port)
> +{
> + return afe_port_start(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
