Re: [PATCH v1] ASoC: codecs: wcd937x: Add wcd937x codec driver

On 05/12/2023 13:34, Prasad Kumpatla wrote:

Add wcd937x codec and wcd937x-sdw soundwire drivers.

Signed-off-by: Mohammad Rafi Shaik quic_mohs@quicinc.com Signed-off-by: Prasad Kumpatla quic_pkumpatl@quicinc.com

---

sound/soc/codecs/Kconfig | 20 + sound/soc/codecs/Makefile | 7 + sound/soc/codecs/wcd937x-sdw.c | 1147 ++++++++++++ sound/soc/codecs/wcd937x.c | 3047 ++++++++++++++++++++++++++++++++ sound/soc/codecs/wcd937x.h | 654 +++++++

Please run scripts/checkpatch.pl and fix reported warnings. Some warnings can be ignored, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear.

The warning about undocumented ABI is important. You cannot just ignore it.

5 files changed, 4875 insertions(+) create mode 100644 sound/soc/codecs/wcd937x-sdw.c create mode 100644 sound/soc/codecs/wcd937x.c create mode 100644 sound/soc/codecs/wcd937x.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 3429419ca694..b89118e8c391 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -275,6 +275,7 @@ config SND_SOC_ALL_CODECS imply SND_SOC_UDA1380 imply SND_SOC_WCD9335 imply SND_SOC_WCD934X

• imply SND_SOC_WCD937X_SDW imply SND_SOC_WCD938X_SDW imply SND_SOC_LPASS_MACRO_COMMON imply SND_SOC_LPASS_RX_MACRO

@@ -2038,6 +2039,25 @@ config SND_SOC_WCD934X The WCD9340/9341 is a audio codec IC Integrated in Qualcomm SoCs like SDM845.

+config SND_SOC_WCD937X

• depends on SND_SOC_WCD937X_SDW
• tristate
• depends on SOUNDWIRE || !SOUNDWIRE
• select SND_SOC_WCD_CLASSH

+config SND_SOC_WCD937X_SDW

That's one big patch. How about splitting the patch per driver for easier review?

• tristate "WCD9370/WCD9375 Codec - SDW"
• select SND_SOC_WCD937X
• select SND_SOC_WCD_MBHC
• select REGMAP_IRQ
• depends on SOUNDWIRE
• select REGMAP_SOUNDWIRE
• help

•  The WCD9370/9375 is an audio codec IC used with SoCs
  

•  like SC7280 or QCM6490 chipsets, and it connected
  

•  via soundwire.
  

•  To compile this codec driver say Y or m.
  

...

+static int wcd937x_probe(struct platform_device *pdev) +{

• struct component_match *match = NULL;
• struct device *dev = &pdev->dev;
• struct wcd937x_priv *wcd937x;
• struct wcd_mbhc_config *cfg;
• int ret;
• wcd937x = devm_kzalloc(dev, sizeof(*wcd937x), GFP_KERNEL);
• if (!wcd937x)

• return -ENOMEM;
  

• dev_set_drvdata(dev, wcd937x);
• mutex_init(&wcd937x->micb_lock);
• wcd937x->reset_gpio = of_get_named_gpio(dev->of_node, "reset-gpios", 0);

Why not devm API?

• if (wcd937x->reset_gpio < 0)

• return dev_err_probe(dev, wcd937x->reset_gpio,
  

• 		     "Failed to get reset gpio\n");
  

• wcd937x->us_euro_gpio = devm_gpiod_get_optional(dev, "us-euro", GPIOD_OUT_LOW);
• if (IS_ERR(wcd937x->us_euro_gpio))

• return dev_err_probe(dev, PTR_ERR(wcd937x->us_euro_gpio),
  

• 		"us-euro swap Control GPIO not found\n");
  

• cfg = &wcd937x->mbhc_cfg;
• cfg->swap_gnd_mic = wcd937x_swap_gnd_mic;
• wcd937x->supplies[0].supply = "vdd-rxtx";
• wcd937x->supplies[1].supply = "vdd-px";
• wcd937x->supplies[2].supply = "vdd-mic-bias";
• ret = devm_regulator_bulk_get(dev, WCD937X_MAX_BULK_SUPPLY, wcd937x->supplies);
• if (ret)

• return dev_err_probe(dev, ret, "Failed to get supplies\n");
  

• ret = regulator_bulk_enable(WCD937X_MAX_BULK_SUPPLY, wcd937x->supplies);
• if (ret)

• return dev_err_probe(dev, ret, "Failed to enable supplies\n");
  

• /* Get the buck separately, as it needs special handling */
• wcd937x->buck_supply = devm_regulator_get(dev, "vdd-buck");
• if (IS_ERR(wcd937x->buck_supply))

• return dev_err_probe(dev, PTR_ERR(wcd937x->buck_supply),
  

• 		     "Failed to get buck supply\n");
  

• ret = regulator_enable(wcd937x->buck_supply);
• if (ret)

• return dev_err_probe(dev, ret, "Failed to enable buck supply\n");
  

• wcd937x_dt_parse_micbias_info(dev, wcd937x);
• cfg->mbhc_micbias = MIC_BIAS_2;
• cfg->anc_micbias = MIC_BIAS_2;
• cfg->v_hs_max = WCD_MBHC_HS_V_MAX;
• cfg->num_btn = WCD937X_MBHC_MAX_BUTTONS;
• cfg->micb_mv = wcd937x->micb2_mv;
• cfg->linein_th = 5000;
• cfg->hs_thr = 1700;
• cfg->hph_thr = 50;
• wcd_dt_parse_mbhc_data(dev, &wcd937x->mbhc_cfg);
• ret = wcd937x_add_slave_components(wcd937x, dev, &match);
• if (ret)

• return ret;
  

• wcd937x_reset(wcd937x);
• ret = component_master_add_with_match(dev, &wcd937x_comp_ops, match);
• if (ret)

• return ret;
  

• pm_runtime_set_autosuspend_delay(dev, 1000);
• pm_runtime_use_autosuspend(dev);
• pm_runtime_mark_last_busy(dev);
• pm_runtime_set_active(dev);
• pm_runtime_enable(dev);
• pm_runtime_idle(dev);
• return ret;

+}

+static void wcd937x_remove(struct platform_device *pdev) +{

• component_master_del(&pdev->dev, &wcd937x_comp_ops);
• dev_set_drvdata(&pdev->dev, NULL);

+}

+static struct platform_driver wcd937x_codec_driver = {

• .probe = wcd937x_probe,
• .remove_new = wcd937x_remove,
• .driver = {

• .name = "wcd937x_codec",
  

• .of_match_table = of_match_ptr(wcd937x_of_match),
  

Drop of_match_ptr. You have warnings in your code.

• .suppress_bind_attrs = true,
  

Why?

• },

+};

...

+#if IS_ENABLED(CONFIG_SND_SOC_WCD937X_SDW) +int wcd937x_sdw_free(struct wcd937x_sdw_priv *wcd,

•      struct snd_pcm_substream *substream,
  

•      struct snd_soc_dai *dai);
  

I don't think this driver makes sense on its own, without the SDW part. Why not using dependency in Kconfig?

Best regards, Krzysztof