linux-kernel - Re: [PATCH v2 1/3] media: dt-bindings: ov5675: document YAML binding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <889135d8-575e-3f95-4c65-ff3c40f64b05@theobroma-systems.com>
Date:   Fri, 6 May 2022 15:48:00 +0200
From:   Quentin Schulz <quentin.schulz@...obroma-systems.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Quentin Schulz <foss+kernel@...il.net>
Cc:     shawnx.tu@...el.com, mchehab@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, linux-media@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] media: dt-bindings: ov5675: document YAML binding

Hi Krzysztof,

On 5/4/22 16:46, Krzysztof Kozlowski wrote:
> On 04/05/2022 15:55, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@...obroma-systems.com>
>>
>> This patch adds documentation of device tree in YAML schema for the
>> OV5675 CMOS image sensor from Omnivision.
>>
>> Cc: Quentin Schulz <foss+kernel@...il.net>
> 
> Don't Cc yourself in commits. This goes to the Git history, so
> assumption is that the "other you" knows that you sent it. :)
> 

It's a habit I've taken so that in the event I switch jobs, there's 
still an address where I can be reached.

But I forgot the kernel as a .mailmap :) so I can avoid doing it.

>> Signed-off-by: Quentin Schulz <quentin.schulz@...obroma-systems.com>
>> ---
>>
>> v2:
>>   - fixed incorrect id,
>>   - fixed device tree example by adding missing dt-bindings headers,
>>   - fixed device tree example by using vcc_1v2 for dvdd supply, as requested
>>   in datasheet,
>>
>>   .../bindings/media/i2c/ovti,ov5675.yaml       | 139 ++++++++++++++++++
>>   MAINTAINERS                                   |   1 +
>>   2 files changed, 140 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
>> new file mode 100644
>> index 000000000000..29df2f82c631
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
>> @@ -0,0 +1,139 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
>> +%YAML 1.2
>> +---
>> +$id: https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_schemas_media_i2c_ovti-2Cov5675.yaml-23&d=DwICaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=5gh6rUPawGp8Fw7sPXVsvOViSUSnEVGWWB9RK_CSQCeBjoEfunHaI0w2GiB0zjq_&s=W_iQiFCa2XM4k350eNawjzJR2ZTUuSx-VM4E1iuknz4&e=
>> +$schema: https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_meta-2Dschemas_core.yaml-23&d=DwICaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=5gh6rUPawGp8Fw7sPXVsvOViSUSnEVGWWB9RK_CSQCeBjoEfunHaI0w2GiB0zjq_&s=nrri3o0sr-6AopqJCaHZ94dUfOwS8r1V7ybfSwD7v2M&e=
>> +
>> +title: Omnivision OV5675 CMOS Sensor Device Tree Bindings
> 
> s/Device Tree Bindings//
> 
>> +
>> +maintainers:
>> +  - Quentin Schulz <quentin.schulz@...obroma-systems.com>
>> +
>> +description: |-
>> +  The Omnivision OV5675 is a high performance, 1/5-inch, 5 megapixel, CMOS
>> +  image sensor that delivers 2592x1944 at 30fps. It provides full-frame,
>> +  sub-sampled, and windowed 10-bit MIPI images in various formats via the
>> +  Serial Camera Control Bus (SCCB) interface. This chip is programmable
>> +  through I2C and two-wire SCCB. The sensor output is available via CSI-2
>> +  serial data output (up to 2-lane).
>> +
>> +properties:
>> +  compatible:
>> +    const: ovti,ov5675
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    description:
>> +      Input clock for the sensor.
>> +    items:
>> +      - const: xvclk
> 
> Just "xv" is preferred.
> 

The name of the clock in the datasheet is XVCLK though. Wouldn't it be 
confusing to describe HW by using names different from the datasheet?

>> +
>> +  clock-frequency:
>> +    description:
>> +      Frequency of the xvclk clock in Hertz.
>> +
>> +  dovdd-supply:
>> +    description:
>> +      Definition of the regulator used as interface power supply.
>> +
>> +  avdd-supply:
>> +    description:
>> +      Definition of the regulator used as analog power supply.
>> +
>> +  dvdd-supply:
>> +    description:
>> +      Definition of the regulator used as digital power supply.
>> +
>> +  reset-gpios:
>> +    description:
>> +      The phandle and specifier for the GPIO that controls sensor reset.
>> +      This corresponds to the hardware pin XSHUTDOWN which is physically
>> +      active low.
> 
> Needs maxItems
> 
>> +
>> +  port:
>> +    type: object
> 
> Open other bindings and compare how it is done there. This looks like
> /schemas/graph.yaml#/$defs/port-base
> 

Did that but used an old kernel as base :/

>> +    additionalProperties: false
>> +    description:
>> +      A node containing an output port node with an endpoint definition
>> +      as documented in
>> +      Documentation/devicetree/bindings/media/video-interfaces.txt
>> +
>> +    properties:
>> +      endpoint:
>> +        type: object
> 
> Missing ref
> 
>> +
>> +        properties:
>> +          data-lanes:
>> +            description: |-
> 
> No need for "|-"
> 
>> +              The driver only supports 2-lane operation.
> 
> Please remove references to driver. It's not part of hardware.
> 
>> +            items:
>> +              - const: 1
>> +              - const: 2
>> +
>> +          link-frequencies:
>> +            $ref: /schemas/types.yaml#/definitions/uint64-array
> 
> The ref should be already provided by video-interfaces.
> 
>> +            description:
>> +              Allowed data bus frequencies. 450000000Hz is supported by the driver.
> 
> Again, skip driver reference. However you need to describe the number of
> items.
> 

Currently, the driver is limited to 450 MHz link-freq and 2 data lanes, 
while the HW advertises: "The OV5675 supports a MIPI interface of up to 
2-lanes. The MIPI interface can be configured for 1/2-lane and each lane

is capable of a data transfer rate of up to 900 Mbps."

Was wondering what I am supposed to do in this situation as I see 
Documentation/devicetree/bindings/media/i2c/ov8856.yaml mentioning 
driver limitations in the dt-bindings.

Cheers,
Quentin