[2/4] rtl28xxu: swap frontend order for devices with slave demodulators

  Signed-off-by: Benjamin Larsson <benjamin@southpole.se>
---
 drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
  

I am not sure even idea of that. You didn't add even commit description, 
like all the other patches too :( You should really start adding commit 
messages explaining why and how commit is.

So the question is why that patch should be applied?

On the other-hand, how there is
if (fe->id == 1 && onoff) {
... as I don't remember any patch changing it to 0. I look my tree FE ID 
is 0. Do you have some unpublished hacks?

Antti


On 12/13/2014 02:18 AM, Benjamin Larsson wrote:
> Signed-off-by: Benjamin Larsson <benjamin@southpole.se>
> ---
>   drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> index ab48b5f..cdc342a 100644
> --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> @@ -863,6 +863,7 @@ static int rtl2832u_frontend_attach(struct dvb_usb_adapter *adap)
>
>   		/* attach slave demodulator */
>   		if (priv->slave_demod == SLAVE_DEMOD_MN88472) {
> +			struct dvb_frontend *tmp_fe;
>   			struct mn88472_config mn88472_config = {};
>
>   			mn88472_config.fe = &adap->fe[1];
> @@ -887,6 +888,12 @@ static int rtl2832u_frontend_attach(struct dvb_usb_adapter *adap)
>   			}
>
>   			priv->i2c_client_slave_demod = client;
> +
> +			/* Swap frontend order */
> +			tmp_fe = adap->fe[0];
> +			adap->fe[0] = adap->fe[1];
> +			adap->fe[1] = tmp_fe;
> +
>   		} else {
>   			struct mn88473_config mn88473_config = {};
>
> @@ -1373,7 +1380,7 @@ static int rtl2832u_frontend_ctrl(struct dvb_frontend *fe, int onoff)
>
>   	/* bypass slave demod TS through master demod */
>   	if (fe->id == 1 && onoff) {
> -		ret = rtl2832_enable_external_ts_if(adap->fe[0]);
> +		ret = rtl2832_enable_external_ts_if(adap->fe[1]);
>   		if (ret)
>   			goto err;
>   	}
>
  

On 12/13/2014 05:02 AM, Antti Palosaari wrote:
> I am not sure even idea of that. You didn't add even commit 
> description, like all the other patches too :( You should really start 
> adding commit messages explaining why and how commit is.
>
> So the question is why that patch should be applied?

Lots of legacy applications doesn't set the frontend number and use 0 by 
default. For me to use w_scan I need this change. If that is reason good 
enough I can amend that to the commit message and resend?

>
> On the other-hand, how there is
> if (fe->id == 1 && onoff) {
> ... as I don't remember any patch changing it to 0. I look my tree FE 
> ID is 0. Do you have some unpublished hacks?

No hacks, it works for me that way.

>
> Antti
MvH
Benjamin Larsson


--
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
  

On 12/13/2014 01:09 PM, Benjamin Larsson wrote:
> On 12/13/2014 05:02 AM, Antti Palosaari wrote:
>> I am not sure even idea of that. You didn't add even commit
>> description, like all the other patches too :( You should really start
>> adding commit messages explaining why and how commit is.
>>
>> So the question is why that patch should be applied?
>
> Lots of legacy applications doesn't set the frontend number and use 0 by
> default. For me to use w_scan I need this change. If that is reason good
> enough I can amend that to the commit message and resend?
>
>>
>> On the other-hand, how there is
>> if (fe->id == 1 && onoff) {
>> ... as I don't remember any patch changing it to 0. I look my tree FE
>> ID is 0. Do you have some unpublished hacks?
>
> No hacks, it works for me that way.

Do you understand that code at all?

Now it is:
FE0 == (fe->id == 0) == RTL2832
FE1 == (fe->id == 1) == MN88472

you changed it to:
FE0 == (fe->id == 0) == MN88472
FE1 == (fe->id == 1) == RTL2832

Then there is:

/* bypass slave demod TS through master demod */
if (fe->id == 1 && onoff) {
	ret = rtl2832_enable_external_ts_if(adap->fe[1]);
	if (ret)
		goto err;
}

After your change that code branch is taken when RTL2832 demod is 
activated / used. Shouldn't TS bypass enabled just opposite, when 
MN88472 is used....


Antti
  

On 12/13/2014 02:35 PM, Antti Palosaari wrote:
>
>
> Do you understand that code at all?

No I can't really say I understand all the workings of the media api.

>
> Now it is:
> FE0 == (fe->id == 0) == RTL2832
> FE1 == (fe->id == 1) == MN88472
>
> you changed it to:
> FE0 == (fe->id == 0) == MN88472
> FE1 == (fe->id == 1) == RTL2832

I thought the rtl2832u_frontend_attach() actually attached the devices. 
Then the id's would have followed the frontend.


>
> Then there is:
>
> /* bypass slave demod TS through master demod */
> if (fe->id == 1 && onoff) {
>     ret = rtl2832_enable_external_ts_if(adap->fe[1]);
>     if (ret)
>         goto err;
> }
>
> After your change that code branch is taken when RTL2832 demod is 
> activated / used. Shouldn't TS bypass enabled just opposite, when 
> MN88472 is used....
>
>
> Antti
>

This intent of the patch was for better backwards compatibility with old 
software. This isn't strictly needed so consider the patch dropped.

MvH
Benjamin Larsson
--
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