Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161917AbXECOFE (ORCPT ); Thu, 3 May 2007 10:05:04 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1161948AbXECOFE (ORCPT ); Thu, 3 May 2007 10:05:04 -0400 Received: from pentafluge.infradead.org ([213.146.154.40]:42038 "EHLO pentafluge.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161917AbXECOFA (ORCPT ); Thu, 3 May 2007 10:05:00 -0400 Subject: Re: [linux-dvb] Re: DST/BT878 module customization (.. was: Critical points about ...) From: Mauro Carvalho Chehab To: Trent Piepho Cc: Markus Rechberger , linux-dvb@linuxtv.org, linux-kernel@vger.kernel.org, Manu Abraham In-Reply-To: References: <1178031333.3958.63.camel@localhost> <1178071431.11109.62.camel@localhost> Content-Type: text/plain; charset=utf-8 Date: Thu, 03 May 2007 11:02:56 -0300 Message-Id: <1178200976.12651.104.camel@localhost> Mime-Version: 1.0 X-Mailer: Evolution 2.10.0-5mdv2007.1 Content-Transfer-Encoding: 8bit X-SRS-Rewrite: SMTP reverse-path rewritten from by pentafluge.infradead.org See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4416 Lines: 115 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:

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.

(*) 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()". 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'm open to other technical reviews of the patches. -- Cheers, Mauro - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/