[PATCH 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
Add devicetree bindings documentation file for sc7180 sound card.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org --- .../bindings/sound/qcom,sc7180.yaml | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
diff --git a/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml new file mode 100644 index 000000000000..d60d2880d991 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml @@ -0,0 +1,123 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/qcom,sc7180.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Technologies Inc. SC7180 ASoC sound card driver + +maintainers: + - Rohit kumar rohitkr@codeaurora.org + - Cheng-Yi Chiang cychiang@chromium.org + +description: | + This binding describes the SC7180 sound card, which uses LPASS for audio. + +definitions: + + link-name: + description: Indicates dai-link name and PCM stream name. + $ref: /schemas/types.yaml#/definitions/string + maxItems: 1 + + dai: + type: object + properties: + sound-dai: + maxItems: 1 + $ref: /schemas/types.yaml#/definitions/phandle-array + description: phandle array of the codec or CPU DAI + + required: + - sound-dai + + unidirectional: + description: Specify direction of unidirectional dai link. + 0 for playback only. 1 for capture only. + $ref: /schemas/types.yaml#/definitions/uint32 + +properties: + compatible: + contains: + enum: + - qcom,sc7180-sndcard + + audio-routing: + $ref: /schemas/types.yaml#/definitions/non-unique-string-array + description: |- + 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. + + model: + $ref: /schemas/types.yaml#/definitions/string + description: User specified audio sound card name + +patternProperties: + "^dai-link-[0-9]+$": + description: | + Each subnode represents a dai link. Subnodes of each dai links would be + cpu/codec dais. + + type: object + + properties: + link-name: + $ref: "#/definitions/link-name" + + unidirectional: + $ref: "#/definitions/unidirectional" + + cpu: + $ref: "#/definitions/dai" + + codec: + $ref: "#/definitions/dai" + + required: + - link-name + - cpu + - codec + + additionalProperties: false + +examples: + + - | + snd { + compatible = "qcom,sc7180-sndcard"; + model = "sc7180-snd-card"; + + pinctrl-names = "default"; + pinctrl-0 = <&sec_mi2s_active &sec_mi2s_dout_active + &sec_mi2s_ws_active &pri_mi2s_active + &pri_mi2s_dout_active &pri_mi2s_ws_active + &pri_mi2s_din_active &pri_mi2s_mclk_active>; + + audio-routing = + "Headphone Jack", "HPOL", + "Headphone Jack", "HPOR"; + + dai-link-0 { + link-name = "MultiMedia0"; + cpu { + sound-dai = <&lpass_cpu 0>; + }; + + codec { + sound-dai = <&alc5682 0>; + }; + }; + + dai-link-1 { + link-name = "MultiMedia1"; + unidirectional = <0>; + cpu { + sound-dai = <&lpass_cpu 1>; + }; + + codec { + sound-dai = <&max98357a>; + }; + }; + };
From: Ajit Pandey ajitp@codeaurora.org
Add new driver to register sound card on sc7180 trogdor board and do the required configuration for lpass cpu dai and external codecs connected over MI2S interfaces.
Signed-off-by: Ajit Pandey ajitp@codeaurora.org Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org --- Note: - This patch depends on this patch series so it is not ready to be merged now. ASoC: qcom: Add support for SC7180 lpass variant https://patchwork.kernel.org/cover/11650649/ - The patch is made by the collaboration of Cheng-Yi Chiang cychiang@chromium.org Rohit kumar rohitkr@codeaurora.org Ajit Pandey ajitp@codeaurora.org But Ajit has left codeaurora. - We may reduce the duplicate code of sc7180_parse_of and qcom_snd_parse_of in snd/soc/qcom/common.c. However, qcom_snd_parse_of has specific logic with qdsp. In this initial version, to keep it simple, I choose to only implement needed device property parsing by sc7180 sound card in sc7180_parse_of.
Thanks for the review!
sound/soc/qcom/Kconfig | 11 ++ sound/soc/qcom/Makefile | 2 + sound/soc/qcom/sc7180.c | 410 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 423 insertions(+) create mode 100644 sound/soc/qcom/sc7180.c
diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig index cfca0f730c61..4adc2362865b 100644 --- a/sound/soc/qcom/Kconfig +++ b/sound/soc/qcom/Kconfig @@ -109,3 +109,14 @@ config SND_SOC_SDM845 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. + +config SND_SOC_SC7180 + tristate "SoC Machine driver for SC7180 boards" + depends on SND_SOC_QCOM + select SND_SOC_LPASS_SC7180 + select SND_SOC_MAX98357A + select SND_SOC_RT5682 + help + To add support for audio on Qualcomm Technologies Inc. + SC7180 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 41b2c7a23a4d..3f6275d90526 100644 --- a/sound/soc/qcom/Makefile +++ b/sound/soc/qcom/Makefile @@ -15,12 +15,14 @@ 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 +snd-soc-sc7180-objs := sc7180.o snd-soc-qcom-common-objs := common.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 +obj-$(CONFIG_SND_SOC_SC7180) += snd-soc-sc7180.o obj-$(CONFIG_SND_SOC_QCOM_COMMON) += snd-soc-qcom-common.o
#DSP lib diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c new file mode 100644 index 000000000000..cbe6b487d432 --- /dev/null +++ b/sound/soc/qcom/sc7180.c @@ -0,0 +1,410 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2020, The Linux Foundation. All rights reserved. + * + * sc7180.c -- ALSA SoC Machine driver for SC7180 + */ + +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/of_device.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/jack.h> +#include <sound/soc.h> +#include <uapi/linux/input-event-codes.h> +#include <dt-bindings/sound/sc7180-lpass.h> +#include "../codecs/rt5682.h" +#include "common.h" +#include "lpass.h" + +#define DEFAULT_SAMPLE_RATE_48K 48000 +#define DEFAULT_MCLK_RATE 19200000 +#define RT5682_PLL1_FREQ (48000 * 512) + +int sc7180_parse_of(struct snd_soc_card *card) +{ + struct device_node *np; + struct device_node *codec = NULL; + struct device_node *platform = NULL; + struct device_node *cpu = NULL; + struct device *dev = card->dev; + struct snd_soc_dai_link *link; + struct of_phandle_args args; + struct snd_soc_dai_link_component *dlc; + int ret, num_links, dir; + + ret = snd_soc_of_parse_card_name(card, "model"); + if (ret) { + dev_err(dev, "Error parsing card name: %d\n", ret); + return ret; + } + + /* DAPM routes */ + if (of_property_read_bool(dev->of_node, "audio-routing")) { + ret = snd_soc_of_parse_audio_routing(card, + "audio-routing"); + if (ret) + return ret; + } + + /* Populate links */ + num_links = of_get_child_count(dev->of_node); + + /* Allocate the DAI link array */ + card->dai_link = kcalloc(num_links, sizeof(*link), GFP_KERNEL); + if (!card->dai_link) + return -ENOMEM; + + card->num_links = num_links; + link = card->dai_link; + + for_each_child_of_node(dev->of_node, np) { + dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL); + if (!dlc) + return -ENOMEM; + + link->cpus = &dlc[0]; + link->platforms = &dlc[1]; + + link->num_cpus = 1; + link->num_platforms = 1; + + ret = of_property_read_string(np, "link-name", &link->name); + if (ret) { + dev_err(card->dev, "error getting codec dai_link name\n"); + goto err; + } + + of_property_read_u32(np, "unidirectional", &dir); + if (dir == 0) + link->playback_only = 1; + else if (dir == 1) + link->capture_only = 1; + + cpu = of_get_child_by_name(np, "cpu"); + codec = of_get_child_by_name(np, "codec"); + + if (!cpu) { + dev_err(dev, "%s: Can't find cpu DT node\n", + link->name); + ret = -EINVAL; + goto err; + } + + ret = of_parse_phandle_with_args(cpu, "sound-dai", + "#sound-dai-cells", 0, &args); + if (ret) { + dev_err(card->dev, "%s: error getting cpu phandle\n", + link->name); + goto err; + } + link->cpus->of_node = args.np; + link->id = args.args[0]; + + ret = snd_soc_of_get_dai_name(cpu, &link->cpus->dai_name); + if (ret) { + dev_err(card->dev, "%s: error getting cpu dai name\n", + link->name); + goto err; + } + + if (codec) { + ret = snd_soc_of_get_dai_link_codecs(dev, codec, link); + if (ret < 0) { + dev_err(card->dev, "%s: codec dai not found\n", + link->name); + goto err; + } + } else { + dlc = devm_kzalloc(dev, sizeof(*dlc), GFP_KERNEL); + if (!dlc) + return -ENOMEM; + + link->codecs = dlc; + link->num_codecs = 1; + + link->codecs->dai_name = "snd-soc-dummy-dai"; + link->codecs->name = "snd-soc-dummy"; + } + + link->platforms->of_node = link->cpus->of_node; + link->stream_name = link->name; + link++; + + of_node_put(cpu); + of_node_put(codec); + } + + return 0; +err: + of_node_put(np); + of_node_put(cpu); + of_node_put(codec); + of_node_put(platform); + kfree(card->dai_link); + return ret; +} + +struct sc7180_snd_data { + struct snd_soc_jack jack; + bool jack_setup; + struct snd_soc_card *card; + u32 pri_mi2s_clk_count; +}; + +static int sc7180_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 = asoc_rtd_to_cpu(rtd, 0); + int ret = 0; + + switch (cpu_dai->id) { + case MI2S_PRIMARY: + break; + case MI2S_SECONDARY: + break; + default: + pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id); + break; + } + return ret; +} + +static void sc7180_jack_free(struct snd_jack *jack) +{ + struct snd_soc_component *component = jack->private_data; + + snd_soc_component_set_jack(component, NULL, NULL); +} + +static int sc7180_dai_init(struct snd_soc_pcm_runtime *rtd) +{ + struct snd_soc_component *component; + struct snd_soc_card *card = rtd->card; + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); + struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card); + struct snd_jack *jack; + int rval; + + if (!pdata->jack_setup) { + rval = snd_soc_card_jack_new( + card, "Headset Jack", + SND_JACK_HEADSET | + SND_JACK_HEADPHONE | + SND_JACK_BTN_0 | SND_JACK_BTN_1 | + SND_JACK_BTN_2 | SND_JACK_BTN_3, + &pdata->jack, NULL, 0); + + if (rval < 0) { + dev_err(card->dev, "Unable to add Headphone Jack\n"); + return rval; + } + + jack = pdata->jack.jack; + + snd_jack_set_key(jack, SND_JACK_BTN_0, KEY_PLAYPAUSE); + snd_jack_set_key(jack, SND_JACK_BTN_1, KEY_VOICECOMMAND); + snd_jack_set_key(jack, SND_JACK_BTN_2, KEY_VOLUMEUP); + snd_jack_set_key(jack, SND_JACK_BTN_3, KEY_VOLUMEDOWN); + pdata->jack_setup = true; + } + + switch (cpu_dai->id) { + case MI2S_PRIMARY: + jack = pdata->jack.jack; + component = codec_dai->component; + + jack->private_data = component; + jack->private_free = sc7180_jack_free; + rval = snd_soc_component_set_jack(component, + &pdata->jack, NULL); + if (rval != 0 && rval != -EOPNOTSUPP) { + dev_warn(card->dev, "Failed to set jack: %d\n", rval); + return rval; + } + break; + case MI2S_SECONDARY: + break; + default: + pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id); + break; + } + + return 0; +} + +static int sc7180_snd_startup(struct snd_pcm_substream *substream) +{ + unsigned int codec_dai_fmt = SND_SOC_DAIFMT_CBS_CFS; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_card *card = rtd->card; + struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card); + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); + int ret; + + switch (cpu_dai->id) { + case MI2S_PRIMARY: + codec_dai_fmt |= SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S; + if (++data->pri_mi2s_clk_count == 1) { + snd_soc_dai_set_sysclk(cpu_dai, + LPASS_MCLK0, + DEFAULT_MCLK_RATE, + SNDRV_PCM_STREAM_PLAYBACK); + } + snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); + + /* Configure PLL1 for codec */ + ret = snd_soc_dai_set_pll(codec_dai, 0, RT5682_PLL1_S_MCLK, + DEFAULT_MCLK_RATE, RT5682_PLL1_FREQ); + if (ret < 0) { + dev_err(rtd->dev, "can't set codec pll: %d\n", ret); + return ret; + } + + /* Configure sysclk for codec */ + ret = snd_soc_dai_set_sysclk(codec_dai, RT5682_SCLK_S_PLL1, + RT5682_PLL1_FREQ, + SND_SOC_CLOCK_IN); + if (ret < 0) + dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n", + ret); + + break; + case MI2S_SECONDARY: + break; + default: + pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id); + break; + } + return 0; +} + +static void sc7180_snd_shutdown(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_card *card = rtd->card; + struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card); + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + + switch (cpu_dai->id) { + case MI2S_PRIMARY: + if (--data->pri_mi2s_clk_count == 0) { + snd_soc_dai_set_sysclk(cpu_dai, + LPASS_MCLK0, + 0, + SNDRV_PCM_STREAM_PLAYBACK); + } + break; + case MI2S_SECONDARY: + break; + default: + pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id); + break; + } +} + +static const struct snd_soc_ops sc7180_be_ops = { + .hw_params = sc7180_snd_hw_params, + .startup = sc7180_snd_startup, + .shutdown = sc7180_snd_shutdown, +}; + +static const struct snd_soc_dapm_widget sc7180_snd_widgets[] = { + SND_SOC_DAPM_HP("Headphone Jack", NULL), + SND_SOC_DAPM_MIC("Headset Mic", NULL), +}; + +static void sc7180_add_ops(struct snd_soc_card *card) +{ + struct snd_soc_dai_link *link; + int i; + + for_each_card_prelinks(card, i, link) { + link->ops = &sc7180_be_ops; + link->init = sc7180_dai_init; + } +} + +static int sc7180_snd_platform_probe(struct platform_device *pdev) +{ + struct snd_soc_card *card; + struct sc7180_snd_data *data; + struct device *dev = &pdev->dev; + int ret; + + card = kzalloc(sizeof(*card), GFP_KERNEL); + if (!card) + return -ENOMEM; + + /* Allocate the private data */ + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) { + ret = -ENOMEM; + goto data_alloc_fail; + } + + card->dapm_widgets = sc7180_snd_widgets; + card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets); + card->dev = dev; + dev_set_drvdata(dev, card); + ret = sc7180_parse_of(card); + if (ret) { + dev_err(dev, "Error parsing OF data\n"); + goto parse_dt_fail; + } + + data->card = card; + snd_soc_card_set_drvdata(card, data); + + sc7180_add_ops(card); + 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: + kfree(card->dai_link); +parse_dt_fail: + kfree(data); +data_alloc_fail: + kfree(card); + return ret; +} + +static int sc7180_snd_platform_remove(struct platform_device *pdev) +{ + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev); + struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card); + + snd_soc_unregister_card(card); + kfree(card->dai_link); + kfree(data); + kfree(card); + return 0; +} + +static const struct of_device_id sc7180_snd_device_id[] = { + { .compatible = "qcom,sc7180-sndcard" }, + {}, +}; +MODULE_DEVICE_TABLE(of, sc7180_snd_device_id); + +static struct platform_driver sc7180_snd_driver = { + .probe = sc7180_snd_platform_probe, + .remove = sc7180_snd_platform_remove, + .driver = { + .name = "msm-snd-sc7180", + .of_match_table = sc7180_snd_device_id, + }, +}; +module_platform_driver(sc7180_snd_driver); + +MODULE_DESCRIPTION("sc7180 ASoC Machine Driver"); +MODULE_LICENSE("GPL v2");
On Fri, Jul 17, 2020 at 8:02 PM Cheng-Yi Chiang cychiang@chromium.org wrote:
diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c new file mode 100644 index 000000000000..cbe6b487d432 --- /dev/null +++ b/sound/soc/qcom/sc7180.c @@ -0,0 +1,410 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Copyright (c) 2020, The Linux Foundation. All rights reserved.
- sc7180.c -- ALSA SoC Machine driver for SC7180
- */
Use "//" for all lines (see https://lkml.org/lkml/2020/5/14/332).
+#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/of_device.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/jack.h> +#include <sound/soc.h> +#include <uapi/linux/input-event-codes.h> +#include <dt-bindings/sound/sc7180-lpass.h> +#include "../codecs/rt5682.h" +#include "common.h" +#include "lpass.h"
Insert a blank line in between <...> and "..." and sort the list alphabetically to make it less likely to conflict.
+static int sc7180_snd_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)+{
Dummy function? Or is it still work in progress?
struct snd_soc_pcm_runtime *rtd = substream->private_data;struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);int ret = 0;switch (cpu_dai->id) {case MI2S_PRIMARY:break;case MI2S_SECONDARY:break;default:pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
-EINVAL.
+static int sc7180_dai_init(struct snd_soc_pcm_runtime *rtd) +{
struct snd_soc_component *component;struct snd_soc_card *card = rtd->card;struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);struct snd_jack *jack;int rval;if (!pdata->jack_setup) {rval = snd_soc_card_jack_new(card, "Headset Jack",SND_JACK_HEADSET |SND_JACK_HEADPHONE |SND_JACK_BTN_0 | SND_JACK_BTN_1 |SND_JACK_BTN_2 | SND_JACK_BTN_3,&pdata->jack, NULL, 0);if (rval < 0) {dev_err(card->dev, "Unable to add Headphone Jack\n");return rval;}jack = pdata->jack.jack;snd_jack_set_key(jack, SND_JACK_BTN_0, KEY_PLAYPAUSE);snd_jack_set_key(jack, SND_JACK_BTN_1, KEY_VOICECOMMAND);snd_jack_set_key(jack, SND_JACK_BTN_2, KEY_VOLUMEUP);snd_jack_set_key(jack, SND_JACK_BTN_3, KEY_VOLUMEDOWN);pdata->jack_setup = true;
This block is something I don't expect to be in "dai_init" (i.e. there is only 1 headset jack, why do we need to run the code for n times).
switch (cpu_dai->id) {case MI2S_PRIMARY:jack = pdata->jack.jack;component = codec_dai->component;jack->private_data = component;jack->private_free = sc7180_jack_free;rval = snd_soc_component_set_jack(component,&pdata->jack, NULL);if (rval != 0 && rval != -EOPNOTSUPP) {dev_warn(card->dev, "Failed to set jack: %d\n", rval);return rval;}break;case MI2S_SECONDARY:break;default:pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
-EINVAL.
+static int sc7180_snd_startup(struct snd_pcm_substream *substream) +{
unsigned int codec_dai_fmt = SND_SOC_DAIFMT_CBS_CFS;struct snd_soc_pcm_runtime *rtd = substream->private_data;struct snd_soc_card *card = rtd->card;struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);int ret;switch (cpu_dai->id) {case MI2S_PRIMARY:codec_dai_fmt |= SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S;
If the format is fixed, could it put somewhere statically?
if (++data->pri_mi2s_clk_count == 1) {
Don't it need to be atomic?
snd_soc_dai_set_sysclk(cpu_dai,LPASS_MCLK0,DEFAULT_MCLK_RATE,SNDRV_PCM_STREAM_PLAYBACK);}snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);/* Configure PLL1 for codec */ret = snd_soc_dai_set_pll(codec_dai, 0, RT5682_PLL1_S_MCLK,DEFAULT_MCLK_RATE, RT5682_PLL1_FREQ);if (ret < 0) {dev_err(rtd->dev, "can't set codec pll: %d\n", ret);return ret;}/* Configure sysclk for codec */ret = snd_soc_dai_set_sysclk(codec_dai, RT5682_SCLK_S_PLL1,RT5682_PLL1_FREQ,SND_SOC_CLOCK_IN);if (ret < 0)dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n",ret);break;case MI2S_SECONDARY:break;default:pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
-EINVAL.
+static void sc7180_snd_shutdown(struct snd_pcm_substream *substream) +{
struct snd_soc_pcm_runtime *rtd = substream->private_data;struct snd_soc_card *card = rtd->card;struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);switch (cpu_dai->id) {case MI2S_PRIMARY:if (--data->pri_mi2s_clk_count == 0) {
Atomic?
snd_soc_dai_set_sysclk(cpu_dai,LPASS_MCLK0,0,SNDRV_PCM_STREAM_PLAYBACK);}break;case MI2S_SECONDARY:break;default:pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
-EINVAL.
+static int sc7180_snd_platform_probe(struct platform_device *pdev) +{
struct snd_soc_card *card;struct sc7180_snd_data *data;struct device *dev = &pdev->dev;int ret;card = kzalloc(sizeof(*card), GFP_KERNEL);if (!card)return -ENOMEM;
Looks like you don't need to allocate the card in runtime. Also you need to use the devm version if needed.
/* Allocate the private data */data = kzalloc(sizeof(*data), GFP_KERNEL);
Use devm.
card->dapm_widgets = sc7180_snd_widgets;card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
Can the struct snd_soc_card allocate statically?
sc7180_add_ops(card);ret = snd_soc_register_card(card);
devm.
I didn't dive into the logic too much. Would need another round review if any newer version.
Hi Tzung-Bi, Thanks for the review! On Mon, Jul 20, 2020 at 10:47 AM Tzung-Bi Shih tzungbi@google.com wrote:
On Fri, Jul 17, 2020 at 8:02 PM Cheng-Yi Chiang cychiang@chromium.org wrote:
diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c new file mode 100644 index 000000000000..cbe6b487d432 --- /dev/null +++ b/sound/soc/qcom/sc7180.c @@ -0,0 +1,410 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Copyright (c) 2020, The Linux Foundation. All rights reserved.
- sc7180.c -- ALSA SoC Machine driver for SC7180
- */
Use "//" for all lines (see https://lkml.org/lkml/2020/5/14/332).
Thanks for the pointer. Fixed in v2.
+#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/of_device.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/jack.h> +#include <sound/soc.h> +#include <uapi/linux/input-event-codes.h> +#include <dt-bindings/sound/sc7180-lpass.h> +#include "../codecs/rt5682.h" +#include "common.h" +#include "lpass.h"
Insert a blank line in between <...> and "..." and sort the list alphabetically to make it less likely to conflict.
Fixed in v2.
+static int sc7180_snd_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)+{
Dummy function? Or is it still work in progress?
Removed in v2.
struct snd_soc_pcm_runtime *rtd = substream->private_data;struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);int ret = 0;switch (cpu_dai->id) {case MI2S_PRIMARY:break;case MI2S_SECONDARY:break;default:pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);-EINVAL.
Removed in v2.
+static int sc7180_dai_init(struct snd_soc_pcm_runtime *rtd) +{
struct snd_soc_component *component;struct snd_soc_card *card = rtd->card;struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);struct snd_jack *jack;int rval;if (!pdata->jack_setup) {rval = snd_soc_card_jack_new(card, "Headset Jack",SND_JACK_HEADSET |SND_JACK_HEADPHONE |SND_JACK_BTN_0 | SND_JACK_BTN_1 |SND_JACK_BTN_2 | SND_JACK_BTN_3,&pdata->jack, NULL, 0);if (rval < 0) {dev_err(card->dev, "Unable to add Headphone Jack\n");return rval;}jack = pdata->jack.jack;snd_jack_set_key(jack, SND_JACK_BTN_0, KEY_PLAYPAUSE);snd_jack_set_key(jack, SND_JACK_BTN_1, KEY_VOICECOMMAND);snd_jack_set_key(jack, SND_JACK_BTN_2, KEY_VOLUMEUP);snd_jack_set_key(jack, SND_JACK_BTN_3, KEY_VOLUMEDOWN);pdata->jack_setup = true;This block is something I don't expect to be in "dai_init" (i.e. there is only 1 headset jack, why do we need to run the code for n times).
Thanks for the suggestion. In v2 I am using aux device so this function is cleaned up to be specific to aux device for jack detection.
switch (cpu_dai->id) {case MI2S_PRIMARY:jack = pdata->jack.jack;component = codec_dai->component;jack->private_data = component;jack->private_free = sc7180_jack_free;rval = snd_soc_component_set_jack(component,&pdata->jack, NULL);if (rval != 0 && rval != -EOPNOTSUPP) {dev_warn(card->dev, "Failed to set jack: %d\n", rval);return rval;}break;case MI2S_SECONDARY:break;default:pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);-EINVAL.
Removed in v2.
+static int sc7180_snd_startup(struct snd_pcm_substream *substream) +{
unsigned int codec_dai_fmt = SND_SOC_DAIFMT_CBS_CFS;struct snd_soc_pcm_runtime *rtd = substream->private_data;struct snd_soc_card *card = rtd->card;struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);int ret;switch (cpu_dai->id) {case MI2S_PRIMARY:codec_dai_fmt |= SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S;If the format is fixed, could it put somewhere statically?
Fixed in v2.
if (++data->pri_mi2s_clk_count == 1) {Don't it need to be atomic?
soc_pcm_open and soc_pcm_close are protected by card->pcm_mutex so they will happen in sequence.
snd_soc_dai_set_sysclk(cpu_dai,LPASS_MCLK0,DEFAULT_MCLK_RATE,SNDRV_PCM_STREAM_PLAYBACK);}snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);/* Configure PLL1 for codec */ret = snd_soc_dai_set_pll(codec_dai, 0, RT5682_PLL1_S_MCLK,DEFAULT_MCLK_RATE, RT5682_PLL1_FREQ);if (ret < 0) {dev_err(rtd->dev, "can't set codec pll: %d\n", ret);return ret;}/* Configure sysclk for codec */ret = snd_soc_dai_set_sysclk(codec_dai, RT5682_SCLK_S_PLL1,RT5682_PLL1_FREQ,SND_SOC_CLOCK_IN);if (ret < 0)dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n",ret);break;case MI2S_SECONDARY:break;default:pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);-EINVAL.
Fixed in v2
+static void sc7180_snd_shutdown(struct snd_pcm_substream *substream) +{
struct snd_soc_pcm_runtime *rtd = substream->private_data;struct snd_soc_card *card = rtd->card;struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);switch (cpu_dai->id) {case MI2S_PRIMARY:if (--data->pri_mi2s_clk_count == 0) {Atomic?
ditto
snd_soc_dai_set_sysclk(cpu_dai,LPASS_MCLK0,0,SNDRV_PCM_STREAM_PLAYBACK);}break;case MI2S_SECONDARY:break;default:pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);-EINVAL.
not needed since this returns void
+static int sc7180_snd_platform_probe(struct platform_device *pdev) +{
struct snd_soc_card *card;struct sc7180_snd_data *data;struct device *dev = &pdev->dev;int ret;card = kzalloc(sizeof(*card), GFP_KERNEL);if (!card)return -ENOMEM;Looks like you don't need to allocate the card in runtime. Also you need to use the devm version if needed.
Thanks for the great suggestion. In v2 I am using a static sound card. Also, use devm wherever possible to greatly simplify the code.
/* Allocate the private data */data = kzalloc(sizeof(*data), GFP_KERNEL);Use devm.
Fixed in v2.
card->dapm_widgets = sc7180_snd_widgets;card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);Can the struct snd_soc_card allocate statically?
Fixed in v2.
sc7180_add_ops(card);ret = snd_soc_register_card(card);devm.
I didn't dive into the logic too much. Would need another round review if any newer version.
Thanks again.
Hi,
On Fri, Jul 17, 2020 at 5:02 AM Cheng-Yi Chiang cychiang@chromium.org wrote:
Add devicetree bindings documentation file for sc7180 sound card.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org
.../bindings/sound/qcom,sc7180.yaml | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+)
A bit of a mechanical review since my audio knowledge is not strong.
create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
diff --git a/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml new file mode 100644 index 000000000000..d60d2880d991 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml @@ -0,0 +1,123 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/qcom,sc7180.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Qualcomm Technologies Inc. SC7180 ASoC sound card driver
+maintainers:
- Rohit kumar rohitkr@codeaurora.org
- Cheng-Yi Chiang cychiang@chromium.org
+description: |
- This binding describes the SC7180 sound card, which uses LPASS for audio.
nit: you don't need the pipe at the end of the "description" line. That means that newlines are important and you don't need it.
+definitions:
I haven't yet seen much yaml using definitions like this. It feels like overkill for some of these properties, especially ones that are only ever used once in the "properties:" section and are/or are really simple.
- link-name:
- description: Indicates dai-link name and PCM stream name.
- $ref: /schemas/types.yaml#/definitions/string
- maxItems: 1
- dai:
- type: object
- properties:
sound-dai:maxItems: 1$ref: /schemas/types.yaml#/definitions/phandle-arraydescription: phandle array of the codec or CPU DAI- required:
- sound-dai- unidirectional:
- description: Specify direction of unidirectional dai link.
0 for playback only. 1 for capture only.- $ref: /schemas/types.yaml#/definitions/uint32
So if the property isn't there then it's _not_ unidirectional and if it is there then this specifies the direction, right? I almost wonder if this should just be two boolean properties, like:
playback-only; capture-only;
...but I guess I'd leave it to Rob and/or Mark to say what they liked better. In any case if you keep it how you have it then you should use yaml to force it to be either 0 or 1 if present.
+properties:
- compatible:
- contains:
enum:- qcom,sc7180-sndcard
Just:
properties: compatible: const: qcom,sc7180-sndcard
- audio-routing:
- $ref: /schemas/types.yaml#/definitions/non-unique-string-array
- description: |-
A list of the connections between audio components. Each entry is apair of strings, the first being the connection's sink, the secondbeing the connection's source.
You don't need the "|-" after the "description:". That says newlines are important but strip the newline from the end.
- model:
- $ref: /schemas/types.yaml#/definitions/string
- description: User specified audio sound card name
+patternProperties:
- "^dai-link-[0-9]+$":
- description: |
Each subnode represents a dai link. Subnodes of each dai links would becpu/codec dais.
From looking at "simple-card.yaml", I'm gonna guess that instead of
encoding the link number in the name of the node that you should actually use a unit address and a reg in the subnodes.
...also, again your description doesn't need the "|" at the end. Maybe https://yaml-multiline.info/ will be useful to you?
- type: object
- properties:
link-name:$ref: "#/definitions/link-name"unidirectional:$ref: "#/definitions/unidirectional"cpu:$ref: "#/definitions/dai"codec:$ref: "#/definitions/dai"- required:
- link-name- cpu- codec- additionalProperties: false
+examples:
- |
- snd {
Can you use the full node name "sound" here?
compatible = "qcom,sc7180-sndcard";model = "sc7180-snd-card";pinctrl-names = "default";pinctrl-0 = <&sec_mi2s_active &sec_mi2s_dout_active&sec_mi2s_ws_active &pri_mi2s_active&pri_mi2s_dout_active &pri_mi2s_ws_active&pri_mi2s_din_active &pri_mi2s_mclk_active>;
I think pinctrl is usually not in the dt examples.
...also, shouldn't the mi2s pinctrl be in the i2s nodes, not in the overall sound node?
audio-routing ="Headphone Jack", "HPOL","Headphone Jack", "HPOR";dai-link-0 {link-name = "MultiMedia0";cpu {sound-dai = <&lpass_cpu 0>;};codec {sound-dai = <&alc5682 0>;};};dai-link-1 {link-name = "MultiMedia1";unidirectional = <0>;cpu {sound-dai = <&lpass_cpu 1>;};codec {sound-dai = <&max98357a>;};};- };
-- 2.28.0.rc0.105.gf9edc3c819-goog
On Fri, Jul 17, 2020 at 11:03 PM Doug Anderson dianders@chromium.org wrote:
Hi,
Thanks for the review!
On Fri, Jul 17, 2020 at 5:02 AM Cheng-Yi Chiang cychiang@chromium.org wrote:
Add devicetree bindings documentation file for sc7180 sound card.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org
.../bindings/sound/qcom,sc7180.yaml | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+)
A bit of a mechanical review since my audio knowledge is not strong.
create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
diff --git a/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml new file mode 100644 index 000000000000..d60d2880d991 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml @@ -0,0 +1,123 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/qcom,sc7180.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Qualcomm Technologies Inc. SC7180 ASoC sound card driver
+maintainers:
- Rohit kumar rohitkr@codeaurora.org
- Cheng-Yi Chiang cychiang@chromium.org
+description: |
- This binding describes the SC7180 sound card, which uses LPASS for audio.
nit: you don't need the pipe at the end of the "description" line. That means that newlines are important and you don't need it.
Thanks for the explanation. Fixed in v2.
+definitions:
I haven't yet seen much yaml using definitions like this. It feels like overkill for some of these properties, especially ones that are only ever used once in the "properties:" section and are/or are really simple.
ACK. In v2 I only kept dai definition and removed others.
- link-name:
- description: Indicates dai-link name and PCM stream name.
- $ref: /schemas/types.yaml#/definitions/string
- maxItems: 1
- dai:
- type: object
- properties:
sound-dai:maxItems: 1$ref: /schemas/types.yaml#/definitions/phandle-arraydescription: phandle array of the codec or CPU DAI- required:
- sound-dai- unidirectional:
- description: Specify direction of unidirectional dai link.
0 for playback only. 1 for capture only.- $ref: /schemas/types.yaml#/definitions/uint32
So if the property isn't there then it's _not_ unidirectional and if it is there then this specifies the direction, right? I almost wonder if this should just be two boolean properties, like:
playback-only; capture-only;
...but I guess I'd leave it to Rob and/or Mark to say what they liked better. In any case if you keep it how you have it then you should use yaml to force it to be either 0 or 1 if present.
ACK Use playback-only and capture-only in v2 instead.
+properties:
- compatible:
- contains:
enum:- qcom,sc7180-sndcardJust:
properties: compatible: const: qcom,sc7180-sndcard
Fixed in v2.
- audio-routing:
- $ref: /schemas/types.yaml#/definitions/non-unique-string-array
- description: |-
A list of the connections between audio components. Each entry is apair of strings, the first being the connection's sink, the secondbeing the connection's source.You don't need the "|-" after the "description:". That says newlines are important but strip the newline from the end.
Fixed in v2.
- model:
- $ref: /schemas/types.yaml#/definitions/string
- description: User specified audio sound card name
+patternProperties:
- "^dai-link-[0-9]+$":
- description: |
Each subnode represents a dai link. Subnodes of each dai links would becpu/codec dais.From looking at "simple-card.yaml", I'm gonna guess that instead of encoding the link number in the name of the node that you should actually use a unit address and a reg in the subnodes.
Thanks for the explanation. Fixed in v2. I think this naming is better, although there is no usage in the machine driver for the reg.
...also, again your description doesn't need the "|" at the end. Maybe https://yaml-multiline.info/ will be useful to you?
Thanks for the explanation and the pointer!
- type: object
- properties:
link-name:$ref: "#/definitions/link-name"unidirectional:$ref: "#/definitions/unidirectional"cpu:$ref: "#/definitions/dai"codec:$ref: "#/definitions/dai"- required:
- link-name- cpu- codec- additionalProperties: false
+examples:
- |
- snd {
Can you use the full node name "sound" here?
Fixed in v2.
compatible = "qcom,sc7180-sndcard";model = "sc7180-snd-card";pinctrl-names = "default";pinctrl-0 = <&sec_mi2s_active &sec_mi2s_dout_active&sec_mi2s_ws_active &pri_mi2s_active&pri_mi2s_dout_active &pri_mi2s_ws_active&pri_mi2s_din_active &pri_mi2s_mclk_active>;I think pinctrl is usually not in the dt examples.
Fixed in v2.
...also, shouldn't the mi2s pinctrl be in the i2s nodes, not in the overall sound node?
Yes. Thanks for pointing this out. Fixed in dts file in chromium.
audio-routing ="Headphone Jack", "HPOL","Headphone Jack", "HPOR";dai-link-0 {link-name = "MultiMedia0";cpu {sound-dai = <&lpass_cpu 0>;};codec {sound-dai = <&alc5682 0>;};};dai-link-1 {link-name = "MultiMedia1";unidirectional = <0>;cpu {sound-dai = <&lpass_cpu 1>;};codec {sound-dai = <&max98357a>;};};- };
-- 2.28.0.rc0.105.gf9edc3c819-goog
participants (3)
-
Cheng-Yi Chiang -
Doug Anderson -
Tzung-Bi Shih