Re: [PATCH 1/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] Probe deferral is broken and errors are masked and ignored when fetching the syscon-acspcie-tx-rx property.
--

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

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

This commit adds a device tree setting to configure the ACSPCIe TX/RX mode
for the OMAP PCIe PHY subsystem. It uses a syscon regmap to apply the
configuration during probe.

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index b5543b5c674c8..801a6239328e1 100644
> --- 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,
> +						      "syscon-acspcie-tx-rx",
> +						      2, args);
> +	if (IS_ERR(regmap)) {
> +		dev_warn(phy->dev, "can't get syscon-acspcie-tx-rx\n");
> +		return -EINVAL;

[Severity: High]
Does this code inadvertently break probe deferral?

If the syscon provider is not yet ready, syscon_regmap_lookup_by_phandle_args()
will return -EPROBE_DEFER. Masking this actual error code into -EINVAL could
prevent the driver from probing later.

> +	}
> +
> +	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);

[Severity: High]
Is it intentional that the error returned from ti_pipe3_acspcie_tx_rx_mode()
is ignored?

The value of ret is captured but immediately overwritten by
ti_pipe3_get_pll_base(). This causes the probe to proceed even if the syscon
lookup fails or requires deferral.

>  	if (ret)
>  		return ret;

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