Re: [EXT] Re: [PATCH 1/5] dt-bindings: media: i2c: Add bindings for OX05B1S sensor driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello Mirela,

On Wed, Oct 30, 2024 at 08:02:44AM +0200, Mirela Rabulea wrote:
> On 29.10.2024 13:57, Laurent Pinchart wrote:
> >>> +
> >>> +  orientation: true
> >>> +  rotation: true
> >>
> >> I think you can drop both of these too.
> >
> > Aren't they needed given that the binding ends with
> >
> > additionalProperties: false
> >
> > ?
>
> I added orientation & rotation properties in order to support 
> orientation and rotation controls, libcamera warns about those (optional 
> requirements last time I checked).

The orientation and rotation properties should certainly be specified in
DT sources. They are standardized in video-interface-devices.yaml which
Bryan pointed out you should reference. If you're not familiar yet with
with how the YAML schemas used for DT bindings reference core schemas,
now would be a good time to have a look at it :-)

In a nutshell, you'll find that all properties need to be properly
defined with appropriate constraints, and properties shared by multiple
devices have constraints defined in core schemas. Some are included
automatically and are applied based on property names, other need a
manual $ref. You can have a look at
https://github.com/devicetree-org/dt-schema.git to see core schemas that
get automatically selected, they specify "select: true". For example the
schemas defining the reg or clocks properties don't have to be manually
referenced.

Bryan's comment about dropping the orientation and rotation properties
was related to the fact that the video-interface-devices.yaml schema
defines them already. With "unevaluatedProperties: false", you won't
need to specify "orientation: true". With "additionalProperties: false",
you will. It's a good idea to learn about the difference between those
two and how they really work.

> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >
> > The device requires a clock, shouldn't the clocks property be required ?
> 
> I intentionally left the clock optional, because NXP has a converter 
> board which supports both ox05b1s and os08a20 sensor, and the converter 
> board has an oscillator, and we are using that, not the SOC clock.

That's fine, you can have a fixed clock node in DT to model that. DT
bindings describe the intrinsic needs of a particular device. If the
sensor requires a clock, I think it should be mandatory. If the sensor
itself could operate without an external clock (i.e. if it had an
internal oscillator) then the property could be optional.

> Here is how the module looks like for os08a20 for imx8mp:
> 
> https://docs.nxp.com/bundle/AN13712/page/topics/os08a20_sensor_module.html
> 
> There's a newer revision for the converter board for imx95, sorry but I 
> do not have a link for that.
> 
> For imx8mp, we used in the past the clock from the SOC, then switched to 
> the external clock (from the converter board).
> 
> I think Omnivision has their own module.
> 
> So, I thought leaving the clock as optional allows for more flexibility.
> 
> >>> +  - port
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    #include <dt-bindings/gpio/gpio.h>
> >>> +
> >>> +    i2c {
> >>> +        #address-cells = <1>;
> >>> +        #size-cells = <0>;
> >>> +
> >>> +        ox05b1s: ox05b1s@36 {
> >>> +            compatible = "ovti,ox05b1s";
> >>> +            reg = <0x36>;
> >>> +            reset-gpios = <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW>;
> >
> > This isn't specified in the bindings. Does the example validate ?
> 
> Apparently yes, I mean dt_binding_check passed:
> 
> $ rm Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.example.dtb
> 
> $ make dt_binding_check DT_CHECKER_FLAGS=-m 
> DT_SCHEMA_FILES=ovti,ox05b1s.yaml
>    DTC [C] 
> Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.example.dtb
> 
> I have dtschema-2024.10.dev6+g12c3cd5.
> 
> 
> The "reset-gpios" is described in this binding, as the GPIO connected to 
> the XSHUTDOWN pin.

Ah sorry, Bryan dropped that part from his reply, so I didn't notice it.

> The <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW> is what works for imx95 
> ("nxp,pcal6408"), for imx8mp this works:
> 
> reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;

-- 
Regards,

Laurent Pinchart