[PATCH v4 0/6] nfc: s3fwrn5: support the S3NRN4V variant

[PATCH v4 0/6] nfc: s3fwrn5: support the S3NRN4V variant

[Jorijn van der Graaf]

This adds support for the Samsung S3NRN4V, a later part in the Samsung
NCI NFC controller line the s3fwrn5 driver covers, found e.g. on the
Fairphone 6 (SM7635).

The S3NRN4V differs from the already-supported parts in three ways:
it ships with working firmware, and its bootloader reports a hardware
version the driver has no flash base address for (so the firmware
download step is skipped); it loads its RF calibration data through
a different proprietary command (DUAL_OPTION); and its reference
clock speed is set with the single-byte FW_CFG form the vendor HAL
uses on this generation.

Patches 1-2 are preparatory cleanups from the v2 review; patch 3 adds
the compatible to the binding and documents the PVDD supply; patch 4
makes the driver enable that supply; patch 5 implements the variant;
patch 6 adds the Fairphone 6 board nodes -- included so the hardware
can be tested, per David's v2 request; it depends on patch 3 and is
expected to be picked up via the qcom tree (linux-arm-msm) once the
binding has landed.

Tested on a Fairphone 6 running a milos-mainline kernel: reader mode
polls and reads ISO 14443-4 tags reliably, from a fresh boot and
across driver reloads, with the calibration files present and also
absent (the chip keeps its stored calibration and still reads).
The v4 driver and board nodes were re-verified the same way. The
test kernel is v7.1.2-based and predates this base's net/nfc/nci
fixes: response/notification parse hardening (rsp.c, ntf.c) and a
data-completion race fix (data.c), none changing behaviour for
well-formed traffic; the rest of the delta (a conn-close leak fix
for dynamic NCI connections this driver never creates, LLCP,
nfc-digital, the UART transport) is off the NCI/I2C path this
driver uses.

Luca, you were right about the clock: the schematic net into the
chip's XI is PMK_CLK7_LN_NFC. On your "always on anyways" half: not
quite, but not simply our vote either -- v3 voted RPMH_RF_CLK2 and
still read tags, and only with no clocks property at all does the
chip stop activating tags. A clock vote is required; what keeps
that buffer running when the vote names a different clock is not
something we have identified. RPMH_LN_BB_CLK2 is independently
sufficient on hardware. The two questions from the v2 thread
(calibration-table variance, linux-firmware submission) are still
open; nothing here depends on them.

David, the calibration scheme follows our v2 exchange: fixed
chip-scoped paths, with a DT table selector deferred until a second
table set exists. If you'd rather have the selector now
(samsung,calibration-variant, or the generic firmware-name), say so
and v5 will add it.

The series is based on the nfc tree's for-next.

Changes in v4:
 - Drop the clk-req-gpios property and the driver's clock-gating
   machinery: on-device A/B testing showed the machinery has no
   effect -- the chip reads tags with the clock gated on CLK_REQ,
   with it left always-on, and with the CLK_REQ pad described in
   pinctrl or left undescribed. The S3NRN4V uses the existing
   always-on clock path.
 - Drop Conor's Acked-by on the binding patch: clk-req-gpios, its
   dependencies clause and the S3NRN4V example are gone (rationale in
   that patch's notes).
 - Reference the buffer the chip is wired to: RPMH_LN_BB_CLK2, not
   RPMH_RF_CLK2 (Luca); verified on hardware that the reference is
   load-bearing (no clocks reference -> no tag activation).
 - New patch 4: enable the PVDD supply, before any pin or clock of
   the chip is touched, so the property the binding requires is
   actually consumed (flagged by the Sashiko AI review of v3).
 - Send FW_CFG from the ->init hook, before CORE_RESET, matching the
   vendor stack's ordering (v3 wrongly called ->setup the earliest
   hook available); verified on hardware that the command is
   required (without it the chip stops answering RF_DISCOVER and
   never reads a tag).
 - Allocate the merged calibration stream with kvmalloc(); reject an
   empty blob pair up front. Abort the update session if a section
   transfer fails, and version-gate the upload on DUAL_OPTION
   GET_VER -- both as the vendor stack does.
 - Declare the calibration blobs with MODULE_FIRMWARE(); log an
   actual calibration update at dev_info, like the legacy path.
 - Correct why the firmware download is skipped: the bootloader does
   answer GET_BOOTINFO, but reports a hardware version the driver
   has no flash base address for (v1-v3 said the bootloader protocol
   was unimplemented; tested).
 - State what the FW_CFG selector means: the vendor's own test tool
   decodes 0x11 as the 19.2 MHz reference (v3 called the encoding
   undocumented).
 - Board DTS style (Konrad): interrupts-extended, grouped -gpios
   properties, one nfc-default-state with per-function sub-nodes, and
   the two node comments dropped -- their surviving substance is in
   the commit message, the CLK_REQ half retired with the machinery
   (also Luca).

Changes in v3 (full list in the lore thread):
 - Calibration data handling reworked per David's review: chip-scoped
   samsung/s3nrn4v/{hwreg,swreg}.bin paths, "RF calibration data"
   naming, firmware_request_nowarn(), calibration-variant property
   deferred until a device needs a second table set.
 - New patches 1-2 (of_match_ptr drop, driver-name string literal),
   new board DTS patch; rebased onto the nfc tree's for-next.

v3: https://lore.kernel.org/20260726222405.153008-1-jorijnvdgraaf@catcrafts.net
v2: https://lore.kernel.org/20260705190621.128257-1-jorijnvdgraaf@catcrafts.net
v1: https://lore.kernel.org/20260703202601.78563-1-jorijnvdgraaf@catcrafts.net

