-----Original Message----- From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org] Sent: 16 October 2022 08:48 PM To: Padmanabhan Rajanbabu p.rajanbabu@samsung.com; lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com; perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com; alim.akhtar@samsung.com; rcsekar@samsung.com; aswani.reddy@samsung.com Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux- kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org Subject: Re: [PATCH 4/6] ASoC: samsung: fsd: Add FSD soundcard driver
On 14/10/2022 06:21, Padmanabhan Rajanbabu wrote:
Add a soundcard driver for FSD audio interface to bridge the CPU DAI of samsung I2S with the codec and platform driver.
Thank you for your patch. There is something to discuss/improve.
+#define FSD_PREFIX "tesla," +#define FSD_DAI_SRC_PCLK 3 +#define FSD_DAI_RFS_192 192 +#define FSD_DAI_BFS_48 48 +#define FSD_DAI_BFS_96 96 +#define FSD_DAI_BFS_192 192
+struct fsd_card_priv {
- struct snd_soc_card card;
- struct snd_soc_dai_link *dai_link;
- u32 tdm_slots;
- u32 tdm_slot_width;
+};
+static unsigned int fsd_card_get_rfs(unsigned int rate) {
- return FSD_DAI_RFS_192;
This wrapper is a bit silly...
okay, will remove the wrapper and assign the macro value directly
+}
+static unsigned int fsd_card_get_bfs(unsigned int channels) {
- switch (channels) {
- case 1:
- case 2:
return FSD_DAI_BFS_48;
- case 3:
- case 4:
return FSD_DAI_BFS_96;
- case 5:
- case 6:
- case 7:
- case 8:
return FSD_DAI_BFS_192;
- default:
return FSD_DAI_BFS_48;
- }
+}
+static unsigned int fsd_card_get_psr(unsigned int rate) {
- switch (rate) {
- case 8000: return 43;
- case 11025: return 31;
- case 16000: return 21;
- case 22050: return 16;
- case 32000: return 11;
- case 44100: return 8;
- case 48000: return 7;
- case 64000: return 5;
- case 88200: return 4;
- case 96000: return 4;
- case 192000: return 2;
- default: return 1;
- }
+}
+static int fsd_card_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params
*params) {
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_card *card = rtd->card;
- struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
- struct snd_soc_dai_link *link = rtd->dai_link;
- struct fsd_card_priv *priv = snd_soc_card_get_drvdata(card);
- unsigned int rfs, bfs, psr;
- int ret = 0, cdclk_dir;
- rfs = fsd_card_get_rfs(params_rate(params));
- bfs = fsd_card_get_bfs(params_channels(params));
- psr = fsd_card_get_psr(params_rate(params));
- /* Configure the Root Clock Source */
- ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_OPCLK,
false, FSD_DAI_SRC_PCLK);
- if (ret < 0) {
dev_err(card->dev, "Failed to set OPCLK: %d\n", ret);
goto err;
- }
- ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_RCLKSRC_0,
false, false);
- if (ret < 0) {
dev_err(card->dev, "Failed to set RCLKSRC: %d\n", ret);
Don't you need to cleanup on error paths?
we might not be needing, since the sound card neither configures any clock sources directly nor involves in any memory allocation during hw_params.
goto err;
- }
- /* Set CPU DAI configuration */
- ret = snd_soc_dai_set_fmt(cpu_dai, link->dai_fmt);
- if (ret < 0) {
dev_err(card->dev, "Failed to set DAIFMT: %d\n", ret);
goto err;
- }
- if (link->dai_fmt & SND_SOC_DAIFMT_CBC_CFC) {
cdclk_dir = SND_SOC_CLOCK_OUT;
- } else if (link->dai_fmt & SND_SOC_DAIFMT_CBP_CFP) {
cdclk_dir = SND_SOC_CLOCK_IN;
- } else {
dev_err(card->dev, "Missing Clock Master information\n");
goto err;
- }
- /* Set Clock Source for CDCLK */
- ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_CDCLK,
rfs, cdclk_dir);
- if (ret < 0) {
dev_err(card->dev, "Failed to set CDCLK: %d\n", ret);
goto err;
- }
- ret = snd_soc_dai_set_clkdiv(cpu_dai, SAMSUNG_I2S_DIV_RCLK, psr);
- if (ret < 0) {
dev_err(card->dev, "Failed to set PSR: %d\n", ret);
goto err;
- }
- ret = snd_soc_dai_set_clkdiv(cpu_dai, SAMSUNG_I2S_DIV_BCLK, bfs);
- if (ret < 0) {
dev_err(card->dev, "Failed to set BCLK: %d\n", ret);
goto err;
- }
- if (priv->tdm_slots) {
ret = snd_soc_dai_set_tdm_slot(cpu_dai, false, false,
priv->tdm_slots, priv->tdm_slot_width);
if (ret < 0) {
dev_err(card->dev,
"Failed to configure in TDM mode:%d\n", ret);
goto err;
}
- }
+err:
- return ret;
+}
+static const struct snd_soc_ops fsd_card_ops = {
- .hw_params = fsd_card_hw_params,
+};
+static struct fsd_card_priv *fsd_card_parse_of(struct snd_soc_card +*card) {
- struct fsd_card_priv *priv;
- struct snd_soc_dai_link *link;
- struct device *dev = card->dev;
- struct device_node *node = dev->of_node;
- struct device_node *np, *cpu_node, *codec_node;
- struct snd_soc_dai_link_component *dlc;
- int ret, id = 0, num_links;
- ret = snd_soc_of_parse_card_name(card, "model");
- if (ret) {
dev_err(dev, "Error parsing card name: %d\n", ret);
return ERR_PTR(ret);
return dev_err_probe
Okay
- }
- if (of_property_read_bool(dev->of_node, "widgets")) {
ret = snd_soc_of_parse_audio_simple_widgets(card, "widgets");
if (ret)
return ERR_PTR(ret);
- }
- /* Add DAPM routes to the card */
- if (of_property_read_bool(node, "audio-routing")) {
ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
if (ret)
return ERR_PTR(ret);
- }
- num_links = of_get_child_count(node);
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return ERR_PTR(-ENOMEM);
- priv->dai_link = devm_kzalloc(dev, num_links * sizeof(*priv-
dai_link),
- GFP_KERNEL);
- if (!priv->dai_link)
return ERR_PTR(-ENOMEM);
- priv->tdm_slots = 0;
- priv->tdm_slot_width = 0;
- snd_soc_of_parse_tdm_slot(node, NULL, NULL, &priv->tdm_slots,
&priv->tdm_slot_width);
- link = priv->dai_link;
- for_each_child_of_node(node, np) {
dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
if (!dlc)
return ERR_PTR(-ENOMEM);
link->id = id;
link->cpus = &dlc[0];
link->platforms = &dlc[1];
link->num_cpus = 1;
link->num_codecs = 1;
link->num_platforms = 1;
cpu_node = of_get_child_by_name(np, "cpu");
if (!cpu_node) {
dev_err(dev, "Missing CPU/Codec node\n");
ret = -EINVAL;
goto err_cpu_node;
}
ret = snd_soc_of_get_dai_link_cpus(dev, cpu_node, link);
if (ret < 0) {
dev_err(dev, "Error Parsing CPU DAI name\n");
goto err_cpu_name;
}
link->platforms->of_node = link->cpus->of_node;
codec_node = of_get_child_by_name(np, "codec");
if (codec_node) {
ret = snd_soc_of_get_dai_link_codecs(dev, codec_node,
link);
if (ret < 0) {
dev_err(dev, "Error Parsing Codec DAI name\n");
goto err_codec_name;
}
} else {
dlc = devm_kzalloc(dev, sizeof(*dlc), GFP_KERNEL);
if (!dlc) {
ret = -ENOMEM;
goto err_cpu_name;
}
link->codecs = dlc;
link->codecs->dai_name = "snd-soc-dummy-dai";
link->codecs->name = "snd-soc-dummy";
link->dynamic = 1;
snd_soc_dai_link_set_capabilities(link);
link->ignore_suspend = 1;
link->nonatomic = 1;
}
ret = asoc_simple_parse_daifmt(dev, np, codec_node,
FSD_PREFIX, &link->dai_fmt);
if (ret)
link->dai_fmt = SND_SOC_DAIFMT_NB_NF |
SND_SOC_DAIFMT_CBC_CFC |
SND_SOC_DAIFMT_I2S;
ret = of_property_read_string(np, "link-name", &link-
name);
if (ret) {
dev_err(card->dev, "Error Parsing link name\n");
goto err_codec_name;
}
link->stream_name = link->name;
link->ops = &fsd_card_ops;
link++;
id++;
of_node_put(cpu_node);
of_node_put(codec_node);
- }
- card->dai_link = priv->dai_link;
- card->num_links = num_links;
- return priv;
+err_codec_name:
- of_node_put(codec_node);
+err_cpu_name:
- of_node_put(cpu_node);
+err_cpu_node:
- of_node_put(np);
- return ERR_PTR(ret);
+}
+static int fsd_platform_probe(struct platform_device *pdev) {
- struct device *dev = &pdev->dev;
- struct snd_soc_card *card;
- struct fsd_card_priv *fsd_priv;
- card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
- if (!card)
return -ENOMEM;
- card->dev = dev;
- fsd_priv = fsd_card_parse_of(card);
Drop the indentation before =
Okay
- if (IS_ERR(fsd_priv)) {
dev_err(&pdev->dev, "Error Parsing DAI Link: %ld\n",
PTR_ERR(fsd_priv));
return PTR_ERR(fsd_priv);
return dev_err_probe
Okay
- }
- snd_soc_card_set_drvdata(card, fsd_priv);
- return devm_snd_soc_register_card(&pdev->dev, card); }
+static const struct of_device_id fsd_device_id[] = {
- { .compatible = "tesla,fsd-card" },
- {},
+}; +MODULE_DEVICE_TABLE(of, fsd_device_id);
+static struct platform_driver fsd_platform_driver = {
- .driver = {
.name = "fsd-card",
.of_match_table = of_match_ptr(fsd_device_id),
of_match_ptr comes with maybe_unused. Or drop it.
Okay
Best regards, Krzysztof
Thank you for reviewing the patch.