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
_______________________________________________
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
