Re: [linux-dvb] [RFC] tuner-refactor-phase-2

2007-10-26 Thread Mauro Carvalho Chehab
> NO -- the tuner changes do not touch bttv -- they are all internal to
> the tuner code.

Good to know.

> If you have to remove some v4l1 support from tuner-core, that will
> simply be the removal of a few lines, and it can be easily done by hand.
> 
> OTOH, If you push in the v4l1 removal first, and it conflicts with the
> pending tuner changes, that _will_ cause a problem.  It will require for
> the entire patch series to be regenerated, and this will result in
> holding up Hans from moving forward with his work, until I have time to
> regenerate the entire series.

Let's see what'll happen at the merging stage. If you're saying that the
changes are all internal, conflicts shouldn't arrive.

You both should notice that bttv v4l1 tree touches on most i2c audio
drivers, converting driver APIs to V4L2. Currently, except for msp3400,
all other drivers support only V4L1 calls. A V4L2 userspace app won't be
able to properly configure audio on non-msp34xx bttv boards. This is the
main reason for doing this change.

> If you are going to push in the xc2028 stuff, the core changes should go
> in first, then you should alter your new patch as required.  I do not
> expect this xc2028 driver to work with the devices that I have, and I
> believe that you are going to create a large confusion about firmware,
> not to mention that you do not have any legal rights to alter or
> distribute the firmware.

I'm not discussing firmwares. If this can't be solved on a clean way,
there's always the "dirty" way of having a script that extracts the
firmware from the windows driver. Several drivers, including the old
versions of ivtv and pvrusb2, as well as some newer ones, like opera1
uses this approach.

Yet, I really hope that we can have some official permission for
distributing Linux firmwares from all vendors.

> I wouldn't rush in the xc2028 stuff before the other changes.

If not committed, newer changes at tuner API will require more work, and
I don't want to spend my time re-basing it again.

The hybrid design phase 1 changes required me almost 6 hours to port the
driver, just to satisfy the newer API, without adding a single newer
functionality.

Converting it were nice, since I've discovered that there are several
bad things at the current hybrid design.

For example, I had to change several parameter exchanges at the driver
due to things like:

tuner_info("foo")

tuner_info internally depends of a var called "priv->i2c_propos". If you
don't have this declared, the compilation will fail. 

So, it is required to add i2c_propos at the private structure, and to
rename the driver internal structure to "priv", even being a name that
doesn't mean anything inside a driver. 

The tuner core required a hidden "priv" struct is really bad (*)

(*) Ok, the previous tuner_info call also had hidden parameters.

We should avoid this kind of design on newer code. We should, instead,
define tuner_info to be used like:
tuner_info(i2c_propos, "foo")

As the result of this kind of changes, I needed to add 6 newer fields to
my "priv" struct (in fact, struct xc2028_data), and rename it from the
nice "xc2028" name to the meaningless "priv", just to satisfy the newer
hybrid design.

Since there are more phases to happen, if the driver is not inserted on
core, it will mean that more rebase will be needed, for every newer
tuner redesign phase.

-- 
Cheers,
Mauro


___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb


Re: [linux-dvb] [RFC] tuner-refactor-phase-2

2007-10-26 Thread Michael Krufky
Mauro Carvalho Chehab wrote:
> Em Sex, 2007-10-26 às 12:09 +0200, Hans Verkuil escreveu:
>   
>> On Friday 26 October 2007 06:24, Michael Krufky wrote:
>> 
>>> Mauro Carvalho Chehab wrote:
>>>   
 Hi Michael,

 Em Seg, 2007-10-22 às 16:03 -0400, [EMAIL PROTECTED] escreveu:
 
> Mauro & others,
>
> After our conversation last week, I decided to move forward with
> tuner-refactor-phase-2, so that you can have the pathway for your
> tda9887 & tea5767 changes to go in without clashing with my
> pending work.
>
> My code is now ready for review:
>
> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-2
>   
 I expect a few troubles on merging the newer patches for 2.6.25,
 since there are several significant changes that are expected to
 happen, during this development period, like:

 - the newer tuner-redesign changesets;
 - i2c redesign;
 - bttv removal of V4L1 support;
 
