Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/03/2022 17:05, Jacopo Mondi wrote:
> Hi Krzysztof,
> 
> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
>> On 10/03/2022 18:16, Jacopo Mondi wrote:
>>> Hi Krzysztof
>>>
>>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
>>>> On 10/03/2022 14:08, Jacopo Mondi wrote:
>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>>>>> ---
>>>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
>>>>
>>>> Add the file to maintainers entry.
>>>>
>>>
>>> Right
>>>
>>>>>  1 file changed, 93 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..dc4a3297bf6f
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>
>>>> Missing vendor prefix in file name.
>>>>
>>>
>>> Right x2
>>>
>>>>> @@ -0,0 +1,93 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
>>>>> +
>>>>> +maintainers:
>>>>> +  - Jacopo Mondi <jacopo@xxxxxxxxxx>
>>>>
>>>> Please add also driver maintainer.
>>>>
>>>
>>> I never got what the policy was, if the maintainer entries here only
>>> refer to the binding file or to the driver too
>>
>> It is a person responsible for the bindings, so indeed it might not feed
>> existing maintainer.
>>
>>>
>>>>> +
>>>>> +description: |-
>>>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
>>>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
>>>>> +  controlled through an I2C compatible control bus.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: ovti,ov5670
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clock-frequency:
>>>>> +    description: Frequency of the xclk clock.
>>>>
>>>> Is the xclk external clock coming to the sensor? If yes, there should be
>>>> a "clocks" property.
>>>>
>>>
>>> To be honest I was not sure about this, as clock-frequency is already
>>> used by the driver for the ACPI part, but it seems to in DT bindings
>>> it is a property meant to be specified in the clock providers, even if
>>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
>>> really clarify this
>>>
>>> Clock consumer should rather use 'clocks' and point to the provider's
>>> phandle if my understanding is right.
>>
>> This is a clock-frequency, not clock reference. For external clocks, a
> 
> Yes, I was suggesting to replace clock-frequency with clocks, that
> accepts a phandle.
> 
> The thing is, the driver parses 'clock-frequency'
> 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> 
> which I assume comes from ACPI (as the driver was developed for an
> ACPI platform).
> 
> If in DTS we don't use it, I then need to
> 
> #ifdef CONFIG_ACPI
> 
> #elif defined CONFIG_OF
> 
> #endif
> 
> Which I would really like to avoid.
> 
> Anyone with ACPI experience that knows where clock-frequency comes
> from ?

I would assume that ACPI simply does not support common clock framework,
so it had to use clock-frequency. Several of such drivers were added by
folks from Intel which use ACPI, not Devicetree.

> 
>> clock phandles + assigned-clock-rates should be rather used. However for
>> internal clocks, this is a perfectly valid property.
>>
>> Therefore the question is - what is the "xclk"?
> 
> xclk is the clock fed to the sensor, which which all its internal
> clocks are generated, so it's indeed an 'external' clock. As I've
> said, clock-frequency seems to be meant for clock providers, and
> the image sensor is a clock consumer.

Regardless whether clock-frequency stays or not, you need the clocks
property in such case.

> 
>>
>>>
>>>>> +
>>>>> +  pwdn-gpios:
>>>>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>>>>
>>>> maxItems
>>>>
>>>
>>> I thought it was not necessary with a single description: entry. But
>>> looking at the dt-schema source I fail to find any commit mentioning
>>> that.
>>
>> The purpose of maxItems is to constrain the number of GPIOs, so two
>> would be incorrect.
>>
> 
> I recall that with a single description entry then maxItems: 1 was
> assumed by the dt-schema validator, but I cannot find references to
> any commit, so I'll add it.
> 
>>>
>>>>> +
>>>>> +  reset-gpios:
>>>>> +    description:
>>>>> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
>>>>
>>>> maxItems
>>>>
>>>>> +
>>>>> +  avdd-supply:
>>>>> +    description: Analog circuit power. Typically 2.8V.
>>>>> +
>>>>> +  dvdd-supply:
>>>>> +    description: Digital circuit power. Typically 1.2V.
>>>>> +
>>>>> +  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:
>>>>> +            description: The sensor supports 1 or 2 data lanes operations.
>>>>> +            minItems: 1
>>>>> +            maxItems: 2
>>>>> +            items:
>>>>> +              maximum: 2
>>>>
>>>> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'
>>>
>>> No 0 is not allowed, but the data-lanes properties should accept any
>>> of the following combinations
>>>         <1>
>>>         <1 2>
>>>         <2 1>
>>>
>>> As the chip seems to support lane re-ordering.
>>>
>>> using enum would allow to between <1> or <2> if I got it right?
>>
>> Yeah, enum would be equivalent. I find it more readable, than min+max,
>> but it's not a strong preference.
>>
> 
> I don't think enum is equivalent, as it specifies a set of valid values
> a property can assume, but it does not support arrays.

It is equivalent, just has to be used in equivalent way.

> 
> https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.16.1.2.
> 
> enum
>   The value of this keyword MUST be an array. This array SHOULD have
>   at least one element. Elements in the array SHOULD be unique.
> 
>   An instance validates successfully against this keyword if its value
>   is equal to one of the elements in this keyword's array value.
> > In facts:

That's not good usage. See for example:
Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml


Best regards,
Krzysztof



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux