[PATCH net-next v2 0/2] nfc: s3fwrn5: support the S3NRN4V variant

[PATCH net-next v2 0/2] nfc: s3fwrn5: support the S3NRN4V variant

[Jorijn van der Graaf]

This adds support for the Samsung S3NRN4V, an S3FWRN5-family NFC
controller found e.g. on the Fairphone 6 (SM7635), to the s3fwrn5
driver.

The S3NRN4V differs from the already-supported parts in three ways: it
ships with working firmware behind a bootloader protocol the driver
does not implement (so firmware download is skipped), it loads its RF
registers through a different proprietary command (DUAL_OPTION), and it
gates its reference clock through a CLK_REQ line that the driver must
service for the chip to be able to generate the 13.56 MHz poll carrier.

Patch 1 adds the compatible and the clk-req-gpios property to the
binding; patch 2 implements the variant in the driver.

Tested on a Fairphone 6 running a milos-mainline kernel: reader mode polls
and reads ISO 14443-4 tags reliably, both from a fresh boot and across
driver reloads.

Changes in v2:
 - Drop the -i2c bus suffix from the new compatible: it is now plain
   samsung,s3nrn4v (Requested by: Conor Dooley).
 - Close a race in the probe-time CLK_REQ seeding by reading the GPIO
   level under clk_lock, so a stale level can never overwrite a fresher
   state applied by the irq thread (found by the Sashiko AI review of
   v1).
 - Binding completeness: document the PVDD supply (required for the
   S3NRN4V), add an S3NRN4V example exercising the new properties, make
   clk-req-gpios depend on clocks, and describe the CLK_REQ pin in
   hardware terms.
 - Rework the binding commit message: justify the GPIO modelling in
   hardware terms and explain why no fallback compatible applies.
 - Add an s3nrn4v i2c_device_id entry carrying the variant so both
   match paths agree, and describe the of_match_ptr() removal in the
   driver commit message.
 - Reject malformed rfreg blobs (word alignment, single-byte section
   index bound) up front instead of failing at STOP_UPDATE.
 - Handle gpiod_get_value_cansleep() failure in the CLK_REQ sync
   instead of treating an error as "clock off".
v1: https://lore.kernel.org/20260703202601.78563-1-jorijnvdgraaf@catcrafts.net

Jorijn van der Graaf (2):
  dt-bindings: net: nfc: samsung,s3fwrn5: add S3NRN4V and clk-req-gpios
  nfc: s3fwrn5: support the S3NRN4V variant

 .../bindings/net/nfc/samsung,s3fwrn5.yaml     |  65 ++++++++-
 drivers/nfc/s3fwrn5/core.c                    |  40 ++++-
 drivers/nfc/s3fwrn5/i2c.c                     | 138 ++++++++++++++++--
 drivers/nfc/s3fwrn5/nci.c                     | 119 ++++++++++++++-
 drivers/nfc/s3fwrn5/nci.h                     |  32 +++-
 drivers/nfc/s3fwrn5/s3fwrn5.h                 |  14 +-
 drivers/nfc/s3fwrn5/uart.c                    |   2 +-
 7 files changed, 394 insertions(+), 16 deletions(-)


base-commit: 805185b7c7a1069e407b6f7b3bc98e44d415f484
-- 
2.55.0

[PATCH net-next v2 1/2] dt-bindings: net: nfc: samsung,s3fwrn5: add S3NRN4V and clk-req-gpios

[Jorijn van der Graaf]

The S3NRN4V is an S3FWRN5-family NCI NFC controller 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 speaks a different protocol
and its RF configuration is loaded through a different proprietary
command set.

Document the optional clk-req-gpios property: the controller's CLK_REQ
output is asserted for as long as the chip needs its external reference
clock, notably while generating the 13.56 MHz poll carrier. The pin is
a level signal reflecting the chip's current clock demand, not a
one-shot event, so it is modelled as a GPIO. No user of the handshake
is known on the already-supported parts, so the property is restricted
to the S3NRN4V (easily relaxed should one appear), and it depends on
clocks, as its purpose is gating an external clock.

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. Add an example for the new
device exercising the new properties.

Assisted-by: Claude:claude-opus-4-8
Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
Changes in v2:
 - Drop the -i2c bus suffix from the new compatible; the parent bus
   node already implies the interface (Requested by: Conor Dooley).
 - Document the PVDD supply; required for the S3NRN4V, optional for the
   existing parts whose deployed DTs never described a supply.
 - Add an S3NRN4V example exercising clk-req-gpios, clocks and
   pvdd-supply.
 - Make clk-req-gpios depend on clocks.
 - Describe the CLK_REQ pin in hardware terms (what the pin is, not
   what the OS does with it).
 - Rework the commit message: justify the GPIO modelling in hardware
   terms and explain why no fallback compatible applies.
v1: https://lore.kernel.org/20260703202601.78563-2-jorijnvdgraaf@catcrafts.net

 .../bindings/net/nfc/samsung,s3fwrn5.yaml     | 65 ++++++++++++++++++-
 1 file changed, 64 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..1e784a90d015 100644
--- a/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
+++ b/Documentation/devicetree/bindings/net/nfc/samsung,s3fwrn5.yaml
@@ -14,12 +14,20 @@ properties:
     enum:
       - samsung,s3fwrn5-i2c
       - samsung,s3fwrn82
+      - samsung,s3nrn4v
 
   en-gpios:
     maxItems: 1
     description:
       Output GPIO pin used for enabling/disabling the chip
 
+  clk-req-gpios:
+    maxItems: 1
+    description:
+      Input GPIO pin connected to the controller's CLK_REQ output, which the
+      controller asserts for as long as it requires its external reference
+      clock.
+
   interrupts:
     maxItems: 1
 
@@ -29,6 +37,9 @@ properties:
   clocks:
     maxItems: 1
 
+  pvdd-supply:
+    description: PVDD power supply
+
   wake-gpios:
     maxItems: 1
     description:
@@ -53,17 +64,45 @@ required:
   - en-gpios
   - wake-gpios
 
+# The clock-request handshake gates an external clock, so it needs one.
+dependencies:
+  clk-req-gpios: [ clocks ]
+
 allOf:
   - if:
       properties:
         compatible:
           contains:
-            const: samsung,s3fwrn5-i2c
+            enum:
+              - samsung,s3fwrn5-i2c
+              - samsung,s3nrn4v
     then:
       required:
         - interrupts
         - reg
 
+  # No user of the clock-request handshake is known on the other parts.
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              const: samsung,s3nrn4v
+    then:
+      properties:
+        clk-req-gpios: false
+
+  # 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>
@@ -97,3 +136,27 @@ examples:
 
         };
     };
+  # S3NRN4V with a clock-request gated external clock
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        nfc@27 {
+            compatible = "samsung,s3nrn4v";
+            reg = <0x27>;
+
+            interrupt-parent = <&tlmm>;
+            interrupts = <31 IRQ_TYPE_EDGE_RISING>;
+
+            en-gpios = <&tlmm 56 GPIO_ACTIVE_HIGH>;
+            wake-gpios = <&tlmm 7 GPIO_ACTIVE_HIGH>;
+            clk-req-gpios = <&tlmm 6 GPIO_ACTIVE_HIGH>;
+
+            clocks = <&rpmhcc 4>;
+            pvdd-supply = <&nfc_pvdd>;
+        };
+    };
-- 
2.55.0

Re: [PATCH net-next v2 1/2] dt-bindings: net: nfc: samsung,s3fwrn5: add S3NRN4V and clk-req-gpios

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 52 bytes --]

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH net-next v2 2/2] nfc: s3fwrn5: support the S3NRN4V variant

[Jorijn van der Graaf]

The S3NRN4V (e.g. on the Fairphone 6, SM7635) is an S3FWRN5-family NFC
controller that needs different bring-up, selected with a new
samsung,s3nrn4v compatible:

 - It ships with working firmware behind a bootloader protocol this
   driver does not implement (GET_BOOTINFO times out), so the firmware
   download step is skipped. Its RF registers are (re)loaded with the
   proprietary DUAL_OPTION command (the HW and SW register blobs merged
   into a single stream) instead of the START/SET/STOP_RFREG sequence.

 - Its reference clock speed is configured with the single-byte FW_CFG
   form, sent from the ->setup hook (after CORE_RESET, before CORE_INIT).
   The selector value (0x11) is taken from the vendor configuration for
   this part; its encoding is not documented.

 - It gates its XI clock through a CLK_REQ line: the chip drives it high
   when it needs the clock, notably to synthesise the 13.56 MHz poll
   carrier. Left always-on, the free-running clock never lets the chip's
   TX PLL lock on a fresh start and it cannot poll (it falls back to
   listen only). Service the handshake when a clk-req GPIO is described,
   gating the clock on it; without one the clock stays always-on.

The variant is carried as match data by both the OF and the I2C device
id tables so the two match paths agree, and the OF table is now
referenced unconditionally for its match data, so drop the
of_match_ptr()/__maybe_unused annotations from it.

The error policy differs between the two configuration steps on purpose:
a clock misconfiguration is fatal (a ->setup failure aborts CORE_INIT),
whereas an RF-register update failure is only warned about and bring-up
continues, since the chip falls back to the RF registers programmed in
its flash and NFC may still work.

Unlike the host-endian word read in the legacy rfreg path, the
DUAL_OPTION checksum is accumulated with get_unaligned_le32() and emitted
little-endian explicitly, so it is correct regardless of CPU endianness.

Existing S3FWRN5 / S3FWRN82 setups keep the firmware-download path and
the always-on clock, unchanged.

Assisted-by: Claude:claude-opus-4-8
Assisted-by: Claude:claude-fable-5
Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
---
Changes in v2:
 - Rename the new compatible to samsung,s3nrn4v, matching the binding
   change (Requested by: Conor Dooley).
 - Close a race in the probe-time CLK_REQ seeding: the GPIO level is
   now read under clk_lock (new s3fwrn5_i2c_clk_sync(), used by both
   the irq thread and probe), so a level read before the irq fired can
   never overwrite the fresher state the irq thread applied (found by
   the Sashiko AI review of v1).
 - Reject malformed rfreg blobs (word alignment, single-byte section
   index bound) up front instead of failing at STOP_UPDATE.
 - Handle gpiod_get_value_cansleep() failure instead of gating the
   clock off on error.
 - Add an s3nrn4v i2c_device_id entry carrying the variant so both
   match paths agree.
 - Describe the of_match_ptr()/__maybe_unused removal in the commit
   message.
v1: https://lore.kernel.org/20260703202601.78563-3-jorijnvdgraaf@catcrafts.net

 drivers/nfc/s3fwrn5/core.c    |  40 +++++++++-
 drivers/nfc/s3fwrn5/i2c.c     | 138 +++++++++++++++++++++++++++++++---
 drivers/nfc/s3fwrn5/nci.c     | 119 ++++++++++++++++++++++++++++-
 drivers/nfc/s3fwrn5/nci.h     |  32 +++++++-
 drivers/nfc/s3fwrn5/s3fwrn5.h |  14 +++-
 drivers/nfc/s3fwrn5/uart.c    |   2 +-
 6 files changed, 330 insertions(+), 15 deletions(-)

diff --git a/drivers/nfc/s3fwrn5/core.c b/drivers/nfc/s3fwrn5/core.c
index af0fa8bd970b..59317eaad7ac 100644
--- a/drivers/nfc/s3fwrn5/core.c
+++ b/drivers/nfc/s3fwrn5/core.c
@@ -122,11 +122,47 @@ static int s3fwrn5_nci_send(struct nci_dev *ndev, struct sk_buff *skb)
 	return 0;
 }
 