>>> ^^^ These above do not conflict with each other. 
>>>   
>
> I suspect that bttv V4L1 removal will conflict with both changesets.
> I'll need to decide what's the better order for merging those 3 changes
> to avoid breaking bttv. 
>   
NO -- the tuner changes do not touch bttv -- they are all internal to
the tuner code.

If you have to remove some v4l1 support from tuner-core, that will
simply be the removal of a few lines, and it can be easily done by hand.

OTOH, If you push in the v4l1 removal first, and it conflicts with the
pending tuner changes, that _will_ cause a problem.  It will require for
the entire patch series to be regenerated, and this will result in
holding up Hans from moving forward with his work, until I have time to
regenerate the entire series.

> bttv has several "hacks" for probing i2c audio devices. Depending on the
> way Hans touched bttv, the conflicts will emerge.
>
>   
Hans did not touch anything yet -- he's waiting for the pending changes
to first be merged.

As I said before, Hans and I spoke about our changes with each other,
and made sure that we would not create any patch conflicts.  Everybody
is waiting for the large changes to be merged in first before they move
on to making new changes to the affected code.

> With the new tuner-xc2028, the tuner will only work after receiving a
> TUNER_SET_CONFIG, specifying the firmware driver name[2] and the audio
> mode (RF or MTS).
>
> [2] from what I got, it seems that different bridge chips may need
> different firmwares. TM6000 driver, for example, doesn't work with
> xc3028 version 2.7 firmware.
>   
If you are going to push in the xc2028 stuff, the core changes should go
in first, then you should alter your new patch as required.  I do not
expect this xc2028 driver to work with the devices that I have, and I
believe that you are going to create a large confusion about firmware,
not to mention that you do not have any legal rights to alter or
distribute the firmware.

I wouldn't rush in the xc2028 stuff before the other changes.

-Mike

___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [RFC] tuner-refactor-phase-2

2007-10-26 Thread Hans Verkuil
On Friday 26 October 2007 13:28, Mauro Carvalho Chehab wrote:
> Em Sex, 2007-10-26 às 12:09 +0200, Hans Verkuil escreveu:
> > On Friday 26 October 2007 06:24, Michael Krufky wrote:
> > > Mauro Carvalho Chehab wrote:
> > > > Hi Michael,
> > > >
> > > > Em Seg, 2007-10-22 às 16:03 -0400, [EMAIL PROTECTED] escreveu:
> > > >> Mauro & others,
> > > >>
> > > >> After our conversation last week, I decided to move forward
> > > >> with tuner-refactor-phase-2, so that you can have the pathway
> > > >> for your tda9887 & tea5767 changes to go in without clashing
> > > >> with my pending work.
> > > >>
> > > >> My code is now ready for review:
> > > >>
> > > >> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-2
> > > >
> > > > I expect a few troubles on merging the newer patches for
> > > > 2.6.25, since there are several significant changes that are
> > > > expected to happen, during this development period, like:
> > > >
> > > > - the newer tuner-redesign changesets;
> > > > - i2c redesign;
> > > > - bttv removal of V4L1 support;
> > >
> > > ^^^ These above do not conflict with each other.
>
> I suspect that bttv V4L1 removal will conflict with both changesets.
> I'll need to decide what's the better order for merging those 3
> changes to avoid breaking bttv.
>
> Any decision taken, however, would mean that the newer v4l-dvb tree
> will require more tests than it used to require, to be stable.
>
> >  Hans and I have
> >
> > > coordinated such that we will not patch clash, and the i2c
> > > conversions deal mostly with client modules...  any impact on
> > > bttv, if any, will be localized in the i2c code.
>
> bttv has several "hacks" for probing i2c audio devices. Depending on
> the way Hans touched bttv, the conflicts will emerge.
>
> > >  The pending bttv patch
> > > probably needs updating anyway, due to changes upstream.
>
> Yes. before hybrid redesign, however, the changes at bttv weren't
> much significant. I'm counting with you both to help on solving the
> conflicts that might emerge.
>
> > I agree with Mike here. My i2c patches do not touch the tuner, nor
> > should they conflict with anything else. It does the initial
> > conversion of a bunch of i2c drivers and installs the framework. No
> > v4l driver is actually using it yet, ivtv will be the first to
> > switch over.
>
> If you didn't touch much on bttv, than it should be easier to solve
> the conflicts.
>
> > > The changes above hold priority over the two below.
> > >
> > > > - xc3028 old code removal;
> > > > - tuner-xc2028 addition;
> > >
> > > Regardless, the xc2028 changes are unlikely to cause any conflict
> > > with the other changes.
> > >
> > > Hans is waiting for the tuner-refactor-phase-2 tree to be merged
> > > before he pushes in the i2c changes, and you should wait for
> > > those both to be merged before dealing with xc2028, in my
> > > opinion.
> >
> > Actually, xc2028 can go in before my i2c changes since these i2c
> > changes to not involve the tuner. They are not dependent on one
> > another.
>
> I'll probably try to do xc2028 and hybrid tuner changes about the
> same time, since both deals with similar things.
>
> Since I've replaced the CONFIG_TUNER_XC3028 by CONFIG_XC2028 on all
> drivers [1], including ivtv, some minor conflicts might arrive. Since
> ivtv xc3028 support is based at the userspace module, you will need
> to fix ivtv driver later, for it to work with the newer driver.
>
> [1] http://linuxtv.org/hg/~mchehab/tm6000-ng/rev/065874933156
>
> With the new tuner-xc2028, the tuner will only work after receiving a
> TUNER_SET_CONFIG, specifying the firmware driver name[2] and the
> audio mode (RF or MTS).
>
> [2] from what I got, it seems that different bridge chips may need
> different firmwares. TM6000 driver, for example, doesn't work with
> xc3028 version 2.7 firmware.
>
> > > > Since those envolves several changes at core, it is likely that
> > > > changesets will conflict.
> > >
> > > anything is possible, but i don't think it's likely :-)
> > >
> > > > So, I intend to carefully handle the 2.6.25 changesets already
> > > > finished during this weekend, hopefully.
> > >
> > > OK.  I have more changes planned that depend on these... If I add
> > > changesets to the tree, i'll send you addendums to my original
> > > pull request.
> >
> > From my perspective it would be great if the tuner and i2c changes
> > would go in on Saturday, then I can use Sunday to do the tuner i2c
> > conversion, deliver yet another i2c driver for the Mitsubishi
> > M52790 A/V switch and add in ivtv patches to support two new
> > boards. It's no wonder ivtv suffers relatively often from i2c
> > detection issues: it supports no less than 15 different i2c
> > devices.
>
> I'll try to finish this on Sat, but I can't promise.

Of course, I understand.

Just to make it absolutely clear: my i2c changes DO NOT TOUCH bttv. Or 
any other V4L driver for that matter. You need to explicitly change the 
V4L driver to make use of the new i2c code, without t

Re: [linux-dvb] [RFC] tuner-refactor-phase-2

2007-10-26 Thread Mauro Carvalho Chehab
Em Sex, 2007-10-26 às 12:09 +0200, Hans Verkuil escreveu:
> On Friday 26 October 2007 06:24, Michael Krufky wrote:
> > Mauro Carvalho Chehab wrote:
> > > Hi Michael,
> > >
> > > Em Seg, 2007-10-22 às 16:03 -0400, [EMAIL PROTECTED] escreveu:
> > >> Mauro & others,
> > >>
> > >> After our conversation last week, I decided to move forward with
> > >> tuner-refactor-phase-2, so that you can have the pathway for your
> > >> tda9887 & tea5767 changes to go in without clashing with my
> > >> pending work.
> > >>
> > >> My code is now ready for review:
> > >>
> > >> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-2
> > >
> > > I expect a few troubles on merging the newer patches for 2.6.25,
> > > since there are several significant changes that are expected to
> > > happen, during this development period, like:
> > >
> > > - the newer tuner-redesign changesets;
> > > - i2c redesign;
> > > - bttv removal of V4L1 support;
> >
> > ^^^ These above do not conflict with each other. 

