| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 21 Oct 2022 14:34:01 +0530
From: "Padmanabhan Rajanbabu" <p.rajanbabu@...sung.com>
To: "'Krzysztof Kozlowski'" <krzysztof.kozlowski@...aro.org>,
<lgirdwood@...il.com>, <broonie@...nel.org>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <s.nawrocki@...sung.com>,
<perex@...ex.cz>, <tiwai@...e.com>, <pankaj.dubey@...sung.com>,
<alim.akhtar@...sung.com>, <rcsekar@...sung.com>,
<aswani.reddy@...sung.com>
Cc: <alsa-devel@...a-project.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-samsung-soc@...r.kernel.org>
Subject: RE: [PATCH 4/6] ASoC: samsung: fsd: Add FSD soundcard driver
> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@...aro.org]
> Sent: 16 October 2022 08:48 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@...sung.com>;
> lgirdwood@...il.com; broonie@...nel.org; robh+dt@...nel.org;
> krzysztof.kozlowski+dt@...aro.org; s.nawrocki@...sung.com;
> perex@...ex.cz; tiwai@...e.com; pankaj.dubey@...sung.com;
> alim.akhtar@...sung.com; rcsekar@...sung.com;
> aswani.reddy@...sung.com
> Cc: alsa-devel@...a-project.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; linux-samsung-soc@...r.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.
Powered by blists - more mailing lists