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

[Mark Brown]

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.

Attachment: signature.asc
Description: PGP signature