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...

+}

+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?

• 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

• }
• 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 =

• 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

• }
• 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.

Best regards, Krzysztof