Date:   Mon, 1 Jan 2018 16:45:23 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     srinivas.kandagatla@...aro.org
Cc:     Andy Gross <andy.gross@...aro.org>,
        Mark Brown <broonie@...nel.org>, linux-arm-msm@...r.kernel.org,
        alsa-devel@...a-project.org, David Brown <david.brown@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Patrick Lai <plai@...eaurora.org>,
        Banajit Goswami <bgoswami@...eaurora.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, linux-soc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, sboyd@...eaurora.org
Subject: Re: [RESEND PATCH v2 04/15] ASoC: qcom: qdsp6: Add support to Q6AFE

On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@...aro.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?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Powered by blists - more mailing lists