Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Jacob Chen]

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_=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)?
>

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
>

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Jacob Chen]

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_=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)?
>

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
>

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Jacob Chen]

Hi all,

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.
>

BIT(0) are not necessary, just i saw many driver have set it both with BIT(5).

>>  }
>>
>>  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.
>

I do some experiments.
I didn't have instruments to test, so i just observe it through phy registers.

If BIT(5) are cleared in "ov5647_sensor_power" and do nothing about it
in "ov5647_stream_on",
Phy didn't get a SoT from sensor in "ov5647_stream_on" and it keep its
clock lane in lp mode.


if BIT(5) are set in "ov5647_sensor_power", and cleared in "ov5647_stream_on".
Phy will get a SoT and the clock lane will enter hs mode.


So i'm pretty sure that LP-11 must not be achieved with the BIT(5) cleared.

> regards,
> Luis
>

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Jacob Chen]

Hi all,

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.
>

BIT(0) are not necessary, just i saw many driver have set it both with BIT(5).

>>  }
>>
>>  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.
>

I do some experiments.
I didn't have instruments to test, so i just observe it through phy registers.

If BIT(5) are cleared in "ov5647_sensor_power" and do nothing about it
in "ov5647_stream_on",
Phy didn't get a SoT from sensor in "ov5647_stream_on" and it keep its
clock lane in lp mode.


if BIT(5) are set in "ov5647_sensor_power", and cleared in "ov5647_stream_on".
Phy will get a SoT and the clock lane will enter hs mode.


So i'm pretty sure that LP-11 must not be achieved with the BIT(5) cleared.

> regards,
> Luis
>

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[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_=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

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[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_=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

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Philipp Zabel]

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

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Philipp Zabel]

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

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Jacob Chen]

Hi Philipp,

2017-08-07 16:17 GMT+08:00 Philipp Zabel :
> Hi Jacob,
>
> On Mon, 2017-08-07 at 15:11 +0800, Jacob Chen wrote:
>> Hi all,
>>
>> 2017-07-25 10:34 GMT+08:00 Jacob Chen :
>> > According to datasheet, BIT5 in reg-0x4800 are used to
>> > enable/disable clock lane gate.
>> >
>> > It's wrong to make clock lane free running before
>> > sensor stream on was called, while the mipi phy
>> > are not initialized.
>> >
>> > Signed-off-by: Jacob Chen 
>>>
>> > ---
>> >  drivers/media/i2c/ov5647.c | 10 +-
>> >  1 file changed, 9 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
>> > index 95ce90f..d3e6fd0 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,7 @@ static int __sensor_init(struct v4l2_subdev *sd)
>> > return ret;
>> > }
>> >
>> > -   return ov5647_write(sd, 0x4800, 0x04);
>> > +   return ov5647_stream_off(sd);
>> >  }
>> >
>> >  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.

> regards
> Philipp
>

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Jacob Chen]

Hi Philipp,

2017-08-07 16:17 GMT+08:00 Philipp Zabel :
> Hi Jacob,
>
> On Mon, 2017-08-07 at 15:11 +0800, Jacob Chen wrote:
>> Hi all,
>>
>> 2017-07-25 10:34 GMT+08:00 Jacob Chen :
>> > According to datasheet, BIT5 in reg-0x4800 are used to
>> > enable/disable clock lane gate.
>> >
>> > It's wrong to make clock lane free running before
>> > sensor stream on was called, while the mipi phy
>> > are not initialized.
>> >
>> > Signed-off-by: Jacob Chen 
>>>
>> > ---
>> >  drivers/media/i2c/ov5647.c | 10 +-
>> >  1 file changed, 9 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
>> > index 95ce90f..d3e6fd0 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,7 @@ static int __sensor_init(struct v4l2_subdev *sd)
>> > return ret;
>> > }
>> >
>> > -   return ov5647_write(sd, 0x4800, 0x04);
>> > +   return ov5647_stream_off(sd);
>> >  }
>> >
>> >  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.

> regards
> Philipp
>

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Philipp Zabel]

Hi Jacob,

On Mon, 2017-08-07 at 15:11 +0800, Jacob Chen wrote:
> Hi all,
> 
> 2017-07-25 10:34 GMT+08:00 Jacob Chen :
> > According to datasheet, BIT5 in reg-0x4800 are used to
> > enable/disable clock lane gate.
> >
> > It's wrong to make clock lane free running before
> > sensor stream on was called, while the mipi phy
> > are not initialized.
> >
> > Signed-off-by: Jacob Chen 
>>
> > ---
> >  drivers/media/i2c/ov5647.c | 10 +-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 95ce90f..d3e6fd0 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,7 @@ static int __sensor_init(struct v4l2_subdev *sd)
> > return ret;
> > }
> >
> > -   return ov5647_write(sd, 0x4800, 0x04);
> > +   return ov5647_stream_off(sd);
> >  }
> >
> >  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.

regards
Philipp

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Philipp Zabel]

Hi Jacob,

On Mon, 2017-08-07 at 15:11 +0800, Jacob Chen wrote:
> Hi all,
> 
> 2017-07-25 10:34 GMT+08:00 Jacob Chen :
> > According to datasheet, BIT5 in reg-0x4800 are used to
> > enable/disable clock lane gate.
> >
> > It's wrong to make clock lane free running before
> > sensor stream on was called, while the mipi phy
> > are not initialized.
> >
> > Signed-off-by: Jacob Chen 
>>
> > ---
> >  drivers/media/i2c/ov5647.c | 10 +-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 95ce90f..d3e6fd0 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,7 @@ static int __sensor_init(struct v4l2_subdev *sd)
> > return ret;
> > }
> >
> > -   return ov5647_write(sd, 0x4800, 0x04);
> > +   return ov5647_stream_off(sd);
> >  }
> >
> >  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.

regards
Philipp

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Jacob Chen]

Hi all,

2017-07-25 10:34 GMT+08:00 Jacob Chen :
> According to datasheet, BIT5 in reg-0x4800 are used to
> enable/disable clock lane gate.
>
> It's wrong to make clock lane free running before
> sensor stream on was called, while the mipi phy
> are not initialized.
>
> Signed-off-by: Jacob Chen 
> ---
>  drivers/media/i2c/ov5647.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 95ce90f..d3e6fd0 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,7 @@ static int __sensor_init(struct v4l2_subdev *sd)
> return ret;
> }
>
> -   return ov5647_write(sd, 0x4800, 0x04);
> +   return ov5647_stream_off(sd);
>  }
>
>  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.

Re: [PATCH] media: i2c: OV5647: gate clock lane before stream on

[Jacob Chen]

Hi all,

2017-07-25 10:34 GMT+08:00 Jacob Chen :
> According to datasheet, BIT5 in reg-0x4800 are used to
> enable/disable clock lane gate.
>
> It's wrong to make clock lane free running before
> sensor stream on was called, while the mipi phy
> are not initialized.
>
> Signed-off-by: Jacob Chen 
> ---
>  drivers/media/i2c/ov5647.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 95ce90f..d3e6fd0 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,7 @@ static int __sensor_init(struct v4l2_subdev *sd)
> return ret;
> }
>
> -   return ov5647_write(sd, 0x4800, 0x04);
> +   return ov5647_stream_off(sd);
>  }
>
>  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.