Jorijn van der Graaf (6):
  nfc: s3fwrn5: drop of_match_ptr() from the I2C driver
  nfc: s3fwrn5: use the "s3fwrn5_i2c" string literal directly
  dt-bindings: net: nfc: samsung,s3fwrn5: add the S3NRN4V
  nfc: s3fwrn5: enable the PVDD supply
  nfc: s3fwrn5: support the S3NRN4V variant
  arm64: dts: qcom: milos-fairphone-fp6: Add NFC

 .../bindings/net/nfc/samsung,s3fwrn5.yaml     |  19 +-
 .../boot/dts/qcom/milos-fairphone-fp6.dts     |  33 +++-
 drivers/nfc/s3fwrn5/core.c                    |  34 +++-
 drivers/nfc/s3fwrn5/i2c.c                     |  26 ++-
 drivers/nfc/s3fwrn5/nci.c                     | 179 +++++++++++++++++-
 drivers/nfc/s3fwrn5/nci.h                     |  23 ++-
 drivers/nfc/s3fwrn5/s3fwrn5.h                 |  14 +-
 drivers/nfc/s3fwrn5/uart.c                    |   2 +-
 8 files changed, 314 insertions(+), 16 deletions(-)


base-commit: f0054d864e6e734add71e29961f5ae15211ad24d
-- 
2.55.0

[PATCH v4 1/6] nfc: s3fwrn5: drop of_match_ptr() from the I2C driver

[Jorijn van der Graaf]

