From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 1/1] Added patch file and Makefile entry for CA0132 HDA codec Date: Wed, 08 Jun 2011 08:04:20 +0200 Message-ID: References: <1307491585-29767-1-git-send-email-ian_minett@creativelabs.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 70DA324535 for ; Wed, 8 Jun 2011 08:04:21 +0200 (CEST) In-Reply-To: <1307491585-29767-1-git-send-email-ian_minett@creativelabs.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: Ian Minett Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org At Tue, 7 Jun 2011 17:06:25 -0700, Ian Minett wrote: > > Thank you very much for the feedback - we've modified the patch following your recommendations: > > - moved the patch to the alsa-kernel git tree > - included the Kconfig change in the patch > - tidied up the mutex calls and brought the number of access retries down. Thanks for the updates! The code looks almost good for merge, but just a few things. See below. > Any comments welcome, please let us know if anything should be changed further. > > Thanks, > Ian > > Signed-off-by: Ian Minett Please give the changelog text to include whenever you re-submit the patch. > +static int chipio_write_address(struct hda_codec *codec, > + unsigned int chip_addx) > +{ > + unsigned int res = 0; > + int retry = 50; > + > + /* send low 16 bits of the address */ > + do { > + res = snd_hda_codec_read(codec, WIDGET_CHIP_CTRL, 0, > + HDA_SHORT_CMD_VENDOR_CHIP_IO_ADDRESS_LOW, > + chip_addx & 0xffff); > + retry--; > + } while (res != HDA_STATUS_VENDOR_CHIP_IO_OK && retry); > + > + if (!retry) > + return -EIO; I'd write a helper function including this loop, e.g. static int chipio_send(struct hda_codec *codec, unsigned int reg, unsigned int data) { unsigned int res; int retry = 50; do { res = snd_hda_codec_read(codec, WIDGET_CHIP_CTRL, 0, reg, data); if (res == HDA_STATUS_VENDOR_CHIP_IO_OK) return 0; } while (--retry); return -EIO; } This code chunk appears repeatedly in the following codes. > +static int ca0132_hp_switch_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); > + struct ca0132_spec *spec = codec->spec; > + long *valp = ucontrol->value.integer.value; > + unsigned int data; > + int err; > + > + /* any change? */ > + if (spec->curr_hp_switch == *valp) > + return 0; You need to wake up the controller/codec appropriately in the control callback if the hardware access is needed. Call snd_hda_power_up() / snd_hda_power_down() appropriately. In you case, call snd_hda_power_up() at this point, and call snd_hda_power_down() at exit of the function. > +/* > + * refresh widget caps that stored in cache > + */ > +static void ca0132_refresh_widget_caps(struct hda_codec *codec) > +{ Oh, this is a tricky part I overlooked at the previous time. This looks like a dead code, though. Is it still needed? > + int i; > + hda_nid_t nid, start; > + > + start = nid = codec->start_nid; > + snd_printd("ca0132_refresh_widget_caps: start_nid=%d\n", start); > + for (i = 0; i < codec->num_nodes; i++, nid++) { > + codec->wcaps[i] = snd_hda_param_read(codec, nid, > + AC_PAR_AUDIO_WIDGET_CAP); > + snd_printd("wcaps[0x%02x]=0x%x\n", i+start, codec->wcaps[i]); Don't use snd_printd(). CONFIG_SND_DEBUG is usually turned on by most distros, so it'd be annoying to see this at each time. Use snd_printdd() if any. > + } > +} > +static void ca0132_set_ct_ext(struct hda_codec *codec, int enable) > +{ > + /* Set Creative extension */ > + snd_printd("SET CREATIVE EXTENSION\n"); Use snd_printdd(). > + snd_hda_codec_write(codec, WIDGET_CHIP_CTRL, 0, > + HDA_LONG_CMD_VENDOR_CHIP_IO_CT_EXTENSIONS_ENABLE, > + enable); > + msleep(20); Isn't this command needed when you recover from S3/S4 or powersave? If yes, this should be called in the init callback instead of patch_ca0132(). > +static void ca0132_init_chip(struct hda_codec *codec) > +{ > + struct ca0132_spec *spec = codec->spec; > + > + mutex_init(&spec->chipio_mutex); > +} > + > +static void ca0132_exit_chip(struct hda_codec *codec) > +{ > + /* put any chip cleanup stuffs here. */ > +} > + > +static int ca0132_init(struct hda_codec *codec) > +{ > + struct ca0132_spec *spec = codec->spec; > + struct auto_pin_cfg *cfg = &spec->autocfg; > + int i; > + > + for (i = 0; i < spec->multiout.num_dacs; i++) { > + init_output(codec, spec->out_pins[i], > + spec->multiout.dac_nids[i]); > + } > + init_output(codec, cfg->hp_pins[0], spec->hp_dac); > + init_output(codec, cfg->dig_out_pins[0], spec->dig_out); > + > + for (i = 0; i < spec->num_inputs; i++) > + init_input(codec, spec->input_pins[i], spec->adcs[i]); > + > + init_input(codec, cfg->dig_in_pin, spec->dig_in); > + > + return 0; > +} > + > + > +static void ca0132_free(struct hda_codec *codec) > +{ > + ca0132_exit_chip(codec); > + kfree(codec->spec); > +} > + > +static struct hda_codec_ops ca0132_patch_ops = { > + .build_controls = ca0132_build_controls, > + .build_pcms = ca0132_build_pcms, > + .init = ca0132_init, > + .free = ca0132_free, > +}; > + > + > + > +static int patch_ca0132(struct hda_codec *codec) > +{ > + struct ca0132_spec *spec; > + > + snd_printd("patch_ca0132\n"); Use snd_printdd(). > + spec = kzalloc(sizeof(*spec), GFP_KERNEL); Missing error check. > + memset((void *)spec, 0, sizeof(*spec)); No need for cast. thanks, Takashi