Re: [PATCH v2] soc-camera: tw9910: modify V/H outpit pin setting to use VALID
Dear Guennadi > Hm, strange... This doesn't work at all for me. Getting only timeouts. > Have you tested this on Migo-R? Hmm.. strange... It works well on my environment. Of course Migo-R too. my environment is based on your 20091105 patches and my patches Kuninori Morimoto (13): soc-camera: tw9910: Add revision control soc-camera: tw9910: fix a persistent ID calculation soc-camera: tw9910: Add tri-state control soc-camera: tw9910: Add power control soc-camera: tw9910: tw9910_set_hsync clean up soc-camera: tw9910: Add revision control on tw9910_set_hsync ->soc-camera: tw9910: modify V/H outpit pin setting to use VALID soc-camera: tw9910: modify output format soc-camera: tw9910: remove cropping soc-camera: sh_mobile_ceu: Add V4L2_FIELD_INTERLACED_BT/TB support soc-camera: tw9910: use V4L2_FIELD_INTERLACED_BT soc-camera: sh_mobile_ceu_camera: Add support sync polarity selection soc-camera: tw9910: Add sync polarity support if you use arrow point, you can get strange color output, but get image I think. (to get correct color, next "modify output format" is needed) Best regards -- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] soc-camera: tw9910: modify V/H outpit pin setting to use VALID
On Fri, 13 Nov 2009, Kuninori Morimoto wrote:
>
> Signed-off-by: Kuninori Morimoto
> ---
> v1 -> v2
>
> o remove un-understandable explain.
> -> tw9910_query_bus_param need not modify now
> o move OUTCTR1 setting to tw9910_set_bus_param
>
> drivers/media/video/tw9910.c | 38 --
> 1 files changed, 8 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/video/tw9910.c b/drivers/media/video/tw9910.c
> index 6d8dede..82135f2 100644
> --- a/drivers/media/video/tw9910.c
> +++ b/drivers/media/video/tw9910.c
> @@ -239,18 +239,6 @@ struct tw9910_priv {
> u32 revision;
> };
>
> -/*
> - * register settings
> - */
> -
> -#define ENDMARKER { 0xff, 0xff }
> -
> -static const struct regval_list tw9910_default_regs[] =
> -{
> - { OUTCTR1, VSP_LO | VSSL_VVALID | HSP_HI | HSSL_HSYNC },
> - ENDMARKER,
> -};
> -
> static const enum v4l2_imgbus_pixelcode tw9910_color_codes[] = {
> V4L2_IMGBUS_FMT_VYUY,
> };
> @@ -463,20 +451,6 @@ static int tw9910_set_hsync(struct i2c_client *client,
> return ret;
> }
>
> -static int tw9910_write_array(struct i2c_client *client,
> - const struct regval_list *vals)
> -{
> - while (vals->reg_num != 0xff) {
> - int ret = i2c_smbus_write_byte_data(client,
> - vals->reg_num,
> - vals->value);
> - if (ret < 0)
> - return ret;
> - vals++;
> - }
> - return 0;
> -}
> -
> static void tw9910_reset(struct i2c_client *client)
> {
> tw9910_mask_set(client, ACNTL1, SRESET, SRESET);
> @@ -578,7 +552,14 @@ static int tw9910_s_stream(struct v4l2_subdev *sd, int
> enable)
> static int tw9910_set_bus_param(struct soc_camera_device *icd,
> unsigned long flags)
> {
> - return 0;
> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> + struct i2c_client *client = sd->priv;
> +
> + /*
> + * set OUTCTR1
> + */
> + return i2c_smbus_write_byte_data(client, OUTCTR1,
> + VSSL_VVALID | HSSL_DVALID);
Hm, strange... This doesn't work at all for me. Getting only timeouts.
Have you tested this on Migo-R?
Thanks
Guennadi
> }
>
> static unsigned long tw9910_query_bus_param(struct soc_camera_device *icd)
> @@ -681,9 +662,6 @@ static int tw9910_s_crop(struct v4l2_subdev *sd, struct
> v4l2_crop *a)
>* reset hardware
>*/
> tw9910_reset(client);
> - ret = tw9910_write_array(client, tw9910_default_regs);
> - if (ret < 0)
> - goto tw9910_set_fmt_error;
>
> /*
>* set bus width
> --
> 1.6.3.3
>
---
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] soc-camera: tw9910: modify V/H outpit pin setting to use VALID
Dear Guennadi Thank you for your comment ! > > I think your order will be added in next patch. > > "Add polarities support" > > I will send it > > OK ? > > Ok, we can do that too. Thank you > This is how the pxa_camera.c driver does it in pxa_camera_set_bus_param(). > What actually happens in sh_mobile_ceu_camera.c, is that it keeps both > polarities in its call to the client, so, the client picks up randomly one > of them, then the host does the same. So, with a probability of 1/2 they > will choose opposite polarities:-) > > > I guess, to add your order, we needs more than 2 patches. > > > > "modify soc_camera_bus_param_compatible behavior" > > No. We should fix the sh-CEU driver. Thank you !! I can understand !! I will study pxa_camera_set_bus_param. Best regards -- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] soc-camera: tw9910: modify V/H outpit pin setting to use VALID
On Mon, 16 Nov 2009, Kuninori Morimoto wrote: > > Dear Guennadi > > Thank you > > > Oh, indeed. Ok, but can you add proper support for both high and low > > polarities? > > I think your order will be added in next patch. > "Add polarities support" > I will send it > OK ? Ok, we can do that too. > By the way. > Maybe I should ask you about it. > > My question is that who select ACTIVE HI / LOW ? > > - > unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER | > SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH | > + SOCAM_VSYNC_ACTIVE_LOW | SOCAM_HSYNC_ACTIVE_LOW | > SOCAM_DATA_ACTIVE_HIGH | priv->info->buswidth; > - > > If I add above, and (for example) CEU - tw9910 case, > tw9910_query_bus_param will be used in > sh_mobile_ceu_set_bus_param. > > Then, the answer from soc_camera_bus_param_compatible > have both xSYNC_ACTIVE_HIGH/LOW. > In this case, CEU behavior will be LOW, > though it have HIGH. > Please check it. > > In my opinion, the answer from soc_camera_bus_param_compatible > should not have both ACTIVE HIGH/LOW. > This value will be used in tw9910_set_bus_param too. Right, sh_mobile_ceu_camera.c support for clients, that support both high and low polarities of various signals is incorrect. Here's what should happen: 1. the host supports both high and low polarity of a specific signal 2. the client also supports both of them 3. the host then should choose one of them, preferably according to platform configuration 4. the host should use the chosen polarity to configure the client and itself This is how the pxa_camera.c driver does it in pxa_camera_set_bus_param(). What actually happens in sh_mobile_ceu_camera.c, is that it keeps both polarities in its call to the client, so, the client picks up randomly one of them, then the host does the same. So, with a probability of 1/2 they will choose opposite polarities:-) > I guess, to add your order, we needs more than 2 patches. > > "modify soc_camera_bus_param_compatible behavior" No. We should fix the sh-CEU driver. > "tw9910: Add polarities support" 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] soc-camera: tw9910: modify V/H outpit pin setting to use VALID
Dear Guennadi Thank you > Oh, indeed. Ok, but can you add proper support for both high and low > polarities? I think your order will be added in next patch. "Add polarities support" I will send it OK ? By the way. Maybe I should ask you about it. My question is that who select ACTIVE HI / LOW ? - unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER | SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH | + SOCAM_VSYNC_ACTIVE_LOW | SOCAM_HSYNC_ACTIVE_LOW | SOCAM_DATA_ACTIVE_HIGH | priv->info->buswidth; - If I add above, and (for example) CEU - tw9910 case, tw9910_query_bus_param will be used in sh_mobile_ceu_set_bus_param. Then, the answer from soc_camera_bus_param_compatible have both xSYNC_ACTIVE_HIGH/LOW. In this case, CEU behavior will be LOW, though it have HIGH. Please check it. In my opinion, the answer from soc_camera_bus_param_compatible should not have both ACTIVE HIGH/LOW. This value will be used in tw9910_set_bus_param too. I guess, to add your order, we needs more than 2 patches. "modify soc_camera_bus_param_compatible behavior" "tw9910: Add polarities support" Best regards -- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] soc-camera: tw9910: modify V/H outpit pin setting to use VALID
On Fri, 13 Nov 2009, Kuninori Morimoto wrote: > > Dear Guennadi > > Thank you for checking patch > > > + ret = i2c_smbus_write_byte_data(client, OUTCTR1, > > + VSP_LO | VSSL_VVALID | > > + HSP_LO | HSSL_DVALID); > > > > now you dropped VSP_LO | HSP_LO, could you, please, explain, why? Also, > > sorry for not explaining properly. Yesterday I wrote > > Because VSP_LO = HSP_LO = 0. Oh, indeed. Ok, but can you add proper support for both high and low polarities? > And when I use xVALID, xSP_LO mean ACTIVE HI. > > So, I drop these explain because it is just un-understandable. 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] soc-camera: tw9910: modify V/H outpit pin setting to use VALID
Dear Guennadi Thank you for checking patch > + ret = i2c_smbus_write_byte_data(client, OUTCTR1, > + VSP_LO | VSSL_VVALID | > + HSP_LO | HSSL_DVALID); > > now you dropped VSP_LO | HSP_LO, could you, please, explain, why? Also, > sorry for not explaining properly. Yesterday I wrote Because VSP_LO = HSP_LO = 0. And when I use xVALID, xSP_LO mean ACTIVE HI. So, I drop these explain because it is just un-understandable. Best regards -- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] soc-camera: tw9910: modify V/H outpit pin setting to use VALID
Hello Morimoto-san
On Fri, 13 Nov 2009, Kuninori Morimoto wrote:
>
> Signed-off-by: Kuninori Morimoto
> ---
> v1 -> v2
>
> o remove un-understandable explain.
> -> tw9910_query_bus_param need not modify now
> o move OUTCTR1 setting to tw9910_set_bus_param
>
> drivers/media/video/tw9910.c | 38 --
> 1 files changed, 8 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/video/tw9910.c b/drivers/media/video/tw9910.c
> index 6d8dede..82135f2 100644
> --- a/drivers/media/video/tw9910.c
> +++ b/drivers/media/video/tw9910.c
> @@ -239,18 +239,6 @@ struct tw9910_priv {
> u32 revision;
> };
>
> -/*
> - * register settings
> - */
> -
> -#define ENDMARKER { 0xff, 0xff }
> -
> -static const struct regval_list tw9910_default_regs[] =
> -{
> - { OUTCTR1, VSP_LO | VSSL_VVALID | HSP_HI | HSSL_HSYNC },
> - ENDMARKER,
> -};
> -
> static const enum v4l2_imgbus_pixelcode tw9910_color_codes[] = {
> V4L2_IMGBUS_FMT_VYUY,
> };
> @@ -463,20 +451,6 @@ static int tw9910_set_hsync(struct i2c_client *client,
> return ret;
> }
>
> -static int tw9910_write_array(struct i2c_client *client,
> - const struct regval_list *vals)
> -{
> - while (vals->reg_num != 0xff) {
> - int ret = i2c_smbus_write_byte_data(client,
> - vals->reg_num,
> - vals->value);
> - if (ret < 0)
> - return ret;
> - vals++;
> - }
> - return 0;
> -}
> -
> static void tw9910_reset(struct i2c_client *client)
> {
> tw9910_mask_set(client, ACNTL1, SRESET, SRESET);
> @@ -578,7 +552,14 @@ static int tw9910_s_stream(struct v4l2_subdev *sd, int
> enable)
> static int tw9910_set_bus_param(struct soc_camera_device *icd,
> unsigned long flags)
> {
> - return 0;
> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> + struct i2c_client *client = sd->priv;
> +
> + /*
> + * set OUTCTR1
> + */
> + return i2c_smbus_write_byte_data(client, OUTCTR1,
> + VSSL_VVALID | HSSL_DVALID);
In v1 you did
+ ret = i2c_smbus_write_byte_data(client, OUTCTR1,
+ VSP_LO | VSSL_VVALID |
+ HSP_LO | HSSL_DVALID);
now you dropped VSP_LO | HSP_LO, could you, please, explain, why? Also,
sorry for not explaining properly. Yesterday I wrote
let's advertise both supported HSYNC and VSYNC polarities in
tw9910_query_bus_param() and configure the chip accordingly in
tw9910_set_bus_param()
which means, in tw9910_query_bus_param() you'd do
unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
+ SOCAM_VSYNC_ACTIVE_LOW | SOCAM_HSYNC_ACTIVE_LOW |
SOCAM_DATA_ACTIVE_HIGH | priv->info->buswidth;
and in tw9910_set_bus_param() you'd check which polarity is set like
if (flags & SOCAM_VSYNC_ACTIVE_HIGH)
ret = i2c_smbus_write_byte_data(...);
else
ret = i2c_smbus_write_byte_data(...);
if (ret < 0)
return ret;
if (flags & SOCAM_VSYNC_ACTIVE_HIGH)
ret = i2c_smbus_write_byte_data(...);
else
ret = i2c_smbus_write_byte_data(...);
Or are there any objections against this? I would really do this even if
we cannot positively test opposite polarities now.
Thanks
Guennadi
> }
>
> static unsigned long tw9910_query_bus_param(struct soc_camera_device *icd)
> @@ -681,9 +662,6 @@ static int tw9910_s_crop(struct v4l2_subdev *sd, struct
> v4l2_crop *a)
>* reset hardware
>*/
> tw9910_reset(client);
> - ret = tw9910_write_array(client, tw9910_default_regs);
> - if (ret < 0)
> - goto tw9910_set_fmt_error;
>
> /*
>* set bus width
> --
> 1.6.3.3
>
---
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] soc-camera: tw9910: modify V/H outpit pin setting to use VALID
Signed-off-by: Kuninori Morimoto
---
v1 -> v2
o remove un-understandable explain.
-> tw9910_query_bus_param need not modify now
o move OUTCTR1 setting to tw9910_set_bus_param
drivers/media/video/tw9910.c | 38 --
1 files changed, 8 insertions(+), 30 deletions(-)
diff --git a/drivers/media/video/tw9910.c b/drivers/media/video/tw9910.c
index 6d8dede..82135f2 100644
--- a/drivers/media/video/tw9910.c
+++ b/drivers/media/video/tw9910.c
@@ -239,18 +239,6 @@ struct tw9910_priv {
u32 revision;
};
-/*
- * register settings
- */
-
-#define ENDMARKER { 0xff, 0xff }
-
-static const struct regval_list tw9910_default_regs[] =
-{
- { OUTCTR1, VSP_LO | VSSL_VVALID | HSP_HI | HSSL_HSYNC },
- ENDMARKER,
-};
-
static const enum v4l2_imgbus_pixelcode tw9910_color_codes[] = {
V4L2_IMGBUS_FMT_VYUY,
};
@@ -463,20 +451,6 @@ static int tw9910_set_hsync(struct i2c_client *client,
return ret;
}
-static int tw9910_write_array(struct i2c_client *client,
- const struct regval_list *vals)
-{
- while (vals->reg_num != 0xff) {
- int ret = i2c_smbus_write_byte_data(client,
- vals->reg_num,
- vals->value);
- if (ret < 0)
- return ret;
- vals++;
- }
- return 0;
-}
-
static void tw9910_reset(struct i2c_client *client)
{
tw9910_mask_set(client, ACNTL1, SRESET, SRESET);
@@ -578,7 +552,14 @@ static int tw9910_s_stream(struct v4l2_subdev *sd, int
enable)
static int tw9910_set_bus_param(struct soc_camera_device *icd,
unsigned long flags)
{
- return 0;
+ struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+ struct i2c_client *client = sd->priv;
+
+ /*
+* set OUTCTR1
+*/
+ return i2c_smbus_write_byte_data(client, OUTCTR1,
+VSSL_VVALID | HSSL_DVALID);
}
static unsigned long tw9910_query_bus_param(struct soc_camera_device *icd)
@@ -681,9 +662,6 @@ static int tw9910_s_crop(struct v4l2_subdev *sd, struct
v4l2_crop *a)
* reset hardware
*/
tw9910_reset(client);
- ret = tw9910_write_array(client, tw9910_default_regs);
- if (ret < 0)
- goto tw9910_set_fmt_error;
/*
* set bus width
--
1.6.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
