* [PATCH v2 0/2] Add documentation and machine driver for SC7180 sound card
@ 2020-07-21 10:44 Cheng-Yi Chiang
2020-07-21 10:44 ` [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
2020-07-21 10:44 ` [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
0 siblings, 2 replies; 11+ 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, dgreid, linux-arm-kernel, Cheng-Yi Chiang
Note:
- The machine driver patch depends on LPASS patch series so it is not ready to be merged now.
ASoC: qcom: Add support for SC7180 lpass variant https://patchwork.kernel.org/cover/11650649/
- The machine driver patch is made by the collaboration of
Cheng-Yi Chiang <cychiang@chromium.org>
Rohit kumar <rohitkr@codeaurora.org>
Ajit Pandey <ajitp@codeaurora.org>
But Ajit has left codeaurora.
Changes from v1 to v2:
- Ducumentation: Addressed all suggestions from Doug.
- Machine driver:
- Fix comment style for license.
- Sort includes.
- Remove sc7180_snd_hw_params.
- Remove sc7180_dai_init and use aux device instead for headset jack registration.
- Statically define format for Primary MI2S.
- Atomic is not a concern because there is mutex in card to make sure
startup and shutdown happen sequentially.
- Fix missing return -EINVAL in startup.
- Use static sound card.
- Use devm_kzalloc to avoid kfree.
Thanks for the review!
Ajit Pandey (1):
ASoC: qcom: sc7180: Add machine driver for sound card registration
Cheng-Yi Chiang (1):
ASoC: qcom: dt-bindings: Add sc7180 machine bindings
.../bindings/sound/qcom,sc7180.yaml | 130 ++++++
sound/soc/qcom/Kconfig | 11 +
sound/soc/qcom/Makefile | 2 +
sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++
4 files changed, 523 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
create mode 100644 sound/soc/qcom/sc7180.c
--
2.28.0.rc0.105.gf9edc3c819-goog
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
2020-07-21 10:44 [PATCH v2 0/2] Add documentation and machine driver for SC7180 sound card Cheng-Yi Chiang
@ 2020-07-21 10:44 ` Cheng-Yi Chiang
2020-07-22 9:25 ` Srinivas Kandagatla
2020-07-21 10:44 ` [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
1 sibling, 1 reply; 11+ 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, dgreid, linux-arm-kernel, Cheng-Yi Chiang
Add devicetree bindings documentation file for sc7180 sound card.
Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
---
.../bindings/sound/qcom,sc7180.yaml | 130 ++++++++++++++++++
1 file changed, 130 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
diff --git a/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
new file mode 100644
index 000000000000..82f9483276eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/qcom,sc7180.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies Inc. SC7180 ASoC sound card driver
+
+maintainers:
+ - Rohit kumar <rohitkr@codeaurora.org>
+ - Cheng-Yi Chiang <cychiang@chromium.org>
+
+description:
+ This binding describes the SC7180 sound card which uses LPASS for audio.
+
+definitions:
+
+ dai:
+ type: object
+ properties:
+ sound-dai:
+ maxItems: 1
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+ description: phandle array of the codec or CPU DAI
+
+ required:
+ - sound-dai
+
+properties:
+ compatible:
+ contains:
+ const: qcom,sc7180-sndcard
+
+ audio-routing:
+ $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+ description:
+ A list of the connections between audio components. Each entry is a
+ pair of strings, the first being the connection's sink, the second
+ being the connection's source.
+
+ model:
+ $ref: /schemas/types.yaml#/definitions/string
+ description: User specified audio sound card name
+
+ aux-dev:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: phandle of the codec for headset detection
+
+patternProperties:
+ "^dai-link(@[0-9]+)?$":
+ description:
+ Each subnode represents a dai link. Subnodes of each dai links would be
+ cpu/codec dais.
+
+ type: object
+
+ properties:
+ link-name:
+ description: Indicates dai-link name and PCM stream name.
+ $ref: /schemas/types.yaml#/definitions/string
+ maxItems: 1
+
+ reg:
+ description: dai link address.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ maxItems: 1
+
+ playback-only:
+ description: Specify that the dai link is only for playback.
+ $ref: /schemas/types.yaml#/definitions/flag
+
+ capture-only:
+ description: Specify that the dai link is only for capture.
+ $ref: /schemas/types.yaml#/definitions/flag
+
+ cpu:
+ $ref: "#/definitions/dai"
+
+ codec:
+ $ref: "#/definitions/dai"
+
+ required:
+ - link-name
+ - reg
+ - cpu
+ - codec
+
+ additionalProperties: false
+
+examples:
+
+ - |
+ sound {
+ compatible = "qcom,sc7180-sndcard";
+ model = "sc7180-snd-card";
+
+ audio-routing =
+ "Headphone Jack", "HPOL",
+ "Headphone Jack", "HPOR";
+
+ aux-dev = <&alc5682>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ dai-link@0 {
+ reg = <0>;
+ link-name = "MultiMedia0";
+ cpu {
+ sound-dai = <&lpass_cpu 0>;
+ };
+
+ codec {
+ sound-dai = <&alc5682 0>;
+ };
+ };
+
+ dai-link@1 {
+ reg = <1>;
+ link-name = "MultiMedia1";
+ playback-only;
+ cpu {
+ sound-dai = <&lpass_cpu 1>;
+ };
+
+ codec {
+ sound-dai = <&max98357a>;
+ };
+ };
+ };
--
2.28.0.rc0.105.gf9edc3c819-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
2020-07-21 10:44 [PATCH v2 0/2] Add documentation and machine driver for SC7180 sound card Cheng-Yi Chiang
2020-07-21 10:44 ` [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
@ 2020-07-21 10:44 ` Cheng-Yi Chiang
2020-07-22 3:15 ` Tzung-Bi Shih
2020-07-22 9:25 ` Srinivas Kandagatla
1 sibling, 2 replies; 11+ 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] 11+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
2020-07-21 10:44 ` [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
@ 2020-07-22 3:15 ` Tzung-Bi Shih
2020-07-31 8:55 ` Cheng-yi Chiang
2020-07-22 9:25 ` Srinivas Kandagatla
1 sibling, 1 reply; 11+ 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] 11+ messages in thread
* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
2020-07-21 10:44 ` [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
2020-07-22 3:15 ` Tzung-Bi Shih
@ 2020-07-22 9:25 ` Srinivas Kandagatla
2020-07-23 18:44 ` Stephan Gerhold
2020-07-31 8:55 ` Cheng-yi Chiang
1 sibling, 2 replies; 11+ 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] 11+ messages in thread
* Re: [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
2020-07-21 10:44 ` [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
@ 2020-07-22 9:25 ` Srinivas Kandagatla
2020-07-31 8:55 ` Cheng-yi Chiang
0 siblings, 1 reply; 11+ 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, dgreid, pierre-louis.bossart,
linux-arm-kernel
On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> Add devicetree bindings documentation file for sc7180 sound card.
>
> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
> .../bindings/sound/qcom,sc7180.yaml | 130 ++++++++++++++++++
> 1 file changed, 130 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
>
> diff --git a/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> new file mode 100644
> index 000000000000..82f9483276eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> @@ -0,0 +1,130 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/qcom,sc7180.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies Inc. SC7180 ASoC sound card driver
> +
> +maintainers:
> + - Rohit kumar <rohitkr@codeaurora.org>
> + - Cheng-Yi Chiang <cychiang@chromium.org>
> +
> +description:
> + This binding describes the SC7180 sound card which uses LPASS for audio.
> +
> +definitions:
> +
> + dai:
> + type: object
> + properties:
> + sound-dai:
> + maxItems: 1
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + description: phandle array of the codec or CPU DAI
> +
> + required:
> + - sound-dai
> +
> +properties:
> + compatible:
> + contains:
> + const: qcom,sc7180-sndcard
> +
> + audio-routing:
> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> + description:
> + A list of the connections between audio components. Each entry is a
> + pair of strings, the first being the connection's sink, the second
> + being the connection's source.
> +
> + model:
> + $ref: /schemas/types.yaml#/definitions/string
> + description: User specified audio sound card name
> +
> + aux-dev:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: phandle of the codec for headset detection
Why do we need this? You should be able to set the jack for codec
snd_soc_component_set_jack()?
> +
> +patternProperties:
> + "^dai-link(@[0-9]+)?$":
> + description:
> + Each subnode represents a dai link. Subnodes of each dai links would be
> + cpu/codec dais.
> +
> + type: object
> +
> + properties:
> + link-name:
> + description: Indicates dai-link name and PCM stream name.
> + $ref: /schemas/types.yaml#/definitions/string
> + maxItems: 1
> +
> + reg:
> + description: dai link address.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + maxItems: 1
Why do we need this?? I have not seen the parsing code using this.
> +
> + playback-only:
> + description: Specify that the dai link is only for playback.
> + $ref: /schemas/types.yaml#/definitions/flag
> +
> + capture-only:
> + description: Specify that the dai link is only for capture.
> + $ref: /schemas/types.yaml#/definitions/flag
> +
Are these because the cpu/codec dais are single directional?
If so you can extend snd_soc_dai_link_set_capabilities() and use this
function.
--srini
> + cpu:
> + $ref: "#/definitions/dai"
> +
> + codec:
> + $ref: "#/definitions/dai"
> +
> + required:
> + - link-name
> + - reg
> + - cpu
> + - codec
> +
> + additionalProperties: false
> +
> +examples:
> +
> + - |
> + sound {
> + compatible = "qcom,sc7180-sndcard";
> + model = "sc7180-snd-card";
> +
> + audio-routing =
> + "Headphone Jack", "HPOL",
> + "Headphone Jack", "HPOR";
> +
> + aux-dev = <&alc5682>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + dai-link@0 {
> + reg = <0>;
> + link-name = "MultiMedia0";
> + cpu {
> + sound-dai = <&lpass_cpu 0>;
> + };
> +
> + codec {
> + sound-dai = <&alc5682 0>;
> + };
> + };
> +
> + dai-link@1 {
> + reg = <1>;
> + link-name = "MultiMedia1";
> + playback-only;
> + cpu {
> + sound-dai = <&lpass_cpu 1>;
> + };
> +
> + codec {
> + sound-dai = <&max98357a>;
> + };
> + };
> + };
>
^ permalink raw reply [flat|nested] 11+ 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
2020-07-31 8:55 ` Cheng-yi Chiang
2020-07-31 8:55 ` Cheng-yi Chiang
1 sibling, 1 reply; 11+ 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] 11+ messages in thread
* Re: [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
2020-07-22 9:25 ` Srinivas Kandagatla
@ 2020-07-31 8:55 ` Cheng-yi Chiang
0 siblings, 0 replies; 11+ 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, Dylan Reid,
Pierre-Louis Bossart, Linux ARM
On Wed, Jul 22, 2020 at 5:25 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> > Add devicetree bindings documentation file for sc7180 sound card.
> >
> > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> > ---
> > .../bindings/sound/qcom,sc7180.yaml | 130 ++++++++++++++++++
> > 1 file changed, 130 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> > new file mode 100644
> > index 000000000000..82f9483276eb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> > @@ -0,0 +1,130 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/qcom,sc7180.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Technologies Inc. SC7180 ASoC sound card driver
> > +
> > +maintainers:
> > + - Rohit kumar <rohitkr@codeaurora.org>
> > + - Cheng-Yi Chiang <cychiang@chromium.org>
> > +
> > +description:
> > + This binding describes the SC7180 sound card which uses LPASS for audio.
> > +
> > +definitions:
> > +
> > + dai:
> > + type: object
> > + properties:
> > + sound-dai:
> > + maxItems: 1
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + description: phandle array of the codec or CPU DAI
> > +
> > + required:
> > + - sound-dai
> > +
> > +properties:
> > + compatible:
> > + contains:
> > + const: qcom,sc7180-sndcard
> > +
> > + audio-routing:
> > + $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> > + description:
> > + A list of the connections between audio components. Each entry is a
> > + pair of strings, the first being the connection's sink, the second
> > + being the connection's source.
> > +
> > + model:
> > + $ref: /schemas/types.yaml#/definitions/string
> > + description: User specified audio sound card name
> > +
> > + aux-dev:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: phandle of the codec for headset detection
>
>
> Why do we need this? You should be able to set the jack for codec
> snd_soc_component_set_jack()?
>
>
I put my explanation in the machine driver patch since there is more
context there.
> > +
> > +patternProperties:
> > + "^dai-link(@[0-9]+)?$":
> > + description:
> > + Each subnode represents a dai link. Subnodes of each dai links would be
> > + cpu/codec dais.
> > +
> > + type: object
> > +
> > + properties:
> > + link-name:
> > + description: Indicates dai-link name and PCM stream name.
> > + $ref: /schemas/types.yaml#/definitions/string
> > + maxItems: 1
> > +
> > + reg:
> > + description: dai link address.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + maxItems: 1
>
> Why do we need this?? I have not seen the parsing code using this.
>
>
When checking the yaml file using dt_binding_check, I got this warnings:
$ make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
CHKDT Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
SCHEMA Documentation/devicetree/bindings/processed-schema-examples.yaml
DTC Documentation/devicetree/bindings/sound/qcom,sc7180.example.dt.yaml
Documentation/devicetree/bindings/sound/qcom,sc7180.example.dts:32.24-41.15:
Warning (unit_address_vs_reg): /example-0/sound/dai-link@0: node has a
unit name, but no reg or ranges property
Documentation/devicetree/bindings/sound/qcom,sc7180.example.dts:43.24-52.15:
Warning (unit_address_vs_reg): /example-0/sound/dai-link@1: node has a
unit name, but no reg or ranges property
CHECK Documentation/devicetree/bindings/sound/qcom,sc7180.example.dt.yaml
Should I ignore these warnings because reg is not used in the driver ?
> > +
> > + playback-only:
> > + description: Specify that the dai link is only for playback.
> > + $ref: /schemas/types.yaml#/definitions/flag
> > +
> > + capture-only:
> > + description: Specify that the dai link is only for capture.
> > + $ref: /schemas/types.yaml#/definitions/flag
> > +
>
> Are these because the cpu/codec dais are single directional?
>
> If so you can extend snd_soc_dai_link_set_capabilities() and use this
> function.
I found that this is not needed since soc_new_pcm already takes care
of checking capture/playback capability in non-dpcm cases.
>
>
> --srini
>
Thanks for reviewing the patch!
> > + cpu:
> > + $ref: "#/definitions/dai"
> > +
> > + codec:
> > + $ref: "#/definitions/dai"
> > +
> > + required:
> > + - link-name
> > + - reg
> > + - cpu
> > + - codec
> > +
> > + additionalProperties: false
> > +
> > +examples:
> > +
> > + - |
> > + sound {
> > + compatible = "qcom,sc7180-sndcard";
> > + model = "sc7180-snd-card";
> > +
> > + audio-routing =
> > + "Headphone Jack", "HPOL",
> > + "Headphone Jack", "HPOR";
> > +
> > + aux-dev = <&alc5682>;
> > +
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + dai-link@0 {
> > + reg = <0>;
> > + link-name = "MultiMedia0";
> > + cpu {
> > + sound-dai = <&lpass_cpu 0>;
> > + };
> > +
> > + codec {
> > + sound-dai = <&alc5682 0>;
> > + };
> > + };
> > +
> > + dai-link@1 {
> > + reg = <1>;
> > + link-name = "MultiMedia1";
> > + playback-only;
> > + cpu {
> > + sound-dai = <&lpass_cpu 1>;
> > + };
> > +
> > + codec {
> > + sound-dai = <&max98357a>;
> > + };
> > + };
> > + };
> >
^ permalink raw reply [flat|nested] 11+ 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
0 siblings, 0 replies; 11+ 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] 11+ 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
@ 2020-07-31 8:55 ` Cheng-yi Chiang
1 sibling, 0 replies; 11+ 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] 11+ 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
0 siblings, 0 replies; 11+ 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] 11+ messages in thread
end of thread, other threads:[~2020-07-31 8:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 10:44 [PATCH v2 0/2] Add documentation and machine driver for SC7180 sound card Cheng-Yi Chiang
2020-07-21 10:44 ` [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
2020-07-22 9:25 ` Srinivas Kandagatla
2020-07-31 8:55 ` Cheng-yi Chiang
2020-07-21 10:44 ` [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
2020-07-22 3:15 ` Tzung-Bi Shih
2020-07-31 8:55 ` Cheng-yi Chiang
2020-07-22 9:25 ` Srinivas Kandagatla
2020-07-23 18:44 ` Stephan Gerhold
2020-07-31 8:55 ` Cheng-yi Chiang
2020-07-31 8:55 ` Cheng-yi Chiang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).