[linux-dvb] [PATCH] Multi protocol support (stage #1)

Manu Abraham abraham.manu at gmail.com
Thu Apr 27 02:17:19 CEST 2006


Johannes Stezenbach wrote:
> Hi Manu,
>
> How does this proposal relate to the DVB-S2 proposal by mws?
> Does it supersede it?
>
>   

I think Marcel (mws) also like this patch, apart from the comments you 
have. I had been working on the stb0899 + stb6100 (a multi standard 
device that supports DVB-S/S2 and DSS with a silicon tuner. It is now a 
lot done. hence i took a hand at the patch. It should be complete soon.

>> diff -Naurp multi-proto-orig/linux/drivers/media/dvb/dvb-core/dvb_frontend.c multi-proto2/linux/drivers/media/dvb/dvb-core/dvb_frontend.c
>> --- multi-proto-orig/linux/drivers/media/dvb/dvb-core/dvb_frontend.c    2006-04-20 20:56:39.000000000 +0400
>> +++ multi-proto2/linux/drivers/media/dvb/dvb-core/dvb_frontend.c        2006-04-25 03:46:55.000000000 +0400
>> @@ -94,6 +94,8 @@ struct dvb_frontend_private {
>>         /* thread/frontend values */
>>         struct dvb_device *dvbdev;
>>         struct dvb_frontend_parameters parameters;
>> +       struct dvb_frontend_params params;
>>     
>
> Bad naming -- make it clear which one is new / legacy.
>
>   

I will add in some comments.

>> +struct modcod_table dvbs2_modcod_lookup[] = {
>> +       { MOD_QPSK_1_4,         FE_MOD_QPSK,    FE_FECRATE_1_4  },
>>     
>
> I see this cruft is actually in the DVB-S2 standard. Ugh!
>
>   

It is not just in the standard alone, but in fact these values are 
directly returned from the STB0899 DSP itself. We can straight away get 
the modulation/code rates directly from it.


>> +int decode_dvbs2_modcod(struct dvb_frontend *fe, enum fe_modcod modcod)
>> +{
>> +       int i;
>> +
>> +       struct modcod_table *table;
>> +       struct dvb_frontend_private *fepriv = fe->frontend_priv;
>> +       struct dvb_frontend_params *params = &fepriv->params;
>> +
>> +       if (params->delivery & FE_DELSYS_DVBS2) {
>> +               for (i = 0, table = dvbs2_modcod_lookup;
>> +                       i < ARRAY_SIZE(dvbs2_modcod_lookup);
>> +                       i++, table++) {
>> +
>> +                       if (table->modcod == modcod) {
>> +                               params->delsys.dvbs2.modulation = table->modulation;
>> +                               params->delsys.dvbs2.fecrate = table->fecrate;
>> +                       }
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(decode_dvbs2_modcod);
>>     
>
> What's the loop for? Why not just make modcod an index into the array?
>
>   

yeah, right.

>> diff -Naurp multi-proto-orig/linux/drivers/media/dvb/dvb-core/dvb_frontend.h multi-proto2/linux/drivers/media/dvb/dvb-core/dvb_frontend.h
>> --- multi-proto-orig/linux/drivers/media/dvb/dvb-core/dvb_frontend.h    2006-04-20 20:56:39.000000000 +0400
>> +++ multi-proto2/linux/drivers/media/dvb/dvb-core/dvb_frontend.h        2006-04-25 03:30:38.000000000 +0400
>> @@ -61,6 +61,12 @@ struct dvb_tuner_info {
>>         u32 bandwidth_step;
>> };
>>
>> +struct modcod_table {
>> +       u32 modcod;
>> +       u32 modulation;
>> +       u32 fecrate;
>> +};
>> +
>>     
>
> Is used in implementation only -> move to .c file
>
>   

ok


>> diff -Naurp multi-proto-orig/linux/include/linux/dvb/frontend.h multi-proto2/linux/include/linux/dvb/frontend.h
>> --- multi-proto-orig/linux/include/linux/dvb/frontend.h 2006-04-20 20:56:40.000000000 +0400
>> +++ multi-proto2/linux/include/linux/dvb/frontend.h     2006-04-25 03:47:10.000000000 +0400
>> @@ -274,4 +274,273 @@ struct dvb_frontend_event {
>>
>> #define FE_DISHNETWORK_SEND_LEGACY_CMD _IO('o', 80) /* unsigned int */
>>
>> +/**
>> + *     Supported delivery systems
>> + *     some frontends are capable of supporting
>> + *     multiple delivery systems
>> + */
>> +enum fe_delsys {
>> +       FE_DELSYS_QUERY                 = 0x00000000,
>>     
>
> How is QUERY used?
>
>   

Initially we had only one MODCOD table from where everything was 
decoded, at that time Ralph suggested that we can use "0" to query to 
identify between multi standard devices, which was a very cool idea. 
Then someone came up with the thought that there wouldn't be enough 
space in the structure for the future, and hence now  we have split up 
the table. I think it would be better if we replace QUERY with IGNORE, 
ie FE_DELSYS_IGNORE etc, we can ask GET_PARAMS/CAPS not to retrieve that 
value. this will be helpful to avoid unnecessary I2C communication, 
which results in faster results.


>> +       FE_DELSYS_DVBS                  = 0x00000001,
>> +       FE_DELSYS_DVBS2                 = 0x00000002,
>> +       FE_DELSYS_DSS                   = 0x00000004,
>> +       FE_DELSYS_DVBC                  = 0x00000008,
>> +       FE_DELSYS_DVBT                  = 0x00000010,
>> +       FE_DELSYS_DVBH                  = 0x00000020,
>> +       FE_DELSYS_ATSC                  = 0x00000040,
>> +       FE_DELSYS_AUTO                  = 0x10000000
>>     
>
> What does AUTO mean here?
> Did you mean to use 0x80000000?
>
>   

Ah, yes. Human error.

> Generally, I prefer to write (1 << 31) etc., but that's
> a matter of taste.
>
>
>   

Ah, yes, since you pointed it out, will go that way, using the bitmasks 
only for the STB0899

>> +};
>> +
>> +enum fe_matype {
>> +       FE_MATYPE_QUERY                 = 0x00000000,
>> +       FE_TRANSPORT                    = 0x00000001,
>> +       FE_GENERIC_PACKET               = 0x00000002,
>> +       FE_GENERIC_CONTINUOUS           = 0x00000004,
>> +       FE_RESERVED                     = 0x00000008,
>> +       FE_MATYPE_AUTO                  = 0x10000000
>> +};
>>     
>
> I'm not sure if it makes sense to include this. Let's stick
> with MPEG2 TS untils someone can demonstrate the other
> modes are useful.
>
>   

Well, the Nyquist rolloff rates are based on these parameters, ie, a = 
0.20, a =0.25, a = 0.35

at the STB0899 side i have something like this , which maps to this like 
this ..

enum stb0899_rrc_alpha {
    STB0899_RRC_20            = 0x00000000,
    STB0899_RRC_25,
    STB0899_RRC_35
};

>> +struct dvb_frontend_cap {
>> +       enum fe_delsys                  delivery;
>> +       enum fe_inversion               inversion;
>> +
>> +       union {
>> +               struct dvbs_params      dvbs;
>> +               struct dvbs2_params     dvbs2;
>> +               struct dss_params       dss;
>> +               struct dvbc_params      dvbc;
>> +               struct dvbt_params      dvbt;
>> +               struct dvbh_params      dvbh;
>> +               struct atsc_params      atsc;
>> +       } delsys;
>> +};
>>     
>
> if this doesn't report capabilities then _cap is the wrong name
>
>   

I noticed it a little while back only. In fact the IOCTL naming is 
wrong. :-(
I will fix this.

>> +struct dvb_frontend_params {
>> +       __u32                           frequency;
>> +       enum fe_delsys                  delivery;
>> +       enum fe_inversion               inversion;
>> +
>> +       union {
>> +               struct dvbs_params      dvbs;
>> +               struct dvbs2_params     dvbs2;
>> +               struct dss_params       dss;
>> +               struct dvbc_params      dvbc;
>> +               struct dvbt_params      dvbt;
>> +               struct dvbh_params      dvbh;
>> +               struct atsc_params      atsc;
>> +       } delsys;
>>     
>
> The kernel now requires gcc-3.x, so you could use anonymous unions if
> you want.
> But you repeat the same mistake which we already made: No room
> for future binary compitable extension :-(
>
>   

What's your thought on removing the size field in the IOCTL, so that the 
size won't be an issue ?
That way it will be binary compatible. :-) Or any other thoughts you 
have ? We can retain the size for now, but we can remove it as and when 
need arises ..

>> +#define FE_SET_PARAMS                  _IOW('o', 81, struct dvb_frontend_params)
>> +#define FE_GET_PARAMS                  _IOWR('o', 82, struct dvb_frontend_cap)
>>     
>
> Again, the name doesn't make it clear which is new / legacy.
> It wouldn't hurt to add a short comment, too.
>
>   

Ah, i will update it up with the changes. Thanks for the comments.


Thanks,
Manu





More information about the linux-dvb mailing list