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).
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.
> 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".
Regards,
Andy
>
> 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
> >
> >
>
>
>
_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel