Re: PATCH - ESI Juli driver

[Takashi Iwai <tiwai@suse.de>]

At Mon, 17 Mar 2008 10:37:22 +0100,
Pavel Hofman wrote:
> 
> >>
> >> I did not want to copy/paste any algorithmical code from ice1724.c to 
> >> juli.c as it complicates future maintenance. Now, the vt1724 code in 
> >> ice1724.c is pretty much about general algorhitms, specific routines are 
> >> in stdclock_... I tested the original clock with Prodigy192 which 
> >> provided testing analog/SPDIF signals. There are no functional changes.
> > 
> > I see the point that you want to make it generic.  But, too many
> > callbacks, in other words, too many tuning parameters make the code
> > difficult to follow.  It's a question of balance.
> 
> Takashi, I understand your concern. On the other hand, if the callbacks 
> are well documented, it actually makes the code easier to follow, as it 
> separates general algorithms from detailed HW implementation. I can 
> certainly add more comments to the callback functions.
> 
> Honestly, I do not know which callbacks to remove (apart of those two 
> not redefined in Juli). Otherwise I will have to copy/paste serious 
> portions of code to juli.c which I would really want to avoid.
>
> As I see it, the clocking code in ice1724 was written for one specific 
> implementation. There was no need to do so in a general way and I 
> completely understand that. Nevertheless, juli uses a different clock 
> implementation and I believe the time has come to make the code in 
> ice1724 more general. Unfortunately, I do not know of any other 
> technology but callbacks.
> 
> What would you recommend?

IMO, rate_code can be avoided.  Instead of exposing the encoded value,
better to use the raw rate value as parameters.
And, the texts inf rates_info can be generated dynamically.

So, what we need primarily are callbacks to get and set the current
rate setting.  Suppose rate=0 as SPDIF-in, we can pass the raw rate
value.  Then snd_vt1724_pro_internal_clock_get() would just a function
to get the current rate and compares it with the given rate_info[]
value, returns the index.

How to set stream-specific hw_params is another question.  But surely
we can cut off a bit more.


thanks,

Takashi