> On Mon, 2009-08-03 at 08:30 +0200, Hans Verkuil wrote: >> On Sunday 02 August 2009 20:02:30 Andy Walls wrote: >> > Mauro, >> > >> > Please pull from http://linuxtv.org/hg/~awalls/v4l-dvb >> > >> > for the following 18 changesets. >> >> Hi Andy, >> >> I'm going to NAK this series, I'm afraid. My apologies, I should have >> mailed >> you that my review was delayed a bit. I quickly looked at this and there >> are >> several things that need to be corrected first. > > Hans, > > That's fine. I somewhat expected you might. :) > > I'm going on vacation soon and didn't want to leave all those changed > LOC hanging around if no one had a problem with them. > > In my opinion all the changes can wait - no rush. > > >> First of all any control (even private ones) should be added to >> videodev2.h. >> That way applications have access to the CIDs if they want to set them >> explicitly, and it ensures that the CIDs are all unique. > > I was under the impression that for private range controls, the app > could just display the string and appropraite control to the user (if > the apps chose to do so). Applications typically enumerate all controls. So any controls you add are visible to e.g. tvtime or xawtv. > > All of the controls I added were very hardware specific, so I'd be > really surprised if an app wanted to include their ID specifically. > > >> BTW, I wonder whether we shouldn't split off all the control ID stuff >> into a separate header as it is beginning to be a major part of the >> videodev2.h header. Mauro, what do you think? > > If it reduces the complexity, it's fine by me. And obviously, I > wouldn't expect all the apps to start inclding the new header > explcitily. It should be included by videodev2.h itself since we do not want to have to modify applications. > > > >> In addition they should be documented in the spec. It makes it more >> work, >> but it ensures that both developers and end-users can actually read >> about >> what these controls do. One problem we have with some of the existing >> private controls is that they are undocumented and that's something we >> want to avoid. > > That seems reasonable. It didn't occur to me given the very hardware > specific nature of the controls. > > > >> I also have doubts about the wishdom of about the usefulness of adding >> secondary bass/treble/balance/mute and loudness controls. A secondary >> volume might be necessary (I would have to reread the thread on that), >> but the others? What's the point of two bass controls? > > For volume, a user thought he could get less noisy audio, if volume > control happened in the ADC chip (WM8739). > > The 2 bass controls don't make sense - good point. I was mechanically > changing things there without thinking. > > > >> And are the other cx18_av controls really needed? > > They make life easier for troubleshooting - see below. > > >> Aren't these things >> that it is the responsibility of the driver to setup correctly? > > Yes, in the general case. > > There are specific video sources that Tom Ryan has (Sony security > Camera) with a CX23418 and Levante Novak has (a VCR generated OSD) with > a CX25843/CX23416, that have trouble locking. (I only addressed the cx18 > ones at the momemnt). > > My problems were that for all the possible permutations of register > tweaks that might make their system work: > > 1. those tweaks may not work well for the general case of a normal > consumer device > > 2. v4l2-dbg is cumbersome for more than a few tests and can be useless > without a datasheet. That leads to a *lot* of debugging email > exchanges. > > 3. The controls can enforce some sensible rules (e.g. always set the > Chroma clamp level to mid-code) that a user/technician may not know to > maintain using v4l2-dbg. > > 4. As much as I appreciate the users' willingnees to help, I don't think > people mailing me expensive video devices to troubleshoot is a good or > sustainable debugging model. > > >> And if >> they are really needed, then we should perhaps start thinking of an >> 'advanced' control flag to let apps know that such controls might better >> be added to an 'advanced' control panel. > > Yes. I was thinking "technician" controls. Right now the only user > roles we support is "operator" with the V4L2 controls, and "developer" > with v4l2-dbg. > > It would be nice to support a role of someone who knows A/V systems and > antennas and needs to do troubleshooting - hence a "technician". This sounds like a good idea, but I'm not sure whether controls are really the best way of doing this. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
