Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 27/01/2023 21:38, Sakari Ailus wrote:
> Hi Krzysztof,
> 
> On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote:
>> On 27/01/2023 19:14, Jacopo Mondi wrote:
>>> Hi Krzysztof
>>>
>>> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote:
>>>> On 26/01/2023 17:59, Jacopo Mondi wrote:
>>>>> Add the bindings documentation for Omnivision OV5670 image sensor.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>>> ---
>>>>
>>>> (...)
>>>>
>>>>> +
>>>>> +  dovdd-supply:
>>>>> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
>>>>> +
>>>>> +  port:
>>>>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>>>>> +    additionalProperties: false
>>>>> +
>>>>> +    properties:
>>>>> +      endpoint:
>>>>> +        $ref: /schemas/media/video-interfaces.yaml#
>>>>> +        unevaluatedProperties: false
>>>>> +
>>>>> +        properties:
>>>>> +          data-lanes:
>>>>> +            minItems: 1
>>>>> +            maxItems: 2
>>>>> +            items:
>>>>> +              enum: [1, 2]
>>>>> +
>>>>> +          clock-noncontinuous: true
>>>>
>>>> You do not need this. Drop.
>>>>
>>>
>>> Is this due to "unevaluatedProperties: false" ?
>>>
>>> I read you recent explanation to a similar question on the Visconti
>>> bindings. Let me summarize my understanding:
>>>
>>> For a given schema a property could be
>>> - required
>>>         required:
>>>           - foo
>>>
>>> - optional
>>>         by default with "unevaluatedProperties: false"
>>>         "foo: true" with "additionalProperties: false"
>>>
>>> - forbidden
>>>         "foo: false" with "unevaluatedProperties: false"
>>>         by default wiht "additionalProperties: false"
>>>
>>> clock-noncontinuous is defined in video-interfaces.yaml. as I specify
>>> "unevaluatedProperties: false" does it mean
>>> all the properties defined in video-interfaces.yaml are optionally
>>> accepted ? If that's the case that's not what I want as
>>> clock-noncontinuous is -the only- property from that file we want to
>>> accept here (and data-lanes ofc).
>>>
>>> Should I change "unevaluatedProperties: false" to
>>> "additionalProperties: false" and keep "clock-noncontinuous: true"  ?
>>>
>>
>> Why would you disallow other properties? Just because driver does not
>> use them? That's not correct, driver change but bindings should stay the
>> same.
> 
> The clock-noncontinuous property is relevant for the hardware. There are
> some properties not listed here that might be relevant (for all camera
> sensors) but most properties in video-interfaces.yaml are not applicable to
> this device.
> 
> I also think is be useful to say what is relevant in DT bindings, as the
> other sources of information left are hardware datasheets (if you have
> access to them) or the driver (which is supposed not to be relevant for the
> bindings).
> 

Then it might be meaningful to list all allowed properties - even if not
currently supported by the driver - and use additionalProperties:false.
This has drawback - whenever video-interfaces gets something new, the
bindings here (and other such devices) will have to be explicitly enabled.

Best regards,
Krzysztof