[PATCH 0/2] omap3isp/rx-51: Add vdds_csib regulator handling

[PATCH 0/2] omap3isp/rx-51: Add vdds_csib regulator handling

[Kalle Jokiniemi]

The CSIb block is used in rx-51 to handle camera ccp2 IO. Adding
support to omap3isp driver for managing the power supply for the
CSIb IO complex via regulator framework. Also create the
apropriate regulator definitions in the rx-51 board file.

I propose to push this set through the linux-media, since most
of the changes are on the omap3isp driver side.

Any comments/review appreciated. Tested on Nokia N900 and the
MeeGo testing daily images (.37 based kernel). Patches on top
of Mauro's linux-next tree, build tested and boot tested with
that. 

Kalle Jokiniemi (2):
  OMAP3: ISP: Add regulator control for omap34xx
  OMAP3: RX-51: define vdds_csib regulator supply

 arch/arm/mach-omap2/board-rx51-peripherals.c |    9 +++++++++
 drivers/media/video/omap3isp/ispccp2.c       |   24 +++++++++++++++++++++++-
 drivers/media/video/omap3isp/ispccp2.h       |    1 +
 3 files changed, 33 insertions(+), 1 deletions(-)

[PATCH 1/2] OMAP3: ISP: Add regulator control for omap34xx

[Kalle Jokiniemi]

The current omap3isp driver is missing regulator handling
for CSIb complex in omap34xx based devices. This patch
adds a mechanism for this to the omap3isp driver.

Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
---
 drivers/media/video/omap3isp/ispccp2.c |   24 +++++++++++++++++++++++-
 drivers/media/video/omap3isp/ispccp2.h |    1 +
 2 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispccp2.c b/drivers/media/video/omap3isp/ispccp2.c
index 0e16cab..3b17b0d 100644
--- a/drivers/media/video/omap3isp/ispccp2.c
+++ b/drivers/media/video/omap3isp/ispccp2.c
@@ -30,6 +30,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/uaccess.h>
+#include <linux/regulator/consumer.h>
 
 #include "isp.h"
 #include "ispreg.h"
@@ -163,6 +164,9 @@ static void ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable)
 	struct isp_pipeline *pipe = to_isp_pipeline(&ccp2->subdev.entity);
 	int i;
 
+	if (enable && ccp2->vdds_csib)
+		regulator_enable(ccp2->vdds_csib);
+
 	/* Enable/Disable all the LCx channels */
 	for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
 		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LCx_CTRL(i),
@@ -186,6 +190,8 @@ static void ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable)
 				    ISPCCP2_LC01_IRQENABLE,
 				    ISPCCP2_LC01_IRQSTATUS_LC0_FS_IRQ);
 	}
+	if (!enable && ccp2->vdds_csib)
+		regulator_disable(ccp2->vdds_csib);
 }
 
 /*
@@ -1137,6 +1143,10 @@ error:
  */
 void omap3isp_ccp2_cleanup(struct isp_device *isp)
 {
+	struct isp_ccp2_device *ccp2 = &isp->isp_ccp2;
+
+	if (isp->revision == ISP_REVISION_2_0)
+		regulator_put(ccp2->vdds_csib);
 }
 
 /*
@@ -1155,10 +1165,22 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 	 * the CSI2c or CSI2a receivers. The PHY then needs to be explicitly
 	 * configured.
 	 *
+	 * On the OMAP34xx the CSI1/CCB is operated in the CSIb IO complex,
+	 * which is powered by vdds_csib power rail. Hence the request for
+	 * the regulator.
+	 *
 	 * TODO: Don't hardcode the usage of PHY1 (shared with CSI2c).
 	 */
-	if (isp->revision == ISP_REVISION_15_0)
+	if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
+	} else if (isp->revision == ISP_REVISION_2_0) {
+		ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib");
+		if (IS_ERR(ccp2->vdds_csib)) {
+			dev_dbg(isp->dev,
+				"Could not get regulator vdds_csib\n");
+			ccp2->vdds_csib = NULL;
+		}
+	}
 
 	ret = ccp2_init_entities(ccp2);
 	if (ret < 0)
