Re: [PATCH 3/3] ASoC: codecs: add new pm4125 audio codec driver

["Alexey Klimov" <alexey.klimov@linaro.org>]

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