Hi, Jonathan We really appreciate you can review these patches! Sorry for late response. >-----Original Message----- >From: Jonathan Corbet [mailto:corbet@xxxxxxx] >Sent: Sunday, 30 September, 2012 03:41 >To: Albert Wang >Cc: g.liakhovetski@xxxxxx; linux-media@xxxxxxxxxxxxxxx; Libin Yang >Subject: Re: [PATCH 2/4] [media] marvell-ccic: core: add soc camera support on >marvell-ccic mcam-core > >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. > Yes. Your concern is reasonable, I can understand it. Actually, we ever try to split the patch into some smaller ones, but it looks will let thing be more complicated. So we keep the 2 big patches and look forward your comments and suggestions firstly. We will continue to discuss how to split them if you insist. >The SOC camera stuff could maybe use a little more thought. Why does this driver >*need* to be a SOC camera driver? 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. > Yes, #ifdef is indeed not a good method. We will continue to discuss how to remove them. Maybe I can describe that why we add SOC camera mode: SOC camera is optional for camera driver, so we try to keep the original method of marvell-ccic Just let user to select use SOC camera or not use it >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. > >That, I think, is how I'd like to go toward a cleaner, more reviewable, more maintainable >solution. Make sense? > Yes. We agree with you! :) >Thanks, > >jon Thanks Albert Wang 86-21-61092656 -- 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
