Re: [PATCH v5 5/5] ASoC: cs40l50: Support I2S streaming to CS40L50

On Thu, Jan 04, 2024 at 10:36:38PM +0000, James Ogletree wrote:

Introduce support for Cirrus Logic Device CS40L50: a haptic driver with waveform memory, integrated DSP, and closed-loop algorithms.

The ASoC driver enables I2S streaming to the device.

Signed-off-by: James Ogletree jogletre@opensource.cirrus.com

+#include <linux/mfd/cs40l50.h> +#include <linux/pm_runtime.h> +#include <sound/pcm_params.h> +#include <sound/soc.h>

Need to also include bitfield.h for FIELD_PREP etc.

+static int cs40l50_clk_en(struct snd_soc_dapm_widget *w,

• 	  struct snd_kcontrol *kcontrol,
  

• 	  int event)
  

+{

• struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
• struct cs40l50_codec *codec = snd_soc_component_get_drvdata(comp);
• int ret;
• switch (event) {
• case SND_SOC_DAPM_POST_PMU:

• ret = regmap_write(codec->regmap, CS40L50_DSP_QUEUE, CS40L50_STOP_PLAYBACK);
  

• if (ret)
  

• 	return ret;
  

• ret = regmap_write(codec->regmap, CS40L50_DSP_QUEUE, CS40L50_START_I2S);
  

• if (ret)
  

• 	return ret;
  

Feels weird that we don't wait for these two commands to be acknowledged by the DSP before doing the clock swap. Is that intentional? Is the DSP just guaranteed to be so fast it doesn't matter, in which case a comment would be nice.

+static int cs40l50_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt) +{

• struct cs40l50_codec *codec = snd_soc_component_get_drvdata(codec_dai->component);
• if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS)

• return -EINVAL;
  

• switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
• case SND_SOC_DAIFMT_NB_NF:

• codec->daifmt = 0;
  

• break;
  

• case SND_SOC_DAIFMT_NB_IF:

• codec->daifmt = CS40L50_ASP_FSYNC_INV_MASK;
  

• break;
  

• case SND_SOC_DAIFMT_IB_NF:

• codec->daifmt = CS40L50_ASP_BCLK_INV_MASK;
  

• break;
  

• case SND_SOC_DAIFMT_IB_IF:

• codec->daifmt = CS40L50_ASP_FSYNC_INV_MASK | CS40L50_ASP_BCLK_INV_MASK;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) == SND_SOC_DAIFMT_I2S)

• codec->daifmt |= FIELD_PREP(CS40L50_ASP_FMT_MASK, CS40L50_ASP_FMT_I2S);
  

It feels unlikely the chip supports all formats with no additional settings? Probably should have a switch for the supported formats and return an error.

+static struct snd_soc_dai_driver cs40l50_dai[] = {

• {

• .name = "cs40l50-pcm",
  

• .id = 0,
  

• .playback = {
  

• 	.stream_name = "ASP Playback",
  

• 	.channels_min = 1,
  

• 	.channels_max = 2,
  

• 	.rates = SNDRV_PCM_RATE_48000,
  

• 	.formats = CS40L50_FORMATS,
  

• },
  

• .ops = &cs40l50_dai_ops,
  

• .symmetric_rate = 1,
  

The symmetric_rate feels a bit redundant since we only have playback supported.

Thanks, Charles