Re: [PATCH 2/3] ASoC: cs42l84: Add new codec driver

[Mark Brown <broonie@xxxxxxxxxx>]

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.

Attachment: signature.asc
Description: PGP signature