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

[Mirela Rabulea <mirela.rabulea@nxp.com>]

Hi Laurent,

On 04.11.2024 16:25, Laurent Pinchart wrote:
> 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.

Thanks for info :)

I just sent the v2, I added video-interface-devices.yaml.

>
>>>>> +
>>>>> +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.

Also, in v2, I added the clock as mandatory. Strictly from sensor's 
module point of view, it is mandatory (will use a fixed clock for nxp 
board). Also added regulators.

Again, thank you for feedback.


Regards,

Mirela

>
>> 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