Message-Id: <1178200976.12651.104.camel@localhost>
Date:	Thu, 03 May 2007 11:02:56 -0300
From:	Mauro Carvalho Chehab <mchehab@...radead.org>
To:	Trent Piepho <xyzzy@...akeasy.org>
Cc:	Markus Rechberger <mrechberger@...il.com>, linux-dvb@...uxtv.org,
	linux-kernel@...r.kernel.org, Manu Abraham <abraham.manu@...il.com>
Subject: Re: [linux-dvb] Re: DST/BT878 module customization (..
	was:	Critical points about ...)

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()". 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@...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