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 ian_minett@creativelabs.com
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