Re: [PATCH 2/2] ASoC: Add driver for CX2092X I2S Audio DSP

From: Mark Brown <broonie@kernel.org>
To: simon.ho.cnxt@gmail.com
Cc: Simon Ho <simon.ho@conexant.com>,
	alsa-devel@alsa-project.org, tiwai@suse.com, lgirdwood@gmail.com
Subject: Re: [PATCH 2/2] ASoC: Add driver for CX2092X I2S Audio DSP
Date: Sun, 19 Feb 2017 16:27:17 +0000	[thread overview]
Message-ID: <20170219162717.kwbeapezjjpcp6br@sirena.org.uk> (raw)
In-Reply-To: <1487427991-18174-3-git-send-email-simon.ho@conexant.com>

[-- Attachment #1.1: Type: text/plain, Size: 1553 bytes --]

On Sat, Feb 18, 2017 at 10:26:31PM +0800, simon.ho.cnxt@gmail.com wrote:

> +static const struct snd_kcontrol_new cx2092x_snd_controls[] = {
> +	CX2092X_CONTROL("SendCmd", cmd_info, cmd_get, cmd_put,
> +		SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_WRITE|
> +		SNDRV_CTL_ELEM_ACCESS_VOLATILE),

This is just tunnels commands from some magic userspace application
straight through to the device, taking all the control out of standard
Linux.  What are these controls doing and why aren't we managing them
through standard interfaces?  This tends to end up creating problems
with things like power management as for example userspace might not
figure out that things like suspend and resume are happening.

> +static int cx2092x_reset(struct snd_soc_codec *codec)
> +{
> +	struct cx2092x_priv *cx2092x = snd_soc_codec_get_drvdata(codec);
> +
> +	if (gpio_is_valid(cx2092x->gpio_reset)) {
> +		gpio_set_value(cx2092x->gpio_reset, 0);
> +		mdelay(10);
> +		gpio_set_value(cx2092x->gpio_reset, 1);

Use the _cansleep() versions unless you've got a good reason not to,
that way GPIOs on slow buses can be used.

> +	if (of_match_device(cx2092x_dt_ids, dev))
> +		cx2092x->gpio_reset =
> +			of_get_named_gpio(dev->of_node, "reset-gpio", 0);

Why do we care if we managed to match the device ID?  The GPIO is
optional anyway.  It's also better to use the gpiod_ APIs if you can,
that's more modern and removes the need to special case DT.

> +	if (gpio_is_valid(cx2092x->gpio_reset)) {
> +		int ret;

Don't declare variables in the middle of functions.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]