On Mon, Nov 22, 2010 at 04:19:42PM +0100, ext Aguirre, Sergio wrote: > 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. :) You're true. Maybe we could rework in future part of the driver's PM. I'm fine with this patch. :) I only suggested something to make it a little better. But as AUTOIDLE is not being set anyway, the result is the same. Br, David > > 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 -- 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