+static int s3fwrn5_nci_setup(struct nci_dev *ndev)
+{
+	struct s3fwrn5_info *info = nci_get_drvdata(ndev);
+
+	/*
+	 * Runs after CORE_RESET, before CORE_INIT. The S3NRN4V needs its
+	 * reference clock configured here (the downstream stack does it in the
+	 * bootloader, before CORE_RESET, but this is the earliest hook the NCI
+	 * core offers and the chip accepts it).
+	 */
+	if (info->variant == S3FWRN5_VARIANT_S3NRN4V)
+		return s3fwrn5_nci_clk_cfg(info);
+
+	return 0;
+}
+
 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) {
+		/*
+		 * The S3NRN4V ships with working firmware behind a bootloader
+		 * protocol this driver does not implement, so there is no
+		 * download step; the NCI core has already done CORE_RESET +
+		 * CORE_INIT. Just (re)load the RF registers via DUAL_OPTION.
+		 */
+		ret = s3fwrn5_nci_rf_configure_dual(info, "sec_s3nrn4v_hwreg.bin",
+						    "sec_s3nrn4v_swreg.bin");
+		/*
+		 * Keep going even if the blobs could not be loaded: the chip
+		 * still enumerates and falls back to the RF registers programmed
+		 * in its flash, so NFC may work anyway.
+		 */
+		if (ret < 0)
+			dev_warn(&ndev->nfc_dev->dev,
+				 "rfreg configure failed (%d)\n", ret);
+		return 0;
+	}
+
 	if (s3fwrn5_firmware_init(info)) {
 		//skip bootloader mode
 		return 0;
@@ -152,13 +188,14 @@ static const struct nci_ops s3fwrn5_nci_ops = {
 	.open = s3fwrn5_nci_open,
 	.close = s3fwrn5_nci_close,
 	.send = s3fwrn5_nci_send,
+	.setup = s3fwrn5_nci_setup,
 	.post_setup = s3fwrn5_nci_post_setup,
 	.prop_ops = s3fwrn5_nci_prop_ops,
 	.n_prop_ops = ARRAY_SIZE(s3fwrn5_nci_prop_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 +207,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);
diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
index e9a34d27a369..7d20e737e402 100644
--- a/drivers/nfc/s3fwrn5/i2c.c
+++ b/drivers/nfc/s3fwrn5/i2c.c
@@ -23,9 +23,76 @@ struct s3fwrn5_i2c_phy {
 	struct i2c_client *i2c_dev;
 	struct clk *clk;
 
+	/*
+	 * Optional hardware clock-request handshake. When a CLK_REQ GPIO is
+	 * wired, the chip drives it high while it needs its XI clock -- notably
+	 * to generate the poll/reader carrier -- and the clock is gated on it
+	 * instead of being left always-on (which never lets the chip's TX PLL
+	 * lock on a fresh clock start, leaving it unable to poll).
+	 */
+	struct gpio_desc *gpio_clk_req;
+	bool clk_on;
+	struct mutex clk_lock;	/* serialises clk_on against the CLK_REQ irq */
+
 	unsigned int irq_skip:1;
 };
 
+static void s3fwrn5_i2c_clk_set_locked(struct s3fwrn5_i2c_phy *phy, bool on)
+{
+	lockdep_assert_held(&phy->clk_lock);
+
+	if (on && !phy->clk_on) {
+		int ret = clk_prepare_enable(phy->clk);
+
+		if (ret == 0)
+			phy->clk_on = true;
+		else
+			dev_warn_once(&phy->i2c_dev->dev,
+				      "failed to enable clock (%d); NFC may not poll\n",
+				      ret);
+	} else if (!on && phy->clk_on) {
+		clk_disable_unprepare(phy->clk);
+		phy->clk_on = false;
+	}
+}
+
+/*
+ * Apply the current CLK_REQ level. Reading the GPIO under clk_lock makes
+ * concurrent callers (the CLK_REQ irq thread and the probe-time seeding)
+ * safe: whoever runs last applies a level read after the earlier update,
+ * never a stale one.
+ */
+static void s3fwrn5_i2c_clk_sync(struct s3fwrn5_i2c_phy *phy)
+{
+	int level;
+
+	mutex_lock(&phy->clk_lock);
+	level = gpiod_get_value_cansleep(phy->gpio_clk_req);
+	if (level >= 0)
+		s3fwrn5_i2c_clk_set_locked(phy, level > 0);
+	else
+		dev_warn_once(&phy->i2c_dev->dev,
+			      "failed to read CLK_REQ (%d); keeping clock state\n",
+			      level);
+	mutex_unlock(&phy->clk_lock);
+}
+
+static void s3fwrn5_i2c_clk_disable_action(void *data)
+{
+	struct s3fwrn5_i2c_phy *phy = data;
+
+	mutex_lock(&phy->clk_lock);
+	s3fwrn5_i2c_clk_set_locked(phy, false);
+	mutex_unlock(&phy->clk_lock);
+}
+
+static irqreturn_t s3fwrn5_i2c_clk_req_thread(int irq, void *phy_id)
+{
+	s3fwrn5_i2c_clk_sync(phy_id);
+
+	return IRQ_HANDLED;
+}
+
 static void s3fwrn5_i2c_set_mode(void *phy_id, enum s3fwrn5_mode mode)
 {
 	struct s3fwrn5_i2c_phy *phy = phy_id;
@@ -146,6 +213,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;
 
@@ -172,15 +240,61 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client)
 	 * S3FWRN5 depends on a clock input ("XI" pin) to function properly.
 	 * Depending on the hardware configuration this could be an always-on
 	 * oscillator or some external clock that must be explicitly enabled.
-	 * Make sure the clock is running before starting S3FWRN5.
+	 *
+	 * If a CLK_REQ GPIO is wired, the chip gates the clock itself (driving
+	 * CLK_REQ high when it needs XI); service that handshake. Otherwise just
+	 * make sure the clock is running before starting S3FWRN5.
 	 */
-	phy->clk = devm_clk_get_optional_enabled(&client->dev, NULL);
-	if (IS_ERR(phy->clk))
-		return dev_err_probe(&client->dev, PTR_ERR(phy->clk),
-				     "failed to get clock\n");
+	mutex_init(&phy->clk_lock);
+	phy->gpio_clk_req = devm_gpiod_get_optional(&client->dev, "clk-req",
+						    GPIOD_IN);
+	if (IS_ERR(phy->gpio_clk_req))
+		return PTR_ERR(phy->gpio_clk_req);
+
+	if (phy->gpio_clk_req) {
+		int clk_req_irq;
+
+		phy->clk = devm_clk_get_optional(&client->dev, NULL);
+		if (IS_ERR(phy->clk))
+			return dev_err_probe(&client->dev, PTR_ERR(phy->clk),
+					     "failed to get clock\n");
+
+		/*
+		 * Unlike the always-on branch below, this clock is enabled by
+		 * hand from the CLK_REQ handler, so devm will not disable it on
+		 * unbind. Gate it off explicitly if it is still on at teardown.
+		 */
+		ret = devm_add_action_or_reset(&client->dev,
+					       s3fwrn5_i2c_clk_disable_action,
+					       phy);
+		if (ret)
+			return ret;
+
+		clk_req_irq = gpiod_to_irq(phy->gpio_clk_req);
+		if (clk_req_irq < 0)
+			return clk_req_irq;
+
+		ret = devm_request_threaded_irq(&client->dev, clk_req_irq, NULL,
+						s3fwrn5_i2c_clk_req_thread,
+						IRQF_TRIGGER_RISING |
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						"s3fwrn5_clk_req", phy);
+		if (ret)
+			return ret;
+
+		/* Seed the clock state from the current CLK_REQ level. */
+		s3fwrn5_i2c_clk_sync(phy);
+	} else {
+		phy->clk = devm_clk_get_optional_enabled(&client->dev, NULL);
+		if (IS_ERR(phy->clk))
+			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;
 
@@ -205,13 +319,17 @@ 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_DRIVER_NAME, .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[] __maybe_unused = {
-	{ .compatible = "samsung,s3fwrn5-i2c", },
+static const struct of_device_id of_s3fwrn5_i2c_match[] = {
+	{ .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);
@@ -219,7 +337,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,
diff --git a/drivers/nfc/s3fwrn5/nci.c b/drivers/nfc/s3fwrn5/nci.c
index 5a9de11bbece..7034fb810e18 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,7 @@ 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] = {
+const struct nci_driver_ops s3fwrn5_nci_prop_ops[5] = {
 	{
 		.opcode = nci_opcode_pack(NCI_GID_PROPRIETARY,
 				NCI_PROP_SET_RFREG),
@@ -41,6 +44,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_prop_rsp,
+	},
 };
 
 #define S3FWRN5_RFREG_SECTION_SIZE 252
@@ -117,3 +125,112 @@ int s3fwrn5_nci_rf_configure(struct s3fwrn5_info *info, const char *fw_name)
 	release_firmware(fw);
 	return ret;
 }
+
+/*
+ * Configure the reference clock. The S3NRN4V expects the single-byte FW_CFG
+ * form (just the clock-speed selector). The downstream stack sends this in the
+ * bootloader before CORE_RESET; the earliest the mainline NCI core lets us in
+ * is the ->setup hook (after CORE_RESET, before CORE_INIT), which works.
+ */
+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);
+}
+
+/*
+ * S3NRN4V RF register update. The HW and SW register blobs are merged into a
+ * single stream (HW first) and pushed via the DUAL_OPTION command:
+ * 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)
+{
+	const struct firmware *hw_fw = NULL, *sw_fw = NULL;
+	struct nci_prop_dual_set_option_cmd set_option;
+	struct device *dev = &info->ndev->nfc_dev->dev;
+	size_t merged_size, i, len;
+	u8 *merged = NULL;
+	u8 stop_cmd[3];
+	u32 checksum;
+	u8 sub_oid;
+	int ret;
+
+	ret = request_firmware(&hw_fw, hw_name, dev);
+	if (ret < 0)
+		return ret;
+	ret = request_firmware(&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 % 4 ||
+	    merged_size > 256 * NCI_PROP_DUAL_SECTION_SIZE) {
+		dev_err(dev, "invalid rfreg blob size (%zu)\n", merged_size);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	merged = kmalloc(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);
+
+	/* Running sum of the merged stream as little-endian 32-bit words. */
+	checksum = 0;
+	for (i = 0; i + 4 <= merged_size; i += 4)
+		checksum += get_unaligned_le32(merged + i);
+
+	dev_dbg(dev, "rfreg dual-option 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 rfreg 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, "rfreg update error (code=%d)\n", ret);
+			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 rfreg update\n");
+		goto out;
+	}
+
+	dev_dbg(dev, "rfreg dual-option update: success\n");
+out:
+	kfree(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..23179ba095a1 100644
--- a/drivers/nfc/s3fwrn5/nci.h
+++ b/drivers/nfc/s3fwrn5/nci.h
@@ -40,6 +40,13 @@ struct nci_prop_stop_rfreg_rsp {
 
 #define NCI_PROP_FW_CFG		0x28
 
+/*
+ * Single-byte FW_CFG payload (clock-speed selector) for the S3NRN4V reference
+ * clock. Taken from the vendor configuration for this part (the encoding is
+ * not documented).
+ */
+#define NCI_PROP_FW_CFG_CLK_SPEED	0x11
+
 struct nci_prop_fw_cfg_cmd {
 	__u8 clk_type;
 	__u8 clk_speed;
@@ -50,7 +57,30 @@ struct nci_prop_fw_cfg_rsp {
 	__u8 status;
 };
 
-extern const struct nci_driver_ops s3fwrn5_nci_prop_ops[4];
+/*
+ * The S3NRN4V updates its RF registers through a single "dual option" command
+ * (a sub-OID selects the operation) instead of the START/SET/STOP_RFREG
+ * opcodes above, and expects the HW and SW register blobs merged into one
+ * stream.
+ */
+#define NCI_PROP_DUAL_OPTION		0x2a
+
+#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;	/* NCI_PROP_DUAL_SUB_SET_OPTION */
+	__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..2d8c12091fba 100644
--- a/drivers/nfc/s3fwrn5/s3fwrn5.h
+++ b/drivers/nfc/s3fwrn5/s3fwrn5.h
@@ -21,6 +21,17 @@ enum s3fwrn5_mode {
 	S3FWRN5_MODE_FW,
 };
 
+enum s3fwrn5_variant {
+	/* S3FWRN5 / S3FWRN82: firmware is downloaded by this driver */
+	S3FWRN5_VARIANT_FWDL,
+	/*
+	 * S3NRN4V: ships with working firmware behind a bootloader protocol
+	 * this driver does not implement; skip the download, configure the
+	 * clock (FW_CFG) and update the RF registers via the DUAL_OPTION cmd.
+	 */
+	S3FWRN5_VARIANT_S3NRN4V,
+};
+
 struct s3fwrn5_phy_ops {
 	void (*set_wake)(void *id, bool sleep);
 	void (*set_mode)(void *id, enum s3fwrn5_mode);
@@ -36,6 +47,7 @@ struct s3fwrn5_info {
 	const struct s3fwrn5_phy_ops *phy_ops;
 
 	struct s3fwrn5_fw_info fw_info;
+	enum s3fwrn5_variant variant;
 
 	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 540a4ddb0b05..47172d739a41 100644
--- a/drivers/nfc/s3fwrn5/uart.c
+++ b/drivers/nfc/s3fwrn5/uart.c
@@ -137,7 +137,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 net-next v2 2/2] nfc: s3fwrn5: support the S3NRN4V variant

[David Heidelberg]

On 05/07/2026 21:06, Jorijn van der Graaf wrote:
> The S3NRN4V (e.g. on the Fairphone 6, SM7635) is an S3FWRN5-family NFC
> controller that needs different bring-up, selected with a new
> samsung,s3nrn4v compatible:
> 
>   - It ships with working firmware behind a bootloader protocol this
>     driver does not implement (GET_BOOTINFO times out), so the firmware
>     download step is skipped. Its RF registers are (re)loaded with the
>     proprietary DUAL_OPTION command (the HW and SW register blobs merged
>     into a single stream) instead of the START/SET/STOP_RFREG sequence.
> 
>   - Its reference clock speed is configured with the single-byte FW_CFG
>     form, sent from the ->setup hook (after CORE_RESET, before CORE_INIT).
>     The selector value (0x11) is taken from the vendor configuration for
>     this part; its encoding is not documented.
> 
>   - It gates its XI clock through a CLK_REQ line: the chip drives it high
>     when it needs the clock, notably to synthesise the 13.56 MHz poll
>     carrier. Left always-on, the free-running clock never lets the chip's
>     TX PLL lock on a fresh start and it cannot poll (it falls back to
>     listen only). Service the handshake when a clk-req GPIO is described,
>     gating the clock on it; without one the clock stays always-on.
> 
> The variant is carried as match data by both the OF and the I2C device
> id tables so the two match paths agree, and the OF table is now
> referenced unconditionally for its match data, so drop the
> of_match_ptr()/__maybe_unused annotations from it.

Hello Jorijn,

thank you for the work on the driver!

Please, send the "drop the of_match_ptr()/__maybe_unused annotations from it." 
type of change as part of the series, but as a separate commit before the new HW 
support introduction.

Since you touch S3FWRN5_I2C_DRIVER_NAME, replace define S3FWRN5_I2C_DRIVER_NAME 
occurenced with the "s3fwrn5_i2c" directly before introducing the support (also 
separate commit)


> 
> The error policy differs between the two configuration steps on purpose:
> a clock misconfiguration is fatal (a ->setup failure aborts CORE_INIT),
> whereas an RF-register update failure is only warned about and bring-up
> continues, since the chip falls back to the RF registers programmed in
> its flash and NFC may still work.
> 
> Unlike the host-endian word read in the legacy rfreg path, the
> DUAL_OPTION checksum is accumulated with get_unaligned_le32() and emitted
> little-endian explicitly, so it is correct regardless of CPU endianness.
> 
> Existing S3FWRN5 / S3FWRN82 setups keep the firmware-download path and
> the always-on clock, unchanged.
> 
> Assisted-by: Claude:claude-opus-4-8
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
> ---
> Changes in v2:
>   - Rename the new compatible to samsung,s3nrn4v, matching the binding
>     change (Requested by: Conor Dooley).
>   - Close a race in the probe-time CLK_REQ seeding: the GPIO level is
>     now read under clk_lock (new s3fwrn5_i2c_clk_sync(), used by both
>     the irq thread and probe), so a level read before the irq fired can
>     never overwrite the fresher state the irq thread applied (found by
>     the Sashiko AI review of v1).
>   - Reject malformed rfreg blobs (word alignment, single-byte section
>     index bound) up front instead of failing at STOP_UPDATE.
>   - Handle gpiod_get_value_cansleep() failure instead of gating the
>     clock off on error.
>   - Add an s3nrn4v i2c_device_id entry carrying the variant so both
>     match paths agree.
>   - Describe the of_match_ptr()/__maybe_unused removal in the commit
>     message.
> v1: https://lore.kernel.org/20260703202601.78563-3-jorijnvdgraaf@catcrafts.net
> 
>   drivers/nfc/s3fwrn5/core.c    |  40 +++++++++-
>   drivers/nfc/s3fwrn5/i2c.c     | 138 +++++++++++++++++++++++++++++++---
>   drivers/nfc/s3fwrn5/nci.c     | 119 ++++++++++++++++++++++++++++-
>   drivers/nfc/s3fwrn5/nci.h     |  32 +++++++-
>   drivers/nfc/s3fwrn5/s3fwrn5.h |  14 +++-
>   drivers/nfc/s3fwrn5/uart.c    |   2 +-
>   6 files changed, 330 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/nfc/s3fwrn5/core.c b/drivers/nfc/s3fwrn5/core.c
> index af0fa8bd970b..59317eaad7ac 100644
> --- a/drivers/nfc/s3fwrn5/core.c
> +++ b/drivers/nfc/s3fwrn5/core.c
> @@ -122,11 +122,47 @@ static int s3fwrn5_nci_send(struct nci_dev *ndev, struct sk_buff *skb)
>   	return 0;
>   }
>   
> +static int s3fwrn5_nci_setup(struct nci_dev *ndev)
> +{
> +	struct s3fwrn5_info *info = nci_get_drvdata(ndev);
> +
> +	/*
> +	 * Runs after CORE_RESET, before CORE_INIT. The S3NRN4V needs its
> +	 * reference clock configured here (the downstream stack does it in the
> +	 * bootloader, before CORE_RESET, but this is the earliest hook the NCI
> +	 * core offers and the chip accepts it).
> +	 */
> +	if (info->variant == S3FWRN5_VARIANT_S3NRN4V)
> +		return s3fwrn5_nci_clk_cfg(info);
> +
> +	return 0;
> +}
> +
>   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) {
> +		/*
> +		 * The S3NRN4V ships with working firmware behind a bootloader
> +		 * protocol this driver does not implement, so there is no
> +		 * download step; the NCI core has already done CORE_RESET +
> +		 * CORE_INIT. Just (re)load the RF registers via DUAL_OPTION.
> +		 */
> +		ret = s3fwrn5_nci_rf_configure_dual(info, "sec_s3nrn4v_hwreg.bin",
> +						    "sec_s3nrn4v_swreg.bin");
> +		/*
> +		 * Keep going even if the blobs could not be loaded: the chip
> +		 * still enumerates and falls back to the RF registers programmed
> +		 * in its flash, so NFC may work anyway.
> +		 */
> +		if (ret < 0)
> +			dev_warn(&ndev->nfc_dev->dev,
> +				 "rfreg configure failed (%d)\n", ret);
> +		return 0;
> +	}
> +
>   	if (s3fwrn5_firmware_init(info)) {
>   		//skip bootloader mode
>   		return 0;
> @@ -152,13 +188,14 @@ static const struct nci_ops s3fwrn5_nci_ops = {
>   	.open = s3fwrn5_nci_open,
>   	.close = s3fwrn5_nci_close,
>   	.send = s3fwrn5_nci_send,
> +	.setup = s3fwrn5_nci_setup,
>   	.post_setup = s3fwrn5_nci_post_setup,
>   	.prop_ops = s3fwrn5_nci_prop_ops,
>   	.n_prop_ops = ARRAY_SIZE(s3fwrn5_nci_prop_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 +207,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);
> diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
> index e9a34d27a369..7d20e737e402 100644
> --- a/drivers/nfc/s3fwrn5/i2c.c
> +++ b/drivers/nfc/s3fwrn5/i2c.c
> @@ -23,9 +23,76 @@ struct s3fwrn5_i2c_phy {
>   	struct i2c_client *i2c_dev;
>   	struct clk *clk;
>   
> +	/*
> +	 * Optional hardware clock-request handshake. When a CLK_REQ GPIO is
> +	 * wired, the chip drives it high while it needs its XI clock -- notably
> +	 * to generate the poll/reader carrier -- and the clock is gated on it
> +	 * instead of being left always-on (which never lets the chip's TX PLL
> +	 * lock on a fresh clock start, leaving it unable to poll).
> +	 */
> +	struct gpio_desc *gpio_clk_req;
> +	bool clk_on;
> +	struct mutex clk_lock;	/* serialises clk_on against the CLK_REQ irq */
> +
>   	unsigned int irq_skip:1;
>   };
>   
> +static void s3fwrn5_i2c_clk_set_locked(struct s3fwrn5_i2c_phy *phy, bool on)
> +{
> +	lockdep_assert_held(&phy->clk_lock);
> +
> +	if (on && !phy->clk_on) {
> +		int ret = clk_prepare_enable(phy->clk);
> +
> +		if (ret == 0)
> +			phy->clk_on = true;
> +		else
> +			dev_warn_once(&phy->i2c_dev->dev,
> +				      "failed to enable clock (%d); NFC may not poll\n",
> +				      ret);
> +	} else if (!on && phy->clk_on) {
> +		clk_disable_unprepare(phy->clk);
> +		phy->clk_on = false;
> +	}
> +}
> +
> +/*
> + * Apply the current CLK_REQ level. Reading the GPIO under clk_lock makes
> + * concurrent callers (the CLK_REQ irq thread and the probe-time seeding)
> + * safe: whoever runs last applies a level read after the earlier update,
> + * never a stale one.
> + */
> +static void s3fwrn5_i2c_clk_sync(struct s3fwrn5_i2c_phy *phy)
> +{
> +	int level;
> +
> +	mutex_lock(&phy->clk_lock);
> +	level = gpiod_get_value_cansleep(phy->gpio_clk_req);
> +	if (level >= 0)
> +		s3fwrn5_i2c_clk_set_locked(phy, level > 0);
> +	else
> +		dev_warn_once(&phy->i2c_dev->dev,
> +			      "failed to read CLK_REQ (%d); keeping clock state\n",
> +			      level);
> +	mutex_unlock(&phy->clk_lock);
> +}
> +
> +static void s3fwrn5_i2c_clk_disable_action(void *data)
> +{
> +	struct s3fwrn5_i2c_phy *phy = data;
> +
> +	mutex_lock(&phy->clk_lock);
> +	s3fwrn5_i2c_clk_set_locked(phy, false);
> +	mutex_unlock(&phy->clk_lock);
> +}
> +
> +static irqreturn_t s3fwrn5_i2c_clk_req_thread(int irq, void *phy_id)
> +{
> +	s3fwrn5_i2c_clk_sync(phy_id);
> +
> +	return IRQ_HANDLED;
> +}
> +
>   static void s3fwrn5_i2c_set_mode(void *phy_id, enum s3fwrn5_mode mode)
>   {
>   	struct s3fwrn5_i2c_phy *phy = phy_id;
> @@ -146,6 +213,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;
>   
> @@ -172,15 +240,61 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client)
>   	 * S3FWRN5 depends on a clock input ("XI" pin) to function properly.
>   	 * Depending on the hardware configuration this could be an always-on
>   	 * oscillator or some external clock that must be explicitly enabled.
> -	 * Make sure the clock is running before starting S3FWRN5.
> +	 *
> +	 * If a CLK_REQ GPIO is wired, the chip gates the clock itself (driving
> +	 * CLK_REQ high when it needs XI); service that handshake. Otherwise just
> +	 * make sure the clock is running before starting S3FWRN5.
>   	 */
> -	phy->clk = devm_clk_get_optional_enabled(&client->dev, NULL);
> -	if (IS_ERR(phy->clk))
> -		return dev_err_probe(&client->dev, PTR_ERR(phy->clk),
> -				     "failed to get clock\n");
> +	mutex_init(&phy->clk_lock);
> +	phy->gpio_clk_req = devm_gpiod_get_optional(&client->dev, "clk-req",
> +						    GPIOD_IN);
> +	if (IS_ERR(phy->gpio_clk_req))
> +		return PTR_ERR(phy->gpio_clk_req);
> +
> +	if (phy->gpio_clk_req) {
> +		int clk_req_irq;
> +
> +		phy->clk = devm_clk_get_optional(&client->dev, NULL);
> +		if (IS_ERR(phy->clk))
> +			return dev_err_probe(&client->dev, PTR_ERR(phy->clk),
> +					     "failed to get clock\n");
> +
> +		/*
> +		 * Unlike the always-on branch below, this clock is enabled by
> +		 * hand from the CLK_REQ handler, so devm will not disable it on
> +		 * unbind. Gate it off explicitly if it is still on at teardown.
> +		 */
> +		ret = devm_add_action_or_reset(&client->dev,
> +					       s3fwrn5_i2c_clk_disable_action,
> +					       phy);
> +		if (ret)
> +			return ret;
> +
> +		clk_req_irq = gpiod_to_irq(phy->gpio_clk_req);
> +		if (clk_req_irq < 0)
> +			return clk_req_irq;
> +
> +		ret = devm_request_threaded_irq(&client->dev, clk_req_irq, NULL,
> +						s3fwrn5_i2c_clk_req_thread,
> +						IRQF_TRIGGER_RISING |
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						"s3fwrn5_clk_req", phy);
> +		if (ret)
> +			return ret;
> +
> +		/* Seed the clock state from the current CLK_REQ level. */
> +		s3fwrn5_i2c_clk_sync(phy);
> +	} else {
> +		phy->clk = devm_clk_get_optional_enabled(&client->dev, NULL);
> +		if (IS_ERR(phy->clk))
> +			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;
>   
> @@ -205,13 +319,17 @@ 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_DRIVER_NAME, .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[] __maybe_unused = {
> -	{ .compatible = "samsung,s3fwrn5-i2c", },
> +static const struct of_device_id of_s3fwrn5_i2c_match[] = {
> +	{ .compatible = "samsung,s3fwrn5-i2c",
> +	  .data = (void *)S3FWRN5_VARIANT_FWDL, },
> +	{ .compatible = "samsung,s3nrn4v",
> +	  .data = (void *)S3FWRN5_VARIANT_S3NRN4V, },

is S3NRN4V really a variant of S3FWRN5 or is it just S3NRN4V?

>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, of_s3fwrn5_i2c_match);
> @@ -219,7 +337,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,
> diff --git a/drivers/nfc/s3fwrn5/nci.c b/drivers/nfc/s3fwrn5/nci.c
> index 5a9de11bbece..7034fb810e18 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,7 @@ 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] = {
> +const struct nci_driver_ops s3fwrn5_nci_prop_ops[5] = {
>   	{
>   		.opcode = nci_opcode_pack(NCI_GID_PROPRIETARY,
>   				NCI_PROP_SET_RFREG),
> @@ -41,6 +44,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_prop_rsp,
> +	},
>   };
>   
>   #define S3FWRN5_RFREG_SECTION_SIZE 252
> @@ -117,3 +125,112 @@ int s3fwrn5_nci_rf_configure(struct s3fwrn5_info *info, const char *fw_name)
>   	release_firmware(fw);
>   	return ret;
>   }
> +
> +/*
> + * Configure the reference clock. The S3NRN4V expects the single-byte FW_CFG
> + * form (just the clock-speed selector). The downstream stack sends this in the
> + * bootloader before CORE_RESET; the earliest the mainline NCI core lets us in
> + * is the ->setup hook (after CORE_RESET, before CORE_INIT), which works.
> + */
> +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);
> +}
> +
> +/*
> + * S3NRN4V RF register update. The HW and SW register blobs are merged into a
> + * single stream (HW first) and pushed via the DUAL_OPTION command:
> + * 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).
> + */

While it's "register update" and function is named "configure_dual", it's 
loading firmware.

If it's not a firmware, but only configuration, it can reside inside the driver, 
maybe LLM even be able to decode to understandable sequence of registers and values.

For next revision of the patch, I'll likely still have some additional feedback.

With next revision send also as last patch the device-tree entry for the 
Fairphone 6, so we can also get additional testing from developers/users.

Thank you again!
David

[...]

Re: [PATCH net-next v2 2/2] nfc: s3fwrn5: support the S3NRN4V variant

[Jorijn van der Graaf]

Hello David,

Thank you for the review!

On 19/07/2026 15:15, David Heidelberg wrote:
> Please, send the "drop the of_match_ptr()/__maybe_unused annotations
> from it." type of change as part of the series, but as a separate
> commit before the new HW support introduction.

Will do in v3.

> Since you touch S3FWRN5_I2C_DRIVER_NAME, replace define
> S3FWRN5_I2C_DRIVER_NAME occurenced with the "s3fwrn5_i2c" directly
> before introducing the support (also separate commit)

Will do, also in v3.

> is S3NRN4V really a variant of S3FWRN5 or is it just S3NRN4V?

It is a separate, later part, but from the same Samsung S.LSI NFC
controller line this driver covers. Samsung's downstream stack drives
that whole line with one kernel driver and one HAL: the HAL's product
table lists the N5 (S3FWRN5) and N82 (S3FWRN82) generations next to
RN4V (S3NRN4V) and others, dispatching on a product code reported by
the chip's bootloader, and the parts share the I2C framing, the
power-control GPIO scheme and the proprietary-NCI style of
configuration. The generational differences (bootloader protocol,
RF-register transport command, FW_CFG payload form) are exactly what
this patch dispatches on -- the same way the driver already supports
the S3FWRN82 next to the S3FWRN5.

That said, "S3FWRN5-family" can indeed be read as "a variant of the
S3FWRN5 chip", which it is not, so I'll reword it in v3 to something
like "a later part of the same Samsung NFC controller line". The
phrase also sits in the commit message of the already-acked binding
patch; I'll tweak it there too (commit message only) and note it in
the changelog.

> While it's "register update" and function is named "configure_dual",
> it's loading firmware.
>
> If it's not a firmware, but only configuration, it can reside inside
> the driver, maybe LLM even be able to decode to understandable
> sequence of registers and values.

There are two separate things here: the chip's executable firmware
(~180 KiB) ships in its flash and is not touched by this patch at all
-- its download protocol is not implemented, which is why the
download step is skipped. What is loaded here are only the two RF
register tables (~3.5 KiB combined).

Those tables are configuration by nature, but I don't think they can
reside in the driver:

- They are board-specific analog/RF tuning, not chip constants: the
  values match a particular antenna/matching-network design, and the
  vendor revises them across software releases (my two Fairphone 6
  units shipped different builds of these files, with different
  version stamps embedded). A different S3NRN4V board design would be
  expected to ship its own tables. Per-device data loaded at runtime
  is what request_firmware() is there for, much like Wi-Fi
  board/calibration files. The tables ship in the device's vendor
  image, which is where I extracted them from.

- There is nothing to decode them against. The register map of these
  controllers is not publicly documented, and even Samsung's own
  (Apache-licensed) HAL treats the register content as opaque: the
  only part of the image it interprets is a 16-byte metadata trailer
  at its end (version stamps, used to decide whether an update is
  needed at all, plus a region code), while the register content
  itself is pushed to the chip untouched, in 252-byte sections.
  Nothing in the stream or in the vendor stack identifies
  address/value pairs one could transcribe, so "decoded" into the
  driver this could only become a 3.5 KiB hex array in C, and we
  would lose the ability to ship a newer table without rebuilding
  the kernel.

- It also mirrors what this driver already does for the parts it
  supports: s3fwrn5_nci_rf_configure() loads the same class of table
  (sec_s3fwrn5_rfreg.bin) with request_firmware() and pushes it via
  the older START/SET/STOP_RFREG commands. The new function is the
  same operation over the newer parts' transport command.

If the naming reads confusingly I'm happy to rename the function or
extend its comment to spell out the firmware-vs-register-table
distinction.

> For next revision of the patch, I'll likely still have some
> additional feedback.

Understood -- I'll send the v3 with all of the above shortly.

> With next revision send also as last patch the device-tree entry for
> the Fairphone 6, so we can also get additional testing from
> developers/users.

Will do -- v3 will carry the Fairphone 6 DT patch at the end of the
series, marked as included for testing and presumably to be picked up
via the Qualcomm DT tree once the driver side is settled; I'll Cc
linux-arm-msm and the qcom maintainers on that patch.

Thanks again,
Jorijn

Re: [PATCH net-next v2 2/2] nfc: s3fwrn5: support the S3NRN4V variant

[David Heidelberg]

On 19/07/2026 16:22, Jorijn van der Graaf wrote:
> Hello David,
> 
> Thank you for the review!
> 
> On 19/07/2026 15:15, David Heidelberg wrote:
>> Please, send the "drop the of_match_ptr()/__maybe_unused annotations
>> from it." type of change as part of the series, but as a separate
>> commit before the new HW support introduction.
> 
> Will do in v3.
> 
>> Since you touch S3FWRN5_I2C_DRIVER_NAME, replace define
>> S3FWRN5_I2C_DRIVER_NAME occurenced with the "s3fwrn5_i2c" directly
>> before introducing the support (also separate commit)
> 
> Will do, also in v3.
> 
>> is S3NRN4V really a variant of S3FWRN5 or is it just S3NRN4V?
> 
> It is a separate, later part, but from the same Samsung S.LSI NFC
> controller line this driver covers. Samsung's downstream stack drives
> that whole line with one kernel driver and one HAL: the HAL's product
> table lists the N5 (S3FWRN5) and N82 (S3FWRN82) generations next to
> RN4V (S3NRN4V) and others, dispatching on a product code reported by
> the chip's bootloader, and the parts share the I2C framing, the
> power-control GPIO scheme and the proprietary-NCI style of
> configuration. The generational differences (bootloader protocol,
> RF-register transport command, FW_CFG payload form) are exactly what
> this patch dispatches on -- the same way the driver already supports
> the S3FWRN82 next to the S3FWRN5.
> 
> That said, "S3FWRN5-family" can indeed be read as "a variant of the
> S3FWRN5 chip", which it is not, so I'll reword it in v3 to something
> like "a later part of the same Samsung NFC controller line". The
> phrase also sits in the commit message of the already-acked binding
> patch; I'll tweak it there too (commit message only) and note it in
> the changelog.
> 
>> While it's "register update" and function is named "configure_dual",
>> it's loading firmware.
>>
>> If it's not a firmware, but only configuration, it can reside inside
>> the driver, maybe LLM even be able to decode to understandable
>> sequence of registers and values.
> 
> There are two separate things here: the chip's executable firmware
> (~180 KiB) ships in its flash and is not touched by this patch at all
> -- its download protocol is not implemented, which is why the
> download step is skipped. What is loaded here are only the two RF
> register tables (~3.5 KiB combined).
> 
> Those tables are configuration by nature, but I don't think they can
> reside in the driver:
> 
> - They are board-specific analog/RF tuning, not chip constants: the
>    values match a particular antenna/matching-network design, and the
>    vendor revises them across software releases (my two Fairphone 6
>    units shipped different builds of these files, with different
>    version stamps embedded). A different S3NRN4V board design would be
>    expected to ship its own tables. Per-device data loaded at runtime
>    is what request_firmware() is there for, much like Wi-Fi
>    board/calibration files. The tables ship in the device's vendor
>    image, which is where I extracted them from.
> 
> - There is nothing to decode them against. The register map of these
>    controllers is not publicly documented, and even Samsung's own
>    (Apache-licensed) HAL treats the register content as opaque: the
>    only part of the image it interprets is a 16-byte metadata trailer
>    at its end (version stamps, used to decide whether an update is
>    needed at all, plus a region code), while the register content
>    itself is pushed to the chip untouched, in 252-byte sections.
>    Nothing in the stream or in the vendor stack identifies
>    address/value pairs one could transcribe, so "decoded" into the
>    driver this could only become a 3.5 KiB hex array in C, and we
>    would lose the ability to ship a newer table without rebuilding
>    the kernel.
> 
> - It also mirrors what this driver already does for the parts it
>    supports: s3fwrn5_nci_rf_configure() loads the same class of table
>    (sec_s3fwrn5_rfreg.bin) with request_firmware() and pushes it via
>    the older START/SET/STOP_RFREG commands. The new function is the
>    same operation over the newer parts' transport command.
> 
> If the naming reads confusingly I'm happy to rename the function or
> extend its comment to spell out the firmware-vs-register-table
> distinction.

Thanks,

now it makes more sense to me, feel free to name it as calibration data.

I would suggest to introduce something as a calibration-variant (see ath10k code).

If I understand right, firmware location path could look like

default path + driver vendor and model + device vendor and model + revision

/lib/firmware/ + Samsung/s3nrn4v/ + Fairphone/FP5/hwrevision.bin

/cc Luca here, as he may know more about the different configuration data shipped.

We should assume the configuration will be shipped with linux-firmware at some 
point.

David

> 
>> For next revision of the patch, I'll likely still have some
>> additional feedback.
> 
> Understood -- I'll send the v3 with all of the above shortly.
> 
>> With next revision send also as last patch the device-tree entry for
>> the Fairphone 6, so we can also get additional testing from
>> developers/users.
> 
> Will do -- v3 will carry the Fairphone 6 DT patch at the end of the
> series, marked as included for testing and presumably to be picked up
> via the Qualcomm DT tree once the driver side is settled; I'll Cc
> linux-arm-msm and the qcom maintainers on that patch.
> 
> Thanks again,
> Jorijn

-- 
David Heidelberg

Re: [PATCH net-next v2 2/2] nfc: s3fwrn5: support the S3NRN4V variant

[Jorijn van der Graaf]

Hello David,

On 19/07/2026 16:56, David Heidelberg wrote:
> now it makes more sense to me, feel free to name it as calibration
> data.

Will do -- v3 will say "RF calibration data" in the commit message and
comments.

> I would suggest to introduce something as a calibration-variant (see
> ath10k code).
>
> If I understand right, firmware location path could look like
>
> default path + driver vendor and model + device vendor and model +
> revision
>
> /lib/firmware/ + Samsung/s3nrn4v/ + Fairphone/FP5/hwrevision.bin

That layout makes sense to me. My inclination for v3:

- request the files from a chip-scoped directory:
  samsung/s3nrn4v/hwreg.bin + samsung/s3nrn4v/swreg.bin (the existing
  parts' sec_s3fwrn5_rfreg.bin is untouched);

- an optional samsung,calibration-variant string property (modelled
  on ath10k's qcom,calibration-variant) that, when set, makes the
  driver try a device-scoped location first, e.g.
  samsung/s3nrn4v/fairphone-fp6/hwreg.bin, falling back to the bare
  files above.

The variant lookup can also be added compatibly later, once a second
S3NRN4V design (or a diverging FP6 revision) actually appears -- so
if you'd rather keep v3 minimal, I'd do only the directory move now
and add the property when first needed. Either works for me.

So far I have seen no evidence of per-revision or per-region variants
on the FP6: the only difference I have identified between my two
units' tables is the vendor software release (build-date stamps).
But Fairphone would know better.

> /cc Luca here, as he may know more about the different configuration
> data shipped.

Luca, two questions your way:

- do FP6 hardware revisions (or regional SKUs) ship different NFC
  hwreg/swreg tables, or is it one set per software release?

- could Fairphone submit these tables to linux-firmware? I assume
  the redistribution question sits with Fairphone/Samsung, and having
  them there would make NFC work out of the box on mainline.

> We should assume the configuration will be shipped with
> linux-firmware at some point.

Agreed -- that is exactly why I'd like to settle the path scheme now.

v3 is otherwise ready, including the Fairphone 6 DT patch; rather
than sending it right away as I said earlier today, I'll give this
a few days for Luca's input and then send it.

Thanks,
Jorijn