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

Mark Brown <[email protected]> Wed, 12 Aug 2026 19:37:14 +0100
Newsgroups	org.kernel.vger.linux-sound
Message-ID	<[email protected]>
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.
signature.asc (application/pgp-signature, 488 B)