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

[Takashi Iwai <tiwai@suse.de>]

At Thu, 2 Jun 2011 13:14:25 -0700,
Ian Minett wrote:
> 
> Attached is a patch that adds support for the CA0132 HDA codec,
> creating patch_ca0132.c in pci/hda.

Thanks for the patch!

>  It also adds an entry to the pci/hda makefile that builds the codec
>  when SND_HDA_CODEC_CA0132 is defined. However, we are not sure
>  where to add the KConfig entry for SND_HDA_CODEC_CA0132, which is
>  below, or if we should even create such an entry for this
>  submission.

Yes, at best, please include Kconfig change into the same patch.

But I see now you are patching alsa-driver code instead of kernel tree,
so it's not too trivial indeed.  I recommend you to patch your driver
in the kernel tree first, check it works, then submit the patch, instead
of modifying alsa-driver tree.  We can change alsa-driver tree later for
the new stuff.

About the code, see some quick reviews below.

> +/*
> + * CA0132 chip access stuffs
> + */
> +static int chipio_acquire_mutex(struct hda_codec *codec)
> +{
> +	struct ca0132_spec *spec = codec->spec;
> +
> +	mutex_lock(&spec->chipio_mutex);
> +	return 0;
> +}
> +
> +static int chipio_release_mutex(struct hda_codec *codec)
> +{
> +	struct ca0132_spec *spec = codec->spec;
> +
> +	mutex_unlock(&spec->chipio_mutex);
> +	return 0;
> +}

Avoid to introduce such wrappers.  It's more readable to get spec in
the function and call mutex_lock/unlock appropriately, in general.


> +/*
> + * Write chip address through the vendor widget -- NOT protected by the Mutex!
> + */
> +static int chipio_write_address(struct hda_codec *codec,
> +				unsigned int chip_addx)
> +{
> +	unsigned int res = 0;
> +	int retry = 100;
> +
> +	/* 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);

100 retries look fairly much.  Do we really need so long access?


thanks,

Takashi