Re: [Openvortex-dev] Driver updates to come.

[Jeff Muizelaar]

I readded the openvortex-dev CC.

On 10 Jul 2003, Manuel Jander wrote:

> Hi Jeff,
>
> > I am reviewing the patch right now, without actually testing yet. I'll
> > list stuff as I come across it.
> >
> > 1. I think it would be better to use something like 'struct vortex_stream'
> > instead of 'stream_t'. stream_t gives the impression that it is a global
> > type and not something specific to the drivers. Also, typedefs, although
> > used readily through out alsa, are frounded upon in kernel style
>
> Agree.
>
> > 2. How about seperating vortex_adb_checkinout into vortex_adb_checkin and
> > vortex_adb_checkout, so that the each function only does one thing.
>
> You'll see that the resulting code is much simpler as it is now.
> Separating this function would lead to more useless code. The entire
> route de/alloc is just uniformely decided with the variable "en". I do
> not agree separating it.

I definitely see where you are comming from here, and agree that it is not
nice to duplicate the routing code in allocroute. However, I think keeping
the interface we export to au88x0_pcm.c sane is more important. Ie. using
something more like:

int  vortex_adb_allocroute(vortex_t *vortex, int nr_ch, int dir)
void vortex_adb_freeroute(vortex_t *vortex, stream_t *stream)

instead of just:

int vortex_adb_allocroute(vortex_t *vortex, int dma, int nr_ch, int dir)


seperating checkinout is not nearly as important as this, it just seems a
little bit more natural when this interface change is made.

I attached a very quickly hacked together untested uncompiled patch that
does things more like this.

> > 3. locking: I am not sure if using a spinlock is the best thing to do
> > here. Afaik we don't ever adjust the routing in the interrupt context so
> > sleeping during route allocation should be ok. I was thinking of just
> > having a adb semaphore.
>
> The lock needs o be in place during the resource checkout or checkin,
> since if other streams de/allocate resources at the same time there is a
> chance of a conflict.

I know. I meant for the spinlock to be replaced with a semaphore. This
isn't really important right now though...

>
> > 4. I merged the spin_unlock stuff in read/write_codec
>
> Oops. I didnt meant to change anything inside of the codec write/read
> functions.
>
> > Other than that, from my brief look, things look pretty good.
>
> Something that should be noticed:
>
> - This patch will respawn the Chip and Dale bug for now, because it
> allows usage of the DMA engine 0. The rate of the occurrences dropped a
> lot but it still appears sometimes. I suspect that somewhere in the DMA,
> FIFO functions, setting one DMA overwrites other neighbor addresses,
> because sometimes setting up DMA 1 when DMA 0 is in a "chip and dale
> state", it fixes the behaviour of DMA 0, and it continues playing
> correctly. That is, DMA 15 messes with DMA 14, DMA 14 messes with DMA 13
> .... DMA 1 messes with DMA 0, and DMA 0 messes up something below which
> causes the chip and dale bug.
>
> Best Regards
>
> Manuel Jander
>

resman-split.patch
Description: Text document