diff mbox series

ASoC: qcom: add sdm845 sound card support

Message ID 1529320591-22434-1-git-send-email-rohitkr@codeaurora.org
State Superseded, archived
Headers show
Series ASoC: qcom: add sdm845 sound card support | expand

Commit Message

Rohit Kumar June 18, 2018, 11:16 a.m. UTC
This patch adds sdm845 audio machine driver support.

Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
---
 .../devicetree/bindings/sound/qcom,sdm845.txt      |  87 ++++
 sound/soc/qcom/Kconfig                             |   9 +
 sound/soc/qcom/Makefile                            |   2 +
 sound/soc/qcom/sdm845.c                            | 539 +++++++++++++++++++++
 4 files changed, 637 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,sdm845.txt
 create mode 100644 sound/soc/qcom/sdm845.c

Comments

Vinod Koul June 19, 2018, 5:05 a.m. UTC | #1
On 18-06-18, 16:46, Rohit kumar wrote:

> +struct sdm845_snd_data {
> +	struct snd_soc_card *card;
> +	struct regulator *vdd_supply;
> +	struct snd_soc_dai_link dai_link[];
> +};
> +
> +static struct mutex pri_mi2s_res_lock;
> +static struct mutex quat_tdm_res_lock;

any reason why the locks can't be part of sdm845_snd_data?
Also why do we need two locks ?

> +static atomic_t pri_mi2s_clk_count;
> +static atomic_t quat_tdm_clk_count;

Any specific reason for using atomic variables?

> +static unsigned int tdm_slot_offset[8] = {0, 4, 8, 12, 16, 20, 24, 28};
> +
> +static int sdm845_tdm_snd_hw_params(struct snd_pcm_substream *substream,
> +					struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +	int ret = 0;
> +	int channels, slot_width;
> +
> +	channels = params_channels(params);
> +	if (channels < 1 || channels > 8) {

I though ch = 0 would be caught by framework and IIRC ASoC doesn't
support more than 8 channels

> +		pr_err("%s: invalid param channels %d\n",
> +				__func__, channels);
> +		return -EINVAL;
> +	}
> +
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		slot_width = 32;
> +		break;
> +	default:
> +		pr_err("%s: invalid param format 0x%x\n",
> +				__func__, params_format(params));

why not use dev_err, bonus you get device name printer with the logs :)

