From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D2D27C433E0 for ; Thu, 14 May 2020 09:36:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B3B0A206A5 for ; Thu, 14 May 2020 09:36:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725999AbgENJgY (ORCPT ); Thu, 14 May 2020 05:36:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46166 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1725935AbgENJgY (ORCPT ); Thu, 14 May 2020 05:36:24 -0400 Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BD6C7C061A0C for ; Thu, 14 May 2020 02:36:23 -0700 (PDT) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: rcn) with ESMTPSA id 0244F2A06B8 Date: Thu, 14 May 2020 11:36:17 +0200 From: Ricardo =?utf-8?Q?Ca=C3=B1uelo?= To: Laurent Pinchart Cc: kernel@collabora.com, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, geert+renesas@glider.be, robh+dt@kernel.org, xuwei5@hisilicon.com Subject: Re: [PATCH v2 6/6] dt-bindings: drm: bridge: adi,adv7511.txt: convert to yaml Message-ID: <20200514093617.dwhmqaasc3z5ixy6@rcn-XPS-13-9360> Mail-Followup-To: Laurent Pinchart , kernel@collabora.com, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, geert+renesas@glider.be, robh+dt@kernel.org, xuwei5@hisilicon.com References: <20200511110611.3142-1-ricardo.canuelo@collabora.com> <20200511110611.3142-7-ricardo.canuelo@collabora.com> <20200514015412.GF7425@pendragon.ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200514015412.GF7425@pendragon.ideasonboard.com> User-Agent: NeoMutt/20171215 Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org Hi Laurent, thanks for the thorough review. Some comments below: On jue 14-05-2020 04:54:12, Laurent Pinchart wrote: > > +description: | > > + The ADV7511, ADV7511W and ADV7513 are HDMI audio and video > > + transmitters compatible with HDMI 1.4 and DVI 1.0. They support color > > + space conversion, S/PDIF, CEC and HDCP. They support RGB input > > + interface. > > I would write the last sentence as "The transmitter input is parallel > RGB or YUV data." as YUV is also supported. Ok. > > + adi,input-colorspace: > > + description: Input color space. > > + allOf: > > + - $ref: /schemas/types.yaml#/definitions/string > > + - enum: [ rgb, yuv422, yuv444 ] > > Isn't string implied ? Can't you write > > adi,input-colorspace: > description: Input color space. > enum: [ rgb, yuv422, yuv444 ] example-schema.yaml says that Vendor specific properties have slightly different schema requirements than common properties. They must have at least a type definition and 'description'. However, dt_binding_check doesn't seem to enforce this rule for string properties, and I saw a couple of vendor-specific string properties in other bindings that don't define the type either, so I'm going to follow your suggestion but only for string properties, the rest need a type definition. I noticed I can remove the "allOf" keywords from these too. > > + adi,embedded-sync: > > + description: > > + The input uses synchronization signals embedded in the data > > + stream (similar to BT.656). Defaults to 0 (separate H/V > > + synchronization signals). > > + allOf: > > + - $ref: /schemas/types.yaml#/definitions/uint32 > > + - enum: [ 0, 1 ] > > + - default: 0 > > This be a boolean property (it is read as a bool by the driver, the > property being absent means false, the property being present means > true). You're completely right. > > + ports: > > + description: > > + The ADV7511(W)/13 has two video ports and one audio port. This node > > + models their connections as documented in > > + Documentation/devicetree/bindings/media/video-interfaces.txt > > + Documentation/devicetree/bindings/graph.txt > > + type: object > > + properties: > > + port@0: > > + description: Video port for the RGB, YUV or DSI input. > > s/RGB, YUV or DSI/RGB or YUV/ Ok. > > +if: > > + not: > > + properties: > > + adi,input-colorspace: > > + contains: > > + enum: [ rgb, yuv444 ] > > + adi,input-clock: > > + contains: > > + const: 1x > > As both properties take a single value, I think you can omit > "contains:". I think it's required here, removing it makes the test fail. > > +required: > > + - compatible > > + - reg > > + - ports > > + - adi,input-depth > > + - adi,input-colorspace > > + - adi,input-clock > > Shouldn't the power supplies be required ? Good question. The original binding is not completely clear on this. There's a "Required properties" section at the top that doesn't include the supplies, the supplies block for the ADV7511/11w/13 looks like a gray area in that regard, while the same block for ADV7533/35 is more explicit in that they are required, and they are not listed in the "Optional properties" section. However, most of the DTs that use this binding don't define any supplies. And AFAICT from looking at the code, regulator_get() will use a dummy regulator and show a warning for every expected regulator that's not defined in the DT. If we want to be more strict and require the definition of all the supplies, there will be many more DTs changes in the series, and I'm not sure I'll be able to do that in a reasonable amount of time. I'm looking at them and it's not always clear which regulators to use or if they are even defined. > > +description: | > > + The ADV7533 and ADV7535 are HDMI audio and video transmitters > > + compatible with HDMI 1.4 and DVI 1.0. They support color space > > + conversion, S/PDIF, CEC and HDCP. They support DSI for input pixels. > > I would write the last sentence as "The transmitter input is MIPI DSI.". > > ... > > > + Disables the interal timing generator. The chip will rely on the > > s/interal/internal/ > > > + sync signals in the DSI data lanes, rather than generate its own > > s/generate/generating/ > > ... > > > + properties: > > + port@0: > > + description: > > + Video port for the RGB, YUV or DSI input. The remote endpoint > > s/RGB, YUV or // Ok. > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + adv7511w_in: endpoint { > > + remote-endpoint = <&dpi_out>; > > + }; > > + }; > > + > > + port@1 { > > + reg = <1>; > > + adv7511_out: endpoint { > > + remote-endpoint = <&hdmi_connector_in>; > > + }; > > + }; > > The name of the two endpoints doesn't match the adv7533. The remote > endpoint phandle for port 0 should have dsi in its name. Oops, thanks for catching these. Cheers, Ricardo From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,UNPARSEABLE_RELAY,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A0232C433DF for ; Thu, 14 May 2020 09:36:28 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 723B920671 for ; Thu, 14 May 2020 09:36:28 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Yi7srqmK" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 723B920671 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=collabora.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=zlGPe1Q85/cOQrr5F8syrz5OuZzwKhWQuY6eSQyhSCQ=; b=Yi7srqmKj1s9gg 5qgpvBFYFhN8aRrwW9p8DBwlHzgVnkN80gnPpmLRyfSqWAKc3pg+G6eCwSJ8tG+9b1XL65xjaS1/1 rw2MyJKDLPYN6nWC8LgmsmkKOAxqDZz0sUFwG85/Y31FX5e5rcc32RdyhjDED6IKNy5jNeJcI+XMR 19KpeIkx/3aFvhQ2otSFft8r3EGmC3ddD2NnYgJc15gL3X7wqOYRwHLgygjVV0Ci2L0WV9lxXfvdC 92JzwvFIC8RijiFEGhDa0aTC/Ax5GYIAHbiDYtedVQCeB0WgdXkrBPu7/HtHLnH6yM0tquUBU/1wi z7BoN2JYHJCTuZhLtmaw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jZAHz-00069a-V3; Thu, 14 May 2020 09:36:27 +0000 Received: from bhuna.collabora.co.uk ([46.235.227.227]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jZAHw-000697-41 for linux-arm-kernel@lists.infradead.org; Thu, 14 May 2020 09:36:25 +0000 Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: rcn) with ESMTPSA id 0244F2A06B8 Date: Thu, 14 May 2020 11:36:17 +0200 From: Ricardo =?utf-8?Q?Ca=C3=B1uelo?= To: Laurent Pinchart Subject: Re: [PATCH v2 6/6] dt-bindings: drm: bridge: adi,adv7511.txt: convert to yaml Message-ID: <20200514093617.dwhmqaasc3z5ixy6@rcn-XPS-13-9360> Mail-Followup-To: Laurent Pinchart , kernel@collabora.com, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, geert+renesas@glider.be, robh+dt@kernel.org, xuwei5@hisilicon.com References: <20200511110611.3142-1-ricardo.canuelo@collabora.com> <20200511110611.3142-7-ricardo.canuelo@collabora.com> <20200514015412.GF7425@pendragon.ideasonboard.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200514015412.GF7425@pendragon.ideasonboard.com> User-Agent: NeoMutt/20171215 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200514_023624_423983_9FA313EE X-CRM114-Status: GOOD ( 23.88 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, geert+renesas@glider.be, xuwei5@hisilicon.com, robh+dt@kernel.org, kernel@collabora.com, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Laurent, thanks for the thorough review. Some comments below: On jue 14-05-2020 04:54:12, Laurent Pinchart wrote: > > +description: | > > + The ADV7511, ADV7511W and ADV7513 are HDMI audio and video > > + transmitters compatible with HDMI 1.4 and DVI 1.0. They support color > > + space conversion, S/PDIF, CEC and HDCP. They support RGB input > > + interface. > > I would write the last sentence as "The transmitter input is parallel > RGB or YUV data." as YUV is also supported. Ok. > > + adi,input-colorspace: > > + description: Input color space. > > + allOf: > > + - $ref: /schemas/types.yaml#/definitions/string > > + - enum: [ rgb, yuv422, yuv444 ] > > Isn't string implied ? Can't you write > > adi,input-colorspace: > description: Input color space. > enum: [ rgb, yuv422, yuv444 ] example-schema.yaml says that Vendor specific properties have slightly different schema requirements than common properties. They must have at least a type definition and 'description'. However, dt_binding_check doesn't seem to enforce this rule for string properties, and I saw a couple of vendor-specific string properties in other bindings that don't define the type either, so I'm going to follow your suggestion but only for string properties, the rest need a type definition. I noticed I can remove the "allOf" keywords from these too. > > + adi,embedded-sync: > > + description: > > + The input uses synchronization signals embedded in the data > > + stream (similar to BT.656). Defaults to 0 (separate H/V > > + synchronization signals). > > + allOf: > > + - $ref: /schemas/types.yaml#/definitions/uint32 > > + - enum: [ 0, 1 ] > > + - default: 0 > > This be a boolean property (it is read as a bool by the driver, the > property being absent means false, the property being present means > true). You're completely right. > > + ports: > > + description: > > + The ADV7511(W)/13 has two video ports and one audio port. This node > > + models their connections as documented in > > + Documentation/devicetree/bindings/media/video-interfaces.txt > > + Documentation/devicetree/bindings/graph.txt > > + type: object > > + properties: > > + port@0: > > + description: Video port for the RGB, YUV or DSI input. > > s/RGB, YUV or DSI/RGB or YUV/ Ok. > > +if: > > + not: > > + properties: > > + adi,input-colorspace: > > + contains: > > + enum: [ rgb, yuv444 ] > > + adi,input-clock: > > + contains: > > + const: 1x > > As both properties take a single value, I think you can omit > "contains:". I think it's required here, removing it makes the test fail. > > +required: > > + - compatible > > + - reg > > + - ports > > + - adi,input-depth > > + - adi,input-colorspace > > + - adi,input-clock > > Shouldn't the power supplies be required ? Good question. The original binding is not completely clear on this. There's a "Required properties" section at the top that doesn't include the supplies, the supplies block for the ADV7511/11w/13 looks like a gray area in that regard, while the same block for ADV7533/35 is more explicit in that they are required, and they are not listed in the "Optional properties" section. However, most of the DTs that use this binding don't define any supplies. And AFAICT from looking at the code, regulator_get() will use a dummy regulator and show a warning for every expected regulator that's not defined in the DT. If we want to be more strict and require the definition of all the supplies, there will be many more DTs changes in the series, and I'm not sure I'll be able to do that in a reasonable amount of time. I'm looking at them and it's not always clear which regulators to use or if they are even defined. > > +description: | > > + The ADV7533 and ADV7535 are HDMI audio and video transmitters > > + compatible with HDMI 1.4 and DVI 1.0. They support color space > > + conversion, S/PDIF, CEC and HDCP. They support DSI for input pixels. > > I would write the last sentence as "The transmitter input is MIPI DSI.". > > ... > > > + Disables the interal timing generator. The chip will rely on the > > s/interal/internal/ > > > + sync signals in the DSI data lanes, rather than generate its own > > s/generate/generating/ > > ... > > > + properties: > > + port@0: > > + description: > > + Video port for the RGB, YUV or DSI input. The remote endpoint > > s/RGB, YUV or // Ok. > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + adv7511w_in: endpoint { > > + remote-endpoint = <&dpi_out>; > > + }; > > + }; > > + > > + port@1 { > > + reg = <1>; > > + adv7511_out: endpoint { > > + remote-endpoint = <&hdmi_connector_in>; > > + }; > > + }; > > The name of the two endpoints doesn't match the adv7533. The remote > endpoint phandle for port 0 should have dsi in its name. Oops, thanks for catching these. Cheers, Ricardo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel