Re: [PATCH v3 2/2] arm: dts: nxp: imx: Add devicetree files for M48

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] ARM: dts: imx6qdl-draeger-m48: missing GPIO_OPEN_DRAIN on I2C recovery GPIOs
- [Medium] ARM: dts: imx6qdl-draeger-m48: halved LVDS pixel clock in split mode
- [Medium] ARM: dts: imx6: missing ODE and SION bits on HDMI DDC pins in draeger-m48
- [Low] ARM: dts: imx6qdl-draeger-m48: undocumented draeger vendor prefix
--

commit 1f358a609d69dc3047258ea0965effc8740f95c7
Author: Petko Manolov <petko.manolov@konsulko.com>

arm: dts: nxp: imx: Add devicetree files for M48

This commit adds devicetree files for the i.MX6 DualLite and i.MX6 Quad
variants of the Draeger M48 board.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6dl-draeger-m48.dts b/arch/arm/boot/dts/nxp/imx/imx6dl-draeger-m48.dts
> new file mode 100644
> index 0000000000000..b0bc9a67f5b34
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6dl-draeger-m48.dts
> @@ -0,0 +1,16 @@
[ ... ]
> +/ {
> +	model = "Draeger M48 i.MX6 DualLite";
> +	compatible = "draeger,imx6dl-m48", "fsl,imx6dl";
> +};
> diff --git a/arch/arm/boot/dts/nxp/imx/imx6q-draeger-m48.dts b/arch/arm/boot/dts/nxp/imx/imx6q-draeger-m48.dts
> new file mode 100644
> index 0000000000000..ff9c337e93fbf
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6q-draeger-m48.dts
> @@ -0,0 +1,16 @@
[ ... ]
> +/ {
> +	model = "Draeger M48 i.MX6 Quad";
> +	compatible = "draeger,imx6q-m48", "fsl,imx6q";
> +};

[Severity: Low]
The root compatible properties in
arch/arm/boot/dts/nxp/imx/imx6dl-draeger-m48.dts and
arch/arm/boot/dts/nxp/imx/imx6q-draeger-m48.dts introduce the "draeger"
vendor prefix, but Documentation/devicetree/bindings/vendor-prefixes.yaml
only contains entries for "^dptel,.*" and "^dragino,.*" in patternProperties:

  "^dptel,.*":
    description: Guangdong Dapu Telecom Co., Ltd.
  "^dragino,.*":
    description: Dragino Technology Co., Limited

Should the "draeger" vendor prefix be documented in
Documentation/devicetree/bindings/vendor-prefixes.yaml so that checkpatch.pl
and schema validation do not warn about an undocumented compatible string
vendor?

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6qdl-draeger-m48.dtsi b/arch/arm/boot/dts/nxp/imx/imx6qdl-draeger-m48.dtsi
> new file mode 100644
> index 0000000000000..19207d45764c0
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6qdl-draeger-m48.dtsi
> @@ -0,0 +1,213 @@
[ ... ]
> +&hdmi {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_hdmi>;
> +	status = "okay";
> +};
> +
> +&i2c2 {
> +	clock-frequency = <100000>;
> +	pinctrl-names = "default", "gpio";
> +	pinctrl-0 = <&pinctrl_i2c2>;
> +	pinctrl-1 = <&pinctrl_i2c2_gpio>;
> +	scl-gpios = <&gpio4 12 GPIO_ACTIVE_HIGH>;
> +	sda-gpios = <&gpio4 13 GPIO_ACTIVE_HIGH>;
> +	status = "okay";
> +
> +	eeprom_mac: eeprom@50 {
> +		compatible = "atmel,24c08";
> +		reg = <0x50>;
> +		pagesize = <16>;
> +	};
> +};
> +
> +&i2c3 {
> +	pinctrl-names = "default", "gpio";
> +	pinctrl-0 = <&pinctrl_i2c3>;
> +	pinctrl-1 = <&pinctrl_i2c3_gpio>;
> +	scl-gpios = <&gpio3 17 GPIO_ACTIVE_HIGH>;
> +	sda-gpios = <&gpio3 18 GPIO_ACTIVE_HIGH>;
> +	status = "okay";
> +};

[Severity: Medium]
During adapter registration for &i2c2 and &i2c3,
i2c_init_recovery() -> i2c_gpio_init_generic_recovery() in
drivers/i2c/i2c-core-base.c requests scl-gpios with
GPIOD_OUT_HIGH_OPEN_DRAIN and sda-gpios with GPIOD_IN.

When GPIO_OPEN_DRAIN is not set on scl-gpios in the devicetree,
gpiod_configure_flags() in drivers/gpio/gpiolib.c logs a runtime warning:

	if (lflags & GPIO_OPEN_DRAIN)
		set_bit(GPIOD_FLAG_OPEN_DRAIN, &desc->flags);
	else if (dflags & GPIOD_FLAGS_BIT_OPEN_DRAIN) {
		set_bit(GPIOD_FLAG_OPEN_DRAIN, &desc->flags);
		gpiod_warn(desc,
			   "enforced open drain please flag it properly in DT/ACPI DSDT/board file\n");
	}

In addition, because sda-gpios is requested with GPIOD_IN,
i2c_init_recovery() in drivers/i2c/i2c-core-base.c checks whether the line is
single-ended before enabling sda driving for bus recovery:

		if (bri->sda_gpiod) {
			bri->get_sda = get_sda_gpio_value;
			if (gpiod_get_direction(bri->sda_gpiod) == GPIO_LINE_DIRECTION_OUT ||
			    gpiod_is_single_ended(bri->sda_gpiod))
				bri->set_sda = set_sda_gpio_value;
		}

Without GPIO_OPEN_DRAIN on sda-gpios, gpiod_is_single_ended(bri->sda_gpiod)
returns false and bri->set_sda remains NULL, preventing recovery from driving
sda to generate a stop condition. Also, pinctrl_i2c2_gpio and
pinctrl_i2c3_gpio below use pad config 0x1b820 without the sion bit
(0x40000000), unlike pinctrl_i2c2 and pinctrl_i2c3 (0x4001b820).

Should scl-gpios and sda-gpios in &i2c2 and &i2c3 be flagged with
(GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN), and should pinctrl_i2c2_gpio and
pinctrl_i2c3_gpio also set 0x4001b820?

> +
> +&ldb {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_lvds>;
> +	fsl,dual-channel;
> +	status = "okay";
> +
> +	lvds-channel@0 {
> +		fsl,data-mapping = "spwg";
> +		fsl,data-width = <24>;
> +		status = "okay";
> +
> +		display-timings {
> +			native-mode = <&timing0>;
> +
> +			/* LVDS split mode */
> +			timing0: timing-1920x1080 {
> +				clock-frequency = <71190000>;	/* pixclk / 2 */
> +				hactive = <1920>;
> +				vactive = <1080>;
> +				hback-porch = <60>;		/* left * 2 */
> +				hfront-porch = <60>;		/* right * 2 */
> +				vback-porch = <30>;
> +				vfront-porch = <3>;
> +				hsync-len = <60>;		/* hsync * 2 */
> +				vsync-len = <17>;
> +			};

[Severity: Medium]
In timing0 under &ldb, the horizontal parameters specify the full two-channel
line totals (1920 + 60 + 60 + 60 = 2100 pixels per line, 1130 lines per
frame), which require a 142.38 MHz pixel clock for 60 Hz, while
clock-frequency is pre-divided by 2 to 71190000.

When fsl,dual-channel is enabled, imx_ldb_encoder_atomic_mode_set() in
drivers/gpu/drm/imx/ipuv3/imx-ldb.c treats mode->clock as the full combined
display interface pixel clock (di_clk = mode->clock * 1000) and already
halves the per-channel serial clock multiplier:

	if (dual) {
		serial_clk = 3500UL * mode->clock;
		imx_ldb_set_clock(ldb, mux, 0, serial_clk, di_clk);
		imx_ldb_set_clock(ldb, mux, 1, serial_clk, di_clk);
	} else {
		serial_clk = 7000UL * mode->clock;
		imx_ldb_set_clock(ldb, mux, imx_ldb_ch->chno, serial_clk,
				  di_clk);
	}

Meanwhile, ipu_di_sync_config_noninterlaced() in
drivers/gpu/ipu-v3/ipu-di.c clocks all 2100 horizontal pixels per line at
di_clk.

Will pre-dividing clock-frequency to 71190000 here cause the display
interface and lvds channels to run at half the intended clock rate (30 Hz
instead of 60 Hz), and should clock-frequency be <142380000> instead?

> +		};
> +	};
> +};
[ ... ]
> +&iomuxc {
[ ... ]
> +	pinctrl_hdmi: hdmigrp {
> +		fsl,pins = <
> +			MX6QDL_PAD_EIM_D16__HDMI_TX_DDC_SDA	0x1b0b0
> +			MX6QDL_PAD_EIM_EB2__HDMI_TX_DDC_SCL	0x1b0b0
> +		>;
> +	};

[Severity: Medium]
The &hdmi node in arch/arm/boot/dts/nxp/imx/imx6qdl-draeger-m48.dtsi does not
specify an external ddc-i2c-bus phandle, so dw_hdmi_probe() in
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c uses the internal DesignWare ddc
i2c master on the pads configured by pinctrl_hdmi.

Here MX6QDL_PAD_EIM_D16__HDMI_TX_DDC_SDA and
MX6QDL_PAD_EIM_EB2__HDMI_TX_DDC_SCL are configured with 0x1b0b0, which omits
both open drain enable (0x800) and software input on (0x40000000). Other
i.MX6 boards using the internal hdmi ddc controller (such as
imx6qdl-apalis.dtsi and imx6qdl-colibri.dtsi in pinctrl_hdmi_ddc) configure
these pads with 0x4001b8b1.

Without open drain and software input on enabled on these pads, can hdmi edid
reads over the internal ddc i2c bus fail?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021092140.4142493-1-petko.manolov@konsulko.com?part=2