diff --git a/drivers/media/video/omap3isp/ispccp2.h b/drivers/media/video/omap3isp/ispccp2.h
index 5505a86..6674e9d 100644
--- a/drivers/media/video/omap3isp/ispccp2.h
+++ b/drivers/media/video/omap3isp/ispccp2.h
@@ -81,6 +81,7 @@ struct isp_ccp2_device {
 	struct isp_interface_mem_config mem_cfg;
 	struct isp_video video_in;
 	struct isp_csiphy *phy;
+	struct regulator *vdds_csib;
 	unsigned int error;
 	enum isp_pipeline_stream_state state;
 	wait_queue_head_t wait;
-- 
1.7.1

[PATCH 2/2] OMAP3: RX-51: define vdds_csib regulator supply

[Kalle Jokiniemi]

The RX-51 uses the CSIb IO complex for camera operation. The
board file is missing definition for the regulator supplying
the CSIb complex, so this is added for better power
management.

Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
---
 arch/arm/mach-omap2/board-rx51-peripherals.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
index bbcb677..1324ba3 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -337,6 +337,13 @@ static struct omap2_hsmmc_info mmc[] __initdata = {
 static struct regulator_consumer_supply rx51_vmmc1_supply =
 	REGULATOR_SUPPLY("vmmc", "omap_hsmmc.0");
 
+static struct regulator_consumer_supply rx51_vaux2_supplies[] = {
+	REGULATOR_SUPPLY("vdds_csib", "omap3isp"),
+	{
+		.supply = "vaux2",
+	},
+};
+
 static struct regulator_consumer_supply rx51_vaux3_supply =
 	REGULATOR_SUPPLY("vmmc", "omap_hsmmc.1");
 
@@ -400,6 +407,8 @@ static struct regulator_init_data rx51_vaux2 = {
 		.valid_ops_mask		= REGULATOR_CHANGE_MODE
 					| REGULATOR_CHANGE_STATUS,
 	},
+	.num_consumer_supplies	= ARRAY_SIZE(rx51_vaux2_supplies),
+	.consumer_supplies	= rx51_vaux2_supplies,
 };
 
 /* VAUX3 - adds more power to VIO_18 rail */
-- 
1.7.1

Re: [PATCH 2/2] OMAP3: RX-51: define vdds_csib regulator supply

[Tony Lindgren]

* Kalle Jokiniemi <kalle.jokiniemi@nokia.com> [110429 00:09]:
> The RX-51 uses the CSIb IO complex for camera operation. The
> board file is missing definition for the regulator supplying
> the CSIb complex, so this is added for better power
> management.
> 
> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>

This looks safe to merge along with the other patch:

Acked-by: Tony Lindgren <tony@atomide.com>

> ---
>  arch/arm/mach-omap2/board-rx51-peripherals.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
> index bbcb677..1324ba3 100644
> --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
> +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
> @@ -337,6 +337,13 @@ static struct omap2_hsmmc_info mmc[] __initdata = {
>  static struct regulator_consumer_supply rx51_vmmc1_supply =
>  	REGULATOR_SUPPLY("vmmc", "omap_hsmmc.0");
>  
> +static struct regulator_consumer_supply rx51_vaux2_supplies[] = {
> +	REGULATOR_SUPPLY("vdds_csib", "omap3isp"),
> +	{
> +		.supply = "vaux2",
> +	},
> +};
> +
>  static struct regulator_consumer_supply rx51_vaux3_supply =
>  	REGULATOR_SUPPLY("vmmc", "omap_hsmmc.1");
>  
> @@ -400,6 +407,8 @@ static struct regulator_init_data rx51_vaux2 = {
>  		.valid_ops_mask		= REGULATOR_CHANGE_MODE
>  					| REGULATOR_CHANGE_STATUS,
>  	},
> +	.num_consumer_supplies	= ARRAY_SIZE(rx51_vaux2_supplies),
> +	.consumer_supplies	= rx51_vaux2_supplies,
>  };
>  
>  /* VAUX3 - adds more power to VIO_18 rail */
> -- 
> 1.7.1
> 

Re: [PATCH 1/2] OMAP3: ISP: Add regulator control for omap34xx

[Laurent Pinchart]

Hi Kalle,

On Friday 29 April 2011 09:11:59 Kalle Jokiniemi wrote:
> The current omap3isp driver is missing regulator handling
> for CSIb complex in omap34xx based devices. This patch
> adds a mechanism for this to the omap3isp driver.
> 
> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>

Thanks for the patch.

The CSIb pins are multiplexed with the parallel interface cam_d[6:9] signals, 
so the driver might need to handle the vdds_csib regulator for the parallel 
interface as well. We can leave that out now though, as I'm not sure we'll 
ever see a platform that will require that.

> ---
>  drivers/media/video/omap3isp/ispccp2.c |   24 +++++++++++++++++++++++-
>  drivers/media/video/omap3isp/ispccp2.h |    1 +
>  2 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/ispccp2.c
> b/drivers/media/video/omap3isp/ispccp2.c index 0e16cab..3b17b0d 100644
> --- a/drivers/media/video/omap3isp/ispccp2.c
> +++ b/drivers/media/video/omap3isp/ispccp2.c
> @@ -30,6 +30,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/uaccess.h>
> +#include <linux/regulator/consumer.h>
> 
>  #include "isp.h"
>  #include "ispreg.h"
> @@ -163,6 +164,9 @@ static void ccp2_if_enable(struct isp_ccp2_device
> *ccp2, u8 enable) struct isp_pipeline *pipe =
> to_isp_pipeline(&ccp2->subdev.entity); int i;
> 
> +	if (enable && ccp2->vdds_csib)
> +		regulator_enable(ccp2->vdds_csib);
> +
>  	/* Enable/Disable all the LCx channels */
>  	for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
>  		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LCx_CTRL(i),
> @@ -186,6 +190,8 @@ static void ccp2_if_enable(struct isp_ccp2_device
> *ccp2, u8 enable) ISPCCP2_LC01_IRQENABLE,
>  				    ISPCCP2_LC01_IRQSTATUS_LC0_FS_IRQ);
>  	}

If you resubmit the patch to address the comments below, please add a blank 
line here.

> +	if (!enable && ccp2->vdds_csib)
> +		regulator_disable(ccp2->vdds_csib);
>  }
> 
>  /*
> @@ -1137,6 +1143,10 @@ error:
>   */
>  void omap3isp_ccp2_cleanup(struct isp_device *isp)
>  {
> +	struct isp_ccp2_device *ccp2 = &isp->isp_ccp2;
> +
> +	if (isp->revision == ISP_REVISION_2_0)
> +		regulator_put(ccp2->vdds_csib);

What about testing ccp2->vdds_csib != NULL here like you do above ? Not all 
ES2.0 platforms will use a regulator, so you can end up calling 
regulator_put(NULL). regulator_put() will return immediately, but the API 
doesn't allow it explictly either.

If regulator_put(NULL) is deemed to be safe, I would remove the revision check 
here. If it isn't, I would replace it with a ccp2->vdds_csib != NULL check.

>  }
> 
>  /*
> @@ -1155,10 +1165,22 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>  	 * the CSI2c or CSI2a receivers. The PHY then needs to be explicitly
>  	 * configured.
>  	 *
> +	 * On the OMAP34xx the CSI1/CCB is operated in the CSIb IO complex,

CSI1/CCB ? Do you mean CCP ?

The OMAP34xx has no CCP2 support anyway, so I would s,CSI1/CCB,CSI1 receiver,.

> +	 * which is powered by vdds_csib power rail. Hence the request for
> +	 * the regulator.
> +	 *
>  	 * TODO: Don't hardcode the usage of PHY1 (shared with CSI2c).
>  	 */
> -	if (isp->revision == ISP_REVISION_15_0)
> +	if (isp->revision == ISP_REVISION_15_0) {
>  		ccp2->phy = &isp->isp_csiphy1;
> +	} else if (isp->revision == ISP_REVISION_2_0) {
> +		ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib");
> +		if (IS_ERR(ccp2->vdds_csib)) {
> +			dev_dbg(isp->dev,
> +				"Could not get regulator vdds_csib\n");
> +			ccp2->vdds_csib = NULL;
> +		}
> +	}

If you resubmit your patch to address the above comments, could you please 
reorder the code (and the comment) here and put the ES2.0 check before the 
15.0 ?

