From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xin Ji Subject: Re: [PATCH v1 1/2] dt-bindings: drm/bridge: anx7625: MIPI to DP transmitter binding Date: Mon, 23 Sep 2019 03:55:48 +0000 Message-ID: <20190923035539.GA5916@xin-VirtualBox> References: <606dba07640f0c9aba930e1dfb5d6a797f393ecc.1568957789.git.xji@analogixsemi.com> <20190920094621.GA12950@pendragon.ideasonboard.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20190920094621.GA12950@pendragon.ideasonboard.com> Content-Language: en-US Content-ID: <89EF8D1BB3962448B6334707B56223DD@namprd04.prod.outlook.com> Sender: linux-kernel-owner@vger.kernel.org To: Laurent Pinchart Cc: "devel@driverdev.osuosl.org" , Andrzej Hajda , Neil Armstrong , Jonas Karlman , Jernej Skrabec , David Airlie , Daniel Vetter , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , Sheng Pan List-Id: dri-devel@lists.freedesktop.org Hi Laurent Pinchart, thanks for your comment. On Fri, Sep 20, 2019 at 12:46:21PM +0300, Laurent Pinchart wrote: > Hello Xin Ji, >=20 > Thank you for the patch. >=20 > On Fri, Sep 20, 2019 at 06:05:03AM +0000, 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. >=20 > I assume you meant MIPI DSI ? MIPI has released more standards than DSI, > so it doesn't hurt to specify this explicitly. It support DSI and DPI, I will to point out. >=20 > According to > https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBr= ief_0.pdf, > the ANX7625 supports for MIPI DSI and DPI on the input side. > Furthermore, it seems to output DisplayPort on USB Type-C. Should this > be documented ? It can support both eDP output or USB Type-C output. >=20 > > You can add support to your board with binding. > >=20 > > Example: > > anx_bridge: anx7625@58 { > > compatible =3D "analogix,anx7625"; > > reg =3D <0x58>; > > low-power-mode =3D <1>; > > dsi-supported =3D <1>; > > dsi-channel-id =3D <1>; > > dsi-lanes-num =3D <4>; > > internal-pannel-supported =3D <1>; > > pon-gpios =3D <&gpio0 45 GPIO_ACTIVE_LOW>; > > reset-gpios =3D <&gpio0 73 GPIO_ACTIVE_LOW>; > > status =3D "okay"; > > port { > > anx7625_1_in: endpoint { > > remote-endpoint =3D <&mipi_dsi_bridge_1>; > > }; > > }; > > }; > >=20 > > Signed-off-by: Xin Ji > > --- > > .../bindings/display/bridge/anx7625.yaml | 84 ++++++++++++++= ++++++++ > > 1 file changed, 84 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/display/bridge/an= x7625.yaml > >=20 > > diff --git a/Documentation/devicetree/bindings/display/bridge/anx7625.y= aml b/Documentation/devicetree/bindings/display/bridge/anx7625.yaml > > new file mode 100644 > > index 0000000..95fe18b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/bridge/anx7625.yaml > > @@ -0,0 +1,84 @@ > > +# 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 > > + > > +description: | > > + The ANX7625 is an ultra-low power 4K Mobile HD Transmitter > > + designed for portable devices. > > + > > +properties: > > + compatible: > > + items: > > + - const: analogix,anx7625 > > + > > + reg: > > + maxItems: 1 > > + > > + low-power-gpios: > > + description: Low power mode support feature > > + maxItems: 1 > > + > > + hpd-gpios: > > + description: used for HPD interrupt > > + maxItems: 1 > > + > > + pon-gpios: > > + description: used for power on chip control > > + maxItems: 1 > > + > > + reset-gpios: > > + description: used for reset chip control > > + maxItems: 1 >=20 > How about mentioning which pin of the ANX7625 each GPIO refers to ? For > the low-power, pon and reset GPIOs I assume they directly control the > chip. We have standard names for some GPIOs, such as reset or enable. Is > there one of the low-power and pon GPIOs that would qualify as an enable > signal ? OK, I think pon-gpios can qualify as an enable. >=20 > What is the HPD GPIO for ? Does the chip output and HPD signal ? Once the anx7625 received eDP HPD signal, the firmware will report HPD interrupt to AP through defined gpio interrupt pin "hpd-gpios". It used for interrupt between anx7625 and AP, not used for output. >=20 > > + > > + extcon-supported: > > + description: external connector interface support flag > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + > > + internal-pannel-supported: > > + description: indicate does internal pannel exist or not > > + $ref: /schemas/types.yaml#/definitions/uint32 >=20 > s/pannel/panel/ OK, will fix it. >=20 > Are those two properties mutually exclusive ? If so you should combine > them in a single required property with two values. My feeling is that > they should be dropped though, please see below. Yes, they are mutually exclusive. There are 3 case, one is support google "external connector" framework, one support internal panel, the other is support normal eDP output, so I defined two flags to distinguish them here. Based on your comment below, I'll define output port to distinguish them. >=20 > > + > > + dsi-supported: > > + description: support MIPI DSI or DPI > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + > > + dsi-channel-id: > > + description: dsi channel index > > + $ref: /schemas/types.yaml#/definitions/uint32 >=20 > This does not belong to DT, the virtual channel used by the DSI source > should be queried at runtime by communicating between drivers. I didn't know where can quiry this value. Can you give me more detail? >=20 > > + > > + dsi-lanes-num: > > + description: dsi lanes used num > > + $ref: /schemas/types.yaml#/definitions/uint32 >=20 > Please use the standard data-lanes property as defined in > video-interface.txt. It should be specified in the DSI endpoints. OK, I'll fetch the dsi lanes from DSI endpoints. >=20 > > + > > + port@0: > > + type: object > > + description: > > + A port node pointing to MIPI DSI host port node. >=20 > You need at least 3 ports: >=20 > - a DPI input port I'll add DPI input port. > - a DSI input port > - an output port >=20 > The dsi-supported property should be dropped, detecting the type of > input should be done based on which of the DPI or DSI input port is > connected. OK. >=20 > Assuming the ANX7625 can also output DisplayPort directly without going > through USB Type-C (I can't verify that without the datasheet), I think > you should use two output ports, one for USB Type-C and one for > DisplayPort. The extcon-supported and internal-pannel-supported > properties should be removed, and deduced from the connect output port. I think there should exist 3 output port, one is for extern connector, one is for normal eDP, other is for internal panel eDP. >=20 > > + > > +required: > > + - compatible > > + - reg > > + - dsi-channel-id > > + - dsi-lanes-num > > + - port@0 > > + > > +example: > > + - | > > + anx_bridge: anx7625@58 { > > + compatible =3D "analogix,anx7625"; > > + reg =3D <0x58>; > > + low-power-gpios =3D <0>; > > + dsi-supported =3D <1>; > > + dsi-channel-id =3D <1>; > > + dsi-lanes-num =3D <4>; > > + hpd-gpios =3D <&gpio1 19 IRQ_TYPE_LEVEL_LOW>; > > + status =3D "okay"; > > + }; >=20 > You mention the port@0 node as being required, but it's missing from the > example. OK, I'll change it in the next series. >=20 > --=20 > Regards, >=20 > Laurent Pinchart