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

Manu Abraham abraham.manu at gmail.com
Sat May 6 03:26:52 CEST 2006


Hello Johannes,

Johannes Stezenbach wrote:
> Hi Manu,
>
> lots of minor comments from my side. Don't take them
> too seriously if you disagree with my opinion, just
> address the ones where you think I'm obviously right ;-)
>
>   

No issues, i have been taking in a lot of beatings these days, so some 
additionally won't make any much difference.
Anyway, Lengthy mail follows. I know you hate lengthy mails, but 
couldn't make it shorter. :-)

> On Fri, May 05, 2006, Manu Abraham wrote:
>   
>> --- multiproto3.orig/linux/drivers/media/dvb/dvb-core/dvb_frontend.c	2006-04-29 03:22:23.000000000 +0400
>> +++ multiproto3/linux/drivers/media/dvb/dvb-core/dvb_frontend.c	2006-04-29 21:39:59.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;
>>     
>
> What is the reason for keeping parameters?
> It is superseded by params, isn't it?
>
>   

Yes, currently parameters( the legacy one) is used by all drivers. 
Removing it straight away will result in fixing all drivers straight 
away. Many drivers are yet to recover from the tuner refactoring yet, at 
this point of time if we remove parameters and make params the default 
one, it will be a doom for sure. We should define a period by which the 
drivers should be ported to the new operations.


> Generally, what is the plan?
> - change all frontend drivers to use set_params/get_params
>   and struct dvb_frontend_params?
>   

Yes. This should be the task, but we should maintain parameters, till we 
fix in the drivers.

> - or map FE_SET_PARAMS / FE_GET_PARAMS to set_frontend / get_frontend
>   for old frontend drivers?
>
> Please add comments to mark deprecated or alternative old/new parts.
>
> params vs. parameters naming without comment is a recipe for confusion.
>
>   

I thought i already did, marking them Legacy and New

>> +	struct dvb_frontend_cap caps;
>>     
>
> why do we need this here?
>
>   
>> +struct modcod_table {
>> +	__u32 modcod;
>> +	__u32 modulation;
>> +	__u32 fecrate;
>>     
>
> should use u32, not __u32
>
>   

Right, i did follow LKML the previous days.

>> +};
>> +
>> +struct modcod_table dvbs2_modcod_lookup[] = {
>>     
>
> should be static
>
>   

Right.

>> +	{ FE_MODCOD_DUMMY_PLFRAME,	FE_MOD_NONE,	FE_FECRATE_NONE	},
>> +	{ FE_MODCOD_QPSK_1_4,		FE_MOD_QPSK,	FE_FECRATE_1_4	},
>> +	{ FE_MODCOD_QPSK_1_3,		FE_MOD_QPSK,	FE_FECRATE_1_3	},
>> +	{ FE_MODCOD_QPSK_2_5,		FE_MOD_QPSK,	FE_FECRATE_2_5	},
>> +	{ FE_MODCOD_QPSK_1_2,		FE_MOD_QPSK,	FE_FECRATE_1_2	},
>> +	{ FE_MODCOD_QPSK_3_5,		FE_MOD_QPSK,	FE_FECRATE_3_5	},
>> +	{ FE_MODCOD_QPSK_2_3,		FE_MOD_QPSK,	FE_FECRATE_2_3	},
>> +	{ FE_MODCOD_QPSK_3_4,		FE_MOD_QPSK,	FE_FECRATE_3_4	},
>> +	{ FE_MODCOD_QPSK_4_5,		FE_MOD_QPSK,	FE_FECRATE_4_5	},
>> +	{ FE_MODCOD_QPSK_5_6,		FE_MOD_QPSK,	FE_FECRATE_5_6	},
>> +	{ FE_MODCOD_QPSK_8_9,		FE_MOD_QPSK,	FE_FECRATE_8_9	},
>> +	{ FE_MODCOD_QPSK_9_10,		FE_MOD_QPSK,	FE_FECRATE_9_10	},
>> +	{ FE_MODCOD_8PSK_3_5,		FE_MOD_8PSK,	FE_FECRATE_3_5	},
>> +	{ FE_MODCOD_8PSK_2_3,		FE_MOD_8PSK,	FE_FECRATE_2_3	},
>> +	{ FE_MODCOD_8PSK_3_4,		FE_MOD_8PSK,	FE_FECRATE_3_4	},
>> +	{ FE_MODCOD_8PSK_5_6,		FE_MOD_8PSK,	FE_FECRATE_5_6	},
>> +	{ FE_MODCOD_8PSK_8_9,		FE_MOD_8PSK,	FE_FECRATE_8_9	},
>> +	{ FE_MODCOD_8PSK_9_10,		FE_MOD_8PSK,	FE_FECRATE_9_10	},
>> +	{ FE_MODCOD_16APSK_2_3,		FE_MOD_16APSK,	FE_FECRATE_2_3	},
>> +	{ FE_MODCOD_16APSK_3_4,		FE_MOD_16APSK,	FE_FECRATE_3_4	},
>> +	{ FE_MODCOD_16APSK_4_5,		FE_MOD_16APSK,	FE_FECRATE_4_5	},
>> +	{ FE_MODCOD_16APSK_5_6,		FE_MOD_16APSK,	FE_FECRATE_5_6	},
>> +	{ FE_MODCOD_16APSK_8_9,		FE_MOD_16APSK,	FE_FECRATE_8_9	},
>> +	{ FE_MODCOD_16APSK_9_10,	FE_MOD_16APSK,	FE_FECRATE_9_10	},
>> +	{ FE_MODCOD_32APSK_3_4,		FE_MOD_32APSK,	FE_FECRATE_3_4	},
>> +	{ FE_MODCOD_32APSK_4_5,		FE_MOD_32APSK,	FE_FECRATE_4_5	},
>> +	{ FE_MODCOD_32APSK_5_6,		FE_MOD_32APSK,	FE_FECRATE_5_6	},
>> +	{ FE_MODCOD_32APSK_8_9,		FE_MOD_32APSK,	FE_FECRATE_8_9	},
>> +	{ FE_MODCOD_32APSK_9_10,	FE_MOD_32APSK,	FE_FECRATE_9_10	},
>> +	{ FE_MODCOD_RESERVED_1,		FE_MOD_RSVD,	FE_FECRATE_RSVD },
>> +	{ FE_MODCOD_BPSK_1_3,		FE_MOD_BPSK,	FE_FECRATE_1_3  },
>> +	{ FE_MODCOD_BPSK_1_4,		FE_MOD_BPSK,	FE_FECRATE_1_4	},
>> +	{ FE_MODCOD_RESERVED_2,		FE_MOD_RSVD,	FE_FECRATE_RSVD }
>> +};
>> +
>> +int decode_dvbs2_modcod(struct dvb_frontend *fe, enum fe_modcod modcod)
>> +{
>> +	struct modcod_table *table = dvbs2_modcod_lookup;
>> +	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>> +	struct dvb_frontend_params *params = &fepriv->params;
>> +
>> +	table += modcod;
>>     
>
> no bounds check? 
>
>   
Yes, will check.

>> +	params->delsys.dvbs2.modulation = table->modulation;
>> +	params->delsys.dvbs2.fecrate = table->fecrate;
>> +
>> +	return 0;
>>     
>
> why not return void?
>
> I would prefer the following (untested):
>
> /* decode DVB-S2 modcod field as defined in EN 302 307 section 5.5.2.2 */
> void dvb_decode_dvbs2_modcod(u32 modcod, u32 *modulation, u32 *fec)
> {
> 	BUG_ON(modcod >= ARRAY_SIZE(dvbs2_modcod_lookup));
> 	*modulation = dvbs2_modcod_lookup[modcod].modulation;
> 	*fec = dvbs2_modcod_lookup[modcod].fecrate;
> }
>
>   

Ok, done

>> +	case FE_GET_CAPS:
>> +		if (fe->ops->get_caps) {
>> +			memcpy(parg, &fepriv->caps, sizeof (struct dvb_frontend_cap));
>> +			err = fe->ops->get_caps(fe, (struct dvb_frontend_cap*) parg);
>> +		}
>> +		break;
>>     
>
> What is the memcpy() good for?
>
>   

Umm.. gone ..

>> +	/*	New callbacks based on the new IOCTL's		*/
>>     
>
> scratch that comment; better add "superseded by ..." to the old ones
>
>   

Done

>> +	int (*get_caps)(struct dvb_frontend* fe, struct dvb_frontend_cap* caps);
>> +	int (*set_params)(struct dvb_frontend* fe, struct dvb_frontend_params* params);
>> +	int (*get_params)(struct dvb_frontend* fe, struct dvb_frontend_params* params);
>> +
>>     
>
>
>   
>> +/**
>> + *	FE_GET_INFO, is now a legacy IOCTL as well
>>     
>
> better: "superseded by ..."
>
>   
Right

> Anyway, apps will have to keep using this if they find that
> FE_GET_CAPS is not supported by the drivers.
>
>   
Yeah, eventually it will need to move on.

>> +/**
>> + *	Legacy IOCTL's
>> + */
>>  #define FE_SET_FRONTEND		   _IOW('o', 76, struct dvb_frontend_parameters)
>>  #define FE_GET_FRONTEND		   _IOR('o', 77, struct dvb_frontend_parameters)
>>  #define FE_SET_FRONTEND_TUNE_MODE  _IO('o', 81) /* unsigned int */
>>     
>
> FE_SET_FRONTEND_TUNE_MODE is not legacy, is it?
>
>   
>> +/**
>> + *	New operations for new and multiple delivery systems.
>> + *
>> + *	References:
>> + *	DVB-S:  ISO 13818-1/ITU H.222, EN 300 468
>>     
>
> EN 300 421
>
>   
>> + *	DVB-S2: EN 300 468, EN 301 210, TR 102 376
>>     
>
> what is EN 301 210? 
>   

DSNG: Some information on 8PSK and the Nyquist rolloff rates
> add EN 302 307
>
>   

Done

>> + *	DVB-C:	ISO 13818-1/ITU H.222, EN 300 468, EN 300 429
>> + *	DVB-T:	ISO 13818-1/ITU H.222, EN 300 468, EN 300 744
>> + *	DVB-H:	EN 300 468, EN 302 304
>>     
>
> ATSC: A/53A + A80
>
>   

Thanks

> better remove EN 300 468 and ISO 13818-1/ITU H.222 from this list
>
>   

Sure
>> +/**
>> + *	Supported Delivery systems, some devices
>> + *	are capable of supporting multiple delivery systems.
>> + *
>> + *	FE_DELSYS_IGNORE, causes the delivery system
>> + *	not to be queried.
>>     
>
> why is IGNORE useful?
>
>   

>> + *
>> + *	FE_DELSYS_AUTO, Some devices/drivers, might in
>> + *	future be capable of detectinng the delivery system.
>> + */
>> +enum fe_delsys {
>> +	FE_DELSYS_IGNORE		= (0 <<  0),
>> +	FE_DELSYS_DVBS			= (1 <<  0),
>> +	FE_DELSYS_DVBS2			= (1 <<  1),
>> +	FE_DELSYS_DSS			= (1 <<  2),
>>     
>
> please put DSS last, I still doubt it is useful to have
>
>   

Sure, Someone is doing it. Parser is done, (The person sent me a copy) 
It wouldn't be too hard to do the demux.

>> +	FE_DELSYS_DVBC			= (1 <<  3),
>> +	FE_DELSYS_DVBT			= (1 <<  4),
>> +	FE_DELSYS_DVBH			= (1 <<  5),
>> +	FE_DELSYS_ATSC			= (1 <<  6),
>> +	FE_DELSYS_AUTO			= (1 << 31)
>> +};
>> +
>> +/**
>> + *	Supported Transport types, some delivery systems
>> + *	support multiple transports too.
>> + *
>> + *	FE_MATYPE_IGNORE, causes the transport type not
>> + *	to be queried.
>>     
>
> again, why is IGNORE useful?
>   

In many cases, the current status needs to be queried from the demod. 
Many a cases the user application might require only a certain subset of 
the feature. In this case the smaller subset can be queried fast 
(queried over i2c). (I remember not all i2c implementations are great, 
even if the hardware implementation is nice, driver might be bad due to 
the geological location .. ;-))

>> + *
>> + *	FE_MATYPE_AUTO, Some devices/drivers, might
>> + *	support auto detecting the transport type, depending
>> + *	on some other settings and or hardware features.
>> + *
>> + *	NOTE: This is specified by the DVB-S2 specifications
>> + *	This change of transport type might require an additional
>> + *	change at the PCI bridge level, for some implementations.
>> + */
>> +enum fe_matype {
>> +	FE_MATYPE_IGNORE		= (0 <<  0),
>> +	FE_MATYPE_TRANSPORT		= (1 <<  0),
>> +	FE_MATYPE_GENERIC_PACKET	= (1 <<  1),
>> +	FE_MATYPE_GENERIC_CONTINUOUS	= (1 <<  2),
>> +	FE_MATYPE_RESERVED		= (1 <<  3),
>> +	FE_MATYPE_AUTO			= (1 << 31)
>> +};
>>     
>
> enum fe_matype isn't used anywhere
>
> also, please always spell out non-obvious acronyms in a comment
> (probably "mode adaption type")
>
> But EN 302 307 defines MATYPE as a two-byte field, this
> enum fe_matype is just the 2bit TS/GS field of the
> whole MATYPE.
>
>   

Form these 3 and
>> +
>> +/**
>> + *	Supported Stream Priorities
>>     
>
> after having read "Supported" this many time I think you whould
> scratch it 
>
>   
>> + *
>> + *	Some delivery systems do have streams having
>> + *	different priorities. ie, High priority/Low priority.
>>     
>
> better: "Select high or low priority stream when hierachical
> coding is used"
>
>   
>> + *
>> + *	NOTE: The default should be the High priority stream.
>> + */
>> +enum fe_stream {
>> +	FE_STREAM_HP			= (1 <<  0),
>> +	FE_STREAM_LP			= (1 <<  1)
>> +};
>>     
>
> this is called "priority" in the specs
>
>   

will change

>> +
>> +/**
>> + *	Supported Modulations.
>> + *	Some devices support multiple modulation types
>>     
>
> actually, most demods do
>
>   
>> +enum fe_modulations {
>>     
>
> I prefer the singular for enum type names.
>
>   

But that already exists, and did not want to tamper that.

>> +	FE_MOD_IGNORE			= (0 <<  0),
>> +	FE_MOD_NONE			= (1 <<  0),
>>     
>
> what is FE_MOD_NONE?
>   

Multistandard tuners/demods can return that state, can be possibly an 
error condition or a modulation unset condition,

>> +	FE_MOD_BPSK			= (1 <<  1),
>> +	FE_MOD_QPSK			= (1 <<  2),
>> +	FE_MOD_OQPSK			= (1 <<  3),
>> +	FE_MOD_8PSK			= (1 <<  4),
>> +	FE_MOD_16APSK			= (1 <<  5),
>> +	FE_MOD_32APSK			= (1 <<  6),
>> +	FE_MOD_QAM16			= (1 <<  7),
>> +	FE_MOD_QAM32			= (1 <<  8),
>> +	FE_MOD_QAM64			= (1 <<  9),
>> +	FE_MOD_QAM128			= (1 << 10),
>> +	FE_MOD_QAM256			= (1 << 11),
>> +	FE_MOD_QAM512			= (1 << 12),
>> +	FE_MOD_QAM1024			= (1 << 13),
>> +	FE_MOD_QAMAUTO			= (1 << 14),
>> +	FE_MOD_OFDM			= (1 << 15),
>> +	FE_MOD_COFDM			= (1 << 16),
>> +	FE_MOD_VSB8			= (1 << 17),
>> +	FE_MOD_VSB16			= (1 << 18),
>> +	FE_MOD_RSVD			= (1 << 19),
>>     
>
> what is RSVD? if it is RESERVED, then call it RESERVED.
>   

Ok.
> but what would RESERVED be good for?
>
>   

Ok, can be removed.
>> +	FE_MOD_AUTO			= (1 << 31)
>> +};
>> +
>> +/**
>> + *	Supported FEC (Forward Error Correction) Code rates
>> + *
>> + *	FE_FECRATE_IGNORE, causes the FECRATE type not
>> + *	to be queried.
>>     
>
> IMHO FE_FEC_1_2 etc. looks better than FE_FECRATE_1_2
>
>   
>> +	FE_FECRATE_RSVD			= (1 << 14),
>>     
>
> again, what's it good for?
>
>   

I put RESERVED as the last value in there

>> +/**
>> + *	Supported MODCOD types
>> + *
>> + *	The MODCOD types are specific to the DVB-S2 specification
>> + *	Some devices directly take the MODCOD values as an input
>> + *	or just output the same.
>> + *
>> + *	In any case a single frontend will not be able to do
>> + *	multiple modulations/code rates at any given point of time.
>> + */
>> +enum fe_modcod {
>> +	FE_MODCOD_DUMMY_PLFRAME		= 0,
>>     
>
> enum fe_modcod isn't used anywhere except the
> decode_dvbs2_modcod() helper function. Either
> use it in the API or move it to dvb_frontend.h.
>
>
>   

Compliant drivers uses the modcod straight away. I was still working on 
the STB0899. I was of the thought would add it alongwith it.

>> +
>> +/**
>> + *	Supported Bandwidth modes
>>     
>
> a bandwidth is not a mode
>
>   
>> +enum fe_hierarchy_info {
>> +enum fe_rolloff {
>> +enum fe_interleaver {
>>     
>
> EN 300 468 V1.7.1 combines these in a single
> hierarchy_information field. Is it useful to
> split them into three enums?
>
>   

Well, i originally had it as one only, after the innumerous discussions 
it became like that.

>> +/**
>> + *	DVB-S parameters
>> + */
>> +struct dvbs_params {
>> +	__u32				symbol_rate;
>> +
>> +	enum fe_modulations		modulation;
>> +	enum fe_fecrates		fecrate;
>>     
>
> again, IMHO either "fec" or "coderate", but not "fecrate"
>
>   

ok, will go for coderate

>> +struct dvbs2_params {
>> +	__u32				symbol_rate;
>> +
>> +	enum fe_modulations		modulation;
>> +	enum fe_fecrates		fecrate;
>> +	enum fe_stream			streamtype;
>> +	enum fe_fecrates		coderate_HP;
>> +	enum fe_fecrates		coderate_LP;
>>     
>
> do we really have three fecs?
> also, if you have hierachical modulation you need a way to
> select the HP or LP stream
>   

streamtype is used for that.

> I don't fully understand the DVB-S2 spec yet, but I think
> the way to select the stream is by using an 8bit
> input_stream_identifier.
>
>
>   

H.3 describes it. 8 bits isn;t needed, anyway boiling down to the demod, 
that doesn't happen, it is a simple knob to select between the streams.

There aren't many S2 chipsets. ST has STB0899, Broadcom has 4500. 
Croadcom doesn't sell to PC card manufacturers, but only STB vendors. 
There is Conexant also as i heard, but a very buggy chip. Well the S2 
devices do have bugs. Even the datasheets have bugs.

>> +/**
>> + *	Padding to handle future binary compatibility issues.
>> + *
>> + *	NOTE: The padding is a dummy parameter !
>> + */
>> +struct pad_params {
>> +	__u8 pad[512];
>> +};
>>     
>
> just scratch this struct and put the pad directly in the union
>
>   

It would make it look a bit consistent though. Everyone else really 
liked it, except for your comment and Trent's.

> also, 512 is a bit excessive; the larges *_params we have is
> 48 bytes (if I counted correctly), I'd say 128 bytes
> of pad should be enough
>
>   

okay, will change to 128

>> +/**
>> + *	Frontend capability information
>> + */
>> +struct dvb_frontend_cap {
>> +	char				name[128];
>> +
>> +	__u32				frequency_min;
>> +	__u32				frequency_max;
>> +	__u32				frequency_stepsize;
>> +	__u32				frequency_tolerance;
>> +	__u32				symbol_rate_min;
>> +	__u32				symbol_rate_max;
>> +	__u32				symbol_rate_tolerance;
>> +
>> +	enum fe_delsys			delivery;
>>     
>
> is this a bitset?
>
>   

Delivery System, Yes. There can be multistandard devices
>> +	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;
>> +		struct pad_params	pad;
>> +	} delsys;
>>     
>
> what does this mean in terms of capabilities?
> every enum a bitset?
>
>   
It depends on what the driver supports

> If delivery is a bitset, what does it return?
>
>   

It is not done yet, i was thinking like this. haven't tried compiling 
yet. The driver is a bit huge, compared to other frontends.


static int stb0899_get_dvbs_caps(struct dvb_frontend *fe,
                                 struct dvb_frontend_cap *caps)
{
        caps->name                      = "STB0899 Multi-standard (DVB-S 
Mode)";
        caps->frequency_min             = 950000;
        caps->frequency_max             = 2150000;
        caps->symbol_rate_min           = 1000000;
        caps->symbol_rate_max           = 45000000;

        caps->inversion                 = FE_INVERSION_IGNORE   | 
FE_INVERSION_OFF |
                                          FE_INVERSION_ON       | 
FE_INVERSION_AUTO;
        caps->delsys.dvbs.modulation    = FE_MOD_QPSK;
        caps->delsys.dvbs.fecrate       = FE_FECRATE_1_2 | FE_FECRATE_2_3 |
                                          FE_FECRATE_3_4 | FE_FECRATE_5_6 |
                                          FE_FECRATE_6_7;

        return 0;
}

static int stb0899_get_dss_caps(struct dvb_frontend *fe,
                                struct dvb_frontend_cap *caps)
{
        caps->name                      = "STB0899 Multi-standard (DSS 
Mode)";
        caps->frequency_min             = 950000;
        caps->frequency_max             = 2150000;
        caps->symbol_rate_min           = 1000000;
        caps->symbol_rate_max           = 45000000;

        caps->inversion                 = FE_INVERSION_IGNORE   | 
FE_INVERSION_OFF |
                                          FE_INVERSION_ON       | 
FE_INVERSION_AUTO;
        caps->delsys.dvbs.modulation    = FE_MOD_QPSK;
        caps->delsys.dvbs.fecrate       = FE_FECRATE_1_2 | FE_FECRATE_2_3 |
                                          FE_FECRATE_3_4 | FE_FECRATE_5_6 |
                                          FE_FECRATE_6_7;

        return 0;
}

static int stb0899_get_dvbs2_caps(struct dvb_frontend *fe,
                                  struct dvb_frontend_cap *caps)
{
        caps->name                      = "STB0899 Multi-standard 
(DVB-S2 Mode)";
        caps->frequency_min             = 950000;
        caps->frequency_max             = 2150000;
        caps->symbol_rate_min           = 1000000;
        caps->symbol_rate_max           = 45000000;

        caps->inversion                 = FE_INVERSION_IGNORE   | 
FE_INVERSION_OFF |
                                          FE_INVERSION_ON       | 
FE_INVERSION_AUTO;

        caps->delsys.dvbs.modulation    = FE_MOD_QPSK | FE_MOD_8PSK | 
FE_MOD_16APSK;

        caps->delsys.dvbs.fecrate       = FE_FECRATE_1_4 | FE_FECRATE_1_3 |
                                          FE_FECRATE_2_5 | FE_FECRATE_1_2 |
                                          FE_FECRATE_3_5 | FE_FECRATE_2_3 |
                                          FE_FECRATE_3_4 | FE_FECRATE_4_5 |
                                          FE_FECRATE_5_6 | FE_FECRATE_8_9 |
                                          FE_FECRATE_9_10;

        return 0;
}

static int stb0899_get_caps(struct dvb_frontend *fe,
                            struct dvb_frontend_cap *caps)
{
        switch (caps->delivery) {
        case FE_DELSYS_DVBS:
                stb0899_get_dvbs_caps(fe, caps);
                break;
        case FE_DELSYS_DSS:
                stb0899_get_dss_caps(fe, caps);
                break;
        case FE_DELSYS_DVBS2:
                stb0899_get_dvbs2_cap(fe, caps); 
        }

        return 0;
}


>> +/**
>> + *	We have the NEW IOCTL's defined now. This IOCTL
>> + *	is supposed to handle all the new delivery systems
>> + *	FE_GET_CAPS, gets all the capabilities in one go.
>> + *
>> + *	For the GET IOCTL's ie FE_GET_PARAMS and FE_GET_CAPS
>> + *	some of the parameters maybe selectively queried by
>> + *	setting that relevant parameter to IGNORE.
>>     
>
> Is this IGNORE stuff useful for apps?
>
>   

It would be yes. It can selectively query what it wants.


> The description is just noise. How does an app use this?
> What does the app need to know about the frontend?
> I.e. are there other calls which need to be different
> depending on the information returned here? Or is it
> just informational?
>
>   

For a multistandard frontend, what all it supports. In most cases it is 
informational. But it can be the current set state too.
>> + */
>> +#define FE_GET_CAPS		   _IOR('o', 84, struct dvb_frontend_cap)
>>     
>
>   
>> +/**
>> + *	We have the NEW IOCTL's defined now. These IOCTL's
>> + *	are supposed to handle all the new delivery systems.
>>     
>
> better: add "superseded by..." to old ioctls
>
>   

Ok, Fine ..

>> + *	FE_SET_PARAMS, sets all the parameters in one go.
>> + *	FE_GET_PARAMS, gets all the parameters in one go.
>> + *
>> + *	For the GET IOCTL's ie FE_GET_PARAMS and FE_GET_CAPS
>> + *	some of the parameters maybe selectively queried by
>> + *	setting that relevant parameter to IGNORE.
>>     
>
> same as with GET_CAPS:
>
> what is IGNORE good for?
> How do apps use this? I.e.
>
>   

same as i explained earlier.

> 	/* tune to the stream with the specified parameters */
>   
>> +#define FE_SET_PARAMS		   _IOW('o', 82, struct dvb_frontend_params)
>>     
>
> 	/* query parameters for currently tuned stream,
> 	 * only valid when FE_READ_STATUS indicates FE_HAS_LOCK
> 	 */
>   
>> +#define FE_GET_PARAMS		   _IOR('o', 83, struct dvb_frontend_params)
>>     
>
> Please also increment the minor version in linux/dvb/version.h.
>
>   

Right.

> Generally, the DVB-S2 stuff doesn't seem to be very well thought out.
> It's probably better to take some of the advanced stuff out for now,
>   

Well, there isn't much of advanced stuff now, just only the basic DVB-S2 
is used.

> but make sure that the API is defined such a way that it is
> easy to add more parameters for DVB-S2 later in a binary
> compatbile way.
>
>   
Ok.

> Removing stuff from an API is not an option, even if it was wrong.
> So better make sure everything which is in it is correct.
>
> BTW, is anyone working on a driver for a DVB-S2 tuner?
> Or does anyone have a data sheet for such a device?
>
>   
Yes, quite a lot of people have it now (as i understand, other than some 
people around us), well that chips is something to make people real 
crazy. The documents are available under NDA only.

Well, i have done ~ 3000 lines of code for the STB0899 + STB6100. I got 
approval from Twinhan to publish the driver under OSS.

There are 4 cards now, (1) The Twinhan card (not the DST, but a brand 
new one) (2) KNC1 (SAA7146 based) (3) TechnoTrend (SAA7146 based) (4) 
Micronas (nGene) But none of these cards are out for purchase for the 
public as of now, some few sample cards were sold/given by all these 
vendors.

Currently, myself is taking a go at it. Ralph is having a nGene. For 
testing we have a KNC1 and a Twinhan. We can test with the nGene too.

As far as i heard some people are trying to cut and paste the Windows 
code for a minimal Linux DVB-S driver and trying to get an approval from 
the vendor (from the ST sample code), but the problem is that the 
original code for the DVB-S2 specific parts (It has 2 DSP's builtin, i2c 
is really different) is not from ST itself, forgetting about the vendor 
itself, but from a 3rd party and hence cannot be cut n' pasted.

But we (myself, Christoph, Andrew and Ralph) are working on it (you 
might've seen the math functions, it all goes in there, there are more) 
(Other's did help help in fixing various Areas, people who can be 
mentioned are Marcel (mws) Sigmund and Julian providing various 
feedbacks and bug hunting etc) Most of the things are done, it needs 
some fixing here and there due to some bugs.

It has a Silicon Tuner based on the STB6100, hence the earlier 
TUNER_RE_FACTORING by Andrew, which additionally took care of the PLL 
thing too.

Regarding the Broadcom device, i think there is a driver (genpix) which 
has a dst style frontend. But as i looked at the code , i didn't see any 
S2 specifics.

Wow, didn't think that so much was there to be fixed ! Thanks for taking 
the time to hunt them down. Taking a deep breath now.

Thanks,
Manu



More information about the linux-dvb mailing list