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: <Z_LHv4S-FcgvGldA@finisterre.sirena.org.uk>
Date: Sun, 6 Apr 2025 19:28:15 +0100
From: Mark Brown <broonie@...nel.org>
To: Luca Weiss <luca@...aweiss.eu>
Cc: ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org,
	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>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	Weidong Wang <wangweidong.a@...nic.com>,
	linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 2/3] ASoC: codecs: Add aw8898 amplifier driver

On Sun, Apr 06, 2025 at 03:03:16PM +0200, Luca Weiss wrote:

Overall this driver feels like it's not terribly well integrated into
the subsystem - it's not using the standard framework features for
things.  The code itself looks broadly fine but things need moving about
a bit to feel more like a standard driver.

> @@ -0,0 +1,583 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2018 AWINIC Technology CO., LTD
> + * Author: Nick Li <liweilei@...nic.com.cn>
> + *
> + * Copyright (c) 2025 Luca Weiss <luca@...aweiss.eu>
> + */

Please make the entire comment a C++ one so things look more
intentional.

> +static void aw8898_set_mute(struct aw8898 *aw8898, bool mute)
> +{
> +	unsigned int val = AW8898_PWMCTRL_HMUTE_DISABLE;
> +
> +	if (mute)
> +		val = AW8898_PWMCTRL_HMUTE_ENABLE;
> +
> +	regmap_update_bits(aw8898->regmap, AW8898_PWMCTRL,
> +			   AW8898_PWMCTRL_HMUTE_MASK,
> +			   FIELD_PREP(AW8898_PWMCTRL_HMUTE_MASK, val));
> +}

This should either be the standard DAI mute operation, user controlled
or there should be a clear explanation of what's going on.  Or just
inlined into callers given how trivial it is.

> +static void aw8898_set_power(struct aw8898 *aw8898, bool on)
> +{
> +	unsigned int val = AW8898_SYSCTRL_PW_PDN;
> +
> +	if (on)
> +		val = AW8898_SYSCTRL_PW_ACTIVE;
> +
> +	regmap_update_bits(aw8898->regmap, AW8898_SYSCTRL,
> +			   AW8898_SYSCTRL_PW_MASK,
> +			   FIELD_PREP(AW8898_SYSCTRL_PW_MASK, val));
> +}

This should be in the standard framework power management flows
(probably either a DAPM widget or set_bias_level(), it looks closer to
the latter).

> +static int aw8898_dev_mode_put(struct snd_kcontrol *kcontrol,
> +			       struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> +	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(component);
> +
> +	if (aw8898->dev_mode == ucontrol->value.enumerated.item[0])
> +		return 0;
> +
> +	aw8898->dev_mode = ucontrol->value.enumerated.item[0];
> +
> +	aw8898_update_dev_mode(aw8898);
> +
> +	return 1;
> +}

There is no validation of the written value here.  Please use the
mixer-test selftest to vaidate things.

> +/*
> + * -127.5 dB min, 0.5 dB steps, no mute
> + * Note: The official datasheet claims to be able to attenuate between 0 dB and
> + * -96 dB with 0.5 dB/step, but the register values are 0-255 so this doesn't
> + * really line up. It's a best guess.
> + */

It is common for registers to have out of spec values which should never
be written, they might behave in undesirable ways.  The controls should
only expose whatever the documented values are.

> +static int aw8898_startup(struct snd_pcm_substream *substream,
> +			  struct snd_soc_dai *dai)
> +{
> +	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(dai->component);
> +
> +	aw8898_set_power(aw8898, true);
> +
> +	return 0;
> +}

As noted above the power should be managed from power management
operations, either set_bias_level() or DAPM.

> +static int aw8898_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
> +	struct snd_soc_component *component = dai->component;
> +
> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S:
> +		if ((fmt & SND_SOC_DAIFMT_MASTER_MASK)

_CLOCK_PROVIDER_MASK.

> +static int aw8898_mute(struct snd_soc_dai *dai, int mute, int stream)
> +{
> +	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(dai->component);
> +
> +	mutex_lock(&aw8898->cfg_lock);
> +
> +	if (mute) {
> +		aw8898_stop(aw8898);
> +	} else {
> +		if (!aw8898->cfg_loaded) {
> +			aw8898_cold_start(aw8898);
> +		} else {
> +			aw8898_update_dev_mode(aw8898);
> +			aw8898_start(aw8898);
> +		}
> +	}
> +
> +	mutex_unlock(&aw8898->cfg_lock);

A mute operation should not be doing anything as expensive as this, it
should just be something more like a single register write (eg, just the
mute).  This looks more like power management operations.  

> +static int aw8898_component_probe(struct snd_soc_component *component)
> +{
> +	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	aw8898->component = component;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(aw8898->supplies),
> +				    aw8898->supplies);
> +	if (ret) {
> +		dev_err(component->dev, "Failed to enable supplies: %d\n",
> +			ret);
> +		return ret;
> +	}

It seems random to do this separately to the rest of the power
management for the device?

> +	snd_soc_add_component_controls(component, aw8898_controls,
> +				       ARRAY_SIZE(aw8898_controls));

Just specify the controls in the driver struct when registering.

> +static const struct regmap_config aw8898_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +
> +	.max_register = AW8898_MAX_REGISTER,
> +	.cache_type = REGCACHE_RBTREE,
> +};

Use _MAPLE unless you have a particular reason not to, it's a more
modern data structure.

> +static int aw8898_probe(struct i2c_client *client)
> +{
> +	struct aw8898 *aw8898;
> +	int ret;

> +	aw8898->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(aw8898->reset))
> +		return dev_err_probe(&client->dev, PTR_ERR(aw8898->reset),
> +				     "Failed to get reset GPIO\n");
> +
> +	aw8898_reset(aw8898);
> +
> +	ret = aw8898_check_chipid(aw8898);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Chip ID check failed\n");

We didn't power on the regulators before trying to validate the chip ID
which probably isn't going to go terribly well if the regulators are
controllable...

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