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. 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. 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? 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. 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? And are the other cx18_av controls really needed? Aren't these things that it is the responsibility of the driver to setup correctly? 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. Regards, Hans > > Note that most of these changes add cx18 driver specific controls, > except the last 2. Those changes fix an I2S clock divisor problem in > the cx18 driver and add secondary audio controls to the ivtv driver. > > > 01/18: cx18: Add user control for setting the extra +12 dB of video gain > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=2ef91a4f42a9 > > 02/18: cx18: Reformat "Extra 12dB Gain" control string to look more natural > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=82bf413c60d8 > > 03/18: cx18: Add a control query fill function for cx18 specific user controls > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=22260bf345c1 > > 04/18: cx18: Add cx18_av core Luma Droop Compensation user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=33f8e5621fe3 > > 05/18: cx18: Add cx18_av core Chroma Droop Compensation user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=1ce8fba3d190 > > 06/18: cx18: Add cx18_av core Luma Clamping user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=85e1fc01dd6f > > 07/18: cx18: Add cx18_av core Chroma Clamping user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=a042df5b8281 > > 08/18: cx18: Add cx18_av core Auto Luma Clamping Level user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=311b5969116c > > 09/18: cx18: Add cx18_av core Auto Digital Gain Control user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=4b1af2abd533 > > 10/18: cx18: Add cx18_av Auto Analog Gain Control user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=8f405aeb13f2 > > 11/18: cx18: Add cx18_av core Auto Sync Height Crush user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=6ae9e079aaaa > > 12/18: cx18: Add cx18_av core Luma Clamping Voltage user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=df97564687a8 > > 13/18: cx18 Add cx18_av core Digital Gain Level control; make func file scope > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=f7713cb28208 > > 14/18: cx18: Add cx18_av core Analog Gain Level user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=750adcd07696 > > 15/18: cx18: Add cx18_av core Sync Tip Height user control > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=ca30fa94db6e > > 16/18: cx18: Add cx18_av Sync Acquired/Lost Error Threshold user controls > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=e4b2c5d550b5 > > 17/18: cx18: Ensure I2S output of A/V core clocks 24 bits/sample > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=ab8e4bd49b05 > > 18/18: ivtv: Add ability to control a secondary audio chip via user controls > http://linuxtv.org/hg/~awalls/v4l-dvb?cmd=changeset;node=c9f13db7a53e > > > cx18/cx18-av-audio.c | 46 +++-- > cx18/cx18-av-core.c | 441 ++++++++++++++++++++++++++++++++++++++++++++++----- > cx18/cx18-av-core.h | 46 ++++- > cx18/cx18-controls.c | 76 ++++++++ > ivtv/ivtv-cards.c | 2 > ivtv/ivtv-cards.h | 1 > ivtv/ivtv-controls.c | 114 +++++++++++++ > ivtv/ivtv-driver.c | 1 > ivtv/ivtv-driver.h | 1 > 9 files changed, 668 insertions(+), 60 deletions(-) > > Thanks, > Andy > > -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- 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
