linux-kernel - Re: [PATCH 2/2] ASoC: codecs: add support for ES8375

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: <42d25977-8975-4c6e-b8a4-a733e62875b2@sirena.org.uk>
Date: Wed, 21 May 2025 12:42:02 +0100
From: Mark Brown <broonie@...nel.org>
To: Zhang Yi <zhangyi@...rest-semi.com>
Cc: robh@...nel.org, tiwai@...e.com, devicetree@...r.kernel.org,
	conor+dt@...nel.org, lgirdwood@...il.com,
	linux-kernel@...r.kernel.org, linux-sound@...r.kernel.org,
	perex@...ex.cz, krzk+dt@...nel.org,
	amadeuszx.slawinski@...ux.intel.com, krzk@...nel.org
Subject: Re: [PATCH 2/2] ASoC: codecs: add support for ES8375

On Wed, May 21, 2025 at 06:42:47PM +0800, Zhang Yi wrote:

> The driver is for codec es8375 of everest

This looks mostly fine, a few small nits below but nothing major.

> +static int es8375_dmic_set(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{

> +	if (val) {
> +		regmap_update_bits_check(es8375->regmap, ES8375_ADC1, 0x80, 0x80, &changed1);
> +		es8375->dmic_enable = 0x01;
> +	} else {
> +		regmap_update_bits_check(es8375->regmap, ES8375_ADC1, 0x80, 0x00, &changed1);
> +		es8375->dmic_enable = 0x00;
> +	}

Instead of overriding this you could just read the register value when
you need it, you've got a register cache so it should be fast enough and
it's a lot less code.

> +static const struct snd_kcontrol_new es8375_snd_controls[] = {
> +	/* Capture Path */
> +	SOC_SINGLE_TLV("ADC OSR GAIN", ES8375_ADC_OSR_GAIN,
> +			ADC_OSR_GAIN_SHIFT_0, ES8375_ADC_OSR_GAIN_MAX, 0,
> +			es8375_adc_osr_gain_tlv),

Gain/volume controls should end in Volume as covered in control-names.rst.

> +	SOC_SINGLE("ADC Invert", ES8375_ADC1, ADC_INV_SHIFT_6, 1, 0),

On/off switches should end in Switch.

> +	ret = regulator_get_voltage(es8375->core_supply[0].consumer);

Might be nicer to have something better than a magic number to ensure
that the supplies are in order, or use a specific variable.

> +	case SND_SOC_DAIFMT_CBC_CFP:    /* MASTER MODE */
> +		es8375->mastermode = 1;
> +		regmap_update_bits(es8375->regmap, ES8375_RESET1,
> +				0x80, 0x80);
> +		break;
> +	case SND_SOC_DAIFMT_CBC_CFC:    /* SLAVE MODE */

Please avoid using outdated terminologoy for clock provider and
consumer.

> +static void es8375_init(struct snd_soc_component *component)
> +{

> +	regmap_write(es8375->regmap, ES8375_ADC_VOLUME, 0xBF);
> +	regmap_write(es8375->regmap, ES8375_DAC_VOLUME, 0xBF);

Some of these settings look like they are (or should be) user
controllable and should be left at the chip defaults, the volumes above
really stand out.  We use chip defaults to avoid having to decide which
use cases to configure for.

> +static struct regmap_config es8375_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = ES8375_REG_MAX,
> +	.cache_type = REGCACHE_RBTREE,

Unless you've got a really strong reason for using _RBTREE new drivers
should use _MAPLE, it's a more modern underlying data structure and
makes choices more suitable for current systems.

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