On Sat, May 06, 2006, Trent Piepho wrote: > 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. Yeah, I don't have much time these days and it seems I missed 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? This is in implementation only, API will keep it of course. > > ATSC: A/53A + A80 > > Is A/80 used in any way? How about this: I have no idea, I just have some old versions of these on my hard disk (A80 is about satellite delivery). > * ATSC: ATSC Doc A/53E, ANSI/SCTE 07 2000 Thanks, it's probably more accurate info. > > > + * 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 Thanks, now I get why there once was a FE_DELSYS_QUERY. This is totally not obvious from reading frontend.h, it needs to be documented with a comment. > 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); you missed to memset(&cap, 0, sizeof(cap)); to set all fields to _IGNORE, and then cap.delsys.atsc.modulation = 0xdeadbeef; // dummy value != FE_MOD_IGNORE > 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; > } > > 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. Dunno, both sounds a little bit cumbersome... Thanks, Johannes _______________________________________________ linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
