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

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

 



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

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

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

> 
> as the data-lane property is defined in video-interfaces.yaml
> 
>   data-lanes:
>     $ref: /schemas/types.yaml#/definitions/uint32-array
>     minItems: 1
>     maxItems: 8
>     items:
>       # Assume up to 9 physical lane indices
>       maximum: 8
>     description:
>       An array of physical data lane indexes. Position of an entry determines
>       the logical lane number, while the value of an entry indicates physical
>       lane, e.g. for 2-lane MIPI CSI-2 bus we could have "data-lanes = <1 2>;",
>       assuming the clock lane is on hardware lane 0. If the hardware does not
>       support lane reordering, monotonically incremented values shall be used
>       from 0 or 1 onwards, depending on whether or not there is also a clock
>       lane. This property is valid for serial busses only (e.g. MIPI CSI-2).
> 
> I did the same but restricted the max number of items to 2, and the
> maximum value to 2 as well

Makes sense, but you should also restrict the minimum (so not 0). enum
solves this.

> 
>>
>> no clock-lanes?
>>
> 
> clock lane is fixed on lane #0 afaict

ok


Best regards,
Krzysztof



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux