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.