Re: [Alsa-devel] Re: [PATCH 2.6.13.1 1/1] CS5535 AUDIO ALSA driver

[Takashi Iwai <tiwai@suse.de>]

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