I suspect that bttv V4L1 removal will conflict with both changesets.
I'll need to decide what's the better order for merging those 3 changes
to avoid breaking bttv. 

Any decision taken, however, would mean that the newer v4l-dvb tree will
require more tests than it used to require, to be stable.

>  Hans and I have
> > coordinated such that we will not patch clash, and the i2c
> > conversions deal mostly with client modules...  any impact on bttv,
> > if any, will be localized in the i2c code.

bttv has several "hacks" for probing i2c audio devices. Depending on the
way Hans touched bttv, the conflicts will emerge.

> >  The pending bttv patch
> > probably needs updating anyway, due to changes upstream.
Yes. before hybrid redesign, however, the changes at bttv weren't much
significant. I'm counting with you both to help on solving the conflicts
that might emerge.

> I agree with Mike here. My i2c patches do not touch the tuner, nor 
> should they conflict with anything else. It does the initial conversion 
> of a bunch of i2c drivers and installs the framework. No v4l driver is 
> actually using it yet, ivtv will be the first to switch over.
If you didn't touch much on bttv, than it should be easier to solve the
conflicts.

> > The changes above hold priority over the two below.
> >
> > > - xc3028 old code removal;
> > > - tuner-xc2028 addition;
> >
> > Regardless, the xc2028 changes are unlikely to cause any conflict
> > with the other changes.
> >
> > Hans is waiting for the tuner-refactor-phase-2 tree to be merged
> > before he pushes in the i2c changes, and you should wait for those
> > both to be merged before dealing with xc2028, in my opinion.
> 
> Actually, xc2028 can go in before my i2c changes since these i2c changes 
> to not involve the tuner. They are not dependent on one another.

I'll probably try to do xc2028 and hybrid tuner changes about the same
time, since both deals with similar things.

Since I've replaced the CONFIG_TUNER_XC3028 by CONFIG_XC2028 on all
drivers [1], including ivtv, some minor conflicts might arrive. Since
ivtv xc3028 support is based at the userspace module, you will need to
fix ivtv driver later, for it to work with the newer driver.

[1] http://linuxtv.org/hg/~mchehab/tm6000-ng/rev/065874933156

With the new tuner-xc2028, the tuner will only work after receiving a
TUNER_SET_CONFIG, specifying the firmware driver name[2] and the audio
mode (RF or MTS).

[2] from what I got, it seems that different bridge chips may need
different firmwares. TM6000 driver, for example, doesn't work with
xc3028 version 2.7 firmware.

> > > Since those envolves several changes at core, it is likely that
> > > changesets will conflict.
> >
> > anything is possible, but i don't think it's likely :-)
> >
> > > So, I intend to carefully handle the 2.6.25 changesets already
> > > finished during this weekend, hopefully.
> >
> > OK.  I have more changes planned that depend on these... If I add
> > changesets to the tree, i'll send you addendums to my original pull
> > request.
> 
> From my perspective it would be great if the tuner and i2c changes would 
> go in on Saturday, then I can use Sunday to do the tuner i2c 
> conversion, deliver yet another i2c driver for the Mitsubishi M52790 
> A/V switch and add in ivtv patches to support two new boards. It's no 
> wonder ivtv suffers relatively often from i2c detection issues: it 
> supports no less than 15 different i2c devices.

I'll try to finish this on Sat, but I can't promise.

Cheers,
Mauro


___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [RFC] tuner-refactor-phase-2

2007-10-26 Thread Hans Verkuil
On Friday 26 October 2007 06:24, Michael Krufky wrote:
> Mauro Carvalho Chehab wrote:
> > Hi Michael,
> >
> > Em Seg, 2007-10-22 às 16:03 -0400, [EMAIL PROTECTED] escreveu:
> >> Mauro & others,
> >>
> >> After our conversation last week, I decided to move forward with
> >> tuner-refactor-phase-2, so that you can have the pathway for your
> >> tda9887 & tea5767 changes to go in without clashing with my
> >> pending work.
> >>
> >> My code is now ready for review:
> >>
> >> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-2
> >
> > I expect a few troubles on merging the newer patches for 2.6.25,
> > since there are several significant changes that are expected to
> > happen, during this development period, like:
> >
> > - the newer tuner-redesign changesets;
> > - i2c redesign;
> > - bttv removal of V4L1 support;
>
> ^^^ These above do not conflict with each other.  Hans and I have
> coordinated such that we will not patch clash, and the i2c
> conversions deal mostly with client modules...  any impact on bttv,
> if any, will be localized in the i2c code.  The pending bttv patch
> probably needs updating anyway, due to changes upstream.

