diff mbox

PULL request - http://linuxtv.org/hg/~pb/v4l-dvb/

Message ID Pine.LNX.4.58.0905271825120.32713@shell2.speakeasy.net (mailing list archive)
State RFC
Headers show

Commit Message

Trent Piepho May 28, 2009, 2:14 a.m. UTC
On Tue, 26 May 2009, Patrick Boettcher wrote:
> > MHz to 161 Mhz as welll as 439-447 MHz.  Probably wrong.  My guess is the
> > data sheet says, "low band 48 to 154 MHz, mid band 161 MHz to 439 MHz,
> > etc.," where 154 is the frequency of the last channel in the low band and
> > 161 is the first channel in the mid band.  Then someone translated this to
> > code without really understanding what's going on.  It should probably be:
> >
> > 	if      (params->frequency > 443000000) bs = 0x08;
> > 	else if (params->frequency > 157500000) bs = 0x0a;
> > 	else					bs = 0x09;
> >
> > The tuner's limits should already be enforced elsewhere.  Or just convert
> > this to use dvb_pll.
>
> Thanks for pointing this out, can you please provide a patch for that?
> Preferably for the dvb_pll-solution?

I've done this, but found some more mistakes in the flexcop code wrt
frontend attachment.

In patch 7469 you changed a failure path "dvb_frontend_detach(fc->fe)" into
a "fc->fe->ops->release(fc->fe)", which isn't correct.  There is more stuff
dvb_frontend_detach() does besides call the main release method.

The various attach functions in flexcop-fe-tuner don't return success/fail,
which is a problem when frontend attachment partially fails.  For instance
if mt352 attachment works but dvb-pll attachment then fails the driver
will think everything is ok because fc->fe is non-NULL, but the tuner will
not work.  It makes more sense to consider this an error and clean up.

There are a couple other places where frontend attachment can partially
fail where I wasn't entirely sure what's right.  In
skystar2_rev27_attach(), after the s5h1420 demod is attached, attachment of
a ISL6421 LNB and ITD1000 tuner is attempted.  If these fail an error
message is printed but the rest of the code will consider the frontend to
be successfully attached.  Can the frontend work if the ISL6421 or ITD1000
didn't attach (which can happen if the driver isn't present even if the
hardware is fine)?  I'm guessing not and called these cases an error.  If
it's not an error, then the err() printout should probably be warning or
info level.

Does this patch to fix these problems look ok?

Maybe change "if (!i2c_tuner) return 0;" into "BUG_ON(!i2c_tuner);"?

In the case of a partial frontend attachment failure, should we keep
trying the rest of the frontends in the list or fail out early?  This patch
does the former, but the latter could easily be done by adding a "break" to
the end of the "Clean up partially attached frontend" code block.


# HG changeset patch
# User Trent Piepho <xyzzy@speakeasy.org>
# Date 1243476805 25200
# Node ID 27aa25fe54ae2fd9a166bfdb3cec4c4ecadc9cba
# Parent  480905f8768ce2e5b01da1c6d723cf95cf134d0b
b2c2: Fix problems with frontend attachment

From: Trent Piepho <xyzzy@speakeasy.org>

The frontend attachment code didn't handle cases where the frontend
partially failed to attach.  For instance, when the demod was attached
successfully but the tuner driver wasn't compiled or fails to init for some
reason.  In these cases we try to clean up the partial attachment and fail
instead of proceeding with a broken frontend.

If frontend registration fails, clean up with dvb_frontend_detach() rather
than just calling the frontend's main release method.  The former does some
additional stuff, like release an attached tuner and take care of putting
symbols when dynamic binding is used.

In skystar2_rev23_attach() it's not necessary to set fc->dev_type, that
gets set before skystar2_rev23_attach() is called.

Priority: normal

Signed-off-by: Trent Piepho <xyzzy@speakeasy.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Patrick Boettcher June 11, 2009, 8:36 a.m. UTC | #1
Hi Trent,

On Wed, 27 May 2009, Trent Piepho wrote:

> On Tue, 26 May 2009, Patrick Boettcher wrote:
>>> MHz to 161 Mhz as welll as 439-447 MHz.  Probably wrong.  My guess is the
>>> data sheet says, "low band 48 to 154 MHz, mid band 161 MHz to 439 MHz,
>>> etc.," where 154 is the frequency of the last channel in the low band and
>>> 161 is the first channel in the mid band.  Then someone translated this to
>>> code without really understanding what's going on.  It should probably be:
>>>
>>> 	if      (params->frequency > 443000000) bs = 0x08;
>>> 	else if (params->frequency > 157500000) bs = 0x0a;
>>> 	else					bs = 0x09;
>>>
>>> The tuner's limits should already be enforced elsewhere.  Or just convert
>>> this to use dvb_pll.
>>
>> Thanks for pointing this out, can you please provide a patch for that?
>> Preferably for the dvb_pll-solution?
>
> I've done this, but found some more mistakes in the flexcop code wrt
> frontend attachment.
>
> In patch 7469 you changed a failure path "dvb_frontend_detach(fc->fe)" into
> a "fc->fe->ops->release(fc->fe)", which isn't correct.  There is more stuff
> dvb_frontend_detach() does besides call the main release method.
>
> The various attach functions in flexcop-fe-tuner don't return success/fail,
> which is a problem when frontend attachment partially fails.  For instance
> if mt352 attachment works but dvb-pll attachment then fails the driver
> will think everything is ok because fc->fe is non-NULL, but the tuner will
> not work.  It makes more sense to consider this an error and clean up.
>
> There are a couple other places where frontend attachment can partially
> fail where I wasn't entirely sure what's right.  In
> skystar2_rev27_attach(), after the s5h1420 demod is attached, attachment of
> a ISL6421 LNB and ITD1000 tuner is attempted.  If these fail an error
> message is printed but the rest of the code will consider the frontend to
> be successfully attached.  Can the frontend work if the ISL6421 or ITD1000
> didn't attach (which can happen if the driver isn't present even if the
> hardware is fine)?  I'm guessing not and called these cases an error.  If
> it's not an error, then the err() printout should probably be warning or
> info level.
>
> Does this patch to fix these problems look ok?

In fact, everything looks correct in my eyes. I'll ask Mauro to pull any 
minute from now.

I even have an explaination why the failing attach of the itd1000 was 
not errored out: when I did the development I was using a userspace 
proprietary driver to validate, for that I needed the demod to be 
attached...

Thanks for cleaning up this mess.

best regards,

Patrick.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trent Piepho June 11, 2009, 10:35 p.m. UTC | #2
On Thu, 11 Jun 2009, Patrick Boettcher wrote:
> On Wed, 27 May 2009, Trent Piepho wrote:
> > On Tue, 26 May 2009, Patrick Boettcher wrote:
> > Does this patch to fix these problems look ok?
>
> In fact, everything looks correct in my eyes. I'll ask Mauro to pull any
> minute from now.
>
> I even have an explaination why the failing attach of the itd1000 was
> not errored out: when I did the development I was using a userspace
> proprietary driver to validate, for that I needed the demod to be
> attached...
>
> Thanks for cleaning up this mess.

Now that that patch is done, here is the rest of the series with dvb-pll
conversions.  There is an additional patch to the flexcop card detecting
code.  The #if defined(CONFIG_...) stuff didn't take into account code
being compiled into the kernel.

Here's the series:

01/08: dvb-pll: Add Samsung TDTC9251DH0 DVB-T NIM
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=3d148fee550b

02/08: dvb-pll: Add support for Samsung TBDU18132 DVB-S NIM
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=5a8e32e94aa7

03/08: dvb-pll: Add support for Samsung TBMU24112 DVB-S NIM
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c28394056a5a

04/08: dvb-pll: Add support for Alps TDEE4 DVB-C NIM
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=90f54e498f90

05/08: b2c2: fix frontends compiled into kernel
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=748c762fcf3e

06/08: b2c2: Use dvb-pll for AirStar DVB-T's tuner
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=fcacc65753f1

07/08: b2c2: Use dvb-pll for Skystar2 rev 2.3 and rev 2.6
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=8ef37f1432e6

