[linux-dvb] Re: [Patch] Adding support for the Hauppage HVR1100

[Steve Toth]

Johannes Stezenbach wrote:
> On Thu, Dec 01, 2005, Steve Toth wrote:
>   
>> Mauro Carvalho Chehab wrote:
>>     
>>> Em Qui, 2005-12-01 às 01:54 -0500, Steve Toth escreveu:
>>>       
>>>> --- linux/drivers/media/video/cx88/cx88-i2c.c	16 Oct 2005 12:13:58 
>>>> -0000	1.33
>>>> +++ linux/drivers/media/video/cx88/cx88-i2c.c	1 Dec 2005 06:39:32 
>>>> -0000
>>>> @@ -140,7 +140,20 @@ void cx88_call_i2c_clients(struct cx88_c
>>>> {
>>>> 	if (0 != core->i2c_rc)
>>>> 		return;
>>>> -	i2c_clients_command(&core->i2c_adap, cmd, arg);
>>>> +
>>>> +	if (core->dvbdev == NULL) {
>>>> +		i2c_clients_command(&core->i2c_adap, cmd, arg);
>>>> +	} else {
>>>> +
>>>> +		if (core->dvbdev->dvb.frontend->ops->enable_plli2c)
>>>> +		 
>>>> core->dvbdev->dvb.frontend->ops->enable_plli2c(core->dvbdev->dvb.frontend);
>>>> +
>>>> +		i2c_clients_command(&core->i2c_adap, cmd, arg);
>>>> +
>>>> +		if (core->dvbdev->dvb.frontend->ops->disable_plli2c)
>>>> +		 
>>>> core->dvbdev->dvb.frontend->ops->disable_plli2c(core->dvbdev->dvb.frontend);
>>>> +	}
>>>> +
>>>> }
>>>>    
>>>>         
>>> 	Hmmm... IMHO, cx88-i2c is not the best place for it. It seems to be
>>> better somewere at cx88-dvb or at a merger tuner-simple/dvb-pll code.
>>>  
>>>       
>>>>    
>>>>         
>> Thanks. In a previous patch I tried the cx88-dvb method. It made cx88 
>> look very ugly. See the previous patch on the list, and all of my 
>> comments/findings.
>>
>> The fact is: enable/disable has to be execute before and after every 
>> possible i2c transaction in cx88. It's basically a patch designed to 
>> solve an I2C related gate problem with any dvb frontend (not 
>> specifically the cx22702). In the original patch, (posted a few days 
>> ago) I had many external symbols defined in cx22702, cx88-dvb and had 
>> patches to cx88-dvb, cx88-video. It was pretty ugly and created a lot of 
>> circular depmod references.
>>     
>
> Hm, it is called enable_plli2c(). Can it be added in a place where
> we know that the following i2c transaction talks to the PLL?
> Or do you want to rename it?
>   
Thanks.

I don't understand, rename it to...  what exactly? Something cx22702 
specific you mean? If that's the case, I'm not sure I agree. This really 
is no longer a cx22702 only patch, it modifies the framework to allow 
all demodulators drivers to have their gates opened or closed, assuming 
you have a reference to the frontend.

In principle, if you have a reference to a dvb_frontend, you can 
enable/disable the gate with no concept of whether it's a cx22702, a 
stv0229 or anything else. It also allows the exports (stv0299 in the 
fute) to be removed, if that's beneficial to anyone.

Maybe the overall approach is wrong. Initially I tried a board specific 
patch, which nobody liked. Then I tried a generic framework patch, which 
people don't appear to like. Is their another way?

In terms of it's place in the kernel. I originally had it (previous 
patch) surrounding all of the necessary call_i2c_client calls in 
cx88-video (and probably cx88-tvaudio  + cx88-alsa in the future?) but 
it REALLY did look ugly and it was just replicating the same code time 
after time. For example, it was in six different places in the same file.

Instead (this time), I tried to do something that guaranteed any future 
V4L IOCTL calls (in cx88) that make i2c requests we're automatically 
dealt with cleanly with zero developer awareness of the problem. By 
placing the enable/disable in the generate i2c handler, it's in one 
place and it's not going to be a problem if someone implements another 
call to call_i2c_clients.

So, I guess we are trying the following trade-off:   Do we put the code 
in lots of key places, many many times to deal with each specific I2C 
cx88 V4L IOCTL ... hoping that future developers will also remember to 
add the same code?.... or, Do we do it in one place and it's invisible 
to the cx88 V4L IOCTL developers until they try and use I2C access (in 
which case it's done automatically for them)?

No easy answer, what do you think? I've posted patches for three 
different approached to the list. One was fairly bad, open or nothing 
for specific boards - introducing potential rf noise. The second tried 
patching individual ioctls but got ugly quickly as the number of ioctls 
was almost into double figures (lots of repeated code). Lastly, my 
preferred approach - this way - basically a fundamental part of the 
dvb/v4l-cx88 framework.

I'm happy to try another approach if anyone has ideas.
>   
>> This is a patch which means we can get the HVR1100/HVR110LP/HVR1300 and 
>> another prototype reference designs supported by the kernel quickly, in 
>> a clean implementation.
>>
>> Johannes might want to shoot me for patching dvb_frontend.h though :)
>>     
>
> No. My only concern is that we don't want to add quick hacks to
> support one type of card which mgith create maintenance problems for other
> cards later.
> BTW, it isn't my code you're patching, in fact I do very
> little development myself. I wish more of the developers on linux-dvb
> would take the time to review patches. And I also wish the developers
> with CVS accounts would stil post their patches here for review
> instead of just whacking their cruft into CVS.
>
> The current lack of peer review for DVB stuff is bad.
>
>   

It's difficult for me, and probably you. You're clearly responsible for 
keeping the dvb kernel clean, and I want to make sure that the patches 
you get offer real value (without any Hauppauge bias).

I do keep asking for feedback from everyone.

Regards,

Steve