Date: Thu, 15 Sep 2005 12:15:04 +0200
Message-ID: <s5hk6hi8wnr.wl%tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: jayakumar.alsa@gmail.com
Cc: perex@suse.cz, mj@ucw.cz, alsa-devel@lists.sourceforge.net,
       linux-kernel@vger.kernel.org
Subject: Re: [Alsa-devel] [RFC 2.6.13.1 1/1] CS5535 AUDIO ALSA driver
In-Reply-To: <200509150904.j8F94NKP000991@localhost.localdomain>
References: <200509150904.j8F94NKP000991@localhost.localdomain>
User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.7 (=?ISO-8859-4?Q?Sanj=F2?=) APEL/10.6 MULE XEmacs/21.5 (beta18)
 (chestnut) (+CVS-20041021) (i386-suse-linux)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3306
Lines: 93

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/