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 <liwei...@awinic.com.cn>
> + *
> + * Copyright (c) 2025 Luca Weiss <l...@lucaweiss.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...
signature.asc
Description: PGP signature
