Message-ID: <20070503153602.268100@gmx.net>
Date:	Thu, 03 May 2007 17:36:02 +0200
From:	"Uwe Bugla" <uwe.bugla@....de>
To:	Manu Abraham <abraham.manu@...il.com>, mchehab@...radead.org
Cc:	linux-kernel@...r.kernel.org, xyzzy@...akeasy.org,
	linux-dvb@...uxtv.org
Subject: Re: [linux-dvb] Re: DST/BT878 module customization (..	was:	Critical
	points about ...)


-------- Original-Nachricht --------
Datum: Thu, 03 May 2007 19:15:32 +0400
Von: Manu Abraham <abraham.manu@...il.com>
An: Mauro Carvalho Chehab <mchehab@...radead.org>
CC: linux-dvb@...uxtv.org, Trent Piepho <xyzzy@...akeasy.org>, linux-kernel@...r.kernel.org
Betreff: Re: [linux-dvb] Re: DST/BT878 module customization (..	was:	Critical	points about ...)

> Mauro Carvalho Chehab wrote:
> > Em Qua, 2007-05-02 às 04:10 -0700, Trent Piepho escreveu:
> >> On Tue, 1 May 2007, Mauro Carvalho Chehab wrote:
> > 
> >>> However, when dst is selected, I got those errors:
> >> When I made this patch I was basing it off a patch I made around 9
> months
> >> ago.  I thought since that one was ok, this would be ok too, and in my
> >> hurry to get it done I've clearly put too little effort into checking
> it,
> >> and I'm sorry to have wasted your time.
> >>
> >> I promise, this time it's right!
> >> http://linuxtv.org/hg/~tap/dst-new
> > 
> > Confirmed. Now the patch is properly working. My tests were done with a
> > board with DST. Those are the results:
> > 
> > 1) when DST is unselected, on a board with DST, it will print the errors
> > indicating that the Kconfig items were not selected:
> > 
> > DVB: registering new adapter (bttv0).
> > DVB: Unable to find symbol dst_attach()
> > frontend_init: Could not find a Twinhan DST.
> > dvb-bt8xx: A frontend driver was not found for device 109e/0878
> subsystem fbfb/f800
> > 
> > The only issue is the wrong printk msg, stating that a "frontend driver"
> > were not found. As this issue also happens with the current driver, due
> > the usage of dvb_attach() macro, I don't see any regressions.
> > 
> > It would be nice, however, to have a patch making dvb_attach more
> > generic, by e.g. having a variant that allows passing another message.
> > 
> > Trying to run dvb programs like scan and kaffeine will properly fail.
> > 
> > 2) With DST selected, the driver works properly. 
> > 
> > The changes also solved the issue of removing the dst drivers. Before
> > the patch, sometimes it is required to force removal of dst module with
> > the '-f' flag, since the module count were wrong.
> > 
> > ---
> > 
> > I'll risk to make a briefing of the technical points:
> > 
> > <SUMMARY>
> > Current issues:
> > 	1) allow deselecting DST/DST_CA support, since this is not needed by
> > some supported boards;
> > 	2) sometimes, module removal don't work with dst, since usage count
> > becomes wrong;
> > 	3) if dst or dst_ca is not properly loaded, the printk message wrongly
> > indicates that "A frontend driver was not found"
> > 
> > The Trent's original patch were written on Aug, 2006 (*). The current
> > version fixes (1) and (2). It doesn't touch on (3). There aren't any
> > other patches properly fixing (1) and (2).
> > 
> > There's an argument against the prototype changes on dst_attach and
> > dst_ca_attach since they aren't frontend.
> > </SUMMARY>
> > 
> > (*) Notice: I can't remember why the patches were not applied on that
> > time. I think somebody nacked they, although I didn't found any record
> > about the patch on my mailbox.
> > 
> > About the usage of frontend support for a non-frontend module, I really
> > can't see any issues at the current implementation, since even before
> > the patch, all DST initialization are done by a routine called
> > "frontend_init()". 
> 
> *Wrong*.  Frontend Init sends a command to the whole system to
> initialize the frontend, Not initialize the DST
> Whatever frontend naming conventions are in there are a relic from
> Andrew's FE_REFACTORING changesets.
> 
> Even dst not being a frontend, the initialization
> > gets the result of the attachment process, using it as a "frontend":
> > 
> > 	state = kmalloc(sizeof (struct dst_state), GFP_KERNEL);
> > 
> >         state->config = &dst_config;
> >         state->i2c = card->i2c_adapter;
> >         state->bt = card->bt;
> >         state->dst_ca = NULL;
> > 
> >         dvb_attach(dst_attach, state, &card->dvb_adapter);
> > 
> >         card->fe = &state->frontend;
> > 
> > 	(comments and error checks removed to make code cleaner)
> > 
> > The patch basically moved state initialization to dst_attach. The above
> > code, after the patch is:
> > 	
> > 	card->fe = dvb_attach(dst_attach, &dst_config, card->bt,
> card->i2c_adapter);
> > 
> > So, I didn't noticed any regressions by running the modified driver nor
> > I couldn't identify any regressions by inspecting the source code. 
> > 
> > The end result seems also cleaner and easier to understand, preserving
> > all characteristics of the original code. Also, it uses dvb_attach the
> > same way as the other dvb helper modules.
> > 
> > If later any changes will be needed to allow using DST on different
> > configurations, future patches probably will need to move dst
> > initialization to other places, and use different callbacks for it,
> > altering the above initialization. I can't see why those changes would
> > possibly avoid any further changes at the driver.
> > 
> > That's said, I intend to commit the two patches, fixing the current
> > issues, at this weekend.
> > 
> 
> I did explain my stand in a previous mail.
> 
> NACK

Technically may be OK, psychologically this decision is desasterous. Why don't you learn, for god's sake?
> 
> 
> _______________________________________________
> linux-dvb mailing list
> linux-dvb@...uxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb

-- 
"Feel free" - 10 GB Mailbox, 100 FreeSMS/Monat ...
Jetzt GMX TopMail testen: http://www.gmx.net/de/go/topmail
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists