| 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
| ||
|
Date: Tue, 7 Mar 2023 14:20:14 +0800
From: Jack Zhu <jack.zhu@...rfivetech.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
"Mauro Carvalho Chehab" <mchehab@...nel.org>,
Robert Foss <rfoss@...nel.org>,
Todor Tomov <todor.too@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Maxime Ripard <mripard@...nel.org>,
Philipp Zabel <p.zabel@...gutronix.de>
CC: <linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>, <changhuang.liang@...rfivetech.com>
Subject: Re: [PATCH v1 01/11] media: dt-bindings: starfive,jh7110-camss: add
binding document
On 2023/3/3 16:34, Krzysztof Kozlowski wrote:
> On 02/03/2023 10:19, jack.zhu wrote:
>> Add DT binding document for Starfive Camera subsystem driver
>>
>> Signed-off-by: jack.zhu <jack.zhu@...rfivetech.com>
>> ---
>> .../bindings/media/starfive,jh7110-camss.yaml | 150 ++++++++++++++++++
>> 1 file changed, 150 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml b/Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
>> new file mode 100644
>> index 000000000000..9a34944ca0ab
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
>> @@ -0,0 +1,150 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/media/starfive,jh7110-camss.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>
> Drop quotes from both.
OK, I will fix it in V2 version.
>
>> +
>> +title: Starfive SoC CAMSS ISP
>> +
>> +maintainers:
>> + - Jack Zhu <jack.zhu@...rfivetech.com>
>> + - Changhuang Liang <changhuang.liang@...rfivetech.com>
>> +
>> +description: |
>
> No need for '|'
OK, I will fix it.
>
>> + The Starfive CAMSS ISP is a Camera interface for Starfive JH7110 SoC.It
>> + consists of a VIN controller(Video In Controller, a top-level control until)
>> + and a ISP.
>
> "an ISP", I think
I will revise it.
>
>> +
>> +properties:
>> + compatible:
>> + const: starfive,jh7110-camss
>> +
>> + reg:
>> + minItems: 2
>
> Drop minItems, no need.
OK, I will drop it.
>
>> + maxItems: 2
>> +
>> + reg-names:
>> + items:
>> + - const: syscon
>> + - const: isp
>> +
>> + clocks:
>> + minItems: 7
>
> Drop mintems
I will drop it.
>
>> + maxItems: 7
>> +
>> + clock-names:
>> + items:
>> + - const: clk_apb_func
>> + - const: clk_wrapper_clk_c
>> + - const: clk_dvp_inv
>> + - const: clk_axiwr
>> + - const: clk_mipi_rx0_pxl
>> + - const: clk_ispcore_2x
>> + - const: clk_isp_axi
>
> Drop "clk" prefix
I will drop it.
>
>> +
>> + resets:
>> + minItems: 6
>
> Drop
I will drop it.
>
>> + maxItems: 6
>> +
>> + reset-names:
>> + items:
>> + - const: rst_wrapper_p
>
> Drop rst prefix
OK, will fix it.
>
>> + - const: rst_wrapper_c
>> + - const: rst_axird
>> + - const: rst_axiwr
>> + - const: rst_isp_top_n
>> + - const: rst_isp_top_axi
>> +
>> + power-domains:
>> + items:
>> + - description: JH7110 PD ISP - ISP Power Domain Switch Controller.
>
> Drop redundant pieces, e.g. "PD ISP"
OK, will fix it.
>
>> +
>> + interrupts:
>> + minItems: 4
>
> Drop
OK, will drop it.
>
>> + maxItems: 4
>> +
>> + ports:
>> + $ref: /schemas/graph.yaml#/properties/ports
>> +
>> + properties:
>> + port@1:
>
> And what about port@0?
port@0 is reserved for DVP sensor, although it is not supported yet.
>
>> + $ref: /schemas/graph.yaml#/$defs/port-base
>> + unevaluatedProperties: false
>> + description:
>> + Input port for receiving CSI data.
>> +
>> + properties:
>> + endpoint@1:
>
> Hm, do you have more than one endpoint in this port? Why unit address?
OK, I will change it to "endpoint:"
>
>> + $ref: video-interfaces.yaml#
>> + unevaluatedProperties: false
>> +
>> + required:
>> + - port@1
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - reg-names
>> + - clocks
>> + - clock-names
>> + - resets
>> + - reset-names
>> + - power-domains
>> + - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> +
>
> Drop blank line
I will drop it.
>
>> + stfcamss: camss@...40000 {
>
> isp@
I will alter it.
>
>> + compatible = "starfive,jh7110-camss";
>> + reg = <0x19840000 0x10000>,
>> + <0x19870000 0x30000>;
>
> All this looks misaligned
>> + reg-names = "syscon", "isp";
>> + clocks = <&ispcrg 0>,
>> + <&ispcrg 13>,
>
> Looks even worse...
I will fix it.
>
>> + <&ispcrg 2>,
>> + <&ispcrg 12>,
>> + <&ispcrg 1>,
>> + <&syscrg 51>,
>> + <&syscrg 52>;
>> + clock-names = "clk_apb_func",
>> + "clk_wrapper_clk_c",
>> + "clk_dvp_inv",
>> + "clk_axiwr",
>> + "clk_mipi_rx0_pxl",
>> + "clk_ispcore_2x",
>> + "clk_isp_axi";
>> + resets = <&ispcrg 0>,
>> + <&ispcrg 1>,
>> + <&ispcrg 10>,
>> + <&ispcrg 11>,
>> + <&syscrg 41>,
>> + <&syscrg 42>;
>> + reset-names = "rst_wrapper_p",
>> + "rst_wrapper_c",
>> + "rst_axird",
>> + "rst_axiwr",
>> + "rst_isp_top_n",
>> + "rst_isp_top_axi";
>> + power-domains = <&pwrc 5>;
>> + interrupts = <92>, <87>, <88>, <90>;
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + port@1 {
>> + reg = <1>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + vin_from_csi2rx: endpoint@1 {
>> + reg = <1>;
>> + remote-endpoint = <&csi2rx_to_vin>;
>> + };
>> + };
>> + };
>> + };
>
> Best regards,
> Krzysztof
>
Powered by blists - more mailing lists