On Sat, 29 Sep 2012, Jonathan Corbet wrote: > On Fri, 28 Sep 2012 21:47:20 +0800 > Albert Wang <twang13@xxxxxxxxxxx> wrote: > > > This patch adds the support of Soc Camera on marvell-ccic mcam-core. > > The Soc Camera mode does not compatible with current mode. > > Only one mode can be used at one time. > > > > To use Soc Camera, CONFIG_VIDEO_MMP_SOC_CAMERA should be defined. > > What's more, the platform driver should support Soc camera at the same time. > > > > Also add MIPI interface and dual CCICs support in Soc Camera mode. > > I'm glad this work is being done, but I have some high-level grumbles > to start with. > > This patch is too big, and does several things. I think there needs to > be one to add SOC support (but see below), one to add planar formats, > one to add MIPI, one for the second CCIC, etc. That will make them all > easier to review. > > The SOC camera stuff could maybe use a little more thought. Why does > this driver *need* to be a SOC camera driver? It probably doesn't, but if the author wishes to do so - we can try to do this cleanly. > If that is truly > necessary (or sufficiently beneficial), can we get to the point where > that's the only mode? I really dislike the two modes; we're > essentially perpetuating the two-drivers concept in a #ifdef'd form; it > would be good not to do that. > > If there is truly some reason why both modes need to exist, can we > arrange things so that the core doesn't know the difference? I'd like > to see no new ifdefs there if possible, it already has way too many. A strong +1. Ideally we should identify common code, add soc-camera mode as a separate file and re-use the common stuff. > That, I think, is how I'd like to go toward a cleaner, more reviewable, > more maintainable solution. Make sense? Definitely! Thanks Guennadi > Thanks, > > jon > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
