[2/5] soc-camera: tw9910: Add output signal control

  tw9910 can control output signal.
This patch will stop all signal when video was stopped.

Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
---
 drivers/media/video/tw9910.c |   35 ++++++++++++++++++++++++-----------
 1 files changed, 24 insertions(+), 11 deletions(-)
  

On Tue, 13 Oct 2009, Kuninori Morimoto wrote:

> tw9910 can control output signal.
> This patch will stop all signal when video was stopped.
> 
> Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
> ---
>  drivers/media/video/tw9910.c |   35 ++++++++++++++++++++++++-----------
>  1 files changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/video/tw9910.c b/drivers/media/video/tw9910.c
> index 5152d56..8bda689 100644
> --- a/drivers/media/video/tw9910.c
> +++ b/drivers/media/video/tw9910.c
> @@ -152,7 +152,8 @@
>  			 /* 1 : non-auto */
>  #define VSCTL       0x08 /* 1 : Vertical out ctrl by DVALID */
>  			 /* 0 : Vertical out ctrl by HACTIVE and DVALID */
> -#define OEN         0x04 /* Output Enable together with TRI_SEL. */
> +#define OEN         0x00 /* Enable output */
> +#define EN_TRI_SEL  0x04 /* TRI_SEL output */

Is this to tri-state the output? Ok, from the datasheet it tri-states all 
outputs except clocks. My copy of the datasheet is funny at this point. It 
first describes OEN = bit 2 of OPFORM, and then the TRI_SEL field, which 
is said to occupy bits 1-0, but is documented together with bit 2 with 
values 0-7... And you cannot really say that values 0-3 have a feature 
distinguishing them from values 4-7. So, I wouldn't separate OEN and just 
use the bits 2-0 as a single field. And call the required values like

#define OEN_TRI_SEL_ALL_ON	0
#define OEN_TRI_SEL_CLK_ON	4

>  
>  /* OUTCTR1 */
>  #define VSP_LO      0x00 /* 0 : VS pin output polarity is active low */
> @@ -236,7 +237,6 @@ struct tw9910_priv {
>  
>  static const struct regval_list tw9910_default_regs[] =
>  {
> -	{ OPFORM,  0x00 },
>  	{ OUTCTR1, VSP_LO | VSSL_VVALID | HSP_HI | HSSL_HSYNC },
>  	ENDMARKER,
>  };
> @@ -513,19 +513,32 @@ static int tw9910_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct i2c_client *client = sd->priv;
>  	struct tw9910_priv *priv = to_tw9910(client);
> +	u8 val;
>  
> -	if (!enable)
> +	if (!enable) {
> +		switch (priv->rev) {
> +		case 0:
> +			val = EN_TRI_SEL | 0x2;
> +			break;
> +		case 1:
> +			val = EN_TRI_SEL | 0x3;
> +			break;
> +		}
>  		return 0;
> +	} else {

Ok, it's 8:30 here, so, I might be still not quite awake... but I fail to 
understand, why you bother calculating val above if you anyway just return 
immediately without using it? And if that return is misplaced - what are 
those 2 and 3 constants doing?

>  
> -	if (!priv->scale) {
> -		dev_err(&client->dev, "norm select error\n");
> -		return -EPERM;
> +		if (!priv->scale) {
> +			dev_err(&client->dev, "norm select error\n");
> +			return -EPERM;
> +		}
> +
> +		dev_dbg(&client->dev, "%s %dx%d\n",
> +			priv->scale->name,
> +			priv->scale->width,
> +			priv->scale->height);
>  	}
>  
> -	dev_dbg(&client->dev, "%s %dx%d\n",
> -		 priv->scale->name,
> -		 priv->scale->width,
> -		 priv->scale->height);
> +	tw9910_mask_set(client, OPFORM, 0x7, val);

...and you don't get an "uninitialised variable" warning here? I don't see 
where val gets set in the enable case... Please, wake me up if I'm 
dreaming. Oh, I see, you fix it in the next patch. Please, don't do that! 
Don't introduce bugs to fix them in a later patch. Do it here.

>  
>  	return 0;

Yes, tri-stating outputs for switched off streaming is better than doing 
nothing at all, but isn't there anything else that can be safely powered 
down? What about CLK_PDN, Y_PDN and C_PDN in ACNTL? YSV, CSV and PLL_PDN 
in Analog Control II? I would also expect, that we can at least tristate 
all outputs without any problem, we shouldn't need pixel clock running 
with disabled streaming.

>  }
> -- 
> 1.6.0.4

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
  

Dear Guennadi

Thank you for checking patch

> Is this to tri-state the output? Ok, from the datasheet it tri-states all 
> outputs except clocks. My copy of the datasheet is funny at this point. It 
(snip)
> #define OEN_TRI_SEL_ALL_ON	0
> #define OEN_TRI_SEL_CLK_ON	4

OK

> Ok, it's 8:30 here, so, I might be still not quite awake... but I fail to 
> understand, why you bother calculating val above if you anyway just return 
(snip)
> dreaming. Oh, I see, you fix it in the next patch. Please, don't do that! 
> Don't introduce bugs to fix them in a later patch. Do it here.

oh my god.
It is very buggy patch.
It seems to had broken while patch clean up

> While at it, could you also fix that typo copied from the datasheet: 
> s/HSGEGIN/HSBEGIN/g?

OK

>>  struct tw9910_video_info {
>> -	unsigned long          buswidth;
>> +	u32          flags;
>>  	enum tw9910_mpout_pin  mpout;
>>  	struct soc_camera_link link;
>>  	u16 start_offset;
>
>Hm, I am not convinced I liked all this. Do we understand what these 
>configuration options are doing? Datasheet is not very verbose on that 
>occasion. Is this configuration really platform-specific? What values have 
>you found meaningful in which cases?

I need research about this.
I might have made a big mistake. 

I should re-check my patches and consider all of your comment.
Thank you

Best regards
--
Kuninori Morimoto
 
--
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