linux-kernel - Re: [PATCH v6 3/7] ASoC: codecs: wcd937x: add wcd937x codec driver

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <834d31cc-f4bc-4db7-a25b-f9869e550eb6@wanadoo.fr>
Date: Sun, 16 Jun 2024 17:06:18 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Mohammad Rafi Shaik <quic_mohs@...cinc.com>,
 Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
 Banajit Goswami <bgoswami@...cinc.com>, Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
 Takashi Iwai <tiwai@...e.com>
Cc: alsa-devel@...a-project.org, linux-arm-msm@...r.kernel.org,
 linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, quic_rohkumar@...cinc.com,
 quic_pkumpatl@...cinc.com, Konrad Dybcio <konrad.dybcio@...aro.org>
Subject: Re: [PATCH v6 3/7] ASoC: codecs: wcd937x: add wcd937x codec driver -
 another comment

Le 11/06/2024 à 09:45, Mohammad Rafi Shaik a écrit :
> From: Prasad Kumpatla <quic_pkumpatl@...cinc.com>
> 
> This patch adds basic SoundWire codec driver to support for
> WCD9370/WCD9375 TX and RX devices.
> 
> The WCD9370/WCD9375 has Multi Button Headset Control hardware to
> support Headset insertion, type detection, 8 headset buttons detection,
> Over Current detection and Impedence measurements.
> This patch adds support for this using wcd-mbhc apis.
> 
> Co-developed-by: Konrad Dybcio <konrad.dybcio@...aro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
> Signed-off-by: Prasad Kumpatla <quic_pkumpatl@...cinc.com>
> Co-developed-by: Mohammad Rafi Shaik <quic_mohs@...cinc.com>
> Signed-off-by: Mohammad Rafi Shaik <quic_mohs@...cinc.com>
> ---


Hi,

this patch has reached -next, but I have a question.

If I'm correct, I can send a patch, but if the fix can be folded 
somewhere, this is also fine for me.

...

> +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 = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(wcd937x->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(wcd937x->reset_gpio),
> +				     "failed to reset wcd 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))

regulator_bulk_disable() is missing here...

> +		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");

... and here...

Also, should 'buck_supply' be disabled in the error handling path of the 
probe and in the remove function? (and maybe even somewhere else related 
to wcd937x_codec_enable_vdd_buck())

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

... and here...

> +
> +	wcd937x_reset(wcd937x);
> +
> +	ret = component_master_add_with_match(dev, &wcd937x_comp_ops, match);
> +	if (ret)
> +		return ret;

... and here.

Maybe a devm_add_action_ior_reset() could help?

> +
> +	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)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct wcd937x_priv *wcd937x = dev_get_drvdata(dev);
> +
> +	component_master_del(&pdev->dev, &wcd937x_comp_ops);
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_dont_use_autosuspend(dev);
> +
> +	regulator_bulk_disable(WCD937X_MAX_BULK_SUPPLY, wcd937x->supplies);
> +	regulator_bulk_free(WCD937X_MAX_BULK_SUPPLY, wcd937x->supplies);

This has been allocated with devm_regulator_bulk_get(), so this call 
looks redundant (but harmless).

> +}

...