> +static int sdm845_snd_startup(struct snd_pcm_substream *substream)
> +{
> +	unsigned int fmt = SND_SOC_DAIFMT_CBS_CFS;
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +
> +	pr_debug("%s: dai_id: 0x%x\n", __func__, cpu_dai->id);

It is good for debug but not very useful here, so removing it would be
good

> +	switch (cpu_dai->id) {
> +	case PRIMARY_MI2S_RX:
> +	case PRIMARY_MI2S_TX:
> +		mutex_lock(&pri_mi2s_res_lock);
> +		if (atomic_inc_return(&pri_mi2s_clk_count) == 1) {
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +				Q6AFE_LPASS_CLK_ID_MCLK_1,
> +				DEFAULT_MCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +				Q6AFE_LPASS_CLK_ID_PRI_MI2S_IBIT,
> +				DEFAULT_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> +		}
> +		mutex_unlock(&pri_mi2s_res_lock);

why do we need locking here? Can you please explain that.

> +		snd_soc_dai_set_fmt(cpu_dai, fmt);
> +		break;

empty line after break helps in readability

> +static int sdm845_sbc_parse_of(struct snd_soc_card *card)
> +{
> +	struct device *dev = card->dev;
> +	struct snd_soc_dai_link *link;
> +	struct device_node *np, *codec, *platform, *cpu, *node;
> +	int ret, num_links;
> +	struct sdm845_snd_data *data;
> +
> +	ret = snd_soc_of_parse_card_name(card, "qcom,model");
> +	if (ret) {
> +		dev_err(dev, "Error parsing card name: %d\n", ret);
> +		return ret;
> +	}
> +
> +	node = dev->of_node;
> +
> +	/* DAPM routes */
> +	if (of_property_read_bool(node, "qcom,audio-routing")) {
> +		ret = snd_soc_of_parse_audio_routing(card,
> +					"qcom,audio-routing");
> +		if (ret)
> +			return ret;
> +	}

so if we dont find audio-routing, then? we seems to continue..
Srinivas Kandagatla June 19, 2018, 8:46 a.m. UTC | #2
Thanks Rohit for the patch!

On 18/06/18 12:16, Rohit kumar wrote:
> This patch adds sdm845 audio machine driver support.
> 
> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
> ---
>   .../devicetree/bindings/sound/qcom,sdm845.txt      |  87 ++++
>   sound/soc/qcom/Kconfig                             |   9 +
>   sound/soc/qcom/Makefile                            |   2 +
>   sound/soc/qcom/sdm845.c                            | 539 +++++++++++++++++++++
>   4 files changed, 637 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/sound/qcom,sdm845.txt

Split the bindings into a separate patch!

>   create mode 100644 sound/soc/qcom/sdm845.c
> 
> diff --git a/Documentation/devicetree/bindings/sound/qcom,sdm845.txt b/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
> new file mode 100644
> index 0000000..fc0e98c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
> @@ -0,0 +1,87 @@
> +* Qualcomm Technologies Inc. SDM845 ASoC sound card driver
> +
> +This binding describes the SDM845 sound card, which uses qdsp for audio.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "qcom,sdm845-sndcard"
> +
> +- qcom,audio-routing:
> +	Usage: Optional
> +	Value type: <stringlist>
> +	Definition:  A list of the connections between audio components.
> +		  Each entry is a pair of strings, the first being the
> +		  connection's sink, the second being the connection's
> +		  source. Valid names could be power supplies, MicBias
> +		  of codec and the jacks on the board.
> +
> +- cdc-vdd-supply:
> +	Usage: Optional
> +	Value type: <phandle>
> +	Definition: phandle of regulator supply required for codec vdd.
> +
> += dailinks
> +Each subnode of sndcard represents either a dailink, and subnodes of each
> +dailinks would be cpu/codec/platform dais.
> +
> +- link-name:
> +	Usage: required
> +	Value type: <string>
> +	Definition: User friendly name for dai link
> +
> += CPU, PLATFORM, CODEC dais subnodes
> +- cpu:
> +	Usage: required
> +	Value type: <subnode>
> +	Definition: cpu dai sub-node
> +
> +- codec:
> +	Usage: required
> +	Value type: <subnode>
> +	Definition: codec dai sub-node
> +
> +- platform:
> +	Usage: opional

Optional
> +	Value type: <subnode>
> +	Definition: platform dai sub-node
> +
> +- sound-dai:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: dai phandle/s and port of CPU/CODEC/PLATFORM node.
> +
> +Example:
> +
> +audio {
> +	compatible = "qcom,sdm845-sndcard";
> +	qcom,model = "sdm845-snd-card";
> +	pinctrl-names = "pri_active", "pri_sleep";
> +	pinctrl-0 = <&pri_mi2s_active &pri_mi2s_ws_active>;
> +	pinctrl-1 = <&pri_mi2s_sleep &pri_mi2s_ws_sleep>;
> +
> +	qcom,audio-routing = "PRI_MI2S_RX Audio Mixer", "Pri-mi2s-gpio";
> +
> +	cdc-vdd-supply = <&pm8998_l14>;
> +
> +	fe@1 {
Lets not use fe or be reference here, its just a link.

> +		link-name = "MultiMedia1";
> +		cpu {
> +			sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA1>;
> +		};
> +		platform {
> +			sound-dai = <&q6asmdai>;
> +		};
asmdai has sound-cell specifier as 1, so this will dtc will throw 
warning for this.

have a look at how 8996 is done.

> +	};
> +
> +	be@1 {
> +		link-name = "PRI MI2S Playback";
> +		cpu {
> +			sound-dai = <&q6afedai PRIMARY_MI2S_RX>;
> +		};
> +
> +		platform {
> +			sound-dai = <&q6routing>;
> +		};
> +	};
> +};
> diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
> index 87838fa..09de50d 100644
> --- a/sound/soc/qcom/Kconfig
> +++ b/sound/soc/qcom/Kconfig
> @@ -90,3 +90,12 @@ config SND_SOC_MSM8996
>             Support for Qualcomm Technologies LPASS audio block in
>             APQ8096 SoC-based systems.
>             Say Y if you want to use audio device on this SoCs
> +
> +config SND_SOC_SDM845
> +	tristate "SoC Machine driver for SDM845 boards"
> +	depends on QCOM_APR
> +	select SND_SOC_QDSP6
> +	help
> +	  To add support for audio on Qualcomm Technologies Inc.
> +	  SDM845 SoC-based systems.
> +          Say Y if you want to use audio device on this SoCs
> diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
> index 206945b..ac9609e 100644
> --- a/sound/soc/qcom/Makefile
> +++ b/sound/soc/qcom/Makefile
> @@ -14,10 +14,12 @@ obj-$(CONFIG_SND_SOC_LPASS_APQ8016) += snd-soc-lpass-apq8016.o
>   snd-soc-storm-objs := storm.o
>   snd-soc-apq8016-sbc-objs := apq8016_sbc.o
>   snd-soc-apq8096-objs := apq8096.o
> +snd-soc-sdm845-objs := sdm845.o
>   
>   obj-$(CONFIG_SND_SOC_STORM) += snd-soc-storm.o
>   obj-$(CONFIG_SND_SOC_APQ8016_SBC) += snd-soc-apq8016-sbc.o
>   obj-$(CONFIG_SND_SOC_MSM8996) += snd-soc-apq8096.o
> ++obj-$(CONFIG_SND_SOC_SDM845) += snd-soc-sdm845.o

?? looks like typo here.

>   
>   #DSP lib
>   obj-$(CONFIG_SND_SOC_QDSP6) += qdsp6/
> diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
> new file mode 100644
> index 0000000..70d2611
> --- /dev/null
> +++ b/sound/soc/qcom/sdm845.c
> @@ -0,0 +1,539 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/component.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/atomic.h>

> +#include <linux/of_device.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <linux/soc/qcom/apr.h>
> +#include "qdsp6/q6afe.h"
> +
> +#define DEFAULT_SAMPLE_RATE_48K		48000
> +#define DEFAULT_MCLK_RATE		24576000
> +#define DEFAULT_BCLK_RATE		1536000
> +
> +struct sdm845_snd_data {
> +	struct snd_soc_card *card;
> +	struct regulator *vdd_supply;
> +	struct snd_soc_dai_link dai_link[];
> +};
> +
> +static struct mutex pri_mi2s_res_lock;
> +static struct mutex quat_tdm_res_lock;
> +static atomic_t pri_mi2s_clk_count;
> +static atomic_t quat_tdm_clk_count;
> +
> +static unsigned int tdm_slot_offset[8] = {0, 4, 8, 12, 16, 20, 24, 28};
> +static int sdm845_snd_hw_params(struct snd_pcm_substream *substream,
> +					struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +	int ret = 0;
> +
> +	switch (cpu_dai->id) {
> +	case QUATERNARY_TDM_RX_0:
> +	case QUATERNARY_TDM_TX_0:
> +		ret = sdm845_tdm_snd_hw_params(substream, params);
> +		break;
> +	default:
> +		pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
> +		break;
> +	}
> +	return ret;
> +}
> +
> +static int sdm845_snd_startup(struct snd_pcm_substream *substream)
> +{
> +	unsigned int fmt = SND_SOC_DAIFMT_CBS_CFS;
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +
> +	pr_debug("%s: dai_id: 0x%x\n", __func__, cpu_dai->id);
> +	switch (cpu_dai->id) {
> +	case PRIMARY_MI2S_RX:
> +	case PRIMARY_MI2S_TX:
> +		mutex_lock(&pri_mi2s_res_lock);

Mutex and atomic variables looks redundant here.
Can you explain why do you need both?
> +		if (atomic_inc_return(&pri_mi2s_clk_count) == 1) { > +			snd_soc_dai_set_sysclk(cpu_dai,
> +				Q6AFE_LPASS_CLK_ID_MCLK_1,
> +				DEFAULT_MCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +				Q6AFE_LPASS_CLK_ID_PRI_MI2S_IBIT,
> +				DEFAULT_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> +		}
> +		mutex_unlock(&pri_mi2s_res_lock);
> +		snd_soc_dai_set_fmt(cpu_dai, fmt);
> +		break;
> +	case QUATERNARY_TDM_RX_0:
> +	case QUATERNARY_TDM_TX_0:
> +		mutex_lock(&quat_tdm_res_lock);
> +		if (atomic_inc_return(&quat_tdm_clk_count) == 1) {
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +				Q6AFE_LPASS_CLK_ID_QUAD_TDM_IBIT,
> +				DEFAULT_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> +		}
> +		mutex_unlock(&quat_tdm_res_lock);
> +		break;
> +	default:
> +		pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
> +		break;
> +	}
> +	return 0;
> +}
> +

...
> +
> +static int sdm845_sbc_parse_of(struct snd_soc_card *card)
> +{
> +	struct device *dev = card->dev;
> +	struct snd_soc_dai_link *link;
> +	struct device_node *np, *codec, *platform, *cpu, *node;
> +	int ret, num_links;
> +	struct sdm845_snd_data *data;
> +
> +	ret = snd_soc_of_parse_card_name(card, "qcom,model");
> +	if (ret) {
> +		dev_err(dev, "Error parsing card name: %d\n", ret);
> +		return ret;
> +	}
> +
> +	node = dev->of_node;
> +
> +	/* DAPM routes */
> +	if (of_property_read_bool(node, "qcom,audio-routing")) {
> +		ret = snd_soc_of_parse_audio_routing(card,
> +					"qcom,audio-routing");
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Populate links */
> +	num_links = of_get_child_count(node);
> +
> +	dev_info(dev, "Found %d child audio dai links..\n", num_links);

Looks unnessary!

> +	/* Allocate the private data and the DAI link array */
> +	data = kzalloc(sizeof(*data) + sizeof(*link) * num_links,
> +			GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	card->dai_link = &data->dai_link[0];
> +	card->num_links = num_links;
> +	card->dapm_widgets = sdm845_widgets;
> +	card->num_dapm_widgets = ARRAY_SIZE(sdm845_widgets);
> +
> +	link = data->dai_link;
> +	data->card = card;
> +
> +	for_each_child_of_node(node, np) {
> +		cpu = of_get_child_by_name(np, "cpu");
> +		platform = of_get_child_by_name(np, "platform");
> +		codec = of_get_child_by_name(np, "codec");
> +
> +		if (!cpu) {
> +			dev_err(dev, "Can't find cpu DT node\n");
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +
> +		link->cpu_of_node = of_parse_phandle(cpu, "sound-dai", 0);
> +		if (!link->cpu_of_node) {
> +			dev_err(card->dev, "error getting cpu phandle\n");
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +
> +		link->platform_of_node = of_parse_phandle(platform,
> +				"sound-dai", 0);
> +		if (!link->platform_of_node) {
> +			dev_err(card->dev, "error getting platform phandle\n");
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +
> +		ret = snd_soc_of_get_dai_name(cpu, &link->cpu_dai_name);
> +		if (ret) {
> +			dev_err(card->dev, "error getting cpu dai name\n");
> +			goto fail;
> +		}
> +
> +		if (codec) {
> +			ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
> +			if (ret < 0) {
> +				dev_err(card->dev, "error getting codec dai name\n");
> +				goto fail;
> +			}
> +			link->no_pcm = 1;
> +			link->ignore_suspend = 1;
> +			link->ignore_pmdown_time = 1;
> +			link->ops = &sdm845_be_ops;
> +			link->be_hw_params_fixup = sdm845_be_hw_params_fixup;
> +		} else {
> +			link->dynamic = 1;
> +			link->codec_dai_name = "snd-soc-dummy-dai";
> +			link->codec_name = "snd-soc-dummy";
> +		}
> +

You could optimize this code, have a look at apq8096.c which does 
exactly same thing.

> +		ret = of_property_read_string(np, "link-name", &link->name);
> +		if (ret) {
> +			dev_err(card->dev,
> +				"error getting codec dai_link name\n");
> +			goto fail;
> +		}
> +
> +		link->dpcm_playback = 1;
> +		link->dpcm_capture = 1;
> +		link->stream_name = link->name;
> +		link++;
> +	}
> +	dev_set_drvdata(dev, card);
> +	snd_soc_card_set_drvdata(card, data);
> +
> +	return ret;
> +fail:
> +	kfree(data);
> +	return ret;
> +}
...
> +
> +static void sdm845_unbind(struct device *dev)
> +{
> +	struct snd_soc_card *card = dev_get_drvdata(dev);
> +	struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
> +
> +	mutex_destroy(&pri_mi2s_res_lock);
> +	mutex_destroy(&quat_tdm_res_lock);
> +	if (data->vdd_supply)
> +		regulator_put(data->vdd_supply);
> +	component_unbind_all(dev, card);
> +	snd_soc_unregister_card(card);
> +	kfree(data);
> +	kfree(card);
> +}
> +
> +static const struct component_master_ops sdm845_ops = {
> +	.bind = sdm845_bind,
> +	.unbind = sdm845_unbind,
> +};
> +
> +static int sdm845_runtime_resume(struct device *dev)
> +{
> +	struct snd_soc_card *card = dev_get_drvdata(dev);
> +	struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
> +
> +	if (!data->vdd_supply) {
> +		dev_dbg(dev, "no supplies defined\n");
> +		return 0;
> +	}
> +
> +	if (regulator_enable(data->vdd_supply))
> +		dev_err(dev, "Enable regulator supply failed\n");
> +
> +	return 0;
> +}
> +
> +static struct platform_driver sdm845_snd_driver = {
> +	.probe = sdm845_snd_platform_probe,
> +	.remove = sdm845_snd_platform_remove,
> +	.driver = {
> +		.name = "msm-snd-sdm845",
> +		.pm = &sdm845_pm_ops,
> +		.owner = THIS_MODULE,
not necessary!
> +		.of_match_table = of_match_ptr(sdm845_snd_device_id),
> +	},
> +};
> +module_platform_driver(sdm845_snd_driver);
> +
> +MODULE_DESCRIPTION("sdm845 ASoC Machine Driver");
> +MODULE_LICENSE("GPL v2");
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rohit Kumar June 19, 2018, 1:50 p.m. UTC | #3
Thanks Vinod for reviewing.


On 6/19/2018 10:35 AM, Vinod wrote:
> On 18-06-18, 16:46, Rohit kumar wrote:
>
>> +struct sdm845_snd_data {
>> +	struct snd_soc_card *card;
>> +	struct regulator *vdd_supply;
>> +	struct snd_soc_dai_link dai_link[];
>> +};
>> +
>> +static struct mutex pri_mi2s_res_lock;
>> +static struct mutex quat_tdm_res_lock;
> any reason why the locks can't be part of sdm845_snd_data?
> Also why do we need two locks ?
No specific reason, I will move it to sdm845_snd_data.
These locks are used to protect enable/disable of bit clocks. We have 
Primary MI2S RX/TX
and Quaternary TDM RX/TX interfaces. For primary mi2s rx/tx, we have 
single clock which is
synchronized with pri_mi2s_res_lock. For Quat TDM RX/TX, we are using 
quat_tdm_res_lock.
We need two locks as we are protecting two different resources.
>
>> +static atomic_t pri_mi2s_clk_count;
>> +static atomic_t quat_tdm_clk_count;
> Any specific reason for using atomic variables?
Nothing as such. As we are using mutex to synchronize, we can make it 
non- atomic.
Will do it in next-spin.
>
>> +static unsigned int tdm_slot_offset[8] = {0, 4, 8, 12, 16, 20, 24, 28};
>> +
>> +static int sdm845_tdm_snd_hw_params(struct snd_pcm_substream *substream,
>> +					struct snd_pcm_hw_params *params)
>> +{
>> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> +	int ret = 0;
>> +	int channels, slot_width;
>> +
>> +	channels = params_channels(params);
>> +	if (channels < 1 || channels > 8) {
> I though ch = 0 would be caught by framework and IIRC ASoC doesn't
> support more than 8 channels

OK. Will check and remove.
>> +		pr_err("%s: invalid param channels %d\n",
>> +				__func__, channels);
>> +		return -EINVAL;
>> +	}
>> +
>> +	switch (params_format(params)) {
>> +	case SNDRV_PCM_FORMAT_S32_LE:
>> +	case SNDRV_PCM_FORMAT_S24_LE:
>> +	case SNDRV_PCM_FORMAT_S16_LE:
>> +		slot_width = 32;
>> +		break;
>> +	default:
>> +		pr_err("%s: invalid param format 0x%x\n",
>> +				__func__, params_format(params));
> why not use dev_err, bonus you get device name printer with the logs :)

Sure. Will change it.
>> +static int sdm845_snd_startup(struct snd_pcm_substream *substream)
>> +{
>> +	unsigned int fmt = SND_SOC_DAIFMT_CBS_CFS;
>> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> +
>> +	pr_debug("%s: dai_id: 0x%x\n", __func__, cpu_dai->id);
> It is good for debug but not very useful here, so removing it would be
> good

OK
>> +	switch (cpu_dai->id) {
>> +	case PRIMARY_MI2S_RX:
>> +	case PRIMARY_MI2S_TX:
>> +		mutex_lock(&pri_mi2s_res_lock);
>> +		if (atomic_inc_return(&pri_mi2s_clk_count) == 1) {
>> +			snd_soc_dai_set_sysclk(cpu_dai,
>> +				Q6AFE_LPASS_CLK_ID_MCLK_1,
>> +				DEFAULT_MCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
>> +			snd_soc_dai_set_sysclk(cpu_dai,
>> +				Q6AFE_LPASS_CLK_ID_PRI_MI2S_IBIT,
>> +				DEFAULT_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
>> +		}
>> +		mutex_unlock(&pri_mi2s_res_lock);
> why do we need locking here? Can you please explain that.
So, we can have two usecases: one with primary mi2s rx and other with 
primary mi2s tx.
Lock is required to increment  pri_mi2s_clk_count and enable clock so 
that disable of one
usecase does not disable the clock.
>
>> +		snd_soc_dai_set_fmt(cpu_dai, fmt);
>> +		break;
> empty line after break helps in readability

Sure. Will add that change.
>> +static int sdm845_sbc_parse_of(struct snd_soc_card *card)
>> +{
>> +	struct device *dev = card->dev;
>> +	struct snd_soc_dai_link *link;
>> +	struct device_node *np, *codec, *platform, *cpu, *node;
>> +	int ret, num_links;
>> +	struct sdm845_snd_data *data;
>> +
>> +	ret = snd_soc_of_parse_card_name(card, "qcom,model");
>> +	if (ret) {
>> +		dev_err(dev, "Error parsing card name: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	node = dev->of_node;
>> +
>> +	/* DAPM routes */
>> +	if (of_property_read_bool(node, "qcom,audio-routing")) {
>> +		ret = snd_soc_of_parse_audio_routing(card,
>> +					"qcom,audio-routing");
>> +		if (ret)
>> +			return ret;
>> +	}
> so if we dont find audio-routing, then? we seems to continue..
Right. Its not mandatory to have qcom,audio-routing in device tree.

Regards,
Rohit
Rohit Kumar June 19, 2018, 1:58 p.m. UTC | #4
Thanks Srinivas for reviewing.


On 6/19/2018 2:16 PM, Srinivas Kandagatla wrote:
> Thanks Rohit for the patch!
>
> On 18/06/18 12:16, Rohit kumar wrote:
>> This patch adds sdm845 audio machine driver support.
>>
>> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
>> ---
>>   .../devicetree/bindings/sound/qcom,sdm845.txt      |  87 ++++
>>   sound/soc/qcom/Kconfig                             |   9 +
>>   sound/soc/qcom/Makefile                            |   2 +
>>   sound/soc/qcom/sdm845.c                            | 539 
>> +++++++++++++++++++++
>>   4 files changed, 637 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/sound/qcom,sdm845.txt
>
> Split the bindings into a separate patch!

Sure, will do it in next spin.
>
>>   create mode 100644 sound/soc/qcom/sdm845.c
>>
>> diff --git a/Documentation/devicetree/bindings/sound/qcom,sdm845.txt 
>> b/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
>> new file mode 100644
>> index 0000000..fc0e98c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
>> @@ -0,0 +1,87 @@
>> +* Qualcomm Technologies Inc. SDM845 ASoC sound card driver
>> +
>> +This binding describes the SDM845 sound card, which uses qdsp for 
>> audio.
>>
[..]
>> +
>> +- codec:
>> +    Usage: required
>> +    Value type: <subnode>
>> +    Definition: codec dai sub-node
>> +
>> +- platform:
>> +    Usage: opional
>
> Optional

okay
>> +    Value type: <subnode>
>> +    Definition: platform dai sub-node
>> +
>> +- sound-dai:
>> +    Usage: required
>> +    Value type: <phandle>
>> +    Definition: dai phandle/s and port of CPU/CODEC/PLATFORM node.
>> +
>> +Example:
>> +
>> +audio {
>> +    compatible = "qcom,sdm845-sndcard";
>> +    qcom,model = "sdm845-snd-card";
>> +    pinctrl-names = "pri_active", "pri_sleep";
>> +    pinctrl-0 = <&pri_mi2s_active &pri_mi2s_ws_active>;
>> +    pinctrl-1 = <&pri_mi2s_sleep &pri_mi2s_ws_sleep>;
>> +
>> +    qcom,audio-routing = "PRI_MI2S_RX Audio Mixer", "Pri-mi2s-gpio";
>> +
>> +    cdc-vdd-supply = <&pm8998_l14>;
>> +
>> +    fe@1 {
> Lets not use fe or be reference here, its just a link.
okay

>
>> +        link-name = "MultiMedia1";
>> +        cpu {
>> +            sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA1>;
>> +        };
>> +        platform {
>> +            sound-dai = <&q6asmdai>;
>> +        };
> asmdai has sound-cell specifier as 1, so this will dtc will throw 
> warning for this.
>
> have a look at how 8996 is done.

ok, sure
>
>> +    };
>> +
>> +    be@1 {
>>
[..]
>> diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
>> index 206945b..ac9609e 100644
>> --- a/sound/soc/qcom/Makefile
>> +++ b/sound/soc/qcom/Makefile
>> @@ -14,10 +14,12 @@ obj-$(CONFIG_SND_SOC_LPASS_APQ8016) += 
>> snd-soc-lpass-apq8016.o
>>   snd-soc-storm-objs := storm.o
>>   snd-soc-apq8016-sbc-objs := apq8016_sbc.o
>>   snd-soc-apq8096-objs := apq8096.o
>> +snd-soc-sdm845-objs := sdm845.o
>>     obj-$(CONFIG_SND_SOC_STORM) += snd-soc-storm.o
>>   obj-$(CONFIG_SND_SOC_APQ8016_SBC) += snd-soc-apq8016-sbc.o
>>   obj-$(CONFIG_SND_SOC_MSM8996) += snd-soc-apq8096.o
>> ++obj-$(CONFIG_SND_SOC_SDM845) += snd-soc-sdm845.o
>
> ?? looks like typo here.
>
Right. Will update.
>
>>     #DSP lib
>>   obj-$(CONFIG_SND_SOC_QDSP6) += qdsp6/
>> diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
>> new file mode 100644
>> index 0000000..70d2611
>> --- /dev/null
>> +++ b/sound/soc/qcom/sdm845.c
>> @@ -0,0 +1,539 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/component.h>
[..]
>> +}
>> +
>> +static int sdm845_snd_startup(struct snd_pcm_substream *substream)
>> +{
>> +    unsigned int fmt = SND_SOC_DAIFMT_CBS_CFS;
>> +    struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +    struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> +
>> +    pr_debug("%s: dai_id: 0x%x\n", __func__, cpu_dai->id);
>> +    switch (cpu_dai->id) {
>> +    case PRIMARY_MI2S_RX:
>> +    case PRIMARY_MI2S_TX:
>> +        mutex_lock(&pri_mi2s_res_lock);
>
> Mutex and atomic variables looks redundant here.
> Can you explain why do you need both?
Right. Only mutex is required here. I will make count as non-atomic.

>
> ...
>> +
>> +static int sdm845_sbc_parse_of(struct snd_soc_card *card)
>> +{
>> +    struct device *dev = card->dev;
>> +    struct snd_soc_dai_link *link;
>> +    struct device_node *np, *codec, *platform, *cpu, *node;
>> +    int ret, num_links;
>> +    struct sdm845_snd_data *data;
>> +
>> +    ret = snd_soc_of_parse_card_name(card, "qcom,model");
>> +    if (ret) {
>> +        dev_err(dev, "Error parsing card name: %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    node = dev->of_node;
>> +
>> +    /* DAPM routes */
>> +    if (of_property_read_bool(node, "qcom,audio-routing")) {
>> +        ret = snd_soc_of_parse_audio_routing(card,
>> +                    "qcom,audio-routing");
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    /* Populate links */
>> +    num_links = of_get_child_count(node);
>> +
>> +    dev_info(dev, "Found %d child audio dai links..\n", num_links);
>
> Looks unnessary!

Ok . Will remove it in next patchset.
>
>> +
>> +        link->platform_of_node = of_parse_phandle(platform,
>> +                "sound-dai", 0);
>> +        if (!link->platform_of_node) {
>> +            dev_err(card->dev, "error getting platform phandle\n");
>> +            ret = -EINVAL;
>> +            goto fail;
>> +        }
>> +
>> +        ret = snd_soc_of_get_dai_name(cpu, &link->cpu_dai_name);
>> +        if (ret) {
>> +            dev_err(card->dev, "error getting cpu dai name\n");
>> +            goto fail;
>> +        }
>> +
>> +        if (codec) {
>> +            ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
>> +            if (ret < 0) {
>> +                dev_err(card->dev, "error getting codec dai name\n");
>> +                goto fail;
>> +            }
>> +            link->no_pcm = 1;
>> +            link->ignore_suspend = 1;
>> +            link->ignore_pmdown_time = 1;
>> +            link->ops = &sdm845_be_ops;
>> +            link->be_hw_params_fixup = sdm845_be_hw_params_fixup;
>> +        } else {
>> +            link->dynamic = 1;
>> +            link->codec_dai_name = "snd-soc-dummy-dai";
>> +            link->codec_name = "snd-soc-dummy";
>> +        }
>> +
>
> You could optimize this code, have a look at apq8096.c which does 
> exactly same thing.
>

Okay. will check and update.
> ...
>> +
>> +static void sdm845_unbind(struct device *dev)
>> +{
>> +    struct snd_soc_card *card = dev_get_drvdata(dev);
>> +    struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
>> +
>> +    mutex_destroy(&pri_mi2s_res_lock);
>> +    mutex_destroy(&quat_tdm_res_lock);
>> +    if (data->vdd_supply)
>> +        regulator_put(data->vdd_supply);
>> +    component_unbind_all(dev, card);
>> +    snd_soc_unregister_card(card);
>> +    kfree(data);
>> +    kfree(card);
>> +}
>> +
>> +static const struct component_master_ops sdm845_ops = {
>> +    .bind = sdm845_bind,
>> +    .unbind = sdm845_unbind,
>> +};
>> +
>> +static int sdm845_runtime_resume(struct device *dev)
>> +{
>> +    struct snd_soc_card *card = dev_get_drvdata(dev);
>> +    struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
>> +
>> +    if (!data->vdd_supply) {
>> +        dev_dbg(dev, "no supplies defined\n");
>> +        return 0;
>> +    }
>> +
>> +    if (regulator_enable(data->vdd_supply))
>> +        dev_err(dev, "Enable regulator supply failed\n");
>> +
>> +    return 0;
>> +}
>> +
>> +static struct platform_driver sdm845_snd_driver = {
>> +    .probe = sdm845_snd_platform_probe,
>> +    .remove = sdm845_snd_platform_remove,
>> +    .driver = {
>> +        .name = "msm-snd-sdm845",
>> +        .pm = &sdm845_pm_ops,
>> +        .owner = THIS_MODULE,
> not necessary!

Okay. Will remove it.
>> +        .of_match_table = of_match_ptr(sdm845_snd_device_id),
>> +    },
>> +};
>> +module_platform_driver(sdm845_snd_driver);
>> +
>> +MODULE_DESCRIPTION("sdm845 ASoC Machine Driver");
>> +MODULE_LICENSE("GPL v2");
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Regards,
Rohit
Vinod Koul June 19, 2018, 4:22 p.m. UTC | #5
Hi Rohit,

On 19-06-18, 19:20, Rohit Kumar wrote:
> On 6/19/2018 10:35 AM, Vinod wrote:
> > On 18-06-18, 16:46, Rohit kumar wrote:
> > 
> > > +struct sdm845_snd_data {
> > > +	struct snd_soc_card *card;
> > > +	struct regulator *vdd_supply;
> > > +	struct snd_soc_dai_link dai_link[];
> > > +};
> > > +
> > > +static struct mutex pri_mi2s_res_lock;
> > > +static struct mutex quat_tdm_res_lock;
> > any reason why the locks can't be part of sdm845_snd_data?
> > Also why do we need two locks ?
> No specific reason, I will move it to sdm845_snd_data.
> These locks are used to protect enable/disable of bit clocks. We have
> Primary MI2S RX/TX
> and Quaternary TDM RX/TX interfaces. For primary mi2s rx/tx, we have single
> clock which is
> synchronized with pri_mi2s_res_lock. For Quat TDM RX/TX, we are using
> quat_tdm_res_lock.
> We need two locks as we are protecting two different resources.

I think bigger question is why do you need any locks? What is the race
scenario you envision which needs protection
Vinod Koul June 20, 2018, 9:31 a.m. UTC | #6
Hi Rohit,

On 20-06-18, 13:07, Rohit Kumar wrote:
> > On 19-06-18, 19:20, Rohit Kumar wrote:
> > > On 6/19/2018 10:35 AM, Vinod wrote:
> > > > On 18-06-18, 16:46, Rohit kumar wrote:
> > > > 
> > > > > +struct sdm845_snd_data {
> > > > > +	struct snd_soc_card *card;
> > > > > +	struct regulator *vdd_supply;
> > > > > +	struct snd_soc_dai_link dai_link[];
> > > > > +};
> > > > > +
> > > > > +static struct mutex pri_mi2s_res_lock;
> > > > > +static struct mutex quat_tdm_res_lock;
> > > > any reason why the locks can't be part of sdm845_snd_data?
> > > > Also why do we need two locks ?
> > > No specific reason, I will move it to sdm845_snd_data.
> > > These locks are used to protect enable/disable of bit clocks. We have
> > > Primary MI2S RX/TX
> > > and Quaternary TDM RX/TX interfaces. For primary mi2s rx/tx, we have single
> > > clock which is
> > > synchronized with pri_mi2s_res_lock. For Quat TDM RX/TX, we are using
> > > quat_tdm_res_lock.
> > > We need two locks as we are protecting two different resources.
> > I think bigger question is why do you need any locks? What is the race
> > scenario you envision which needs protection
> > 
> 
> Below is one of the race condition:
> 
> Thread1                          |         Thread2
> ----------------------------------------------------------
> startup()                         |
> count++;                         |    startup()
> read count (count = 1) |
> enable_clock()               |    count++;   //count = 2
> shutdown()                    |
> count--;// count = 1      |
>                                          | read count (count = 1)
>                                          | enable_clock()
> 
> Here clock will be enabled twice but disable will be called only once when
> count = 0.
> 
> This will make the clock always enabled. So, I think we should keep either
> mutex lock or atomic variable to synchronize this.

we are using DPCM here right?
Srinivas Kandagatla June 20, 2018, 9:53 a.m. UTC | #7
On 20/06/18 10:31, Vinod wrote:
>> Here clock will be enabled twice but disable will be called only once when
>> count = 0.
>>
>> This will make the clock always enabled. So, I think we should keep either
>> mutex lock or atomic variable to synchronize this.
> we are using DPCM here right?

We should probably model this clk as DAPM widget so we do not need to 
handle this in machine code.

--srini
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul June 20, 2018, 10:28 a.m. UTC | #8
On 20-06-18, 10:53, Srinivas Kandagatla wrote:
> On 20/06/18 10:31, Vinod wrote:
> > > Here clock will be enabled twice but disable will be called only once when
> > > count = 0.
> > > 
> > > This will make the clock always enabled. So, I think we should keep either
> > > mutex lock or atomic variable to synchronize this.
> > we are using DPCM here right?
> 
> We should probably model this clk as DAPM widget so we do not need to handle
> this in machine code.

Sure that would be even better, but my point was that we need not worry
about .startup racing in case of DPCM as it holds a card mutex before it
calls PCM ops.. so it is already serialized..
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/qcom,sdm845.txt b/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
new file mode 100644
index 0000000..fc0e98c
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
@@ -0,0 +1,87 @@ 
+* Qualcomm Technologies Inc. SDM845 ASoC sound card driver
+
+This binding describes the SDM845 sound card, which uses qdsp for audio.
+
+- compatible:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "qcom,sdm845-sndcard"
+
+- qcom,audio-routing:
+	Usage: Optional
+	Value type: <stringlist>
+	Definition:  A list of the connections between audio components.
+		  Each entry is a pair of strings, the first being the
+		  connection's sink, the second being the connection's
+		  source. Valid names could be power supplies, MicBias
+		  of codec and the jacks on the board.
+
+- cdc-vdd-supply:
+	Usage: Optional
+	Value type: <phandle>
+	Definition: phandle of regulator supply required for codec vdd.
+
+= dailinks
+Each subnode of sndcard represents either a dailink, and subnodes of each
+dailinks would be cpu/codec/platform dais.
+
+- link-name:
+	Usage: required
+	Value type: <string>
+	Definition: User friendly name for dai link
+
+= CPU, PLATFORM, CODEC dais subnodes
+- cpu:
+	Usage: required
+	Value type: <subnode>
+	Definition: cpu dai sub-node
+
+- codec:
+	Usage: required
+	Value type: <subnode>
+	Definition: codec dai sub-node
+
+- platform:
+	Usage: opional
+	Value type: <subnode>
+	Definition: platform dai sub-node
+
+- sound-dai:
+	Usage: required
+	Value type: <phandle>
+	Definition: dai phandle/s and port of CPU/CODEC/PLATFORM node.
+
+Example:
+
+audio {
+	compatible = "qcom,sdm845-sndcard";
+	qcom,model = "sdm845-snd-card";
+	pinctrl-names = "pri_active", "pri_sleep";
+	pinctrl-0 = <&pri_mi2s_active &pri_mi2s_ws_active>;
+	pinctrl-1 = <&pri_mi2s_sleep &pri_mi2s_ws_sleep>;
+
+	qcom,audio-routing = "PRI_MI2S_RX Audio Mixer", "Pri-mi2s-gpio";
+
+	cdc-vdd-supply = <&pm8998_l14>;
+
+	fe@1 {
+		link-name = "MultiMedia1";
+		cpu {
+			sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA1>;
+		};
+		platform {
+			sound-dai = <&q6asmdai>;
+		};
+	};
+
+	be@1 {
+		link-name = "PRI MI2S Playback";
+		cpu {
+			sound-dai = <&q6afedai PRIMARY_MI2S_RX>;
+		};
+
+		platform {
+			sound-dai = <&q6routing>;
+		};
+	};
+};
diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
index 87838fa..09de50d 100644
--- a/sound/soc/qcom/Kconfig
+++ b/sound/soc/qcom/Kconfig
@@ -90,3 +90,12 @@  config SND_SOC_MSM8996
           Support for Qualcomm Technologies LPASS audio block in
           APQ8096 SoC-based systems.
           Say Y if you want to use audio device on this SoCs
+
+config SND_SOC_SDM845
+	tristate "SoC Machine driver for SDM845 boards"
+	depends on QCOM_APR
+	select SND_SOC_QDSP6
+	help
+	  To add support for audio on Qualcomm Technologies Inc.
+	  SDM845 SoC-based systems.
+          Say Y if you want to use audio device on this SoCs
diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
index 206945b..ac9609e 100644
--- a/sound/soc/qcom/Makefile
+++ b/sound/soc/qcom/Makefile
@@ -14,10 +14,12 @@  obj-$(CONFIG_SND_SOC_LPASS_APQ8016) += snd-soc-lpass-apq8016.o
 snd-soc-storm-objs := storm.o
 snd-soc-apq8016-sbc-objs := apq8016_sbc.o
 snd-soc-apq8096-objs := apq8096.o
+snd-soc-sdm845-objs := sdm845.o
 
 obj-$(CONFIG_SND_SOC_STORM) += snd-soc-storm.o
 obj-$(CONFIG_SND_SOC_APQ8016_SBC) += snd-soc-apq8016-sbc.o
 obj-$(CONFIG_SND_SOC_MSM8996) += snd-soc-apq8096.o
++obj-$(CONFIG_SND_SOC_SDM845) += snd-soc-sdm845.o
 
 #DSP lib
 obj-$(CONFIG_SND_SOC_QDSP6) += qdsp6/
diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
new file mode 100644
index 0000000..70d2611
--- /dev/null
+++ b/sound/soc/qcom/sdm845.c
@@ -0,0 +1,539 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/component.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/atomic.h>
+#include <linux/of_device.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <linux/soc/qcom/apr.h>
+#include "qdsp6/q6afe.h"
+
+#define DEFAULT_SAMPLE_RATE_48K		48000
+#define DEFAULT_MCLK_RATE		24576000
+#define DEFAULT_BCLK_RATE		1536000
+
+struct sdm845_snd_data {
+	struct snd_soc_card *card;
+	struct regulator *vdd_supply;
+	struct snd_soc_dai_link dai_link[];
+};
+
+static struct mutex pri_mi2s_res_lock;
+static struct mutex quat_tdm_res_lock;
+static atomic_t pri_mi2s_clk_count;
+static atomic_t quat_tdm_clk_count;
+
+static unsigned int tdm_slot_offset[8] = {0, 4, 8, 12, 16, 20, 24, 28};
+
+static int sdm845_tdm_snd_hw_params(struct snd_pcm_substream *substream,
+					struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	int ret = 0;
+	int channels, slot_width;
+
+	channels = params_channels(params);
+	if (channels < 1 || channels > 8) {
+		pr_err("%s: invalid param channels %d\n",
+				__func__, channels);
+		return -EINVAL;
+	}
+
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S32_LE:
+	case SNDRV_PCM_FORMAT_S24_LE:
+	case SNDRV_PCM_FORMAT_S16_LE:
+		slot_width = 32;
+		break;
+	default:
+		pr_err("%s: invalid param format 0x%x\n",
+				__func__, params_format(params));
+		return -EINVAL;
+	}
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0x3,
+				channels, slot_width);
+		if (ret < 0) {
+			pr_err("%s: failed to set tdm slot, err:%d\n",
+					__func__, ret);
+			goto end;
+		}
+
+		ret = snd_soc_dai_set_channel_map(cpu_dai, 0, NULL,
+				channels, tdm_slot_offset);
+		if (ret < 0) {
+			pr_err("%s: failed to set channel map, err:%d\n",
+					__func__, ret);
+			goto end;
+		}
+	} else {
+		ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0xf, 0,
+				channels, slot_width);
+		if (ret < 0) {
+			pr_err("%s: failed to set tdm slot, err:%d\n",
+					__func__, ret);
+			goto end;
+		}
+
+		ret = snd_soc_dai_set_channel_map(cpu_dai, channels,
+				tdm_slot_offset, 0, NULL);
+		if (ret < 0) {
+			pr_err("%s: failed to set channel map, err:%d\n",
+					__func__, ret);
+			goto end;
+		}
+	}
+end:
+	return ret;
+}
+
+static int sdm845_snd_hw_params(struct snd_pcm_substream *substream,
+					struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	int ret = 0;
+
+	switch (cpu_dai->id) {
+	case QUATERNARY_TDM_RX_0:
+	case QUATERNARY_TDM_TX_0:
+		ret = sdm845_tdm_snd_hw_params(substream, params);
+		break;
+	default:
+		pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
+		break;
+	}
+	return ret;
+}
+
+static int sdm845_snd_startup(struct snd_pcm_substream *substream)
+{
+	unsigned int fmt = SND_SOC_DAIFMT_CBS_CFS;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+
+	pr_debug("%s: dai_id: 0x%x\n", __func__, cpu_dai->id);
+	switch (cpu_dai->id) {
+	case PRIMARY_MI2S_RX:
+	case PRIMARY_MI2S_TX:
+		mutex_lock(&pri_mi2s_res_lock);
+		if (atomic_inc_return(&pri_mi2s_clk_count) == 1) {
+			snd_soc_dai_set_sysclk(cpu_dai,
+				Q6AFE_LPASS_CLK_ID_MCLK_1,
+				DEFAULT_MCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
+			snd_soc_dai_set_sysclk(cpu_dai,
+				Q6AFE_LPASS_CLK_ID_PRI_MI2S_IBIT,
+				DEFAULT_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
+		}
+		mutex_unlock(&pri_mi2s_res_lock);
+		snd_soc_dai_set_fmt(cpu_dai, fmt);
+		break;
+	case QUATERNARY_TDM_RX_0:
+	case QUATERNARY_TDM_TX_0:
+		mutex_lock(&quat_tdm_res_lock);
+		if (atomic_inc_return(&quat_tdm_clk_count) == 1) {
+			snd_soc_dai_set_sysclk(cpu_dai,
+				Q6AFE_LPASS_CLK_ID_QUAD_TDM_IBIT,
+				DEFAULT_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
+		}
+		mutex_unlock(&quat_tdm_res_lock);
+		break;
+	default:
+		pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
+		break;
+	}
+	return 0;
+}
+
+static void  sdm845_snd_shutdown(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+
+	pr_debug("%s: dai_id: 0x%x\n", __func__, cpu_dai->id);
+	switch (cpu_dai->id) {
+	case PRIMARY_MI2S_RX:
+	case PRIMARY_MI2S_TX:
+		mutex_lock(&pri_mi2s_res_lock);
+		if (!atomic_dec_return(&pri_mi2s_clk_count)) {
+			snd_soc_dai_set_sysclk(cpu_dai,
+				Q6AFE_LPASS_CLK_ID_MCLK_1,
+				0, SNDRV_PCM_STREAM_PLAYBACK);
+			snd_soc_dai_set_sysclk(cpu_dai,
+				Q6AFE_LPASS_CLK_ID_PRI_MI2S_IBIT,
+				0, SNDRV_PCM_STREAM_PLAYBACK);
+		};
+		mutex_unlock(&pri_mi2s_res_lock);
+		break;
+	case QUATERNARY_TDM_RX_0:
+	case QUATERNARY_TDM_TX_0:
+		mutex_lock(&quat_tdm_res_lock);
+		if (!atomic_dec_return(&quat_tdm_clk_count)) {
+			snd_soc_dai_set_sysclk(cpu_dai,
+				Q6AFE_LPASS_CLK_ID_QUAD_TDM_IBIT,
+				0, SNDRV_PCM_STREAM_PLAYBACK);
+		}
+		mutex_unlock(&quat_tdm_res_lock);
+		break;
+	default:
+		pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
+		break;
+	}
+}
+
+static struct snd_soc_ops sdm845_be_ops = {
+	.hw_params = sdm845_snd_hw_params,
+	.startup = sdm845_snd_startup,
+	.shutdown = sdm845_snd_shutdown,
+};
+
+static int sdm845_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
+				struct snd_pcm_hw_params *params)
+{
+	struct snd_interval *rate = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_RATE);
+	struct snd_interval *channels = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_CHANNELS);
+
+	rate->min = rate->max = DEFAULT_SAMPLE_RATE_48K;
+	channels->min = channels->max = 2;
+
+	return 0;
+}
+
+static const struct snd_soc_dapm_widget sdm845_widgets[] = {
+	SND_SOC_DAPM_PINCTRL("Pri-mi2s-gpio", "pri_active", "pri_sleep"),
+	SND_SOC_DAPM_PINCTRL("Quat-tdm-gpio", "quat_active", "quat_sleep"),
+};
+
+static int sdm845_sbc_parse_of(struct snd_soc_card *card)
+{
+	struct device *dev = card->dev;
+	struct snd_soc_dai_link *link;
+	struct device_node *np, *codec, *platform, *cpu, *node;
+	int ret, num_links;
+	struct sdm845_snd_data *data;
+
+	ret = snd_soc_of_parse_card_name(card, "qcom,model");
+	if (ret) {
+		dev_err(dev, "Error parsing card name: %d\n", ret);
+		return ret;
+	}
+
+	node = dev->of_node;
+
+	/* DAPM routes */
+	if (of_property_read_bool(node, "qcom,audio-routing")) {
+		ret = snd_soc_of_parse_audio_routing(card,
+					"qcom,audio-routing");
+		if (ret)
+			return ret;
+	}
+
+	/* Populate links */
+	num_links = of_get_child_count(node);
+
+	dev_info(dev, "Found %d child audio dai links..\n", num_links);
+	/* Allocate the private data and the DAI link array */
+	data = kzalloc(sizeof(*data) + sizeof(*link) * num_links,
+			GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	card->dai_link = &data->dai_link[0];
+	card->num_links = num_links;
+	card->dapm_widgets = sdm845_widgets;
+	card->num_dapm_widgets = ARRAY_SIZE(sdm845_widgets);
+
+	link = data->dai_link;
+	data->card = card;
+
+	for_each_child_of_node(node, np) {
+		cpu = of_get_child_by_name(np, "cpu");
+		platform = of_get_child_by_name(np, "platform");
+		codec = of_get_child_by_name(np, "codec");
+
+		if (!cpu) {
+			dev_err(dev, "Can't find cpu DT node\n");
+			ret = -EINVAL;
+			goto fail;
+		}
+
+		link->cpu_of_node = of_parse_phandle(cpu, "sound-dai", 0);
+		if (!link->cpu_of_node) {
+			dev_err(card->dev, "error getting cpu phandle\n");
+			ret = -EINVAL;
+			goto fail;
+		}
+
+		link->platform_of_node = of_parse_phandle(platform,
+				"sound-dai", 0);
+		if (!link->platform_of_node) {
+			dev_err(card->dev, "error getting platform phandle\n");
+			ret = -EINVAL;
+			goto fail;
+		}
+
+		ret = snd_soc_of_get_dai_name(cpu, &link->cpu_dai_name);
+		if (ret) {
+			dev_err(card->dev, "error getting cpu dai name\n");
+			goto fail;
+		}
+
+		if (codec) {
+			ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
+			if (ret < 0) {
+				dev_err(card->dev, "error getting codec dai name\n");
+				goto fail;
+			}
+			link->no_pcm = 1;
+			link->ignore_suspend = 1;
+			link->ignore_pmdown_time = 1;
+			link->ops = &sdm845_be_ops;
+			link->be_hw_params_fixup = sdm845_be_hw_params_fixup;
+		} else {
+			link->dynamic = 1;
+			link->codec_dai_name = "snd-soc-dummy-dai";
+			link->codec_name = "snd-soc-dummy";
+		}
+
+		ret = of_property_read_string(np, "link-name", &link->name);
+		if (ret) {
+			dev_err(card->dev,
+				"error getting codec dai_link name\n");
+			goto fail;
+		}
+
+		link->dpcm_playback = 1;
+		link->dpcm_capture = 1;
+		link->stream_name = link->name;
+		link++;
+	}
+	dev_set_drvdata(dev, card);
+	snd_soc_card_set_drvdata(card, data);
+
+	return ret;
+fail:
+	kfree(data);
+	return ret;
+}
+
+static void sdm845_init_supplies(struct device *dev)
+{
+	struct snd_soc_card *card = dev_get_drvdata(dev);
+	struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
+
+	data->vdd_supply = regulator_get(dev, "cdc-vdd");
+	if (IS_ERR(data->vdd_supply)) {
+		dev_err(dev, "Unable to get regulator supplies\n");
+		data->vdd_supply = NULL;
+		return;
+	}
+
+	if (regulator_enable(data->vdd_supply))
+		dev_err(dev, "Unable to enable vdd supply\n");
+}
+
+static void sdm845_deinit_supplies(struct device *dev)
+{
+	struct snd_soc_card *card = dev_get_drvdata(dev);
+	struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
+
+	if (!data->vdd_supply)
+		return;
+
+	regulator_disable(data->vdd_supply);
+	regulator_put(data->vdd_supply);
+}
+
+static int sdm845_bind(struct device *dev)
+{
+	struct snd_soc_card *card;
+	int ret;
+
+	card = kzalloc(sizeof(*card), GFP_KERNEL);
+	if (!card)
+		return -ENOMEM;
+
+	ret = component_bind_all(dev, card);
+	if (ret) {
+		dev_err(dev, "Audio components bind failed: %d\n", ret);
+		goto bind_fail;
+	}
+
+	card->dev = dev;
+	ret = sdm845_sbc_parse_of(card);
+	if (ret) {
+		dev_err(dev, "Error parsing OF data\n");
+		goto parse_dt_fail;
+	}
+	sdm845_init_supplies(dev);
+
+	mutex_init(&pri_mi2s_res_lock);
+	mutex_init(&quat_tdm_res_lock);
+	atomic_set(&pri_mi2s_clk_count, 0);
+	atomic_set(&quat_tdm_clk_count, 0);
+	ret = snd_soc_register_card(card);
+	if (ret) {
+		dev_err(dev, "Sound card registration failed\n");
+		goto register_card_fail;
+	}
+	return ret;
+
+register_card_fail:
+	mutex_destroy(&pri_mi2s_res_lock);
+	mutex_destroy(&quat_tdm_res_lock);
+	sdm845_deinit_supplies(dev);
+	kfree(snd_soc_card_get_drvdata(card));
+parse_dt_fail:
+	component_unbind_all(dev, card);
+bind_fail:
+	kfree(card);
+	return ret;
+}
+
+static void sdm845_unbind(struct device *dev)
+{
+	struct snd_soc_card *card = dev_get_drvdata(dev);
+	struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
+
+	mutex_destroy(&pri_mi2s_res_lock);
+	mutex_destroy(&quat_tdm_res_lock);
+	if (data->vdd_supply)
+		regulator_put(data->vdd_supply);
+	component_unbind_all(dev, card);
+	snd_soc_unregister_card(card);
+	kfree(data);
+	kfree(card);
+}
+
+static const struct component_master_ops sdm845_ops = {
+	.bind = sdm845_bind,
+	.unbind = sdm845_unbind,
+};
+
+static int sdm845_runtime_resume(struct device *dev)
+{
+	struct snd_soc_card *card = dev_get_drvdata(dev);
+	struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
+
+	if (!data->vdd_supply) {
+		dev_dbg(dev, "no supplies defined\n");
+		return 0;
+	}
+
+	if (regulator_enable(data->vdd_supply))
+		dev_err(dev, "Enable regulator supply failed\n");
+
+	return 0;
+}
+
+static int sdm845_runtime_suspend(struct device *dev)
+{
+	struct snd_soc_card *card = dev_get_drvdata(dev);
+	struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
+
+	if (!data->vdd_supply) {
+		dev_dbg(dev, "no supplies defined\n");
+		return 0;
+	}
+
+	if (regulator_disable(data->vdd_supply))
+		dev_err(dev, "Disable regulator supply failed\n");
+
+	return 0;
+}
+
+static const struct dev_pm_ops sdm845_pm_ops = {
+	SET_RUNTIME_PM_OPS(sdm845_runtime_suspend,
+			sdm845_runtime_resume, NULL)
+};
+
+static int sdm845_compare_of(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static void sdm845_release_of(struct device *dev, void *data)
+{
+	of_node_put(data);
+}
+
+static int add_audio_components(struct device *dev,
+				struct component_match **matchptr)
+{
+	struct device_node *np, *platform, *cpu, *node, *dai_node;
+
+	node = dev->of_node;
+
+	for_each_child_of_node(node, np) {
+		cpu = of_get_child_by_name(np, "cpu");
+		if (cpu) {
+			dai_node = of_parse_phandle(cpu, "sound-dai", 0);
+			of_node_get(dai_node);
+			component_match_add_release(dev, matchptr,
+					sdm845_release_of,
+					sdm845_compare_of,
+					dai_node);
+		}
+
+		platform = of_get_child_by_name(np, "platform");
+		if (platform) {
+			dai_node = of_parse_phandle(platform, "sound-dai", 0);
+			component_match_add_release(dev, matchptr,
+					sdm845_release_of,
+					sdm845_compare_of,
+					dai_node);
+		}
+	}
+
+	return 0;
+}
+
+static int sdm845_snd_platform_probe(struct platform_device *pdev)
+{
+	struct component_match *match = NULL;
+	int ret;
+
+	ret = add_audio_components(&pdev->dev, &match);
+	if (ret)
+		return ret;
+
+	return component_master_add_with_match(&pdev->dev, &sdm845_ops, match);
+}
+
+static int sdm845_snd_platform_remove(struct platform_device *pdev)
+{
+	component_master_del(&pdev->dev, &sdm845_ops);
+	return 0;
+}
+
+static const struct of_device_id sdm845_snd_device_id[]  = {
+	{ .compatible = "qcom,sdm845-sndcard" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sdm845_snd_device_id);
+
+static struct platform_driver sdm845_snd_driver = {
+	.probe = sdm845_snd_platform_probe,
+	.remove = sdm845_snd_platform_remove,
+	.driver = {
+		.name = "msm-snd-sdm845",
+		.pm = &sdm845_pm_ops,
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(sdm845_snd_device_id),
+	},
+};
+module_platform_driver(sdm845_snd_driver);
+
+MODULE_DESCRIPTION("sdm845 ASoC Machine Driver");
+MODULE_LICENSE("GPL v2");