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

["Martin Povišer" <povik@cutebit.org>]

> On 31. 3. 2022, at 13:59, Mark Brown <broonie@kernel.org> wrote:
> 
> On Thu, Mar 31, 2022 at 02:04:49AM +0200, Martin Povišer wrote:
> 
>> --- /dev/null
>> +++ b/sound/soc/apple/macaudio.c
>> @@ -0,0 +1,597 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * ASoC machine driver for Apple Silicon Macs
>> + *
> 
> Please make the entire comment a C++ one so things look more
> intentional.
> 
>> +		/* CPU side is bit and frame clock master, I2S with both clocks inverted */
> 
> Please refer to clock providers here.
> 
>> +		ret = of_property_read_string(np, "link-name", &link->name);
>> +		if (ret) {
>> +			dev_err(card->dev, "Missing link name\n");
>> +			goto err_put_np;
>> +		}
> 
> This doesn't look like it's mandatory in the binding.

Good catch!

>> +static int macaudio_init(struct snd_soc_pcm_runtime *rtd)
>> +{
>> +	struct snd_soc_card *card = rtd->card;
>> +	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
>> +	struct snd_soc_component *component;
>> +	int ret, i;
>> +
>> +	if (rtd->num_codecs > 1) {
>> +		ret = macaudio_assign_tdm(rtd);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	for_each_rtd_components(rtd, i, component)
>> +		snd_soc_component_set_jack(component, &ma->jack, NULL);
> 
> What is the jack configuration this is attempting to describe?  It looks
> like you have some dedicated speaker driver devices which are going to
> get attached to jacks here for example.

We know the speakers will ignore the set_jack call. There’s one jack and
this way we know the jack codec will attach to it, for speakers it’s a no-op.
(If you prefer I will special-case it to the jack codec.)

>> +} macaudio_kctlfixes[] = {
>> +	{"* ASI1 Sel", "Left"},
>> +	{"* ISENSE Switch", "Off"},
>> +	{"* VSENSE Switch", "Off"},
>> +	{ }
>> +};
>> +
>> +static bool macaudio_kctlfix_matches(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);
>> +}
> 
> This looks worryingly like use case configuration.

I go over this in the cover letter! This is fixing the TDM slot selection
and disabling voltage/current sensing on the speaker amp codecs, which have
no business being exposed to userspace as options. This is not use case,
this not letting people blow their speakers from userspace.

> 
>> +/*
>> + * Maybe this could be a general ASoC function?
>> + */
>> +static void snd_soc_kcontrol_set_strval(struct snd_soc_card *card,
>> +				struct snd_kcontrol *kcontrol, const char *strvalue)
> 
> No, we should not be setting user visible control values from the
> kernel.  This shouldn't be a machine driver function either.  What are
> you trying to accomplish here?

See above.

Martin