Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753472AbdHGM0H (ORCPT ); Mon, 7 Aug 2017 08:26:07 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:56647 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753225AbdHGM0F (ORCPT ); Mon, 7 Aug 2017 08:26:05 -0400 Message-ID: <1502108760.2490.28.camel@pengutronix.de> Subject: Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on From: Philipp Zabel To: Jacob Chen Cc: "open list:ARM/Rockchip SoC..." , linux-kernel@vger.kernel.org, roliveir@synopsys.com, Linux Media Mailing List , Mauro Carvalho Chehab , vladimir_zapolskiy@mentor.com, Hans Verkuil , sakari.ailus@linux.intel.com, slongerbeam@gmail.com, robh+dt@kernel.org, lolivei@synopsys.com Date: Mon, 07 Aug 2017 14:26:00 +0200 In-Reply-To: References: <1500950041-5449-1-git-send-email-jacob-chen@iotwrt.com> <1502093851.2490.4.camel@pengutronix.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 2001:67c:670:100:3ad5:47ff:feaf:1a17 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2641 Lines: 80 Hi Jacob, On Mon, 2017-08-07 at 19:06 +0800, Jacob Chen wrote: [...] > >> > --- a/drivers/media/i2c/ov5647.c > >> > +++ b/drivers/media/i2c/ov5647.c > >> > @@ -253,6 +253,10 @@ static int ov5647_stream_on(struct v4l2_subdev *sd) > >> > { > >> > int ret; > >> > > >> > + ret = ov5647_write(sd, 0x4800, 0x04); > >> > + if (ret < 0) > >> > + return ret; > >> > + So this clears BIT(1) (force clock lane to low power mode) and BIT(5) (gate clock lane while idle) that were set by ov5647_stream_off() during __sensor_init() due to the change below. Is there a reason, btw, that this driver is full of magic register addresses and values? A few #defines would make this a lot more readable. > >> > ret = ov5647_write(sd, 0x4202, 0x00); > >> > if (ret < 0) > >> > return ret; > >> > @@ -264,6 +268,10 @@ static int ov5647_stream_off(struct v4l2_subdev *sd) > >> > { > >> > int ret; > >> > > >> > + ret = ov5647_write(sd, 0x4800, 0x25); > >> > + if (ret < 0) > >> > + return ret; > >> > + > >> > ret = ov5647_write(sd, 0x4202, 0x0f); > >> > if (ret < 0) > >> > return ret; > >> > @@ -320,7 +328,7 @@ static int __sensor_init(struct v4l2_subdev *sd) > >> > return ret; > >> > } > >> > > >> > - return ov5647_write(sd, 0x4800, 0x04); > >> > + return ov5647_stream_off(sd); I see now that BIT(2) (keep bus in LP-11 while idle) is and was always set. So the change is that initially, additionally to LP-11 mode, the clock lane is gated and forced into low power mode, as well? > >> > } > >> > > >> > static int ov5647_sensor_power(struct v4l2_subdev *sd, int on) > >> > -- > >> > 2.7.4 > >> > > >> > >> Can anyone comment on it? > >> > >> I saw there is a same discussion in https://patchwork.kernel.org/patch/9569031/ > >> There is a comment in i.MX CSI2 driver. > >> " > >> Configure MIPI Camera Sensor to put all Tx lanes in LP-11 state. > >> This must be carried out by the MIPI sensor's s_power(ON) subdev > >> op. > >> " > >> That's what this patch do, sensor driver should make sure that clock > >> lanes are in stop state while not streaming. > > > > This is not the same, as far as I can tell. BIT(5) is just clock lane > > gating, as you describe above. To put the bus into LP-11 state, BIT(2) > > needs to be set. > > > > Yeah, but i double that clock lane is not in LP11 when continue clock > mode is enabled. If indeed LP-11 state is not achieved while the sensor is idle, as long as BIT(5) is cleared, I think this patch is correct. regards Philipp