Re: [PATCH 1/1] Added patch file and Makefile entry for CA0132 HDA codec

[Takashi Iwai <tiwai@suse.de>]

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