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] [thread-next>] [day] [month] [year] [list]

Message-Id: <DB0YYV10UD2Q.M36VAZJOVE7V@linaro.org>
Date: Tue, 01 Jul 2025 20:35:42 +0100
From: "Alexey Klimov" <alexey.klimov@...aro.org>
To: "Mark Brown" <broonie@...nel.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:56 PM BST, Mark Brown wrote:
> 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.

My bad, thanks for pointing it out. I'll change that.

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

I tried to make a minimal working version that we're going to
update with more patches during next submission.

Right now there seems to be at least two approaches:
-- pull in everything and send it in one go. Srini said that it will
be much more difficult to review due to the volume of code;
-- provide few patches that iteratively update the initial one and
add more functionality. The similar way when wcd937x was posted.
I counted there 5 patches for wcd937x. We probably can do that with
this audio-codec. In that case I need to figure out the right way
to split it.

My main issue was MBHC part -- it is needed to avoid IRQ storm
from pdm watchdog interrupts but MBHC requires more things to be
added for its support, that's why there are some empty/placeholder
functions.

What do you think should be the right strategy here?

In theory I can remove MBHC and make much smaller driver but
it will suffer from IRQ storms with some kernel WARNINGs generated.
Or maybe I should remove watchdog interrupts from it as well.

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

This approach seems to be inherited from older wcd-family codec and
wcd939x.c (wcd939x_wd_handle_irq) provides this comment that I can copy
and adjust here like this:

/*
 * HPHR/HPHL Watchdog interrupt threaded handler
 *
 * Watchdog interrupts are expected to be enabled when switching on  the HPHL/R
 * in order to make sure the interrupts are acked by the regmap_irq handler
 * io allow PDM sync. We could leave those interrupts masked but we would
 * not haveany valid way to enable/disable them without violating irq layers.
 *
 * The HPHR/HPHL Watchdog interrupts are handled by regmap_irq, so requesting
 * a threaded handler is the safest way to be able to ack those interrupts
 * without colliding with the regmap_irq setup.
 */


>> +#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.

Could you please elaborate a bit more? Should it be instantiated
as an MFD device or platform device?

As far as I understood we need references to soundwire child nodes
of the codec (which are in DT) hence this one is described 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.

Thanks, I pull in a change/patch that fixes-reworks this.

Thank you,
Alexey Klimov