[PATCH 1/3] ASoC: codecs: add SN624x SDCA SoundWire driver

Hi Mark Brown:
	Thanks for your comments. I will address them in v2.
Best regards
Qianghua Wang


>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
Re: [PATCH 1/3] ASoC: codecs: add SN624x SDCA SoundWire driver

On Wed, Aug 12, 2026 at 02:59:42PM +0800, qianghua wang wrote:

> Add SoundWire SDCA driver for Senary SN624x multi-function codecs 
> (jack, speaker amp, and DMIC). Extend the Senary MAINTAINERS entry for 
> the new codec files only; Intel ACPI match tables remain under INTEL 
> ASoC DRIVERS.

This changelog describes the series, not this patch which only adds the CODEC driver.

> @@ -238,6 +238,7 @@ config SND_SOC_ALL_CODECS
>  	imply SND_SOC_RT715_SDCA_SDW
>  	imply SND_SOC_RT721_SDCA_SDW
>  	imply SND_SOC_RT722_SDCA_SDW
> +	imply SND_SOC_SN624X_SDCA_SDW
>  	imply SND_SOC_RT1308_SDW
>  	imply SND_SOC_RT1316_SDW
>  	imply SND_SOC_RT1318

Please keep this, the Makefile and the Kconfig lexically sorted.

> +config SND_SOC_SN624X_SDCA_SDW
> +       tristate "Senary SN624x SDCA Codec - SDW"
> +       depends on SOUNDWIRE
> +       select REGMAP_SOUNDWIRE
> +       select REGMAP_SOUNDWIRE_MBQ
> +       help

This also depends on SDCA.

> +/*
> + * Trace helper: load with trace=1 or:
> + *   echo 1 > /sys/module/snd_soc_sn624x_sdca/parameters/trace
> + * For finer logs without trace=1, enable dynamic_debug on this file.
> + */
> +static bool sn624x_trace;
> +module_param_named(trace, sn624x_trace, bool, 0644); 
> +MODULE_PARM_DESC(trace,
> +                "extra dev_dbg for probe, jack, SDW status (default 
> +off)");

You probably want to look at tracepoints and trace_printk() here, these are much more flexible features and can be easily tweaked at runtime.
In general it's probably better to skip all these module parameters for upstream, they all look like debugging stuff which should hopefully not be needed.

> +static void sn624x_log_power_mode_once(struct device *dev,
> +				       struct sn624x_sdca_priv *sn624x) {
> +	unsigned int pm;
> +	int ret;
> +
> +	if (!sn624x || !sn624x->regmap)
> +		return;
> +	if (atomic_xchg(&sn624x_power_mode_logged, 1) != 0)
> +		return;

There doesn't seem to be any way to set this?  Also consider printk_once().

> +static int sn624x_sdca_pcm_hw_params(struct snd_pcm_substream *substream,
> +				     struct snd_pcm_hw_params *params,
> +				     struct snd_soc_dai *dai)
> +{

> +	stream_config.frame_rate = params_rate(params);
> +	stream_config.ch_count = ch;

Do we need to configure SDCA clocking as well?

> +static int sn624x_sdca_dev_system_suspend(struct device *dev) {
> +	struct sn624x_sdca_priv *sn624x = dev_get_drvdata(dev);
> +
> +	if (!sn624x->first_hw_init)
> +		return 0;
> +
> +	cancel_delayed_work_sync(&sn624x->jack_detect_work);
> +
> +	mutex_lock(&sn624x->disable_irq_lock);
> +	sn624x->disable_irq = true;
> +	sn624x_sdca_jack_irq_mask(sn624x);
> +	mutex_unlock(&sn624x->disable_irq_lock);
> +
> +	return sn624x_sdca_dev_suspend(dev); }

I'd expect the work to be cancelled after disabling the interrupt, otherwise the interrupt could schedule more work between the cancellation and the disable.