[linux-dvb] Re: refactoring

[Ralph Metzler <rjkm@metzlerbros.de>]

Holger Waechtler writes:
 > Gerd Knorr wrote:
 > 
 > >
 > >>I have left it using the kernel i2c_adapter stuff, and i2c_transfer() 
 > >>functions as before.
 > >>    
 > >>
 > >
 > >Which is a bad idea.
 > >
 > 
 > why? please elaborate.
 > 
 > >  Have you actually looked at my cx22702 version?
 > >There is much generic, reusable code in there.  And it is generic
 > >*because* it makes heavy use of the kernl i2c infrastructure.
 > >
 > 
 > no. that's just the reason why it's not portable, not testable in a 
 > userspace mockup and hardly portable to other OSes like MacOS-X and BSD. 
 > The new approach without dependencies to kernel APIs is much nicer.

You could completely rewrite lots of drivers (and not for the better)
with this argument. Of course Linux kernel APIs primarily make it
easier to write drivers just for Linux. So, I should not use any
Linux kernel APIs because they are not in userspace, MacOS-X and BSD?

 
 > >  It
 > >creates and registeres i2c_client structs for both demod + pll and then
 > >passes around just a i2c_client ptr (instead of a fe-specific struct) to
 > >many functions, like this one here:
 > >
 > >   static int pll_dtt759x_set_tv_freq(struct i2c_client *c, u32 freq,
 > >			              int bandwidth);
 > >
 > >It's absolutely no problem to move that to some other -- say -- pll-lib
 > >module and then have other frontend driver reuse it.
 > >  
 > >
 > 
 > I have to say that I prefer Andrew's approach much more, it's simply 
 > more professional + easier to understand and maintain.


No, it also has disadvantages. It is less modular and you need to
write lots of code to support each frontend specifically in each driver.
I have several drivers where I, instead of just supporting an I2C
driver, would have to write tons of new code for frontend support on
top of that if I used that approach.


 > >>>And free the drivers private data.  And any other cleanups the driver
 > >>>migth have to do depending on the hardware.  We certainly need a
 > >>>detach function.
 > >>>      
 > >>>
 > >>There is no private data any longer.
 > >>    
 > >>
 > >
 > >Yes, I've seen the code now.  Thats is a serve design bug.  The drivers
 > >private data should not be public.  And it should be allocated and freed
 > >by the driver itself and not by someone else.
 > >  
 > >
 > 
 > Please calm down. 

Where was he not calm?!?!
This is a valid point. Using private data directly from other
code is usually a sign for a bad design flaw.


 > Not allocating dynamic memory but using static data 
 > instead is always a good thing.

Why?
Now each and every driver has to take care of it instead of just the
demodulator driver.




Ralph