linux-kernel - Re: [PATCH 3/3] ASoC: codecs: add new pm4125 audio 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] [day] [month] [year] [list]

Message-ID: <aF01gRFjsKgy6j4V@finisterre.sirena.org.uk>
Date: Thu, 26 Jun 2025 12:56:49 +0100
From: Mark Brown <broonie@...nel.org>
To: Alexey Klimov <alexey.klimov@...aro.org>
Cc: Srinivas Kandagatla <srini@...nel.org>,
	Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Stephen Boyd <sboyd@...nel.org>,
	Lee Jones <lee@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>, linux-arm-msm@...r.kernel.org,
	linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
	Srinivas Kandagatla <srinivas.kandagatla@....qualcomm.com>
Subject: Re: [PATCH 3/3] ASoC: codecs: add new pm4125 audio codec driver

On Thu, Jun 26, 2025 at 12:50:31AM +0100, Alexey Klimov wrote:

> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -297,6 +297,7 @@ config SND_SOC_ALL_CODECS
>  	imply SND_SOC_WCD937X_SDW
>  	imply SND_SOC_WCD938X_SDW
>  	imply SND_SOC_WCD939X_SDW
> +	imply SND_SOC_PM4125_SDW
>  	imply SND_SOC_LPASS_MACRO_COMMON
>  	imply SND_SOC_LPASS_RX_MACRO
>  	imply SND_SOC_LPASS_TX_MACRO

Please keep this file sorted, there's obviously been some things missed
but please don't make it worse.

> +obj-$(CONFIG_SND_SOC_PM4125_SDW) += snd-soc-pm4125-sdw.o
> +obj-$(CONFIG_SND_SOC_PM4125)   += snd-soc-pm4125.o
> +ifdef CONFIG_SND_SOC_PM4125_SDW
> +# avoid link failure by forcing sdw code built-in when needed
> +obj-$(CONFIG_SND_SOC_PM4125) += snd-soc-pm4125-sdw.o
> +endif

Other drivers sort this out in Kconfig, do as they do.

> +static int pm4125_micbias_control(struct snd_soc_component *component,
> +				  int micb_num, int req, bool is_dapm)
> +{
> +	return 0;
> +}

Why have this empty function which is only called from within the
driver?  At best it's making the callers look like they do something.

> +static irqreturn_t pm4125_wd_handle_irq(int irq, void *data)
> +{
> +	return IRQ_HANDLED;
> +}

Why bother regisering for the interrupt at all if you're just going to
ignore it?

> +#if defined(CONFIG_OF)
> +static const struct of_device_id pm4125_of_match[] = {
> +	{ .compatible = "qcom,pm4125-codec" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, pm4125_of_match);
> +#endif

Why does this compatible exist?  If the driver is instantiated from a
as a Linux software contruct it shouldn't appear in the DT.

> +const u8 pm4125_reg_access_digital[
> +	PM4125_REG(PM4125_DIGITAL_REGISTERS_MAX_SIZE)] = {
> +		[PM4125_REG(PM4125_DIG_SWR_CHIP_ID0)] = RD_REG,
> +		[PM4125_REG(PM4125_DIG_SWR_CHIP_ID1)] = RD_REG,
> +		[PM4125_REG(PM4125_DIG_SWR_CHIP_ID2)] = RD_REG,

Data tables like this shouldn't be in headers, they should be in C
files.  At worst you might end up with duplicate copies in the object
code.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)