| 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
| ||
|
Message-ID: <95aae292-976b-4725-b506-769418d7d534@linaro.org>
Date: Tue, 5 Dec 2023 14:37:00 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Prasad Kumpatla <quic_pkumpatl@...cinc.com>,
srinivas.kandagatla@...aro.org, broonie@...nel.org,
agross@...nel.org, andersson@...nel.org, quic_plai@...cinc.com,
quic_rohkumar@...cinc.com, alsa-devel@...a-project.org,
linux-kernel@...r.kernel.org, quic_phaniu@...cinc.com,
quic_avangala@...cinc.com
Cc: Mohammad Rafi Shaik <quic_mohs@...cinc.com>
Subject: 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@...cinc.com>
> Signed-off-by: Prasad Kumpatla <quic_pkumpatl@...cinc.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
Powered by blists - more mailing lists