Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 3 Jan 2023 20:17:58 +0000
From:   Mark Brown <broonie@kernel.org>
To:     wangweidong.a@awinic.com
Cc:     lgirdwood@gmail.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, perex@perex.cz, tiwai@suse.com,
        ckeepax@opensource.cirrus.com, rf@opensource.cirrus.com,
        povik+lin@cutebit.org, pierre-louis.bossart@linux.intel.com,
        james.schulman@cirrus.com, flatmax@flatmax.com,
        cezary.rojewski@intel.com, srinivas.kandagatla@linaro.org,
        tanureal@opensource.cirrus.com, steve@sk2.org, stephan@gerhold.net,
        zhuning0077@gmail.com, shumingf@realtek.com,
        alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, zhaolei@awinic.com,
        liweilei@awinic.com, yijiangtao@awinic.com, duanyibo@awinic.com
Subject: Re: [PATCH V8 1/5] ASoC: codecs: Add i2c and codec registration for
 aw883xx and their associated operation functions
Message-ID: <Y7SNdgU6b5r+YsWK@sirena.org.uk>
References: <20221230093454.190579-1-wangweidong.a@awinic.com>
 <20221230093454.190579-2-wangweidong.a@awinic.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="929h2gIydsuzAm1i"
Content-Disposition: inline
In-Reply-To: <20221230093454.190579-2-wangweidong.a@awinic.com>
Precedence: bulk


--929h2gIydsuzAm1i
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Dec 30, 2022 at 05:34:50PM +0800, wangweidong.a@awinic.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.

--929h2gIydsuzAm1i
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmO0jXUACgkQJNaLcl1U
h9CN2gf/TOQnuPxL2AA3udiB5SD+QAMkx6soCgpSdYQhUmLA1PK1tZOrfxK5q5ci
g/6SdR+XIF/lbO2Yy2q9HP8lYelnmJg0KWZvoY8lCmxWhGsirEenA3fz0ce8mHXl
L/RaWnOIEH1nKo7SW1BSHeJ3hblXaqJ+egDuaJ5Rl+ouUIKVoK9PfuLXTeBoJtRZ
OWpd9fT650Lk0hES7oOFnaFNLOYI9mmE1ir3t0WUioBlqUWhcbo+D2wvAJvG0XzY
Cu5DJ8FfLtws39/PlguaFy0lLCkLGogPG7FuUmlwfLnp5CsY+q09R4WUO9apuYD7
+C/m98h3/y8TIW2EnpDGWxCMKdaWtQ==
=ZN+S
-----END PGP SIGNATURE-----

--929h2gIydsuzAm1i--