2021-05-11 02:28:14

by Shawn Guo

Subject: Re: [PATCH v3 3/4] arm64: dts: imx8mq-nitrogen: add lt8912 MIPI-DSI to HDMI

On Thu, Apr 01, 2021 at 01:23:55AM +0200, Adrien Grassein wrote:
> Add support of the lt8912b in the DTB.
> This adds the support of the DB_DSIHD daugther board from
> Boundary Devices.
>
> Signed-off-by: Adrien Grassein <[email protected]>
> ---
> .../boot/dts/freescale/imx8mq-nitrogen.dts | 120 ++++++++++++++++++
> 1 file changed, 120 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-nitrogen.dts b/arch/arm64/boot/dts/freescale/imx8mq-nitrogen.dts
> index 04992cbba56e..4ffd23ea705f 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq-nitrogen.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-nitrogen.dts
> @@ -34,6 +34,19 @@ power {
> };
> };
>
> + hdmi-connector {
> + compatible = "hdmi-connector";
> + ddc-i2c-bus = <&ddc_i2c_bus>;
> + label = "hdmi";
> + type = "a";
> +
> + port {
> + hdmi_connector_in: endpoint {
> + remote-endpoint = <&lt8912_out>;
> + };
> + };
> + };
> +
> reg_usb_otg_vbus: regulator-usb-otg-vbus {
> compatible = "regulator-fixed";
> pinctrl-names = "default";
> @@ -81,6 +94,9 @@ reg_vref_5v: regulator-vref-5v {
> };
> };
>
> +&dphy {
> + status = "okay";
> +};
>
> &fec1 {
> pinctrl-names = "default";
> @@ -193,6 +209,97 @@ rtc@68 {
> };
> };
>
> +&i2c4 {
> + clock-frequency = <100000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_i2c4>;
> + status = "okay";
> +
> + pca9546: i2cmux9546@70 {

Node name should be generic, so 9546 should be dropped from there?

> + compatible = "nxp,pca9546";
> + reg = <0x70>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + i2c4@0 {

Is number 4 really needed in node name?

> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clock-frequency = <100000>;
> +
> + hdmi-bridge@48 {
> + compatible = "lontium,lt8912b";
> + reg = <0x48> ;
> + reset-gpios = <&max7323 0 GPIO_ACTIVE_LOW>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + hdmi_out_in: endpoint {
> + data-lanes = <1 2 3 4>;
> + remote-endpoint = <&mipi_dsi_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + lt8912_out: endpoint {
> + remote-endpoint = <&hdmi_connector_in>;
> + };
> + };
> + };
> + };
> + };
> +
> + ddc_i2c_bus: i2c4@1 {
> + reg = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clock-frequency = <100000>;
> + };
> +
> + i2c4@3 {
> + reg = <3>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clock-frequency = <100000>;
> +
> + max7323: max7323@68 {

Can we have a generic node name for this device?

> + compatible = "maxim,max7323";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_max7323>;
> + gpio-controller;
> + reg = <0x68>;
> + #gpio-cells = <2>;
> + };
> + };
> + };
> +};
> +
> +&lcdif {
> + status = "okay";
> +};
> +
> +&mipi_dsi {
> + status = "okay";

Move it to end of property list.

> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ports {
> + port@1 {
> + reg = <1>;

Newline between property and child node.

Shawn

> + mipi_dsi_out: endpoint {
> + remote-endpoint = <&hdmi_out_in>;
> + };
> + };
> + };
> +};
> +
> &uart1 { /* console */
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_uart1>;
> @@ -368,6 +475,19 @@ MX8MQ_IOMUXC_GPIO1_IO06_GPIO1_IO6 0x49
> >;
> };
>
> + pinctrl_i2c4: i2c4grp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_I2C4_SCL_I2C4_SCL 0x4000007f
> + MX8MQ_IOMUXC_I2C4_SDA_I2C4_SDA 0x4000007f
> + >;
> + };
> +
> + pinctrl_max7323: max7323grp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_NAND_RE_B_GPIO3_IO15 0x19
> + >;
> + };
> +
> pinctrl_reg_arm_dram: reg-arm-dramgrp {
> fsl,pins = <
> MX8MQ_IOMUXC_SAI5_RXD3_GPIO3_IO24 0x16
> --
> 2.25.1
>

2021-05-11 19:38:46

by Adrien Grassein

Subject: Re: [PATCH v3 3/4] arm64: dts: imx8mq-nitrogen: add lt8912 MIPI-DSI to HDMI

Hi Shawn,

Thanks for your review.

Le mar. 11 mai 2021 à 04:27, Shawn Guo <[email protected]> a écrit :
>
> On Thu, Apr 01, 2021 at 01:23:55AM +0200, Adrien Grassein wrote:
> > Add support of the lt8912b in the DTB.
> > This adds the support of the DB_DSIHD daugther board from
> > Boundary Devices.
> >
> > Signed-off-by: Adrien Grassein <[email protected]>
> > ---
> > .../boot/dts/freescale/imx8mq-nitrogen.dts | 120 ++++++++++++++++++
> > 1 file changed, 120 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-nitrogen.dts b/arch/arm64/boot/dts/freescale/imx8mq-nitrogen.dts
> > index 04992cbba56e..4ffd23ea705f 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mq-nitrogen.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx8mq-nitrogen.dts
> > @@ -34,6 +34,19 @@ power {
> > };
> > };
> >
> > + hdmi-connector {
> > + compatible = "hdmi-connector";
> > + ddc-i2c-bus = <&ddc_i2c_bus>;
> > + label = "hdmi";
> > + type = "a";
> > +
> > + port {
> > + hdmi_connector_in: endpoint {
> > + remote-endpoint = <&lt8912_out>;
> > + };
> > + };
> > + };
> > +
> > reg_usb_otg_vbus: regulator-usb-otg-vbus {
> > compatible = "regulator-fixed";
> > pinctrl-names = "default";
> > @@ -81,6 +94,9 @@ reg_vref_5v: regulator-vref-5v {
> > };
> > };
> >
> > +&dphy {
> > + status = "okay";
> > +};
> >
> > &fec1 {
> > pinctrl-names = "default";
> > @@ -193,6 +209,97 @@ rtc@68 {
> > };
> > };
> >
> > +&i2c4 {
> > + clock-frequency = <100000>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_i2c4>;
> > + status = "okay";
> > +
> > + pca9546: i2cmux9546@70 {
>
> Node name should be generic, so 9546 should be dropped from there?
>
> > + compatible = "nxp,pca9546";
> > + reg = <0x70>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + i2c4@0 {
>
> Is number 4 really needed in node name?
>
I add 4 to keep it coherent with the DTS file (i2c1@0 for example).

> > + reg = <0>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + clock-frequency = <100000>;
> > +
> > + hdmi-bridge@48 {
> > + compatible = "lontium,lt8912b";
> > + reg = <0x48> ;
> > + reset-gpios = <&max7323 0 GPIO_ACTIVE_LOW>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + hdmi_out_in: endpoint {
> > + data-lanes = <1 2 3 4>;
> > + remote-endpoint = <&mipi_dsi_out>;
> > + };
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > +
> > + lt8912_out: endpoint {
> > + remote-endpoint = <&hdmi_connector_in>;
> > + };
> > + };
> > + };
> > + };
> > + };
> > +
> > + ddc_i2c_bus: i2c4@1 {
> > + reg = <1>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + clock-frequency = <100000>;
> > + };
> > +
> > + i2c4@3 {
> > + reg = <3>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + clock-frequency = <100000>;
> > +
> > + max7323: max7323@68 {
>
> Can we have a generic node name for this device?
>
> > + compatible = "maxim,max7323";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_max7323>;
> > + gpio-controller;
> > + reg = <0x68>;
> > + #gpio-cells = <2>;
> > + };
> > + };
> > + };
> > +};
> > +
> > +&lcdif {
> > + status = "okay";
> > +};
> > +
> > +&mipi_dsi {
> > + status = "okay";
>
> Move it to end of property list.
>
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + ports {
> > + port@1 {
> > + reg = <1>;
>
> Newline between property and child node.
>
> Shawn
>
> > + mipi_dsi_out: endpoint {
> > + remote-endpoint = <&hdmi_out_in>;
> > + };
> > + };
> > + };
> > +};
> > +
> > &uart1 { /* console */
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_uart1>;
> > @@ -368,6 +475,19 @@ MX8MQ_IOMUXC_GPIO1_IO06_GPIO1_IO6 0x49
> > >;
> > };
> >
> > + pinctrl_i2c4: i2c4grp {
> > + fsl,pins = <
> > + MX8MQ_IOMUXC_I2C4_SCL_I2C4_SCL 0x4000007f
> > + MX8MQ_IOMUXC_I2C4_SDA_I2C4_SDA 0x4000007f
> > + >;
> > + };
> > +
> > + pinctrl_max7323: max7323grp {
> > + fsl,pins = <
> > + MX8MQ_IOMUXC_NAND_RE_B_GPIO3_IO15 0x19
> > + >;
> > + };
> > +
> > pinctrl_reg_arm_dram: reg-arm-dramgrp {
> > fsl,pins = <
> > MX8MQ_IOMUXC_SAI5_RXD3_GPIO3_IO24 0x16
> > --
> > 2.25.1
> >

The other comments are OK.
I will send you an updated patch set.

Thanks again,