[PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari]

Signed-off-by: Antti Palosaari <crope@iki.fi>
---
  drivers/media/dvb/dvb-usb/dvb-usb-dvb.c  |   85 
+++++++++++++++++++++++-------
  drivers/media/dvb/dvb-usb/dvb-usb-init.c |    4 ++
  drivers/media/dvb/dvb-usb/dvb-usb.h      |   11 +++-
  3 files changed, 78 insertions(+), 22 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
index d8c0bd9..5e34df7 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
@@ -162,8 +162,11 @@ static int dvb_usb_fe_wakeup(struct dvb_frontend *fe)

  	dvb_usb_device_power_ctrl(adap->dev, 1);

-	if (adap->fe_init)
-		adap->fe_init(fe);
+	if (adap->props.frontend_ctrl)
+		adap->props.frontend_ctrl(fe, 1);
+
+	if (adap->fe_init[fe->id])
+		adap->fe_init[fe->id](fe);

  	return 0;
  }
@@ -172,45 +175,89 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
  {
  	struct dvb_usb_adapter *adap = fe->dvb->priv;

-	if (adap->fe_sleep)
-		adap->fe_sleep(fe);
+	if (adap->fe_sleep[fe->id])
+		adap->fe_sleep[fe->id](fe);
+
+	if (adap->props.frontend_ctrl)
+		adap->props.frontend_ctrl(fe, 0);

  	return dvb_usb_device_power_ctrl(adap->dev, 0);
  }

  int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
  {
+	int ret, i, x;
+
+	memset(adap->fe, 0, sizeof(adap->fe));
+
  	if (adap->props.frontend_attach == NULL) {
-		err("strange: '%s' #%d doesn't want to attach a 
frontend.",adap->dev->desc->name, adap->id);
+		err("strange: '%s' #%d doesn't want to attach a frontend.",
+			adap->dev->desc->name, adap->id);
+
  		return 0;
  	}

-	/* re-assign sleep and wakeup functions */
-	if (adap->props.frontend_attach(adap) == 0 && adap->fe[0] != NULL) {
-		adap->fe_init  = adap->fe[0]->ops.init;  adap->fe[0]->ops.init  = 
dvb_usb_fe_wakeup;
-		adap->fe_sleep = adap->fe[0]->ops.sleep; adap->fe[0]->ops.sleep = 
dvb_usb_fe_sleep;
+	/* register all given adapter frontends */
+	if (adap->props.num_frontends)
+		x = adap->props.num_frontends - 1;
+	else
+		x = 0;
+
+	for (i = 0; i <= x; i++) {
+		ret = adap->props.frontend_attach(adap);
+		if (ret || adap->fe[i] == NULL) {
+			/* only print error when there is no FE at all */
+			if (i == 0)
+				err("no frontend was attached by '%s'",
+					adap->dev->desc->name);
+
+			return 0;
+		}

-		if (dvb_register_frontend(&adap->dvb_adap, adap->fe[0])) {
-			err("Frontend registration failed.");
-			dvb_frontend_detach(adap->fe[0]);
-			adap->fe[0] = NULL;
-			return -ENODEV;
+		adap->fe[i]->id = i;
+
+		/* re-assign sleep and wakeup functions */
+		adap->fe_init[i] = adap->fe[i]->ops.init;
+		adap->fe[i]->ops.init  = dvb_usb_fe_wakeup;
+		adap->fe_sleep[i] = adap->fe[i]->ops.sleep;
+		adap->fe[i]->ops.sleep = dvb_usb_fe_sleep;
+
+		if (dvb_register_frontend(&adap->dvb_adap, adap->fe[i])) {
+			err("Frontend %d registration failed.", i);
+			dvb_frontend_detach(adap->fe[i]);
+			adap->fe[i] = NULL;
+			/* In error case, do not try register more FEs,
+			 * still leaving already registered FEs alive. */
+			if (i == 0)
+				return -ENODEV;
+			else
+				return 0;
  		}

  		/* only attach the tuner if the demod is there */
  		if (adap->props.tuner_attach != NULL)
  			adap->props.tuner_attach(adap);
-	} else
-		err("no frontend was attached by '%s'",adap->dev->desc->name);
+	}

  	return 0;
  }

  int dvb_usb_adapter_frontend_exit(struct dvb_usb_adapter *adap)
  {
-	if (adap->fe[0] != NULL) {
-		dvb_unregister_frontend(adap->fe[0]);
-		dvb_frontend_detach(adap->fe[0]);
+	int i;
+
+	/* unregister all given adapter frontends */
+	if (adap->props.num_frontends)
+		i = adap->props.num_frontends - 1;
+	else
+		i = 0;
+
+	for (; i >= 0; i--) {
+		if (adap->fe[i] != NULL) {
+			dvb_unregister_frontend(adap->fe[i]);
+			dvb_frontend_detach(adap->fe[i]);
+		}
  	}
+
  	return 0;
  }
diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-init.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-init.c
index 2e3ea0f..f9af348 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-init.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-init.c
@@ -77,6 +77,10 @@ static int dvb_usb_adapter_init(struct dvb_usb_device 
*d, short *adapter_nrs)
  			return ret;
  		}

+		/* use exclusive FE lock if there is multiple shared FEs */
+		if (adap->fe[1])
+			adap->dvb_adap.mfe_shared = 1;
+
  		d->num_adapters_initialized++;
  		d->state |= DVB_USB_STATE_DVB;
  	}
diff --git a/drivers/media/dvb/dvb-usb/dvb-usb.h 
b/drivers/media/dvb/dvb-usb/dvb-usb.h
index 2e57bff..a3e77b2 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb.h
+++ b/drivers/media/dvb/dvb-usb/dvb-usb.h
@@ -124,6 +124,8 @@ struct usb_data_stream_properties {
   * @caps: capabilities of the DVB USB device.
   * @pid_filter_count: number of PID filter position in the optional 
hardware
   *  PID-filter.
+ * @num_frontends: number of frontends of the DVB USB adapter.
+ * @frontend_ctrl: called to power on/off active frontend.
   * @streaming_ctrl: called to start and stop the MPEG2-TS streaming of the
   *  device (not URB submitting/killing).
   * @pid_filter_ctrl: called to en/disable the PID filter, if any.
@@ -141,7 +143,9 @@ struct dvb_usb_adapter_properties {
  #define DVB_USB_ADAP_RECEIVES_204_BYTE_TS         0x08
  	int caps;
  	int pid_filter_count;
+	int num_frontends;

+	int (*frontend_ctrl)   (struct dvb_frontend *, int);
  	int (*streaming_ctrl)  (struct dvb_usb_adapter *, int);
  	int (*pid_filter_ctrl) (struct dvb_usb_adapter *, int);
  	int (*pid_filter)      (struct dvb_usb_adapter *, int, u16, int);
@@ -345,6 +349,7 @@ struct usb_data_stream {
   *
   * @stream: the usb data stream.
   */
+#define MAX_NO_OF_FE_PER_ADAP 2
  struct dvb_usb_adapter {
  	struct dvb_usb_device *dev;
  	struct dvb_usb_adapter_properties props;
@@ -363,11 +368,11 @@ struct dvb_usb_adapter {
  	struct dmxdev        dmxdev;
  	struct dvb_demux     demux;
  	struct dvb_net       dvb_net;
-	struct dvb_frontend *fe[1];
+	struct dvb_frontend *fe[MAX_NO_OF_FE_PER_ADAP];
  	int                  max_feed_count;

-	int (*fe_init)  (struct dvb_frontend *);
-	int (*fe_sleep) (struct dvb_frontend *);
+	int (*fe_init[MAX_NO_OF_FE_PER_ADAP])  (struct dvb_frontend *);
+	int (*fe_sleep[MAX_NO_OF_FE_PER_ADAP]) (struct dvb_frontend *);

  	struct usb_data_stream stream;

-- 
1.7.6

-- 
http://palosaari.fi/

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Mauro Carvalho Chehab]

Em 25-07-2011 21:17, Antti Palosaari escreveu:
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  drivers/media/dvb/dvb-usb/dvb-usb-dvb.c  |   85 +++++++++++++++++++++++-------
>  drivers/media/dvb/dvb-usb/dvb-usb-init.c |    4 ++
>  drivers/media/dvb/dvb-usb/dvb-usb.h      |   11 +++-
>  3 files changed, 78 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
> index d8c0bd9..5e34df7 100644
> --- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
> +++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
> @@ -162,8 +162,11 @@ static int dvb_usb_fe_wakeup(struct dvb_frontend *fe)
> 
>      dvb_usb_device_power_ctrl(adap->dev, 1);
> 
> -    if (adap->fe_init)
> -        adap->fe_init(fe);
> +    if (adap->props.frontend_ctrl)
> +        adap->props.frontend_ctrl(fe, 1);
> +
> +    if (adap->fe_init[fe->id])
> +        adap->fe_init[fe->id](fe);
> 
>      return 0;
>  }
> @@ -172,45 +175,89 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
>  {
>      struct dvb_usb_adapter *adap = fe->dvb->priv;
> 
> -    if (adap->fe_sleep)
> -        adap->fe_sleep(fe);
> +    if (adap->fe_sleep[fe->id])
> +        adap->fe_sleep[fe->id](fe);
> +
> +    if (adap->props.frontend_ctrl)
> +        adap->props.frontend_ctrl(fe, 0);
> 
>      return dvb_usb_device_power_ctrl(adap->dev, 0);
>  }
> 
>  int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
>  {
> +    int ret, i, x;
> +
> +    memset(adap->fe, 0, sizeof(adap->fe));
> +
>      if (adap->props.frontend_attach == NULL) {
> -        err("strange: '%s' #%d doesn't want to attach a frontend.",adap->dev->desc->name, adap->id);
> +        err("strange: '%s' #%d doesn't want to attach a frontend.",
> +            adap->dev->desc->name, adap->id);
> +
>          return 0;
>      }
> 
> -    /* re-assign sleep and wakeup functions */
> -    if (adap->props.frontend_attach(adap) == 0 && adap->fe[0] != NULL) {
> -        adap->fe_init  = adap->fe[0]->ops.init;  adap->fe[0]->ops.init  = dvb_usb_fe_wakeup;
> -        adap->fe_sleep = adap->fe[0]->ops.sleep; adap->fe[0]->ops.sleep = dvb_usb_fe_sleep;
> +    /* register all given adapter frontends */
> +    if (adap->props.num_frontends)
> +        x = adap->props.num_frontends - 1;
> +    else
> +        x = 0;
> +
> +    for (i = 0; i <= x; i++) {
> +        ret = adap->props.frontend_attach(adap);
> +        if (ret || adap->fe[i] == NULL) {
> +            /* only print error when there is no FE at all */
> +            if (i == 0)
> +                err("no frontend was attached by '%s'",
> +                    adap->dev->desc->name);

This doesn't seem right. One thing is to accept adap->fe[1] to be
NULL. Another thing is to accept an error at the attach. IMO, the
logic should be something like:

	if (ret < 0)
		return ret;

	if (!i && !adap->fe[0]) {
		err("no adapter!");
		return -ENODEV;
	}

> +
> +            return 0;
> +        }
> 
> -        if (dvb_register_frontend(&adap->dvb_adap, adap->fe[0])) {
> -            err("Frontend registration failed.");
> -            dvb_frontend_detach(adap->fe[0]);
> -            adap->fe[0] = NULL;
> -            return -ENODEV;
> +        adap->fe[i]->id = i;
> +
> +        /* re-assign sleep and wakeup functions */
> +        adap->fe_init[i] = adap->fe[i]->ops.init;
> +        adap->fe[i]->ops.init  = dvb_usb_fe_wakeup;
> +        adap->fe_sleep[i] = adap->fe[i]->ops.sleep;
> +        adap->fe[i]->ops.sleep = dvb_usb_fe_sleep;
> +
> +        if (dvb_register_frontend(&adap->dvb_adap, adap->fe[i])) {
> +            err("Frontend %d registration failed.", i);
> +            dvb_frontend_detach(adap->fe[i]);

There is a special case here: for DRX-K, we can't call dvb_frontend_detach().
as just one drxk_attach() returns the two pointers. While this is not fixed,
we need to add some logic here to check if the adapter were attached.

> +            adap->fe[i] = NULL;
> +            /* In error case, do not try register more FEs,
> +             * still leaving already registered FEs alive. */

I think that the proper thing to do is to detach everything, if one of
the attach fails. There isn't much sense on keeping the device partially
initialized.

> +            if (i == 0)
> +                return -ENODEV;
> +            else
> +                return 0;
>          }
> 
>          /* only attach the tuner if the demod is there */
>          if (adap->props.tuner_attach != NULL)
>              adap->props.tuner_attach(adap);
> -    } else
> -        err("no frontend was attached by '%s'",adap->dev->desc->name);
> +    }
> 
>      return 0;
>  }
> 
>  int dvb_usb_adapter_frontend_exit(struct dvb_usb_adapter *adap)
>  {
> -    if (adap->fe[0] != NULL) {
> -        dvb_unregister_frontend(adap->fe[0]);
> -        dvb_frontend_detach(adap->fe[0]);
> +    int i;
> +
> +    /* unregister all given adapter frontends */
> +    if (adap->props.num_frontends)
> +        i = adap->props.num_frontends - 1;
> +    else
> +        i = 0;
> +
> +    for (; i >= 0; i--) {
> +        if (adap->fe[i] != NULL) {
> +            dvb_unregister_frontend(adap->fe[i]);
> +            dvb_frontend_detach(adap->fe[i]);
> +        }
>      }
> +
>      return 0;
>  }
> diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-init.c b/drivers/media/dvb/dvb-usb/dvb-usb-init.c
> index 2e3ea0f..f9af348 100644
> --- a/drivers/media/dvb/dvb-usb/dvb-usb-init.c
> +++ b/drivers/media/dvb/dvb-usb/dvb-usb-init.c
> @@ -77,6 +77,10 @@ static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs)
>              return ret;
>          }
> 
> +        /* use exclusive FE lock if there is multiple shared FEs */
> +        if (adap->fe[1])
> +            adap->dvb_adap.mfe_shared = 1;

Why? multiple FE doesn't mean that they're mutually exclusive.

> +
>          d->num_adapters_initialized++;
>          d->state |= DVB_USB_STATE_DVB;
>      }
> diff --git a/drivers/media/dvb/dvb-usb/dvb-usb.h b/drivers/media/dvb/dvb-usb/dvb-usb.h
> index 2e57bff..a3e77b2 100644
> --- a/drivers/media/dvb/dvb-usb/dvb-usb.h
> +++ b/drivers/media/dvb/dvb-usb/dvb-usb.h
> @@ -124,6 +124,8 @@ struct usb_data_stream_properties {
>   * @caps: capabilities of the DVB USB device.
>   * @pid_filter_count: number of PID filter position in the optional hardware
>   *  PID-filter.
> + * @num_frontends: number of frontends of the DVB USB adapter.
> + * @frontend_ctrl: called to power on/off active frontend.
>   * @streaming_ctrl: called to start and stop the MPEG2-TS streaming of the
>   *  device (not URB submitting/killing).
>   * @pid_filter_ctrl: called to en/disable the PID filter, if any.
> @@ -141,7 +143,9 @@ struct dvb_usb_adapter_properties {
>  #define DVB_USB_ADAP_RECEIVES_204_BYTE_TS         0x08
>      int caps;
>      int pid_filter_count;
> +    int num_frontends;
> 
> +    int (*frontend_ctrl)   (struct dvb_frontend *, int);
>      int (*streaming_ctrl)  (struct dvb_usb_adapter *, int);
>      int (*pid_filter_ctrl) (struct dvb_usb_adapter *, int);
>      int (*pid_filter)      (struct dvb_usb_adapter *, int, u16, int);
> @@ -345,6 +349,7 @@ struct usb_data_stream {
>   *
>   * @stream: the usb data stream.
>   */
> +#define MAX_NO_OF_FE_PER_ADAP 2
>  struct dvb_usb_adapter {
>      struct dvb_usb_device *dev;
>      struct dvb_usb_adapter_properties props;
> @@ -363,11 +368,11 @@ struct dvb_usb_adapter {
>      struct dmxdev        dmxdev;
>      struct dvb_demux     demux;
>      struct dvb_net       dvb_net;
> -    struct dvb_frontend *fe[1];
> +    struct dvb_frontend *fe[MAX_NO_OF_FE_PER_ADAP];
>      int                  max_feed_count;
> 
> -    int (*fe_init)  (struct dvb_frontend *);
> -    int (*fe_sleep) (struct dvb_frontend *);
> +    int (*fe_init[MAX_NO_OF_FE_PER_ADAP])  (struct dvb_frontend *);
> +    int (*fe_sleep[MAX_NO_OF_FE_PER_ADAP]) (struct dvb_frontend *);
> 
>      struct usb_data_stream stream;
> 

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari]

On 07/27/2011 10:06 PM, Mauro Carvalho Chehab wrote:

>> +    for (i = 0; i<= x; i++) {
>> +        ret = adap->props.frontend_attach(adap);
>> +        if (ret || adap->fe[i] == NULL) {
>> +            /* only print error when there is no FE at all */
>> +            if (i == 0)
>> +                err("no frontend was attached by '%s'",
>> +                    adap->dev->desc->name);
>
> This doesn't seem right. One thing is to accept adap->fe[1] to be
> NULL. Another thing is to accept an error at the attach. IMO, the
> logic should be something like:
>
> 	if (ret<  0)
> 		return ret;
>
> 	if (!i&&  !adap->fe[0]) {
> 		err("no adapter!");
> 		return -ENODEV;
> 	}

Heh, I tried to keep it functioning as earlier not to break anything! 
Only thing it does now differently is that it keeps silent when 2nd FE 
attach fails since we don't know always before fe attach if there is fe 
or not.

So since it *does not change old behaviour* it must be OK. Let fix old 
problems later. There is millions of DVB USB callbacks failing silently 
- like tuner_attach etc.

Surely I want also fix many old issues but it is always too risky.


>> +        if (dvb_register_frontend(&adap->dvb_adap, adap->fe[i])) {
>> +            err("Frontend %d registration failed.", i);
>> +            dvb_frontend_detach(adap->fe[i]);
>
> There is a special case here: for DRX-K, we can't call dvb_frontend_detach().
> as just one drxk_attach() returns the two pointers. While this is not fixed,
> we need to add some logic here to check if the adapter were attached.

There is currently no DVB USB driver using DRX-K thus it is not problem. 
If someone will add such support can fix DRX-K driver.

Also take into account it is possible to register only 1 FE using DVB 
USB and manually register 2nd FE as it have been those days.


>> +            adap->fe[i] = NULL;
>> +            /* In error case, do not try register more FEs,
>> +             * still leaving already registered FEs alive. */
>
> I think that the proper thing to do is to detach everything, if one of
> the attach fails. There isn't much sense on keeping the device partially
> initialized.

It is not attach which fails here, it is dvb_register_frontend 
registration. And I thought that too since it is rather major problem. 
But as it seems to be very easy to do it like that I did. Not big issue 
though.



>> +        /* use exclusive FE lock if there is multiple shared FEs */
>> +        if (adap->fe[1])
>> +            adap->dvb_adap.mfe_shared = 1;
>
> Why? multiple FE doesn't mean that they're mutually exclusive.

And again we ran same MFE discussion..... But if you look DVB USB you 
can see its main idea have been to register multiple adapters in case of 
not exclusive frontends. That have been most likely implemented like 
that from day-0 of DVB USB.

Another point is that it is not that patch which adds support for MFE. 
It is old issue and I am not responsible of it. See patch where this 
decision is made in year 2008:

commit 59b1842da1c6f33ad2e8da82d3dfb3445751d964
Author: Darron Broad <darron@kewl.org>
Date:   Sat Oct 11 11:44:05 2008 -0300

     V4L/DVB (9227): MFE: Add multi-frontend mutual exclusion


Why to change again old behaviour?


Could you now read all with care. As I pointed out almost all your 
findings are are decisions made *earlier*.

Only thing which is not coming from oldies is that you suggest to 
unregister all frontends if one register fails.

regards
Antti


-- 
http://palosaari.fi/

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Mauro Carvalho Chehab]

Em 27-07-2011 16:49, Antti Palosaari escreveu:
> On 07/27/2011 10:06 PM, Mauro Carvalho Chehab wrote:
> 
>>> +    for (i = 0; i<= x; i++) {
>>> +        ret = adap->props.frontend_attach(adap);
>>> +        if (ret || adap->fe[i] == NULL) {
>>> +            /* only print error when there is no FE at all */
>>> +            if (i == 0)
>>> +                err("no frontend was attached by '%s'",
>>> +                    adap->dev->desc->name);
>>
>> This doesn't seem right. One thing is to accept adap->fe[1] to be
>> NULL. Another thing is to accept an error at the attach. IMO, the
>> logic should be something like:
>>
>>     if (ret<  0)
>>         return ret;
>>
>>     if (!i&&  !adap->fe[0]) {
>>         err("no adapter!");
>>         return -ENODEV;
>>     }
> 
> Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps 
> silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
> 
> So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB 
> callbacks failing silently - like tuner_attach etc.
> 
> Surely I want also fix many old issues but it is always too risky.

I'm talking about adding another bad behavior: let MFE to have one of the FE's
failing.
> 
> 
>>> +        if (dvb_register_frontend(&adap->dvb_adap, adap->fe[i])) {
>>> +            err("Frontend %d registration failed.", i);
>>> +            dvb_frontend_detach(adap->fe[i]);
>>
>> There is a special case here: for DRX-K, we can't call dvb_frontend_detach().
>> as just one drxk_attach() returns the two pointers. While this is not fixed,
>> we need to add some logic here to check if the adapter were attached.
> 
> There is currently no DVB USB driver using DRX-K thus it is not problem. If someone will add such support can fix DRX-K driver.

I agree. I was just rising the question. The glue should be bound together with the first
dvb-usb DRX-K driver.

> Also take into account it is possible to register only 1 FE using DVB USB and manually register 2nd FE as it have been those days.
> 
> 
>>> +            adap->fe[i] = NULL;
>>> +            /* In error case, do not try register more FEs,
>>> +             * still leaving already registered FEs alive. */
>>
>> I think that the proper thing to do is to detach everything, if one of
>> the attach fails. There isn't much sense on keeping the device partially
>> initialized.

> It is not attach which fails here, it is dvb_register_frontend registration.

Yes, sure. I've miss-named.

> And I thought that too since it is rather major problem. But as it seems to be very easy to do it like that I did. Not big issue though.

Core functions should be more strict than drivers. So, I think it should really
enforce an all-or-nothing behavior here.

>>> +        /* use exclusive FE lock if there is multiple shared FEs */
>>> +        if (adap->fe[1])
>>> +            adap->dvb_adap.mfe_shared = 1;
>>
>> Why? multiple FE doesn't mean that they're mutually exclusive.
> 
> And again we ran same MFE discussion..... But if you look DVB USB you can see its main idea
> have been to register multiple adapters in case of not exclusive frontends. That have been
> most likely implemented like that from day-0 of DVB USB.
> 
> Another point is that it is not that patch which adds support for MFE. It is old issue and 
> I am not responsible of it. See patch where this decision is made in year 2008:
> 
> commit 59b1842da1c6f33ad2e8da82d3dfb3445751d964
> Author: Darron Broad <darron@kewl.org>
> Date:   Sat Oct 11 11:44:05 2008 -0300
> 
>     V4L/DVB (9227): MFE: Add multi-frontend mutual exclusion
> 
> 
> Why to change again old behaviour?
> 
> 
> Could you now read all with care. As I pointed out almost all your findings are are decisions made *earlier*.
> 
> Only thing which is not coming from oldies is that you suggest to unregister all frontends if one register fails.

You miss-understood me. I'm not saying that we should drop support for mfe_shared flag.
I'm just saying that dvb-usb core should not assume that a driver with more than one
FE have them into a mutually exclusive configuration.

If you look at dvb-core, the default for it is mfe_shared = 0. We should keep the same
default for dvb-usb. 

In other words, it should be up to the drivers to change mfe_shared flag to 1, and not
to the core.

Cheers,
Mauro

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari]

On 07/27/2011 11:06 PM, Mauro Carvalho Chehab wrote:
> Em 27-07-2011 16:49, Antti Palosaari escreveu:
>> On 07/27/2011 10:06 PM, Mauro Carvalho Chehab wrote:
>>
>>>> +    for (i = 0; i<= x; i++) {
>>>> +        ret = adap->props.frontend_attach(adap);
>>>> +        if (ret || adap->fe[i] == NULL) {
>>>> +            /* only print error when there is no FE at all */
>>>> +            if (i == 0)
>>>> +                err("no frontend was attached by '%s'",
>>>> +                    adap->dev->desc->name);
>>>
>>> This doesn't seem right. One thing is to accept adap->fe[1] to be
>>> NULL. Another thing is to accept an error at the attach. IMO, the
>>> logic should be something like:
>>>
>>>      if (ret<   0)
>>>          return ret;
>>>
>>>      if (!i&&   !adap->fe[0]) {
>>>          err("no adapter!");
>>>          return -ENODEV;
>>>      }
>>
>> Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps
>> silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
>>
>> So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB
>> callbacks failing silently - like tuner_attach etc.
>>
>> Surely I want also fix many old issues but it is always too risky.
>
> I'm talking about adding another bad behavior: let MFE to have one of the FE's
> failing.

Let the device driver made decision! I can add check that if ret < 0 
then fail as fatal, but if there is ret=0 and fe=NULL is not fatal. But 
since it was discarded earlier I didnt added such check.

There is many devices have like 2 demod places on board. Then it is 
housed as demod#0 + demod#1, #demod0 or #demod1. You will not know that 
before trying to attach demod. Thus it *must* be possible to discard 
missing demod silently. Surely there is soon someone end user making bug 
reports if you print error on that case or even deregister whole device.

>>>> +        if (dvb_register_frontend(&adap->dvb_adap, adap->fe[i])) {
>>>> +            err("Frontend %d registration failed.", i);
>>>> +            dvb_frontend_detach(adap->fe[i]);
>>>
>>> There is a special case here: for DRX-K, we can't call dvb_frontend_detach().
>>> as just one drxk_attach() returns the two pointers. While this is not fixed,
>>> we need to add some logic here to check if the adapter were attached.
>>
>> There is currently no DVB USB driver using DRX-K thus it is not problem. If someone will add such support can fix DRX-K driver.
>
> I agree. I was just rising the question. The glue should be bound together with the first
> dvb-usb DRX-K driver.
>
>> Also take into account it is possible to register only 1 FE using DVB USB and manually register 2nd FE as it have been those days.
>>
>>
>>>> +            adap->fe[i] = NULL;
>>>> +            /* In error case, do not try register more FEs,
>>>> +             * still leaving already registered FEs alive. */
>>>
>>> I think that the proper thing to do is to detach everything, if one of
>>> the attach fails. There isn't much sense on keeping the device partially
>>> initialized.
>
>> It is not attach which fails here, it is dvb_register_frontend registration.
>
> Yes, sure. I've miss-named.
>
>> And I thought that too since it is rather major problem. But as it seems to be very easy to do it like that I did. Not big issue though.
>
> Core functions should be more strict than drivers. So, I think it should really
> enforce an all-or-nothing behavior here.

Lets add it later, like next merge window.

>>>> +        /* use exclusive FE lock if there is multiple shared FEs */
>>>> +        if (adap->fe[1])
>>>> +            adap->dvb_adap.mfe_shared = 1;
>>>
>>> Why? multiple FE doesn't mean that they're mutually exclusive.
>>
>> And again we ran same MFE discussion..... But if you look DVB USB you can see its main idea
>> have been to register multiple adapters in case of not exclusive frontends. That have been
>> most likely implemented like that from day-0 of DVB USB.
>>
>> Another point is that it is not that patch which adds support for MFE. It is old issue and
>> I am not responsible of it. See patch where this decision is made in year 2008:
>>
>> commit 59b1842da1c6f33ad2e8da82d3dfb3445751d964
>> Author: Darron Broad<darron@kewl.org>
>> Date:   Sat Oct 11 11:44:05 2008 -0300
>>
>>      V4L/DVB (9227): MFE: Add multi-frontend mutual exclusion
>>
>>
>> Why to change again old behaviour?
>>
>>
>> Could you now read all with care. As I pointed out almost all your findings are are decisions made *earlier*.
>>
>> Only thing which is not coming from oldies is that you suggest to unregister all frontends if one register fails.
>
> You miss-understood me. I'm not saying that we should drop support for mfe_shared flag.
> I'm just saying that dvb-usb core should not assume that a driver with more than one
> FE have them into a mutually exclusive configuration.
>
> If you look at dvb-core, the default for it is mfe_shared = 0. We should keep the same
> default for dvb-usb.
>
> In other words, it should be up to the drivers to change mfe_shared flag to 1, and not
> to the core.

Peace of cake. Anyhow, we still have 2 adapters for DVB USB for that and 
have been long than I can remember. So I cannot real see point of doing 
that.


regards
Antti


-- 
http://palosaari.fi/

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Malcolm Priestley]

On Wed, 2011-07-27 at 16:06 -0300, Mauro Carvalho Chehab wrote:
> Em 25-07-2011 21:17, Antti Palosaari escreveu:
> > Signed-off-by: Antti Palosaari <crope@iki.fi>
> > ---
> >  drivers/media/dvb/dvb-usb/dvb-usb-dvb.c  |   85 +++++++++++++++++++++++-------
> >  drivers/media/dvb/dvb-usb/dvb-usb-init.c |    4 ++
> >  drivers/media/dvb/dvb-usb/dvb-usb.h      |   11 +++-
> >  3 files changed, 78 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
> > index d8c0bd9..5e34df7 100644
> > --- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
> > +++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
> > @@ -162,8 +162,11 @@ static int dvb_usb_fe_wakeup(struct dvb_frontend *fe)
> > 
> >      dvb_usb_device_power_ctrl(adap->dev, 1);
> > 
> > -    if (adap->fe_init)
> > -        adap->fe_init(fe);
> > +    if (adap->props.frontend_ctrl)
> > +        adap->props.frontend_ctrl(fe, 1);
> > +
> > +    if (adap->fe_init[fe->id])
> > +        adap->fe_init[fe->id](fe);
> > 
> >      return 0;
> >  }
> > @@ -172,45 +175,89 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
> >  {
> >      struct dvb_usb_adapter *adap = fe->dvb->priv;
> > 
> > -    if (adap->fe_sleep)
> > -        adap->fe_sleep(fe);
> > +    if (adap->fe_sleep[fe->id])
> > +        adap->fe_sleep[fe->id](fe);
> > +
> > +    if (adap->props.frontend_ctrl)
> > +        adap->props.frontend_ctrl(fe, 0);
> > 
> >      return dvb_usb_device_power_ctrl(adap->dev, 0);
> >  }
> > 
> >  int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
> >  {
> > +    int ret, i, x;
> > +
> > +    memset(adap->fe, 0, sizeof(adap->fe));
> > +
> >      if (adap->props.frontend_attach == NULL) {
> > -        err("strange: '%s' #%d doesn't want to attach a frontend.",adap->dev->desc->name, adap->id);
> > +        err("strange: '%s' #%d doesn't want to attach a frontend.",
> > +            adap->dev->desc->name, adap->id);
> > +
> >          return 0;
> >      }
> > 
> > -    /* re-assign sleep and wakeup functions */
> > -    if (adap->props.frontend_attach(adap) == 0 && adap->fe[0] != NULL) {
> > -        adap->fe_init  = adap->fe[0]->ops.init;  adap->fe[0]->ops.init  = dvb_usb_fe_wakeup;
> > -        adap->fe_sleep = adap->fe[0]->ops.sleep; adap->fe[0]->ops.sleep = dvb_usb_fe_sleep;
> > +    /* register all given adapter frontends */
> > +    if (adap->props.num_frontends)
> > +        x = adap->props.num_frontends - 1;
> > +    else
> > +        x = 0;
> > +
> > +    for (i = 0; i <= x; i++) {
> > +        ret = adap->props.frontend_attach(adap);
> > +        if (ret || adap->fe[i] == NULL) {
> > +            /* only print error when there is no FE at all */
> > +            if (i == 0)
> > +                err("no frontend was attached by '%s'",
> > +                    adap->dev->desc->name);
> 
> This doesn't seem right. One thing is to accept adap->fe[1] to be
> NULL. Another thing is to accept an error at the attach. IMO, the
> logic should be something like:
> 
> 	if (ret < 0)
> 		return ret;
> 
> 	if (!i && !adap->fe[0]) {
> 		err("no adapter!");
> 		return -ENODEV;
> 	}
> 
> > +
> > +            return 0;
> > +        }
> > 
> > -        if (dvb_register_frontend(&adap->dvb_adap, adap->fe[0])) {
> > -            err("Frontend registration failed.");
> > -            dvb_frontend_detach(adap->fe[0]);
> > -            adap->fe[0] = NULL;
> > -            return -ENODEV;
> > +        adap->fe[i]->id = i;
> > +
> > +        /* re-assign sleep and wakeup functions */
> > +        adap->fe_init[i] = adap->fe[i]->ops.init;
> > +        adap->fe[i]->ops.init  = dvb_usb_fe_wakeup;
> > +        adap->fe_sleep[i] = adap->fe[i]->ops.sleep;
> > +        adap->fe[i]->ops.sleep = dvb_usb_fe_sleep;
> > +
> > +        if (dvb_register_frontend(&adap->dvb_adap, adap->fe[i])) {
> > +            err("Frontend %d registration failed.", i);
> > +            dvb_frontend_detach(adap->fe[i]);
> 
> There is a special case here: for DRX-K, we can't call dvb_frontend_detach().
> as just one drxk_attach() returns the two pointers. While this is not fixed,
> we need to add some logic here to check if the adapter were attached.
> 
> > +            adap->fe[i] = NULL;
> > +            /* In error case, do not try register more FEs,
> > +             * still leaving already registered FEs alive. */
> 
> I think that the proper thing to do is to detach everything, if one of
> the attach fails. There isn't much sense on keeping the device partially
> initialized.
> 
>From memory, I recall the existing code doesn't detach the frontend even
if the device driver forces an error. So, the device driver must detach
the frontend first.

The trouble is that dvb-usb is becoming dated as new drivers tend to
work around it. No one likes to touch it, out of fear of breaking
existing drivers.

I think perhaps some kind of legacy wrapper is needed here, with the
moving of dvb-usb to its own core, so more development work can be done.

Regards

Malcolm

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari]

On 07/28/2011 01:07 AM, Malcolm Priestley wrote:
> On Wed, 2011-07-27 at 16:06 -0300, Mauro Carvalho Chehab wrote:
>> Em 25-07-2011 21:17, Antti Palosaari escreveu:
>>> Signed-off-by: Antti Palosaari<crope@iki.fi>

>>> +            adap->fe[i] = NULL;
>>> +            /* In error case, do not try register more FEs,
>>> +             * still leaving already registered FEs alive. */
>>
>> I think that the proper thing to do is to detach everything, if one of
>> the attach fails. There isn't much sense on keeping the device partially
>> initialized.
>>
>> From memory, I recall the existing code doesn't detach the frontend even
> if the device driver forces an error. So, the device driver must detach
> the frontend first.

Yes, if you return for example error (or fe == NULL) for .tuner_attach() 
it does not detach or deregister it - just silently discard all. I 
thought very many times those when implementing this and keep very 
careful not to change old functionality.

> The trouble is that dvb-usb is becoming dated as new drivers tend to
> work around it. No one likes to touch it, out of fear of breaking
> existing drivers.

Yes, so true. I have thought that too. There is a lot of things I want 
to change but those are very hard without massive work.

* We should have priv at the very first. No priv for FW DL example.
* Remote keytable should be property of certain device model not adapter
* There should be way to set count of adapter (and fe) in runtime (this 
is why I allowed to fail 2nd FE attach silently)
* no probe (read eeprom etc) callback (I think read MAC could be renamed 
for probe)
* no FE power control (GPIOs etc) that MFE patch adds this too
* maybe probe1 and probe2 callbacks needed. sequence something like plug 
device => probe1 => download FW => probe2 => attach demod


> I think perhaps some kind of legacy wrapper is needed here, with the
> moving of dvb-usb to its own core, so more development work can be done.

Wish like to thank you these comments Malcolm!

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Patrick Boettcher]

On Thursday 28 July 2011 00:23:06 Antti Palosaari wrote:
> On 07/28/2011 01:07 AM, Malcolm Priestley wrote:
> > On Wed, 2011-07-27 at 16:06 -0300, Mauro Carvalho Chehab wrote:
> >> Em 25-07-2011 21:17, Antti Palosaari escreveu:
> >>> Signed-off-by: Antti Palosaari<crope@iki.fi>
> >>> 
> >>> +            adap->fe[i] = NULL;
> >>> +            /* In error case, do not try register more FEs,
> >>> +             * still leaving already registered FEs alive. */
> >> 
> >> I think that the proper thing to do is to detach everything, if one of
> >> the attach fails. There isn't much sense on keeping the device
> >> partially initialized.
> >> 
> >> From memory, I recall the existing code doesn't detach the frontend
> >> even
> > 
> > if the device driver forces an error. So, the device driver must detach
> > the frontend first.
> 
> Yes, if you return for example error (or fe == NULL) for .tuner_attach()
> it does not detach or deregister it - just silently discard all. I
> thought very many times those when implementing this and keep very
> careful not to change old functionality.
> 
> > The trouble is that dvb-usb is becoming dated as new drivers tend to
> > work around it. No one likes to touch it, out of fear of breaking
> > existing drivers.
> 
> Yes, so true. I have thought that too. There is a lot of things I want
> to change but those are very hard without massive work.
> 
> * We should have priv at the very first. No priv for FW DL example.
> * Remote keytable should be property of certain device model not adapter
> * There should be way to set count of adapter (and fe) in runtime (this
> is why I allowed to fail 2nd FE attach silently)
> * no probe (read eeprom etc) callback (I think read MAC could be renamed
> for probe)
> * no FE power control (GPIOs etc) that MFE patch adds this too
> * maybe probe1 and probe2 callbacks needed. sequence something like plug
> device => probe1 => download FW => probe2 => attach demod

If I had more time I'd add 

* handle suspend/resume calls properly for buggy USB firmwares (iow: all 
devices I saw)

--
Patrick Boettcher - KernelLabs
http://www.kernellabs.com/

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Mauro Carvalho Chehab]

Em 27-07-2011 16:49, Antti Palosaari escreveu:
> On 07/27/2011 10:06 PM, Mauro Carvalho Chehab wrote:
> 
>>> +    for (i = 0; i<= x; i++) {
>>> +        ret = adap->props.frontend_attach(adap);
>>> +        if (ret || adap->fe[i] == NULL) {
>>> +            /* only print error when there is no FE at all */
>>> +            if (i == 0)
>>> +                err("no frontend was attached by '%s'",
>>> +                    adap->dev->desc->name);
>>
>> This doesn't seem right. One thing is to accept adap->fe[1] to be
>> NULL. Another thing is to accept an error at the attach. IMO, the
>> logic should be something like:
>>
>>     if (ret<  0)
>>         return ret;
>>
>>     if (!i&&  !adap->fe[0]) {
>>         err("no adapter!");
>>         return -ENODEV;
>>     }
> 
> Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
> 
> So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB callbacks failing silently - like tuner_attach etc.
> 
> Surely I want also fix many old issues but it is always too risky.

Added support for DRX-K way at dvb-usb:

http://git.linuxtv.org/mchehab/experimental.git/commitdiff/765b3db218f1e9af6432251c2ebe59bc9660cd42
http://git.linuxtv.org/mchehab/experimental.git/commitdiff/37fa5797c58068cc60cca6829bd662cd4f883cfa

One bad thing I noticed with the API is that it calls adap->props.frontend_attach(adap)
several times, instead of just one, without even passing an argument for the driver to
know that it was called twice.

IMO, there are two ways of doing the attach:

1) call it only once, and, inside the driver, it will loop to add the other FE's;
2) add a parameter, at the call, to say what FE needs to be initialized.

I think (1) is preferred, as it is more flexible, allowing the driver to test for
several types of frontends.

Regards,
Mauro

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari]

On 08/01/2011 03:46 AM, Mauro Carvalho Chehab wrote:
> Em 27-07-2011 16:49, Antti Palosaari escreveu:
>> On 07/27/2011 10:06 PM, Mauro Carvalho Chehab wrote:
>>
>>>> +    for (i = 0; i<= x; i++) {
>>>> +        ret = adap->props.frontend_attach(adap);
>>>> +        if (ret || adap->fe[i] == NULL) {
>>>> +            /* only print error when there is no FE at all */
>>>> +            if (i == 0)
>>>> +                err("no frontend was attached by '%s'",
>>>> +                    adap->dev->desc->name);
>>>
>>> This doesn't seem right. One thing is to accept adap->fe[1] to be
>>> NULL. Another thing is to accept an error at the attach. IMO, the
>>> logic should be something like:
>>>
>>>     if (ret<  0)
>>>         return ret;
>>>
>>>     if (!i&&  !adap->fe[0]) {
>>>         err("no adapter!");
>>>         return -ENODEV;
>>>     }
>>
>> Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
>>
>> So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB callbacks failing silently - like tuner_attach etc.
>>
>> Surely I want also fix many old issues but it is always too risky.
> 
> Added support for DRX-K way at dvb-usb:
> 
> http://git.linuxtv.org/mchehab/experimental.git/commitdiff/765b3db218f1e9af6432251c2ebe59bc9660cd42
> http://git.linuxtv.org/mchehab/experimental.git/commitdiff/37fa5797c58068cc60cca6829bd662cd4f883cfa
> 
> One bad thing I noticed with the API is that it calls adap->props.frontend_attach(adap)
> several times, instead of just one, without even passing an argument for the driver to
> know that it was called twice.
> 
> IMO, there are two ways of doing the attach:
> 
> 1) call it only once, and, inside the driver, it will loop to add the other FE's;
> 2) add a parameter, at the call, to say what FE needs to be initialized.
> 
> I think (1) is preferred, as it is more flexible, allowing the driver to test for
> several types of frontends.

For more you add configuration parameters more it goes complex. Now it
calls attach as many times as .num_frontends is set in adapter
configuration. It is currently only DRX-K frontend which does not behave
like other FEs. You have added similar hacks to em28xx and now DVB USB.
Maybe it could be easier to change DRX-K driver to attach and register
as others. Also I see it very easy at least in theory to register as one
DRX-K FE normally and then hack 2nd FE in device driver (which is I
think done other drivers using that chip too).


regards
Antti
-- 
http://palosaari.fi/

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari]

On 07/31/2011 09:28 PM, Patrick Boettcher wrote:
> On Thursday 28 July 2011 00:23:06 Antti Palosaari wrote:
>> * We should have priv at the very first. No priv for FW DL example.
>> * Remote keytable should be property of certain device model not adapter
>> * There should be way to set count of adapter (and fe) in runtime (this
>> is why I allowed to fail 2nd FE attach silently)
>> * no probe (read eeprom etc) callback (I think read MAC could be renamed
>> for probe)
>> * no FE power control (GPIOs etc) that MFE patch adds this too
>> * maybe probe1 and probe2 callbacks needed. sequence something like plug
>> device => probe1 => download FW => probe2 => attach demod
> 
> If I had more time I'd add 
> 
> * handle suspend/resume calls properly for buggy USB firmwares (iow: all 
> devices I saw)

I think I will try to change next that priv is accessible at the very
first. That's rather big problem since there is multiple drivers needing
priv for communication (buffers, msg seq numbers, etc) for example fw
loading.

After that I see important to move remote keytable mapping to level
where is USB IDs, device name and such info.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Mauro Carvalho Chehab]

Em 31-07-2011 22:22, Antti Palosaari escreveu:
> On 08/01/2011 03:46 AM, Mauro Carvalho Chehab wrote:
>> Em 27-07-2011 16:49, Antti Palosaari escreveu:
>>> On 07/27/2011 10:06 PM, Mauro Carvalho Chehab wrote:
>>>
>>>>> +    for (i = 0; i<= x; i++) {
>>>>> +        ret = adap->props.frontend_attach(adap);
>>>>> +        if (ret || adap->fe[i] == NULL) {
>>>>> +            /* only print error when there is no FE at all */
>>>>> +            if (i == 0)
>>>>> +                err("no frontend was attached by '%s'",
>>>>> +                    adap->dev->desc->name);
>>>>
>>>> This doesn't seem right. One thing is to accept adap->fe[1] to be
>>>> NULL. Another thing is to accept an error at the attach. IMO, the
>>>> logic should be something like:
>>>>
>>>>     if (ret<  0)
>>>>         return ret;
>>>>
>>>>     if (!i&&  !adap->fe[0]) {
>>>>         err("no adapter!");
>>>>         return -ENODEV;
>>>>     }
>>>
>>> Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
>>>
>>> So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB callbacks failing silently - like tuner_attach etc.
>>>
>>> Surely I want also fix many old issues but it is always too risky.
>>
>> Added support for DRX-K way at dvb-usb:
>>
>> http://git.linuxtv.org/mchehab/experimental.git/commitdiff/765b3db218f1e9af6432251c2ebe59bc9660cd42
>> http://git.linuxtv.org/mchehab/experimental.git/commitdiff/37fa5797c58068cc60cca6829bd662cd4f883cfa
>>
>> One bad thing I noticed with the API is that it calls adap->props.frontend_attach(adap)
>> several times, instead of just one, without even passing an argument for the driver to
>> know that it was called twice.
>>
>> IMO, there are two ways of doing the attach:
>>
>> 1) call it only once, and, inside the driver, it will loop to add the other FE's;
>> 2) add a parameter, at the call, to say what FE needs to be initialized.
>>
>> I think (1) is preferred, as it is more flexible, allowing the driver to test for
>> several types of frontends.
> 
> For more you add configuration parameters more it goes complex. Now it
> calls attach as many times as .num_frontends is set in adapter
> configuration.

True, but even on a device with separate frontends, it needs to have
some way to track what's the fe number that is free, and even this
won't work.

The logic there expects that, for the first call, it will fill fe[0],
for the second call, it will fill fe[1], and so on. If, for whatever
reason, a call to for fe[0] fails, the driver may do the wrong thing,
e. g. it may be filling fe[0] while fe[1] is expected, fe[1] while fe[2]
is expected, and so on.

As I said before: or it should be called once, or it needs to pass a 
parameter to the driver to indicate what fe is expected to be filled.

> It is currently only DRX-K frontend which does not behave
> like other FEs. You have added similar hacks to em28xx and now DVB USB.
> Maybe it could be easier to change DRX-K driver to attach and register
> as others. Also I see it very easy at least in theory to register as one
> DRX-K FE normally and then hack 2nd FE in device driver (which is I
> think done other drivers using that chip too).

The current way of handling DRX-K is a temporary solution, while we don't come
to a conclusion about how should we address a single frontend, like DRX-K,
that supports multiple delivery systems.

I agree that this is ugly, but mapping the same frontend as two separate FE's
is also ugly.

When MFE were first added, it were to be used by two different frontends. When
the same FE implements more than one delivery system, there are currently two
ways of implementing it: using MFE, or using DVB S2API. Both ways are currently
used, depending on the delivery systems. This is messy.

Regards,
Mauro.

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari]

On 08/01/2011 05:24 AM, Mauro Carvalho Chehab wrote:
> Em 31-07-2011 22:22, Antti Palosaari escreveu:
>> On 08/01/2011 03:46 AM, Mauro Carvalho Chehab wrote:
>>> One bad thing I noticed with the API is that it calls adap->props.frontend_attach(adap)
>>> several times, instead of just one, without even passing an argument for the driver to
>>> know that it was called twice.
>>>
>>> IMO, there are two ways of doing the attach:
>>>
>>> 1) call it only once, and, inside the driver, it will loop to add the other FE's;
>>> 2) add a parameter, at the call, to say what FE needs to be initialized.
>>>
>>> I think (1) is preferred, as it is more flexible, allowing the driver to test for
>>> several types of frontends.

I am planning to change DVB USB MFE call .frontend_attach() only once. 
Is there any comments about that?

Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and 
change is needed ASAP before more MFE devices are coming.

Also .num_frontends can be removed after that, since DVB USB will just 
loop through 0 to MAX FEs and register all FEs found (fe pointer !NULL).

CURRENTLY:
==========
.frontend_attach()
	if (adap->fe_adap[0].fe == NULL)
		adap->fe_adap[0].fe = dvb_attach(DVB-T)
	else if (adap->fe_adap[1].fe == NULL)
		adap->fe_adap[1].fe = dvb_attach(DVB-C)
	else if (adap->fe_adap[2].fe == NULL)
		adap->fe_adap[2].fe = dvb_attach(DVB-S)

PLANNED:
========
.frontend_attach()
	adap->fe_adap[0].fe = dvb_attach(DVB-T)
	adap->fe_adap[1].fe = dvb_attach(DVB-C)
	adap->fe_adap[2].fe = dvb_attach(DVB-S)


regards
Antti
-- 
http://palosaari.fi/

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Michael Krufky]

On Wed, Sep 7, 2011 at 12:51 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 08/01/2011 05:24 AM, Mauro Carvalho Chehab wrote:
>>
>> Em 31-07-2011 22:22, Antti Palosaari escreveu:
>>>
>>> On 08/01/2011 03:46 AM, Mauro Carvalho Chehab wrote:
>>>>
>>>> One bad thing I noticed with the API is that it calls
>>>> adap->props.frontend_attach(adap)
>>>> several times, instead of just one, without even passing an argument for
>>>> the driver to
>>>> know that it was called twice.
>>>>
>>>> IMO, there are two ways of doing the attach:
>>>>
>>>> 1) call it only once, and, inside the driver, it will loop to add the
>>>> other FE's;
>>>> 2) add a parameter, at the call, to say what FE needs to be initialized.
>>>>
>>>> I think (1) is preferred, as it is more flexible, allowing the driver to
>>>> test for
>>>> several types of frontends.
>
> I am planning to change DVB USB MFE call .frontend_attach() only once. Is
> there any comments about that?
>
> Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and change
> is needed ASAP before more MFE devices are coming.
>
> Also .num_frontends can be removed after that, since DVB USB will just loop
> through 0 to MAX FEs and register all FEs found (fe pointer !NULL).
>
> CURRENTLY:
> ==========
> .frontend_attach()
>        if (adap->fe_adap[0].fe == NULL)
>                adap->fe_adap[0].fe = dvb_attach(DVB-T)
>        else if (adap->fe_adap[1].fe == NULL)
>                adap->fe_adap[1].fe = dvb_attach(DVB-C)
>        else if (adap->fe_adap[2].fe == NULL)
>                adap->fe_adap[2].fe = dvb_attach(DVB-S)
>
> PLANNED:
> ========
> .frontend_attach()
>        adap->fe_adap[0].fe = dvb_attach(DVB-T)
>        adap->fe_adap[1].fe = dvb_attach(DVB-C)
>        adap->fe_adap[2].fe = dvb_attach(DVB-S)

Antti,

I don't understand exactly what you are proposing -- Is this a change
for the anysee driver?  ...or is it a change for the dvb-usb
framework?  ...or is it a change to dvb-core, and every driver in the
subsystem?

In the anysee driver, I see that you are using this:

.frontend_attach()
        if (adap->fe_adap[0].fe == NULL)
                adap->fe_adap[0].fe = dvb_attach(DVB-T)
        else if (adap->fe_adap[1].fe == NULL)
                adap->fe_adap[1].fe = dvb_attach(DVB-C)
        else if (adap->fe_adap[2].fe == NULL)
                adap->fe_adap[2].fe = dvb_attach(DVB-S)

I have no problem if you want to change the anysee driver to remove
the second dvb_usb_adap_fe_props context, and replace with the
following:


.frontend_attach()
       adap->fe_adap[0].fe = dvb_attach(DVB-T)
       adap->fe_adap[1].fe = dvb_attach(DVB-C)
       adap->fe_adap[2].fe = dvb_attach(DVB-S)

I believe this will work in the anysee driver for you, even with my
changes that got merged yesterday... However, I do *not* believe that
such change should propogate to the dvb-usb framework or dvb-core
itself, because it can have a large negative impact on the drivers
using it.

For example, my mxl111sf driver was merged yesterday.  Since it is the
initial driver merge, it currently only supports one frontend (ATSC).
The device also supports two other delivery systems, and has two
additional dtv demodulators, each attached via a separate input bus to
the USB device, each streaming on a separate USB endpoint.

Many demod drivers do an ID test or some other kind of initialization
during the _attach() function.  A device like the mxl111sf would have
to manipulate the USB device state and alter the bus operations before
and after each frontend attachment in order for the _attach() calls to
succeed, not to mention the separate calls needed for bus negotiation
to power on the correct demodulator and initialize its streaming data
path.

I repeat, if this is a change that is specific to your anysee driver,
then it seems like a good idea to me.  However, if your plan is to
change dvb-usb itself, and / or dvb-core, then I'd really like to have
a better idea of the implications that this change will bring forth.

So, to help reduce the confusion, can you clarify exactly what code
you plan to change, and what impact it will have on the drivers that
exist today?

Best Regards,

Michael Krufky

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Michael Krufky]

On Wed, Sep 7, 2011 at 1:41 PM, Michael Krufky <mkrufky@kernellabs.com> wrote:
> On Wed, Sep 7, 2011 at 12:51 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 08/01/2011 05:24 AM, Mauro Carvalho Chehab wrote:
>>>
>>> Em 31-07-2011 22:22, Antti Palosaari escreveu:
>>>>
>>>> On 08/01/2011 03:46 AM, Mauro Carvalho Chehab wrote:
>>>>>
>>>>> One bad thing I noticed with the API is that it calls
>>>>> adap->props.frontend_attach(adap)
>>>>> several times, instead of just one, without even passing an argument for
>>>>> the driver to
>>>>> know that it was called twice.
>>>>>
>>>>> IMO, there are two ways of doing the attach:
>>>>>
>>>>> 1) call it only once, and, inside the driver, it will loop to add the
>>>>> other FE's;
>>>>> 2) add a parameter, at the call, to say what FE needs to be initialized.
>>>>>
>>>>> I think (1) is preferred, as it is more flexible, allowing the driver to
>>>>> test for
>>>>> several types of frontends.
>>
>> I am planning to change DVB USB MFE call .frontend_attach() only once. Is
>> there any comments about that?
>>
>> Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and change
>> is needed ASAP before more MFE devices are coming.
>>
>> Also .num_frontends can be removed after that, since DVB USB will just loop
>> through 0 to MAX FEs and register all FEs found (fe pointer !NULL).
>>
>> CURRENTLY:
>> ==========
>> .frontend_attach()
>>        if (adap->fe_adap[0].fe == NULL)
>>                adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>        else if (adap->fe_adap[1].fe == NULL)
>>                adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>        else if (adap->fe_adap[2].fe == NULL)
>>                adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>
>> PLANNED:
>> ========
>> .frontend_attach()
>>        adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>        adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>        adap->fe_adap[2].fe = dvb_attach(DVB-S)
>
> Antti,
>
> I don't understand exactly what you are proposing -- Is this a change
> for the anysee driver?  ...or is it a change for the dvb-usb
> framework?  ...or is it a change to dvb-core, and every driver in the
> subsystem?
>
> In the anysee driver, I see that you are using this:
>
> .frontend_attach()
>         if (adap->fe_adap[0].fe == NULL)
>                 adap->fe_adap[0].fe = dvb_attach(DVB-T)
>         else if (adap->fe_adap[1].fe == NULL)
>                 adap->fe_adap[1].fe = dvb_attach(DVB-C)
>         else if (adap->fe_adap[2].fe == NULL)
>                 adap->fe_adap[2].fe = dvb_attach(DVB-S)
>
> I have no problem if you want to change the anysee driver to remove
> the second dvb_usb_adap_fe_props context, and replace with the
> following:
>
>
> .frontend_attach()
>        adap->fe_adap[0].fe = dvb_attach(DVB-T)
>        adap->fe_adap[1].fe = dvb_attach(DVB-C)
>        adap->fe_adap[2].fe = dvb_attach(DVB-S)
>
> I believe this will work in the anysee driver for you, even with my
> changes that got merged yesterday... However, I do *not* believe that
> such change should propogate to the dvb-usb framework or dvb-core
> itself, because it can have a large negative impact on the drivers
> using it.
>
> For example, my mxl111sf driver was merged yesterday.  Since it is the
> initial driver merge, it currently only supports one frontend (ATSC).
> The device also supports two other delivery systems, and has two
> additional dtv demodulators, each attached via a separate input bus to
> the USB device, each streaming on a separate USB endpoint.
>
> Many demod drivers do an ID test or some other kind of initialization
> during the _attach() function.  A device like the mxl111sf would have
> to manipulate the USB device state and alter the bus operations before
> and after each frontend attachment in order for the _attach() calls to
> succeed, not to mention the separate calls needed for bus negotiation
> to power on the correct demodulator and initialize its streaming data
> path.
>
> I repeat, if this is a change that is specific to your anysee driver,
> then it seems like a good idea to me.  However, if your plan is to
> change dvb-usb itself, and / or dvb-core, then I'd really like to have
> a better idea of the implications that this change will bring forth.
>
> So, to help reduce the confusion, can you clarify exactly what code
> you plan to change, and what impact it will have on the drivers that
> exist today?
>
> Best Regards,
>
> Michael Krufky
>

ADDENDUM:

For the anysee driver, for your single .frontend_attach()
       adap->fe_adap[0].fe = dvb_attach(DVB-T)
       adap->fe_adap[1].fe = dvb_attach(DVB-C)
       adap->fe_adap[2].fe = dvb_attach(DVB-S)

...for this to work in today's dvb-usb code without modification to
the dvb-usb framework, i believe that you should do a test for
(adap->fe_adap[0].fe && adap->fe_adap[1].fe && adap->fe_adap[2].fe )
... if null, then attach, if not null, then exit -- this will prevent
the second context's initialization from occurring twice.

Regards,

Mike

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Michael Krufky]

>> On Wed, Sep 7, 2011 at 12:51 PM, Antti Palosaari <crope@iki.fi> wrote:
>>> Also .num_frontends can be removed after that, since DVB USB will just loop
>>> through 0 to MAX FEs and register all FEs found (fe pointer !NULL).

We need to keep .num_frontends and .num_adapters both, because my next
change to dvb-usb is to convert the hard-sized array of struct
dvb_usb_adapter[MAX_NO_OF_ADAPTER_PER_DEVICE] and struct
dvb_usb_fe_adapter[MAX_NO_OF_FE_PER_ADAP] to a dynamic-allocated array
of pointers, to reduce the size of *all* dvb-usb drivers.

I didn't push all changes yet because I thought we should test each
change for a few days before merging too much all at once.  I wanted
to prevent the introduction of instability by making this a gradual
change so we can test things one by one.

Should I wait a bit before making the conversion of the hardcoded
sized-arrays to dynamic sized property pointers?

Regards,

Mike

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari]