Since commit 9739e07fdb29 ("nfc: Drop __maybe_unused from of_device_id
tables") the OF match table relies on its MODULE_DEVICE_TABLE()
reference to count as used, while of_match_ptr() still discards the
.of_match_table reference on !CONFIG_OF builds. Drop of_match_ptr() so
every configuration uses the same code; the S3NRN4V support added later
in this series carries its variant selector in this table's match data.

Assisted-by: Claude:claude-fable-5
Assisted-by: Claude:claude-opus-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
 drivers/nfc/s3fwrn5/i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
index 4ba762611711..cb73993446e0 100644
--- a/drivers/nfc/s3fwrn5/i2c.c
+++ b/drivers/nfc/s3fwrn5/i2c.c
@@ -219,7 +219,7 @@ MODULE_DEVICE_TABLE(of, of_s3fwrn5_i2c_match);
 static struct i2c_driver s3fwrn5_i2c_driver = {
 	.driver = {
 		.name = S3FWRN5_I2C_DRIVER_NAME,
-		.of_match_table = of_match_ptr(of_s3fwrn5_i2c_match),
+		.of_match_table = of_s3fwrn5_i2c_match,
 	},
 	.probe = s3fwrn5_i2c_probe,
 	.remove = s3fwrn5_i2c_remove,
-- 
2.55.0

[PATCH v4 2/6] nfc: s3fwrn5: use the "s3fwrn5_i2c" string literal directly

[Jorijn van der Graaf]

The S3FWRN5_I2C_DRIVER_NAME define is an indirection for a string that
is fixed ABI (the driver name) and only used within this file. Spell
the string out at its three users and drop the define.

Suggested-by: David Heidelberg <david@ixit.cz>
Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
 drivers/nfc/s3fwrn5/i2c.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
index cb73993446e0..c528c085b43e 100644
--- a/drivers/nfc/s3fwrn5/i2c.c
+++ b/drivers/nfc/s3fwrn5/i2c.c
@@ -16,8 +16,6 @@
 
 #include "phy_common.h"
 
-#define S3FWRN5_I2C_DRIVER_NAME "s3fwrn5_i2c"
-
 struct s3fwrn5_i2c_phy {
 	struct phy_common common;
 	struct i2c_client *i2c_dev;
@@ -186,7 +184,7 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client)
 
 	ret = devm_request_threaded_irq(&client->dev, phy->i2c_dev->irq, NULL,
 		s3fwrn5_i2c_irq_thread_fn, IRQF_ONESHOT,
-		S3FWRN5_I2C_DRIVER_NAME, phy);
+		"s3fwrn5_i2c", phy);
 	if (ret)
 		goto s3fwrn5_remove;
 
@@ -205,7 +203,7 @@ static void s3fwrn5_i2c_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id s3fwrn5_i2c_id_table[] = {
-	{ .name = S3FWRN5_I2C_DRIVER_NAME },
+	{ .name = "s3fwrn5_i2c" },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, s3fwrn5_i2c_id_table);
@@ -218,7 +216,7 @@ MODULE_DEVICE_TABLE(of, of_s3fwrn5_i2c_match);
 
 static struct i2c_driver s3fwrn5_i2c_driver = {
 	.driver = {
-		.name = S3FWRN5_I2C_DRIVER_NAME,
+		.name = "s3fwrn5_i2c",
 		.of_match_table = of_s3fwrn5_i2c_match,
 	},
 	.probe = s3fwrn5_i2c_probe,
-- 
2.55.0

Re: [PATCH v4 2/6] nfc: s3fwrn5: use the "s3fwrn5_i2c" string literal directly

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing endianness conversions when parsing firmware headers and offsets, leading to failures on Big Endian architectures.
--

--- Patch [2]: [PATCH v4 2/6] nfc: s3fwrn5: use the "s3fwrn5_i2c" string literal directly ---
commit 9d4d91e92081c012d8837ad241bfc3be79ee322a
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

nfc: s3fwrn5: use the "s3fwrn5_i2c" string literal directly

This commit replaces the S3FWRN5_I2C_DRIVER_NAME define with the string
literal "s3fwrn5_i2c" directly in the driver. The define was an indirection
for a fixed ABI name and was only used locally within this file.

> diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
> index cb73993446e09..c528c085b43ef 100644
> --- a/drivers/nfc/s3fwrn5/i2c.c
> +++ b/drivers/nfc/s3fwrn5/i2c.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but can this code cause failures on Big Endian
architectures due to missing endianness conversions?

In s3fwrn5_i2c_read(), the 16-bit length field is read directly in native
byte order:

drivers/nfc/s3fwrn5/i2c.c:s3fwrn5_i2c_read() {
    ...
    data_len = (phy->common.mode == S3FWRN5_MODE_NCI) ?
            ((struct nci_ctrl_hdr *)hdr)->plen :
            ((struct s3fwrn5_fw_header *)hdr)->len;
    ...
}

Could this improperly byte-swap on Big Endian systems, leading to oversized
allocations in alloc_skb() and invalid I2C reads?

Additionally, in s3fwrn5_fw_request_firmware(), 32-bit offsets are copied
directly from the little-endian firmware payload into native u32 variables
without using le32_to_cpu():

drivers/nfc/s3fwrn5/firmware.c:s3fwrn5_fw_request_firmware() {
    ...
    memcpy(&sig_off, fw->fw->data + 0x14, 4);
    fw->sig = fw->fw->data + sig_off;
    ...
}

Since sig_off is byte-swapped to a massive value on Big Endian systems, does
this result in an out-of-bounds pointer calculation?

When s3fwrn5_fw_enter_update_mode() later calls s3fwrn5_fw_prep_msg(), could
copying from this fw->sig pointer trigger an unmapped memory read and
kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807021001.239389-1-jorijnvdgraaf@catcrafts.net?part=2

[PATCH v4 3/6] dt-bindings: net: nfc: samsung,s3fwrn5: add the S3NRN4V

[Jorijn van der Graaf]

The S3NRN4V is a later part in the Samsung NCI NFC controller line
this binding documents, found e.g. on the Fairphone 6 (SM7635). Its
host interface is NCI over I2C like the S3FWRN5, but it is not
compatible with any of the existing parts, so no fallback compatible
applies: its bootloader reports a hardware version none of the
existing parts use, and its RF configuration is loaded through a
different proprietary command set.

Also document the PVDD supply, the externally switched rail powering
the controller (boards feed it from a PMIC LDO). It is required for the
new device; deployed DTs for the existing parts never described a
supply, so for those it stays optional.

Assisted-by: Claude:claude-opus-4-8
Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
Conor's Acked-by, given on v2 and carried into v3, is dropped: this
version removes clk-req-gpios (and its dependencies clause) and the
S3NRN4V example. The compatible and pvdd-supply hunks are unchanged
from the acked text.

 .../bindings/net/nfc/samsung,s3fwrn5.yaml     | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml b/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
index 12baee45752c..40a69466802b 100644
--- a/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
+++ b/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
@@ -14,6 +14,7 @@ properties:
     enum:
       - samsung,s3fwrn5-i2c
       - samsung,s3fwrn82
+      - samsung,s3nrn4v
 
   en-gpios:
     maxItems: 1
@@ -29,6 +30,9 @@ properties:
   clocks:
     maxItems: 1
 
+  pvdd-supply:
+    description: PVDD power supply
+
   wake-gpios:
     maxItems: 1
     description:
@@ -58,12 +62,25 @@ allOf:
       properties:
         compatible:
           contains:
-            const: samsung,s3fwrn5-i2c
+            enum:
+              - samsung,s3fwrn5-i2c
+              - samsung,s3nrn4v
     then:
       required:
         - interrupts
         - reg
 
+  # Deployed DTs for the older parts never described a supply, so PVDD is
+  # only required for the new device.
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: samsung,s3nrn4v
+    then:
+      required:
+        - pvdd-supply
+
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
-- 
2.55.0

[PATCH v4 4/6] nfc: s3fwrn5: enable the PVDD supply

[Jorijn van der Graaf]

The controller is powered by an externally switched PVDD rail. Get and
enable it in the i2c phy probe, before any of the chip's pins is driven
or its clock started, so the chip's power does not depend on whatever
state the bootloader or platform left the rail in. Boards that do not
describe the supply resolve to a dummy regulator, so the existing users
keep working; they gain the regulator core's dummy-supply warning until
their DTs describe the rail.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
 drivers/nfc/s3fwrn5/i2c.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
index c528c085b43e..f93d8b47b65e 100644
--- a/drivers/nfc/s3fwrn5/i2c.c
+++ b/drivers/nfc/s3fwrn5/i2c.c
@@ -11,6 +11,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/regulator/consumer.h>
 
 #include <net/nfc/nfc.h>
 
@@ -158,6 +159,11 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client)
 	phy->i2c_dev = client;
 	i2c_set_clientdata(client, phy);
 
+	ret = devm_regulator_get_enable(&client->dev, "pvdd");
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to enable pvdd\n");
+
 	phy->common.gpio_en = devm_gpiod_get(&client->dev, "en", GPIOD_OUT_HIGH);
 	if (IS_ERR(phy->common.gpio_en))
 		return PTR_ERR(phy->common.gpio_en);
-- 
2.55.0

Re: [PATCH v4 4/6] nfc: s3fwrn5: enable the PVDD supply

[sashiko-bot]

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

Pre-existing issues:
- [High] The NCI device is registered and exposed to userspace before its IRQ handler is requested, creating a race condition.
--

--- Patch [4]: [PATCH v4 4/6] nfc: s3fwrn5: enable the PVDD supply ---
commit 3e46e47e27236d4ca640efa94f0c2940f401ccd7
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

nfc: s3fwrn5: enable the PVDD supply

This commit retrieves and enables the PVDD supply in the I2C PHY probe
function. This ensures the controller is powered before any of its pins
are driven or its clock is started.

> diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
> index c528c085b43ef..f93d8b47b65e3 100644
> --- a/drivers/nfc/s3fwrn5/i2c.c
> +++ b/drivers/nfc/s3fwrn5/i2c.c

[ ... ]

> @@ -158,6 +159,11 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client)
>  	phy->i2c_dev = client;
>  	i2c_set_clientdata(client, phy);
>  
> +	ret = devm_regulator_get_enable(&client->dev, "pvdd");
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to enable pvdd\n");
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this function expose
the device to userspace before the IRQ handler is set up?

Looking further down in s3fwrn5_i2c_probe():

	ret = s3fwrn5_probe(&phy->common.ndev, phy, &phy->i2c_dev->dev,
			    &i2c_phy_ops, variant);
	if (ret < 0)
		return ret;

	ret = devm_request_threaded_irq(&client->dev, phy->i2c_dev->irq, NULL,
		s3fwrn5_i2c_irq_thread_fn, IRQF_ONESHOT,
		"s3fwrn5_i2c", phy);

If userspace (like udev or neard) immediately opens the NCI device and
starts sending commands after s3fwrn5_probe() returns, can the hardware
generate an interrupt that is missed because devm_request_threaded_irq()
has not been called yet?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807021001.239389-1-jorijnvdgraaf@catcrafts.net?part=4

[PATCH v4 5/6] nfc: s3fwrn5: support the S3NRN4V variant

[Jorijn van der Graaf]