I agree with Mike here. My i2c patches do not touch the tuner, nor 
should they conflict with anything else. It does the initial conversion 
of a bunch of i2c drivers and installs the framework. No v4l driver is 
actually using it yet, ivtv will be the first to switch over. I'm 
waiting with the tuner conversion for Mike's patches to go in first 
because that gives me a better testing baseline, not because I expect 
merge conflicts. For the same reason the xc2028 addition is unlikely to 
clash with my intended tuner i2c changes.

> The changes above hold priority over the two below.
>
> > - xc3028 old code removal;
> > - tuner-xc2028 addition;
>
> Regardless, the xc2028 changes are unlikely to cause any conflict
> with the other changes.
>
> Hans is waiting for the tuner-refactor-phase-2 tree to be merged
> before he pushes in the i2c changes, and you should wait for those
> both to be merged before dealing with xc2028, in my opinion.

Actually, xc2028 can go in before my i2c changes since these i2c changes 
to not involve the tuner. They are not dependent on one another.

> > Since those envolves several changes at core, it is likely that
> > changesets will conflict.
>
> anything is possible, but i don't think it's likely :-)
>
> > So, I intend to carefully handle the 2.6.25 changesets already
> > finished during this weekend, hopefully.
>
> OK.  I have more changes planned that depend on these... If I add
> changesets to the tree, i'll send you addendums to my original pull
> request.

From my perspective it would be great if the tuner and i2c changes would 
go in on Saturday, then I can use Sunday to do the tuner i2c 
conversion, deliver yet another i2c driver for the Mitsubishi M52790 
A/V switch and add in ivtv patches to support two new boards. It's no 
wonder ivtv suffers relatively often from i2c detection issues: it 
supports no less than 15 different i2c devices.

Regards,

Hans

___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [RFC] tuner-refactor-phase-2

2007-10-25 Thread Michael Krufky
Mauro Carvalho Chehab wrote:
> Hi Michael,
>
> Em Seg, 2007-10-22 às 16:03 -0400, [EMAIL PROTECTED] escreveu:
>   
>> Mauro & others,
>>
>> After our conversation last week, I decided to move forward with 
>> tuner-refactor-phase-2, so that you can have the pathway for your 
>> tda9887 & tea5767 changes to go in without clashing with my pending work.
>>
>> My code is now ready for review:
>>
>> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-2
>> 
>
> I expect a few troubles on merging the newer patches for 2.6.25, since
> there are several significant changes that are expected to happen,
> during this development period, like:
>
> - the newer tuner-redesign changesets;
> - i2c redesign;
> - bttv removal of V4L1 support;
>   
^^^ These above do not conflict with each other.  Hans and I have
coordinated such that we will not patch clash, and the i2c conversions
deal mostly with client modules...  any impact on bttv, if any, will be
localized in the i2c code.  The pending bttv patch probably needs
updating anyway, due to changes upstream.

The changes above hold priority over the two below.
> - xc3028 old code removal;
> - tuner-xc2028 addition;
>   
Regardless, the xc2028 changes are unlikely to cause any conflict with
the other changes.

Hans is waiting for the tuner-refactor-phase-2 tree to be merged before
he pushes in the i2c changes, and you should wait for those both to be
merged before dealing with xc2028, in my opinion.

> Since those envolves several changes at core, it is likely that
> changesets will conflict.
>
>   
anything is possible, but i don't think it's likely :-)
> So, I intend to carefully handle the 2.6.25 changesets already finished
> during this weekend, hopefully.
OK.  I have more changes planned that depend on these... If I add
changesets to the tree, i'll send you addendums to my original pull request.

Cheers,

Mike

___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [RFC] tuner-refactor-phase-2

2007-10-25 Thread Mauro Carvalho Chehab
Hi Michael,

Em Seg, 2007-10-22 às 16:03 -0400, [EMAIL PROTECTED] escreveu:
> Mauro & others,
> 
> After our conversation last week, I decided to move forward with 
> tuner-refactor-phase-2, so that you can have the pathway for your 
> tda9887 & tea5767 changes to go in without clashing with my pending work.
> 
> My code is now ready for review:
> 
> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-2

I expect a few troubles on merging the newer patches for 2.6.25, since
there are several significant changes that are expected to happen,
during this development period, like:

- the newer tuner-redesign changesets;
- i2c redesign;
- bttv removal of V4L1 support;
- xc3028 old code removal;
- tuner-xc2028 addition;

Since those envolves several changes at core, it is likely that
changesets will conflict.

So, I intend to carefully handle the 2.6.25 changesets already finished
during this weekend, hopefully.

-- 
Cheers,
Mauro


___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

Re: [linux-dvb] [RFC] tuner-refactor-phase-2

2007-10-22 Thread mkrufky
Hans Verkuil wrote:
> On Monday 22 October 2007 22:03:03 [EMAIL PROTECTED] wrote:
>   
>> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-2
> Hi Mike,
>
> Looks good!
>
>   
Hans,

Thank you for the review!
> Just some nitpick things:
>
> 1)
>
> tuner-core.c: things like:
>
> -   if (t->ops.tuner_status)
> -   t->ops.tuner_status(t);
> +   if ((ops) && (ops->tuner_status))
> +   ops->tuner_status(t);
>
> A bit too many parenthesis, 'if (ops && ops->tuner_status)' works just 
> as well and is more readable IMHO.
>   
Good point -- I've cleaned that up.
> 2)
>
> Please comment who IS responsible for freeing analog_demod_priv, 
> also 'kfree(t->fe.' should be 'kfree(fe->' to keep the comment in sync 
> with the code changes.
>
> +static void fe_release(struct dvb_frontend *fe)
> +{
> +   if (fe->ops.tuner_ops.release)
> +   fe->ops.tuner_ops.release(fe);
> +
> +   fe->ops.analog_demod_ops = NULL;
> +   /* DO NOT kfree(t->fe.analog_demod_priv) */
> +   fe->analog_demod_priv = NULL;
> +}
>   
Done.  I realize that this could be confusing.  This should be clearer 
with the new comments / better explanation.
> 3)
>
> Personally I don't see the need for changeset 6214 (clean up ops 
> checking in tuner_status function): I thought the original was easier 
> to read. Just remove the '{' '}' checkpatch complains about and it's 
> fine.
>   
I disagree with you here.  In reality, either way should be OK...  The 
way it is right now simplifies matters by checking (ops) once.  If it is 
NULL, then neither .has_signal nor .is_stereo will be checked.  I'd 
prefer to keep it this way.
> Reviewed-by: Hans Verkuil <[EMAIL PROTECTED]>
>   
Thanks.  I will add your reviewed-by tag to all changesets EXCEPT for 
#6214 as mentioned in (3).  I'm going to give it a few hours first, to 
see if anybody else has any comments.
> Regards,
>
>   Hans
>   

Thanks again for the review at such short notice.

Regards,

Mike Krufky



___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb


Re: [linux-dvb] [RFC] tuner-refactor-phase-2

2007-10-22 Thread Hans Verkuil
On Monday 22 October 2007 22:03:03 [EMAIL PROTECTED] wrote:
> Mauro & others,
>
> After our conversation last week, I decided to move forward with
> tuner-refactor-phase-2, so that you can have the pathway for your
> tda9887 & tea5767 changes to go in without clashing with my pending
> work.
>
> My code is now ready for review:
>
> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-2
>
> - Move all tda8275/8275a tuning code from tda8290 module into tda827x
> module - tda827x: fix GPL export on attach function
> - tda8290: add support for NXP TDA18271 tuner and TDA8295 analog
> demod - tuner: move analog_tuner_ops into dvb_frontend_ops
> - tuner: clear analog_demod_ops on release
> - tuner: move analog_demod_priv into struct dvb_frontend
> - dvb_frontend: codingstyle cleanups
> - tuner: convert analog tuner demod sub-modules to dvb_frontend
> interface - tuner: clean up ops checking in tuner_status function
> - move std if setting from tda8290 to tda827x
> - make tda9887 build selectable via Kconfig
>
>  b/linux/drivers/media/dvb/frontends/tda18271.c  | 1062 
>  b/linux/drivers/media/dvb/frontends/tda18271.h  |   40
>  b/linux/drivers/media/video/tda9887.h   |   33
>  linux/Documentation/video4linux/CARDLIST.tuner  |1
>  linux/drivers/media/Kconfig |   14
>  linux/drivers/media/dvb/dvb-core/dvb_frontend.h |   22
>  linux/drivers/media/dvb/frontends/Kconfig   |7
>  linux/drivers/media/dvb/frontends/Makefile  |1
>  linux/drivers/media/dvb/frontends/tda827x.c |  370 ++
>  linux/drivers/media/dvb/frontends/tda827x.h |   13
>  linux/drivers/media/video/Makefile  |3
>  linux/drivers/media/video/tda8290.c | 1297 --
>  linux/drivers/media/video/tda8290.h |   40
>  linux/drivers/media/video/tda9887.c |  161 -
>  linux/drivers/media/video/tuner-core.c  |  248 +
>  linux/drivers/media/video/tuner-driver.h|   25
>  linux/drivers/media/video/tuner-types.c |3
>  linux/drivers/media/video/tveeprom.c|2
>  linux/include/media/tuner.h |2
>  v4l/versions.txt|2
>  20 files changed, 2424 insertions(+), 922 deletions(-)
>
> The four major changes are:
>
> 1) move all tda827x tuning code from tda8290.c into tda827x.c
>
> 2) addition of tda8295 + tda18271 tuner + analog demod combo support.
>
> 3) conversion of tda9887 to a separate module
>
> 4) addition of analog_demod_ops & analog_demod_priv to struct
> dvb_frontend
>
> After this is merged, the analog demods will be accessible via the
> dvb_frontend interface.  For the sake of simplicity, I've kept the
> analog_tuner_ops untouched, and using this structure for the
> demodulators within the dvb_frontend_ops structures.  We can improve
> on this in the future, if necessary.
>
> I've implemented this as a forward reference, so we can make any
> changes to the analog_tuner_ops structure as we see fit, without
> having any impact on dvb-only users of the dvb_frontend.
>
> This started off as a private email, but I believe that I've covered
> all bases.  Since I tend to be lazy with emails in general, I am just
> going to cc the mailing lists and consider this an RFC.
>
> I've taken into account the concerns mentioned in the phase-1 RFC
> thread -- I believe that all will be happy with the way that I've
> done this.
>
> Please let me know if you have any questions or comments.
>
> Cheers,
>
> Mike Krufky

Hi Mike,

Looks good!

Just some nitpick things:

1)

tuner-core.c: things like:

-   if (t->ops.tuner_status)
-   t->ops.tuner_status(t);
+   if ((ops) && (ops->tuner_status))
+   ops->tuner_status(t);

A bit too many parenthesis, 'if (ops && ops->tuner_status)' works just 
as well and is more readable IMHO.

2)

Please comment who IS responsible for freeing analog_demod_priv; 
also 'kfree(t->fe.' should be 'kfree(fe->' to keep the comment in sync 
with the code changes.

+static void fe_release(struct dvb_frontend *fe)
+{
+   if (fe->ops.tuner_ops.release)
+   fe->ops.tuner_ops.release(fe);
+
+   fe->ops.analog_demod_ops = NULL;
+   /* DO NOT kfree(t->fe.analog_demod_priv) */
+   fe->analog_demod_priv = NULL;
+}

3)

Personally I don't see the need for changeset 6214 (clean up ops  
checking in tuner_status function): I thought the original was easier 
to read. Just remove the '{' '}' checkpatch complains about and it's 
fine.

Reviewed-by: Hans Verkuil <[EMAIL PROTECTED]>

Regards,

Hans

___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb