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