At Tue, 20 Sep 2005 17:23:09 -0700,
Andrew Morton wrote:
>
> jayakumar alsa <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |