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

Hi,

On 8/4/26 1:27 PM, [email protected] wrote:
>> +
>> +	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.

It will not. syscon_regmap_lookup_by_phandle_args() internally wraps
of_parse_phandle_with_fixed_args(), the implementation of which can
either return -EINVAL or -ENODEV.

The only additional error path in syscon_regmap_lookup_by_phandle_args()
also returns -ENODEV. At worst, this will mask -ENODEV as -EINVAL.

> 
>> +	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);
> > Is it intentional that the error returned from ti_pipe3_acspcie_tx_rx_mode()
> is ignored?

Yes, I tried to following established convention by modeling the implementation
after other drivers that need to set syscon register bits during probe.

For instance, the dra7 pcie driver. See: dra7xx_pcie_unaligned_memaccess() or
dra7xx_pcie_configure_two_lane(). Those follow the same structure,
returning -EINVAL on lookup failure or the result from regmap_update_bits().

Where the setting is optional, a warning is issued on failure, but the probe
then simply continues.

Thanks,

David