The S3NRN4V (e.g. on the Fairphone 6, SM7635) is a later part in the
Samsung NFC controller line this driver covers. It needs different
bring-up, selected with a new samsung,s3nrn4v compatible:

 - It ships with working firmware, and its bootloader reports a
   hardware version this driver has no flash base address for, so the
   firmware download step is skipped. Its RF calibration data is
   loaded with the proprietary DUAL_OPTION command (the HW and SW
   blobs merged into a single stream) instead of the
   START/SET/STOP_RFREG sequence. The upload is version-gated on
   DUAL_OPTION GET_VER, matching the vendor stack: GET_VER reports
   the chip's current calibration versions, and the push is skipped
   when both match the blobs (each blob carries its version in its
   last 16 bytes). post_setup runs on every device-up, so the
   gate is what keeps the chip's calibration store from being rewritten
   each time NFC is enabled.

 - Its reference clock speed is configured with the single-byte FW_CFG
   form, sent from the ->init hook, before CORE_RESET, in the same
   order the vendor stack sends it. The vendor HAL uses the
   single-byte form for this generation, and its vendor configuration
   sets the selector to 0x11, which selects the 19.2 MHz reference.
   Without this command the chip completes NCI initialization but
   goes silent on RF_DISCOVER -- the command times out unanswered and
   polling never starts (verified on hardware).

The calibration data is requested from a chip-scoped directory,
samsung/s3nrn4v/hwreg.bin and samsung/s3nrn4v/swreg.bin, rather than
the flat sec_s3fwrn5_rfreg.bin name the older parts use.

The variant is carried as match data by both the OF and the I2C device
id tables so the two match paths agree.

Existing S3FWRN5 / S3FWRN82 setups keep the firmware-download path and
the legacy rfreg sequence, unchanged.

Assisted-by: Claude:claude-opus-4-8
Assisted-by: Claude:claude-fable-5
Assisted-by: Claude:claude-opus-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
The upload deliberately does not share a helper with
s3fwrn5_nci_rf_configure(): the framing differs (sub-OID prefix
byte, one merged stream vs per-file sections) and the error policy
differs (abort STOP on a failed section), so a common helper would
carry more branches than it saves.

FW_CFG is sent from ->init rather than from within
s3fwrn5_nci_rf_configure_dual() so the clock configuration still runs
when the calibration files are absent (the upload path returns early
there; the reference clock must not depend on it).

 drivers/nfc/s3fwrn5/core.c    |  34 ++++++-
 drivers/nfc/s3fwrn5/i2c.c     |  12 ++-
 drivers/nfc/s3fwrn5/nci.c     | 179 +++++++++++++++++++++++++++++++++-
 drivers/nfc/s3fwrn5/nci.h     |  23 ++++-
 drivers/nfc/s3fwrn5/s3fwrn5.h |  14 ++-
 drivers/nfc/s3fwrn5/uart.c    |   2 +-
 6 files changed, 256 insertions(+), 8 deletions(-)

diff --git a/drivers/nfc/s3fwrn5/core.c b/drivers/nfc/s3fwrn5/core.c
index af0fa8bd970b..563470cb928d 100644
--- a/drivers/nfc/s3fwrn5/core.c
+++ b/drivers/nfc/s3fwrn5/core.c
@@ -20,6 +20,9 @@
 				NFC_PROTO_ISO14443_B_MASK | \
 				NFC_PROTO_ISO15693_MASK)
 
+#define S3NRN4V_HWREG_NAME	"samsung/s3nrn4v/hwreg.bin"
+#define S3NRN4V_SWREG_NAME	"samsung/s3nrn4v/swreg.bin"
+
 static int s3fwrn5_firmware_init(struct s3fwrn5_info *info)
 {
 	struct s3fwrn5_fw_info *fw_info = &info->fw_info;
@@ -74,6 +77,22 @@ static int s3fwrn5_firmware_update(struct s3fwrn5_info *info)
 	return ret;
 }
 
+static int s3fwrn5_nci_init(struct nci_dev *ndev)
+{
+	struct s3fwrn5_info *info = nci_get_drvdata(ndev);
+	int ret = 0;
+
+	if (info->variant == S3FWRN5_VARIANT_S3NRN4V) {
+		ret = s3fwrn5_nci_clk_cfg(info);
+		if (ret < 0)
+			dev_err(&ndev->nfc_dev->dev,
+				"failed to configure the reference clock: %d\n",
+				ret);
+	}
+
+	return ret;
+}
+
 static int s3fwrn5_nci_open(struct nci_dev *ndev)
 {
 	struct s3fwrn5_info *info = nci_get_drvdata(ndev);
@@ -127,6 +146,15 @@ static int s3fwrn5_nci_post_setup(struct nci_dev *ndev)
 	struct s3fwrn5_info *info = nci_get_drvdata(ndev);
 	int ret;
 
+	if (info->variant == S3FWRN5_VARIANT_S3NRN4V) {
+		ret = s3fwrn5_nci_rf_configure_dual(info, S3NRN4V_HWREG_NAME,
+						    S3NRN4V_SWREG_NAME);
+		if (ret < 0)
+			dev_warn(&ndev->nfc_dev->dev,
+				 "RF calibration data update failed: %d\n", ret);
+		return 0;
+	}
+
 	if (s3fwrn5_firmware_init(info)) {
 		//skip bootloader mode
 		return 0;
@@ -149,6 +177,7 @@ static int s3fwrn5_nci_post_setup(struct nci_dev *ndev)
 }
 
 static const struct nci_ops s3fwrn5_nci_ops = {
+	.init = s3fwrn5_nci_init,
 	.open = s3fwrn5_nci_open,
 	.close = s3fwrn5_nci_close,
 	.send = s3fwrn5_nci_send,
@@ -158,7 +187,7 @@ static const struct nci_ops s3fwrn5_nci_ops = {
 };
 
 int s3fwrn5_probe(struct nci_dev **ndev, void *phy_id, struct device *pdev,
-	const struct s3fwrn5_phy_ops *phy_ops)
+	const struct s3fwrn5_phy_ops *phy_ops, enum s3fwrn5_variant variant)
 {
 	struct s3fwrn5_info *info;
 	int ret;
@@ -170,6 +199,7 @@ int s3fwrn5_probe(struct nci_dev **ndev, void *phy_id, struct device *pdev,
 	info->phy_id = phy_id;
 	info->pdev = pdev;
 	info->phy_ops = phy_ops;
+	info->variant = variant;
 	mutex_init(&info->mutex);
 
 	s3fwrn5_set_mode(info, S3FWRN5_MODE_COLD);
@@ -225,3 +255,5 @@ EXPORT_SYMBOL(s3fwrn5_recv_frame);
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Samsung S3FWRN5 NFC driver");
 MODULE_AUTHOR("Robert Baldyga <r.baldyga@samsung.com>");
+MODULE_FIRMWARE(S3NRN4V_HWREG_NAME);
+MODULE_FIRMWARE(S3NRN4V_SWREG_NAME);
diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
index f93d8b47b65e..884420ac83b5 100644
--- a/drivers/nfc/s3fwrn5/i2c.c
+++ b/drivers/nfc/s3fwrn5/i2c.c
@@ -145,6 +145,7 @@ static irqreturn_t s3fwrn5_i2c_irq_thread_fn(int irq, void *phy_id)
 
 static int s3fwrn5_i2c_probe(struct i2c_client *client)
 {
+	enum s3fwrn5_variant variant;
 	struct s3fwrn5_i2c_phy *phy;
 	int ret;
 
@@ -183,8 +184,9 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client)
 		return dev_err_probe(&client->dev, PTR_ERR(phy->clk),
 				     "failed to get clock\n");
 
+	variant = (uintptr_t)i2c_get_match_data(client);
 	ret = s3fwrn5_probe(&phy->common.ndev, phy, &phy->i2c_dev->dev,
-			    &i2c_phy_ops);
+			    &i2c_phy_ops, variant);
 	if (ret < 0)
 		return ret;
 
@@ -209,13 +211,17 @@ static void s3fwrn5_i2c_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id s3fwrn5_i2c_id_table[] = {
-	{ .name = "s3fwrn5_i2c" },
+	{ .name = "s3fwrn5_i2c", .driver_data = S3FWRN5_VARIANT_FWDL },
+	{ .name = "s3nrn4v", .driver_data = S3FWRN5_VARIANT_S3NRN4V },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, s3fwrn5_i2c_id_table);
 
 static const struct of_device_id of_s3fwrn5_i2c_match[] = {
-	{ .compatible = "samsung,s3fwrn5-i2c" },
+	{ .compatible = "samsung,s3fwrn5-i2c",
+	  .data = (void *)S3FWRN5_VARIANT_FWDL },
+	{ .compatible = "samsung,s3nrn4v",
+	  .data = (void *)S3FWRN5_VARIANT_S3NRN4V },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, of_s3fwrn5_i2c_match);
diff --git a/drivers/nfc/s3fwrn5/nci.c b/drivers/nfc/s3fwrn5/nci.c
index 5a9de11bbece..f1877797d136 100644
--- a/drivers/nfc/s3fwrn5/nci.c
+++ b/drivers/nfc/s3fwrn5/nci.c
@@ -8,6 +8,9 @@
 
 #include <linux/completion.h>
 #include <linux/firmware.h>
+#include <linux/minmax.h>
+#include <linux/slab.h>
+#include <linux/unaligned.h>
 
 #include "s3fwrn5.h"
 #include "nci.h"
@@ -20,7 +23,24 @@ static int s3fwrn5_nci_prop_rsp(struct nci_dev *ndev, struct sk_buff *skb)
 	return 0;
 }
 
-const struct nci_driver_ops s3fwrn5_nci_prop_ops[4] = {
+/*
+ * DUAL_OPTION responses are not uniform: GET_VER answers with the chip's
+ * calibration versions instead of a status byte, so stash the payload for
+ * the caller before completing the request.
+ */
+static int s3fwrn5_nci_dual_rsp(struct nci_dev *ndev, struct sk_buff *skb)
+{
+	struct s3fwrn5_info *info = nci_get_drvdata(ndev);
+
+	info->dual_rsp_len = min_t(unsigned int, skb->len,
+				   sizeof(info->dual_rsp));
+	memcpy(info->dual_rsp, skb->data, info->dual_rsp_len);
+
+	nci_req_complete(ndev, skb->data[0]);
+	return 0;
+}
+
+const struct nci_driver_ops s3fwrn5_nci_prop_ops[5] = {
 	{
 		.opcode = nci_opcode_pack(NCI_GID_PROPRIETARY,
 				NCI_PROP_SET_RFREG),
@@ -41,6 +61,11 @@ const struct nci_driver_ops s3fwrn5_nci_prop_ops[4] = {
 				NCI_PROP_FW_CFG),
 		.rsp = s3fwrn5_nci_prop_rsp,
 	},
+	{
+		.opcode = nci_opcode_pack(NCI_GID_PROPRIETARY,
+				NCI_PROP_DUAL_OPTION),
+		.rsp = s3fwrn5_nci_dual_rsp,
+	},
 };
 
 #define S3FWRN5_RFREG_SECTION_SIZE 252
@@ -117,3 +142,155 @@ int s3fwrn5_nci_rf_configure(struct s3fwrn5_info *info, const char *fw_name)
 	release_firmware(fw);
 	return ret;
 }
+
+/*
+ * The S3NRN4V expects the single-byte FW_CFG form (just the clock-speed
+ * selector).
+ */
+int s3fwrn5_nci_clk_cfg(struct s3fwrn5_info *info)
+{
+	u8 clk_speed = NCI_PROP_FW_CFG_CLK_SPEED;
+
+	return nci_prop_cmd(info->ndev, NCI_PROP_FW_CFG, 1, &clk_speed);
+}
+
+/*
+ * An 8-byte calibration version: 5 bytes of date stamp and a 3-byte CSC
+ * code, at fixed offsets both in a blob's 16-byte tail and in each half of
+ * the GET_VER response (HW at offset 0, SW at offset 15).
+ */
+static bool s3fwrn5_nci_dual_version_eq(const u8 *a, const u8 *b)
+{
+	return !memcmp(a + 5, b + 5, 5) && !memcmp(a + 12, b + 12, 3);
+}
+
+static bool s3fwrn5_nci_dual_cal_current(struct s3fwrn5_info *info,
+					 const struct firmware *hw_fw,
+					 const struct firmware *sw_fw)
+{
+	if (info->dual_rsp_len < 30)
+		return false;
+	if (hw_fw->size < 16 || sw_fw->size < 16)
+		return false;
+
+	return s3fwrn5_nci_dual_version_eq(info->dual_rsp,
+					   hw_fw->data + hw_fw->size - 16) &&
+	       s3fwrn5_nci_dual_version_eq(info->dual_rsp + 15,
+					   sw_fw->data + sw_fw->size - 16);
+}
+
+/*
+ * S3NRN4V RF calibration data update: the HW and SW blobs merged into one
+ * stream (HW first), pushed as START_UPDATE, one SET_OPTION per 252-byte
+ * section, then STOP_UPDATE carrying a 16-bit checksum (running sum of the
+ * merged stream as 32-bit words).
+ */
+int s3fwrn5_nci_rf_configure_dual(struct s3fwrn5_info *info,
+				  const char *hw_name, const char *sw_name)
+{
+	struct nci_prop_dual_set_option_cmd set_option;
+	struct device *dev = &info->ndev->nfc_dev->dev;
+	const struct firmware *hw_fw, *sw_fw;
+	size_t merged_size, i, len;
+	u8 *merged = NULL;
+	u8 stop_cmd[3];
+	u32 checksum;
+	u8 sub_oid;
+	int ret;
+
+	ret = firmware_request_nowarn(&hw_fw, hw_name, dev);
+	if (ret < 0)
+		return ret;
+	ret = firmware_request_nowarn(&sw_fw, sw_name, dev);
+	if (ret < 0)
+		goto out_hw;
+
+	merged_size = hw_fw->size + sw_fw->size;
+
+	/*
+	 * The stream is checksummed as 32-bit words and pushed in at most 256
+	 * sections (the section index is a single byte); reject blobs that
+	 * would silently break either.
+	 */
+	if (!merged_size || merged_size % 4 ||
+	    merged_size > 256 * NCI_PROP_DUAL_SECTION_SIZE) {
+		dev_err(dev, "invalid calibration data size: %zu\n", merged_size);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/*
+	 * Ask the chip for its current calibration versions and skip the
+	 * upload when both already match the blobs; a mismatch or an
+	 * unparseable answer means the upload proceeds. GET_VER answers with
+	 * versions, not a status byte, so nci_prop_cmd()'s return carries no
+	 * meaning here.
+	 */
+	sub_oid = NCI_PROP_DUAL_SUB_GET_VER;
+	info->dual_rsp_len = 0;
+	nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 1, &sub_oid);
+	if (s3fwrn5_nci_dual_cal_current(info, hw_fw, sw_fw)) {
+		dev_dbg(dev, "calibration data already current\n");
+		ret = 0;
+		goto out;
+	}
+
+	merged = kvmalloc(merged_size, GFP_KERNEL);
+	if (!merged) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	memcpy(merged, hw_fw->data, hw_fw->size);
+	memcpy(merged + hw_fw->size, sw_fw->data, sw_fw->size);
+
+	checksum = 0;
+	for (i = 0; i + 4 <= merged_size; i += 4)
+		checksum += get_unaligned_le32(merged + i);
+
+	dev_info(dev, "calibration data update: %s + %s\n", hw_name, sw_name);
+
+	/* START_UPDATE */
+	sub_oid = NCI_PROP_DUAL_SUB_START_UPDATE;
+	ret = nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 1, &sub_oid);
+	if (ret < 0) {
+		dev_err(dev, "Unable to start calibration data update\n");
+		goto out;
+	}
+
+	/* SET_OPTION per section */
+	set_option.sub_oid = NCI_PROP_DUAL_SUB_SET_OPTION;
+	set_option.index = 0;
+	for (i = 0; i < merged_size; i += NCI_PROP_DUAL_SECTION_SIZE) {
+		len = min_t(size_t, merged_size - i, NCI_PROP_DUAL_SECTION_SIZE);
+		memcpy(set_option.data, merged + i, len);
+		ret = nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION,
+				   len + 2, (__u8 *)&set_option);
+		if (ret < 0) {
+			dev_err(dev, "calibration data update error: %d\n",
+				ret);
+			/* Abort form: STOP_UPDATE with the sub-OID alone. */
+			sub_oid = NCI_PROP_DUAL_SUB_STOP_UPDATE;
+			nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 1,
+				     &sub_oid);
+			goto out;
+		}
+		set_option.index++;
+	}
+
+	/* STOP_UPDATE with checksum */
+	stop_cmd[0] = NCI_PROP_DUAL_SUB_STOP_UPDATE;
+	put_unaligned_le16(checksum, &stop_cmd[1]);
+	ret = nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 3, stop_cmd);
+	if (ret < 0) {
+		dev_err(dev, "Unable to stop calibration data update\n");
+		goto out;
+	}
+
+	dev_info(dev, "calibration data update: success\n");
+out:
+	kvfree(merged);
+	release_firmware(sw_fw);
+out_hw:
+	release_firmware(hw_fw);
+	return ret;
+}
diff --git a/drivers/nfc/s3fwrn5/nci.h b/drivers/nfc/s3fwrn5/nci.h
index bc4bce2bbc4d..453a1c9d65eb 100644
--- a/drivers/nfc/s3fwrn5/nci.h
+++ b/drivers/nfc/s3fwrn5/nci.h
@@ -40,6 +40,9 @@ struct nci_prop_stop_rfreg_rsp {
 
 #define NCI_PROP_FW_CFG		0x28
 
+/* S3NRN4V single-byte FW_CFG payload; 0x11 selects the 19.2 MHz reference. */
+#define NCI_PROP_FW_CFG_CLK_SPEED	0x11
+
 struct nci_prop_fw_cfg_cmd {
 	__u8 clk_type;
 	__u8 clk_speed;
@@ -50,7 +53,25 @@ struct nci_prop_fw_cfg_rsp {
 	__u8 status;
 };
 
-extern const struct nci_driver_ops s3fwrn5_nci_prop_ops[4];
+#define NCI_PROP_DUAL_OPTION		0x2a
+
+#define NCI_PROP_DUAL_SUB_GET_VER	0x00
+#define NCI_PROP_DUAL_SUB_START_UPDATE	0x01
+#define NCI_PROP_DUAL_SUB_SET_OPTION	0x02
+#define NCI_PROP_DUAL_SUB_STOP_UPDATE	0x03
+
+#define NCI_PROP_DUAL_SECTION_SIZE	252
+
+struct nci_prop_dual_set_option_cmd {
+	__u8 sub_oid;
+	__u8 index;
+	__u8 data[NCI_PROP_DUAL_SECTION_SIZE];
+};
+
+extern const struct nci_driver_ops s3fwrn5_nci_prop_ops[5];
 int s3fwrn5_nci_rf_configure(struct s3fwrn5_info *info, const char *fw_name);
+int s3fwrn5_nci_rf_configure_dual(struct s3fwrn5_info *info,
+				  const char *hw_name, const char *sw_name);
+int s3fwrn5_nci_clk_cfg(struct s3fwrn5_info *info);
 
 #endif /* __LOCAL_S3FWRN5_NCI_H_ */
diff --git a/drivers/nfc/s3fwrn5/s3fwrn5.h b/drivers/nfc/s3fwrn5/s3fwrn5.h
index 2b492236090b..63f679f17166 100644
--- a/drivers/nfc/s3fwrn5/s3fwrn5.h
+++ b/drivers/nfc/s3fwrn5/s3fwrn5.h
@@ -21,6 +21,13 @@ enum s3fwrn5_mode {
 	S3FWRN5_MODE_FW,
 };
 
+enum s3fwrn5_variant {
+	/* S3FWRN5 / S3FWRN82: firmware is downloaded by this driver */
+	S3FWRN5_VARIANT_FWDL,
+	/* S3NRN4V: no firmware download; FW_CFG + DUAL_OPTION bring-up */
+	S3FWRN5_VARIANT_S3NRN4V,
+};
+
 struct s3fwrn5_phy_ops {
 	void (*set_wake)(void *id, bool sleep);
 	void (*set_mode)(void *id, enum s3fwrn5_mode);
@@ -36,6 +43,11 @@ struct s3fwrn5_info {
 	const struct s3fwrn5_phy_ops *phy_ops;
 
 	struct s3fwrn5_fw_info fw_info;
+	enum s3fwrn5_variant variant;
+
+	/* Last DUAL_OPTION response payload (GET_VER carries versions). */
+	u8 dual_rsp[32];
+	u8 dual_rsp_len;
 
 	struct mutex mutex;
 };
@@ -78,7 +90,7 @@ static inline int s3fwrn5_write(struct s3fwrn5_info *info, struct sk_buff *skb)
 }
 
 int s3fwrn5_probe(struct nci_dev **ndev, void *phy_id, struct device *pdev,
-	const struct s3fwrn5_phy_ops *phy_ops);
+	const struct s3fwrn5_phy_ops *phy_ops, enum s3fwrn5_variant variant);
 void s3fwrn5_remove(struct nci_dev *ndev);
 
 int s3fwrn5_recv_frame(struct nci_dev *ndev, struct sk_buff *skb,
diff --git a/drivers/nfc/s3fwrn5/uart.c b/drivers/nfc/s3fwrn5/uart.c
index 8f142a255101..75397c3dbc58 100644
--- a/drivers/nfc/s3fwrn5/uart.c
+++ b/drivers/nfc/s3fwrn5/uart.c
@@ -136,7 +136,7 @@ static int s3fwrn82_uart_probe(struct serdev_device *serdev)
 	}
 
 	ret = s3fwrn5_probe(&phy->common.ndev, phy, &phy->ser_dev->dev,
-			    &uart_phy_ops);
+			    &uart_phy_ops, S3FWRN5_VARIANT_FWDL);
 	if (ret < 0)
 		goto err_serdev;
 
