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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

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.

> +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.

> +} 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.

> +/*
> + * 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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]