Messages in this thread | | | Date | Thu, 29 Sep 2005 15:04:44 +0200 | From | Takashi Iwai <> | Subject | Re: [Alsa-devel] Re: [PATCH 2.6.13.1 1/1] CS5535 AUDIO ALSA driver |
| |
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/
| |