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

[PATCH v5 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
v5 changes were re-verified on the device: tag reads, with the
version-gate skip taking the restructured exit path. The test kernel
is v7.1.2-based; its net/nfc differs from this base only in the UART
transport (nci/uart.c), which this driver does not use.

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 the next revision will add it.

The two questions we put to Luca in the v2 thread (calibration-table
variance, linux-firmware submission) are still open; nothing here
depends on them.

Changes in v5:
 - Collect Krzysztof's Reviewed-by on patches 1 and 3.
 - Patch 4: drop the redundant regulator-framework sentence from the
   commit message and state why the uart phy is not converted
   (Krzysztof).
 - Patch 5: restructure the exit labels so failures before the
   allocation no longer pass through kvfree(), drop the
   update-announcement print, and log success at dev_dbg() (all
   Krzysztof).
 - Rebase onto the nfc tree's current for-next, which was rebased
   since v4 (the nci parse-hardening and data.c commits v4's cover
   referenced are no longer in for-next after the rebase); patch 1's
   referenced commit id updated to the rebased tree (777fd2dab446,
   was 9739e07fdb29).

Changes in v4 (full list in the lore thread):
 - Drop clk-req-gpios and the driver's clock-gating machinery (A/B
   tested as functionless) and reference the buffer the chip is
   wired to (RPMH_LN_BB_CLK2 -- Luca; verified load-bearing).
 - New patch 4 consuming the binding's pvdd-supply.
 - FW_CFG from the ->init hook and the DUAL_OPTION upload
   version-gated on GET_VER, both matching the vendor stack; board
   DTS style (Konrad).

v4: https://lore.kernel.org/20260807021001.239389-1-jorijnvdgraaf@catcrafts.net
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                     | 178 +++++++++++++++++-
 drivers/nfc/s3fwrn5/nci.h                     |  23 ++-
 drivers/nfc/s3fwrn5/s3fwrn5.h                 |  14 +-
 drivers/nfc/s3fwrn5/uart.c                    |   2 +-
 8 files changed, 313 insertions(+), 16 deletions(-)


base-commit: 9f69d05b5a85c417c73fa2d5c7a2d507ac81cf4b
-- 
2.55.0

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

[Jorijn van der Graaf]

Since commit 777fd2dab446 ("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.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Assisted-by: Claude:claude-fable-5
Assisted-by: Claude:claude-opus-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
The Reviewed-by was collected on v4; the referenced commit id was
refreshed for the rebased for-next (same commit).

 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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Preexisting endianness bug in S3FWRN5 firmware packet length parsing and construction, leading to wrong length values on big-endian architectures.
--

--- Patch [1]: [PATCH v5 1/6] nfc: s3fwrn5: drop of_match_ptr() from the I2C driver ---
commit 5cffdb84b592ca672d62f53d2f736890c7d61164
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

nfc: s3fwrn5: drop of_match_ptr() from the I2C driver

This commit drops of_match_ptr() from the I2C driver to ensure the same
code is used across all configurations. The OF match table now relies on
MODULE_DEVICE_TABLE() to count as used.

> diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
> index 4ba7626117118..cb73993446e09 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,

[Severity: High]
This is a pre-existing issue, but does s3fwrn5_i2c_read() parse the firmware
header length correctly on big-endian systems?

Earlier in 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;

Because struct s3fwrn5_fw_header defines len as a __u16, reading it directly
from the packet without endianness conversion like le16_to_cpu() swaps the
bytes on big-endian architectures. 

Can this lead to allocating an incorrectly sized skb and breaking firmware
communication on these systems?

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

[PATCH v5 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

[PATCH v5 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.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
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: v4
removed 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 v5 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.

The uart phy (s3fwrn82) is unchanged: the PVDD pin exists on that part
too, but no in-tree DT describes an s3fwrn82, so an enable there would
power nothing and could not be tested. It can follow with the first
board that describes 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

[PATCH v5 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     | 178 +++++++++++++++++++++++++++++++++-
 drivers/nfc/s3fwrn5/nci.h     |  23 ++++-
 drivers/nfc/s3fwrn5/s3fwrn5.h |  14 ++-
 drivers/nfc/s3fwrn5/uart.c    |   2 +-
 6 files changed, 255 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..381aa7bf29a2 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,154 @@ 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;
+	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_sw;
+	}
+
+	/*
+	 * 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_sw;
+	}
+
+	merged = kvmalloc(merged_size, GFP_KERNEL);
+	if (!merged) {
+		ret = -ENOMEM;
+		goto out_sw;
+	}
+	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);
+
+	/* 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_dbg(dev, "calibration data update: success\n");
+out:
+	kvfree(merged);
+out_sw:
+	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 v5 5/6] nfc: s3fwrn5: support the S3NRN4V variant

[David Heidelberg]

On 12/08/2026 00:01, Jorijn van der Graaf wrote:
> 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

I was going to make a joke about over-assisted stuff with two-lines of 
Assisted-by before, but... :)

Here's my mental model:

  - It's good to know that you used an LLM (hence Assisted-by).
  - Someone may want to reproduce some generated code, so knowing which model was
    used is useful.
  - When you put THREE ;-) models there, I think it's getting a bit useless, as
    we don't know which part was generated or adjusted by which model. At this
    point, it feels somewhat pointless.

My recommendation would be to keep just one line for the model used for:

a) the majority of the changes
b) the last model used (at least for notable changes)
c) the most powerful model doing the generation and/or review

I personally don't mind either approach. There have been discussions about 
dropping Assisted-by completely, but so far it makes sense to me to keep it, 
just in moderation.

Also, the patchset is shaping up nicely! :)

Thank you for your work,
David

[...]

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

[Jorijn van der Graaf]

On Wed, Aug 12, 2026 at 01:18:33PM +0200, David Heidelberg wrote:
> > Assisted-by: Claude:claude-opus-4-8
> > Assisted-by: Claude:claude-fable-5
> > Assisted-by: Claude:claude-opus-5
>
> Here's my mental model:
>
>   - It's good to know that you used an LLM (hence Assisted-by).
>   - Someone may want to reproduce some generated code, so knowing which model was
>     used is useful.
>   - When you put THREE ;-) models there, I think it's getting a bit useless, as
>     we don't know which part was generated or adjusted by which model. At this
>     point, it feels somewhat pointless.

coding-assistants.rst doesn't say what to do when multiple models are
used, and generated-content.rst says to choose transparency when in
doubt, so I kept the tags each patch had accumulated.

Currently I use claude-fable-5 as my coding assistant and
claude-opus-5 only reviews. Earlier I used the opus models for this
as well. Each tag was added when a model wrote code on a patch, then
deliberately carried forward across revisions.

> My recommendation would be to keep just one line for the model used for:
>
> a) the majority of the changes
> b) the last model used (at least for notable changes)
> c) the most powerful model doing the generation and/or review
>
> I personally don't mind either approach. There have been discussions about
> dropping Assisted-by completely, but so far it makes sense to me to keep it,
> just in moderation.

Agreed that three lines are ambiguous. Future patches will carry a
single Assisted-by line for the model actually being used as the
coding assistant, or just "Assisted-by: LLM" if the simplification
now in the docs tree lands [1]. The opus tags can be dropped from this series.

Thanks,
Jorijn

[1] https://lore.kernel.org/all/87qzkuahlr.fsf@trenco.lwn.net/

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing error check on nci_prop_cmd() for GET_VER causes command-response desynchronization on timeouts.
- [High] Hardware error statuses are ignored because the driver checks for negative return values instead of non-zero.
--

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

nfc: s3fwrn5: support the S3NRN4V variant

Adds support for the Samsung S3NRN4V NFC controller, which requires a new
calibration data bring-up sequence and different clock speed configuration.

> diff --git a/drivers/nfc/s3fwrn5/nci.c b/drivers/nfc/s3fwrn5/nci.c
> --- a/drivers/nfc/s3fwrn5/nci.c
> +++ b/drivers/nfc/s3fwrn5/nci.c

[ ... ]

> +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]
Is it safe to completely ignore the return value of nci_prop_cmd() here?

If the hardware is slow to respond to the initial GET_VER command,
nci_prop_cmd() will return -ETIMEDOUT. Continuing directly to START_UPDATE
without checking for this timeout might cause command-response
desynchronization.

A late response to the timed-out GET_VER command could arrive while waiting
for the START_UPDATE response, which might silently corrupt the sequence
since all DUAL_OPTION sub-commands share the same NCI opcode.

> +		dev_dbg(dev, "calibration data already current\n");
> +		ret = 0;
> +		goto out_sw;
> +	}
> +
> +	merged = kvmalloc(merged_size, GFP_KERNEL);
> +	if (!merged) {

[ ... ]

> +	/* 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) {

[Severity: High]
Does this check miss positive hardware error statuses?

When the chip rejects a command, it returns a positive NCI status byte
(like NCI_STATUS_REJECTED, 0x09). The driver's RX handler passes this status
to nci_req_complete(), which sets req_result. nci_prop_cmd() then returns
this positive integer.

Checking only for ret < 0 might cause the driver to ignore hardware
rejections and erroneously consider the upload chunks successful. Should this
check for ret != 0 instead?

> +			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) {

[Severity: High]
Similarly, would a positive error status from the hardware on STOP_UPDATE be
missed here due to the ret < 0 check?

> +		dev_err(dev, "Unable to stop calibration data update\n");
> +		goto out;
> +	}
> +
> +	dev_dbg(dev, "calibration data update: success\n");

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

[PATCH v5 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 v5 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:
- [Low] Missing pinctrl state for the `wake-gpios` pin (`gpio7`).
--

--- Patch [6]: [PATCH v5 6/6] arm64: dts: qcom: milos-fairphone-fp6: Add NFC ---
commit c9c7cf311411882808cc647f75e1e16f476acbf7
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 to the
milos-fairphone-fp6 device tree. It configures the controller's clock, power
supply, and GPIO lines.

> 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 {
>  	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: Low]
Should the pad configuration for gpio7 (used for wake-gpios) be included in
this pinctrl state?

The nfc@27 node defines wake-gpios on gpio7, but nfc_default only configures
the drive strength and bias for gpio31 (irq) and gpio56 (pd/en). This leaves
the wake-gpios pad configuration to hardware defaults or the bootloader state.

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