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 ;-)
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?
Generally, what is the plan?
- change all frontend drivers to use set_params/get_params
and struct dvb_frontend_params?
- 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.
> + 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
> +};
> +
> +struct modcod_table dvbs2_modcod_lookup[] = {
should be static
> + { 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?
> + 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;
}
> + 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?
> + /* New callbacks based on the new IOCTL's */
scratch that comment; better add "superseded by ..." to the old ones
> + 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 ..."
Anyway, apps will have to keep using this if they find that
FE_GET_CAPS is not supported by the drivers.
> +/**
> + * 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?
add EN 302 307
> + * 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
better remove EN 300 468 and ISO 13818-1/ITU H.222 from this list
> +/**
> + * 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
> + 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?
> + *
> + * 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.
> +
> +/**
> + * 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
> +
> +/**
> + * Supported Modulations.
> + * Some devices support multiple modulation types
actually, most demods do
> +enum fe_modulations {
I prefer the singular for enum type names.
> + FE_MOD_IGNORE = (0 << 0),
> + FE_MOD_NONE = (1 << 0),
what is FE_MOD_NONE?
> + 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.
but what would RESERVED be good for?
> + 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?
> +/**
> + * 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.
> +
> +/**
> + * 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?
> +/**
> + * 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"
> +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
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.
> +/**
> + * 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
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
> +/**
> + * 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?
> + 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?
If delivery is a bitset, what does it return?
> +/**
> + * 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?
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?
> + */
> +#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
> + * 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.
/* 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.
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,
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.
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?
Johannes
_______________________________________________
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
