Hi, Mauro
Mauro Carvalho Chehab schrieb:
> On Thu, 20 Mar 2008 21:41:19 +0100
> Hartmut Hackmann <hartmut.hackmann@xxxxxxxxxxx> wrote:
>
>> HI, Mauro
>>
>> Mauro Carvalho Chehab schrieb:
>>> On Thu, 20 Mar 2008 02:23:59 +0100
>>> Hartmut Hackmann <hartmut.hackmann@xxxxxxxxxxx> wrote:
>>>
>>>>> On your patch, you're just returning, if dev=NULL, at saa7134 callback function. IMO, the correct would be to
>>>>> print an error message and return. Also, we should discover why dev is being
>>>>> null there (I'll try to identify here - the reason - yet, I can't really test,
>>>>> since the saa7134 boards I have don't need any callback.
>>>> That's not the point. In the call in tda827x.c tda827xa_lna_gain(), the argument
>>>> did not point to the saa7134_dev structure as the function expected. I added
>>>> the check for NULL because only at the very first call, the pointer is still
>>>> not valid. I did not check this carefully but i guess this is a matter of the
>>>> initilization sequence of the data structures. IMHO yes, we should understand this
>>>> sometime but this does not have priority because i am sure that the NULL pointer
>>>> occurs only during initialization.
>>> This is caused by a patch conflict between hybrid redesign and the merge of
>>> xc3028 support. The enclosed experimental patch fixes the tuner_callback
>>> argument, on linux/drivers/media/dvb/frontends/tda827x.c.
>>> It should also fix the priv argument on saa7134_tuner_callback(). I can't test
>>> the saa7134 part here, due to the lack of a saa7134 hardware that needs a
>>> callback.
>>>
>>> The patch also intends to make xc3028 easier to use. That part is still not
>>> fully working. I should finish this patch tomorrow.
>>>
>>>>>>> I still need to send a patchset to Linus, after testing compilation
>>>>>>> (unfortunately, I had to postpone, since I need first to free some
>>>>>>> hundreds of Mb on my HD on my /home, to allow kernel compilation).
>>>>>>> Hopefully, I'll have some time tomorrow for doing a "housekeeping".
>>>>>>>
>>>>>> Unfortunately, i deleted you mails describing what went to linux and i don't
>>>>>> have the RC source here :-(
>>>>> You may take a look on master branch on my git tree. I'm about to forward him a
>>>>> series of patches. Hopefully, 2GB free space will be enough for a full kernel
>>>>> compilation. I'll discover soon...
>>>>>
>>>> Jep. Meanwhile Michael confirmed that the problem is not in mainstream,
>>>> so there is no reason to hurry.
>>> Yes.
>>>
>>>> But we should have a bigger audience for my latest changes, so i will send
>>>> you a pull request in a minute.
>>> Could you please test my patch first? Having the same arguments for all
>>> callback functions avoid future mistakes.
>>>
>>> ---
>>> [RFC] Fix tuner_callback for tda827x
>>>
>>> Signed-off-by Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx>
>>>
>> <snip>
>>
>> Your patch does not completely apply for me, it fails in cx88-dvb.c
>
> This is due to some changes that happened later on the tree, for cx88.
>
> I also had some a trivial conflict on saa7134-cards, when merging from your tree ;)
>
> The better is to do an "hg pull -u", before asking me to pull.
>
Basically you are right of corse. I try to avoid this after i once saw a merge
causing an unnecessary huge change log. But this was an old hg version.
> Anyway, the conflict were easily solved.
>
>> I had a close look and found that we are going in the same direction.
>> - The change in tda827x is the same as i did.
>> - In saa7134-cards.c your patch is right. My version just worked by accident.
>> I corrected this in my repository.
>> By the way: the dev pointer is NULL during initialization is gone.
>> I tested again and things work for me.
>
> Great news.
>
>> I would recommend the following:
>> - You pull from my repository (sent you the request yesterday)
>> - You apply the patch *except* the changes in tda827x.c, saa7134-cards.c
>> and saa7134-dvb.c. Afterwards we should be fine.
>
> Ok, I did this.
>
> Patches applied on a order that won't generate bissect compilation issues.
>
> I still had to patch saa7134-dvb, probably due to the conflict
> I've mentioned. I'll double check when importing the changes to -git, since
> I'll need to manually fix there, instead of trusting on kdiff3.
>
> I also added a printk, if dev=NULL. This won't work with the current code, but
> it helps to track future issues, if we have this kind of troubles later with
> other callbacks.
>
That's fine.
>> My other changes to tda827x and saa7134-dvb.c are not only cosmetic. It merged
>> the _lna_gain functions for analog and dvb and adapt the data structures.
>> What do you think?
>
> Seems fine to me.
>
> Just one comment about the config var (this applies also to the previous code):
> I'd prefer to have an enum, instead of config=0,1,2,3. Something like:
>
> enum {
> TDA827x_NO_LNA,
> TDA827x_LNA_VIA8290_LOW,
> TDA827x_LNA_VIA8290_HIGH,
> TDA827x_LNA_VIA_HOST,
> } config;
>
> This helps people to better understand the LNA config code.
>
Hm, i did similar things in other places...
I did not do this here because i wanted to be able to use this variable
with other tuners as well -> use #defines instead?
>> I will be out from friday to monday.
>
> I should also take a break during those days ;)
>
>
> Cheers,
> Mauro
>
Best regards
Hartmut
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
