Re: [PATCH v2 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
To: Philipp Zabel <p.zabel@pengutronix.de>,
	Wenbin Yao <quic_wenbyao@quicinc.com>,
	vkoul@kernel.org, kishon@kernel.org, dmitry.baryshkov@linaro.org,
	abel.vesa@linaro.org, quic_qianyu@quicinc.com,
	neil.armstrong@linaro.org, manivannan.sadhasivam@linaro.org,
	quic_devipriy@quicinc.com, linux-arm-msm@vger.kernel.org,
	linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically
Date: Wed, 12 Feb 2025 13:31:14 +0100	[thread overview]
Message-ID: <93f1f01e-e6b4-4dc2-9485-aba168c6d88c@oss.qualcomm.com> (raw)
In-Reply-To: <6c38b6e028858662aa5d45f4a14b993860b73cc0.camel@pengutronix.de>

On 11.02.2025 10:53 AM, Philipp Zabel wrote:
> On Di, 2025-02-11 at 17:42 +0800, Wenbin Yao wrote:
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> Decide the in-driver logic based on whether the nocsr reset is present
>> and defer checking the appropriateness of that to dt-bindings to save
>> on boilerplate.
>>
>> Reset controller APIs are fine consuming a nullptr, so no additional
>> checks are necessary there.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> ---

[...]

>>  static void qmp_pcie_init_port_b(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tbls *tbls)
>> @@ -4203,11 +4196,14 @@ static int qmp_pcie_reset_init(struct qmp_pcie *qmp)
>>  	if (ret)
>>  		return dev_err_probe(dev, ret, "failed to get resets\n");
>>  
>> -	if (cfg->has_nocsr_reset) {
>> -		qmp->nocsr_reset = devm_reset_control_get_exclusive(dev, "phy_nocsr");
>> -		if (IS_ERR(qmp->nocsr_reset))
>> +	qmp->nocsr_reset = devm_reset_control_get_exclusive(dev, "phy_nocsr");
>> +	if (IS_ERR(qmp->nocsr_reset)) {
>> +		if (PTR_ERR(qmp->nocsr_reset) == -ENOENT ||
>> +		    PTR_ERR(qmp->nocsr_reset) == -EINVAL)
> 
> Why is -EINVAL ignored here?

If the NOCSR (partial) reset is missing, we can still assert the "full" reset
and program the hardware from the ground up. It's also needed for backwards
dt compat as not all platforms described it when originally added.

> Without this you could just use
> devm_reset_control_get_optional_exclusive(), which already turns -
> ENOENT into NULL. That seems to me the correct thing to do, as from
> driver point-of-view, this reset control is optional.

Good point, I forgot _optional_ was a thing in the reset framework

Konrad