| 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
| ||
|
Message-ID: <431375a9-4f62-419c-9ad7-90f3cf892181@wolfvision.net>
Date: Fri, 17 Nov 2023 13:15:52 +0100
From: Michael Riesch <michael.riesch@...fvision.net>
To: Mehdi Djait <mehdi.djait@...tlin.com>, mchehab@...nel.org,
heiko@...ech.de, hverkuil-cisco@...all.nl,
krzysztof.kozlowski+dt@...aro.org, robh+dt@...nel.org,
conor+dt@...nel.org
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
alexandre.belloni@...tlin.com, maxime.chevallier@...tlin.com,
paul.kocialkowski@...tlin.com
Subject: Re: [PATCH v11 1/3] media: dt-bindings: media: add bindings for
Rockchip CIF
Hi Mehdi,
On 11/16/23 12:04, Mehdi Djait wrote:
> Add a documentation for the Rockchip Camera Interface binding.
>
> Signed-off-by: Mehdi Djait <mehdi.djait@...tlin.com>
> ---
> .../bindings/media/rockchip,px30-vip.yaml | 173 ++++++++++++++++++
> 1 file changed, 173 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> new file mode 100644
> index 000000000000..580ed654000c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> @@ -0,0 +1,173 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/rockchip,px30-vip.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip CIF Camera Interface
May I suggest "Rockchip Camera Interface (CIF)"?
> +
> +maintainers:
> + - Mehdi Djait <mehdi.djait@...tlin.com>
> +
> +description:
> + CIF is a camera interface present on some rockchip SoCs. It receives the data
s/rockchip/Rockchip
> + from Camera sensor or CCIR656 encoder and transfers it into system main memory
> + by AXI bus.
> +
> +properties:
> + compatible:
> + const: rockchip,px30-vip
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: ACLK
> + - description: HCLK
> + - description: PCLK
> +
> + clock-names:
> + items:
> + - const: aclk
> + - const: hclk
> + - const: pclk
> +
> + resets:
> + items:
> + - description: AXI
> + - description: AHB
> + - description: PCLK IN
> +
> + reset-names:
> + items:
> + - const: axi
> + - const: ahb
> + - const: pclkin
> +
> + power-domains:
> + maxItems: 1
> +
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
> +
> + properties:
> + port@0:
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + unevaluatedProperties: false
> + description: input port on the parallel interface
In more recent Rockchip SoCs this seems to be described as "DVP
interface (digital parallel input)". Maybe we could use this description
here as well?
> +
> + properties:
> + endpoint:
> + $ref: video-interfaces.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + bus-type:
> + enum: [5, 6]
Not sure whether this is possible, but if we could use the
MEDIA_BUS_TYPE_PARALLEL and MEDIA_BUS_TYPE_BT656 defines here it would
be way more descriptive.
> +
> + required:
> + - bus-type
> +
> + required:
> + - port@0
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/px30-cru.h>
> + #include <dt-bindings/display/sdtv-standards.h>
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/media/video-interfaces.h>
> + #include <dt-bindings/power/px30-power.h>
> +
> + parent {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + video-capture@...90000 {
> + compatible = "rockchip,px30-vip";
> + reg = <0x0 0xff490000 0x0 0x200>;
> + interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru ACLK_CIF>, <&cru HCLK_CIF>, <&cru PCLK_CIF>;
> + clock-names = "aclk", "hclk", "pclk";
> + resets = <&cru SRST_CIF_A>, <&cru SRST_CIF_H>, <&cru SRST_CIF_PCLKIN>;
> + reset-names = "axi", "ahb", "pclkin";
> + power-domains = <&power PX30_PD_VI>;
Sort alphabetically: power-domains before resets.
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + cif_in: endpoint {
> + remote-endpoint = <&tw9900_out>;
> + bus-type = <MEDIA_BUS_TYPE_BT656>;
> + };
> + };
> + };
> + };
> +
> + composite_connector {
> + compatible = "composite-video-connector";
> + label = "tv";
> + sdtv-standards = <(SDTV_STD_PAL | SDTV_STD_NTSC)>;
> +
> + port {
> + composite_to_tw9900: endpoint {
> + remote-endpoint = <&tw9900_to_composite>;
> + };
> + };
> + };
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + video-decoder@44 {
> + compatible = "techwell,tw9900";
> + reg = <0x44>;
> +
> + vdd-supply = <&tw9900_supply>;
> + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
This goes before vdd-supply (alphabetical sorting). No need for blank
lines between the properties.
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reg = <0>;
I think reg property goes first, then the others. No blank lines between
properties, but one blank line between properties and nodes.
> + tw9900_to_composite: endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&composite_to_tw9900>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
Same here.
> + tw9900_out: endpoint {
> + remote-endpoint = <&cif_in>;
> + };
> + };
> + };
> + };
> + };
> + };
> +...
With the inline comments addressed,
Reviewed-by: Michael Riesch <michael.riesch@...fvision.net>
Thanks for your efforts!
Best regards,
Michael
Powered by blists - more mailing lists