[libcamera-devel] [PATCH] libcamera: Add Samsung S5K3L6XX sensor support

[Dorota Czaplejewicz]

Hi Kieran,

On Tue, 12 Oct 2021 15:07:15 +0100
Kieran Bingham <kieran.bingham at ideasonboard.com> wrote:
> Quoting Dorota Czaplejewicz (2021-10-12 12:39:46)
> > The sensor is called s5k3l6xx in the kernel.
> > 
> > The driver is currently out of tree and maintained by Purism.  
> 
> We prefer kernel drivers to be either upstream, or at least on their way
> upstream. Ideally, a minimum of having been posted to the linux-media
> mailinglist, however we know that the review process can take time, so
> we can integrate support to libcamera before it's fully upstream in the
> kernel to support the upstreaming process.
> 
> Is this sensor driver something that you are intending to post to the
> linux-media mailinglist in the near future, or has it been posted
> already?

The driver still has some deficiencies, so I haven't posted it yet. I expect I will within a couple months.
> 
> > 
> > It's the main camera sensor on the Librem 5.
> > 
> > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz at puri.sm>
> > ---
> >  src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> > index 48305ac4..0625a0d8 100644
> > --- a/src/libcamera/camera_sensor_properties.cpp
> > +++ b/src/libcamera/camera_sensor_properties.cpp
> > @@ -134,6 +134,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> >                                 { controls::draft::TestPatternModeColorBars, 1 },
> >                         },
> >                 } },
> > +               { "s5k3l6xx", {
> > +                         .unitCellSize = { 1120, 1120 },
> > +                         .testPatternModes = {
> > +                                 { 0, controls::draft::TestPatternModeOff },
> > +                                 { 1, controls::draft::TestPatternModeSolidColor },
> > +                                 { 2, controls::draft::TestPatternModeColorBars },
> > +                                 { 3, controls::draft::TestPatternModeColorBarsFadeToGray },
> > +                                 { 4, controls::draft::TestPatternModeCustom1 }, /* White */
> > +                                 { 5, controls::draft::TestPatternModePn9 },
I just noticed those two are swapped.

> > +                                 { 6, controls::draft::TestPatternModeCustom1 + 1 }, /* LFSR32 */
> > +                                 { 7, controls::draft::TestPatternModeCustom1 + 2 }, /* Address */  
> 
> That's an interesting way to define the extra custom modes...
> Interstingly, I don't see any users of TestPatternModeCustom1 yet.

I found it directly in the yaml file, where the description suggests this way of dealing with it.
> 
> What is LFSR32 and Address?
Neither is documented in the application note, but LFSR32 appears to be a pseudo-random algorithm: https://gist.github.com/edarc/2946448

> 
> Is 'White' just a solid colour of 'white'?
> 
Yes, the maximum integer value is returned.

> I suspect these modes should be defined within
> src/libcamera/control_ids.yaml where possible.  An all white could be
> TestPatternModeSolidColorWhite or TestPatternModeSolidWhite for intance.
> 
> --
> Kieran
> 
> 
> > +                       },
> > +               } },
> >         };
> >  
> >         const auto it = sensorProps.find(sensor);
> > -- 
> > 2.31.1
> >  

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211012/8e4fba67/attachment-0001.sig>