Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <324d9977-e8bc-b69f-ce8e-b5156035074e@linaro.org>
Date:   Sat, 7 May 2022 14:00:10 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH v2 1/3] media: dt-bindings: ov5675: document YAML binding
Content-Language: en-US
To:     Quentin Schulz <quentin.schulz@theobroma-systems.com>,
        Quentin Schulz <foss+kernel@0leil.net>
Cc:     shawnx.tu@intel.com, mchehab@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-media@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220504135543.59522-1-foss+kernel@0leil.net>
 <18d1032c-1fee-9de5-bd25-752ff9c39200@linaro.org>
 <889135d8-575e-3f95-4c65-ff3c40f64b05@theobroma-systems.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <889135d8-575e-3f95-4c65-ff3c40f64b05@theobroma-systems.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 06/05/2022 15:48, Quentin Schulz wrote:
>>> +  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?

No, because datasheet could call it "xvclk_clk_clk_clk" and it is not a
reason to use it in the bindings. All of these are clocks, so don't add
unnecessary suffixes. The same goes to interrupts (wake not wakeirq) or
DMA (tx not txdma).

> 
>>> +
>>> +  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 :/

Then please do not develop on an older kernel.

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

Bindings describe the hardware and they are used in different projects.
Let's say Linux implementation supports only 450 MHz, but other project
supports 450 and 900, so your bindings would be incorrect in such
case... IOW, bindings should not depend on the implementation.

What is more, the driver might get updated without updating the comments
in the bindings making them incorrect even for Linux.

In the past several bindings contained actual specifics of
implementation, but this is usually not the proper way.

There are clear issues with describing implementation in the bindings,
but what are the benefits?

Best regards,
Krzysztof