Thanks for the comments!
On 02/01/18 00:45, Bjorn Andersson wrote:
On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org wrote:
[..]
+config SND_SOC_QDSP6_AFE
- tristate
- default n
Do you see a particular benefit of having one kernel module per function? Why not just compile them all into the same q6dsp.ko?
No, I do not see any benefit in doing so, I can try to make it as single module in next version.
+config SND_SOC_QDSP6
- tristate "SoC ALSA audio driver for QDSP6"
- select SND_SOC_QDSP6_AFE
- help
To add support for MSM QDSP6 Soc Audio.This will enable sound soc platform specificaudio drivers. This includes q6asm, q6adm,q6afe interfaces to DSP using apr.[..]
+struct q6afev2 {
- void *apr;
apr has a type, even if it's definition is hidden you should use the proper type here.
I agree.
- struct device *dev;
- int state;
- int status;
- struct platform_device *daidev;
- struct mutex afe_cmd_lock;
You shouldn't need to include afe_ in this name.
make sense!
- struct list_head port_list;
- spinlock_t port_list_lock;
- struct list_head node;
+};
[..]
+/* 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,Looks like you have an extra tab here, consider breaking the 80 char "rule" to not have to wrap these.
yep!
AFE_PORT_HDMI_RX, 1},+};
+static struct q6afe_port *afe_find_port(struct q6afev2 *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);
- return p;
+}
Make port_list an idr and you can just use idr_find() instead of rolling your own search function.
Will give that a go.
+static int afe_callback(struct apr_device *adev, struct apr_client_data *data) +{
- struct q6afev2 *afe = dev_get_drvdata(&adev->dev);//priv;
This is perfectly fine, no need to extend the interface with a priv (so drop the comment).
I think it was a leftover, will clean such instances.
- struct q6afe_port *port;
- if (!data) {
dev_err(afe->dev, "%s: Invalid param data\n", __func__);return -EINVAL;- }
Just define on in the apr layer that data will never be NULL, that will save you 4 lines of code in every apr client.
I agree!
- if (data->payload_size) {
uint32_t *payload = data->payload;So the payload is 2 ints, where the first is a command and the second is the status of it. This you can express in a simple struct to make the code even easier on the eye.
Will do that, if it make code more readable.
if (data->opcode == APR_BASIC_RSP_RESULT) {if (payload[1] != 0) {afe->status = payload[1];dev_err(afe->dev,"cmd = 0x%x returned error = 0x%x\n",payload[0], payload[1]);}switch (payload[0]) {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(&port->wait);break;default:dev_err(afe->dev, "Unknown cmd 0x%x\n",payload[0]);If you flip the check for payload_size to return early if there isn't a payload then you can reduce the indentation level one step and probably doesn't have to wrap this line.
yep make sense!
break;}}- }
- return 0;
+} +/**
- q6afe_get_port_id() - Get port id from a given port index
- @index: port index
- Return: Will be an negative on error or valid port_id on success
- */
+int q6afe_get_port_id(int index) +{
- if (index < 0 || index > AFE_PORT_MAX)
return -EINVAL;- return port_maps[index].port_id;
+} +EXPORT_SYMBOL_GPL(q6afe_get_port_id);
+static int afe_apr_send_pkt(struct q6afev2 *afe, void *data,
wait_queue_head_t *wait)Rather than conditionally passing the wait, split this function in afe_send_sync(*afe, *data, wait) and afe_send_async(*afe, *data).
Will do that across other modules too.
+{
- int ret;
- if (wait)
afe->state = AFE_CMD_RESP_NONE;- afe->status = 0;
- ret = apr_send_pkt(afe->apr, data);
- if (ret > 0) {
Check ret < 0 and return here, this saves you one indentation level in the following chunk.
If you then check !wait and return early you can reduce another level.
okay!
if (wait) {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",adsp_err_get_err_str(afe->status));ret = adsp_err_get_lnx_err_code(afe->status);} else {ret = 0;}} else {ret = 0;}- } else {
dev_err(afe->dev, "packet not transmitted\n");ret = -EINVAL;- }
- return ret;
+}
+static int afe_send_cmd_port_start(struct q6afe_port *port) +{
- u16 port_id = port->id;
- struct afe_port_cmd_device_start start;
- struct q6afev2 *afe = port->afe.v2;
- int ret, index;
- index = port->token;
- start.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
APR_HDR_LEN(APR_HDR_SIZE),APR_PKT_VER);- start.hdr.pkt_size = sizeof(start);
- start.hdr.src_port = 0;
- start.hdr.dest_port = 0;
- start.hdr.token = index;
Just put port->token here, saves you a local variable.
yep!
- start.hdr.opcode = AFE_PORT_CMD_DEVICE_START;
- start.port_id = port_id;
- ret = afe_apr_send_pkt(afe, &start, &port->wait);
- if (ret)
dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n",port_id, ret);- return ret;
+}
+static int afe_port_start(struct q6afe_port *port,
union afe_port_config *afe_config)+{
- struct afe_audioif_config_command config;
- struct q6afev2 *afe = port->afe.v2;
- int ret = 0;
- int port_id = port->id;
- int cfg_type;
- int index = 0;
- if (!afe_config) {
Looking at the one caller of this function, afe_config can't be NULL, so no need for this error handling.
okay.
dev_err(afe->dev, "Error, no configuration data\n");ret = -EINVAL;return ret;- }
- index = port->token;
- mutex_lock(&afe->afe_cmd_lock);
- /* Also send the topology id here: */
- config.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
APR_HDR_LEN(APR_HDR_SIZE),APR_PKT_VER);- config.hdr.pkt_size = sizeof(config);
- config.hdr.src_port = 0;
- config.hdr.dest_port = 0;
- config.hdr.token = index;
- cfg_type = port->cfg_type;
- config.hdr.opcode = AFE_PORT_CMD_SET_PARAM_V2;
- config.param.port_id = port_id;
- config.param.payload_size = sizeof(config) - sizeof(struct apr_hdr) -
sizeof(config.param);- config.param.payload_address_lsw = 0x00;
- config.param.payload_address_msw = 0x00;
- config.param.mem_map_handle = 0x00;
- config.pdata.module_id = AFE_MODULE_AUDIO_DEV_INTERFACE;
- config.pdata.param_id = cfg_type;
- config.pdata.param_size = sizeof(config.port);
This looks like a good candidate for a afe_port_set_param() function.
makes sense.
- config.port = *afe_config;
- ret = afe_apr_send_pkt(afe, &config, &port->wait);
- if (ret) {
dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n",port_id, ret);goto fail_cmd;- }
- ret = afe_send_cmd_port_start(port);
+fail_cmd:
- mutex_unlock(&afe->afe_cmd_lock);
- return ret;
+}
[..]
+/**
- q6afe_port_get_from_id() - Get port instance from a port id
- @dev: Pointer to afe child device.
- @id: port id
- Return: Will be an error pointer on error or a valid afe port
- on success.
- */
+struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id)
Will there be any other getter? Otherwise you can just call this q6afe_port_get().
There is one more get, which is basically lookup from index to port number.
+{
- int port_id;
- struct q6afev2 *afe = dev_get_drvdata(dev->parent);
- struct q6afe_port *port;
- int token;
- int cfg_type;
- if (!afe) {
dev_err(dev, "Unable to find instance of afe service\n");return ERR_PTR(-ENOENT);- }
As this comes from the passed dev this check will catch bugs withing this driver, but if the client accidentally passes the wrong dev it's likely that you don't catch it here anyways. Consider dropping the check.
yes!
- token = id;
- if (token < 0 || token > AFE_PORT_MAX) {
dev_err(dev, "AFE port token[%d] invalid!\n", token);return ERR_PTR(-EINVAL);- }
- port_id = port_maps[id].port_id;
- port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
- if (!port)
return ERR_PTR(-ENOMEM);- init_waitqueue_head(&port->wait);
- port->token = token;
- port->id = port_id;
- port->afe.v2 = afe;
- switch (port_id) {
How about moving this switch statement above the allocation?
Yes, this can be done!
- case AFE_PORT_ID_MULTICHAN_HDMI_RX:
cfg_type = AFE_PARAM_ID_HDMI_CONFIG;break;- default:
dev_err(dev, "Invalid port id 0x%x\n", port_id);return ERR_PTR(-EINVAL);- }
- port->cfg_type = cfg_type;
- spin_lock(&afe->port_list_lock);
- list_add_tail(&port->node, &afe->port_list);
- spin_unlock(&afe->port_list_lock);
- return port;
+} +EXPORT_SYMBOL_GPL(q6afe_port_get_from_id);
Regards, Bjorn