linux-kernel - Re: [PATCH V8 1/5] ASoC: codecs: Add i2c and codec registration for aw883xx and their associated operation functions

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: <Y7SNdgU6b5r+YsWK@sirena.org.uk>
Date:   Tue, 3 Jan 2023 20:17:58 +0000
From:   Mark Brown <broonie@...nel.org>
To:     wangweidong.a@...nic.com
Cc:     lgirdwood@...il.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, perex@...ex.cz, tiwai@...e.com,
        ckeepax@...nsource.cirrus.com, rf@...nsource.cirrus.com,
        povik+lin@...ebit.org, pierre-louis.bossart@...ux.intel.com,
        james.schulman@...rus.com, flatmax@...tmax.com,
        cezary.rojewski@...el.com, srinivas.kandagatla@...aro.org,
        tanureal@...nsource.cirrus.com, steve@....org, stephan@...hold.net,
        zhuning0077@...il.com, shumingf@...ltek.com,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, zhaolei@...nic.com,
        liweilei@...nic.com, yijiangtao@...nic.com, duanyibo@...nic.com
Subject: Re: [PATCH V8 1/5] ASoC: codecs: Add i2c and codec registration for
 aw883xx and their associated operation functions

On Fri, Dec 30, 2022 at 05:34:50PM +0800, wangweidong.a@...nic.com wrote:

This looks mostly good, there's a few small nits below but nothing too
huge:

> +++ b/sound/soc/codecs/aw883xx/aw883xx.c
> @@ -0,0 +1,667 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * aw883xx.c --  ALSA SoC AW883XX codec support

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

> +static int aw883xx_startup(struct snd_pcm_substream *substream,
> +			struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *codec = dai->component;
> +	struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
> +
> +	mutex_lock(&aw883xx->lock);
> +	aw883xx_start(aw883xx, AW_ASYNC_START);
> +	mutex_unlock(&aw883xx->lock);
> +
> +	return 0;
> +}

I would not expect power management to be done in the startup and
shutdown callbacks, this should be done in set_bias_level() or DAPM like
with other drivers.

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

> +	time = ucontrol->value.integer.value[0];
> +
> +	if (time < mc->min || time > mc->max)
> +		return 0;

That's in spec, but it's a bit nicer to return an error so that people
are more likely to notice that their attempt to set a value didn't do
what they asked.

> +static int aw883xx_switch_get(struct snd_kcontrol *kcontrol,
> +			struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
> +	struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
> +
> +	ucontrol->value.integer.value[0] = aw883xx->allow_power;
> +
> +	return 0;
> +}

Let the system deal with adding a control that lets the output path be
powered off if that's needed, the driver shouldn't be open coding that -
it's just extra and redundant complexity.  If users do need this then
DAPM is probably a better fit than set_bias_level().

> +	dev_info(aw883xx->aw_pa->dev, "%s : bin load success\n", __func__);

Lower this to a dev_dbg()

> +static void aw883xx_i2c_remove(struct i2c_client *i2c)
> +{
> +	struct aw883xx *aw883xx = i2c_get_clientdata(i2c);
> +
> +	aw883xx_deinit(aw883xx->aw_pa);
> +	snd_soc_unregister_component(&i2c->dev);

I'd expect these to be in the other order, otherwise something could be
using the device while it's partially initialised which probably won't
go well.  Unregistering the component will detach userspace and the card
avoiding that problem.

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