linux-kernel - Re: [PATCH 2/3] ASoC: cs42l84: Add new codec 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: <abe88a18-28fb-40e5-8bd2-3fde5de9cb56@sirena.org.uk>
Date: Wed, 16 Oct 2024 13:22:56 +0100
From: Mark Brown <broonie@...nel.org>
To: James Calligeros <jcalligeros99@...il.com>
Cc: Martin Povišer <povik+lin@...ebit.org>,
	David Rhodes <david.rhodes@...rus.com>,
	Richard Fitzgerald <rf@...nsource.cirrus.com>,
	Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
	asahi@...ts.linux.dev, linux-sound@...r.kernel.org,
	patches@...nsource.cirrus.com, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, Hector Martin <marcan@...can.st>
Subject: Re: [PATCH 2/3] ASoC: cs42l84: Add new codec driver

On Wed, Oct 16, 2024 at 08:41:01PM +1000, James Calligeros wrote:

> The CS42L84 is a codec from Cirrus Logic found in Apple Silicon Macs.
> The chip continues Apple's long tradition of compelling vendors to
> spin out bespoke SKUs that are based on existing IP but made subtly
> incompatible with the publicly available part. CS42L84 is very similar
> to CS42L42, but has a different regmap.

>  sound/soc/codecs/Makefile  |    2 +
>  sound/soc/codecs/cs42l84.c | 1081 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  sound/soc/codecs/cs42l84.h |  210 ++++++++++++++++++++++++++++++++

Something's wrong with your formatting here...

> @@ -0,0 +1,1081 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * cs42l84.c -- CS42L84 ALSA SoC audio driver
> + *

Please make the entire comment a C++ one so it's more clearly
intentional.

> +static const struct regmap_config cs42l84_regmap = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +
> +	.volatile_reg = cs42l84_volatile_register,
> +
> +	.max_register = 0xffff,

Does that register actually exist?

> +	.cache_type = REGCACHE_RBTREE,

Unless you've got a particular reason to use something else new drivers
should use _MAPLE.

> +static int cs42l84_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct cs42l84_private *cs42l84 = snd_soc_component_get_drvdata(component);
> +	unsigned int regval;
> +	int ret;

> +				ret = regmap_read_poll_timeout(cs42l84->regmap,
> +							       CS42L84_PLL_LOCK_STATUS,
> +							       regval,
> +							       (regval & CS42L84_PLL_LOCK_STATUS_LOCKED),
> +							       CS42L84_PLL_LOCK_POLL_US,
> +							       CS42L84_PLL_LOCK_TIMEOUT_US);
> +				if (ret < 0)
> +					dev_warn(component->dev, "PLL failed to lock: %d\n", ret);

This is too heavyweight for a mute operation, do you really need one?

> +	case 0b00: /* open */
> +		dev_dbg(cs42l84->dev, "Detected open circuit on HS4\n");
> +		fallthrough;
> +	case 0b11: /* shorted */
> +	default:
> +		snd_soc_jack_report(cs42l84->jack, SND_JACK_HEADPHONE,
> +				SND_JACK_HEADSET);
> +		cs42l84->hs_type = SND_JACK_HEADPHONE;
> +		dev_dbg(cs42l84->dev, "Detected bare headphone (no mic)\n");
> +	}

For coding style we should have a break at the end of the case.

> +		default:
> +			if (cs42l84->plug_state != CS42L84_TRANS)
> +				cs42l84->plug_state = CS42L84_TRANS;
> +		}

Here too.

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