Re: [PATCH v2 3/3] arm64: dts: qcom: qcm6490-fairphone-fp5: add ST21NFCD NFC

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The default pinctrl state applies `output-high` to the reset pin unconditionally at probe time, before the NFC chip's power supply (`vdd-io-supply`) is enabled, risking hardware latch-up.
- [Low] The `reset-gpios` polarity is specified as `GPIO_ACTIVE_HIGH` to work around inverted logic in the `st-nci` driver, resulting in an inaccurate hardware description.

Pre-existing issues:
- [High] The NCI device is registered and exposed to userspace before its state (the `raw_nci` flag and the IRQ handler) is fully initialized, creating race windows.
--

--- Patch [3]: [PATCH v2 3/3] arm64: dts: qcom: qcm6490-fairphone-fp5: add ST21NFCD NFC ---
commit 5a4863ae6956a4a982d319ea2fa7fb68012f0fdf
Author: Kristian Brox <[email protected]>

arm64: dts: qcom: qcm6490-fairphone-fp5: add ST21NFCD NFC

Enable the ST21NFCD on i2c9 (0x08), IRQ TLMM 41, reset TLMM 38
active-high. Compatible is st,st21nfcd (raw NCI).

SYS_CLK is LN_BB_CLK2. VPS_IO is L18B (vreg_l18b). ese-present and
uicc-present follow the public schematic (NFC_SWP1/SWP2: SWP_SE to
SIM1, SWP_UICC to SIM2). Reader path is tested; SE/HCE is not.

> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> index 04cb9230d29fd..c5d51826b6033 100644
> --- a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> +++ b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> @@ -908,7 +908,23 @@ redriver_ss_in: endpoint {
>  &i2c9 {
>  	status = "okay";
>  
> -	/* ST21NFC NFC @ 28 */
> +	nfc@8 {
> +		compatible = "st,st21nfcd";
> +		reg = <0x08>;
> +
> +		interrupts-extended = <&tlmm 41 IRQ_TYPE_LEVEL_HIGH>;
> +		reset-gpios = <&tlmm 38 GPIO_ACTIVE_HIGH>;

[Severity: Low]
Is this GPIO polarity specified as GPIO_ACTIVE_HIGH to work around inverted
logic in the st-nci driver?

The ST21NFCD physically uses an active-low reset pin. While specifying
GPIO_ACTIVE_HIGH compensates for the driver incorrectly handling it as an
active-high enable line, it forces the DT to describe the physical hardware
polarity incorrectly.

> +
> +		pinctrl-0 = <&nfc_int_default>, <&nfc_reset_default>;
> +		pinctrl-names = "default";
> +
> +		clocks = <&rpmhcc RPMH_LN_BB_CLK2>;
> +		vdd-io-supply = <&vreg_l18b>;

[ ... ]

> @@ -1410,6 +1426,21 @@ hall_sensor_default: hall-sensor-default-state {
>  		drive-strength = <2>;
>  		bias-pull-up;
>  	};
> +
> +	nfc_int_default: nfc-int-default-state {
> +		pins = "gpio41";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +	};
> +
> +	nfc_reset_default: nfc-reset-default-state {
> +		pins = "gpio38";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-high;
> +	};

[Severity: High]
Could this output-high configuration cause a hardware latch-up?

This applies the output-high state to the reset pin unconditionally at probe
time. Driving 1.8V into the pin before the NFC chip's vdd-io-supply is
enabled forward-biases ESD protection diodes, which can cause back-powering
and excessive current draw.

[Severity: High]
This is a pre-existing issue, but I wanted to mention a race condition in the
NCI device registration that might be aggravated by this patch.

In drivers/nfc/st-nci/i2c.c:st_nci_i2c_probe(), the NCI device is registered
and exposed to userspace before its state (the raw_nci flag and the IRQ
handler) is fully initialized:

drivers/nfc/st-nci/i2c.c:st_nci_i2c_probe() {
    ...
	phy->ndlc->raw_nci = phy->raw_nci;

	phy->irq_active = true;
	r = devm_request_threaded_irq(&client->dev, client->irq, NULL,
				st_nci_irq_thread_fn,
				IRQF_ONESHOT,
				ST_NCI_DRIVER_NAME, phy);
    ...
}

Userspace can react to the netlink device registration uevent and bring up the
device before the probe function finishes executing, which can lead to dropped
interrupts or misprocessing of NFC frames.

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