Re: [PATCH 1/1] media: dt-bindings: Add bindings for camera modules

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Jacopo,

On Thu, May 08, 2025 at 07:00:34PM +0200, Jacopo Mondi wrote:
> Hi Sakari,
>   thanks a lot for the proposal, it will be useful for next week discussion

Thank you for the review!

> 
> On Wed, May 07, 2025 at 11:13:38AM +0300, Sakari Ailus wrote:
> > Add bindings for camera modules to allow telling especially the user space
> > which module is found in the system. Camera modules do not have a device
> > node so this is a property for the camera sensor device node. This allows
> > describing modules that contain a single camera sensor.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi all,
> >
> > Here's the patch to give some advance warning for the camera module
> > discussion. The good thing is that it's quite short.
> >
> > The intent indeed is to address the regular use case where we have a
> > single sensor in a camera module. For cases where we have more, we'll need
> > something else, not based on individual properties. I believe this is
> > still the way to go, to address current issues and for a couple of
> > additional reasons:
> >
> > - Cameras with more than one sensor tend to be collections of camera
> >   modules so this is still relevant in most cases.
> >
> > - It's much simpler to have a single property than begin having new nodes
> >   in DT. In practice such nodes would be a poor fit for DT generally as
> >   they have (few or) no functions.
> >
> > The biggest difficulty is still in module identification. These components
> > tend to be often ignored and the best we have for a module name in that
> > case is random-looking string if even that. Besides DT bindings, we need
> > an additional (git?) tree to describe the modules that have no proper
> > names but it could be also useful for those that do, for instance to
> > include information on lens, field of view, IR filter, photos of the
> > module etc. There is some overlap with what libcamera needs, too.
> >
> > - Sakari
> >
> >  .../bindings/media/camera-module.yaml         | 52 +++++++++++++++++++
> >  .../media/video-interface-devices.yaml        |  3 ++
> >  2 files changed, 55 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/camera-module.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/camera-module.yaml b/Documentation/devicetree/bindings/media/camera-module.yaml
> > new file mode 100644
> > index 000000000000..31b898c8c334
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/camera-module.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (C) 2025 Intel Corporation
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/camera-module.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Camera modules
> > +
> > +maintainers:
> > +  - Sakari Ailus <sakari.ailus@linux.intel.com>
> > +
> > +description: |
> > +  Camera modules are devices that embed one or more active devices, including
> > +  Camera Sensors, Voice Coil Motor (VCM) and possibly a flash LED as well as
> > +  other passive devices such as lenses and Ultra-Violet (UV) filters. While the
> > +  camera modules themselves have no OF nodes and have generally no module
> > +  specific functions, it still does matter for the software stack as a whole
> > +  which module the devices are a part of.
> > +
> > +  Two properties are used for this, depending on what is known of the module:
> 
> I might have missed a point here.
> 
> 
> > +
> > +  1. The model of the module is known. In this case the name of the module uses
> > +  the format "vendor,model[,version]" where "vendor" is the manufacturer of the
> > +  module and "model" the name of the model. The version part is optional. In
> > +  such cases the property "camera-module-canonical" will be used. If the vendor
> > +  is not known, the "gpio" vendor prefix is used.
> 
> So if the module is "known" it will be described using the above
> specified triplet
> 
> (also, why "gpio" ?)

"gpio" is reserved. Another option would be to reserve a name for an
unknown vendor. I believe DT maintainers will have an opinion on this.

> 
> > +
> > +  2. The model of the module is unknown. In this case, the module has an
> > +  identifier only, and will be described in detail in the camera module
> > +  database. The property "camera-module-casual" is used to denote such modules.
> 
> If the module is "unknown" it will be identified by a numerical id that
> points to the camera module database where it is "described in
> detail". But if an entry is present in the camera module database, then it's not
> really "unkown", right ?
> 
> What is the actual difference between an "unknown" and a "known"
> module then ?

We could use different terms certainly. I wanted to differentiate here
modules the name of which, and hopefully also the manufacturer of which, is
known. Otherwise we can just describe it by various other means that are
all sub-par compared to having a model and the name of the vendor written
on the side of the module.

> 
> 
> > +
> > +  Before including in this binding documentation, all modules shall also be
> > +  documented in add-URL-here.
> 
> If an entry in the camera module database is a requirement can't we
> simply point to that entry using a numerical id like you proposed for
> the "camera-module-casual" property ?

That exactly is the idea.

> 
> Thanks
>   j
> 
> > +
> > +  All camera modules listed below shall have the name of the sensor as well as
> > +  other devices included in the module as DT compatible string mentioned in a
> > +  comment after the enumeration, separated by a whitespace (" ").
> > +
> > +  Always keep the enumeration alphabetically (1) or numerically (2) sorted.
> > +
> > +properties:
> > +  camera-module-canonical:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum:
> > +      - "dell,0BF122N3" # onnn,ov01a10
> > +  camera-module-casual:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 1 # st,vs6555
> > +
> > +additionalProperties: true
> > diff --git a/Documentation/devicetree/bindings/media/video-interface-devices.yaml b/Documentation/devicetree/bindings/media/video-interface-devices.yaml
> > index cf7712ad297c..27fa6711367e 100644
> > --- a/Documentation/devicetree/bindings/media/video-interface-devices.yaml
> > +++ b/Documentation/devicetree/bindings/media/video-interface-devices.yaml
> > @@ -10,6 +10,9 @@ maintainers:
> >    - Jacopo Mondi <jacopo@jmondi.org>
> >    - Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > +allOf:
> > +  - $ref: /schemas/media/camera-module.yaml#
> > +
> >  properties:
> >    flash-leds:
> >      $ref: /schemas/types.yaml#/definitions/phandle-array

-- 
Regards,

Sakari Ailus