On 09/07/2011 08:45 PM, Michael Krufky wrote:
> On Wed, Sep 7, 2011 at 1:41 PM, Michael Krufky<mkrufky@kernellabs.com>  wrote:
>> On Wed, Sep 7, 2011 at 12:51 PM, Antti Palosaari<crope@iki.fi>  wrote:
>>> On 08/01/2011 05:24 AM, Mauro Carvalho Chehab wrote:
>>>>
>>>> Em 31-07-2011 22:22, Antti Palosaari escreveu:
>>>>>
>>>>> On 08/01/2011 03:46 AM, Mauro Carvalho Chehab wrote:
>>>>>>
>>>>>> One bad thing I noticed with the API is that it calls
>>>>>> adap->props.frontend_attach(adap)
>>>>>> several times, instead of just one, without even passing an argument for
>>>>>> the driver to
>>>>>> know that it was called twice.
>>>>>>
>>>>>> IMO, there are two ways of doing the attach:
>>>>>>
>>>>>> 1) call it only once, and, inside the driver, it will loop to add the
>>>>>> other FE's;
>>>>>> 2) add a parameter, at the call, to say what FE needs to be initialized.
>>>>>>
>>>>>> I think (1) is preferred, as it is more flexible, allowing the driver to
>>>>>> test for
>>>>>> several types of frontends.
>>>
>>> I am planning to change DVB USB MFE call .frontend_attach() only once. Is
>>> there any comments about that?
>>>
>>> Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and change
>>> is needed ASAP before more MFE devices are coming.
>>>
>>> Also .num_frontends can be removed after that, since DVB USB will just loop
>>> through 0 to MAX FEs and register all FEs found (fe pointer !NULL).
>>>
>>> CURRENTLY:
>>> ==========
>>> .frontend_attach()
>>>         if (adap->fe_adap[0].fe == NULL)
>>>                 adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>>         else if (adap->fe_adap[1].fe == NULL)
>>>                 adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>>         else if (adap->fe_adap[2].fe == NULL)
>>>                 adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>>
>>> PLANNED:
>>> ========
>>> .frontend_attach()
>>>         adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>>         adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>>         adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>
>> Antti,
>>
>> I don't understand exactly what you are proposing -- Is this a change
>> for the anysee driver?  ...or is it a change for the dvb-usb
>> framework?  ...or is it a change to dvb-core, and every driver in the
>> subsystem?
>>
>> In the anysee driver, I see that you are using this:
>>
>> .frontend_attach()
>>          if (adap->fe_adap[0].fe == NULL)
>>                  adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>          else if (adap->fe_adap[1].fe == NULL)
>>                  adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>          else if (adap->fe_adap[2].fe == NULL)
>>                  adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>
>> I have no problem if you want to change the anysee driver to remove
>> the second dvb_usb_adap_fe_props context, and replace with the
>> following:
>>
>>
>> .frontend_attach()
>>         adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>         adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>         adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>
>> I believe this will work in the anysee driver for you, even with my
>> changes that got merged yesterday... However, I do *not* believe that
>> such change should propogate to the dvb-usb framework or dvb-core
>> itself, because it can have a large negative impact on the drivers
>> using it.
>>
>> For example, my mxl111sf driver was merged yesterday.  Since it is the
>> initial driver merge, it currently only supports one frontend (ATSC).
>> The device also supports two other delivery systems, and has two
>> additional dtv demodulators, each attached via a separate input bus to
>> the USB device, each streaming on a separate USB endpoint.
>>
>> Many demod drivers do an ID test or some other kind of initialization
>> during the _attach() function.  A device like the mxl111sf would have
>> to manipulate the USB device state and alter the bus operations before
>> and after each frontend attachment in order for the _attach() calls to
>> succeed, not to mention the separate calls needed for bus negotiation
>> to power on the correct demodulator and initialize its streaming data
>> path.
>>
>> I repeat, if this is a change that is specific to your anysee driver,
>> then it seems like a good idea to me.  However, if your plan is to
>> change dvb-usb itself, and / or dvb-core, then I'd really like to have
>> a better idea of the implications that this change will bring forth.
>>
>> So, to help reduce the confusion, can you clarify exactly what code
>> you plan to change, and what impact it will have on the drivers that
>> exist today?
>>
>> Best Regards,
>>
>> Michael Krufky
>>
>
> ADDENDUM:
>
> For the anysee driver, for your single .frontend_attach()
>         adap->fe_adap[0].fe = dvb_attach(DVB-T)
>         adap->fe_adap[1].fe = dvb_attach(DVB-C)
>         adap->fe_adap[2].fe = dvb_attach(DVB-S)
>
> ...for this to work in today's dvb-usb code without modification to
> the dvb-usb framework, i believe that you should do a test for
> (adap->fe_adap[0].fe&&  adap->fe_adap[1].fe&&  adap->fe_adap[2].fe )
> ... if null, then attach, if not null, then exit -- this will prevent
> the second context's initialization from occurring twice.

Yes, I now saw when looked latest anysee driver that you moved 
.streaming_ctrl(), .frontend_attach() and .tuner_attach() to frontend 
property. OK, it is not then relevant anymore to change register all as 
once.

What is size_of_priv used?

regards
Antti




-- 
http://palosaari.fi/

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Michael Krufky]

On Wed, Sep 7, 2011 at 2:20 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 09/07/2011 08:45 PM, Michael Krufky wrote:
>>
>> On Wed, Sep 7, 2011 at 1:41 PM, Michael Krufky<mkrufky@kernellabs.com>
>>  wrote:
>>>
>>> On Wed, Sep 7, 2011 at 12:51 PM, Antti Palosaari<crope@iki.fi>  wrote:
>>>>
>>>> On 08/01/2011 05:24 AM, Mauro Carvalho Chehab wrote:
>>>>>
>>>>> Em 31-07-2011 22:22, Antti Palosaari escreveu:
>>>>>>
>>>>>> On 08/01/2011 03:46 AM, Mauro Carvalho Chehab wrote:
>>>>>>>
>>>>>>> One bad thing I noticed with the API is that it calls
>>>>>>> adap->props.frontend_attach(adap)
>>>>>>> several times, instead of just one, without even passing an argument
>>>>>>> for
>>>>>>> the driver to
>>>>>>> know that it was called twice.
>>>>>>>
>>>>>>> IMO, there are two ways of doing the attach:
>>>>>>>
>>>>>>> 1) call it only once, and, inside the driver, it will loop to add the
>>>>>>> other FE's;
>>>>>>> 2) add a parameter, at the call, to say what FE needs to be
>>>>>>> initialized.
>>>>>>>
>>>>>>> I think (1) is preferred, as it is more flexible, allowing the driver
>>>>>>> to
>>>>>>> test for
>>>>>>> several types of frontends.
>>>>
>>>> I am planning to change DVB USB MFE call .frontend_attach() only once.
>>>> Is
>>>> there any comments about that?
>>>>
>>>> Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and
>>>> change
>>>> is needed ASAP before more MFE devices are coming.
>>>>
>>>> Also .num_frontends can be removed after that, since DVB USB will just
>>>> loop
>>>> through 0 to MAX FEs and register all FEs found (fe pointer !NULL).
>>>>
>>>> CURRENTLY:
>>>> ==========
>>>> .frontend_attach()
>>>>        if (adap->fe_adap[0].fe == NULL)
>>>>                adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>>>        else if (adap->fe_adap[1].fe == NULL)
>>>>                adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>>>        else if (adap->fe_adap[2].fe == NULL)
>>>>                adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>>>
>>>> PLANNED:
>>>> ========
>>>> .frontend_attach()
>>>>        adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>>>        adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>>>        adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>>
>>> Antti,
>>>
>>> I don't understand exactly what you are proposing -- Is this a change
>>> for the anysee driver?  ...or is it a change for the dvb-usb
>>> framework?  ...or is it a change to dvb-core, and every driver in the
>>> subsystem?
>>>
>>> In the anysee driver, I see that you are using this:
>>>
>>> .frontend_attach()
>>>         if (adap->fe_adap[0].fe == NULL)
>>>                 adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>>         else if (adap->fe_adap[1].fe == NULL)
>>>                 adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>>         else if (adap->fe_adap[2].fe == NULL)
>>>                 adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>>
>>> I have no problem if you want to change the anysee driver to remove
>>> the second dvb_usb_adap_fe_props context, and replace with the
>>> following:
>>>
>>>
>>> .frontend_attach()
>>>        adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>>        adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>>        adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>>
>>> I believe this will work in the anysee driver for you, even with my
>>> changes that got merged yesterday... However, I do *not* believe that
>>> such change should propogate to the dvb-usb framework or dvb-core
>>> itself, because it can have a large negative impact on the drivers
>>> using it.
>>>
>>> For example, my mxl111sf driver was merged yesterday.  Since it is the
>>> initial driver merge, it currently only supports one frontend (ATSC).
>>> The device also supports two other delivery systems, and has two
>>> additional dtv demodulators, each attached via a separate input bus to
>>> the USB device, each streaming on a separate USB endpoint.
>>>
>>> Many demod drivers do an ID test or some other kind of initialization
>>> during the _attach() function.  A device like the mxl111sf would have
>>> to manipulate the USB device state and alter the bus operations before
>>> and after each frontend attachment in order for the _attach() calls to
>>> succeed, not to mention the separate calls needed for bus negotiation
>>> to power on the correct demodulator and initialize its streaming data
>>> path.
>>>
>>> I repeat, if this is a change that is specific to your anysee driver,
>>> then it seems like a good idea to me.  However, if your plan is to
>>> change dvb-usb itself, and / or dvb-core, then I'd really like to have
>>> a better idea of the implications that this change will bring forth.
>>>
>>> So, to help reduce the confusion, can you clarify exactly what code
>>> you plan to change, and what impact it will have on the drivers that
>>> exist today?
>>>
>>> Best Regards,
>>>
>>> Michael Krufky
>>>
>>
>> ADDENDUM:
>>
>> For the anysee driver, for your single .frontend_attach()
>>        adap->fe_adap[0].fe = dvb_attach(DVB-T)
>>        adap->fe_adap[1].fe = dvb_attach(DVB-C)
>>        adap->fe_adap[2].fe = dvb_attach(DVB-S)
>>
>> ...for this to work in today's dvb-usb code without modification to
>> the dvb-usb framework, i believe that you should do a test for
>> (adap->fe_adap[0].fe&&  adap->fe_adap[1].fe&&  adap->fe_adap[2].fe )
>> ... if null, then attach, if not null, then exit -- this will prevent
>> the second context's initialization from occurring twice.
>
> Yes, I now saw when looked latest anysee driver that you moved
> .streaming_ctrl(), .frontend_attach() and .tuner_attach() to frontend
> property. OK, it is not then relevant anymore to change register all as
> once.
>
> What is size_of_priv used?

size_of_priv is a signal to the dvb-usb framework to allocate memory
of size, size_of_priv to track device state at a given context.  If
you look in dvb-usb.h, there was always a size_of_priv / void *priv at
the dvb_usb_device context level, and there was always a size_of_priv
/ void *priv at the dvb_usb_adapter context level.   After my MFE
patch, there is now a size_of_priv / void *priv at the
dvb_usb_fe_adapter context level.  This private state structure is
used to track state at the context of each dvb_usb_fe_adap, to manage
the environment needed to switch between the various attached
frontends.  You may take a look in mxl111sf.c to see how this is used
(ie, struct mxl111sf_adap_state *adap_state =
adap->fe_adap[fe->id].priv;)

If size_of_priv is left undefined, it is initialized to 0, and the
void *priv pointer remains null.

Regards,

Mike Krufky

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari]

On 09/07/2011 09:36 PM, Michael Krufky wrote:
> On Wed, Sep 7, 2011 at 2:20 PM, Antti Palosaari<crope@iki.fi>  wrote:

>> Yes, I now saw when looked latest anysee driver that you moved
>> .streaming_ctrl(), .frontend_attach() and .tuner_attach() to frontend
>> property. OK, it is not then relevant anymore to change register all as
>> once.
>>
>> What is size_of_priv used?
>
> size_of_priv is a signal to the dvb-usb framework to allocate memory
> of size, size_of_priv to track device state at a given context.  If
> you look in dvb-usb.h, there was always a size_of_priv / void *priv at
> the dvb_usb_device context level, and there was always a size_of_priv
> / void *priv at the dvb_usb_adapter context level.   After my MFE
> patch, there is now a size_of_priv / void *priv at the
> dvb_usb_fe_adapter context level.  This private state structure is
> used to track state at the context of each dvb_usb_fe_adap, to manage
> the environment needed to switch between the various attached
> frontends.  You may take a look in mxl111sf.c to see how this is used
> (ie, struct mxl111sf_adap_state *adap_state =
> adap->fe_adap[fe->id].priv;)
>
> If size_of_priv is left undefined, it is initialized to 0, and the
> void *priv pointer remains null.

I marvel at there was 3 states, one for device, one for each adapter and 
now even one for each frontend. Surely enough, generally only device 
state is used. And your new driver seems to even use that new FE priv added.

regards
Antti


-- 
http://palosaari.fi/

Re: [PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Michael Krufky]

On Wed, Sep 7, 2011 at 5:10 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 09/07/2011 09:36 PM, Michael Krufky wrote:
>>
>> On Wed, Sep 7, 2011 at 2:20 PM, Antti Palosaari<crope@iki.fi>  wrote:
>
>>> Yes, I now saw when looked latest anysee driver that you moved
>>> .streaming_ctrl(), .frontend_attach() and .tuner_attach() to frontend
>>> property. OK, it is not then relevant anymore to change register all as
>>> once.
>>>
>>> What is size_of_priv used?
>>
>> size_of_priv is a signal to the dvb-usb framework to allocate memory
>> of size, size_of_priv to track device state at a given context.  If
>> you look in dvb-usb.h, there was always a size_of_priv / void *priv at
>> the dvb_usb_device context level, and there was always a size_of_priv
>> / void *priv at the dvb_usb_adapter context level.   After my MFE
>> patch, there is now a size_of_priv / void *priv at the
>> dvb_usb_fe_adapter context level.  This private state structure is
>> used to track state at the context of each dvb_usb_fe_adap, to manage
>> the environment needed to switch between the various attached
>> frontends.  You may take a look in mxl111sf.c to see how this is used
>> (ie, struct mxl111sf_adap_state *adap_state =
>> adap->fe_adap[fe->id].priv;)
>>
>> If size_of_priv is left undefined, it is initialized to 0, and the
>> void *priv pointer remains null.
>
> I marvel at there was 3 states, one for device, one for each adapter and now
> even one for each frontend. Surely enough, generally only device state is
> used. And your new driver seems to even use that new FE priv added.

My new driver requires state tracking at the dvb_usb_fe_adapter
context level, but it does *not* require state tracking at the
dvb_usb_adapter level.  The driver also has state tracking at the
dvb_usb_device context level.  It needs this tracking at both levels,
but not at the middle adapter level.

dib0700, however, requires state tracking at the dvb_usb_adapter
context level and it also requires state tracking at the
dvb_usb_device context level.  Most devices that have multiple
adapters follow this same schema for tracking state within various
context levels.

The private state tracking structures are allocated dynamically as
needed, and only if size_of_priv is defined.  Device property contexts
that do not define size_of_priv simply do not allocate any additional
memory for state tracking.

Having the ability to track private state within each context level
gives the dvb-usb framework the maximum flexibility to work with
various styles of both simple and complex digital media receiver
devices.

Regards,

Mike Krufky

[PATCH 2/3] dvb-usb: multi-frontend support (MFE)

[Antti Palosaari]

Signed-off-by: Antti Palosaari <crope@iki.fi>
---
  drivers/media/dvb/dvb-usb/dvb-usb-dvb.c  |   85 
+++++++++++++++++++++++-------
  drivers/media/dvb/dvb-usb/dvb-usb-init.c |    4 ++
  drivers/media/dvb/dvb-usb/dvb-usb.h      |   11 +++-
  3 files changed, 78 insertions(+), 22 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
