Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 30 Apr 2020 14:01:32 +0800
From:   Xin Ji <xji@analogixsemi.com>
To:     Sam Ravnborg <sam@ravnborg.org>
Cc:     devel@driverdev.osuosl.org,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Andrzej Hajda <a.hajda@samsung.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@siol.net>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        Pi-Hsun Shih <pihsun@chromium.org>,
        Sheng Pan <span@analogixsemi.com>
Subject: Re: [PATCH v8 1/2] dt-bindings: drm/bridge: anx7625: MIPI to DP
 transmitter binding
Message-ID: <20200430060132.GB6645@xin-VirtualBox>
References: <cover.1587880280.git.xji@analogixsemi.com>
 <67ccead807b7d0a50df479cab2c9d325041224bc.1587880280.git.xji@analogixsemi.com>
 <20200427184909.GA15880@ravnborg.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200427184909.GA15880@ravnborg.org>
User-Agent: Mutt/1.5.24 (2015-08-30)
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from xin-VirtualBox (114.247.245.254) by HK2PR0302CA0008.apcprd03.prod.outlook.com (2603:1096:202::18) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA) id 15.20.2979.14 via Frontend Transport; Thu, 30 Apr 2020 06:01:39 +0000
X-Originating-IP: [114.247.245.254]
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 2dd22024-81d2-4e30-5a7f-08d7eccbf34c
X-MS-TrafficTypeDiagnostic: BY5PR04MB6996:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <BY5PR04MB69961C26C836FCE568FD4156C7AA0@BY5PR04MB6996.namprd04.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:9508;
X-Forefront-PRVS: 0389EDA07F
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: p1wKTUCAorEQzSmWXb16+TfXpi7ak4u6Uo9yDI4N/9w50fl9MTf/bALCd8xUOq7I2uo8r82yoC18GzEcxS2auKQXtyCCQZBcI2+ln159Rg0UDz+Fdz+WrpAIQIT8apCnJx0XQ3nFLYS9EnQrI8lr9mJdAGLz4vzNa44l5kNRwUxfZeGhktKxP39nc3zm48TGN+Skj00DHPv+DD9ESitvnTfMkptc2CjJTbPCiORLYp8uHPoG9I6NUiYQUYjpuFwCroX+olTmsvXfZAN62wcTiPpANZSZ3WYXbq9dYP3wjnL0mOXrm5roulmad9irsHWXW+Pax4eq9qtHAPQKJtGLjNA/hFe2z+EILL1seMm13p4Puqm1QL+lFnSsvkJ1SNBF/OC3bNc7fhcdS+84e3be2lw2Tq4AjN9cI0yBEKgyvVjnCQS4raYWjgJCr44Ctqkg+GC3UdDMIk/zc/V+huRI+5C1A0Oj/tx1YA2qkl2dEQBGS2Nj9YONValHbHFn8lE2CWWlD6fIxY0fJcLP75AIqA==
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BY5PR04MB6739.namprd04.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(136003)(366004)(346002)(39840400004)(396003)(376002)(1076003)(66556008)(4326008)(66476007)(186003)(66946007)(55016002)(7416002)(16526019)(26005)(956004)(9686003)(2906002)(6666004)(54906003)(6916009)(8936002)(6496006)(86362001)(33656002)(8676002)(107886003)(33716001)(478600001)(5660300002)(52116002)(316002);DIR:OUT;SFP:1102;
X-MS-Exchange-AntiSpam-MessageData: nSvanHTZKE2srxNW9ynwr+/C978IjFo8Euit5SYaVmg17h3RVK+ylFxz2Dl/1v5n56/fGzlzaUQQap/KXcY799KAUlNhi3tpziwAnVk2QG0l9T8oSjjBiXQrt/M5QmqlZbzFcDkAKLJr7XRXmdrSBHXd6Lu/I3J6ly2dx5+cxsUl8slI8IZKPr4B4cfCOTs/lXpHcp4ym/kgtTlgd/dufyTOwMAAMg4/YeIVk5/nJQZazYmpPw3PN8zuJ2sUso93LGVp6KnPWSXLl9yhcbRra7LBTpjbuP3Usc9s+AJkOCSSdaDZ8BDCfoZXxB/2y41X7HW+jtlGGhGDBCDhB+M8twhV4FwWDvjxeTXHRs5o4CHbqV/9p7JKN3ackUJifTCj8KHeMPQSda4KT75p571vdk4iA2Tj2kEvDKHdMXwd/KUx/yuDn1ZR1bvUZ/N85YlUF9Y8+zj96EvFYdfpvOSSF2UB38vqzkbXcD0BQCPoMWxW+Xa4z5OeQxkDWuPVg77zfbJIrrY4/zPD+M5TIIN/duDj0gju3JL7BwcEjcWoV6JXDom+t9NBHZUvajVcTXTEznPlZDC6j/a+fYgf6YbWGAmRxNPn4Bp4xJT2hJL2M3s1Xqpd9XXAmzeGqGtBDSF+w+xMyPxfcHNPLgqRxjYgjv1gCKykmDwT+jUwVI2GTTX98sYjAwk/RUPj75jK8mq7TdhKZBZCkpKdqhDl+uHpphfb34PppQceaL9oQx4V09YFQMLXZn+iZROm9AP3v1G9FZQq3tePhvq/nYvdR0NUs+lWSqDkUlZKPL0uUt25FyqvEUFAJy5B2tulGUZUKjH8
X-OriginatorOrg: analogixsemi.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2dd22024-81d2-4e30-5a7f-08d7eccbf34c
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Apr 2020 06:01:40.6422
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: b099b0b4-f26c-4cf5-9a0f-d5be9acab205
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: CAVPU8Me5UJF7/8kK/IXKTOCyZHLJE9JYvbRUvi2uXMw8LSxvb/454Gqo3Q4nBrHN24imSUQ5bSHfiuXhRpwZA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR04MB6996
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Sam, thanks for your comments.

On Mon, Apr 27, 2020 at 08:49:09PM +0200, Sam Ravnborg wrote:
> Hi Xin Ji
> 
> On Mon, Apr 27, 2020 at 02:17:46PM +0800, Xin Ji wrote:
> > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed
> > for portable device. It converts MIPI to DisplayPort 1.3 4K.
> 
> Thanks for providing this binding.
> When you re-submit please also send to devicetree@vger.kernel.org.
OK
> 
> Running the binding through dt_binding_check gives me:
> 
> /home/sam/drm/linux.git/Documentation/devicetree/bindings/display/bridge/anx7625.yaml: ignoring, error in schema:
> warning: no schema found in file: /home/sam/drm/linux.git/Documentation/devicetree/bindings/display/bridge/anx7625.yaml
> make[2]: *** [/home/sam/drm/linux.git/Documentation/devicetree/bindings/Makefile:42: Documentation/devicetree/bindings/processed-schema.yaml] Error 255
> make[2]: *** Waiting for unfinished jobs....
> /home/sam/drm/linux.git/Documentation/devicetree/bindings/display/bridge/anx7625.yaml: Additional properties are not allowed ('example' was unexpected)
> /home/sam/drm/linux.git/Documentation/devicetree/bindings/display/bridge/anx7625.yaml: Additional properties are not allowed ('example' was unexpected)
> 
> See writing-schemas.rst for instruction installing tools etc.
OK
> 
> > 
> > You can add support to your board with binding.
> > 
> > Example:
> > 	anx7625_bridge: encoder@58 {
> > 		compatible = "analogix,anx7625";
> > 		reg = <0x58>;
> > 		status = "okay";
> > 		panel-flags = <1>;
> > 		enable-gpios = <&pio 45 GPIO_ACTIVE_HIGH>;
> > 		reset-gpios = <&pio 73 GPIO_ACTIVE_HIGH>;
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		port@0 {
> > 		  reg = <0>;
> > 		  anx_1_in: endpoint {
> > 		    remote-endpoint = <&mipi_dsi>;
> > 		  };
> > 		};
> > 
> > 		port@2 {
> > 		  reg = <2>;
> > 		  anx_1_out: endpoint {
> > 		    remote-endpoint = <&panel_in>;
> > 		  };
> > 		};
> > 	};
> > 
> > Signed-off-by: Xin Ji <xji@analogixsemi.com>
> > ---
> >  .../bindings/display/bridge/anx7625.yaml           | 91 ++++++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/anx7625.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/anx7625.yaml
> > new file mode 100644
> > index 0000000..1149ebb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/anx7625.yaml
> 
> Name the file "analogix,anx7625.yaml".
> (We should rename anx6345.yaml, so others do not omit the company name)
OK
> 
> > @@ -0,0 +1,91 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2019 Analogix Semiconductor, Inc.
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/display/bridge/anx7625.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Analogix ANX7625 SlimPort (4K Mobile HD Transmitter)
> > +
> > +maintainers:
> > +  - Xin Ji <xji@analogixsemi.com>
> > +
> > +description: |
> > +  The ANX7625 is an ultra-low power 4K Mobile HD Transmitter
> > +  designed for portable devices.
> > +
> > +properties:
> > +  "#address-cells": true
> > +  "#size-cells": true
> > +
> > +  compatible:
> > +    items:
> > +      - const: analogix,anx7625
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  panel-flags:
> > +    description: indicate the panel is internal or external.
> > +    maxItems: 1
> This property hint at something needs to be modelled in a different way.
> I do not recall other bindings need similar info.
I'll change it to vendor,int-property
> 
> > +
> > +  interrupts:
> > +    maxItems: 1
> A description would be nice.
OK
> 
> > +
> > +  enable-gpios:
> > +    description: used for power on chip control, POWER_EN pin D2.
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description: used for reset chip control, RESET_N pin B7.
> > +    maxItems: 1
> > +
> > +  port@0:
> > +    type: object
> > +    description:
> > +      A port node pointing to MIPI DSI host port node.
> > +
> > +  port@1:
> > +    type: object
> > +    description:
> > +      A port node pointing to MIPI DPI host port node.
> Maybe explain how it differs from port@0 and why it is optional.
It seems this port is not need any more, I'll remove this port.
> 
> > +
> > +  port@2:
> > +    type: object
> > +    description:
> > +      A port node pointing to panel port node.
> Unless there is a good reason not to then please use a ports node, like
> you see in almost (all?) other bridge bindings.
I'll use ports node.
> 
> > +
> > +required:
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - compatible
> > +  - reg
> > +  - port@0
> > +  - port@2
> 
> additionalProperties: false??
OK
> 
> 
> > +
> > +example:
> It must be named "examples:" - this is what dt_binding_check complains
> about.
OK
> 
> > +  - |
> > +    anx7625_bridge: encoder@58 {
> > +        compatible = "analogix,anx7625";
> > +        reg = <0x58>;
> > +        status = "okay";
> No status in examples.
OK
> 
> > +        panel-flags = <1>;
> > +        enable-gpios = <&pio 45 GPIO_ACTIVE_HIGH>;
> You need to include a header file to pull in GPIO_ACTIVE_HIGH - see what
> other bindings do.
> > +        reset-gpios = <&pio 73 GPIO_ACTIVE_HIGH>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 {
> > +          reg = <0>;
> Be consistent with indent. My personal preference is 4 spaces.
OK
> 
> > +          anx_1_in: endpoint {
> > +            remote-endpoint = <&mipi_dsi>;
> > +          };
> > +        };
> > +
> > +        port@2 {
> > +          reg = <2>;
> > +          anx_1_out: endpoint {
> > +            remote-endpoint = <&panel_in>;
> > +          };
> > +        };
> > +    };
> 
> Lot's of small comments. But if this is your first binding then this is
> expected.
> Looks forward for v2.
> 
> 	Sam