[omap3isp,v2,7/9] omap3isp: Cleanup isp_power_settings

Message ID	1289831401-593-8-git-send-email-saaguirre@ti.com (mailing list archive)
State	Superseded, archived
Headers	Envelope-to: mchehab@pedra Delivery-date: Tue, 16 Nov 2010 09:49:53 -0200 From: Sergio Aguirre <saaguirre@ti.com> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: linux-media@vger.kernel.org, Sergio Aguirre <saaguirre@ti.com> Subject: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings Date: Mon, 15 Nov 2010 08:29:59 -0600 Message-Id: <1289831401-593-8-git-send-email-saaguirre@ti.com> In-Reply-To: <1289831401-593-1-git-send-email-saaguirre@ti.com> References: <1289831401-593-1-git-send-email-saaguirre@ti.com> Precedence: bulk Sender: <mchehab@pedra>

Message ID

1289831401-593-8-git-send-email-saaguirre@ti.com (mailing list archive)

State

Superseded, archived

Headers

Envelope-to: mchehab@pedra
Delivery-date: Tue, 16 Nov 2010 09:49:53 -0200
From: Sergio Aguirre <saaguirre@ti.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org, Sergio Aguirre <saaguirre@ti.com>
Subject: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings
Date: Mon, 15 Nov 2010 08:29:59 -0600
Message-Id: <1289831401-593-8-git-send-email-saaguirre@ti.com>
In-Reply-To: <1289831401-593-1-git-send-email-saaguirre@ti.com>
References: <1289831401-593-1-git-send-email-saaguirre@ti.com>
Precedence: bulk
Sender: <mchehab@pedra>

Commit Message

Aguirre Rodriguez, Sergio Alberto Nov. 15, 2010, 2:29 p.m. UTC

  1. Get rid of CSI2 / CCP2 power settings, as they are controlled
   in the receivers code anyways.
2. Avoid code duplication.

Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
---
 drivers/media/video/isp/isp.c |   49 ++++++-----------------------------------
 1 files changed, 7 insertions(+), 42 deletions(-)

Comments

David Cohen Nov. 19, 2010, 10:18 a.m. UTC | #1

Hi Sergio,

Thanks for the patch.

On Mon, Nov 15, 2010 at 03:29:59PM +0100, ext Sergio Aguirre wrote:
> 1. Get rid of CSI2 / CCP2 power settings, as they are controlled
>    in the receivers code anyways.

CCP2 is not correctly handling this. It's setting SMART STANDBY mode one
when reading from memory. You should fix it before remove such code from
ISP core driver.

> 2. Avoid code duplication.

Agree. But only after considering the comment above.

Regards,

David

