Return-Path: <linux-kernel-owner+willy=40w.ods.org-S1751220AbVI2NEq@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751220AbVI2NEq (ORCPT <rfc822;willy@w.ods.org>);
	Thu, 29 Sep 2005 09:04:46 -0400
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751237AbVI2NEq
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Thu, 29 Sep 2005 09:04:46 -0400
Received: from cantor.suse.de ([195.135.220.2]:53936 "EHLO mx1.suse.de")
	by vger.kernel.org with ESMTP id S1751220AbVI2NEp (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 29 Sep 2005 09:04:45 -0400
Date: Thu, 29 Sep 2005 15:04:44 +0200
Message-ID: <s5hr7b8dnyr.wl%tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Andrew Morton <akpm@osdl.org>
Cc: jayakumar.alsa@gmail.com, perex@suse.cz, mj@ucw.cz,
       alsa-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [Alsa-devel] Re: [PATCH 2.6.13.1 1/1] CS5535 AUDIO ALSA driver
In-Reply-To: <20050920172309.626db866.akpm@osdl.org>
References: <200509190639.j8J6dIM4007948@localhost.localdomain>
	<20050920152830.7ef6733b.akpm@osdl.org>
	<47f5dce305092017031a2ba375@mail.gmail.com>
	<20050920172309.626db866.akpm@osdl.org>
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
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3510
Lines: 88

At Tue, 20 Sep 2005 17:23:09 -0700,
Andrew Morton wrote:
> 
> jayakumar alsa <jayakumar.alsa@gmail.com> wrote:
> >
> > > > +
> > > > +static unsigned short snd_cs5535audio_codec_read(cs5535audio_t *cs5535au,
> > > 
> > > typedefs are unpopular in-kernel.  We generally don't get too fussed if a
> > > driver maintainer really wants them there.  The main objection is that we
> > > now have two names for the same thing.  Plus they cannot be used when
> > > forward-declaring a structure.
> > 
> > I just used those typedefs in order to match the style in all the
> > other alsa drivers. For example:
> > 
> > % egrep typedef $lintree/sound/pci/*.c
> > als4000.c:typedef struct {
> > atiixp.c:typedef struct snd_atiixp atiixp_t;
> > atiixp.c:typedef struct snd_atiixp_dma atiixp_dma_t;
> > atiixp.c:typedef struct snd_atiixp_dma_ops atiixp_dma_ops_t;
> > atiixp.c:typedef struct atiixp_dma_desc {
> > azt3328.c:typedef struct _snd_azf3328 azf3328_t;
> > azt3328.c:typedef struct azf3328_mixer_reg {
> > bt87x.c:typedef struct snd_bt87x bt87x_t;
> > cmipci.c:typedef struct snd_stru_cmipci cmipci_t;
> > <snip>
> > 
> > I'm not sure what to do. I'd be happy to take them out. But I woudn't
> > mind leaving them in if that's what alsa convention is.
> 
> I'd be inclined to stick with the alsa style.  That's just an fyi if you
> plan on working in other places.

It's no longer preferred style in ALSA.
xxx_t had been used for the magic type-cast checks.  But this check
was removed, and there is no reason to stick with its style. 
I'm using struct foo in the recent drivers like hd-audio, too.
Tons of *_t are still there just because of laziness :)

Seriously, if you guys want to clean up them, I wouldn't reject at
all.  But this clean up is at the very tail of my TODO list ;)


> > > 
> > > > +     addr = (u32)substream->runtime->dma_addr;
> > > 
> > > Nope, _snd_pcm_runtime.addr has type dma_addr_t, which is an opaque type,
> > > 64-bit on some platforms.  I expect this driver will blow up on those
> > > platforms.
> > 
> > The 5535 hw's dma descriptor is only 32 bit capable. I guess I should
> > look into informing the dma alloc that the buffers need to be in the
> > lower 32. Would it be okay to drop the upper then?
> 
> An IOMMU permits 64-bit platforms to use 32-bit PCI devices.

But still you have to set a 32bit (bus) address to the buffer
descriptor of this chip.  If you get higher than that, it won't work
anyway.


> > > +#define cs_writel(reg, val)    outl(val, (int) cs5535au->port + reg)
> > > +#define cs_writeb(reg, val)    outb(val, (int) cs5535au->port + reg)
> > > +#define cs_readl(reg)          inl((unsigned short) (cs5535au->port + reg))
> > > +#define cs_readw(reg)          inw((unsigned short) (cs5535au->port + reg))
> > > +#define cs_readb(reg)          inb((unsigned short) (cs5535au->port + reg))
> > > 
> > > erk.   subsystem-wide helper macros which reference local variables?
> > 
> > Ok. I'll change that.
> 
> well, again, if that's alsa-style then you might choose to retain it.  But
> it'd be an unpopular approach in most other kernel code.

Well, it's not ALSA style, too.  We prefer often like
	foo_write(chip, reg, val)
expanded to
	outl(val, (chip)->port + (reg))
but don't assume local variables.


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/