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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <8961DDD2-93FF-4A18-BCA2-90FCE298F517@cutebit.org>
Date:   Thu, 9 Jun 2022 16:09:57 +0200
From:   Martin Povišer <povik+lin@...ebit.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Mark Kettenis <kettenis@...nbsd.org>,
        Hector Martin <marcan@...can.st>,
        Sven Peter <sven@...npeter.dev>, asahi@...ts.linux.dev
Subject: Re: [RFC PATCH v2 5/5] ASoC: apple: Add macaudio machine driver


> On 9. 6. 2022, at 15:33, Mark Brown <broonie@...nel.org> wrote:
> 
> 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.

On these machines we are not producing any other clock for the codecs
besides the bit clock, so I am using MCLK interchangeably for it. (It is
what the sample rate is derived from after all.)

One of the codec drivers this is to be used with (cs42l42) expects to be
given the I2S bit clock with

  snd_soc_dai_set_sysclk(dai, 0, mclk, SND_SOC_CLOCK_IN);

I can rename mclk to bclk in all of the code to make it clearer maybe.
Also the platform driver can take the bit clock value from set_bclk_ratio,
instead of set_sysclk from where it takes it now. The cs42l42 driver I can
patch too to accept set_bclk_ratio.

>> +	{
>> +		/*
>> +		 * 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.

There’s already snd_soc_limit_volume, but it takes a fixed control name.
Can I extend it to understand patterns beginning with a wildcard, like
'* Amp Gain Volume’?