Re: [RFC] TDA8290 / TDA827X with LNA: testers wanted

[Mauro Carvalho Chehab <mchehab@infradead.org>]

On Thu, 20 Mar 2008 21:41:19 +0100
Hartmut Hackmann <hartmut.hackmann@t-online.de> wrote:

> HI, Mauro
> 
> Mauro Carvalho Chehab schrieb:
> > On Thu, 20 Mar 2008 02:23:59 +0100
> > Hartmut Hackmann <hartmut.hackmann@t-online.de> 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@infradead.org>
> > 
> <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.

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.

> 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.

> I will be out from friday to monday.

I should also take a break during those days ;)


Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list