08/08: b2c2: Use dvb-pll for Cablestar2
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=01fd4e3bd1c0


 b2c2/flexcop-fe-tuner.c |  283 +++++++++++++++---------------------------------
 frontends/dvb-pll.c     |   75 ++++++++++++
 frontends/dvb-pll.h     |    4
 3 files changed, 172 insertions(+), 190 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab June 15, 2009, 12:36 p.m. UTC | #3
Em Thu, 11 Jun 2009 15:35:14 -0700 (PDT)
Trent Piepho <xyzzy@speakeasy.org> escreveu:

> On Thu, 11 Jun 2009, Patrick Boettcher wrote:
> > On Wed, 27 May 2009, Trent Piepho wrote:
> > > On Tue, 26 May 2009, Patrick Boettcher wrote:
> > > Does this patch to fix these problems look ok?
> >
> > In fact, everything looks correct in my eyes. I'll ask Mauro to pull any
> > minute from now.
> >
> > I even have an explaination why the failing attach of the itd1000 was
> > not errored out: when I did the development I was using a userspace
> > proprietary driver to validate, for that I needed the demod to be
> > attached...
> >
> > Thanks for cleaning up this mess.
> 
> Now that that patch is done, here is the rest of the series with dvb-pll
> conversions.  There is an additional patch to the flexcop card detecting
> code.  The #if defined(CONFIG_...) stuff didn't take into account code
> being compiled into the kernel.
> 
> Here's the series:
> 
> 01/08: dvb-pll: Add Samsung TDTC9251DH0 DVB-T NIM
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=3d148fee550b
> 
> 02/08: dvb-pll: Add support for Samsung TBDU18132 DVB-S NIM
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=5a8e32e94aa7
> 
> 03/08: dvb-pll: Add support for Samsung TBMU24112 DVB-S NIM
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c28394056a5a
> 
> 04/08: dvb-pll: Add support for Alps TDEE4 DVB-C NIM
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=90f54e498f90
> 
> 05/08: b2c2: fix frontends compiled into kernel
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=748c762fcf3e
> 
> 06/08: b2c2: Use dvb-pll for AirStar DVB-T's tuner
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=fcacc65753f1
> 
> 07/08: b2c2: Use dvb-pll for Skystar2 rev 2.3 and rev 2.6
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=8ef37f1432e6
> 
> 08/08: b2c2: Use dvb-pll for Cablestar2
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=01fd4e3bd1c0

Nice patches. Are those tested with real cards?

In particular, I loved this:

