Hi Johan, Thanks for your review. On 4/2/20 8:35 AM, Johan Jonker wrote: > Hi Helen, > >> # SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> %YAML 1.2 >> --- >> $id: http://devicetree.org/schemas/media/rockchip-isp1.yaml# >> $schema: http://devicetree.org/meta-schemas/core.yaml# >> > >> title: Rockchip SoC Image Signal Processing unit v1 > > Where do we need 'v1' for? Is there a 'v2'? ISPv1 is the Rockchip's name for the IP block. > >> >> maintainers: >> - Helen Koike <helen.koike@xxxxxxxxxxxxx> >> >> description: | >> Rockchip ISP1 is the Camera interface for the Rockchip series of SoCs >> which contains image processing, scaling, and compression functions. >> >> properties: >> compatible: >> const: rockchip,rk3399-cif-isp >> >> reg: >> maxItems: 1 >> >> interrupts: >> maxItems: 1 >> >> iommus: >> maxItems: 1 >> >> power-domains: >> maxItems: 1 >> >> phys: >> maxItems: 1 >> description: phandle for the PHY port >> >> phy-names: >> const: dphy >> >> clocks: >> items: >> - description: ISP clock >> - description: ISP AXI clock clock >> - description: ISP AXI clock wrapper clock >> - description: ISP AHB clock clock >> - description: ISP AHB wrapper clock >> >> clock-names: >> items: >> - const: clk_isp >> - const: aclk_isp >> - const: aclk_isp_wrap >> - const: hclk_isp >> - const: hclk_isp_wrap >> >> # See ./video-interfaces.txt for details >> ports: >> type: object >> additionalProperties: false >> >> properties: >> "#address-cells": >> const: 1 >> >> "#size-cells": >> const: 0 >> >> port@0: >> type: object >> description: connection point for sensors at MIPI-DPHY RX0 > >> additionalProperties: false > > Nothing required here? I was thinking that if there is no endpoint, then nothing is required. But if there is, then #address-cells, #size-cells and reg are. I guess I can just add them as required. I'll add it in the patchseries. > >> >> properties: >> "#address-cells": >> const: 1 >> >> "#size-cells": >> const: 0 >> >> reg: >> const: 0 >> >> patternProperties: >> endpoint: >> type: object >> additionalProperties: false >> >> properties: >> reg: >> maxItems: 1 >> >> data-lanes: >> minItems: 1 >> maxItems: 4 >> >> remote-endpoint: true >> >> required: > >> - port@0 > > The use of '@0' makes "#address-cells" and "#size-cells" also a requirement. > > - "#address-cells" > - "#size-cells" Ok, I'll add it. > >> >> required: >> - compatible > > How about 'reg'? > > - reg ack, I'll add another patch in the series fixing this. > >> - interrupts >> - clocks >> - clock-names >> - power-domains >> - iommus >> - phys >> - phy-names >> - ports >> >> additionalProperties: false >> >> examples: >> - | >> >> #include <dt-bindings/clock/rk3399-cru.h> >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> #include <dt-bindings/power/rk3399-power.h> >> >> parent0: parent@0 { >> #address-cells = <2>; >> #size-cells = <2>; >> >> isp0: isp0@ff910000 { >> compatible = "rockchip,rk3399-cif-isp"; >> reg = <0x0 0xff910000 0x0 0x4000>; >> interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>; >> clocks = <&cru SCLK_ISP0>, >> <&cru ACLK_ISP0>, <&cru ACLK_ISP0_WRAPPER>, >> <&cru HCLK_ISP0>, <&cru HCLK_ISP0_WRAPPER>; >> clock-names = "clk_isp", >> "aclk_isp", "aclk_isp_wrap", >> "hclk_isp", "hclk_isp_wrap"; >> power-domains = <&power RK3399_PD_ISP0>; >> iommus = <&isp0_mmu>; >> phys = <&dphy>; >> phy-names = "dphy"; >> >> ports { >> #address-cells = <1>; >> #size-cells = <0>; >> >> port@0 { >> #address-cells = <1>; >> #size-cells = <0>; >> reg = <0>; >> >> mipi_in_wcam: endpoint@0 { >> reg = <0>; >> remote-endpoint = <&wcam_out>; >> data-lanes = <1 2>; >> }; >> >> mipi_in_ucam: endpoint@1 { >> reg = <1>; >> remote-endpoint = <&ucam_out>; >> data-lanes = <1>; >> }; >> }; >> }; >> }; >> > >> i2c7: i2c@ff160000 { >> clock-frequency = <400000>; >> #address-cells = <1>; >> #size-cells = <0>; > > Incomplete example. > From i2c-rk3x.yaml: > > required: > - compatible > - reg > - interrupts > - clocks > - clock-names The idea was to exemplify how to connect to the sensor nodes below. But I don't see a problem adding a complete i2c example, I'll add it. Thanks Helen > >> >> wcam: camera@36 { >> compatible = "ovti,ov5695"; >> reg = <0x36>; >> >> port { >> wcam_out: endpoint { >> remote-endpoint = <&mipi_in_wcam>; >> data-lanes = <1 2>; >> }; >> }; >> }; >> >> ucam: camera@3c { >> compatible = "ovti,ov2685"; >> reg = <0x3c>; >> >> port { >> ucam_out: endpoint { >> remote-endpoint = <&mipi_in_ucam>; >> data-lanes = <1>; >> }; >> }; >> }; >> }; >> };