> 
> Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> ---
>  drivers/media/video/isp/isp.c |   49 ++++++-----------------------------------
>  1 files changed, 7 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c
> index de9352b..30bdc48 100644
> --- a/drivers/media/video/isp/isp.c
> +++ b/drivers/media/video/isp/isp.c
> @@ -254,48 +254,13 @@ EXPORT_SYMBOL(isp_set_xclk);
>   */
>  static void isp_power_settings(struct isp_device *isp, int idle)
>  {
> -	if (idle) {
> -		isp_reg_writel(isp,
> -			       (ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY <<
> -				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> -			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> -		if (omap_rev() == OMAP3430_REV_ES1_0) {
> -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> -				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
> -					ISPCSI1_MIDLEMODE_SHIFT),
> -				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
> -				       ISPCSI2_SYSCONFIG);
> -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> -				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
> -					ISPCSI1_MIDLEMODE_SHIFT),
> -				       OMAP3_ISP_IOMEM_CCP2,
> -				       ISPCCP2_SYSCONFIG);
> -		}
> -		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
> -			       ISP_CTRL);
> -
> -	} else {
> -		isp_reg_writel(isp,
> -			       (ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY <<
> -				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> -			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> -		if (omap_rev() == OMAP3430_REV_ES1_0) {
> -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> -				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
> -					ISPCSI1_MIDLEMODE_SHIFT),
> -				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
> -				       ISPCSI2_SYSCONFIG);
> -
> -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> -				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
> -					ISPCSI1_MIDLEMODE_SHIFT),
> -				       OMAP3_ISP_IOMEM_CCP2,
> -				       ISPCCP2_SYSCONFIG);
> -		}
> -
> -		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
> -			       ISP_CTRL);
> -	}
> +	isp_reg_writel(isp,
> +		       ((idle ? ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY :
> +				ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY) <<
> +			ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> +		       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> +	isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
> +		       ISP_CTRL);
>  }
>  
>  /*
> -- 
> 1.7.0.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Aguirre Rodriguez, Sergio Alberto Nov. 19, 2010, 3:46 p.m. UTC | #2

> -----Original Message-----
> From: David Cohen [mailto:david.cohen@nokia.com]
> Sent: Friday, November 19, 2010 4:18 AM
> To: Aguirre, Sergio
> Cc: Laurent Pinchart; linux-media@vger.kernel.org
> Subject: Re: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings
> 
> Hi Sergio,
> 
> Thanks for the patch.

Hi David,

Thanks for the comments.

> 
> On Mon, Nov 15, 2010 at 03:29:59PM +0100, ext Sergio Aguirre wrote:
> > 1. Get rid of CSI2 / CCP2 power settings, as they are controlled
> >    in the receivers code anyways.
> 
> CCP2 is not correctly handling this. It's setting SMART STANDBY mode one
> when reading from memory. You should fix it before remove such code from
> ISP core driver.

Ok, agreed.

I'll generate a new patch before this to compensate that. Not a problem.

> 
> > 2. Avoid code duplication.
> 
> Agree. But only after considering the comment above.

Ok.

Thanks and Regards,
Sergio

> 
> Regards,
> 
> David
> 
> >
> > Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> > ---
> >  drivers/media/video/isp/isp.c |   49 ++++++----------------------------
> -------
> >  1 files changed, 7 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/media/video/isp/isp.c
> b/drivers/media/video/isp/isp.c
> > index de9352b..30bdc48 100644
> > --- a/drivers/media/video/isp/isp.c
> > +++ b/drivers/media/video/isp/isp.c
> > @@ -254,48 +254,13 @@ EXPORT_SYMBOL(isp_set_xclk);
> >   */
> >  static void isp_power_settings(struct isp_device *isp, int idle)
> >  {
> > -	if (idle) {
> > -		isp_reg_writel(isp,
> > -			       (ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY <<
> > -				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > -			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > -		if (omap_rev() == OMAP3430_REV_ES1_0) {
> > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > -				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
> > -					ISPCSI1_MIDLEMODE_SHIFT),
> > -				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
> > -				       ISPCSI2_SYSCONFIG);
> > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > -				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
> > -					ISPCSI1_MIDLEMODE_SHIFT),
> > -				       OMAP3_ISP_IOMEM_CCP2,
> > -				       ISPCCP2_SYSCONFIG);
> > -		}
> > -		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE,
> OMAP3_ISP_IOMEM_MAIN,
> > -			       ISP_CTRL);
> > -
> > -	} else {
> > -		isp_reg_writel(isp,
> > -			       (ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY <<
> > -				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > -			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > -		if (omap_rev() == OMAP3430_REV_ES1_0) {
> > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > -				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
> > -					ISPCSI1_MIDLEMODE_SHIFT),
> > -				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
> > -				       ISPCSI2_SYSCONFIG);
> > -
> > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > -				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
> > -					ISPCSI1_MIDLEMODE_SHIFT),
> > -				       OMAP3_ISP_IOMEM_CCP2,
> > -				       ISPCCP2_SYSCONFIG);
> > -		}
> > -
> > -		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE,
> OMAP3_ISP_IOMEM_MAIN,
> > -			       ISP_CTRL);
> > -	}
> > +	isp_reg_writel(isp,
> > +		       ((idle ? ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY :
> > +				ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY) <<
> > +			ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > +		       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > +	isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
> > +		       ISP_CTRL);
> >  }
> >
> >  /*
> > --
> > 1.7.0.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media"
> in
> > the body of a message to majordomo@vger.kernel.org
> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Aguirre Rodriguez, Sergio Alberto Nov. 19, 2010, 10:58 p.m. UTC | #3

> -----Original Message-----
> From: Aguirre, Sergio
> Sent: Friday, November 19, 2010 9:46 AM
> To: 'David Cohen'
> Cc: Laurent Pinchart; linux-media@vger.kernel.org
> Subject: RE: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings
> > -----Original Message-----
> > From: David Cohen [mailto:david.cohen@nokia.com]
> > Sent: Friday, November 19, 2010 4:18 AM
> > To: Aguirre, Sergio
> > Cc: Laurent Pinchart; linux-media@vger.kernel.org
> > Subject: Re: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup
> isp_power_settings
> >
> > Hi Sergio,
> >
> > Thanks for the patch.
> 
> Hi David,
> 
> Thanks for the comments.
> 
> >
> > On Mon, Nov 15, 2010 at 03:29:59PM +0100, ext Sergio Aguirre wrote:
> > > 1. Get rid of CSI2 / CCP2 power settings, as they are controlled
> > >    in the receivers code anyways.
> >
> > CCP2 is not correctly handling this. It's setting SMART STANDBY mode one
> > when reading from memory. You should fix it before remove such code from
> > ISP core driver.
> 
> Ok, agreed.
> 
> I'll generate a new patch before this to compensate that. Not a problem.

Is N900 seeing any functional difference w/ this patch?

Actually, I reanalyzed the patch, and this code should be unexecuted, since
it is conditioned to 3430 ES1.0 chip (omap_rev() == OMAP3430_REV_ES1_0),
which I don't think much people has access. And not definitely any
production quality device.

Even the N900 is using ES3.1 or something like that AFAIK.

So, It should not make any functional difference, unless you have an ES1.0.

Regards,
Sergio

> 
> >
> > > 2. Avoid code duplication.
> >
> > Agree. But only after considering the comment above.
> 
> Ok.
> 
> Thanks and Regards,
> Sergio
> 
> >
> > Regards,
> >
> > David
> >
> > >
> > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> > > ---
> > >  drivers/media/video/isp/isp.c |   49 ++++++--------------------------
> --
> > -------
> > >  1 files changed, 7 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/drivers/media/video/isp/isp.c
> > b/drivers/media/video/isp/isp.c
> > > index de9352b..30bdc48 100644
> > > --- a/drivers/media/video/isp/isp.c
> > > +++ b/drivers/media/video/isp/isp.c
> > > @@ -254,48 +254,13 @@ EXPORT_SYMBOL(isp_set_xclk);
> > >   */
> > >  static void isp_power_settings(struct isp_device *isp, int idle)
> > >  {
> > > -	if (idle) {
> > > -		isp_reg_writel(isp,
> > > -			       (ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY <<
> > > -				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > > -			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > > -		if (omap_rev() == OMAP3430_REV_ES1_0) {
> > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > -				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
> > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > -				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
> > > -				       ISPCSI2_SYSCONFIG);
> > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > -				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
> > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > -				       OMAP3_ISP_IOMEM_CCP2,
> > > -				       ISPCCP2_SYSCONFIG);
> > > -		}
> > > -		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE,
> > OMAP3_ISP_IOMEM_MAIN,
> > > -			       ISP_CTRL);
> > > -
> > > -	} else {
> > > -		isp_reg_writel(isp,
> > > -			       (ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY <<
> > > -				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > > -			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > > -		if (omap_rev() == OMAP3430_REV_ES1_0) {
> > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > -				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
> > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > -				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
> > > -				       ISPCSI2_SYSCONFIG);
> > > -
> > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > -				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
> > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > -				       OMAP3_ISP_IOMEM_CCP2,
> > > -				       ISPCCP2_SYSCONFIG);
> > > -		}
> > > -
> > > -		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE,
> > OMAP3_ISP_IOMEM_MAIN,
> > > -			       ISP_CTRL);
> > > -	}
> > > +	isp_reg_writel(isp,
> > > +		       ((idle ? ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY :
> > > +				ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY) <<
> > > +			ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > > +		       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > > +	isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
> > > +		       ISP_CTRL);
> > >  }
> > >
> > >  /*
> > > --
> > > 1.7.0.4
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-media"
> > in
> > > the body of a message to majordomo@vger.kernel.org
> > > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Cohen Nov. 20, 2010, 10:45 a.m. UTC | #4

On Fri, Nov 19, 2010 at 11:58:32PM +0100, ext Aguirre, Sergio wrote:
> 
> 
> > -----Original Message-----
> > From: Aguirre, Sergio
> > Sent: Friday, November 19, 2010 9:46 AM
> > To: 'David Cohen'
> > Cc: Laurent Pinchart; linux-media@vger.kernel.org
> > Subject: RE: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings
> > > -----Original Message-----
> > > From: David Cohen [mailto:david.cohen@nokia.com]
> > > Sent: Friday, November 19, 2010 4:18 AM
> > > To: Aguirre, Sergio
> > > Cc: Laurent Pinchart; linux-media@vger.kernel.org
> > > Subject: Re: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup
> > isp_power_settings
> > >
> > > Hi Sergio,
> > >
> > > Thanks for the patch.
> > 
> > Hi David,
> > 
> > Thanks for the comments.
> > 
> > >
> > > On Mon, Nov 15, 2010 at 03:29:59PM +0100, ext Sergio Aguirre wrote:
> > > > 1. Get rid of CSI2 / CCP2 power settings, as they are controlled
> > > >    in the receivers code anyways.
> > >
> > > CCP2 is not correctly handling this. It's setting SMART STANDBY mode one
> > > when reading from memory. You should fix it before remove such code from
> > > ISP core driver.
> > 
> > Ok, agreed.
> > 
> > I'll generate a new patch before this to compensate that. Not a problem.
> 
> Is N900 seeing any functional difference w/ this patch?
> 
> Actually, I reanalyzed the patch, and this code should be unexecuted, since
> it is conditioned to 3430 ES1.0 chip (omap_rev() == OMAP3430_REV_ES1_0),
> which I don't think much people has access. And not definitely any
> production quality device.
> 
> Even the N900 is using ES3.1 or something like that AFAIK.
> 
> So, It should not make any functional difference, unless you have an ES1.0.

Your patch is correct once the code you're removing does not belong to
the ISP core driver, but you're not only getting rid of duplicated code.
You're also removing code for an old version.

I'm not much confortable with this, but I won't disagree if you decide
to keep this patch, once you're not breaking any compatibility. But then
I need to revisit it in future and implement a better way to add the
missing code again.

Br,

David

> 
> Regards,
> Sergio
> 
> > 
> > >
> > > > 2. Avoid code duplication.
> > >
> > > Agree. But only after considering the comment above.
> > 
> > Ok.
> > 
> > Thanks and Regards,
> > Sergio
> > 
> > >
> > > Regards,
> > >
> > > David
> > >
> > > >
> > > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> > > > ---
> > > >  drivers/media/video/isp/isp.c |   49 ++++++--------------------------
> > --
> > > -------
> > > >  1 files changed, 7 insertions(+), 42 deletions(-)
> > > >
> > > > diff --git a/drivers/media/video/isp/isp.c
> > > b/drivers/media/video/isp/isp.c
> > > > index de9352b..30bdc48 100644
> > > > --- a/drivers/media/video/isp/isp.c
> > > > +++ b/drivers/media/video/isp/isp.c
> > > > @@ -254,48 +254,13 @@ EXPORT_SYMBOL(isp_set_xclk);
> > > >   */
> > > >  static void isp_power_settings(struct isp_device *isp, int idle)
> > > >  {
> > > > -	if (idle) {
> > > > -		isp_reg_writel(isp,
> > > > -			       (ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY <<
> > > > -				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > > > -			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > > > -		if (omap_rev() == OMAP3430_REV_ES1_0) {
> > > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > > -				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
> > > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > > -				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
> > > > -				       ISPCSI2_SYSCONFIG);
> > > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > > -				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
> > > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > > -				       OMAP3_ISP_IOMEM_CCP2,
> > > > -				       ISPCCP2_SYSCONFIG);
> > > > -		}
> > > > -		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE,
> > > OMAP3_ISP_IOMEM_MAIN,
> > > > -			       ISP_CTRL);
> > > > -
> > > > -	} else {
> > > > -		isp_reg_writel(isp,
> > > > -			       (ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY <<
> > > > -				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > > > -			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > > > -		if (omap_rev() == OMAP3430_REV_ES1_0) {
> > > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > > -				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
> > > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > > -				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
> > > > -				       ISPCSI2_SYSCONFIG);
> > > > -
> > > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > > -				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
> > > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > > -				       OMAP3_ISP_IOMEM_CCP2,
> > > > -				       ISPCCP2_SYSCONFIG);
> > > > -		}
> > > > -
> > > > -		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE,
> > > OMAP3_ISP_IOMEM_MAIN,
> > > > -			       ISP_CTRL);
> > > > -	}
> > > > +	isp_reg_writel(isp,
> > > > +		       ((idle ? ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY :
> > > > +				ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY) <<
> > > > +			ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > > > +		       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > > > +	isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
> > > > +		       ISP_CTRL);
> > > >  }
> > > >
> > > >  /*
> > > > --
> > > > 1.7.0.4
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-media"
> > > in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Cohen Nov. 20, 2010, 10:49 a.m. UTC | #5

On Sat, Nov 20, 2010 at 11:45:21AM +0100, Cohen David (Nokia-MS/Helsinki) wrote:
> On Fri, Nov 19, 2010 at 11:58:32PM +0100, ext Aguirre, Sergio wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Aguirre, Sergio
> > > Sent: Friday, November 19, 2010 9:46 AM
> > > To: 'David Cohen'
> > > Cc: Laurent Pinchart; linux-media@vger.kernel.org
> > > Subject: RE: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup isp_power_settings
> > > > -----Original Message-----
> > > > From: David Cohen [mailto:david.cohen@nokia.com]
> > > > Sent: Friday, November 19, 2010 4:18 AM
> > > > To: Aguirre, Sergio
> > > > Cc: Laurent Pinchart; linux-media@vger.kernel.org
> > > > Subject: Re: [omap3isp][PATCH v2 7/9] omap3isp: Cleanup
> > > isp_power_settings
> > > >
> > > > Hi Sergio,
> > > >
> > > > Thanks for the patch.
> > > 
> > > Hi David,
> > > 
> > > Thanks for the comments.
> > > 
> > > >
> > > > On Mon, Nov 15, 2010 at 03:29:59PM +0100, ext Sergio Aguirre wrote:
> > > > > 1. Get rid of CSI2 / CCP2 power settings, as they are controlled
> > > > >    in the receivers code anyways.
> > > >
> > > > CCP2 is not correctly handling this. It's setting SMART STANDBY mode one
> > > > when reading from memory. You should fix it before remove such code from
> > > > ISP core driver.
> > > 
> > > Ok, agreed.
> > > 
> > > I'll generate a new patch before this to compensate that. Not a problem.
> > 
> > Is N900 seeing any functional difference w/ this patch?
> > 
> > Actually, I reanalyzed the patch, and this code should be unexecuted, since
> > it is conditioned to 3430 ES1.0 chip (omap_rev() == OMAP3430_REV_ES1_0),
> > which I don't think much people has access. And not definitely any
> > production quality device.
> > 
> > Even the N900 is using ES3.1 or something like that AFAIK.
> > 
> > So, It should not make any functional difference, unless you have an ES1.0.
> 
> Your patch is correct once the code you're removing does not belong to
> the ISP core driver, but you're not only getting rid of duplicated code.
> You're also removing code for an old version.
> 
> I'm not much confortable with this, but I won't disagree if you decide
> to keep this patch, once you're not breaking any compatibility. But then
> I need to revisit it in future and implement a better way to add the

s/I need/we need/ :)

David

> missing code again.
> 
> Br,
> 
> David
> 
> > 
> > Regards,
> > Sergio
> > 
> > > 
> > > >
> > > > > 2. Avoid code duplication.
> > > >
> > > > Agree. But only after considering the comment above.
> > > 
> > > Ok.
> > > 
> > > Thanks and Regards,
> > > Sergio
> > > 
> > > >
> > > > Regards,
> > > >
> > > > David
> > > >
> > > > >
> > > > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> > > > > ---
> > > > >  drivers/media/video/isp/isp.c |   49 ++++++--------------------------
> > > --
> > > > -------
> > > > >  1 files changed, 7 insertions(+), 42 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/video/isp/isp.c
> > > > b/drivers/media/video/isp/isp.c
> > > > > index de9352b..30bdc48 100644
> > > > > --- a/drivers/media/video/isp/isp.c
> > > > > +++ b/drivers/media/video/isp/isp.c
> > > > > @@ -254,48 +254,13 @@ EXPORT_SYMBOL(isp_set_xclk);
> > > > >   */
> > > > >  static void isp_power_settings(struct isp_device *isp, int idle)
> > > > >  {
> > > > > -	if (idle) {
> > > > > -		isp_reg_writel(isp,
> > > > > -			       (ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY <<
> > > > > -				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > > > > -			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > > > > -		if (omap_rev() == OMAP3430_REV_ES1_0) {
> > > > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > > > -				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
> > > > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > > > -				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
> > > > > -				       ISPCSI2_SYSCONFIG);
> > > > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > > > -				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
> > > > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > > > -				       OMAP3_ISP_IOMEM_CCP2,
> > > > > -				       ISPCCP2_SYSCONFIG);
> > > > > -		}
> > > > > -		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE,
> > > > OMAP3_ISP_IOMEM_MAIN,
> > > > > -			       ISP_CTRL);
> > > > > -
> > > > > -	} else {
> > > > > -		isp_reg_writel(isp,
> > > > > -			       (ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY <<
> > > > > -				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > > > > -			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > > > > -		if (omap_rev() == OMAP3430_REV_ES1_0) {
> > > > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > > > -				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
> > > > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > > > -				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
> > > > > -				       ISPCSI2_SYSCONFIG);
> > > > > -
> > > > > -			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
> > > > > -				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
> > > > > -					ISPCSI1_MIDLEMODE_SHIFT),
> > > > > -				       OMAP3_ISP_IOMEM_CCP2,
> > > > > -				       ISPCCP2_SYSCONFIG);
> > > > > -		}
> > > > > -
> > > > > -		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE,
> > > > OMAP3_ISP_IOMEM_MAIN,
> > > > > -			       ISP_CTRL);
> > > > > -	}
> > > > > +	isp_reg_writel(isp,
> > > > > +		       ((idle ? ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY :
> > > > > +				ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY) <<
> > > > > +			ISP_SYSCONFIG_MIDLEMODE_SHIFT),
> > > > > +		       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
> > > > > +	isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
> > > > > +		       ISP_CTRL);
> > > > >  }
> > > > >
> > > > >  /*
> > > > > --
> > > > > 1.7.0.4
> > > > >
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-media"
> > > > in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > 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@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff mbox

Patch

diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c
index de9352b..30bdc48 100644
--- a/drivers/media/video/isp/isp.c
+++ b/drivers/media/video/isp/isp.c
@@ -254,48 +254,13 @@  EXPORT_SYMBOL(isp_set_xclk);
  */
 static void isp_power_settings(struct isp_device *isp, int idle)
 {
-	if (idle) {
-		isp_reg_writel(isp,
-			       (ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY <<
-				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
-			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
-		if (omap_rev() == OMAP3430_REV_ES1_0) {
-			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
-					ISPCSI1_MIDLEMODE_SHIFT),
-				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
-				       ISPCSI2_SYSCONFIG);
-			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-				       (ISPCSI1_MIDLEMODE_SMARTSTANDBY <<
-					ISPCSI1_MIDLEMODE_SHIFT),
-				       OMAP3_ISP_IOMEM_CCP2,
-				       ISPCCP2_SYSCONFIG);
-		}
-		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
-			       ISP_CTRL);
-
-	} else {
-		isp_reg_writel(isp,
-			       (ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY <<
-				ISP_SYSCONFIG_MIDLEMODE_SHIFT),
-			       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
-		if (omap_rev() == OMAP3430_REV_ES1_0) {
-			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
-					ISPCSI1_MIDLEMODE_SHIFT),
-				       OMAP3_ISP_IOMEM_CSI2A_REGS1,
-				       ISPCSI2_SYSCONFIG);
-
-			isp_reg_writel(isp, ISPCSI1_AUTOIDLE |
-				       (ISPCSI1_MIDLEMODE_FORCESTANDBY <<
-					ISPCSI1_MIDLEMODE_SHIFT),
-				       OMAP3_ISP_IOMEM_CCP2,
-				       ISPCCP2_SYSCONFIG);
-		}
-
-		isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
-			       ISP_CTRL);
-	}
+	isp_reg_writel(isp,
+		       ((idle ? ISP_SYSCONFIG_MIDLEMODE_SMARTSTANDBY :
+				ISP_SYSCONFIG_MIDLEMODE_FORCESTANDBY) <<
+			ISP_SYSCONFIG_MIDLEMODE_SHIFT),
+		       OMAP3_ISP_IOMEM_MAIN, ISP_SYSCONFIG);
+	isp_reg_writel(isp, ISPCTRL_SBL_AUTOIDLE, OMAP3_ISP_IOMEM_MAIN,
+		       ISP_CTRL);
 }
 
 /*