* [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-21 10:44 ` Cheng-Yi Chiang
0 siblings, 0 replies; 35+ messages in thread
From: Cheng-Yi Chiang @ 2020-07-21 10:44 UTC (permalink / raw)
To: linux-kernel
Cc: Taniya Das, devicetree, tzungbi, Banajit Goswami, alsa-devel,
linux-arm-msm, Patrick Lai, Takashi Iwai, Liam Girdwood,
Rob Herring, Bjorn Andersson, Andy Gross, Rohit kumar,
Mark Brown, dianders, Ajit Pandey, dgreid, Jaroslav Kysela,
linux-arm-kernel, Cheng-Yi Chiang
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: Cheng-Yi Chiang <cychiang@chromium.org>
---
sound/soc/qcom/Kconfig | 11 ++
sound/soc/qcom/Makefile | 2 +
sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 393 insertions(+)
create mode 100644 sound/soc/qcom/sc7180.c
diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
index cfca0f730c61..1b02f521f355 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..3beb2b129d01
--- /dev/null
+++ b/sound/soc/qcom/sc7180.c
@@ -0,0 +1,380 @@
+// 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 <dt-bindings/sound/sc7180-lpass.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/jack.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <uapi/linux/input-event-codes.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)
+
+static int sc7180_headset_init(struct snd_soc_component *component);
+
+static struct snd_soc_aux_dev sc7180_headset_dev = {
+ .dlc = COMP_EMPTY(),
+ .init = sc7180_headset_init,
+};
+
+struct sc7180_snd_data {
+ struct snd_soc_jack jack;
+ struct snd_soc_card *card;
+ u32 pri_mi2s_clk_count;
+};
+
+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_headset_init(struct snd_soc_component *component)
+{
+ struct snd_soc_card *card = component->card;
+ struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
+ struct snd_jack *jack;
+ int rval;
+
+ 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 Headset 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);
+
+ 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;
+ }
+
+ return 0;
+}
+
+
+static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
+ SND_SOC_DAIFMT_NB_NF |
+ SND_SOC_DAIFMT_I2S;
+
+static int sc7180_snd_startup(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);
+ struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
+ int ret;
+
+ switch (cpu_dai->id) {
+ case MI2S_PRIMARY:
+ 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, primary_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:
+ dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
+ cpu_dai->id);
+ return -EINVAL;
+ }
+ 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:
+ dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
+ cpu_dai->id);
+ break;
+ }
+}
+
+static const struct snd_soc_ops sc7180_ops = {
+ .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 struct snd_soc_card sc7180_card = {
+ .owner = THIS_MODULE,
+ .aux_dev = &sc7180_headset_dev,
+ .num_aux_devs = 1,
+ .dapm_widgets = sc7180_snd_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
+};
+
+static 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;
+
+ 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;
+ }
+
+ /* headset aux dev. */
+ sc7180_headset_dev.dlc.of_node = of_parse_phandle(
+ dev->of_node, "aux-dev", 0);
+ if (!sc7180_headset_dev.dlc.of_node) {
+ dev_err(dev,
+ "Property 'aux-dev' missing/invalid\n");
+ return -EINVAL;
+ }
+
+ /* Populate links */
+ num_links = of_get_child_count(dev->of_node);
+
+ /* Allocate the DAI link array */
+ card->dai_link = devm_kcalloc(dev, 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;
+ }
+
+ link->playback_only = of_property_read_bool(np,
+ "playback_only");
+ link->capture_only = of_property_read_bool(np,
+ "capture_only");
+
+ if (link->playback_only && link->capture_only) {
+ dev_err(card->dev,
+ "getting both playback and capture only\n");
+ goto err;
+ }
+
+ 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->ops = &sc7180_ops;
+ 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);
+ return ret;
+}
+
+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 = &sc7180_card;
+
+ /* Allocate the private data */
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ 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");
+ return ret;
+ }
+
+ data->card = card;
+ snd_soc_card_set_drvdata(card, data);
+
+ ret = snd_soc_register_card(card);
+ if (ret) {
+ dev_err(dev, "Sound card registration failed\n");
+ return ret;
+ }
+ return ret;
+}
+
+static int sc7180_snd_platform_remove(struct platform_device *pdev)
+{
+ struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
+
+ snd_soc_unregister_card(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");
--
2.28.0.rc0.105.gf9edc3c819-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-21 10:44 ` Cheng-Yi Chiang
0 siblings, 0 replies; 35+ messages in thread
From: Cheng-Yi Chiang @ 2020-07-21 10:44 UTC (permalink / raw)
To: linux-kernel
Cc: Taniya Das, devicetree, tzungbi, Banajit Goswami, alsa-devel,
linux-arm-msm, Patrick Lai, Takashi Iwai, Liam Girdwood,
Rob Herring, Bjorn Andersson, Andy Gross, Rohit kumar,
Mark Brown, dianders, Ajit Pandey, dgreid, linux-arm-kernel,
Cheng-Yi Chiang
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: Cheng-Yi Chiang <cychiang@chromium.org>
---
sound/soc/qcom/Kconfig | 11 ++
sound/soc/qcom/Makefile | 2 +
sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 393 insertions(+)
create mode 100644 sound/soc/qcom/sc7180.c
diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
index cfca0f730c61..1b02f521f355 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..3beb2b129d01
--- /dev/null
+++ b/sound/soc/qcom/sc7180.c
@@ -0,0 +1,380 @@
+// 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 <dt-bindings/sound/sc7180-lpass.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/jack.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <uapi/linux/input-event-codes.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)
+
+static int sc7180_headset_init(struct snd_soc_component *component);
+
+static struct snd_soc_aux_dev sc7180_headset_dev = {
+ .dlc = COMP_EMPTY(),
+ .init = sc7180_headset_init,
+};
+
+struct sc7180_snd_data {
+ struct snd_soc_jack jack;
+ struct snd_soc_card *card;
+ u32 pri_mi2s_clk_count;
+};
+
+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_headset_init(struct snd_soc_component *component)
+{
+ struct snd_soc_card *card = component->card;
+ struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
+ struct snd_jack *jack;
+ int rval;
+
+ 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 Headset 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);
+
+ 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;
+ }
+
+ return 0;
+}
+
+
+static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
+ SND_SOC_DAIFMT_NB_NF |
+ SND_SOC_DAIFMT_I2S;
+
+static int sc7180_snd_startup(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);
+ struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
+ int ret;
+
+ switch (cpu_dai->id) {
+ case MI2S_PRIMARY:
+ 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, primary_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:
+ dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
+ cpu_dai->id);
+ return -EINVAL;
+ }
+ 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:
+ dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
+ cpu_dai->id);
+ break;
+ }
+}
+
+static const struct snd_soc_ops sc7180_ops = {
+ .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 struct snd_soc_card sc7180_card = {
+ .owner = THIS_MODULE,
+ .aux_dev = &sc7180_headset_dev,
+ .num_aux_devs = 1,
+ .dapm_widgets = sc7180_snd_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
+};
+
+static 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;
+
+ 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;
+ }
+
+ /* headset aux dev. */
+ sc7180_headset_dev.dlc.of_node = of_parse_phandle(
+ dev->of_node, "aux-dev", 0);
+ if (!sc7180_headset_dev.dlc.of_node) {
+ dev_err(dev,
+ "Property 'aux-dev' missing/invalid\n");
+ return -EINVAL;
+ }
+
+ /* Populate links */
+ num_links = of_get_child_count(dev->of_node);
+
+ /* Allocate the DAI link array */
+ card->dai_link = devm_kcalloc(dev, 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;
+ }
+
+ link->playback_only = of_property_read_bool(np,
+ "playback_only");
+ link->capture_only = of_property_read_bool(np,
+ "capture_only");
+
+ if (link->playback_only && link->capture_only) {
+ dev_err(card->dev,
+ "getting both playback and capture only\n");
+ goto err;
+ }
+
+ 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->ops = &sc7180_ops;
+ 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);
+ return ret;
+}
+
+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 = &sc7180_card;
+
+ /* Allocate the private data */
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ 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");
+ return ret;
+ }
+
+ data->card = card;
+ snd_soc_card_set_drvdata(card, data);
+
+ ret = snd_soc_register_card(card);
+ if (ret) {
+ dev_err(dev, "Sound card registration failed\n");
+ return ret;
+ }
+ return ret;
+}
+
+static int sc7180_snd_platform_remove(struct platform_device *pdev)
+{
+ struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
+
+ snd_soc_unregister_card(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");
--
2.28.0.rc0.105.gf9edc3c819-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
2020-07-21 10:44 ` Cheng-Yi Chiang
@ 2020-07-21 18:34 ` kernel test robot
-1 siblings, 0 replies; 35+ messages in thread
From: kernel test robot @ 2020-07-21 18:34 UTC (permalink / raw)
To: Cheng-Yi Chiang, linux-kernel
Cc: kbuild-all, Mark Brown, Taniya Das, Rohit kumar, Banajit Goswami,
Patrick Lai, Andy Gross, Bjorn Andersson, Liam Girdwood,
Rob Herring
[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]
Hi Cheng-Yi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on asoc/for-next]
[also build test ERROR on linux/master linus/master v5.8-rc6 next-20200721]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Cheng-Yi-Chiang/Add-documentation-and-machine-driver-for-SC7180-sound-card/20200721-184808
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: arc-allyesconfig (attached as .config)
compiler: arc-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> sound/soc/qcom/sc7180.c:7:10: fatal error: dt-bindings/sound/sc7180-lpass.h: No such file or directory
7 | #include <dt-bindings/sound/sc7180-lpass.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
vim +7 sound/soc/qcom/sc7180.c
6
> 7 #include <dt-bindings/sound/sc7180-lpass.h>
8 #include <linux/module.h>
9 #include <linux/of_device.h>
10 #include <linux/platform_device.h>
11 #include <sound/core.h>
12 #include <sound/jack.h>
13 #include <sound/pcm.h>
14 #include <sound/pcm_params.h>
15 #include <sound/soc.h>
16 #include <uapi/linux/input-event-codes.h>
17
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64757 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
2020-07-21 10:44 ` Cheng-Yi Chiang
(?)
@ 2020-07-22 3:15 ` Tzung-Bi Shih
-1 siblings, 0 replies; 35+ messages in thread
From: Tzung-Bi Shih @ 2020-07-22 3:15 UTC (permalink / raw)
To: Cheng-Yi Chiang
Cc: Linux Kernel Mailing List, Mark Brown, Taniya Das, Rohit kumar,
Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
Douglas Anderson, dgreid, Tzung-Bi Shih, linux-arm-kernel,
linux-arm-msm, devicetree, ALSA development, Ajit Pandey
On Tue, Jul 21, 2020 at 6:44 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..3beb2b129d01
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> +//
> +//sc7180.c -- ALSA SoC Machine driver for SC7180
Insert an extra space between // and text to make it look better.
> +static int sc7180_headset_init(struct snd_soc_component *component);
> +
> +static struct snd_soc_aux_dev sc7180_headset_dev = {
> + .dlc = COMP_EMPTY(),
> + .init = sc7180_headset_init,
> +};
Move definition of sc7180_headset_dev after sc7180_headset_init( ) so
that you don't need forward declaration of sc7180_headset_init( ).
> +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> + SND_SOC_DAIFMT_NB_NF |
> + SND_SOC_DAIFMT_I2S;
> +
> +static int sc7180_snd_startup(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);
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> + int ret;
> +
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + 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, primary_dai_fmt);
My comment on the previous thread may mislead. My original intent:
move the DAIFMT setting to DAI link->dai_fmt in sc7180_parse_of( ).
If you need to keep it as is: inline the SND_SOC_DAIFMT_* into
snd_soc_dai_set_fmt( ) (i.e. eliminate primary_dai_fmt).
> +static int sc7180_parse_of(struct snd_soc_card *card)
> +{
> + struct device_node *np;
> + struct device_node *codec = NULL;
> + struct device_node *platform = NULL;
The function doesn't use platform.
> + 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;
> +
> + 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;
> + }
> +
> + /* headset aux dev. */
> + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> + dev->of_node, "aux-dev", 0);
> + if (!sc7180_headset_dev.dlc.of_node) {
> + dev_err(dev,
> + "Property 'aux-dev' missing/invalid\n");
> + return -EINVAL;
> + }
> +
> + /* Populate links */
> + num_links = of_get_child_count(dev->of_node);
Eliminate num_links but use card->num_links directly.
> +
> + /* Allocate the DAI link array */
> + card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> + GFP_KERNEL);
> + if (!card->dai_link)
> + return -ENOMEM;
> +
> + card->num_links = num_links;
Ditto, eliminate it.
> + link = card->dai_link;
> +
Eliminate the blank line to make "link = card->dai_link" and the
following for-loop "a whole thing".
> + 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;
> + }
> +
> + link->playback_only = of_property_read_bool(np,
> + "playback_only");
> + link->capture_only = of_property_read_bool(np,
> + "capture_only");
> +
> + if (link->playback_only && link->capture_only) {
> + dev_err(card->dev,
> + "getting both playback and capture only\n");
ret = -EINVAL;
> + goto err;
> + }
> +
> + cpu = of_get_child_by_name(np, "cpu");
> + codec = of_get_child_by_name(np, "codec");
Move to below.
> +
> + 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);
I may overlook it but I failed to find "#sound-dai-cells" in the
dt-binding example. I think it should be in DTS?
> + 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];
I am not quite sure what it will be. I guess one of the following
comes from DTS node name.
#define MI2S_PRIMARY 0
#define MI2S_SECONDARY 1
> +
> + 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;
> + }
> +
Move "codec = of_get_child_by_name(np, "codec");" to here.
> + 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->ops = &sc7180_ops;
> + link++;
> +
> + of_node_put(cpu);
> + of_node_put(codec);
cpu = NULL;
codec = NULL;
In case of double of_node_put( ).
> + }
> +
> + return 0;
> +err:
> + of_node_put(np);
I guess you don't need this.
> + of_node_put(cpu);
> + of_node_put(codec);
> + of_node_put(platform);
Eliminate it, not used.
> +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 = &sc7180_card;
In this case, inline the initialization while declaration.
> +
> + /* Allocate the private data */
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + card->dapm_widgets = sc7180_snd_widgets;
> + card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
Remove them.
> + card->dev = dev;
> + dev_set_drvdata(dev, card);
I guess you don't need this if using devm_snd_soc_register_card(...).
Insert a blank line.
> + ret = sc7180_parse_of(card);
> + if (ret) {
> + dev_err(dev, "Error parsing OF data\n");
> + return ret;
> + }
> +
> + data->card = card;
Looks like data->card is not used.
> + snd_soc_card_set_drvdata(card, data);
> +
> + ret = snd_soc_register_card(card);
> + if (ret) {
> + dev_err(dev, "Sound card registration failed\n");
> + return ret;
> + }
> + return ret;
Just return devm_snd_soc_register_card(...);
> +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> +{
> + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> +
> + snd_soc_unregister_card(card);
> + return 0;
> +}
Can be removed if using devm_snd_soc_register_card( ).
I didn't go through all the cases. But it would be better if all "if
(ret < 0)" can be replaced to "if (ret)".
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-22 3:15 ` Tzung-Bi Shih
0 siblings, 0 replies; 35+ messages in thread
From: Tzung-Bi Shih @ 2020-07-22 3:15 UTC (permalink / raw)
To: Cheng-Yi Chiang
Cc: Taniya Das, devicetree, Tzung-Bi Shih, Banajit Goswami,
ALSA development, Liam Girdwood, linux-arm-msm, Patrick Lai,
Takashi Iwai, Linux Kernel Mailing List, Rob Herring,
Bjorn Andersson, Mark Brown, Rohit kumar, Andy Gross,
Douglas Anderson, Ajit Pandey, dgreid, Jaroslav Kysela,
linux-arm-kernel
On Tue, Jul 21, 2020 at 6:44 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..3beb2b129d01
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> +//
> +//sc7180.c -- ALSA SoC Machine driver for SC7180
Insert an extra space between // and text to make it look better.
> +static int sc7180_headset_init(struct snd_soc_component *component);
> +
> +static struct snd_soc_aux_dev sc7180_headset_dev = {
> + .dlc = COMP_EMPTY(),
> + .init = sc7180_headset_init,
> +};
Move definition of sc7180_headset_dev after sc7180_headset_init( ) so
that you don't need forward declaration of sc7180_headset_init( ).
> +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> + SND_SOC_DAIFMT_NB_NF |
> + SND_SOC_DAIFMT_I2S;
> +
> +static int sc7180_snd_startup(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);
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> + int ret;
> +
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + 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, primary_dai_fmt);
My comment on the previous thread may mislead. My original intent:
move the DAIFMT setting to DAI link->dai_fmt in sc7180_parse_of( ).
If you need to keep it as is: inline the SND_SOC_DAIFMT_* into
snd_soc_dai_set_fmt( ) (i.e. eliminate primary_dai_fmt).
> +static int sc7180_parse_of(struct snd_soc_card *card)
> +{
> + struct device_node *np;
> + struct device_node *codec = NULL;
> + struct device_node *platform = NULL;
The function doesn't use platform.
> + 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;
> +
> + 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;
> + }
> +
> + /* headset aux dev. */
> + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> + dev->of_node, "aux-dev", 0);
> + if (!sc7180_headset_dev.dlc.of_node) {
> + dev_err(dev,
> + "Property 'aux-dev' missing/invalid\n");
> + return -EINVAL;
> + }
> +
> + /* Populate links */
> + num_links = of_get_child_count(dev->of_node);
Eliminate num_links but use card->num_links directly.
> +
> + /* Allocate the DAI link array */
> + card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> + GFP_KERNEL);
> + if (!card->dai_link)
> + return -ENOMEM;
> +
> + card->num_links = num_links;
Ditto, eliminate it.
> + link = card->dai_link;
> +
Eliminate the blank line to make "link = card->dai_link" and the
following for-loop "a whole thing".
> + 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;
> + }
> +
> + link->playback_only = of_property_read_bool(np,
> + "playback_only");
> + link->capture_only = of_property_read_bool(np,
> + "capture_only");
> +
> + if (link->playback_only && link->capture_only) {
> + dev_err(card->dev,
> + "getting both playback and capture only\n");
ret = -EINVAL;
> + goto err;
> + }
> +
> + cpu = of_get_child_by_name(np, "cpu");
> + codec = of_get_child_by_name(np, "codec");
Move to below.
> +
> + 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);
I may overlook it but I failed to find "#sound-dai-cells" in the
dt-binding example. I think it should be in DTS?
> + 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];
I am not quite sure what it will be. I guess one of the following
comes from DTS node name.
#define MI2S_PRIMARY 0
#define MI2S_SECONDARY 1
> +
> + 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;
> + }
> +
Move "codec = of_get_child_by_name(np, "codec");" to here.
> + 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->ops = &sc7180_ops;
> + link++;
> +
> + of_node_put(cpu);
> + of_node_put(codec);
cpu = NULL;
codec = NULL;
In case of double of_node_put( ).
> + }
> +
> + return 0;
> +err:
> + of_node_put(np);
I guess you don't need this.
> + of_node_put(cpu);
> + of_node_put(codec);
> + of_node_put(platform);
Eliminate it, not used.
> +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 = &sc7180_card;
In this case, inline the initialization while declaration.
> +
> + /* Allocate the private data */
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + card->dapm_widgets = sc7180_snd_widgets;
> + card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
Remove them.
> + card->dev = dev;
> + dev_set_drvdata(dev, card);
I guess you don't need this if using devm_snd_soc_register_card(...).
Insert a blank line.
> + ret = sc7180_parse_of(card);
> + if (ret) {
> + dev_err(dev, "Error parsing OF data\n");
> + return ret;
> + }
> +
> + data->card = card;
Looks like data->card is not used.
> + snd_soc_card_set_drvdata(card, data);
> +
> + ret = snd_soc_register_card(card);
> + if (ret) {
> + dev_err(dev, "Sound card registration failed\n");
> + return ret;
> + }
> + return ret;
Just return devm_snd_soc_register_card(...);
> +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> +{
> + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> +
> + snd_soc_unregister_card(card);
> + return 0;
> +}
Can be removed if using devm_snd_soc_register_card( ).
I didn't go through all the cases. But it would be better if all "if
(ret < 0)" can be replaced to "if (ret)".
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-22 3:15 ` Tzung-Bi Shih
0 siblings, 0 replies; 35+ messages in thread
From: Tzung-Bi Shih @ 2020-07-22 3:15 UTC (permalink / raw)
To: Cheng-Yi Chiang
Cc: Taniya Das, devicetree, Tzung-Bi Shih, Banajit Goswami,
ALSA development, Liam Girdwood, linux-arm-msm, Patrick Lai,
Takashi Iwai, Linux Kernel Mailing List, Rob Herring,
Bjorn Andersson, Mark Brown, Rohit kumar, Andy Gross,
Douglas Anderson, Ajit Pandey, dgreid, linux-arm-kernel
On Tue, Jul 21, 2020 at 6:44 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..3beb2b129d01
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> +//
> +//sc7180.c -- ALSA SoC Machine driver for SC7180
Insert an extra space between // and text to make it look better.
> +static int sc7180_headset_init(struct snd_soc_component *component);
> +
> +static struct snd_soc_aux_dev sc7180_headset_dev = {
> + .dlc = COMP_EMPTY(),
> + .init = sc7180_headset_init,
> +};
Move definition of sc7180_headset_dev after sc7180_headset_init( ) so
that you don't need forward declaration of sc7180_headset_init( ).
> +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> + SND_SOC_DAIFMT_NB_NF |
> + SND_SOC_DAIFMT_I2S;
> +
> +static int sc7180_snd_startup(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);
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> + int ret;
> +
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + 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, primary_dai_fmt);
My comment on the previous thread may mislead. My original intent:
move the DAIFMT setting to DAI link->dai_fmt in sc7180_parse_of( ).
If you need to keep it as is: inline the SND_SOC_DAIFMT_* into
snd_soc_dai_set_fmt( ) (i.e. eliminate primary_dai_fmt).
> +static int sc7180_parse_of(struct snd_soc_card *card)
> +{
> + struct device_node *np;
> + struct device_node *codec = NULL;
> + struct device_node *platform = NULL;
The function doesn't use platform.
> + 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;
> +
> + 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;
> + }
> +
> + /* headset aux dev. */
> + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> + dev->of_node, "aux-dev", 0);
> + if (!sc7180_headset_dev.dlc.of_node) {
> + dev_err(dev,
> + "Property 'aux-dev' missing/invalid\n");
> + return -EINVAL;
> + }
> +
> + /* Populate links */
> + num_links = of_get_child_count(dev->of_node);
Eliminate num_links but use card->num_links directly.
> +
> + /* Allocate the DAI link array */
> + card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> + GFP_KERNEL);
> + if (!card->dai_link)
> + return -ENOMEM;
> +
> + card->num_links = num_links;
Ditto, eliminate it.
> + link = card->dai_link;
> +
Eliminate the blank line to make "link = card->dai_link" and the
following for-loop "a whole thing".
> + 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;
> + }
> +
> + link->playback_only = of_property_read_bool(np,
> + "playback_only");
> + link->capture_only = of_property_read_bool(np,
> + "capture_only");
> +
> + if (link->playback_only && link->capture_only) {
> + dev_err(card->dev,
> + "getting both playback and capture only\n");
ret = -EINVAL;
> + goto err;
> + }
> +
> + cpu = of_get_child_by_name(np, "cpu");
> + codec = of_get_child_by_name(np, "codec");
Move to below.
> +
> + 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);
I may overlook it but I failed to find "#sound-dai-cells" in the
dt-binding example. I think it should be in DTS?
> + 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];
I am not quite sure what it will be. I guess one of the following
comes from DTS node name.
#define MI2S_PRIMARY 0
#define MI2S_SECONDARY 1
> +
> + 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;
> + }
> +
Move "codec = of_get_child_by_name(np, "codec");" to here.
> + 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->ops = &sc7180_ops;
> + link++;
> +
> + of_node_put(cpu);
> + of_node_put(codec);
cpu = NULL;
codec = NULL;
In case of double of_node_put( ).
> + }
> +
> + return 0;
> +err:
> + of_node_put(np);
I guess you don't need this.
> + of_node_put(cpu);
> + of_node_put(codec);
> + of_node_put(platform);
Eliminate it, not used.
> +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 = &sc7180_card;
In this case, inline the initialization while declaration.
> +
> + /* Allocate the private data */
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + card->dapm_widgets = sc7180_snd_widgets;
> + card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
Remove them.
> + card->dev = dev;
> + dev_set_drvdata(dev, card);
I guess you don't need this if using devm_snd_soc_register_card(...).
Insert a blank line.
> + ret = sc7180_parse_of(card);
> + if (ret) {
> + dev_err(dev, "Error parsing OF data\n");
> + return ret;
> + }
> +
> + data->card = card;
Looks like data->card is not used.
> + snd_soc_card_set_drvdata(card, data);
> +
> + ret = snd_soc_register_card(card);
> + if (ret) {
> + dev_err(dev, "Sound card registration failed\n");
> + return ret;
> + }
> + return ret;
Just return devm_snd_soc_register_card(...);
> +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> +{
> + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> +
> + snd_soc_unregister_card(card);
> + return 0;
> +}
Can be removed if using devm_snd_soc_register_card( ).
I didn't go through all the cases. But it would be better if all "if
(ret < 0)" can be replaced to "if (ret)".
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
2020-07-22 3:15 ` Tzung-Bi Shih
(?)
@ 2020-07-31 8:55 ` Cheng-yi Chiang
-1 siblings, 0 replies; 35+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31 8:55 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: Linux Kernel Mailing List, Mark Brown, Taniya Das, Rohit kumar,
Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
Douglas Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
ALSA development, Ajit Pandey
On Wed, Jul 22, 2020 at 11:15 AM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Tue, Jul 21, 2020 at 6:44 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..3beb2b129d01
> > --- /dev/null
> > +++ b/sound/soc/qcom/sc7180.c
> > @@ -0,0 +1,380 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > +//
> > +//sc7180.c -- ALSA SoC Machine driver for SC7180
> Insert an extra space between // and text to make it look better.
>
Fixed in v3.
> > +static int sc7180_headset_init(struct snd_soc_component *component);
> > +
> > +static struct snd_soc_aux_dev sc7180_headset_dev = {
> > + .dlc = COMP_EMPTY(),
> > + .init = sc7180_headset_init,
> > +};
> Move definition of sc7180_headset_dev after sc7180_headset_init( ) so
> that you don't need forward declaration of sc7180_headset_init( ).
>
Fixed in v3.
> > +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> > + SND_SOC_DAIFMT_NB_NF |
> > + SND_SOC_DAIFMT_I2S;
> > +
> > +static int sc7180_snd_startup(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);
> > + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > + int ret;
> > +
> > + switch (cpu_dai->id) {
> > + case MI2S_PRIMARY:
> > + 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, primary_dai_fmt);
> My comment on the previous thread may mislead. My original intent:
> move the DAIFMT setting to DAI link->dai_fmt in sc7180_parse_of( ).
>
> If you need to keep it as is: inline the SND_SOC_DAIFMT_* into
> snd_soc_dai_set_fmt( ) (i.e. eliminate primary_dai_fmt).
>
Fixed in v3. Since in v3 I am reusing parse function in common.c, I
will keep the format setting in startup.
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
> > + struct device_node *np;
> > + struct device_node *codec = NULL;
> > + struct device_node *platform = NULL;
> The function doesn't use platform.
>
sc7180_parse_of is removed in v3.
> > + 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;
> > +
> > + 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;
> > + }
> > +
> > + /* headset aux dev. */
> > + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> > + dev->of_node, "aux-dev", 0);
> > + if (!sc7180_headset_dev.dlc.of_node) {
> > + dev_err(dev,
> > + "Property 'aux-dev' missing/invalid\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Populate links */
> > + num_links = of_get_child_count(dev->of_node);
> Eliminate num_links but use card->num_links directly.
>
> > +
> > + /* Allocate the DAI link array */
> > + card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> > + GFP_KERNEL);
> > + if (!card->dai_link)
> > + return -ENOMEM;
> > +
> > + card->num_links = num_links;
> Ditto, eliminate it.
>
> > + link = card->dai_link;
> > +
> Eliminate the blank line to make "link = card->dai_link" and the
> following for-loop "a whole thing".
>
> > + 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;
> > + }
> > +
> > + link->playback_only = of_property_read_bool(np,
> > + "playback_only");
> > + link->capture_only = of_property_read_bool(np,
> > + "capture_only");
> > +
> > + if (link->playback_only && link->capture_only) {
> > + dev_err(card->dev,
> > + "getting both playback and capture only\n");
> ret = -EINVAL;
>
> > + goto err;
> > + }
> > +
> > + cpu = of_get_child_by_name(np, "cpu");
> > + codec = of_get_child_by_name(np, "codec");
> Move to below.
>
> > +
> > + 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);
> I may overlook it but I failed to find "#sound-dai-cells" in the
> dt-binding example. I think it should be in DTS?
>
> > + 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];
> I am not quite sure what it will be. I guess one of the following
> comes from DTS node name.
> #define MI2S_PRIMARY 0
> #define MI2S_SECONDARY 1
>
> > +
> > + 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;
> > + }
> > +
>
> Move "codec = of_get_child_by_name(np, "codec");" to here.
> > + 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->ops = &sc7180_ops;
> > + link++;
> > +
> > + of_node_put(cpu);
> > + of_node_put(codec);
> cpu = NULL;
> codec = NULL;
> In case of double of_node_put( ).
>
> > + }
> > +
> > + return 0;
> > +err:
> > + of_node_put(np);
> I guess you don't need this.
>
> > + of_node_put(cpu);
> > + of_node_put(codec);
> > + of_node_put(platform);
> Eliminate it, not used.
>
> > +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 = &sc7180_card;
> In this case, inline the initialization while declaration.
>
Fixed in v3.
> > +
> > + /* Allocate the private data */
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + card->dapm_widgets = sc7180_snd_widgets;
> > + card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
> Remove them.
>
Fixed in v3.
> > + card->dev = dev;
> > + dev_set_drvdata(dev, card);
> I guess you don't need this if using devm_snd_soc_register_card(...).
>
> Insert a blank line.
Fixed in v3.
> > + ret = sc7180_parse_of(card);
> > + if (ret) {
> > + dev_err(dev, "Error parsing OF data\n");
> > + return ret;
> > + }
> > +
> > + data->card = card;
> Looks like data->card is not used.
>
Removed in v3.
> > + snd_soc_card_set_drvdata(card, data);
> > +
> > + ret = snd_soc_register_card(card);
> > + if (ret) {
> > + dev_err(dev, "Sound card registration failed\n");
> > + return ret;
> > + }
> > + return ret;
> Just return devm_snd_soc_register_card(...);
>
Fixed in v3.
> > +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> > +{
> > + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> > +
> > + snd_soc_unregister_card(card);
> > + return 0;
> > +}
> Can be removed if using devm_snd_soc_register_card( ).
>
>
Removed in v3.
> I didn't go through all the cases. But it would be better if all "if
> (ret < 0)" can be replaced to "if (ret)".
Fixed in v3.
Thanks for the review!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-31 8:55 ` Cheng-yi Chiang
0 siblings, 0 replies; 35+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31 8:55 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: Taniya Das,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Tzung-Bi Shih, Banajit Goswami, ALSA development, Liam Girdwood,
linux-arm-msm, Patrick Lai, Takashi Iwai,
Linux Kernel Mailing List, Rob Herring, Bjorn Andersson,
Mark Brown, Rohit kumar, Andy Gross, Douglas Anderson,
Ajit Pandey, Dylan Reid, Jaroslav Kysela, Linux ARM
On Wed, Jul 22, 2020 at 11:15 AM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Tue, Jul 21, 2020 at 6:44 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..3beb2b129d01
> > --- /dev/null
> > +++ b/sound/soc/qcom/sc7180.c
> > @@ -0,0 +1,380 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > +//
> > +//sc7180.c -- ALSA SoC Machine driver for SC7180
> Insert an extra space between // and text to make it look better.
>
Fixed in v3.
> > +static int sc7180_headset_init(struct snd_soc_component *component);
> > +
> > +static struct snd_soc_aux_dev sc7180_headset_dev = {
> > + .dlc = COMP_EMPTY(),
> > + .init = sc7180_headset_init,
> > +};
> Move definition of sc7180_headset_dev after sc7180_headset_init( ) so
> that you don't need forward declaration of sc7180_headset_init( ).
>
Fixed in v3.
> > +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> > + SND_SOC_DAIFMT_NB_NF |
> > + SND_SOC_DAIFMT_I2S;
> > +
> > +static int sc7180_snd_startup(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);
> > + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > + int ret;
> > +
> > + switch (cpu_dai->id) {
> > + case MI2S_PRIMARY:
> > + 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, primary_dai_fmt);
> My comment on the previous thread may mislead. My original intent:
> move the DAIFMT setting to DAI link->dai_fmt in sc7180_parse_of( ).
>
> If you need to keep it as is: inline the SND_SOC_DAIFMT_* into
> snd_soc_dai_set_fmt( ) (i.e. eliminate primary_dai_fmt).
>
Fixed in v3. Since in v3 I am reusing parse function in common.c, I
will keep the format setting in startup.
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
> > + struct device_node *np;
> > + struct device_node *codec = NULL;
> > + struct device_node *platform = NULL;
> The function doesn't use platform.
>
sc7180_parse_of is removed in v3.
> > + 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;
> > +
> > + 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;
> > + }
> > +
> > + /* headset aux dev. */
> > + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> > + dev->of_node, "aux-dev", 0);
> > + if (!sc7180_headset_dev.dlc.of_node) {
> > + dev_err(dev,
> > + "Property 'aux-dev' missing/invalid\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Populate links */
> > + num_links = of_get_child_count(dev->of_node);
> Eliminate num_links but use card->num_links directly.
>
> > +
> > + /* Allocate the DAI link array */
> > + card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> > + GFP_KERNEL);
> > + if (!card->dai_link)
> > + return -ENOMEM;
> > +
> > + card->num_links = num_links;
> Ditto, eliminate it.
>
> > + link = card->dai_link;
> > +
> Eliminate the blank line to make "link = card->dai_link" and the
> following for-loop "a whole thing".
>
> > + 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;
> > + }
> > +
> > + link->playback_only = of_property_read_bool(np,
> > + "playback_only");
> > + link->capture_only = of_property_read_bool(np,
> > + "capture_only");
> > +
> > + if (link->playback_only && link->capture_only) {
> > + dev_err(card->dev,
> > + "getting both playback and capture only\n");
> ret = -EINVAL;
>
> > + goto err;
> > + }
> > +
> > + cpu = of_get_child_by_name(np, "cpu");
> > + codec = of_get_child_by_name(np, "codec");
> Move to below.
>
> > +
> > + 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);
> I may overlook it but I failed to find "#sound-dai-cells" in the
> dt-binding example. I think it should be in DTS?
>
> > + 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];
> I am not quite sure what it will be. I guess one of the following
> comes from DTS node name.
> #define MI2S_PRIMARY 0
> #define MI2S_SECONDARY 1
>
> > +
> > + 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;
> > + }
> > +
>
> Move "codec = of_get_child_by_name(np, "codec");" to here.
> > + 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->ops = &sc7180_ops;
> > + link++;
> > +
> > + of_node_put(cpu);
> > + of_node_put(codec);
> cpu = NULL;
> codec = NULL;
> In case of double of_node_put( ).
>
> > + }
> > +
> > + return 0;
> > +err:
> > + of_node_put(np);
> I guess you don't need this.
>
> > + of_node_put(cpu);
> > + of_node_put(codec);
> > + of_node_put(platform);
> Eliminate it, not used.
>
> > +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 = &sc7180_card;
> In this case, inline the initialization while declaration.
>
Fixed in v3.
> > +
> > + /* Allocate the private data */
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + card->dapm_widgets = sc7180_snd_widgets;
> > + card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
> Remove them.
>
Fixed in v3.
> > + card->dev = dev;
> > + dev_set_drvdata(dev, card);
> I guess you don't need this if using devm_snd_soc_register_card(...).
>
> Insert a blank line.
Fixed in v3.
> > + ret = sc7180_parse_of(card);
> > + if (ret) {
> > + dev_err(dev, "Error parsing OF data\n");
> > + return ret;
> > + }
> > +
> > + data->card = card;
> Looks like data->card is not used.
>
Removed in v3.
> > + snd_soc_card_set_drvdata(card, data);
> > +
> > + ret = snd_soc_register_card(card);
> > + if (ret) {
> > + dev_err(dev, "Sound card registration failed\n");
> > + return ret;
> > + }
> > + return ret;
> Just return devm_snd_soc_register_card(...);
>
Fixed in v3.
> > +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> > +{
> > + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> > +
> > + snd_soc_unregister_card(card);
> > + return 0;
> > +}
> Can be removed if using devm_snd_soc_register_card( ).
>
>
Removed in v3.
> I didn't go through all the cases. But it would be better if all "if
> (ret < 0)" can be replaced to "if (ret)".
Fixed in v3.
Thanks for the review!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-31 8:55 ` Cheng-yi Chiang
0 siblings, 0 replies; 35+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31 8:55 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: Taniya Das,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Tzung-Bi Shih, Banajit Goswami, ALSA development, Liam Girdwood,
linux-arm-msm, Patrick Lai, Takashi Iwai,
Linux Kernel Mailing List, Rob Herring, Bjorn Andersson,
Mark Brown, Rohit kumar, Andy Gross, Douglas Anderson,
Ajit Pandey, Dylan Reid, Linux ARM
On Wed, Jul 22, 2020 at 11:15 AM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Tue, Jul 21, 2020 at 6:44 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..3beb2b129d01
> > --- /dev/null
> > +++ b/sound/soc/qcom/sc7180.c
> > @@ -0,0 +1,380 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > +//
> > +//sc7180.c -- ALSA SoC Machine driver for SC7180
> Insert an extra space between // and text to make it look better.
>
Fixed in v3.
> > +static int sc7180_headset_init(struct snd_soc_component *component);
> > +
> > +static struct snd_soc_aux_dev sc7180_headset_dev = {
> > + .dlc = COMP_EMPTY(),
> > + .init = sc7180_headset_init,
> > +};
> Move definition of sc7180_headset_dev after sc7180_headset_init( ) so
> that you don't need forward declaration of sc7180_headset_init( ).
>
Fixed in v3.
> > +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> > + SND_SOC_DAIFMT_NB_NF |
> > + SND_SOC_DAIFMT_I2S;
> > +
> > +static int sc7180_snd_startup(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);
> > + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > + int ret;
> > +
> > + switch (cpu_dai->id) {
> > + case MI2S_PRIMARY:
> > + 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, primary_dai_fmt);
> My comment on the previous thread may mislead. My original intent:
> move the DAIFMT setting to DAI link->dai_fmt in sc7180_parse_of( ).
>
> If you need to keep it as is: inline the SND_SOC_DAIFMT_* into
> snd_soc_dai_set_fmt( ) (i.e. eliminate primary_dai_fmt).
>
Fixed in v3. Since in v3 I am reusing parse function in common.c, I
will keep the format setting in startup.
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
> > + struct device_node *np;
> > + struct device_node *codec = NULL;
> > + struct device_node *platform = NULL;
> The function doesn't use platform.
>
sc7180_parse_of is removed in v3.
> > + 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;
> > +
> > + 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;
> > + }
> > +
> > + /* headset aux dev. */
> > + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> > + dev->of_node, "aux-dev", 0);
> > + if (!sc7180_headset_dev.dlc.of_node) {
> > + dev_err(dev,
> > + "Property 'aux-dev' missing/invalid\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Populate links */
> > + num_links = of_get_child_count(dev->of_node);
> Eliminate num_links but use card->num_links directly.
>
> > +
> > + /* Allocate the DAI link array */
> > + card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> > + GFP_KERNEL);
> > + if (!card->dai_link)
> > + return -ENOMEM;
> > +
> > + card->num_links = num_links;
> Ditto, eliminate it.
>
> > + link = card->dai_link;
> > +
> Eliminate the blank line to make "link = card->dai_link" and the
> following for-loop "a whole thing".
>
> > + 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;
> > + }
> > +
> > + link->playback_only = of_property_read_bool(np,
> > + "playback_only");
> > + link->capture_only = of_property_read_bool(np,
> > + "capture_only");
> > +
> > + if (link->playback_only && link->capture_only) {
> > + dev_err(card->dev,
> > + "getting both playback and capture only\n");
> ret = -EINVAL;
>
> > + goto err;
> > + }
> > +
> > + cpu = of_get_child_by_name(np, "cpu");
> > + codec = of_get_child_by_name(np, "codec");
> Move to below.
>
> > +
> > + 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);
> I may overlook it but I failed to find "#sound-dai-cells" in the
> dt-binding example. I think it should be in DTS?
>
> > + 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];
> I am not quite sure what it will be. I guess one of the following
> comes from DTS node name.
> #define MI2S_PRIMARY 0
> #define MI2S_SECONDARY 1
>
> > +
> > + 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;
> > + }
> > +
>
> Move "codec = of_get_child_by_name(np, "codec");" to here.
> > + 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->ops = &sc7180_ops;
> > + link++;
> > +
> > + of_node_put(cpu);
> > + of_node_put(codec);
> cpu = NULL;
> codec = NULL;
> In case of double of_node_put( ).
>
> > + }
> > +
> > + return 0;
> > +err:
> > + of_node_put(np);
> I guess you don't need this.
>
> > + of_node_put(cpu);
> > + of_node_put(codec);
> > + of_node_put(platform);
> Eliminate it, not used.
>
> > +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 = &sc7180_card;
> In this case, inline the initialization while declaration.
>
Fixed in v3.
> > +
> > + /* Allocate the private data */
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + card->dapm_widgets = sc7180_snd_widgets;
> > + card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
> Remove them.
>
Fixed in v3.
> > + card->dev = dev;
> > + dev_set_drvdata(dev, card);
> I guess you don't need this if using devm_snd_soc_register_card(...).
>
> Insert a blank line.
Fixed in v3.
> > + ret = sc7180_parse_of(card);
> > + if (ret) {
> > + dev_err(dev, "Error parsing OF data\n");
> > + return ret;
> > + }
> > +
> > + data->card = card;
> Looks like data->card is not used.
>
Removed in v3.
> > + snd_soc_card_set_drvdata(card, data);
> > +
> > + ret = snd_soc_register_card(card);
> > + if (ret) {
> > + dev_err(dev, "Sound card registration failed\n");
> > + return ret;
> > + }
> > + return ret;
> Just return devm_snd_soc_register_card(...);
>
Fixed in v3.
> > +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> > +{
> > + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> > +
> > + snd_soc_unregister_card(card);
> > + return 0;
> > +}
> Can be removed if using devm_snd_soc_register_card( ).
>
>
Removed in v3.
> I didn't go through all the cases. But it would be better if all "if
> (ret < 0)" can be replaced to "if (ret)".
Fixed in v3.
Thanks for the review!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
2020-07-21 10:44 ` Cheng-Yi Chiang
(?)
@ 2020-07-22 9:25 ` Srinivas Kandagatla
-1 siblings, 0 replies; 35+ messages in thread
From: Srinivas Kandagatla @ 2020-07-22 9:25 UTC (permalink / raw)
To: Cheng-Yi Chiang, linux-kernel
Cc: Mark Brown, Taniya Das, Rohit kumar, Banajit Goswami,
Patrick Lai, Andy Gross, Bjorn Andersson, Liam Girdwood,
Rob Herring, Jaroslav Kysela, Takashi Iwai, dianders, dgreid,
tzungbi, linux-arm-kernel, linux-arm-msm, devicetree, alsa-devel,
Ajit Pandey
Few comments apart from Tzung-Bi comments,
On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> 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: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
> sound/soc/qcom/Kconfig | 11 ++
> sound/soc/qcom/Makefile | 2 +
> sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 393 insertions(+)
> create mode 100644 sound/soc/qcom/sc7180.c
>
> diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> new file mode 100644
> index 000000000000..3beb2b129d01
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,380 @@
> +// 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 <dt-bindings/sound/sc7180-lpass.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <sound/core.h>
> +#include <sound/jack.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <uapi/linux/input-event-codes.h>
> +
> +#include "../codecs/rt5682.h"
> +#include "common.h"
What is that you are using from this header?
> +#include "lpass.h"
> +
> +#define DEFAULT_SAMPLE_RATE_48K 48000
> +#define DEFAULT_MCLK_RATE 19200000
> +#define RT5682_PLL1_FREQ (48000 * 512)
> +
> +static int sc7180_headset_init(struct snd_soc_component *component);
> +
> +static struct snd_soc_aux_dev sc7180_headset_dev = {
> + .dlc = COMP_EMPTY(),
> + .init = sc7180_headset_init,
> +};
Can you explain why can you not use snd_soc_component_set_jack() on the
codec component from link->init() callback?
> +
> +struct sc7180_snd_data {
> + struct snd_soc_jack jack;
> + struct snd_soc_card *card;
> + u32 pri_mi2s_clk_count;
> +};
> +
> +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_headset_init(struct snd_soc_component *component)
> +{
> + struct snd_soc_card *card = component->card;
> + struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> + struct snd_jack *jack;
> + int rval;
> +
> + 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 Headset 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);
> +
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +
> +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> + SND_SOC_DAIFMT_NB_NF |
> + SND_SOC_DAIFMT_I2S;
> +
> +static int sc7180_snd_startup(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);
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> + int ret;
> +
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + 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, primary_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:
> + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> + cpu_dai->id);
> + return -EINVAL;
> + }
> + 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:
> + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> + cpu_dai->id);
> + break;
> + }
> +}
> +
> +static const struct snd_soc_ops sc7180_ops = {
> + .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 struct snd_soc_card sc7180_card = {
> + .owner = THIS_MODULE,
> + .aux_dev = &sc7180_headset_dev,
> + .num_aux_devs = 1,
> + .dapm_widgets = sc7180_snd_widgets,
> + .num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
> +};
> +
> +static int sc7180_parse_of(struct snd_soc_card *card)
> +{
This code is getting duplicated in various places like
apq8016_sbc_parse_of, it will be nice to common this up, if possible!
> + 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;
> +
> + 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;
> + }
> +
> + /* headset aux dev. */
> + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> + dev->of_node, "aux-dev", 0);
> + if (!sc7180_headset_dev.dlc.of_node) {
> + dev_err(dev,
> + "Property 'aux-dev' missing/invalid\n");
> + return -EINVAL;
> + }
> +
> + /* Populate links */
> + num_links = of_get_child_count(dev->of_node);
> +
> + /* Allocate the DAI link array */
> + card->dai_link = devm_kcalloc(dev, 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;
> + }
> +
> + link->playback_only = of_property_read_bool(np,
> + "playback_only");
> + link->capture_only = of_property_read_bool(np,
> + "capture_only");
Does this even work??
You have replaced "-" with "_" for property name?
> +
> + if (link->playback_only && link->capture_only) {
> + dev_err(card->dev,
> + "getting both playback and capture only\n");
> + goto err;
> + }
> +
> + 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->ops = &sc7180_ops;
> + 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);
> + return ret;
> +}
> +
> +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 = &sc7180_card;
> +
> + /* Allocate the private data */
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + 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");
> + return ret;
> + }
> +
> + data->card = card;
> + snd_soc_card_set_drvdata(card, data);
> +
> + ret = snd_soc_register_card(card);
devm_snd_soc_register_card()??
> + if (ret) {
> + dev_err(dev, "Sound card registration failed\n");
> + return ret;
> + }
> + return ret;
> +}
> +
> +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> +{
> + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> +
> + snd_soc_unregister_card(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");
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-22 9:25 ` Srinivas Kandagatla
0 siblings, 0 replies; 35+ messages in thread
From: Srinivas Kandagatla @ 2020-07-22 9:25 UTC (permalink / raw)
To: Cheng-Yi Chiang, linux-kernel
Cc: Taniya Das, devicetree, tzungbi, Banajit Goswami, alsa-devel,
linux-arm-msm, Patrick Lai, Takashi Iwai, Liam Girdwood,
Rob Herring, Bjorn Andersson, Andy Gross, Rohit kumar,
Mark Brown, dianders, Ajit Pandey, dgreid, Jaroslav Kysela,
linux-arm-kernel
Few comments apart from Tzung-Bi comments,
On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> 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: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
> sound/soc/qcom/Kconfig | 11 ++
> sound/soc/qcom/Makefile | 2 +
> sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 393 insertions(+)
> create mode 100644 sound/soc/qcom/sc7180.c
>
> diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> new file mode 100644
> index 000000000000..3beb2b129d01
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,380 @@
> +// 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 <dt-bindings/sound/sc7180-lpass.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <sound/core.h>
> +#include <sound/jack.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <uapi/linux/input-event-codes.h>
> +
> +#include "../codecs/rt5682.h"
> +#include "common.h"
What is that you are using from this header?
> +#include "lpass.h"
> +
> +#define DEFAULT_SAMPLE_RATE_48K 48000
> +#define DEFAULT_MCLK_RATE 19200000
> +#define RT5682_PLL1_FREQ (48000 * 512)
> +
> +static int sc7180_headset_init(struct snd_soc_component *component);
> +
> +static struct snd_soc_aux_dev sc7180_headset_dev = {
> + .dlc = COMP_EMPTY(),
> + .init = sc7180_headset_init,
> +};
Can you explain why can you not use snd_soc_component_set_jack() on the
codec component from link->init() callback?
> +
> +struct sc7180_snd_data {
> + struct snd_soc_jack jack;
> + struct snd_soc_card *card;
> + u32 pri_mi2s_clk_count;
> +};
> +
> +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_headset_init(struct snd_soc_component *component)
> +{
> + struct snd_soc_card *card = component->card;
> + struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> + struct snd_jack *jack;
> + int rval;
> +
> + 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 Headset 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);
> +
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +
> +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> + SND_SOC_DAIFMT_NB_NF |
> + SND_SOC_DAIFMT_I2S;
> +
> +static int sc7180_snd_startup(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);
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> + int ret;
> +
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + 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, primary_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:
> + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> + cpu_dai->id);
> + return -EINVAL;
> + }
> + 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:
> + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> + cpu_dai->id);
> + break;
> + }
> +}
> +
> +static const struct snd_soc_ops sc7180_ops = {
> + .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 struct snd_soc_card sc7180_card = {
> + .owner = THIS_MODULE,
> + .aux_dev = &sc7180_headset_dev,
> + .num_aux_devs = 1,
> + .dapm_widgets = sc7180_snd_widgets,
> + .num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
> +};
> +
> +static int sc7180_parse_of(struct snd_soc_card *card)
> +{
This code is getting duplicated in various places like
apq8016_sbc_parse_of, it will be nice to common this up, if possible!
> + 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;
> +
> + 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;
> + }
> +
> + /* headset aux dev. */
> + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> + dev->of_node, "aux-dev", 0);
> + if (!sc7180_headset_dev.dlc.of_node) {
> + dev_err(dev,
> + "Property 'aux-dev' missing/invalid\n");
> + return -EINVAL;
> + }
> +
> + /* Populate links */
> + num_links = of_get_child_count(dev->of_node);
> +
> + /* Allocate the DAI link array */
> + card->dai_link = devm_kcalloc(dev, 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;
> + }
> +
> + link->playback_only = of_property_read_bool(np,
> + "playback_only");
> + link->capture_only = of_property_read_bool(np,
> + "capture_only");
Does this even work??
You have replaced "-" with "_" for property name?
> +
> + if (link->playback_only && link->capture_only) {
> + dev_err(card->dev,
> + "getting both playback and capture only\n");
> + goto err;
> + }
> +
> + 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->ops = &sc7180_ops;
> + 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);
> + return ret;
> +}
> +
> +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 = &sc7180_card;
> +
> + /* Allocate the private data */
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + 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");
> + return ret;
> + }
> +
> + data->card = card;
> + snd_soc_card_set_drvdata(card, data);
> +
> + ret = snd_soc_register_card(card);
devm_snd_soc_register_card()??
> + if (ret) {
> + dev_err(dev, "Sound card registration failed\n");
> + return ret;
> + }
> + return ret;
> +}
> +
> +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> +{
> + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> +
> + snd_soc_unregister_card(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");
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-22 9:25 ` Srinivas Kandagatla
0 siblings, 0 replies; 35+ messages in thread
From: Srinivas Kandagatla @ 2020-07-22 9:25 UTC (permalink / raw)
To: Cheng-Yi Chiang, linux-kernel
Cc: Taniya Das, devicetree, tzungbi, Banajit Goswami, alsa-devel,
linux-arm-msm, Patrick Lai, Takashi Iwai, Liam Girdwood,
Rob Herring, Bjorn Andersson, Andy Gross, Rohit kumar,
Mark Brown, dianders, Ajit Pandey, dgreid, linux-arm-kernel
Few comments apart from Tzung-Bi comments,
On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> 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: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
> sound/soc/qcom/Kconfig | 11 ++
> sound/soc/qcom/Makefile | 2 +
> sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 393 insertions(+)
> create mode 100644 sound/soc/qcom/sc7180.c
>
> diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> new file mode 100644
> index 000000000000..3beb2b129d01
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,380 @@
> +// 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 <dt-bindings/sound/sc7180-lpass.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <sound/core.h>
> +#include <sound/jack.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <uapi/linux/input-event-codes.h>
> +
> +#include "../codecs/rt5682.h"
> +#include "common.h"
What is that you are using from this header?
> +#include "lpass.h"
> +
> +#define DEFAULT_SAMPLE_RATE_48K 48000
> +#define DEFAULT_MCLK_RATE 19200000
> +#define RT5682_PLL1_FREQ (48000 * 512)
> +
> +static int sc7180_headset_init(struct snd_soc_component *component);
> +
> +static struct snd_soc_aux_dev sc7180_headset_dev = {
> + .dlc = COMP_EMPTY(),
> + .init = sc7180_headset_init,
> +};
Can you explain why can you not use snd_soc_component_set_jack() on the
codec component from link->init() callback?
> +
> +struct sc7180_snd_data {
> + struct snd_soc_jack jack;
> + struct snd_soc_card *card;
> + u32 pri_mi2s_clk_count;
> +};
> +
> +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_headset_init(struct snd_soc_component *component)
> +{
> + struct snd_soc_card *card = component->card;
> + struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> + struct snd_jack *jack;
> + int rval;
> +
> + 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 Headset 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);
> +
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +
> +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> + SND_SOC_DAIFMT_NB_NF |
> + SND_SOC_DAIFMT_I2S;
> +
> +static int sc7180_snd_startup(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);
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> + int ret;
> +
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + 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, primary_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:
> + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> + cpu_dai->id);
> + return -EINVAL;
> + }
> + 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:
> + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> + cpu_dai->id);
> + break;
> + }
> +}
> +
> +static const struct snd_soc_ops sc7180_ops = {
> + .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 struct snd_soc_card sc7180_card = {
> + .owner = THIS_MODULE,
> + .aux_dev = &sc7180_headset_dev,
> + .num_aux_devs = 1,
> + .dapm_widgets = sc7180_snd_widgets,
> + .num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
> +};
> +
> +static int sc7180_parse_of(struct snd_soc_card *card)
> +{
This code is getting duplicated in various places like
apq8016_sbc_parse_of, it will be nice to common this up, if possible!
> + 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;
> +
> + 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;
> + }
> +
> + /* headset aux dev. */
> + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> + dev->of_node, "aux-dev", 0);
> + if (!sc7180_headset_dev.dlc.of_node) {
> + dev_err(dev,
> + "Property 'aux-dev' missing/invalid\n");
> + return -EINVAL;
> + }
> +
> + /* Populate links */
> + num_links = of_get_child_count(dev->of_node);
> +
> + /* Allocate the DAI link array */
> + card->dai_link = devm_kcalloc(dev, 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;
> + }
> +
> + link->playback_only = of_property_read_bool(np,
> + "playback_only");
> + link->capture_only = of_property_read_bool(np,
> + "capture_only");
Does this even work??
You have replaced "-" with "_" for property name?
> +
> + if (link->playback_only && link->capture_only) {
> + dev_err(card->dev,
> + "getting both playback and capture only\n");
> + goto err;
> + }
> +
> + 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->ops = &sc7180_ops;
> + 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);
> + return ret;
> +}
> +
> +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 = &sc7180_card;
> +
> + /* Allocate the private data */
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + 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");
> + return ret;
> + }
> +
> + data->card = card;
> + snd_soc_card_set_drvdata(card, data);
> +
> + ret = snd_soc_register_card(card);
devm_snd_soc_register_card()??
> + if (ret) {
> + dev_err(dev, "Sound card registration failed\n");
> + return ret;
> + }
> + return ret;
> +}
> +
> +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> +{
> + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> +
> + snd_soc_unregister_card(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");
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
2020-07-22 9:25 ` Srinivas Kandagatla
(?)
@ 2020-07-23 18:44 ` Stephan Gerhold
-1 siblings, 0 replies; 35+ messages in thread
From: Stephan Gerhold @ 2020-07-23 18:44 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Cheng-Yi Chiang, linux-kernel, Mark Brown, Taniya Das,
Rohit kumar, Banajit Goswami, Patrick Lai, Andy Gross,
Bjorn Andersson, Liam Girdwood, Rob Herring, Jaroslav Kysela,
Takashi Iwai, dianders, dgreid, tzungbi, linux-arm-kernel,
linux-arm-msm, devicetree, alsa-devel, Ajit Pandey
On Wed, Jul 22, 2020 at 10:25:14AM +0100, Srinivas Kandagatla wrote:
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
>
> This code is getting duplicated in various places like apq8016_sbc_parse_of,
> it will be nice to common this up, if possible!
>
FYI, I started work on making apq8016_sbc use qcom_snd_parse_of()
a while ago already, but didn't find the time to finish it up.
I have now sent it, this should make it possible to use the common
qcom_snd_parse_of() function in this driver as well.
See: https://lore.kernel.org/alsa-devel/20200723183904.321040-1-stephan@gerhold.net/
Stephan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-23 18:44 ` Stephan Gerhold
0 siblings, 0 replies; 35+ messages in thread
From: Stephan Gerhold @ 2020-07-23 18:44 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Taniya Das, devicetree, tzungbi, Banajit Goswami, alsa-devel,
Liam Girdwood, linux-arm-msm, Patrick Lai, Takashi Iwai,
linux-kernel, Rob Herring, Bjorn Andersson, Mark Brown,
Rohit kumar, Andy Gross, dianders, Ajit Pandey, dgreid,
Jaroslav Kysela, linux-arm-kernel, Cheng-Yi Chiang
On Wed, Jul 22, 2020 at 10:25:14AM +0100, Srinivas Kandagatla wrote:
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
>
> This code is getting duplicated in various places like apq8016_sbc_parse_of,
> it will be nice to common this up, if possible!
>
FYI, I started work on making apq8016_sbc use qcom_snd_parse_of()
a while ago already, but didn't find the time to finish it up.
I have now sent it, this should make it possible to use the common
qcom_snd_parse_of() function in this driver as well.
See: https://lore.kernel.org/alsa-devel/20200723183904.321040-1-stephan@gerhold.net/
Stephan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-23 18:44 ` Stephan Gerhold
0 siblings, 0 replies; 35+ messages in thread
From: Stephan Gerhold @ 2020-07-23 18:44 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Taniya Das, devicetree, tzungbi, Banajit Goswami, alsa-devel,
Liam Girdwood, linux-arm-msm, Patrick Lai, Takashi Iwai,
linux-kernel, Rob Herring, Bjorn Andersson, Mark Brown,
Rohit kumar, Andy Gross, dianders, Ajit Pandey, dgreid,
linux-arm-kernel, Cheng-Yi Chiang
On Wed, Jul 22, 2020 at 10:25:14AM +0100, Srinivas Kandagatla wrote:
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
>
> This code is getting duplicated in various places like apq8016_sbc_parse_of,
> it will be nice to common this up, if possible!
>
FYI, I started work on making apq8016_sbc use qcom_snd_parse_of()
a while ago already, but didn't find the time to finish it up.
I have now sent it, this should make it possible to use the common
qcom_snd_parse_of() function in this driver as well.
See: https://lore.kernel.org/alsa-devel/20200723183904.321040-1-stephan@gerhold.net/
Stephan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
2020-07-23 18:44 ` Stephan Gerhold
(?)
@ 2020-07-31 8:55 ` Cheng-yi Chiang
-1 siblings, 0 replies; 35+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31 8:55 UTC (permalink / raw)
To: Stephan Gerhold
Cc: Srinivas Kandagatla, linux-kernel, Mark Brown, Taniya Das,
Rohit kumar, Banajit Goswami, Patrick Lai, Andy Gross,
Bjorn Andersson, Liam Girdwood, Rob Herring, Jaroslav Kysela,
Takashi Iwai, Doug Anderson, Dylan Reid, Tzung-Bi Shih,
Linux ARM, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
Ajit Pandey
On Fri, Jul 24, 2020 at 2:45 AM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Wed, Jul 22, 2020 at 10:25:14AM +0100, Srinivas Kandagatla wrote:
> > > +static int sc7180_parse_of(struct snd_soc_card *card)
> > > +{
> >
> > This code is getting duplicated in various places like apq8016_sbc_parse_of,
> > it will be nice to common this up, if possible!
> >
>
> FYI, I started work on making apq8016_sbc use qcom_snd_parse_of()
> a while ago already, but didn't find the time to finish it up.
> I have now sent it, this should make it possible to use the common
> qcom_snd_parse_of() function in this driver as well.
>
> See: https://lore.kernel.org/alsa-devel/20200723183904.321040-1-stephan@gerhold.net/
>
> Stephan
>
Hi Stephan, thanks a lot for jumping on this to help.
It indeed makes this new driver much cleaner.
I have tested with your patches and it works great.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-31 8:55 ` Cheng-yi Chiang
0 siblings, 0 replies; 35+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31 8:55 UTC (permalink / raw)
To: Stephan Gerhold
Cc: Taniya Das,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Tzung-Bi Shih, Banajit Goswami,
moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
Liam Girdwood, linux-arm-msm, Patrick Lai, Takashi Iwai,
linux-kernel, Rob Herring, Bjorn Andersson, Mark Brown,
Rohit kumar, Andy Gross, Doug Anderson, Srinivas Kandagatla,
Dylan Reid, Jaroslav Kysela, Ajit Pandey, Linux ARM
On Fri, Jul 24, 2020 at 2:45 AM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Wed, Jul 22, 2020 at 10:25:14AM +0100, Srinivas Kandagatla wrote:
> > > +static int sc7180_parse_of(struct snd_soc_card *card)
> > > +{
> >
> > This code is getting duplicated in various places like apq8016_sbc_parse_of,
> > it will be nice to common this up, if possible!
> >
>
> FYI, I started work on making apq8016_sbc use qcom_snd_parse_of()
> a while ago already, but didn't find the time to finish it up.
> I have now sent it, this should make it possible to use the common
> qcom_snd_parse_of() function in this driver as well.
>
> See: https://lore.kernel.org/alsa-devel/20200723183904.321040-1-stephan@gerhold.net/
>
> Stephan
>
Hi Stephan, thanks a lot for jumping on this to help.
It indeed makes this new driver much cleaner.
I have tested with your patches and it works great.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-31 8:55 ` Cheng-yi Chiang
0 siblings, 0 replies; 35+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31 8:55 UTC (permalink / raw)
To: Stephan Gerhold
Cc: Taniya Das,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Tzung-Bi Shih, Banajit Goswami,
moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
Liam Girdwood, linux-arm-msm, Patrick Lai, Takashi Iwai,
linux-kernel, Rob Herring, Bjorn Andersson, Mark Brown,
Rohit kumar, Andy Gross, Doug Anderson, Srinivas Kandagatla,
Dylan Reid, Ajit Pandey, Linux ARM
On Fri, Jul 24, 2020 at 2:45 AM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Wed, Jul 22, 2020 at 10:25:14AM +0100, Srinivas Kandagatla wrote:
> > > +static int sc7180_parse_of(struct snd_soc_card *card)
> > > +{
> >
> > This code is getting duplicated in various places like apq8016_sbc_parse_of,
> > it will be nice to common this up, if possible!
> >
>
> FYI, I started work on making apq8016_sbc use qcom_snd_parse_of()
> a while ago already, but didn't find the time to finish it up.
> I have now sent it, this should make it possible to use the common
> qcom_snd_parse_of() function in this driver as well.
>
> See: https://lore.kernel.org/alsa-devel/20200723183904.321040-1-stephan@gerhold.net/
>
> Stephan
>
Hi Stephan, thanks a lot for jumping on this to help.
It indeed makes this new driver much cleaner.
I have tested with your patches and it works great.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
2020-07-22 9:25 ` Srinivas Kandagatla
(?)
@ 2020-07-31 8:55 ` Cheng-yi Chiang
-1 siblings, 0 replies; 35+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31 8:55 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: linux-kernel, Mark Brown, Taniya Das, Rohit kumar,
Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
Ajit Pandey
On Wed, Jul 22, 2020 at 5:25 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> Few comments apart from Tzung-Bi comments,
>
Thanks a lot for reviewing the patch!
> On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> > 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: Cheng-Yi Chiang <cychiang@chromium.org>
> > ---
> > sound/soc/qcom/Kconfig | 11 ++
> > sound/soc/qcom/Makefile | 2 +
> > sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 393 insertions(+)
> > create mode 100644 sound/soc/qcom/sc7180.c
> >
>
> > diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> > new file mode 100644
> > index 000000000000..3beb2b129d01
> > --- /dev/null
> > +++ b/sound/soc/qcom/sc7180.c
> > @@ -0,0 +1,380 @@
> > +// 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 <dt-bindings/sound/sc7180-lpass.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <sound/core.h>
> > +#include <sound/jack.h>
> > +#include <sound/pcm.h>
> > +#include <sound/pcm_params.h>
> > +#include <sound/soc.h>
> > +#include <uapi/linux/input-event-codes.h>
> > +
> > +#include "../codecs/rt5682.h"
> > +#include "common.h"
>
> What is that you are using from this header?
>
It is not used in v2, but will be used in v3.
> > +#include "lpass.h"
> > +
> > +#define DEFAULT_SAMPLE_RATE_48K 48000
> > +#define DEFAULT_MCLK_RATE 19200000
> > +#define RT5682_PLL1_FREQ (48000 * 512)
> > +
> > +static int sc7180_headset_init(struct snd_soc_component *component);
> > +
> > +static struct snd_soc_aux_dev sc7180_headset_dev = {
> > + .dlc = COMP_EMPTY(),
> > + .init = sc7180_headset_init,
> > +};
>
> Can you explain why can you not use snd_soc_component_set_jack() on the
> codec component from link->init() callback?
>
I am using snd_soc_component_set_jack, but only on aux device.
As Tzungbi suggested in his comment on v1 machine driver patch, the
current approach in other qcom driver like sdm845.c and apq8016_sbc.c
of setting jack is not clean.
For example, in sdm845.c, a bool jack_etup is needed in
sdm845_snd_data so that jack will be created once.
Also, the machine driver assumes that snd_soc_component_set_jack is
used when cpu_dai->id == PRIMARY_MI2S_RX, which is not easy to
understand.
In apq8016_sbc.c, apq8016_sbc_dai_init tries to set jack on every
codec_dai which does not make sense.
These downsides can be avoided by using aux-dev so that the logic of
setting jack is easy to understand, and dai_init function can be much
cleaner.
> > +
> > +struct sc7180_snd_data {
> > + struct snd_soc_jack jack;
> > + struct snd_soc_card *card;
> > + u32 pri_mi2s_clk_count;
> > +};
> > +
> > +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_headset_init(struct snd_soc_component *component)
> > +{
> > + struct snd_soc_card *card = component->card;
> > + struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> > + struct snd_jack *jack;
> > + int rval;
> > +
> > + 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 Headset 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);
> > +
> > + 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;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> > + SND_SOC_DAIFMT_NB_NF |
> > + SND_SOC_DAIFMT_I2S;
> > +
> > +static int sc7180_snd_startup(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);
> > + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > + int ret;
> > +
> > + switch (cpu_dai->id) {
> > + case MI2S_PRIMARY:
> > + 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, primary_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:
> > + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> > + cpu_dai->id);
> > + return -EINVAL;
> > + }
> > + 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:
> > + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> > + cpu_dai->id);
> > + break;
> > + }
> > +}
> > +
> > +static const struct snd_soc_ops sc7180_ops = {
> > + .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 struct snd_soc_card sc7180_card = {
> > + .owner = THIS_MODULE,
> > + .aux_dev = &sc7180_headset_dev,
> > + .num_aux_devs = 1,
> > + .dapm_widgets = sc7180_snd_widgets,
> > + .num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
> > +};
> > +
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
>
> This code is getting duplicated in various places like
> apq8016_sbc_parse_of, it will be nice to common this up, if possible!
>
>
Thanks for the refactoring work of Stephan I can reuse qcom_snd_parse_of.
> > + 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;
> > +
> > + 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;
> > + }
> > +
> > + /* headset aux dev. */
> > + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> > + dev->of_node, "aux-dev", 0);
> > + if (!sc7180_headset_dev.dlc.of_node) {
> > + dev_err(dev,
> > + "Property 'aux-dev' missing/invalid\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Populate links */
> > + num_links = of_get_child_count(dev->of_node);
> > +
> > + /* Allocate the DAI link array */
> > + card->dai_link = devm_kcalloc(dev, 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;
> > + }
> > +
> > + link->playback_only = of_property_read_bool(np,
> > + "playback_only");
> > + link->capture_only = of_property_read_bool(np,
> > + "capture_only");
>
> Does this even work??
> You have replaced "-" with "_" for property name?
>
This does not work because of the typo. But it is not needed so I
removed it in v3.
soc_new_pcm already takes care of capability in non-dpcm case.
> > +
> > + if (link->playback_only && link->capture_only) {
> > + dev_err(card->dev,
> > + "getting both playback and capture only\n");
> > + goto err;
> > + }
> > +
> > + 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->ops = &sc7180_ops;
> > + 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);
> > + return ret;
> > +}
> > +
> > +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 = &sc7180_card;
> > +
> > + /* Allocate the private data */
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + 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");
> > + return ret;
> > + }
> > +
> > + data->card = card;
> > + snd_soc_card_set_drvdata(card, data);
> > +
> > + ret = snd_soc_register_card(card);
>
> devm_snd_soc_register_card()??
>
Fixed in v3.
> > + if (ret) {
> > + dev_err(dev, "Sound card registration failed\n");
> > + return ret;
> > + }
> > + return ret;
> > +}
> > +
> > +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> > +{
> > + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> > +
> > + snd_soc_unregister_card(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");
> >
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-31 8:55 ` Cheng-yi Chiang
0 siblings, 0 replies; 35+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31 8:55 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Taniya Das,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Tzung-Bi Shih, Banajit Goswami,
moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
Liam Girdwood, linux-arm-msm, Patrick Lai, Takashi Iwai,
linux-kernel, Rob Herring, Bjorn Andersson, Mark Brown,
Rohit kumar, Andy Gross, Doug Anderson, Ajit Pandey, Dylan Reid,
Jaroslav Kysela, Linux ARM
On Wed, Jul 22, 2020 at 5:25 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> Few comments apart from Tzung-Bi comments,
>
Thanks a lot for reviewing the patch!
> On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> > 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: Cheng-Yi Chiang <cychiang@chromium.org>
> > ---
> > sound/soc/qcom/Kconfig | 11 ++
> > sound/soc/qcom/Makefile | 2 +
> > sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 393 insertions(+)
> > create mode 100644 sound/soc/qcom/sc7180.c
> >
>
> > diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> > new file mode 100644
> > index 000000000000..3beb2b129d01
> > --- /dev/null
> > +++ b/sound/soc/qcom/sc7180.c
> > @@ -0,0 +1,380 @@
> > +// 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 <dt-bindings/sound/sc7180-lpass.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <sound/core.h>
> > +#include <sound/jack.h>
> > +#include <sound/pcm.h>
> > +#include <sound/pcm_params.h>
> > +#include <sound/soc.h>
> > +#include <uapi/linux/input-event-codes.h>
> > +
> > +#include "../codecs/rt5682.h"
> > +#include "common.h"
>
> What is that you are using from this header?
>
It is not used in v2, but will be used in v3.
> > +#include "lpass.h"
> > +
> > +#define DEFAULT_SAMPLE_RATE_48K 48000
> > +#define DEFAULT_MCLK_RATE 19200000
> > +#define RT5682_PLL1_FREQ (48000 * 512)
> > +
> > +static int sc7180_headset_init(struct snd_soc_component *component);
> > +
> > +static struct snd_soc_aux_dev sc7180_headset_dev = {
> > + .dlc = COMP_EMPTY(),
> > + .init = sc7180_headset_init,
> > +};
>
> Can you explain why can you not use snd_soc_component_set_jack() on the
> codec component from link->init() callback?
>
I am using snd_soc_component_set_jack, but only on aux device.
As Tzungbi suggested in his comment on v1 machine driver patch, the
current approach in other qcom driver like sdm845.c and apq8016_sbc.c
of setting jack is not clean.
For example, in sdm845.c, a bool jack_etup is needed in
sdm845_snd_data so that jack will be created once.
Also, the machine driver assumes that snd_soc_component_set_jack is
used when cpu_dai->id == PRIMARY_MI2S_RX, which is not easy to
understand.
In apq8016_sbc.c, apq8016_sbc_dai_init tries to set jack on every
codec_dai which does not make sense.
These downsides can be avoided by using aux-dev so that the logic of
setting jack is easy to understand, and dai_init function can be much
cleaner.
> > +
> > +struct sc7180_snd_data {
> > + struct snd_soc_jack jack;
> > + struct snd_soc_card *card;
> > + u32 pri_mi2s_clk_count;
> > +};
> > +
> > +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_headset_init(struct snd_soc_component *component)
> > +{
> > + struct snd_soc_card *card = component->card;
> > + struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> > + struct snd_jack *jack;
> > + int rval;
> > +
> > + 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 Headset 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);
> > +
> > + 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;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> > + SND_SOC_DAIFMT_NB_NF |
> > + SND_SOC_DAIFMT_I2S;
> > +
> > +static int sc7180_snd_startup(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);
> > + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > + int ret;
> > +
> > + switch (cpu_dai->id) {
> > + case MI2S_PRIMARY:
> > + 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, primary_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:
> > + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> > + cpu_dai->id);
> > + return -EINVAL;
> > + }
> > + 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:
> > + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> > + cpu_dai->id);
> > + break;
> > + }
> > +}
> > +
> > +static const struct snd_soc_ops sc7180_ops = {
> > + .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 struct snd_soc_card sc7180_card = {
> > + .owner = THIS_MODULE,
> > + .aux_dev = &sc7180_headset_dev,
> > + .num_aux_devs = 1,
> > + .dapm_widgets = sc7180_snd_widgets,
> > + .num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
> > +};
> > +
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
>
> This code is getting duplicated in various places like
> apq8016_sbc_parse_of, it will be nice to common this up, if possible!
>
>
Thanks for the refactoring work of Stephan I can reuse qcom_snd_parse_of.
> > + 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;
> > +
> > + 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;
> > + }
> > +
> > + /* headset aux dev. */
> > + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> > + dev->of_node, "aux-dev", 0);
> > + if (!sc7180_headset_dev.dlc.of_node) {
> > + dev_err(dev,
> > + "Property 'aux-dev' missing/invalid\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Populate links */
> > + num_links = of_get_child_count(dev->of_node);
> > +
> > + /* Allocate the DAI link array */
> > + card->dai_link = devm_kcalloc(dev, 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;
> > + }
> > +
> > + link->playback_only = of_property_read_bool(np,
> > + "playback_only");
> > + link->capture_only = of_property_read_bool(np,
> > + "capture_only");
>
> Does this even work??
> You have replaced "-" with "_" for property name?
>
This does not work because of the typo. But it is not needed so I
removed it in v3.
soc_new_pcm already takes care of capability in non-dpcm case.
> > +
> > + if (link->playback_only && link->capture_only) {
> > + dev_err(card->dev,
> > + "getting both playback and capture only\n");
> > + goto err;
> > + }
> > +
> > + 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->ops = &sc7180_ops;
> > + 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);
> > + return ret;
> > +}
> > +
> > +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 = &sc7180_card;
> > +
> > + /* Allocate the private data */
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + 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");
> > + return ret;
> > + }
> > +
> > + data->card = card;
> > + snd_soc_card_set_drvdata(card, data);
> > +
> > + ret = snd_soc_register_card(card);
>
> devm_snd_soc_register_card()??
>
Fixed in v3.
> > + if (ret) {
> > + dev_err(dev, "Sound card registration failed\n");
> > + return ret;
> > + }
> > + return ret;
> > +}
> > +
> > +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> > +{
> > + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> > +
> > + snd_soc_unregister_card(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");
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
@ 2020-07-31 8:55 ` Cheng-yi Chiang
0 siblings, 0 replies; 35+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31 8:55 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Taniya Das,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Tzung-Bi Shih, Banajit Goswami,
moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
Liam Girdwood, linux-arm-msm, Patrick Lai, Takashi Iwai,
linux-kernel, Rob Herring, Bjorn Andersson, Mark Brown,
Rohit kumar, Andy Gross, Doug Anderson, Ajit Pandey, Dylan Reid,
Linux ARM
On Wed, Jul 22, 2020 at 5:25 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> Few comments apart from Tzung-Bi comments,
>
Thanks a lot for reviewing the patch!
> On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> > 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: Cheng-Yi Chiang <cychiang@chromium.org>
> > ---
> > sound/soc/qcom/Kconfig | 11 ++
> > sound/soc/qcom/Makefile | 2 +
> > sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 393 insertions(+)
> > create mode 100644 sound/soc/qcom/sc7180.c
> >
>
> > diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> > new file mode 100644
> > index 000000000000..3beb2b129d01
> > --- /dev/null
> > +++ b/sound/soc/qcom/sc7180.c
> > @@ -0,0 +1,380 @@
> > +// 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 <dt-bindings/sound/sc7180-lpass.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <sound/core.h>
> > +#include <sound/jack.h>
> > +#include <sound/pcm.h>
> > +#include <sound/pcm_params.h>
> > +#include <sound/soc.h>
> > +#include <uapi/linux/input-event-codes.h>
> > +
> > +#include "../codecs/rt5682.h"
> > +#include "common.h"
>
> What is that you are using from this header?
>
It is not used in v2, but will be used in v3.
> > +#include "lpass.h"
> > +
> > +#define DEFAULT_SAMPLE_RATE_48K 48000
> > +#define DEFAULT_MCLK_RATE 19200000
> > +#define RT5682_PLL1_FREQ (48000 * 512)
> > +
> > +static int sc7180_headset_init(struct snd_soc_component *component);
> > +
> > +static struct snd_soc_aux_dev sc7180_headset_dev = {
> > + .dlc = COMP_EMPTY(),
> > + .init = sc7180_headset_init,
> > +};
>
> Can you explain why can you not use snd_soc_component_set_jack() on the
> codec component from link->init() callback?
>
I am using snd_soc_component_set_jack, but only on aux device.
As Tzungbi suggested in his comment on v1 machine driver patch, the
current approach in other qcom driver like sdm845.c and apq8016_sbc.c
of setting jack is not clean.
For example, in sdm845.c, a bool jack_etup is needed in
sdm845_snd_data so that jack will be created once.
Also, the machine driver assumes that snd_soc_component_set_jack is
used when cpu_dai->id == PRIMARY_MI2S_RX, which is not easy to
understand.
In apq8016_sbc.c, apq8016_sbc_dai_init tries to set jack on every
codec_dai which does not make sense.
These downsides can be avoided by using aux-dev so that the logic of
setting jack is easy to understand, and dai_init function can be much
cleaner.
> > +
> > +struct sc7180_snd_data {
> > + struct snd_soc_jack jack;
> > + struct snd_soc_card *card;
> > + u32 pri_mi2s_clk_count;
> > +};
> > +
> > +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_headset_init(struct snd_soc_component *component)
> > +{
> > + struct snd_soc_card *card = component->card;
> > + struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> > + struct snd_jack *jack;
> > + int rval;
> > +
> > + 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 Headset 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);
> > +
> > + 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;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> > + SND_SOC_DAIFMT_NB_NF |
> > + SND_SOC_DAIFMT_I2S;
> > +
> > +static int sc7180_snd_startup(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);
> > + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > + int ret;
> > +
> > + switch (cpu_dai->id) {
> > + case MI2S_PRIMARY:
> > + 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, primary_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:
> > + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> > + cpu_dai->id);
> > + return -EINVAL;
> > + }
> > + 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:
> > + dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> > + cpu_dai->id);
> > + break;
> > + }
> > +}
> > +
> > +static const struct snd_soc_ops sc7180_ops = {
> > + .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 struct snd_soc_card sc7180_card = {
> > + .owner = THIS_MODULE,
> > + .aux_dev = &sc7180_headset_dev,
> > + .num_aux_devs = 1,
> > + .dapm_widgets = sc7180_snd_widgets,
> > + .num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
> > +};
> > +
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
>
> This code is getting duplicated in various places like
> apq8016_sbc_parse_of, it will be nice to common this up, if possible!
>
>
Thanks for the refactoring work of Stephan I can reuse qcom_snd_parse_of.
> > + 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;
> > +
> > + 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;
> > + }
> > +
> > + /* headset aux dev. */
> > + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> > + dev->of_node, "aux-dev", 0);
> > + if (!sc7180_headset_dev.dlc.of_node) {
> > + dev_err(dev,
> > + "Property 'aux-dev' missing/invalid\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Populate links */
> > + num_links = of_get_child_count(dev->of_node);
> > +
> > + /* Allocate the DAI link array */
> > + card->dai_link = devm_kcalloc(dev, 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;
> > + }
> > +
> > + link->playback_only = of_property_read_bool(np,
> > + "playback_only");
> > + link->capture_only = of_property_read_bool(np,
> > + "capture_only");
>
> Does this even work??
> You have replaced "-" with "_" for property name?
>
This does not work because of the typo. But it is not needed so I
removed it in v3.
soc_new_pcm already takes care of capability in non-dpcm case.
> > +
> > + if (link->playback_only && link->capture_only) {
> > + dev_err(card->dev,
> > + "getting both playback and capture only\n");
> > + goto err;
> > + }
> > +
> > + 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->ops = &sc7180_ops;
> > + 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);
> > + return ret;
> > +}
> > +
> > +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 = &sc7180_card;
> > +
> > + /* Allocate the private data */
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + 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");
> > + return ret;
> > + }
> > +
> > + data->card = card;
> > + snd_soc_card_set_drvdata(card, data);
> > +
> > + ret = snd_soc_register_card(card);
>
> devm_snd_soc_register_card()??
>
Fixed in v3.
> > + if (ret) {
> > + dev_err(dev, "Sound card registration failed\n");
> > + return ret;
> > + }
> > + return ret;
> > +}
> > +
> > +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> > +{
> > + struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> > +
> > + snd_soc_unregister_card(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");
> >
^ permalink raw reply [flat|nested] 35+ messages in thread