On Thu, Aug 5, 2010 at 8:11 AM, Andy Walls <awalls@xxxxxxxxxxxxxxxx> wrote: > On Wed, 2010-08-04 at 11:40 -0400, Steven Toth wrote: >> On 7/31/10 2:28 PM, Andy Walls wrote: >> > On Sat, 2010-07-31 at 12:31 -0400, Steven Toth wrote: >> >> Mauro, >> >> >> >> Please pull from http://www.kernellabs.com/hg/~stoth/cx23885-mpx >> >> >> A pretty large patch set which adds a number of important features to >> >> the cx23885 driver. >> > >> > I have a few cx25840 related comments: > >> > >> > 2. The raw VBI startup you added in the cx25840 module is not going to >> > serve you well. It is true that it is harmless to existing >> > non-CX2388[578] boards. However, any app action that causes >> > cx25840_std_setup() to be called will change register 0x474 to probably >> > something you are not expecting. >> >> It's held up very well in testing. With ntsc-zv-vbi and mencoder I'm not seeing >> any issues. It could be that the API sequence is working in the customer favor >> but non-the-less I don't have any open bugs against VBI. > > I suspect userspace maybe somewhat forgiving for raw VBI samples, for > example to handle situations where CC data appears to be on the wrong > line, etc. In my experience userland VBI is not forgiving at all, at least ZVBI-NTSC-CC isn't. TVTime on the other hand doesn't even use the ZVBI library (which works well) so TVTime is borked and (in my view) should be brought inline. We have a KernelLabs tree on kernellabs.com, Devin has been itching to fix CC on tvtime for a long time. No doubt this will be addresses at some point. I digress. User apps do care, at least the popular debug/sanity tools I've been using. > > Although from inspection, I think if you changes from PAL or SECAM back > to NTSC, that register value will be changed from what your change > initializes it to. In other words, I don't doubt that your change > "works", but, IMO, it appears incomplete and symptoms will likely show > up for someone, someday. Noted. I currently don't have any users who need to switch between PAL or NTSC on a regular basis. This is why it's never shown itself. The ZVBI app is targeted at CC, NTSC only. Likely it would never experience the issue. > > >> It should be made clear that VBI never previous worked on the cx23885, for any >> cards. > > Yes. This is a step forward for cx23885 analog users. > > >> > >> > It would be better for you, in the long run, to fix up >> > cx25840_std_setup() and cx25840_s_raw_fmt() to suit your needs, rather >> > than a one off setting in the init function.. >> > >> > (I will admit the setup of the vblank, voffset, etc. in the cx25840 is >> > not trivial, due to the way the ivtv driver likes to capture raw vbi >> > lines and sliced vbi as if it were raw vbi data. I puzzled most of it >> > out and fixed it up in the cx18 driver to be more sensible. It took a >> > bit of staring at the BT878 data sheet to figure out the timing of the >> > VBI slicer too, since the data is missing from later data sheets.) >> >> Ack. VBI via the cx25840 is a mess generally. > > What makes it really difficult is the regression testing required on any > change to the vertical settings (vblank, voffset, etc.). ivtv sliced > VBI capture depends on those settings, so changes must be coordinated > with ivtv. Changes in ivtv sliced VBI handling means the modules that > handle SAA717X and SAA7115 (and CX2584x) hardware would need to be > instpected and tweaked if necessary, plus regression testing done on > actual hardware. This, and the lack of a good overall API into the cx25840 so parent bridge devices have better 'card level' awareness and can drive it correctly. Dealing with the 25840 under linux is akin to working on a 800 piece jigsaw with boxing gloves. The pieces can be seen but you have no real external mechanism for influencing them in any significant way. If the defaults for a lot of core pieces don't work for you then you're hosed. You recent IR + clock work is an example of this. If I speak as someone who has seen the 25840 implemented inside many windows drivers, the windows implementations are generally much simpler from an API perspective, resulting in faster code fixes for end users. Direct bridge level access to the registers exactly when you need it. The Linux implied (or unimplied) insistence we've had in the past to maintain cx25840 as a single unit is breaking down and showing it's warts. > > >> However, the cx25840 driver is close enough to the avcore that doing a >> cx23885-avcore.c and largely closing all of the source code from cx25840 is an >> exercise in growing the linuxtv tree for little value. > > I disagree. In modern computing systems disk space and memory are cheap > and only getting cheaper. Then I don't want to see a half-hearted nonsense attempt at some 'cx25840-core' module that is shared in a loose sense, it's added engineering for the sake of it. It doesn't fix the regression issues long term. The solution is probably clone the entire code base into the cx23885 driver. However, if I'm going to do this then I may as well go with the 25840 windows reference driver which is fully features, from the cx23885 source code itself. Unwanted legacy is dropped and the cx23885 project with relation to cx25840 specific analog is new, so we have nothing to lose and everything to gain. I'll drop the I2C subdevice interface all together and go with a cx23885_avcore_N() set of functions which do direct I2C on the bus. <metrics snipped> > >From a maintenance perspective, I see quite a case for splitting > CX2310[12] and CX2388[578] support out of the cx25840 module and putting > it somewhere else. The gains in reduced maintainenance time and effort > far outweigh the cost of any bloat. I think I'd rather take a future action and port the reference code at this point. > > >> > 3. This caught my eye: >> > >> > 8 + if (is_cx2388x(state)) { >> > 9 + /* for cx23885 volume doesn't work, >> > 10 + * the calculation always results in >> > 11 + * e4 regardless. >> > 12 + */ >> > 13 + cx25840_write(client, 0x8d4, volume); >> > 14 + } else >> > 15 + cx25840_write(client, 0x8d4, 228 - (vol * 2)); >> > >> > Why is the result always 0xe4? Is that what the register always reads >> > back? >> >> Yes. > > The cx25840 module expects the volume control to have a range from > 0-65535. It divides the input control value by 512 (i.e. volume >> 9). > If you are using a control in the cx23885 driver with a range from 0-100 > or 0-255, that would explain why it doesn't work. Correct. > > BTW, the actual relationship the cx25840 module is implementing is > explained in the end of this old post: > > http://www.gossamer-threads.com/lists/ivtv/devel/39868#39868 Thanks for the feedback Andy. - Steve -- Steven Toth - Kernel Labs http://www.kernellabs.com -- 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
