Re: [PATCH v2 2/3] arm64: dts: freescale: imx8mp-toradex-smarc: Add camera connector nexus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: imx8mp-toradex-smarc: camera-connector nexus breaks fw_devlink
- [Medium] arm64: dts: freescale: undocumented toradex,camera-connector compatible
--

commit d093f7d003a6a1108921f20ccc8a5a3a82a991c7
Author: Ernest Van Hoecke <ernest.vanhoecke@toradex.com>

arm64: dts: freescale: imx8mp-toradex-smarc: Add camera connector nexus

This commit adds two camera connector gpio nexus nodes to
imx8mp-toradex-smarc-dev.dts and adds labels for the camera i2c buses and
mipi csi receivers in imx8mp-toradex-smarc.dtsi.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-toradex-smarc-dev.dts b/arch/arm64/boot/dts/freescale/imx8mp-toradex-smarc-dev.dts
> index c28d63293149b..e0096388fbfe1 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-toradex-smarc-dev.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-toradex-smarc-dev.dts
> @@ -13,6 +13,32 @@ / {
>  		     "toradex,smarc-imx8mp",
>  		     "fsl,imx8mp";
>  
> +	smarc_csi0: camera-connector-0 {
> +		compatible = "toradex,camera-connector";

[Severity: Medium]
Is there a devicetree binding schema documenting the
"toradex,camera-connector" compatible string used in camera-connector-0 and
camera-connector-1 in
arch/arm64/boot/dts/freescale/imx8mp-toradex-smarc-dev.dts?

Neither Documentation/devicetree/bindings/ in the tree nor this patch series
defines "toradex,camera-connector", which causes dtbs_check validation on
imx8mp-toradex-smarc-dev.dtb to fail to match any schema:

Documentation/devicetree/bindings/submitting-patches.rst:
  6) Any compatible strings used in a chip or board DTS file must be
     previously documented in the corresponding DT binding file
     in Documentation/devicetree/bindings.  This rule applies even if
     the Linux device driver does not yet match on the compatible
     string.

Should a binding schema be added before using this compatible string?

> +		#gpio-cells = <2>;
> +		/*
> +		 * CAM_0_CON_RST     (pin 11) - SMARC GPIO2 (P110)
> +		 * CAM_0_CON_PWRCTRL (pin 24) - SMARC GPIO0 (P108)
> +		 */
> +		gpio-map = <0 0 &som_gpio_expander 2 GPIO_ACTIVE_HIGH>,
> +			   <3 0 &som_gpio_expander 0 GPIO_ACTIVE_HIGH>;
> +		gpio-map-mask = <0xffffffff 0x0>;
> +		gpio-map-pass-thru = <0x0 0xffffffff>;
> +	};
> +
> +	smarc_csi1: camera-connector-1 {
> +		compatible = "toradex,camera-connector";
> +		#gpio-cells = <2>;
> +		/*
> +		 * CAM_1_CON_RST     (pin 11) - SMARC GPIO3 (P111)
> +		 * CAM_1_CON_PWRCTRL (pin 24) - SMARC GPIO1 (P109)
> +		 */
> +		gpio-map = <0 0 &som_gpio_expander 3 GPIO_ACTIVE_HIGH>,
> +			   <3 0 &som_gpio_expander 1 GPIO_ACTIVE_HIGH>;
> +		gpio-map-mask = <0xffffffff 0x0>;
> +		gpio-map-pass-thru = <0x0 0xffffffff>;
> +	};

[Severity: High]
Will routing consumer gpios through camera-connector-0 and
camera-connector-1 cause fw_devlink to block consumer camera devices from
probing?

Because camera-connector-0 and camera-connector-1 are top-level nodes with a
compatible property, of_platform_default_populate_init() ->
of_platform_default_populate() -> of_platform_populate() ->
of_platform_bus_create() instantiates platform devices for them that have no
matching driver.

When a consumer device (such as camera@3c in the ov5640 overlay) references
&smarc_csi0 or &smarc_csi1 in reset-gpios and powerdown-gpios, fw_devlink
resolves the supplier through of_link_property() -> parse_gpios() ->
parse_suffix_prop_cells():

drivers/of/property.c:parse_suffix_prop_cells() {
	if (of_parse_phandle_with_args(np, prop_name, cells_name, index,
				       &sup_args))
		return NULL;

	return sup_args.np;
}

Unlike of_get_named_gpiod_flags() in drivers/gpio/gpiolib-of.c, which calls
of_parse_phandle_with_args_map() to traverse gpio-map,
parse_suffix_prop_cells() calls of_parse_phandle_with_args() and stops at
the nexus node itself.

As a result, fw_devlink_create_devlink() links the camera consumer to the
driverless camera-connector platform device instead of som_gpio_expander.
When the camera consumer probes via really_probe() ->
device_links_check_suppliers(), it sees that the supplier has no bound
driver:

drivers/base/core.c:device_links_check_suppliers() {
		if (link->status != DL_STATE_AVAILABLE &&
		    !device_link_test(link, DL_FLAG_SYNC_STATE_ONLY)) {
...
			device_links_missing_supplier(dev);
			ret = dev_err_probe(dev, -EPROBE_DEFER,
					    "supplier %s not ready\n", dev_name(link->supplier));
			break;
		}
}

Note that simply dropping the compatible property from camera-connector-0
and camera-connector-1 without updating fw_devlink to traverse gpio-map
would leave the nexus nodes without a struct device, causing
fwnode_links_check_suppliers() in device_links_check_suppliers() to defer
the consumer permanently with "wait for supplier /camera-connector-0" even
after driver_deferred_probe_timeout expires.

Could this regression be avoided by teaching fw_devlink to resolve through
gpio-map (for example using of_parse_phandle_with_args_map() in
drivers/of/property.c) before relying on a driverless gpio nexus node?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-v1-smarc-imx8mp-cameras-v2-0-cfe3d25e7aed@toradex.com?part=2