Re: [RFC PATCH v2 5/5] ASoC: apple: Add macaudio machine driver

On Mon, Jun 06, 2022 at 09:19:10PM +0200, Martin Povišer wrote:

• /*
  

•  * Primary FE
  

•  *
  

•  * The mclk/fs ratio at 64 for the primary frontend is important
  

•  * to ensure that the headphones codec's idea of left and right
  

•  * in a stereo stream over I2S fits in nicely with everyone else's.
  

•  * (This is until the headphones codec's driver supports
  

•  * set_tdm_slot.)
  

•  *
  

•  * The low mclk/fs ratio precludes transmitting more than two
  

•  * channels over I2S, but that's okay since there is the secondary
  

•  * FE for speaker arrays anyway.
  

•  */
  

• .mclk_fs = 64,
  

• },

This seems weird - it looks like it's confusing MCLK and the bit clock for the audio bus. These are two different clocks. Note that it's very common for devices to require a higher MCLK/fs ratio to deliver the best audio performance, 256fs is standard.

• {

• /*
  

•  * Secondary FE
  

•  *
  

•  * Here we want frames plenty long to be able to drive all
  

•  * those fancy speaker arrays.
  

•  */
  

• .mclk_fs = 256,
  

• }

Same thing here - this is at least confusing MCLK and the bit clock.

+static bool macaudio_match_kctl_name(const char *pattern, const char *name) +{

• if (pattern[0] == '*') {

• int namelen, patternlen;
  

• pattern++;
  

• if (pattern[0] == ' ')
  

• 	pattern++;
  

• namelen = strlen(name);
  

• patternlen = strlen(pattern);
  

• if (namelen > patternlen)
  

• 	name += (namelen - patternlen);
  

• }
• return !strcmp(name, pattern);

+}

+static int macaudio_limit_volume(struct snd_soc_card *card,

• 		 const char *pattern, int max)
  

+{

• struct snd_kcontrol *kctl;
• struct soc_mixer_control *mc;
• int found = 0;
• list_for_each_entry(kctl, &card->snd_card->controls, list) {

• if (!macaudio_match_kctl_name(pattern, kctl->id.name))
  

• 	continue;
  

• found++;
  

• dev_dbg(card->dev, "limiting volume on '%s'\n", kctl->id.name);
  

• /*
  

•  * TODO: This doesn't decrease the volume if it's already
  

•  * above the limit!
  

•  */
  

• mc = (struct soc_mixer_control *)kctl->private_value;
  

• if (max <= mc->max)
  

• 	mc->platform_max = max;
  

• }
• return found;

+}

This shouldn't be open coded in a driver, please factor it out into the core so we've got an API for "set limit X on control Y" then call that.