linux-kernel - Re: [RFC PATCH 5/5] ASoC: 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]

Date:   Thu, 31 Mar 2022 12:59:47 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Martin Povišer <povik+lin@...ebit.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>
Subject: Re: [RFC PATCH 5/5] ASoC: Add macaudio machine driver

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?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)