On 15/03/2022 13:47, Sakari Ailus wrote: > On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote: >> On 15/03/2022 08:59, Sakari Ailus wrote: >>> Hi Krzysztof, Jacopo, >>> >>> On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote: >>>> On 14/03/2022 17:27, Jacopo Mondi wrote: >>>>> Provide the bindings documentation for Omnivision OV5670 image sensor. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx> >>>>> >>>>> --- >>>>> v1->v2 (comments from Krzysztof) >>>>> >>>>> - Rename to include manufacturer name >>>>> - Add entry to MAINTAINERS >>>>> - Add maxItems: to -gpios properties >>>>> - Use common clock properties >>>>> - Use enum: [1, 2] for data lanes >>>>> - Fix whitespace issue in example >>>>> --- >>>>> >>>>> .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ >>>>> MAINTAINERS | 1 + >>>>> 2 files changed, 100 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml >>>>> new file mode 100644 >>>>> index 000000000000..73cf72203f17 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml >>>>> @@ -0,0 +1,99 @@ >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>> +%YAML 1.2 >>>>> +--- >>>>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml# >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>> + >>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor >>>>> + >>>>> +maintainers: >>>>> + - Jacopo Mondi <jacopo@xxxxxxxxxx> >>>>> + >>>>> +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 >>>>> + >>>>> + assigned-clocks: true >>>>> + assigned-clock-parents: true >>>>> + assigned-clock-rates: true >>>> >>>> You should not need these. These are coming with schema. You can add >>>> these to example schema below and double-check. >>> >>> They should probably be required actually. >> >> Why required? The hardware can work with different clocks, get their >> rate and configure internal PLLs/clocks to new value. Having it required >> might have sense for current implementation of driver but this is >> independent of bindings. Bindings do not describe driver, but hardware. > > We've had this discussion before and the result of that was this (see > "Handling clocks"): > > Documentation/driver-api/media/camera-sensor.rst ... and the "Devicetree" chapter explains usage of assigned-clock-xxx. None of these explain why it should be in the bindings. The chapter you referred explicitly mentions that "clock tree is generally configured by the driver", so it has nothing to do with the bindings. Bindings describe hardware, not Linux driver implementation. The hardware can work with multiple frequencies and can support changing these frequencies, probably combined with some reset sequence. Therefore hardware does not require the clock frequency to be always fixed and defined. Driver requires assigned-clock-xxx, not bindings. Do not put Linux implementation specifics into the bindings. Best regards, Krzysztof
