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

[Johannes Stezenbach]

Hi Manu,

How does this proposal relate to the DVB-S2 proposal by mws?
Does it supersede it?

On Tue, Apr 25, 2006, Manu Abraham wrote:

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

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

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

> 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

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

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

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


> +};
> +
> +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.

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

> +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 :-(

> +#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.

> #endif /*_DVBFRONTEND_H_*/



Johannes