On Mon, Jun 1, 2009 at 6:04 PM, Douglas Schilling Landgraf <dougsland@xxxxxxxxx> wrote: > Hello Devin, > > On Mon, 1 Jun 2009 11:19:22 -0400 > Devin Heitmueller <dheitmueller@xxxxxxxxxxxxxx> wrote: > >> On Sat, May 23, 2009 at 11:05 AM, Markus Rechberger >> <mrechberger@xxxxxxxxx> wrote: >> > Hi, >> > >> > On Sat, May 23, 2009 at 4:49 PM, Markus Rechberger >> > <mrechberger@xxxxxxxxx> wrote: >> >> On Sat, May 23, 2009 at 4:04 PM, Markus Rechberger >> >> <mrechberger@xxxxxxxxx> wrote: >> >>> Hi, >> >>> >> >>> for em28xx devices the device node detection can be based on the >> >>> encoded endpoint address, for example EP 0x81 (USB IN, Interrupt), >> >>> 0x82 (analog video EP), 0x83 (analog audio ep), 0x84 (mpeg-ts >> >>> input EP). >> >>> It is not necessary that digital TV devices have a frontend, the >> >>> em28xx chip only specifies an MPEG-TS input EP. >> >>> >> >>> Following patch adds a check based on the Endpoints, although it >> >>> might be extended that all devices match the possible devicenodes >> >>> based on the endpoints, currently the driver registers an analog >> >>> TV node by default for all unknown devices which is not >> >>> necessarily correct, this patch disables the ATV node if no >> >>> analog TV endpoint is available. >> >>> >> >> >> > >> > attached patch fixes the deregistration, as well loads the >> > em28xx-dvb module automatically as soon as an MPEG-TS endpoint was >> > found. >> > >> > Signed-off-by: Markus Rechberger <mrechberger@xxxxxxxxx> >> > >> > best regards, >> > Markus >> > >> >> Hello Markus, >> >> I spent some time reviewing this patch, and the patch's content does >> not seem to match your description of its functionality. Further, >> this patch appears to be a combination of a number of several >> different changes, rather than being broken into separate patches. >> >> First off, I totally agree that the analog subsystem should not be >> loaded on devices such as em287[0-4]. I was going to do this work >> (using the chip id to determine analog support) but just had not had a >> chance to doing the necessary testing to ensure it did not break >> anything. >> >> The patch appears to be primarily for devices that are not supported >> in the kernel. In fact, the logic as written *only* gets used for >> unknown devices. Further, the code that doesn't create the frontend >> device has no application in the kernel. All devices currently in the >> kernel make use of the dvb frontend interface, so there is no >> practical application to loading the driver and setting up the isoc >> handlers but blocking access to the dvb frontend device. >> >> Aside from the code that selectively disables analog support, the >> patch only seems to advance compatibility with your userland em28xx >> framework while providing no benefit to the in-kernel driver. >> >> Regarding the possibility of custom firmware, we currently do not have >> any devices in the in-kernel driver that make use of custom firmware. >> If you could tell me how to check for custom firmware versus the >> default vendor firmware, I could potentially do a patch that uses the >> vendor registers unless custom firmware is installed, at which point >> we could have custom logic (such as using the endpoint definition). >> However, given there are no such devices in-kernel, this is not a high >> priority as far as I am concerned. >> >> For what it's worth, I did add an additional patch to allow the user >> to disable the 480Mbps check via a modprobe option (to avoid a >> regression for any of your existing customers), and I will be checking >> in the code to properly compute the isoc size for em2874/em2884 based >> on the vendor registers (even though there are currently no supported >> devices in the kernel that require it currently). However, I do not >> believe the patch you have proposed is appropriate for inclusion in >> the mainline kernel. > > Agree with you Devin. > > Also, the patch does a lot of changes instead of break it in several > patches. > do you want smaller patches? regards, Markus -- 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
