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-next>] [day] [month] [year] [list]

Message-Id: <20250522054619.9574-1-zhangyi@everest-semi.com>
Date: Thu, 22 May 2025 13:46:19 +0800
From: Zhang Yi <zhangyi@...rest-semi.com>
To: broonie@...nel.org
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

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

I'll read the ES8375_ADC1 when I need the dmic_enable.
So I can replace SOC_DAPM_ENUM_EXT with SOC_DAPM_ENUM
which is used for ADC MUX.

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

Thanks for reminding. I'll modify it.

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

I think I got your point.

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

I'll delete it.

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

Because the default value of the chip's volume register is 0x00,
initializing the device without setting it to 0xbf will
cause the device to mute until the customer sets the volume.

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

ok