Re: [PATCH v2 2/2] phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver mishandles errors from the `ti,syscon-acspcie-tx-rx` property lookup, breaking probe deferral and silently swallowing configuration failures.
--

--- Patch [2]: [PATCH v2 2/2] phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode ---
commit 1cbf4349932708c2c097de105a5c13d7baeaeb61
Author: David Oberhollenzer <[email protected]>

phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode

This commit allows configuring the ACSPCIe TX/RX mode for PCIe PHYs.
It reads the syscon property from the device tree and updates the mode
selection bits in the specified register.

> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c

[ ... ]

> @@ -785,6 +788,24 @@ static int ti_pipe3_get_pll_base(struct ti_pipe3 *phy)
>  	return PTR_ERR_OR_ZERO(phy->pll_ctrl_base);
>  }
>  
> +static int ti_pipe3_acspcie_tx_rx_mode(struct ti_pipe3 *phy)
> +{
> +	struct device_node *np = phy->dev->of_node;
> +	struct regmap *regmap;
> +	unsigned int args[2];
> +
> +	regmap = syscon_regmap_lookup_by_phandle_args(np,
> +						      "ti,syscon-acspcie-tx-rx",
> +						      2, args);
> +	if (IS_ERR(regmap)) {
> +		dev_warn(phy->dev, "can't get ti,syscon-acspcie-tx-rx\n");
> +		return -EINVAL;
> +	}

[Severity: High]
Does converting all syscon lookup errors to -EINVAL break probe deferral?

If syscon_regmap_lookup_by_phandle_args() returns -EPROBE_DEFER because the
syscon driver probes later, this converts the error to -EINVAL and prevents
the driver from deferring properly.

Additionally, if this property is optional on older device trees, would an
-ENOENT return cause spurious warnings in the kernel log?

> +
> +	return regmap_update_bits(regmap, args[0], PCIE_TX_RX_CTRL_MASK,
> +				  args[1] << PCIE_TX_RX_CTRL_SHIFT);
> +}
> +
>  static int ti_pipe3_probe(struct platform_device *pdev)
>  {
>  	struct ti_pipe3 *phy;

[ ... ]

> @@ -807,6 +828,12 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>  	phy->dpll_map = data->dpll_map;
>  	phy->settings = data->settings;
>  
> +	if (phy->mode == PIPE3_MODE_PCIE) {
> +		ret = ti_pipe3_acspcie_tx_rx_mode(phy);
> +		if (ret)
> +			dev_warn(dev, "failed to set ACSPCIe TX/RX mode\n");
> +	}
> +
>  	ret = ti_pipe3_get_pll_base(phy);
>  	if (ret)
>  		return ret;

[Severity: High]
Does this sequence silently swallow the configuration failure?

The error code from ti_pipe3_acspcie_tx_rx_mode() is captured in ret and
logged, but probe execution continues.

Because ti_pipe3_get_pll_base() unconditionally returns 0 for PCIe mode
right after this check, the value in ret is immediately overwritten, and the
probe will not abort.

Could this leave the PCIe bus in a non-functional state without explicitly
failing the driver probe?

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

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy