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

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