[linux-dvb] [PATCH] add DVB-S2 support to frontend.h

[Marcel Siegert]

hi andreas,

On Tuesday 28 February 2006 20:01, Andreas Oberritter wrote:
> Hi Marcel,
> 
> On Mon, 2006-02-27 at 22:59 +0100, Marcel Siegert wrote:
> > +/*! describes the available modulation types. */
> > +enum dvb_fe_modulation {
> > +	DVB_FE_MOD_QPSK		= (1 << 0),
> > +	DVB_FE_MOD_QAM_16	= (1 << 1),
> > +	DVB_FE_MOD_QAM_32	= (1 << 2),
> > +	DVB_FE_MOD_QAM_64	= (1 << 3),
> > +	DVB_FE_MOD_QAM_128	= (1 << 4),
> > +	DVB_FE_MOD_QAM_256	= (1 << 5),
> > +	DVB_FE_MOD_QAM_AUTO	= (1 << 6),
> > +	DVB_FE_MOD_2_VSB	= (1 << 7),
> > +	DVB_FE_MOD_4_VSB	= (1 << 8),
> > +	DVB_FE_MOD_8_VSB	= (1 << 9),
> > +	DVB_FE_MOD_16_VSB	= (1 << 10),
> > +	DVB_FE_MOD_8_PSK	= (1 << 11),
> > +	DVB_FE_MOD_BPSK		= (1 << 12),
> > +	DVB_FE_MOD_MOD_UNKNOWN	= (1 << 31)
> > +};
> 
> why do we need DVB_FE_MOD_QAM_AUTO and DVB_FE_MOD_MOD_UNKNOWN? I suggest
> replacing them with DVB_FE_MOD_AUTO. Maybe MOD_UNKNOWN was meant to be
> returned by FE_GET_FRONTEND, but if the modulation is unknown, then it
> is very unlikely that it was possible to lock the frontend and therefore
> calling FE_GET_FRONTEND in such a case seems to be useless. I think
> FE_GET_FRONTEND should only replace the known bits of the tuning
> parameters as it does today.

as i mentioned some lines above within the patch, i took those structs via c&p from the api v4 cvs.
i think that michael had reasons to name them like this, but i do agree with your argumentation.
i changed that inside my tree now.

removed DVB_FE_MOD_QAM_AUTO and DVB_FE_MOD_MOD_UNKNOWN
added DVB_FE_MOD_AUTO

> 
> I also wonder why QAM and VSB values are sorted by size, but PSK values
> are sorted alphanumerically.

hmm, within this thread there was a small part, i think johannes did that, that
stated we should name the modulations according to the standard.
the standard calls them 8PSK. i did a small change in ordering these enumeration values
in my tree, but that will occur next time also, when a new modulation type is introduced.

> > +/*! describes common supported frontend capabilities. */
> > +enum dvb_fe_common_cap {
> > +	DVB_FE_CAN_INVERSION_AUTO   = (1 << 0), /*!< fixme */
> > +	DVB_FE_CAN_RECOVER          = (1 << 1), /*!< frontend can recover from a cable unplug automatically */
> > +	DVB_FE_CAN_MUTE_TS          = (1 << 2), /*!< frontend can stop spurious TS data output */
> > +	DVB_FE_SUP_HIGH_LNB_VOLTAGE = (1 << 31), /*!< fixme, frontend can deliver higher lnb voltages */
> > +};
> 
> Why are there FIXMEs?
as told before (c&p) from api v4 tree - removed "fixme" in comments within my tree.
 
> > +/*! describes other available, frontend type dependent capabilities. */
> > +enum dvb_fe_other_cap {
> > +	DVB_FE_CAN_TRANSMISSION_MODE_AUTO = (1 << 0), /*!< fixme (DVB-T specific) */
> > +	DVB_FE_CAN_BANDWIDTH_AUTO         = (1 << 1), /*!< fixme (DVB-T specific) */
> > +	DVB_FE_CAN_GUARD_INTERVAL_AUTO    = (1 << 2), /*!< fixme (DVB-T specific) */
> > +	DVB_FE_CAN_HIERARCHY_AUTO         = (1 << 31), /*!< fixme (DVB-T specific) */
> > +};
> 
> Same here. Well, I know these are specific to DVB-T and proably DVB-H,
> but how are these FIXMEs supposed to get fixed?
i removed the comments. i will ask michael why he tagged them fixme.

> > @@ -126,7 +194,8 @@
> >  	FE_HAS_SYNC	= 0x08,   /*  found sync bytes  */
> >  	FE_HAS_LOCK	= 0x10,   /*  everything's working... */
> >  	FE_TIMEDOUT	= 0x20,   /*  no lock within the last ~2 seconds */
> > -	FE_REINIT	= 0x40    /*  frontend was reinitialized,  */
> > +	FE_REINIT	= 0x40,   /*  frontend was reinitialized,  */
> > +	FE_EXTENDED_TP  = 0x80    /*  providing new informations */
> >  } fe_status_t;			  /*  application is recommended to reset */
> 
> First, it's "new information". What shall be reset and what new kind of
> information is provided through which interface?

comments were mixed up. sorry for that. fixed that - and changed  the "new information" comment to 
a more usesable description.

-----------snip----------------------
	FE_TIMEDOUT	= 0x20,   /*  no lock within the last ~2 seconds */
	FE_REINIT	= 0x40,   /*  frontend was reinitialized,  */
			  	  /*  application is recommended to reset */
				  /*  DiSEqC, tone and parameters */
	FE_EXTENDED_TP  = 0x80    /*  dvb_frontend_event does contain the fe_type member and */
				  /*  the dvb_frontend_parameters in addition of __dvb_fronend_parameters_old */
} fe_status_t;
----------snip-----------------------
  
> > +struct dvb_dvbs2_parameters {
> > +	__u32			symbol_rate;    /* symbol rate in Symbols per second */
> 
> It's symbols, not Symbols.
typo fixed. - also fixed that typo within the dvbs/dvbc/dvbt structs that kept this typo already a long time in cvs ;)
 
thanks for your participation :)

best regards 

marcel