+/* Can we use the specified front-end?  Remember that if we are compiled
+ * into the kernel we can't call code that's in modules.  */
+#define FE_SUPPORTED(fe) (defined(CONFIG_DVB_##fe) || \
+       (defined(CONFIG_DVB_##fe##_MODULE) && defined(MODULE)))

IMO, you should add it on a dvb core header. Then, a cleanup patch would be to
simplify all those frontend tests all over all the dvb drivers code.

Patrick, 

I think you have some skystar boards. I'd appreciate if you can review
those patches before applying it.



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Patrick Boettcher June 15, 2009, 12:56 p.m. UTC | #4
Hi Mauro,

On Mon, 15 Jun 2009, Mauro Carvalho Chehab wrote:

> Em Thu, 11 Jun 2009 15:35:14 -0700 (PDT)
> Trent Piepho <xyzzy@speakeasy.org> escreveu:
>
>> On Thu, 11 Jun 2009, Patrick Boettcher wrote:
>>> On Wed, 27 May 2009, Trent Piepho wrote:
>>>> On Tue, 26 May 2009, Patrick Boettcher wrote:
>>>> Does this patch to fix these problems look ok?
>>>
>>> In fact, everything looks correct in my eyes. I'll ask Mauro to pull any
>>> minute from now.
>>>
>>> I even have an explaination why the failing attach of the itd1000 was
>>> not errored out: when I did the development I was using a userspace
>>> proprietary driver to validate, for that I needed the demod to be
>>> attached...
>>>
>>> Thanks for cleaning up this mess.
>>
>> Now that that patch is done, here is the rest of the series with dvb-pll
>> conversions.  There is an additional patch to the flexcop card detecting
>> code.  The #if defined(CONFIG_...) stuff didn't take into account code
>> being compiled into the kernel.
>>
>> Here's the series:
>>
>> 01/08: dvb-pll: Add Samsung TDTC9251DH0 DVB-T NIM
>> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=3d148fee550b
>>
>> 02/08: dvb-pll: Add support for Samsung TBDU18132 DVB-S NIM
>> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=5a8e32e94aa7
>>
>> 03/08: dvb-pll: Add support for Samsung TBMU24112 DVB-S NIM
>> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c28394056a5a
>>
>> 04/08: dvb-pll: Add support for Alps TDEE4 DVB-C NIM
>> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=90f54e498f90
>>
>> 05/08: b2c2: fix frontends compiled into kernel
>> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=748c762fcf3e
>>
>> 06/08: b2c2: Use dvb-pll for AirStar DVB-T's tuner
>> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=fcacc65753f1
>>
>> 07/08: b2c2: Use dvb-pll for Skystar2 rev 2.3 and rev 2.6
>> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=8ef37f1432e6
>>
>> 08/08: b2c2: Use dvb-pll for Cablestar2
>> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=01fd4e3bd1c0
>
> Nice patches. Are those tested with real cards?
>
> In particular, I loved this:
>
> +/* Can we use the specified front-end?  Remember that if we are compiled
> + * into the kernel we can't call code that's in modules.  */
> +#define FE_SUPPORTED(fe) (defined(CONFIG_DVB_##fe) || \
> +       (defined(CONFIG_DVB_##fe##_MODULE) && defined(MODULE)))
>
> IMO, you should add it on a dvb core header. Then, a cleanup patch would be to
> simplify all those frontend tests all over all the dvb drivers code.
>
> Patrick,
>
> I think you have some skystar boards. I'd appreciate if you can review
> those patches before applying it.

The only card I use which is using DVB_PLL (now) is the Airstar DVB-T. So 
we need to take some risks and apply to force users to test :). I will ask 
some users I know.

However I won't be able to commit nor test anything before 3.5 weeks 
from now.

regards,
Patrick.

--
   Mail: patrick.boettcher@desy.de
   WWW:  http://www.wi-bw.tfh-wildau.de/~pboettch/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab June 16, 2009, 4:42 p.m. UTC | #5
Em Mon, 15 Jun 2009 14:56:39 +0200 (CEST)
Patrick Boettcher <patrick.boettcher@desy.de> escreveu:

> Hi Mauro,
> 
> On Mon, 15 Jun 2009, Mauro Carvalho Chehab wrote:
> 
> > Em Thu, 11 Jun 2009 15:35:14 -0700 (PDT)
> > Trent Piepho <xyzzy@speakeasy.org> escreveu:
> >
> >> On Thu, 11 Jun 2009, Patrick Boettcher wrote:
> >>> On Wed, 27 May 2009, Trent Piepho wrote:
> >>>> On Tue, 26 May 2009, Patrick Boettcher wrote:
> >>>> Does this patch to fix these problems look ok?
> >>>
> >>> In fact, everything looks correct in my eyes. I'll ask Mauro to pull any
> >>> minute from now.
> >>>
> >>> I even have an explaination why the failing attach of the itd1000 was
> >>> not errored out: when I did the development I was using a userspace
> >>> proprietary driver to validate, for that I needed the demod to be
> >>> attached...
> >>>
> >>> Thanks for cleaning up this mess.
> >>
> >> Now that that patch is done, here is the rest of the series with dvb-pll
> >> conversions.  There is an additional patch to the flexcop card detecting
> >> code.  The #if defined(CONFIG_...) stuff didn't take into account code
> >> being compiled into the kernel.
> >>
> >> Here's the series:
> >>
> >> 01/08: dvb-pll: Add Samsung TDTC9251DH0 DVB-T NIM
> >> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=3d148fee550b
> >>
> >> 02/08: dvb-pll: Add support for Samsung TBDU18132 DVB-S NIM
> >> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=5a8e32e94aa7
> >>
> >> 03/08: dvb-pll: Add support for Samsung TBMU24112 DVB-S NIM
> >> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c28394056a5a
> >>
> >> 04/08: dvb-pll: Add support for Alps TDEE4 DVB-C NIM
> >> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=90f54e498f90
> >>
> >> 05/08: b2c2: fix frontends compiled into kernel
> >> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=748c762fcf3e
> >>
> >> 06/08: b2c2: Use dvb-pll for AirStar DVB-T's tuner
> >> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=fcacc65753f1
> >>
> >> 07/08: b2c2: Use dvb-pll for Skystar2 rev 2.3 and rev 2.6
> >> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=8ef37f1432e6
> >>
> >> 08/08: b2c2: Use dvb-pll for Cablestar2
> >> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=01fd4e3bd1c0
> >
> > Nice patches. Are those tested with real cards?
> >
> > In particular, I loved this:
> >
> > +/* Can we use the specified front-end?  Remember that if we are compiled
> > + * into the kernel we can't call code that's in modules.  */
> > +#define FE_SUPPORTED(fe) (defined(CONFIG_DVB_##fe) || \
> > +       (defined(CONFIG_DVB_##fe##_MODULE) && defined(MODULE)))
> >
> > IMO, you should add it on a dvb core header. Then, a cleanup patch would be to
> > simplify all those frontend tests all over all the dvb drivers code.
> >
> > Patrick,
> >
> > I think you have some skystar boards. I'd appreciate if you can review
> > those patches before applying it.
> 
> The only card I use which is using DVB_PLL (now) is the Airstar DVB-T. So 
> we need to take some risks and apply to force users to test :). I will ask 
> some users I know.
> 
> However I won't be able to commit nor test anything before 3.5 weeks 
> from now.

Hmm... this will be outside the merge window... I think I'll merge it on our
tree after sending the first pull request for 2.6.30 (probably later today), in
order to cook it for a while at the main tree before merging it.

It would be nice if you can ask those users to test it asap, in order to get it
tested during this week. Otherwise, we'll need to postpone this to 2.6.31.
> 
> regards,
> Patrick.
> 
> --
>    Mail: patrick.boettcher@desy.de
>    WWW:  http://www.wi-bw.tfh-wildau.de/~pboettch/




Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff -r 480905f8768c -r 27aa25fe54ae linux/drivers/media/dvb/b2c2/flexcop-fe-tuner.c
--- a/linux/drivers/media/dvb/b2c2/flexcop-fe-tuner.c	Wed May 27 17:35:06 2009 -0700
+++ b/linux/drivers/media/dvb/b2c2/flexcop-fe-tuner.c	Wed May 27 19:13:25 2009 -0700
@@ -175,23 +175,23 @@  static int skystar23_samsung_tbdu18132_t
 	return 0;
 }

-static void skystar2_rev23_attach(struct flexcop_device *fc,
-	struct i2c_adapter *i2c)
-{
-	fc->fe = dvb_attach(mt312_attach,
-			&skystar23_samsung_tbdu18132_config, i2c);
+static int skystar2_rev23_attach(struct flexcop_device *fc,
+	struct i2c_adapter *i2c)
+{
+	fc->fe = dvb_attach(mt312_attach, &skystar23_samsung_tbdu18132_config, i2c);
 	if (fc->fe != NULL) {
 		struct dvb_frontend_ops *ops = &fc->fe->ops;
-		ops->tuner_ops.set_params \
-			= skystar23_samsung_tbdu18132_tuner_set_params;
+		ops->tuner_ops.set_params   =
+			skystar23_samsung_tbdu18132_tuner_set_params;
 		ops->diseqc_send_master_cmd = flexcop_diseqc_send_master_cmd;
 		ops->diseqc_send_burst      = flexcop_diseqc_send_burst;
 		ops->set_tone               = flexcop_set_tone;
 		ops->set_voltage            = flexcop_set_voltage;
 		fc->fe_sleep                = ops->sleep;
 		ops->sleep                  = flexcop_sleep;
-		fc->dev_type                = FC_SKY_REV23;
-	}
+		return 1;
+	}
+	return 0;
 }
 #endif

@@ -307,7 +307,7 @@  static struct stv0299_config samsung_tbm
 	.set_symbol_rate = samsung_tbmu24112_set_symbol_rate,
 };

-static void skystar2_rev26_attach(struct flexcop_device *fc,
+static int skystar2_rev26_attach(struct flexcop_device *fc,
 	struct i2c_adapter *i2c)
 {
 	fc->fe = dvb_attach(stv0299_attach, &samsung_tbmu24112_config, i2c);
@@ -317,7 +317,9 @@  static void skystar2_rev26_attach(struct
 		ops->set_voltage = flexcop_set_voltage;
 		fc->fe_sleep = ops->sleep;
 		ops->sleep = flexcop_sleep;
-	}
+		return 1;
+	}
+	return 0;
 }
 #endif

@@ -334,43 +336,54 @@  static struct itd1000_config skystar2_re
 	.i2c_address = 0x61,
 };

-static void skystar2_rev27_attach(struct flexcop_device *fc,
-	struct i2c_adapter *i2c)
-{
+static int skystar2_rev27_attach(struct flexcop_device *fc,
+	struct i2c_adapter *i2c)
+{
+	flexcop_ibi_value r108;
+	struct i2c_adapter *i2c_tuner;
+
 	/* enable no_base_addr - no repeated start when reading */
 	fc->fc_i2c_adap[0].no_base_addr = 1;
-	fc->fe = dvb_attach(s5h1420_attach,
-		&skystar2_rev2_7_s5h1420_config, i2c);
-	if (fc->fe != NULL) {
-		flexcop_ibi_value r108;
-		struct i2c_adapter *i2c_tuner \
-		= s5h1420_get_tuner_i2c_adapter(fc->fe);
-		struct dvb_frontend_ops *ops = &fc->fe->ops;
-
-		fc->fe_sleep = ops->sleep;
-		ops->sleep   = flexcop_sleep;
-
-		/* enable no_base_addr - no repeated start when reading */
-		fc->fc_i2c_adap[2].no_base_addr = 1;
-		if (dvb_attach(isl6421_attach, fc->fe,
-			&fc->fc_i2c_adap[2].i2c_adap, 0x08, 1, 1) == NULL)
-			err("ISL6421 could NOT be attached");
-		else
-			info("ISL6421 successfully attached");
-
-		/* the ITD1000 requires a lower i2c clock - is it a problem ? */
-		r108.raw = 0x00000506;
-		fc->write_ibi_reg(fc, tw_sm_c_108, r108);
-		if (i2c_tuner) {
-			if (dvb_attach(itd1000_attach, fc->fe, i2c_tuner,
-				&skystar2_rev2_7_itd1000_config) == NULL)
-				err("ITD1000 could NOT be attached");
-			else
-				info("ITD1000 successfully attached");
-		}
-	} else
-		fc->fc_i2c_adap[0].no_base_addr = 0;
-		/* for the next devices we need it again */
+	fc->fe = dvb_attach(s5h1420_attach, &skystar2_rev2_7_s5h1420_config,
+			    i2c);
+	if (!fc->fe)
+		goto fail;
+
+	i2c_tuner = s5h1420_get_tuner_i2c_adapter(fc->fe);
+	if (!i2c_tuner)
+		goto fail;
+
+	fc->fe_sleep = fc->fe->ops.sleep;
+	fc->fe->ops.sleep = flexcop_sleep;
+
+	/* enable no_base_addr - no repeated start when reading */
+	fc->fc_i2c_adap[2].no_base_addr = 1;
+	if (!dvb_attach(isl6421_attach, fc->fe, &fc->fc_i2c_adap[2].i2c_adap,
+			0x08, 1, 1)) {
+		err("ISL6421 could NOT be attached");
+		goto fail_isl;
+	}
+	info("ISL6421 successfully attached");
+
+	/* the ITD1000 requires a lower i2c clock - is it a problem ? */
+	r108.raw = 0x00000506;
+	fc->write_ibi_reg(fc, tw_sm_c_108, r108);
+	if (!dvb_attach(itd1000_attach, fc->fe, i2c_tuner,
+			&skystar2_rev2_7_itd1000_config)) {
+		err("ITD1000 could NOT be attached");
+		/* Should i2c clock be restored? */
+		goto fail_isl;
+	}
+	info("ITD1000 successfully attached");
+
+	return 1;
+
+fail_isl:
+	fc->fc_i2c_adap[2].no_base_addr = 0;
+fail:
+	/* for the next devices we need it again */
+	fc->fc_i2c_adap[0].no_base_addr = 0;
+	return 0;
 }
 #endif

@@ -387,32 +400,38 @@  static const struct cx24113_config skyst
 	.xtal_khz = 10111,
 };

-static void skystar2_rev28_attach(struct flexcop_device *fc,
-	struct i2c_adapter *i2c)
-{
-	fc->fe = dvb_attach(cx24123_attach,
-			&skystar2_rev2_8_cx24123_config, i2c);
-	if (fc->fe != NULL) {
-		struct i2c_adapter *i2c_tuner \
-			= cx24123_get_tuner_i2c_adapter(fc->fe);
-		if (i2c_tuner != NULL) {
-			if (dvb_attach(cx24113_attach, fc->fe,
-						&skystar2_rev2_8_cx24113_config,
-						i2c_tuner) == NULL)
-				err("CX24113 could NOT be attached");
-			else
-				info("CX24113 successfully attached");
-		}
-
-		fc->fc_i2c_adap[2].no_base_addr = 1;
-		if (dvb_attach(isl6421_attach, fc->fe,
-			&fc->fc_i2c_adap[2].i2c_adap, 0x08, 0, 0) == NULL)
-			err("ISL6421 could NOT be attached");
-		else
-			info("ISL6421 successfully attached");
+static int skystar2_rev28_attach(struct flexcop_device *fc,
+	struct i2c_adapter *i2c)
+{
+	struct i2c_adapter *i2c_tuner;
+
+	fc->fe = dvb_attach(cx24123_attach, &skystar2_rev2_8_cx24123_config,
+			    i2c);
+	if (!fc->fe)
+		return 0;
+
+	i2c_tuner = cx24123_get_tuner_i2c_adapter(fc->fe);;
+	if (!i2c_tuner)
+		return 0;
+
+	if (!dvb_attach(cx24113_attach, fc->fe, &skystar2_rev2_8_cx24113_config,
+			i2c_tuner)) {
+		err("CX24113 could NOT be attached");
+		return 0;
+	}
+	info("CX24113 successfully attached");
+
+	fc->fc_i2c_adap[2].no_base_addr = 1;
+	if (!dvb_attach(isl6421_attach, fc->fe, &fc->fc_i2c_adap[2].i2c_adap,
+			0x08, 0, 0)) {
+		err("ISL6421 could NOT be attached");
+		fc->fc_i2c_adap[2].no_base_addr = 0;
+		return 0;
+	}
+	info("ISL6421 successfully attached");
 	/* TODO on i2c_adap[1] addr 0x11 (EEPROM) there seems to be an
 	 * IR-receiver (PIC16F818) - but the card has no input for that ??? */
-	}
+	return 1;
 }
 #endif

@@ -466,12 +485,15 @@  static struct mt352_config samsung_tdtc9
 	.demod_init    = samsung_tdtc9251dh0_demod_init,
 };

-static void airstar_dvbt_attach(struct flexcop_device *fc,
+static int airstar_dvbt_attach(struct flexcop_device *fc,
 	struct i2c_adapter *i2c)
 {
 	fc->fe = dvb_attach(mt352_attach, &samsung_tdtc9251dh0_config, i2c);
-	if (fc->fe != NULL)
+	if (fc->fe != NULL) {
 		fc->fe->ops.tuner_ops.calc_regs = samsung_tdtc9251dh0_calc_regs;
+		return 1;
+	}
+	return 0;
 }
 #endif

@@ -489,10 +511,11 @@  static struct bcm3510_config air2pc_atsc
 	.request_firmware = flexcop_fe_request_firmware,
 };

-static void airstar_atsc1_attach(struct flexcop_device *fc,
+static int airstar_atsc1_attach(struct flexcop_device *fc,
 	struct i2c_adapter *i2c)
 {
 	fc->fe = dvb_attach(bcm3510_attach, &air2pc_atsc_first_gen_config, i2c);
+	return fc->fe != NULL;
 }
 #endif

@@ -502,13 +525,15 @@  static struct nxt200x_config samsung_tbm
 	.demod_address = 0x0a,
 };

-static void airstar_atsc2_attach(struct flexcop_device *fc,
+static int airstar_atsc2_attach(struct flexcop_device *fc,
 	struct i2c_adapter *i2c)
 {
 	fc->fe = dvb_attach(nxt200x_attach, &samsung_tbmv_config, i2c);
-	if (fc->fe != NULL)
-		dvb_attach(dvb_pll_attach, fc->fe, 0x61, NULL,
-				DVB_PLL_SAMSUNG_TBMV);
+	if (!fc->fe)
+		return 0;
+
+	return !!dvb_attach(dvb_pll_attach, fc->fe, 0x61, NULL,
+			    DVB_PLL_SAMSUNG_TBMV);
 }
 #endif

@@ -521,14 +546,15 @@  static struct lgdt330x_config air2pc_ats
 	.clock_polarity_flip = 1,
 };

-static void airstar_atsc3_attach(struct flexcop_device *fc,
+static int airstar_atsc3_attach(struct flexcop_device *fc,
 	struct i2c_adapter *i2c)
 {
 	fc->fe = dvb_attach(lgdt330x_attach, &air2pc_atsc_hd5000_config, i2c);
-	if (fc->fe != NULL) {
-		dvb_attach(simple_tuner_attach, fc->fe, i2c, 0x61,
-				TUNER_LG_TDVS_H06XF);
-	}
+	if (!fc->fe)
+		return 0;
+
+	return !!dvb_attach(simple_tuner_attach, fc->fe, i2c, 0x61,
+			    TUNER_LG_TDVS_H06XF);
 }
 #endif

@@ -659,22 +685,24 @@  static struct stv0297_config alps_tdee4_
 	.inittab = alps_tdee4_stv0297_inittab,
 };

-static void cablestar2_attach(struct flexcop_device *fc,
+static int cablestar2_attach(struct flexcop_device *fc,
 	struct i2c_adapter *i2c)
 {
 	fc->fc_i2c_adap[0].no_base_addr = 1;
 	fc->fe = dvb_attach(stv0297_attach, &alps_tdee4_stv0297_config, i2c);
-	if (fc->fe != NULL)
-		fc->fe->ops.tuner_ops.set_params \
-		= alps_tdee4_stv0297_tuner_set_params;
-	else
+	if (!fc->fe) {
+		/* Reset for next frontend to try */
 		fc->fc_i2c_adap[0].no_base_addr = 0;
+		return 0;
+	}
+	fc->fe->ops.tuner_ops.set_params = alps_tdee4_stv0297_tuner_set_params;
+	return 1;
 }
 #endif

 static struct {
 	flexcop_device_type_t type;
-	void (*attach)(struct flexcop_device *, struct i2c_adapter *);
+	int (*attach)(struct flexcop_device *, struct i2c_adapter *);
 } flexcop_frontends[] = {
 #if defined(CONFIG_DVB_S5H1420_MODULE)
 	{ FC_SKY_REV27, skystar2_rev27_attach },
@@ -713,9 +741,13 @@  int flexcop_frontend_init(struct flexcop
 		/* type needs to be set before, because of some workarounds
 		 * done based on the probed card type */
 		fc->dev_type = flexcop_frontends[i].type;
-		flexcop_frontends[i].attach(fc, &fc->fc_i2c_adap[0].i2c_adap);
-		if (fc->fe != NULL)
+		if (flexcop_frontends[i].attach(fc, &fc->fc_i2c_adap[0].i2c_adap))
 			goto fe_found;
+		/* Clean up partially attached frontend */
+		if (fc->fe) {
+			dvb_frontend_detach(fc->fe);
+			fc->fe = NULL;
+		}
 	}
 	fc->dev_type = FC_UNK;
 	err("no frontend driver found for this B2C2/FlexCop adapter");
@@ -724,10 +756,8 @@  fe_found:
 fe_found:
 	info("found '%s' .", fc->fe->ops.info.name);
 	if (dvb_register_frontend(&fc->dvb_adapter, fc->fe)) {
-		struct dvb_frontend_ops *ops = &fc->fe->ops;
 		err("frontend registration failed!");
-		if (ops->release != NULL)
-			ops->release(fc->fe);
+		dvb_frontend_detach(fc->fe);
 		fc->fe = NULL;
 		return -EINVAL;
 	}