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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]