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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Cc: Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com>,
	Luca Weiss <luca@z3ntu.xyz>,
	robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	laurent.pinchart@ideasonboard.com, sakari.ailus@iki.fi,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670
Date: Fri, 27 Jan 2023 20:58:20 +0100	[thread overview]
Message-ID: <00139f11-76b0-138a-2f7b-c67d149eb25e@linaro.org> (raw)
In-Reply-To: <20230127181435.3d5rnrg5omxhn6l7@uno.localdomain>

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.

Best regards,
Krzysztof