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