>  	ret = ccp2_init_entities(ccp2);
>  	if (ret < 0)
> diff --git a/drivers/media/video/omap3isp/ispccp2.h
> b/drivers/media/video/omap3isp/ispccp2.h index 5505a86..6674e9d 100644
> --- a/drivers/media/video/omap3isp/ispccp2.h
> +++ b/drivers/media/video/omap3isp/ispccp2.h
> @@ -81,6 +81,7 @@ struct isp_ccp2_device {
>  	struct isp_interface_mem_config mem_cfg;
>  	struct isp_video video_in;
>  	struct isp_csiphy *phy;
> +	struct regulator *vdds_csib;
>  	unsigned int error;
>  	enum isp_pipeline_stream_state state;
>  	wait_queue_head_t wait;

-- 
Regards,

Laurent Pinchart

RE: [PATCH 1/2] OMAP3: ISP: Add regulator control for omap34xx

[kalle.jokiniemi]

Hi,

 > -----Original Message-----
 > From: ext Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
 > Sent: 29. huhtikuuta 2011 12:49
 > To: Jokiniemi Kalle (Nokia-SD/Tampere)
 > Cc: tony@atomide.com; mchebab@infradead.org; linux-
 > omap@vger.kernel.org; linux-media@vger.kernel.org
 > Subject: Re: [PATCH 1/2] OMAP3: ISP: Add regulator control for omap34xx
 > 
 > Hi Kalle,
 > 
 > On Friday 29 April 2011 09:11:59 Kalle Jokiniemi wrote:
 > > The current omap3isp driver is missing regulator handling
 > > for CSIb complex in omap34xx based devices. This patch
 > > adds a mechanism for this to the omap3isp driver.
 > >
 > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
 > 
 > Thanks for the patch.

No problem, thanks for the comments. Good comments, I'll spin another
patch after weekend to address the issues.

- Kalle

 > 
 > The CSIb pins are multiplexed with the parallel interface cam_d[6:9] signals,
 > so the driver might need to handle the vdds_csib regulator for the parallel
 > interface as well. We can leave that out now though, as I'm not sure we'll
 > ever see a platform that will require that.
 > 
 > > ---
 > >  drivers/media/video/omap3isp/ispccp2.c |   24
 > +++++++++++++++++++++++-
 > >  drivers/media/video/omap3isp/ispccp2.h |    1 +
 > >  2 files changed, 24 insertions(+), 1 deletions(-)
 > >
 > > diff --git a/drivers/media/video/omap3isp/ispccp2.c
 > > b/drivers/media/video/omap3isp/ispccp2.c index 0e16cab..3b17b0d 100644
 > > --- a/drivers/media/video/omap3isp/ispccp2.c
 > > +++ b/drivers/media/video/omap3isp/ispccp2.c
 > > @@ -30,6 +30,7 @@
 > >  #include <linux/module.h>
 > >  #include <linux/mutex.h>
 > >  #include <linux/uaccess.h>
 > > +#include <linux/regulator/consumer.h>
 > >
 > >  #include "isp.h"
 > >  #include "ispreg.h"
 > > @@ -163,6 +164,9 @@ static void ccp2_if_enable(struct isp_ccp2_device
 > > *ccp2, u8 enable) struct isp_pipeline *pipe =
 > > to_isp_pipeline(&ccp2->subdev.entity); int i;
 > >
 > > +	if (enable && ccp2->vdds_csib)
 > > +		regulator_enable(ccp2->vdds_csib);
 > > +
 > >  	/* Enable/Disable all the LCx channels */
 > >  	for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
 > >  		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2,
 > ISPCCP2_LCx_CTRL(i),
 > > @@ -186,6 +190,8 @@ static void ccp2_if_enable(struct isp_ccp2_device
 > > *ccp2, u8 enable) ISPCCP2_LC01_IRQENABLE,
 > >  				    ISPCCP2_LC01_IRQSTATUS_LC0_FS_IRQ);
 > >  	}
 > 
 > If you resubmit the patch to address the comments below, please add a blank
 > line here.
 > 
 > > +	if (!enable && ccp2->vdds_csib)
 > > +		regulator_disable(ccp2->vdds_csib);
 > >  }
 > >
 > >  /*
 > > @@ -1137,6 +1143,10 @@ error:
 > >   */
 > >  void omap3isp_ccp2_cleanup(struct isp_device *isp)
 > >  {
 > > +	struct isp_ccp2_device *ccp2 = &isp->isp_ccp2;
 > > +
 > > +	if (isp->revision == ISP_REVISION_2_0)
 > > +		regulator_put(ccp2->vdds_csib);
 > 
 > What about testing ccp2->vdds_csib != NULL here like you do above ? Not all
 > ES2.0 platforms will use a regulator, so you can end up calling
 > regulator_put(NULL). regulator_put() will return immediately, but the API
 > doesn't allow it explictly either.
 > 
 > If regulator_put(NULL) is deemed to be safe, I would remove the revision
 > check
 > here. If it isn't, I would replace it with a ccp2->vdds_csib != NULL check.
 > 
 > >  }
 > >
 > >  /*
 > > @@ -1155,10 +1165,22 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 > >  	 * the CSI2c or CSI2a receivers. The PHY then needs to be explicitly
 > >  	 * configured.
 > >  	 *
 > > +	 * On the OMAP34xx the CSI1/CCB is operated in the CSIb IO complex,
 > 
 > CSI1/CCB ? Do you mean CCP ?
 > 
 > The OMAP34xx has no CCP2 support anyway, so I would s,CSI1/CCB,CSI1
 > receiver,.
 > 
 > > +	 * which is powered by vdds_csib power rail. Hence the request for
 > > +	 * the regulator.
 > > +	 *
 > >  	 * TODO: Don't hardcode the usage of PHY1 (shared with CSI2c).
 > >  	 */
 > > -	if (isp->revision == ISP_REVISION_15_0)
 > > +	if (isp->revision == ISP_REVISION_15_0) {
 > >  		ccp2->phy = &isp->isp_csiphy1;
 > > +	} else if (isp->revision == ISP_REVISION_2_0) {
 > > +		ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib");
 > > +		if (IS_ERR(ccp2->vdds_csib)) {
 > > +			dev_dbg(isp->dev,
 > > +				"Could not get regulator vdds_csib\n");
 > > +			ccp2->vdds_csib = NULL;
 > > +		}
 > > +	}
 > 
 > If you resubmit your patch to address the above comments, could you please
 > reorder the code (and the comment) here and put the ES2.0 check before the
 > 15.0 ?
 > 
 > >  	ret = ccp2_init_entities(ccp2);
 > >  	if (ret < 0)
 > > diff --git a/drivers/media/video/omap3isp/ispccp2.h
 > > b/drivers/media/video/omap3isp/ispccp2.h index 5505a86..6674e9d 100644
 > > --- a/drivers/media/video/omap3isp/ispccp2.h
 > > +++ b/drivers/media/video/omap3isp/ispccp2.h
 > > @@ -81,6 +81,7 @@ struct isp_ccp2_device {
 > >  	struct isp_interface_mem_config mem_cfg;
 > >  	struct isp_video video_in;
 > >  	struct isp_csiphy *phy;
 > > +	struct regulator *vdds_csib;
 > >  	unsigned int error;
 > >  	enum isp_pipeline_stream_state state;
 > >  	wait_queue_head_t wait;
 > 
 > --
 > Regards,
 > 
 > Laurent Pinchart

RE: [PATCH 2/2] OMAP3: RX-51: define vdds_csib regulator supply

[kalle.jokiniemi]

Hi,

 > -----Original Message-----
 > From: ext Tony Lindgren [mailto:tony@atomide.com]
 > Sent: 29. huhtikuuta 2011 12:14
 > To: Jokiniemi Kalle (Nokia-SD/Tampere)
 > Cc: laurent.pinchart@ideasonboard.com; mchebab@infradead.org; linux-
 > omap@vger.kernel.org; linux-media@vger.kernel.org
 > Subject: Re: [PATCH 2/2] OMAP3: RX-51: define vdds_csib regulator supply
 > 
 > * Kalle Jokiniemi <kalle.jokiniemi@nokia.com> [110429 00:09]:
 > > The RX-51 uses the CSIb IO complex for camera operation. The
 > > board file is missing definition for the regulator supplying
 > > the CSIb complex, so this is added for better power
 > > management.
 > >
 > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
 > 
 > This looks safe to merge along with the other patch
 > 
 > Acked-by: Tony Lindgren <tony@atomide.com>

Thanks Tony, adding Mauro's correct email...

- Kalle




 > 
 > > ---
 > >  arch/arm/mach-omap2/board-rx51-peripherals.c |    9 +++++++++
 > >  1 files changed, 9 insertions(+), 0 deletions(-)
 > >
 > > diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c
 > b/arch/arm/mach-omap2/board-rx51-peripherals.c
 > > index bbcb677..1324ba3 100644
 > > --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
 > > +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
 > > @@ -337,6 +337,13 @@ static struct omap2_hsmmc_info mmc[] __initdata =
 > {
 > >  static struct regulator_consumer_supply rx51_vmmc1_supply =
 > >  	REGULATOR_SUPPLY("vmmc", "omap_hsmmc.0");
 > >
 > > +static struct regulator_consumer_supply rx51_vaux2_supplies[] = {
 > > +	REGULATOR_SUPPLY("vdds_csib", "omap3isp"),
 > > +	{
 > > +		.supply = "vaux2",
 > > +	},
 > > +};
 > > +
 > >  static struct regulator_consumer_supply rx51_vaux3_supply =
 > >  	REGULATOR_SUPPLY("vmmc", "omap_hsmmc.1");
 > >
 > > @@ -400,6 +407,8 @@ static struct regulator_init_data rx51_vaux2 = {
 > >  		.valid_ops_mask		= REGULATOR_CHANGE_MODE
 > >  					| REGULATOR_CHANGE_STATUS,
 > >  	},
 > > +	.num_consumer_supplies	= ARRAY_SIZE(rx51_vaux2_supplies),
 > > +	.consumer_supplies	= rx51_vaux2_supplies,
 > >  };
 > >
 > >  /* VAUX3 - adds more power to VIO_18 rail */
 > > --
 > > 1.7.1
 > >

RE: [PATCH 1/2] OMAP3: ISP: Add regulator control for omap34xx

[kalle.jokiniemi]

Hi,

 > -----Original Message-----
 > From: ext Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
 > Sent: 29. huhtikuuta 2011 12:49
 > To: Jokiniemi Kalle (Nokia-SD/Tampere)
 > Cc: tony@atomide.com; mchebab@infradead.org; linux-
 > omap@vger.kernel.org; linux-media@vger.kernel.org
 > Subject: Re: [PATCH 1/2] OMAP3: ISP: Add regulator control for omap34xx
 > 
 > Hi Kalle,
 > 
 > On Friday 29 April 2011 09:11:59 Kalle Jokiniemi wrote:
 > > The current omap3isp driver is missing regulator handling
 > > for CSIb complex in omap34xx based devices. This patch
 > > adds a mechanism for this to the omap3isp driver.
 > >
 > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
 > 
 > Thanks for the patch.

Sent an updated one, comments on what was done below.

 > 
 > The CSIb pins are multiplexed with the parallel interface cam_d[6:9] signals,
 > so the driver might need to handle the vdds_csib regulator for the parallel
 > interface as well. We can leave that out now though, as I'm not sure we'll
 > ever see a platform that will require that.
 > 
 > > ---
 > >  drivers/media/video/omap3isp/ispccp2.c |   24
 > +++++++++++++++++++++++-
 > >  drivers/media/video/omap3isp/ispccp2.h |    1 +
 > >  2 files changed, 24 insertions(+), 1 deletions(-)
 > >
 > > diff --git a/drivers/media/video/omap3isp/ispccp2.c
 > > b/drivers/media/video/omap3isp/ispccp2.c index 0e16cab..3b17b0d 100644
 > > --- a/drivers/media/video/omap3isp/ispccp2.c
 > > +++ b/drivers/media/video/omap3isp/ispccp2.c
 > > @@ -30,6 +30,7 @@
 > >  #include <linux/module.h>
 > >  #include <linux/mutex.h>
 > >  #include <linux/uaccess.h>
 > > +#include <linux/regulator/consumer.h>
 > >
 > >  #include "isp.h"
 > >  #include "ispreg.h"
 > > @@ -163,6 +164,9 @@ static void ccp2_if_enable(struct isp_ccp2_device
 > > *ccp2, u8 enable) struct isp_pipeline *pipe =
 > > to_isp_pipeline(&ccp2->subdev.entity); int i;
 > >
 > > +	if (enable && ccp2->vdds_csib)
 > > +		regulator_enable(ccp2->vdds_csib);
 > > +
 > >  	/* Enable/Disable all the LCx channels */
 > >  	for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
 > >  		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2,
 > ISPCCP2_LCx_CTRL(i),
 > > @@ -186,6 +190,8 @@ static void ccp2_if_enable(struct isp_ccp2_device
 > > *ccp2, u8 enable) ISPCCP2_LC01_IRQENABLE,
 > >  				    ISPCCP2_LC01_IRQSTATUS_LC0_FS_IRQ);
 > >  	}
 > 
 > If you resubmit the patch to address the comments below, please add a blank
 > line here.

