Message ID | uzl6r6re1.wl%morimoto.kuninori@renesas.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@infradead.org Delivery-date: Fri, 13 Nov 2009 03:22:07 +0000 Received: from bombadil.infradead.org [18.85.46.34] by pedra.chehab.org with IMAP (fetchmail-6.3.6) for <mchehab@localhost> (single-drop); Fri, 13 Nov 2009 09:56:32 -0200 (BRST) Received: from vger.kernel.org ([209.132.176.167]) by bombadil.infradead.org with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1N8mjf-0003oO-4Z; Fri, 13 Nov 2009 03:22:07 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755113AbZKMDV6 (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Thu, 12 Nov 2009 22:21:58 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755250AbZKMDV6 (ORCPT <rfc822;linux-media-outgoing>); Thu, 12 Nov 2009 22:21:58 -0500 Received: from mail.renesas.com ([202.234.163.13]:43748 "EHLO mail04.idc.renesas.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755101AbZKMDV5 (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 12 Nov 2009 22:21:57 -0500 X-AuditID: ac140387-00000009000005fd-2c-4afcd0d7639c Received: from guardian01.idc.renesas.com ([172.20.8.200]) by mail04.idc.renesas.com (sendmail) with ESMTP id nAD3LxGL000199; Fri, 13 Nov 2009 12:21:59 +0900 (JST) Received: (from root@localhost) by guardian01.idc.renesas.com with id nAD3LxJn027927; Fri, 13 Nov 2009 12:21:59 +0900 (JST) Received: from mta01.idc.renesas.com (localhost [127.0.0.1]) by mta01.idc.renesas.com with ESMTP id nAD3LwKV022916; Fri, 13 Nov 2009 12:21:58 +0900 (JST) Received: from PG10870.renesas.com ([172.30.8.159]) by ims05.idc.renesas.com (Sendmail) with ESMTPA id <0KT100ESO2ONB6@ims05.idc.renesas.com>; Fri, 13 Nov 2009 12:21:59 +0900 (JST) Date: Fri, 13 Nov 2009 12:04:01 +0900 From: Kuninori Morimoto <morimoto.kuninori@renesas.com> Subject: [PATCH v2] soc-camera: tw9910: modify V/H outpit pin setting to use VALID To: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Cc: Linux Media Mailing List <linux-media@vger.kernel.org> Message-id: <uzl6r6re1.wl%morimoto.kuninori@renesas.com> MIME-version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-type: text/plain; charset=US-ASCII User-Agent: SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL/10.7 Emacs/22.3 (i386-msvc-nt5.1.2600) MULE/5.0 (SAKAKI) Meadow/3.02-dev (RINDOU) (2009-06-17 Rev.4261) X-Brightmail-Tracker: AAAAAA== Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Kuninori Morimoto
Nov. 13, 2009, 3:04 a.m. UTC
Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
---
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(-)
Comments
Hello Morimoto-san On Fri, 13 Nov 2009, Kuninori Morimoto wrote: > > Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com> > --- > 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 <quote> let's advertise both supported HSYNC and VSYNC polarities in tw9910_query_bus_param() and configure the chip accordingly in tw9910_set_bus_param() </quote> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 13 Nov 2009, Kuninori Morimoto wrote: > > Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com> > --- > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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