OMAP3: ISP: Fix unbalanced use of omap3isp_get().

  Do not use omap3isp_get() when what we really want to do is just
enable clocks, since omap3isp_get() has additional, unwanted, side
effects as an increase of the counter.

This prevented omap3isp of working with Beagleboard xM and it has
been tested only with that platform + mt9p031 sensor.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/video/omap3isp/isp.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
  

Hi Javier,

On Thursday 05 May 2011 15:53:08 Javier Martin wrote:
> Do not use omap3isp_get() when what we really want to do is just
> enable clocks, since omap3isp_get() has additional, unwanted, side
> effects as an increase of the counter.
> 
> This prevented omap3isp of working with Beagleboard xM and it has
> been tested only with that platform + mt9p031 sensor.
> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  drivers/media/video/omap3isp/isp.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/isp.c
> b/drivers/media/video/omap3isp/isp.c index 472a693..ca0831f 100644
> --- a/drivers/media/video/omap3isp/isp.c
> +++ b/drivers/media/video/omap3isp/isp.c
> @@ -85,9 +85,11 @@ module_param(autoidle, int, 0444);
>  MODULE_PARM_DESC(autoidle, "Enable OMAP3ISP AUTOIDLE support");
> 
>  static void isp_save_ctx(struct isp_device *isp);
> -
>  static void isp_restore_ctx(struct isp_device *isp);
> 
> +static int isp_enable_clocks(struct isp_device *isp);
> +static void isp_disable_clocks(struct isp_device *isp);
> +
>  static const struct isp_res_mapping isp_res_maps[] = {
>  	{
>  		.isp_rev = ISP_REVISION_2_0,
> @@ -239,10 +241,10 @@ static u32 isp_set_xclk(struct isp_device *isp, u32
> xclk, u8 xclksel)
> 
>  	/* Do we go from stable whatever to clock? */
>  	if (divisor >= 2 && isp->xclk_divisor[xclksel - 1] < 2)
> -		omap3isp_get(isp);
> +		isp_enable_clocks(isp);

This won't work. Let's assume the following sequence of events:

- Userspace opens the sensor subdev device node
- The sensor driver calls to board code to turn the sensor clock on
- Board code calls to the ISP driver to turn XCLK on
- The ISP driver calls isp_enable_clocks()
...
- Userspace opens an ISP video device node
- The ISP driver calls isp_get(), incrementing the reference count
- Userspace closes the ISP video device node
- The ISP driver calls isp_put(), decrementing the reference count
- The reference count reaches 0, the ISP driver calls isp_disable_clocks()

The sensor will then loose its clock, even though the sensor subdev device 
node is still opened.

Could you please explain why the existing code doesn't work on your hardware ?

>  	/* Stopping the clock. */
>  	else if (divisor < 2 && isp->xclk_divisor[xclksel - 1] >= 2)
> -		omap3isp_put(isp);
> +		isp_disable_clocks(isp);
> 
>  	isp->xclk_divisor[xclksel - 1] = divisor;
  

Hi Laurent,
> This won't work. Let's assume the following sequence of events:
>
> - Userspace opens the sensor subdev device node
> - The sensor driver calls to board code to turn the sensor clock on
> - Board code calls to the ISP driver to turn XCLK on
> - The ISP driver calls isp_enable_clocks()
> ...
> - Userspace opens an ISP video device node
> - The ISP driver calls isp_get(), incrementing the reference count
> - Userspace closes the ISP video device node
> - The ISP driver calls isp_put(), decrementing the reference count
> - The reference count reaches 0, the ISP driver calls isp_disable_clocks()
>
> The sensor will then loose its clock, even though the sensor subdev device
> node is still opened.

Of course, you are right, I hadn't thought of it this way.

> Could you please explain why the existing code doesn't work on your hardware ?

Apparently, it is a mistake related to the sensor driver. Sorry about that.

Just one question.
As I can see from mt9v032 driver, open callback is used to enable
clock and close callback is used to disable clock.
Does this mean that if sensor device node is not held open video won't work?
i.e, the following wouldn't work since (2) opens the sensor (enables
clock) and closes it again (disables clock) and (3) only opens CCDC
device node (enables clock with a wrong setting, since it was
previously set to 0 by (2) ) :

(1)  ./media-ctl -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1],
"OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]'
(2)  ./media-ctl -f '"mt9p031 2-0048":0[SGRBG8 320x240], "OMAP3 ISP
CCDC":1[SGRBG8 320x240]'
(3)  ./yavta  -f SGRBG8 -s 320x240 -n 4 --capture=100 --skip 3 -F
`./media-ctl -e "OMAP3 ISP CCDC output"
  

Hi Javier,

On Friday 06 May 2011 12:42:19 javier Martin wrote:
> Hi Laurent,
> 
> > This won't work. Let's assume the following sequence of events:
> > 
> > - Userspace opens the sensor subdev device node
> > - The sensor driver calls to board code to turn the sensor clock on
> > - Board code calls to the ISP driver to turn XCLK on
> > - The ISP driver calls isp_enable_clocks()
> > ...
> > - Userspace opens an ISP video device node
> > - The ISP driver calls isp_get(), incrementing the reference count
> > - Userspace closes the ISP video device node
> > - The ISP driver calls isp_put(), decrementing the reference count
> > - The reference count reaches 0, the ISP driver calls
> > isp_disable_clocks()
> > 
> > The sensor will then loose its clock, even though the sensor subdev
> > device node is still opened.
> 
> Of course, you are right, I hadn't thought of it this way.
> 
> > Could you please explain why the existing code doesn't work on your
> > hardware ?
> 
> Apparently, it is a mistake related to the sensor driver. Sorry about that.

OK. No worries.

> Just one question.
> As I can see from mt9v032 driver, open callback is used to enable clock and
> close callback is used to disable clock.
> Does this mean that if sensor device node is not held open video won't
> work? i.e, the following wouldn't work since (2) opens the sensor (enables
> clock) and closes it again (disables clock) and (3) only opens CCDC device
> node (enables clock with a wrong setting, since it was
> previously set to 0 by (2) ) :
> 
> (1)  ./media-ctl -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1],
> "OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]'
> (2)  ./media-ctl -f '"mt9p031 2-0048":0[SGRBG8 320x240], "OMAP3 ISP
> CCDC":1[SGRBG8 320x240]'
> (3)  ./yavta  -f SGRBG8 -s 320x240 -n 4 --capture=100 --skip 3 -F
> `./media-ctl -e "OMAP3 ISP CCDC output"

Clocks are enabled/disabled by the mt9v032_set_power() function, called both 
by the open() and close() handlers and by the ISP driver through the subdev 
core::s_power operation. If the sensor is part of the pipeline, the OMAP3 ISP 
driver will power it up when the output video node is opened. (3) will thus 
power the sensor up.