-- 
2.55.0

Re: [PATCH v4 5/6] nfc: s3fwrn5: support the S3NRN4V variant

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unprotected data race and state machine desynchronization due to ignored NCI command timeout.
--

--- Patch [5]: [PATCH v4 5/6] nfc: s3fwrn5: support the S3NRN4V variant ---
commit b57146ef7e5e76c557b5b6a3455b96f7592b3c94
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

nfc: s3fwrn5: support the S3NRN4V variant

This commit adds support for the S3NRN4V Samsung NFC controller variant.
It introduces a new initialization sequence and calibration data upload
path using the DUAL_OPTION proprietary command.

> diff --git a/drivers/nfc/s3fwrn5/nci.c b/drivers/nfc/s3fwrn5/nci.c
> index 5a9de11bbecea..f1877797d1365 100644
> --- a/drivers/nfc/s3fwrn5/nci.c
> +++ b/drivers/nfc/s3fwrn5/nci.c
[ ... ]
> @@ -117,3 +142,155 @@ int s3fwrn5_nci_rf_configure(struct s3fwrn5_info *info, const char *fw_name)
[ ... ]
> +int s3fwrn5_nci_rf_configure_dual(struct s3fwrn5_info *info,
> +				  const char *hw_name, const char *sw_name)
> +{
[ ... ]
> +	/*
> +	 * Ask the chip for its current calibration versions and skip the
> +	 * upload when both already match the blobs; a mismatch or an
> +	 * unparseable answer means the upload proceeds. GET_VER answers with
> +	 * versions, not a status byte, so nci_prop_cmd()'s return carries no
> +	 * meaning here.
> +	 */
> +	sub_oid = NCI_PROP_DUAL_SUB_GET_VER;
> +	info->dual_rsp_len = 0;
> +	nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 1, &sub_oid);
> +	if (s3fwrn5_nci_dual_cal_current(info, hw_fw, sw_fw)) {

[Severity: High]
Could ignoring the return value of nci_prop_cmd() here cause a data race?

If the controller is slow or unresponsive, nci_prop_cmd() will return
-ETIMEDOUT. Because the error is ignored, the code immediately calls
s3fwrn5_nci_dual_cal_current() which reads info->dual_rsp_len and
info->dual_rsp.

Concurrently, if a late response arrives, the RX workqueue will execute
s3fwrn5_nci_dual_rsp():

s3fwrn5_nci_dual_rsp() {
	...
	info->dual_rsp_len = min_t(unsigned int, skb->len,
				   sizeof(info->dual_rsp));
	memcpy(info->dual_rsp, skb->data, info->dual_rsp_len);
	...
}

This locklessly overwrites the buffer while it is being read, potentially
leading to a torn read of the calibration version data.

Additionally, would a late response to this timed-out command erroneously
fulfill the subsequent START_UPDATE command, shifting the response stream by
one and breaking synchronization with the hardware?

> +		dev_dbg(dev, "calibration data already current\n");
> +		ret = 0;
> +		goto out;
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807021001.239389-1-jorijnvdgraaf@catcrafts.net?part=5

[PATCH v4 6/6] arm64: dts: qcom: milos-fairphone-fp6: Add NFC

[Jorijn van der Graaf]

Add the Samsung S3NRN4V NCI NFC controller on i2c1. Its XI clock comes
from the PMK7635 CLK7_LN buffer (RPMH_LN_BB_CLK2), and the reference is
load-bearing: with no clocks reference at all the chip polls but never
activates a tag, while a held rpmh clock vote lets it read (verified on
hardware).

PVDD is fed from PM7550 LDO20, the rail the downstream driver switches
for this chip.

The enable line is routed to the chip's power-down input. The driver
drives its "en" descriptor to logical 1 to hold the chip powered down,
and on this board the chip is off while the pin is physically high, so
the line is GPIO_ACTIVE_HIGH. The pin is pulled up so the chip stays
off while the line is not driven.

Assisted-by: Claude:claude-opus-4-8
Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
 .../boot/dts/qcom/milos-fairphone-fp6.dts     | 33 +++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts b/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
index ebdca0e2d2f3..bdba848275db 100644
--- a/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
+++ b/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
@@ -601,9 +601,22 @@ &gcc {
 };
 
 &i2c1 {
-	/* Samsung NFC @ 0x27 */
-
 	status = "okay";
+
+	nfc@27 {
+		compatible = "samsung,s3nrn4v";
+		reg = <0x27>;
+
+		interrupts-extended = <&tlmm 31 IRQ_TYPE_EDGE_RISING>;
+		en-gpios = <&tlmm 56 GPIO_ACTIVE_HIGH>;
+		wake-gpios = <&tlmm 7 GPIO_ACTIVE_HIGH>;
+		clocks = <&rpmhcc RPMH_LN_BB_CLK2>;
+
+		pvdd-supply = <&vreg_l20b>;
+
+		pinctrl-0 = <&nfc_default>;
+		pinctrl-names = "default";
+	};
 };
 
 &i2c3 {
@@ -825,6 +838,22 @@ &tlmm {
 			       <13 1>, /* NC */
 			       <63 2>; /* WLAN UART */
 
+	nfc_default: nfc-default-state {
+		irq-pins {
+			pins = "gpio31";
+			function = "gpio";
+			drive-strength = <2>;
+			bias-disable;
+		};
+
+		pd-pins {
+			pins = "gpio56";
+			function = "gpio";
+			drive-strength = <2>;
+			bias-pull-up;
+		};
+	};
+
 	sdc2_card_det_n: sdc2-card-det-state {
 		pins = "gpio65";
 		function = "gpio";
-- 
2.55.0

Re: [PATCH v4 6/6] arm64: dts: qcom: milos-fairphone-fp6: Add NFC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The 'wake-gpios' pin (gpio7) is entirely omitted from the pinctrl configuration.
--

--- Patch [6]: [PATCH v4 6/6] arm64: dts: qcom: milos-fairphone-fp6: Add NFC ---
commit 971202c103843632f71a5371f28acebc71b278d0
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

arm64: dts: qcom: milos-fairphone-fp6: Add NFC

This commit adds the Samsung S3NRN4V NCI NFC controller on i2c1 for the
Fairphone 6. It configures the clock, power supply, and relevant GPIO pins
for interrupts, power-down, and wake functionality.

> diff --git a/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts b/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
> index ebdca0e2d2f3a..bdba848275db4 100644
> --- a/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
> +++ b/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
> @@ -601,9 +601,22 @@ &gcc {
>  };
>  
>  &i2c1 {
> -	/* Samsung NFC @ 0x27 */
> -
>  	status = "okay";
> +
> +	nfc@27 {
> +		compatible = "samsung,s3nrn4v";
> +		reg = <0x27>;
> +
> +		interrupts-extended = <&tlmm 31 IRQ_TYPE_EDGE_RISING>;
> +		en-gpios = <&tlmm 56 GPIO_ACTIVE_HIGH>;
> +		wake-gpios = <&tlmm 7 GPIO_ACTIVE_HIGH>;
> +		clocks = <&rpmhcc RPMH_LN_BB_CLK2>;
> +
> +		pvdd-supply = <&vreg_l20b>;
> +
> +		pinctrl-0 = <&nfc_default>;
> +		pinctrl-names = "default";
> +	};
>  };

[ ... ]

> @@ -825,6 +838,22 @@ &tlmm {
>  			       <13 1>, /* NC */
>  			       <63 2>; /* WLAN UART */
>  
> +	nfc_default: nfc-default-state {
> +		irq-pins {
> +			pins = "gpio31";
> +			function = "gpio";
> +			drive-strength = <2>;
> +			bias-disable;
> +		};
> +
> +		pd-pins {
> +			pins = "gpio56";
> +			function = "gpio";
> +			drive-strength = <2>;
> +			bias-pull-up;
> +		};
> +	};

[Severity: Medium]
The nfc@27 node defines wake-gpios on gpio7, but is this pin missing from
the pinctrl configuration?

Without an explicit pinctrl configuration for gpio7 in nfc_default, the
wake pin might be left in its bootloader or SoC default state.

Should there be a third pin configuration block here setting the function,
drive strength, and bias for gpio7 to ensure the wake line operates correctly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807021001.239389-1-jorijnvdgraaf@catcrafts.net?part=6