Re: [PATCH v11 35/37] pinctrl: airoha: try to find chip scu node by phandle first

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The fallback logic unconditionally catches all errors from syscon_regmap_lookup_by_phandle(), overwriting transient errors like -EPROBE_DEFER.
- [Low] Unnecessary use of IS_ERR_OR_NULL() for an API that exclusively returns an ERR_PTR on failure.
--

--- Patch [35]: [PATCH v11 35/37] pinctrl: airoha: try to find chip scu node by phandle first ---
commit 85429d10727580a9500263e5a9986ffdc51aa04d
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: try to find chip scu node by phandle first
    
The commit updates the probe function to first search for the chip scu
regmap using a phandle. It falls back to using the SoC specific compatible
string on failure.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 134c57a54b963..153dd825c18c4 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
> @@ -792,9 +792,12 @@ int airoha_pinctrl_probe(struct platform_device *pdev)
>  	if (IS_ERR(pinctrl->regmap))
>  		return PTR_ERR(pinctrl->regmap);
>  
> -	map = syscon_regmap_lookup_by_compatible("airoha,en7581-chip-scu");
> -	if (IS_ERR(map))
> -		return PTR_ERR(map);
> +	map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "airoha,chip-scu");
> +	if (IS_ERR_OR_NULL(map)) {

[Severity: Low]
Does syscon_regmap_lookup_by_phandle() ever return NULL? It looks like
this API exclusively returns either a valid regmap pointer or an ERR_PTR.
Could this just use IS_ERR() instead?

[Severity: High]
Does this introduce a regression where the fallback unconditionally catches
-EPROBE_DEFER?

If syscon_regmap_lookup_by_phandle() returns -EPROBE_DEFER because the
syscon node is not yet ready, the code falls back to
syscon_regmap_lookup_by_compatible().

If the device tree uses the new phandle format and lacks the legacy
compatible string, the fallback will return -ENODEV. This overwrites the
-EPROBE_DEFER from the initial lookup and permanently aborts the probe.

Should the code explicitly check for -EPROBE_DEFER and return it before
falling back?

> +		map = syscon_regmap_lookup_by_compatible(data->chip_scu_compatible);
> +		if (IS_ERR(map))
> +			return PTR_ERR(map);
> +	}
>  
>  	pinctrl->chip_scu = map;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=35