index d8c0bd9..5e34df7 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
@@ -162,8 +162,11 @@ static int dvb_usb_fe_wakeup(struct dvb_frontend *fe)

  	dvb_usb_device_power_ctrl(adap->dev, 1);

-	if (adap->fe_init)
-		adap->fe_init(fe);
+	if (adap->props.frontend_ctrl)
+		adap->props.frontend_ctrl(fe, 1);
+
+	if (adap->fe_init[fe->id])
+		adap->fe_init[fe->id](fe);

  	return 0;
  }
@@ -172,45 +175,89 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
  {
  	struct dvb_usb_adapter *adap = fe->dvb->priv;

-	if (adap->fe_sleep)
-		adap->fe_sleep(fe);
+	if (adap->fe_sleep[fe->id])
+		adap->fe_sleep[fe->id](fe);
+
+	if (adap->props.frontend_ctrl)
+		adap->props.frontend_ctrl(fe, 0);

  	return dvb_usb_device_power_ctrl(adap->dev, 0);
  }

  int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
  {
+	int ret, i, x;
+
+	memset(adap->fe, 0, sizeof(adap->fe));
+
  	if (adap->props.frontend_attach == NULL) {
-		err("strange: '%s' #%d doesn't want to attach a 
frontend.",adap->dev->desc->name, adap->id);
+		err("strange: '%s' #%d doesn't want to attach a frontend.",
+			adap->dev->desc->name, adap->id);
+
  		return 0;
  	}

-	/* re-assign sleep and wakeup functions */
-	if (adap->props.frontend_attach(adap) == 0 && adap->fe[0] != NULL) {
-		adap->fe_init  = adap->fe[0]->ops.init;  adap->fe[0]->ops.init  = 
dvb_usb_fe_wakeup;
-		adap->fe_sleep = adap->fe[0]->ops.sleep; adap->fe[0]->ops.sleep = 
dvb_usb_fe_sleep;
+	/* register all given adapter frontends */
+	if (adap->props.num_frontends)
+		x = adap->props.num_frontends - 1;
+	else
+		x = 0;
+
+	for (i = 0; i <= x; i++) {
+		ret = adap->props.frontend_attach(adap);
+		if (ret || adap->fe[i] == NULL) {
+			/* only print error when there is no FE at all */
+			if (i == 0)
+				err("no frontend was attached by '%s'",
+					adap->dev->desc->name);
+
+			return 0;
+		}

-		if (dvb_register_frontend(&adap->dvb_adap, adap->fe[0])) {
-			err("Frontend registration failed.");
-			dvb_frontend_detach(adap->fe[0]);
-			adap->fe[0] = NULL;
-			return -ENODEV;
+		adap->fe[i]->id = i;
+
+		/* re-assign sleep and wakeup functions */
+		adap->fe_init[i] = adap->fe[i]->ops.init;
+		adap->fe[i]->ops.init  = dvb_usb_fe_wakeup;
+		adap->fe_sleep[i] = adap->fe[i]->ops.sleep;
+		adap->fe[i]->ops.sleep = dvb_usb_fe_sleep;
+
+		if (dvb_register_frontend(&adap->dvb_adap, adap->fe[i])) {
+			err("Frontend %d registration failed.", i);
+			dvb_frontend_detach(adap->fe[i]);
+			adap->fe[i] = NULL;
+			/* In error case, do not try register more FEs,
+			 * still leaving already registered FEs alive. */
+			if (i == 0)
+				return -ENODEV;
+			else
+				return 0;
  		}

  		/* only attach the tuner if the demod is there */
  		if (adap->props.tuner_attach != NULL)
  			adap->props.tuner_attach(adap);
-	} else
-		err("no frontend was attached by '%s'",adap->dev->desc->name);
+	}

  	return 0;
  }

  int dvb_usb_adapter_frontend_exit(struct dvb_usb_adapter *adap)
  {
-	if (adap->fe[0] != NULL) {
-		dvb_unregister_frontend(adap->fe[0]);
-		dvb_frontend_detach(adap->fe[0]);
+	int i;
+
+	/* unregister all given adapter frontends */
+	if (adap->props.num_frontends)
+		i = adap->props.num_frontends - 1;
+	else
+		i = 0;
+
+	for (; i >= 0; i--) {
+		if (adap->fe[i] != NULL) {
+			dvb_unregister_frontend(adap->fe[i]);
+			dvb_frontend_detach(adap->fe[i]);
+		}
  	}
+
  	return 0;
  }
diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-init.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-init.c
index 2e3ea0f..f9af348 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-init.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-init.c
@@ -77,6 +77,10 @@ static int dvb_usb_adapter_init(struct dvb_usb_device 
*d, short *adapter_nrs)
  			return ret;
  		}

+		/* use exclusive FE lock if there is multiple shared FEs */
+		if (adap->fe[1])
+			adap->dvb_adap.mfe_shared = 1;
+
  		d->num_adapters_initialized++;
  		d->state |= DVB_USB_STATE_DVB;
  	}
diff --git a/drivers/media/dvb/dvb-usb/dvb-usb.h 
b/drivers/media/dvb/dvb-usb/dvb-usb.h
index 2e57bff..a3e77b2 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb.h
+++ b/drivers/media/dvb/dvb-usb/dvb-usb.h
@@ -124,6 +124,8 @@ struct usb_data_stream_properties {
   * @caps: capabilities of the DVB USB device.
   * @pid_filter_count: number of PID filter position in the optional 
hardware
   *  PID-filter.
+ * @num_frontends: number of frontends of the DVB USB adapter.
+ * @frontend_ctrl: called to power on/off active frontend.
   * @streaming_ctrl: called to start and stop the MPEG2-TS streaming of the
   *  device (not URB submitting/killing).
   * @pid_filter_ctrl: called to en/disable the PID filter, if any.
@@ -141,7 +143,9 @@ struct dvb_usb_adapter_properties {
  #define DVB_USB_ADAP_RECEIVES_204_BYTE_TS         0x08
  	int caps;
  	int pid_filter_count;
+	int num_frontends;

+	int (*frontend_ctrl)   (struct dvb_frontend *, int);
  	int (*streaming_ctrl)  (struct dvb_usb_adapter *, int);
  	int (*pid_filter_ctrl) (struct dvb_usb_adapter *, int);
  	int (*pid_filter)      (struct dvb_usb_adapter *, int, u16, int);
@@ -345,6 +349,7 @@ struct usb_data_stream {
   *
   * @stream: the usb data stream.
   */
+#define MAX_NO_OF_FE_PER_ADAP 2
  struct dvb_usb_adapter {
  	struct dvb_usb_device *dev;
  	struct dvb_usb_adapter_properties props;
@@ -363,11 +368,11 @@ struct dvb_usb_adapter {
  	struct dmxdev        dmxdev;
  	struct dvb_demux     demux;
  	struct dvb_net       dvb_net;
-	struct dvb_frontend *fe[1];
+	struct dvb_frontend *fe[MAX_NO_OF_FE_PER_ADAP];
  	int                  max_feed_count;

-	int (*fe_init)  (struct dvb_frontend *);
-	int (*fe_sleep) (struct dvb_frontend *);
+	int (*fe_init[MAX_NO_OF_FE_PER_ADAP])  (struct dvb_frontend *);
+	int (*fe_sleep[MAX_NO_OF_FE_PER_ADAP]) (struct dvb_frontend *);

  	struct usb_data_stream stream;

-- 
1.7.6