linux-kernel - Re: [PATCH 03/15] ASoC: mediatek: mt8186: support adda in platform driver

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: <YgZ8fnvd4/Mxm3wp@sirena.org.uk>
Date:   Fri, 11 Feb 2022 15:10:54 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Jiaxin Yu <jiaxin.yu@...iatek.com>
Cc:     lgirdwood@...il.com, tiwai@...e.com, robh+dt@...nel.org,
        matthias.bgg@...il.com, perex@...ex.cz, p.zabel@...gutronix.de,
        geert+renesas@...der.be, trevor.wu@...iatek.com,
        tzungbi@...gle.com, zhangqilong3@...wei.com,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 03/15] ASoC: mediatek: mt8186: support adda in platform
 driver

On Fri, Feb 11, 2022 at 06:38:06PM +0800, Jiaxin Yu wrote:

This looks pretty good, there's some issues below but they're all very
minor, mostly stylistic things rather than anything substantial.

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  MediaTek ALSA SoC Audio DAI ADDA Control
> + *
> + *  Copyright (c) 2022 MediaTek Inc.
> + *  Author: Jiaxin Yu <jiaxin.yu@...iatek.com>
> + */

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

> +static int mtk_adda_ul_event(struct snd_soc_dapm_widget *w,
> +			     struct snd_kcontrol *kcontrol,
> +			     int event)
> +{
> +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> +	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int mtkaif_dmic = afe_priv->mtkaif_dmic;
> +
> +	dev_info(afe->dev, "%s(), name %s, event 0x%x, mtkaif_dmic %d\n",
> +		 __func__, w->name, event, mtkaif_dmic);

This should be dev_dbg() at most, otherwise the logs will get very noisy
(but note that there are trace points in the core which cover this).
There's a bunch of other dev_info() calls like this on DAPM events.

> +		if (afe_priv->mtkaif_protocol == MTKAIF_PROTOCOL_2_CLK_P2)
> +			regmap_write(afe->regmap, AFE_AUD_PAD_TOP, 0x38);
> +		else if (afe_priv->mtkaif_protocol == MTKAIF_PROTOCOL_2)
> +			regmap_write(afe->regmap, AFE_AUD_PAD_TOP, 0x30);
> +		else
> +			regmap_write(afe->regmap, AFE_AUD_PAD_TOP, 0x30);

This could be more clearly written as a switch statement.

> +			if (strcmp(w->name, "ADDA_MTKAIF_CFG") == 0) {
> +				if (afe_priv->mtkaif_chosen_phase[0] < 0 &&
> +				    afe_priv->mtkaif_chosen_phase[1] < 0) {
> +					dev_info(afe->dev,
> +						 "%s(), calib fail mtkaif_chosen_phase[0/1]:%d/%d\n",
> +						 __func__,

Should this be a dev_err() given that the calibration failed?

> +/* mtkaif dmic */
> +static const char * const mt8186_adda_off_on_str[] = {
> +	"Off", "On"
> +};
> +
> +static const struct soc_enum mt8186_adda_enum[] = {
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8186_adda_off_on_str),
> +			    mt8186_adda_off_on_str),
> +};

This is a simple on/off control so should be a standard numeric control
with a name ending in Switch to help UIs handle it properly.

> +static int mt8186_adda_dmic_set(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *cmpnt = snd_soc_kcontrol_component(kcontrol);
> +	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
> +	int dmic_on;
> +
> +	if (ucontrol->value.enumerated.item[0] >= e->items)
> +		return -EINVAL;
> +
> +	dmic_on = ucontrol->value.integer.value[0];
> +
> +	dev_info(afe->dev, "%s(), kcontrol name %s, dmic_on %d\n",
> +		 __func__, kcontrol->id.name, dmic_on);
> +
> +	afe_priv->mtkaif_dmic = dmic_on;
> +	return 0;

This should return 1 if the value changed so an event is generated for
userspace.  You might want to run the mixer-test kselftest (ideally the
version that's in -next as there were a few bits added very recently),
it should detect issues like this.

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