Hi, > -----Original Message----- > From: David Cohen [mailto:david.cohen@xxxxxxxxx] > Sent: Saturday, November 20, 2010 4:49 AM > To: ext Laurent Pinchart > Cc: Aguirre, Sergio; linux-media@xxxxxxxxxxxxxxx > Subject: Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG > fields consistent > > On Sat, Nov 20, 2010 at 11:34:04AM +0100, ext Laurent Pinchart wrote: > > Hi, > > > > On Saturday 20 November 2010 11:00:30 David Cohen wrote: > > > On Sat, Nov 20, 2010 at 12:10:11AM +0100, ext Aguirre, Sergio wrote: > > > > On Friday, November 19, 2010 9:44 AM Aguirre, Sergio wrote: > > > > > On Friday, November 19, 2010 4:06 AM David Cohen wrote: > > > > > > On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre > wrote: > > > > > > > Signed-off-by: Sergio Aguirre <saaguirre@xxxxxx> > > > > > > > --- > > > > > > > > > > > > > > drivers/media/video/isp/ispccp2.c | 3 +-- > > > > > > > drivers/media/video/isp/ispreg.h | 14 ++++++++------ > > > > > > > 2 files changed, 9 insertions(+), 8 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/media/video/isp/ispccp2.c > > > > > > > > > > > > b/drivers/media/video/isp/ispccp2.c > > > > > > > > > > > > > index fa23394..3127a74 100644 > > > > > > > --- a/drivers/media/video/isp/ispccp2.c > > > > > > > +++ b/drivers/media/video/isp/ispccp2.c > > > > > > > @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct > > > > > > > > > > > > isp_ccp2_device *ccp2, > > > > > > > > > > > > > config->src_ofst = 0; > > > > > > > > > > > > > > } > > > > > > > > > > > > > > - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY << > > > > > > > - ISPCSI1_MIDLEMODE_SHIFT), > > > > > > > + isp_reg_writel(isp, > ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, > > > > > > > > > > > > > > OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); > > > > > > > > > > > > To make your cleanup even better, you could change > isp_reg_clr_set() > > > > > > instead. > > > > > > If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to > be an > > > > > > invalid 0x3 value. Despite it cannot happen with the current > code, > > > > > > it's still better to avoid such situations for future versions. > :) > > > > > > > > > > Hmm you're right, I guess I didn't do any real functional changes, > just > > > > > defines renaming. > > > > > > > > > > I can repost an updated patch with this suggestion. No problem. > > > > > > > > David, > > > > > > > > Geez I guess I wasn't paying much attention to this either. Sorry. > > > > > > > > The case you mention about it potentially become 0x3 is not > possible, > > > > because, I'm basically overwriting the whole register with (0x2 << > 12) > > > > Notice the isp_reg_write, there's no OR operation... > > > > > > That's correct. My mistake. > > > IMO ISP power settings still need a better way to be setup, but it > > > definitively does not belong to your cleanup patch. > > > I'm fine with your changes. > > > > > > > Now, this means that we have been implicitly setting other fields as > > > > Zeroes (SOFT_RESET = 0, and AUTO_IDLE = 0) aswell. > > > > > > > > Has AUTOIDLE in CCP2 been tried in N900? I don't have a CCP2 sensor > to > > > > test with :(. > > > > > > I have no idea. Indeed, AUTOIDLE in ISP doesn't seem to be much > reliable > > > so far. It should be set to 0 until somebody proves it can be set to 1 > :) > > > > To summarize things, we're only setting the CCP2 AUTOIDLE bit on ES 1.0 > > devices, and we're clearing it anyway ispccp2_mem_configure(). For the > sake of > > correctness we should replace isp_reg_writel() by isp_reg_clr_set() in > > ispccp2_mem_configure(), which will only make a difference on ES 1.0 > devices > > that have basically no users. Is that OK with both of you ? Yes, It's fine with me. I'll say that if this doesn't change things for N900, which appears to be the only community available device (I know of) that uses CCP2 cameras, then we shouldn't care. If somebody comes up with an ES1.0 in the future, and he does see problems, then we will address that. But that would be a very weird case. In fact, it will be most probably someone from TI with a hidden board buried in some box of scraps, which happens to have hacked the board for a CCP2 camera... The more I think about it, the more I'm getting convinced this shouldn't be a worry. :) Regards, Sergio > > I'm fine with both solutions. > IMO power settings could be improved and not be hardcoded. It could come > from > platform data. But that's another case. :) > > Br, > > David > > > > > -- > > Regards, > > > > Laurent Pinchart > > -- > > 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 -- 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
