Messages in this thread | | | Date | Thu, 15 Sep 2005 12:15:04 +0200 | From | Takashi Iwai <> | Subject | Re: [Alsa-devel] [RFC 2.6.13.1 1/1] CS5535 AUDIO ALSA driver |
| |
Hi,
At Thu, 15 Sep 2005 17:04:23 +0800, jayakumar.alsa@gmail.com wrote: > > Hi Jaroslav, Martin, alsa and kernel folk, > > Appended is my patch adding support for the CS5535 Audio device. I've > done this patch against 2.6.13.1. I didn't find the CS5535 pci id in > pci_ids so I've added those and the remaining pci functions for the > CS5535 device as defines there. I don't have the ability to test the > chip's SPDIF capability yet. I'll add support for it next. Please let > me know if it looks okay so far and if you have any feedback or > suggestions.
Just small glitches I found below:
> diff -uprN -X dontdiff.13.1 linux-2.6.13.1-vanilla/include/sound/cs5535audio.h linux-2.6.13.1/include/sound/cs5535audio.h > --- linux-2.6.13.1-vanilla/include/sound/cs5535audio.h 1970-01-01 07:30:00.000000000 +0730 > +++ linux-2.6.13.1/include/sound/cs5535audio.h 2005-09-15 14:47:41.000000000 +0800
You can put this header filer into sound/pci/cs5535, so that it won't be exported. Then you don't need __KERNEL__ check, too.
> +typedef struct cs5535audio_dma_desc { > + u32 addr; > + u16 size; > + u16 reserved:13; > + u16 jmp:1; > + u16 eop:1; > + u16 eot:1; > +} cs5535audio_dma_desc_t;
The bitfield isn't portable to use to comminucate with the hardware. Better to use u16 and normal bit operations.
> diff -uprN -X dontdiff.13.1 linux-2.6.13.1-vanilla/sound/pci/cs5535audio/cs5535audio.c linux-2.6.13.1/sound/pci/cs5535audio/cs5535audio.c > --- linux-2.6.13.1-vanilla/sound/pci/cs5535audio/cs5535audio.c 1970-01-01 07:30:00.000000000 +0730 > +++ linux-2.6.13.1/sound/pci/cs5535audio/cs5535audio.c 2005-09-15 14:47:41.000000000 +0800 > +#define do_delay() do { \ > + set_current_state(TASK_UNINTERRUPTIBLE); \ > + schedule_timeout(1); \ > +} while (0)
The loop with do_delay() should be replaced with more portable ones like msleep().
> +static unsigned short snd_cs5535audio_codec_read(cs5535audio_t *cs5535au, > + unsigned short reg) > +{ > + unsigned long regdata=0;
Unnecessary initialization :)
> +static int __devinit snd_cs5535audio_create(snd_card_t *card, > + struct pci_dev *pci, > + cs5535audio_t **rcs5535au) (snip) > + cs5535au = kcalloc(1, sizeof(*cs5535au), GFP_KERNEL);
Let's use the new kzalloc().
> diff -uprN -X dontdiff.13.1 linux-2.6.13.1-vanilla/sound/pci/cs5535audio/cs5535audio_pcm.c linux-2.6.13.1/sound/pci/cs5535audio/cs5535audio_pcm.c > --- linux-2.6.13.1-vanilla/sound/pci/cs5535audio/cs5535audio_pcm.c 1970-01-01 07:30:00.000000000 +0730 > +++ linux-2.6.13.1/sound/pci/cs5535audio/cs5535audio_pcm.c 2005-09-15 14:47:41.000000000 +0800 > +static int cs5535audio_build_dma_packets(cs5535audio_t *cs5535au, > + cs5535audio_dma_t *dma, > + snd_pcm_substream_t *substream, > + unsigned int periods, > + unsigned int period_bytes) > +{ (snip) > + spin_lock(&cs5535au->reg_lock); > + dma->ops->disable_dma(cs5535au); > + dma->ops->setup_prd(cs5535au, jmpprd_addr); > + spin_unlock(&cs5535au->reg_lock);
You need spin_lock_irq() here, instead.
thanks,
Takashi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |