[v1] libipa: camera_sensor_helper: Add OV5675 black level
diff mbox series

Message ID 20240723141814.76823-1-dse@thaumatec.com
State Accepted
Headers show
Series
  • [v1] libipa: camera_sensor_helper: Add OV5675 black level
Related show

Commit Message

Daniel Semkowicz July 23, 2024, 2:17 p.m. UTC
Add black level value for OV5675 camera.
According to datasheet, default value is 0x10, 10 bits width.

Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
---
 src/ipa/libipa/camera_sensor_helper.cpp | 2 ++
 1 file changed, 2 insertions(+)

Comments

Quentin Schulz July 23, 2024, 2:25 p.m. UTC | #1
Hi Daniel,

On 7/23/24 4:17 PM, Daniel Semkowicz wrote:
> Add black level value for OV5675 camera.
> According to datasheet, default value is 0x10, 10 bits width.
> 
> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>

Yes.... but I just checked now the init sequence in the Linux kernel 
driver and it's different from the default :/

The 10b value is stored in two registers: 0x4002 BLC[9:8] as LSBs and 
0x4003 for BLC[7:0]. The default value is indeed 0x00 and 0x10, BUT the 
driver sets 0x4003 to 0x40 so I think this needs to be 0x40? Can you see 
if this makes things better?

Cheers,
Quentin
Kieran Bingham July 23, 2024, 2:39 p.m. UTC | #2
Quoting Daniel Semkowicz (2024-07-23 15:17:58)
> Add black level value for OV5675 camera.
> According to datasheet, default value is 0x10, 10 bits width.

I don't have that datasheet, but given the above statement the below is
correct:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index a1339c83..79608c00 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -601,6 +601,8 @@ class CameraSensorHelperOv5675 : public CameraSensorHelper
>  public:
>         CameraSensorHelperOv5675()
>         {
> +               /* From datasheet: 0x10 at 10bits. */
> +               blackLevel_ = 1024;
>                 gainType_ = AnalogueGainLinear;
>                 gainConstants_.linear = { 1, 0, 0, 128 };
>         }
> -- 
> 2.45.2
> 
>
Daniel Semkowicz July 23, 2024, 2:47 p.m. UTC | #3
True, I missed it... Thanks!

I will upload v2 with value from kernel driver.


On Tue, Jul 23, 2024 at 4:39 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Daniel Semkowicz (2024-07-23 15:17:58)
> > Add black level value for OV5675 camera.
> > According to datasheet, default value is 0x10, 10 bits width.
>
> I don't have that datasheet, but given the above statement the below is
> correct:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >
> > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> > ---
> >  src/ipa/libipa/camera_sensor_helper.cpp | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > index a1339c83..79608c00 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -601,6 +601,8 @@ class CameraSensorHelperOv5675 : public CameraSensorHelper
> >  public:
> >         CameraSensorHelperOv5675()
> >         {
> > +               /* From datasheet: 0x10 at 10bits. */
> > +               blackLevel_ = 1024;
> >                 gainType_ = AnalogueGainLinear;
> >                 gainConstants_.linear = { 1, 0, 0, 128 };
> >         }
> > --
> > 2.45.2
> >
> >
Kieran Bingham July 23, 2024, 2:52 p.m. UTC | #4
Quoting Quentin Schulz (2024-07-23 15:25:34)
> Hi Daniel,
> 
> On 7/23/24 4:17 PM, Daniel Semkowicz wrote:
> > Add black level value for OV5675 camera.
> > According to datasheet, default value is 0x10, 10 bits width.
> > 
> > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> 
> Yes.... but I just checked now the init sequence in the Linux kernel 
> driver and it's different from the default :/
> 
> The 10b value is stored in two registers: 0x4002 BLC[9:8] as LSBs and 
> 0x4003 for BLC[7:0]. The default value is indeed 0x00 and 0x10, BUT the 
> driver sets 0x4003 to 0x40 so I think this needs to be 0x40? Can you see 
> if this makes things better?

Yikes,

That means we really can't even trust the datasheet values and we should
be exposing that value from the kernel through the control framework and
reading it (or setting it ?) at runtime.

--
Kieran

Patch
diff mbox series

diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
index a1339c83..79608c00 100644
--- a/src/ipa/libipa/camera_sensor_helper.cpp
+++ b/src/ipa/libipa/camera_sensor_helper.cpp
@@ -601,6 +601,8 @@  class CameraSensorHelperOv5675 : public CameraSensorHelper
 public:
 	CameraSensorHelperOv5675()
 	{
+		/* From datasheet: 0x10 at 10bits. */
+		blackLevel_ = 1024;
 		gainType_ = AnalogueGainLinear;
 		gainConstants_.linear = { 1, 0, 0, 128 };
 	}