Re: [PATCH v3 1/2] media: i2c: OV5647: ensure clock lane in LP-11 state before streaming on
Hi all,
Sorry for the delay and thank you for the fix.
I just checked the databook and the changes makes sense.
cheers,
Luis
On 16-Oct-17 13:23, Sakari Ailus wrote:
> Luis,
>
> Any comment on these?
>
> On Sun, Oct 01, 2017 at 06:22:37PM +0800, Jacob Chen wrote:
>> When I was supporting Rpi Camera Module on the ASUS Tinker board,
>> I found this driver have some issues with rockchip's mipi-csi driver.
>> It didn't place clock lane in LP-11 state before performing
>> D-PHY initialisation.
>>
>> From our experience, on some OV sensors,
>> LP-11 state is not achieved while BIT(5)-0x4800 is cleared.
>>
>> So let's set BIT(5) and BIT(0) both while not streaming, in order to
>> coax the clock lane into LP-11 state.
>>
>> 0x4800 : MIPI CTRL 00
>> BIT(5) : clock lane gate enable
>> 0: continuous
>> 1: none-continuous
>> BIT(0) : manually set clock lane
>> 0: Not used
>> 1: used
>>
>> Signed-off-by: Jacob Chen <jacob-c...@iotwrt.com>
>> ---
>> drivers/media/i2c/ov5647.c | 13 -
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
>> index 95ce90fdb876..247302d01f53 100644
>> --- 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;
>> +
>> 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,10 @@ static int __sensor_init(struct v4l2_subdev *sd)
>> return ret;
>> }
>>
>> -return ov5647_write(sd, 0x4800, 0x04);
>> +/*
>> + * stream off to make the clock lane into LP-11 state.
>> + */
>> +return ov5647_stream_off(sd);
>> }
>>
>> static int ov5647_sensor_power(struct v4l2_subdev *sd, int on)
>> --
>> 2.14.1
>>
>
Reviewed-by: Luis Oliveira <loli...@synopsys.com>
Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on
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_=DwICaQ=DPL6_X_6JkXFx7AXWqB0tg=eMn12aiiNuIDjtRi5xEzC7tWJkpra2vl_XYFVvfxIGE=eortcRXje2uLyZNI_-Uw3Ur_z24tb-e4pZfom7WhdE0=6sLc76bhjR0IdaA3ArZ7F7slgtcyGz8pDTzAF_CBLno=
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)?
Sorry if I misunderstood something.
regards,
Luis
[PATCH] staging: media: atomisp: fix misspelled word in comment
This fix "overrided", the correct past tense form of "override" is "overridden". Signed-off-by: Luis Oliveira <loli...@synopsys.com> --- drivers/staging/media/atomisp/i2c/ov2680.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/atomisp/i2c/ov2680.c b/drivers/staging/media/atomisp/i2c/ov2680.c index c08dd0b18fbb..566091035c64 100644 --- a/drivers/staging/media/atomisp/i2c/ov2680.c +++ b/drivers/staging/media/atomisp/i2c/ov2680.c @@ -1122,7 +1122,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, } /*recall flip functions to avoid flip registers -* were overrided by default setting +* were overridden by default setting */ if (h_flag) ov2680_h_flip(sd, h_flag); -- 2.11.0
