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

On Fri, Aug 07, 2026 at 04:10:00AM +0200, Jorijn van der Graaf wrote:
> +/*
> + * S3NRN4V RF calibration data update: the HW and SW blobs merged into one
> + * stream (HW first), pushed as START_UPDATE, one SET_OPTION per 252-byte
> + * section, then STOP_UPDATE carrying a 16-bit checksum (running sum of the
> + * merged stream as 32-bit words).
> + */
> +int s3fwrn5_nci_rf_configure_dual(struct s3fwrn5_info *info,
> +				  const char *hw_name, const char *sw_name)
> +{
> +	struct nci_prop_dual_set_option_cmd set_option;
> +	struct device *dev = &info->ndev->nfc_dev->dev;
> +	const struct firmware *hw_fw, *sw_fw;
> +	size_t merged_size, i, len;
> +	u8 *merged = NULL;
> +	u8 stop_cmd[3];
> +	u32 checksum;
> +	u8 sub_oid;
> +	int ret;
> +
> +	ret = firmware_request_nowarn(&hw_fw, hw_name, dev);
> +	if (ret < 0)
> +		return ret;
> +	ret = firmware_request_nowarn(&sw_fw, sw_name, dev);
> +	if (ret < 0)
> +		goto out_hw;
> +
> +	merged_size = hw_fw->size + sw_fw->size;
> +
> +	/*
> +	 * The stream is checksummed as 32-bit words and pushed in at most 256
> +	 * sections (the section index is a single byte); reject blobs that
> +	 * would silently break either.
> +	 */
> +	if (!merged_size || merged_size % 4 ||
> +	    merged_size > 256 * NCI_PROP_DUAL_SECTION_SIZE) {
> +		dev_err(dev, "invalid calibration data size: %zu\n", merged_size);
> +		ret = -EINVAL;
> +		goto out;

Why does this error path kfrees the 'merged'? It's not yet allocated.

> +	}
> +
> +	/*
> +	 * 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;

Same here

> +	}
> +
> +	merged = kvmalloc(merged_size, GFP_KERNEL);
> +	if (!merged) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	memcpy(merged, hw_fw->data, hw_fw->size);
> +	memcpy(merged + hw_fw->size, sw_fw->data, sw_fw->size);
> +
> +	checksum = 0;
> +	for (i = 0; i + 4 <= merged_size; i += 4)
> +		checksum += get_unaligned_le32(merged + i);
> +
> +	dev_info(dev, "calibration data update: %s + %s\n", hw_name, sw_name);

Drop

> +
> +	/* START_UPDATE */
> +	sub_oid = NCI_PROP_DUAL_SUB_START_UPDATE;
> +	ret = nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 1, &sub_oid);
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to start calibration data update\n");
> +		goto out;
> +	}
> +
> +	/* SET_OPTION per section */
> +	set_option.sub_oid = NCI_PROP_DUAL_SUB_SET_OPTION;
> +	set_option.index = 0;
> +	for (i = 0; i < merged_size; i += NCI_PROP_DUAL_SECTION_SIZE) {
> +		len = min_t(size_t, merged_size - i, NCI_PROP_DUAL_SECTION_SIZE);
> +		memcpy(set_option.data, merged + i, len);
> +		ret = nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION,
> +				   len + 2, (__u8 *)&set_option);
> +		if (ret < 0) {
> +			dev_err(dev, "calibration data update error: %d\n",
> +				ret);
> +			/* Abort form: STOP_UPDATE with the sub-OID alone. */
> +			sub_oid = NCI_PROP_DUAL_SUB_STOP_UPDATE;
> +			nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 1,
> +				     &sub_oid);
> +			goto out;
> +		}
> +		set_option.index++;
> +	}
> +
> +	/* STOP_UPDATE with checksum */
> +	stop_cmd[0] = NCI_PROP_DUAL_SUB_STOP_UPDATE;
> +	put_unaligned_le16(checksum, &stop_cmd[1]);
> +	ret = nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 3, stop_cmd);
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to stop calibration data update\n");
> +		goto out;
> +	}
> +
> +	dev_info(dev, "calibration data update: success\n");

dev_dbg, drivers should be silent on success, unless this is somehow
important and unusual message.

> +out:
> +	kvfree(merged);
> +	release_firmware(sw_fw);
> +out_hw:
> +	release_firmware(hw_fw);
> +	return ret;

Best regards,
Krzysztof