[alsa-devel] [PATCH 2/2] ASoC: Add driver for CX2092X I2S Audio DSP

[Simon Ho]

On Sun, Feb 19, 2017 at 04:27:17PM +0000, Mark Brown wrote:
> On Sat, Feb 18, 2017 at 10:26:31PM +0800, simon.ho.cnxt at 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.
> 

This device is designed for virtual assisant application need to be always
open, listening for users to summon it. There is no any power saving mode 
support on this device. The processed voice data will be sent to automatic 
speech recognition (ASR) application for further processing and the
processed voice data will be optimized for ASR rather than a human being.
So such as mute and volume gain control are not necessary. These tunnels 
commands are actucally for debugging purpose only, they won't be used on
normal operation.  

> > +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.
> 

Thanks for input, I changed it to _cansleep() version.

> > +	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.
> 

No, we don't need to care the deivce ID. Remved.

Learned, will use gpiod_ APIs instead.

> > +	if (gpio_is_valid(cx2092x->gpio_reset)) {
> > +		int ret;
> 
> Don't declare variables in the middle of functions.

Understood. corrected it.