Done.


 > 
 > > +	if (!enable && ccp2->vdds_csib)
 > > +		regulator_disable(ccp2->vdds_csib);
 > >  }
 > >
 > >  /*
 > > @@ -1137,6 +1143,10 @@ error:
 > >   */
 > >  void omap3isp_ccp2_cleanup(struct isp_device *isp)
 > >  {
 > > +	struct isp_ccp2_device *ccp2 = &isp->isp_ccp2;
 > > +
 > > +	if (isp->revision == ISP_REVISION_2_0)
 > > +		regulator_put(ccp2->vdds_csib);
 > 
 > What about testing ccp2->vdds_csib != NULL here like you do above ? Not all
 > ES2.0 platforms will use a regulator, so you can end up calling
 > regulator_put(NULL). regulator_put() will return immediately, but the API
 > doesn't allow it explictly either.
 > 
 > If regulator_put(NULL) is deemed to be safe, I would remove the revision
 > check
 > here. If it isn't, I would replace it with a ccp2->vdds_csib != NULL check.

Regulator_put checks for NULL, so it's safe to call always.


 > 
 > >  }
 > >
 > >  /*
 > > @@ -1155,10 +1165,22 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 > >  	 * the CSI2c or CSI2a receivers. The PHY then needs to be explicitly
 > >  	 * configured.
 > >  	 *
 > > +	 * On the OMAP34xx the CSI1/CCB is operated in the CSIb IO complex,
 > 
 > CSI1/CCB ? Do you mean CCP ?
 > 
 > The OMAP34xx has no CCP2 support anyway, so I would s,CSI1/CCB,CSI1
 > receiver,.

Done.

 > 
 > > +	 * which is powered by vdds_csib power rail. Hence the request for
 > > +	 * the regulator.
 > > +	 *
 > >  	 * TODO: Don't hardcode the usage of PHY1 (shared with CSI2c).
 > >  	 */
 > > -	if (isp->revision == ISP_REVISION_15_0)
 > > +	if (isp->revision == ISP_REVISION_15_0) {
 > >  		ccp2->phy = &isp->isp_csiphy1;
 > > +	} else if (isp->revision == ISP_REVISION_2_0) {
 > > +		ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib");
 > > +		if (IS_ERR(ccp2->vdds_csib)) {
 > > +			dev_dbg(isp->dev,
 > > +				"Could not get regulator vdds_csib\n");
 > > +			ccp2->vdds_csib = NULL;
 > > +		}
 > > +	}
 > 
 > If you resubmit your patch to address the above comments, could you please
 > reorder the code (and the comment) here and put the ES2.0 check before the
 > 15.0 ?

Done for both the code and comments.

- Kalle

 > 
 > >  	ret = ccp2_init_entities(ccp2);
 > >  	if (ret < 0)
 > > diff --git a/drivers/media/video/omap3isp/ispccp2.h
 > > b/drivers/media/video/omap3isp/ispccp2.h index 5505a86..6674e9d 100644
 > > --- a/drivers/media/video/omap3isp/ispccp2.h
 > > +++ b/drivers/media/video/omap3isp/ispccp2.h
 > > @@ -81,6 +81,7 @@ struct isp_ccp2_device {
 > >  	struct isp_interface_mem_config mem_cfg;
 > >  	struct isp_video video_in;
 > >  	struct isp_csiphy *phy;
 > > +	struct regulator *vdds_csib;
 > >  	unsigned int error;
 > >  	enum isp_pipeline_stream_state state;
 > >  	wait_queue_head_t wait;
 > 
 > --
 > Regards,
 > 
 > Laurent Pinchart