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
