Hi Mauro, On Sunday 04 September 2011 15:32:28 Mauro Carvalho Chehab wrote: > Em 04-09-2011 06:01, Laurent Pinchart escreveu: > > On Sunday 04 September 2011 00:21:38 Mauro Carvalho Chehab wrote: > >> Em 24-08-2011 10:25, Laurent Pinchart escreveu: > >>> On Wednesday 24 August 2011 14:19:01 Hiremath, Vaibhav wrote: > >>>> On Wednesday, August 24, 2011 5:00 PM Laurent Pinchart wrote: > >>>>> On Wednesday 24 August 2011 13:21:27 Ravi, Deepthy wrote: > >>>>>> On Wed, Aug 24, 2011 at 4:47 PM, Laurent Pinchart wrote: > >>>>>>> On Friday 19 August 2011 15:48:45 Deepthy Ravi wrote: > >>>>>>>> From: Vaibhav Hiremath <hvaibhav@xxxxxx> > >>>>>>>> > >>>>>>>> Fix the build break caused when CONFIG_MEDIA_CONTROLLER > >>>>>>>> option is disabled and if any sensor driver has to be used > >>>>>>>> between MC and non MC framework compatible devices. > >>>>>>>> > >>>>>>>> For example,if tvp514x video decoder driver migrated to > >>>>>>>> MC framework is being built without CONFIG_MEDIA_CONTROLLER > >>>>>>>> option enabled, the following error messages will result. > >>>>>>>> drivers/built-in.o: In function `tvp514x_remove': > >>>>>>>> drivers/media/video/tvp514x.c:1285: undefined reference to > >>>>>>>> `media_entity_cleanup' > >>>>>>>> drivers/built-in.o: In function `tvp514x_probe': > >>>>>>>> drivers/media/video/tvp514x.c:1237: undefined reference to > >>>>>>>> `media_entity_init' > >>>>>>> > >>>>>>> If the tvp514x is migrated to the MC framework, its Kconfig option > >>>>>>> should depend on MEDIA_CONTROLLER. > >>>>>> > >>>>>> The same TVP514x driver is being used for both MC and non MC > >>>>>> compatible devices, for example OMAP3 and AM35x. So if it is made > >>>>>> dependent on MEDIA CONTROLLER, we cannot enable the driver for MC > >>>>>> independent devices. > >>>>> > >>>>> Then you should use conditional compilation in the tvp514x driver > >>>>> itself. Or > >>>> > >>>> No. I am not in favor of conditional compilation in driver code. > >>> > >>> Actually, thinking some more about this, you should make the tvp514x > >>> driver depend on CONFIG_MEDIA_CONTROLLER unconditionally. This doesn't > >>> mean that the driver will become unusable by applications that are not > >>> MC-aware. Hosts/bridges don't have to export subdev nodes, they can > >>> just call subdev pad-level operations internally and let applications > >>> control the whole device through a single V4L2 video node. > >>> > >>>>> better, port the AM35x driver to the MC API. > >>>> > >>>> Why should we use MC if I have very simple device (like AM35x) which > >>>> only supports single path? I can very well use simple V4L2 sub-dev > >>>> based approach (master - slave), isn't it? > >>> > >>> The AM35x driver should use the in-kernel MC and V4L2 subdev APIs, but > >>> it doesn't have to expose them to userspace. > >> > >> I don't agree. If AM35x doesn't expose the MC API to userspace, > >> CONFIG_MEDIA_CONTROLLER should not be required at all. > >> > >> Also, according with the Linux best practices, when #if tests for > >> config symbols are required, developers should put it into the header > >> files, and not inside the code, as it helps to improve code > >> readability. From > >> > >> Documentation/SubmittingPatches: > >> 2) #ifdefs are ugly > >> > >> Code cluttered with ifdefs is difficult to read and maintain. Don't do > >> it. Instead, put your ifdefs in a header, and conditionally define > >> 'static inline' functions, or macros, which are used in the code. > >> Let the compiler optimize away the "no-op" case. > >> > >> So, this patch is perfectly fine on my eyes. > > > > I'm sorry, but I don't agree. > > > > Regarding the V4L2 subdev pad-level API, the goal is to convert all host > > and subdev drivers to it, so that's definitely the way to go. This does > > *not* mean that subdevs must expose a subdev device node. That's > > entirely optional. What I'm talking about is switching from > > video::*_mbus_fmt operations to pad::*_fmt operations. The pad-level > > format operations are very similar to video-level format operations, and > > more generic. Drivers shouldn't implement both. > > I agree that implementing two ways for doing the same thing is a bad idea, > but especially since your idea is to convert all subdevs to it, this type > of conversion should not require enabling CONFIG_MEDIA_CONTROLLER, as this > feature is used to enable the MC userspace API. > > > Regarding the MC API, drivers are not required to register a media_device > > instance. I have no issue with that. However, drivers should initialized > > the subdev's embedded media_entity, as that's required by subdev > > pad-level operations to get the number of pads for a subdev. > > There are two solutions: > > 1) add some "fallback" method at the core to use the video::*_mbus_fmt way, > when MC is disabled; > > 2) split the config options into two: one configurable by the user to > enable the userspace MC API, and another, used internally that would > select the MC internal API when drivers need it. > > As your plan is to convert all drivers to the new way, (2) doesn't make > much sense in long term, as, at the end, all drivers will be selecting it. But (1) makes even less sense :-) The issue here is that MC-enabled drivers will use pad-level subdev operations, so those operations need to be implemented in subdev drivers used by MC-enabled hosts/bridges. I don't like the idea of having two sets of similar operations in subdevs, as that will result in duplicate code. We should thus implement only pad-level subdev operations for MC-aware subdevs (a wrapper method can be implemented in the code to convert video operations to subdev operations transparently for non MC-aware hosts/bridges). This requires media_entity_init() and media_entity_cleanup() being available to those subdev drivers regardless of whether the host/bridge exposes the MC API to userspace. I don't mind splitting the config option. An alternative would be to compile media_entity_init() and media_entity_cleanup() based on CONFIG_MEDIA_SUPPORT instead of CONFIG_MEDIA_CONTROLLER, but that looks a bit hackish to me. > Also, I don't like the idea of increasing drivers complexity for the > existing drivers that work properly without MC. All those core conversions > that were done in the last two years caused already too much instability > to them. > > We should really avoid touching on them again for something that won't be > adding any new feature nor fixing any known bug. We don't have to convert them all in one go right now, we can implement pad- level operations support selectively when a subdev driver becomes used by an MC-enabled host/bridge driver. > > This will result in no modification to the userspace. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
