On Sat, 6 May 2006, Johannes Stezenbach wrote: > 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 ;-) I made a couple of the same comments already and there was a bit of discussion: http://thread.gmane.org/gmane.linux.drivers.dvb/25401/focus=25538 might save some time to check that. > > 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? I hope you don't plan on dropping the old userpsace interface too soon. Everyone's software will break. > > + * 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 Is A/80 used in any way? How about this: * ATSC: ATSC Doc A/53E, ANSI/SCTE 07 2000 > > + * 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? My thought would be that GET_PARAMS with delivery set to FE_DELSYS_IGNORE would just return frequency. SET_PARAMS with FE_DELSYS_IGNORE would just set the frequency (and inversion? Why is inversion here?) and leave the rest of the parameters as is. It's not specified anywhere, but I thought that GET_CAPS with delivery set to FE_DELSYS_IGNORE (wasn't there a QUERY before?) would return the supported delsyses. Like this: int check_for_atsc(int fd) { struct dvb_frontend_cap; cap.delivery = FE_DELSYS_IGNORE; // Get FE type ioctl(fd, FE_GET_CAPS, &cap); if(!(caps.delivery & FE_DELSYS_ATSC)) { printf("ATSC not supported\n"); return 0; } cap.delivery = FE_DELSYS_ATSC; // Get ATSC parameters ioctl(fd, FE_GET_CAPS, &cap); printf("Supported modulations: %s%s%s%s\n", cap.delsys.atsc.modulation&FE_MOD_VSB8?"8-VSB ":"", cap.delsys.atsc.modulation&FE_MOD_QAM64?"64-QAM ":"", cap.delsys.atsc.modulation&FE_MOD_QAM256?"256-QAM ":"", cap.delsys.atsc.modulation&FE_MOD_QAMAUTO?"QAM Auto":""); return 1; } > > +struct dvbs2_params { > > + enum fe_fecrates fecrate; > > + enum fe_fecrates coderate_HP; > > + enum fe_fecrates coderate_LP; > > do we really have three fecs? Yes, how does three fec rates work? > 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. What about adding a version number to the param and caps structures? Make it the first field. The userspace program sets the version number it is using. The driver can check for old versions and handle them correctly. Normally you just add a new field to the end of the structure. All the driver has to do is check if version >= X, before it looks at a field that was adding in version X. A different way of doing the same thing is to add a size field. Now you just check if the offset of the field you want to look at is less than the size field. This is what I did to keep binary compatibility between old versions of the client and server in Freeciv. New fields were added to the end of packets. When receiving packets, the packet size was checked to see if new fields were present. _______________________________________________ linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
