Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752268AbdIKB7j (ORCPT ); Sun, 10 Sep 2017 21:59:39 -0400 Received: from mail-qk0-f193.google.com ([209.85.220.193]:34093 "EHLO mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752246AbdIKB7h (ORCPT ); Sun, 10 Sep 2017 21:59:37 -0400 X-Google-Smtp-Source: AOwi7QA3pDYN5J2UGrKGN4oXy01bCdqC9w2cQ8FYZ6QjZ51C9I4xUBoSBeu7mc7/4tkzT9dfDMeIOFqivvVI1Yzm3N4= MIME-Version: 1.0 In-Reply-To: <5121da6b-a37d-270b-2587-b2ee77635546@synopsys.com> References: <1500950041-5449-1-git-send-email-jacob-chen@iotwrt.com> <1502093851.2490.4.camel@pengutronix.de> <1502108760.2490.28.camel@pengutronix.de> <5121da6b-a37d-270b-2587-b2ee77635546@synopsys.com> From: Jacob Chen Date: Mon, 11 Sep 2017 09:59:35 +0800 Message-ID: Subject: Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on To: Luis Oliveira Cc: Philipp Zabel , "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, Steve Longerbeam , robh+dt@kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v8B1xhdU015080 Content-Length: 3832 Lines: 116 Hi Luis, 2017-08-07 22:48 GMT+08:00 Luis Oliveira : > Hi all, > > I'm new here, I got to be Maintainer of this driver by the old Maintainer > recommendation. Still getting the hang of it :) > > On 07-Aug-17 13:26, Philipp Zabel wrote: >> 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. >> > > For what I can see I agree that a few register name setting could be done. > >>>>>> 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? >> > > This is my interpretation 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://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9569031_&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=eMn12aiiNuIDjtRi5xEzC7tWJkpra2vl_XYFVvfxIGE&m=eortcRXje2uLyZNI_-Uw3Ur_z24tb-e4pZfom7WhdE0&s=6sLc76bhjR0IdaA3ArZ7F7slgtcyGz8pDTzAF_CBLno&e= >>>>> 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. > > I think by spec it shouldn't got to stopstate in continuous clock. > >> >> 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 >> > > As far as I understand, bit[5] set to 1 will force clock lane to be gated (in > other words it will be forced to be in LP-11 if there are no packets to > transmit). But also LP-11 must not be achieved with the BIT(5) cleared (free > running mode)? > Yes. When the BIT(5) is cleared, clock lane will be in continuous mode immediately, unless BIT(0) is set. > Sorry if I misunderstood something. > > regards, > Luis >