Message-ID: <20260206-bulky-aardwark-of-examination-b056ee@quoll>
Date: Fri, 6 Feb 2026 10:27:09 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: 434779359@...com
Cc: Linus Walleij <linusw@...nel.org>, 
	Bartosz Golaszewski <brgl@...nel.org>, linux-kernel@...r.kernel.org, netdev@...r.kernel.org, 
	linux-gpio@...r.kernel.org, xuchen <bright.xu@...ot.com>
Subject: Re: [PATCH] nfc: supply: Add PN7160 drivers   This patch adds support for the PN7160 ICs used in
 Qualcomm reference designs.

On Fri, Feb 06, 2026 at 01:31:10AM -0500, 434779359@...com wrote:
> From: xuchen <bright.xu@...ot.com>
> 

This patch fails on so many levels that I am not going to provide any
deep review. If you followed basic guidelines, basic patch submission
guides (e.g. see Michael Opdenacker talk on this year FOSDEM), tutorials
or docs, you would solve all of the trivialities.

You did not read these basic docs, so do not be surprised if your
contributions will be entirely ignored.

Please use subject prefixes matching the subsystem. You can get them for
example with 'git log --oneline -- DIRECTORY_OR_FILE' on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

There is no "supply" here.

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

Please run standard kernel tools for static analysis, like coccinelle,
smatch and sparse, and fix reported warnings. Also please check for
warnings when building with W=1 for gcc and clang. Most of these
commands (checks or W=1 build) can build specific targets, like some
directory, to narrow the scope to only your code. The code here looks
like it needs a fix. Feel free to get in touch if the warning is not
clear.

> Signed-off-by: xuchen <434779359@...com>
> ---
>  drivers/nfc/pn7160/Kconfig    |  40 +++
>  drivers/nfc/pn7160/Makefile   |   6 +
>  drivers/nfc/pn7160/common.c   | 371 ++++++++++++++++++++++++
>  drivers/nfc/pn7160/common.h   |  36 +++
>  drivers/nfc/pn7160/i2c_drv.c  | 531 ++++++++++++++++++++++++++++++++++
>  drivers/nfc/pn7160/i2c_drv.h  |  26 ++
>  drivers/nfc/pn7160/platform.h | 164 +++++++++++
>  7 files changed, 1174 insertions(+)
>  create mode 100644 drivers/nfc/pn7160/Kconfig
>  create mode 100644 drivers/nfc/pn7160/Makefile
>  create mode 100644 drivers/nfc/pn7160/common.c
>  create mode 100644 drivers/nfc/pn7160/common.h
>  create mode 100644 drivers/nfc/pn7160/i2c_drv.c
>  create mode 100644 drivers/nfc/pn7160/i2c_drv.h
>  create mode 100644 drivers/nfc/pn7160/platform.h
> 
> diff --git a/drivers/nfc/pn7160/Kconfig b/drivers/nfc/pn7160/Kconfig
> new file mode 100644
> index 000000000000..fb497bc33059
> --- /dev/null
> +++ b/drivers/nfc/pn7160/Kconfig
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# near field communication configuration
> +#
> +
> +config NXP_NFC_I2C
> +	tristate "NFC I2C Slave driver for NXP-NFCC"
> +	depends on I2C
> +	help
> +	  This enables the NFC driver for PN71xx based devices.
> +	  This is for I2C connected version. NCI protocol logic
> +	  resides in the usermode and it has no other NFC dependencies.
> +
> +	  If unsure, say N.
> +
> +config NXP_NFC_SPI
> +	tristate "NFC SPI Slave driver for NXP-NFCC"
> +	depends on SPI
> +	help
> +	  This enables the NFC driver for PN71xx based devices.
> +	  This is for SPI connected version. NCI protocol logic
> +	  resides in the usermode and it has no other NFC dependencies.
> +
> +	  If unsure, say N.
> +
> +config NXP_NFC_RECOVERY
> +	bool "NXP based NFC minimal FW update support"
> +	depends on NXP_NFC_I2C && I2C
> +	default y
> +	help
> +	  This enables NFC minimal FW update.
> +	  This feature allows updating the firmware of NXP NFC controllers
> +	  in recovery mode. It is required for field updates and bug fixes.
> +	  The driver will handle the download mode and firmware transfer
> +	  when this option is enabled.
> +
> +	  If unsure, say N.
> +
> +source "drivers/nfc/pn7160/Kconfig"
> +endif
> diff --git a/drivers/nfc/pn7160/Makefile b/drivers/nfc/pn7160/Makefile
> new file mode 100644
> index 000000000000..8c6f670eaa7a
> --- /dev/null
> +++ b/drivers/nfc/pn7160/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Makefile for nfc devices
> +#
> +obj-m			+= nxpnfc_i2c.o
> +nxpnfc_i2c-objs		:= common.o i2c_drv.o
> diff --git a/drivers/nfc/pn7160/common.c b/drivers/nfc/pn7160/common.c
> new file mode 100644
> index 000000000000..cd433912764b
> --- /dev/null
> +++ b/drivers/nfc/pn7160/common.c
> @@ -0,0 +1,371 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2015, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2019-2021 NXP
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +
> +#include "common.h"
> +
> +int nfc_parse_dt(struct device *dev, struct platform_configs *nfc_configs,
> +		 uint8_t interface)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct platform_gpio *nfc_gpio = &nfc_configs->gpio;
> +
> +	if (!np) {
> +		pr_err("%s: nfc of_node NULL\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	nfc_gpio->irq = -EINVAL;
> +	nfc_gpio->dwl_req = -EINVAL;
> +	nfc_gpio->ven = -EINVAL;
> +
> +	/* irq required for i2c based chips only */
> +	if (interface == PLATFORM_IF_I2C || interface == PLATFORM_IF_SPI) {
> +		nfc_gpio->irq = of_get_named_gpio(np, DTS_IRQ_GPIO_STR, 0);
> +		if ((!gpio_is_valid(nfc_gpio->irq))) {
> +			pr_err("%s: irq gpio invalid %d\n", __func__,
> +			       nfc_gpio->irq);
> +			return -EINVAL;
> +		}
> +		pr_info("%s: irq %d\n", __func__, nfc_gpio->irq);

Drivers use dev_xxx and there is no need to put __func__ everywhere.

There is even no need for this debug print. Read coding style.

This is such a poor code...

> +	}
> +	nfc_gpio->ven = of_get_named_gpio(np, DTS_VEN_GPIO_STR, 0);
> +	if ((!gpio_is_valid(nfc_gpio->ven))) {
> +		pr_err("%s: ven gpio invalid %d\n", __func__, nfc_gpio->ven);
> +		return -EINVAL;
> +	}
> +	/* some products like sn220 does not required fw dwl pin */
> +	nfc_gpio->dwl_req = of_get_named_gpio(np, DTS_FWDN_GPIO_STR, 0);
> +	if ((!gpio_is_valid(nfc_gpio->dwl_req)))
> +		pr_warn("%s: dwl_req gpio invalid %d\n", __func__,
> +			nfc_gpio->dwl_req);
> +
> +	pr_info("%s: %d, %d, %d\n", __func__, nfc_gpio->irq, nfc_gpio->ven,
> +		nfc_gpio->dwl_req);
> +	return 0;
> +}
> +EXPORT_SYMBOL(nfc_parse_dt);

No, why the heck you export internal functions? This is not even needed.

...


> +void nfc_misc_unregister(struct nfc_dev *nfc_dev, int count)
> +{
> +	pr_debug("%s: entry\n", __func__);

No, this is not accepted since long time.

> +	device_destroy(nfc_dev->nfc_class, nfc_dev->devno);
> +	cdev_del(&nfc_dev->c_dev);
> +	class_destroy(nfc_dev->nfc_class);
> +	unregister_chrdev_region(nfc_dev->devno, count);
> +}
> +EXPORT_SYMBOL(nfc_misc_unregister);
> +
> +int nfc_misc_register(struct nfc_dev *nfc_dev,
> +		      const struct file_operations *nfc_fops, int count,
> +		      char *devname, char *classname)
> +{
> +	int ret = 0;
> +
> +	ret = alloc_chrdev_region(&nfc_dev->devno, 0, count, devname);

So you just added custom interface? No, NAK.


> +	if (ret < 0) {
> +		pr_err("%s: failed to alloc chrdev region ret %d\n", __func__,
> +		       ret);
> +		return ret;
> +	}
> +	nfc_dev->nfc_class = class_create(classname);
> +	if (IS_ERR(nfc_dev->nfc_class)) {
> +		ret = PTR_ERR(nfc_dev->nfc_class);
> +		pr_err("%s: failed to register device class ret %d\n", __func__,
> +		       ret);
> +		unregister_chrdev_region(nfc_dev->devno, count);
> +		return ret;
> +	}
> +	cdev_init(&nfc_dev->c_dev, nfc_fops);
> +	ret = cdev_add(&nfc_dev->c_dev, nfc_dev->devno, count);
> +	if (ret < 0) {
> +		pr_err("%s: failed to add cdev ret %d\n", __func__, ret);
> +		class_destroy(nfc_dev->nfc_class);
> +		unregister_chrdev_region(nfc_dev->devno, count);
> +		return ret;
> +	}
> +	nfc_dev->nfc_device = device_create(nfc_dev->nfc_class, NULL,
> +					    nfc_dev->devno, nfc_dev, devname);
> +	if (IS_ERR(nfc_dev->nfc_device)) {
> +		ret = PTR_ERR(nfc_dev->nfc_device);
> +		pr_err("%s: failed to create the device ret %d\n", __func__,
> +		       ret);
> +		cdev_del(&nfc_dev->c_dev);
> +		class_destroy(nfc_dev->nfc_class);
> +		unregister_chrdev_region(nfc_dev->devno, count);
> +		return ret;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(nfc_misc_register);
> +
> +/**
> + * nfc_ioctl_power_states() - power control
> + * @nfc_dev:    nfc device data structure
> + * @arg:    mode that we want to move to
> + *
> + * Device power control. Depending on the arg value, device moves to
> + * different states, refer platform.h for args
> + *
> + * Return: -ENOIOCTLCMD if arg is not supported, 0 in any other case
> + */
> +static int nfc_ioctl_power_states(struct nfc_dev *nfc_dev, unsigned long arg)
> +{
> +	int ret = 0;
> +	struct platform_gpio *nfc_gpio = &nfc_dev->configs.gpio;
> +
> +	if (arg == NFC_POWER_OFF) {

No. Look at other drivers how this is handled.

NAK

...

> diff --git a/drivers/nfc/pn7160/common.h b/drivers/nfc/pn7160/common.h
> new file mode 100644
> index 000000000000..2451db295fc8
> --- /dev/null
> +++ b/drivers/nfc/pn7160/common.h
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2015, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2019-2021 NXP
> + */
> +
> +#ifndef _COMMON_H_
> +#define _COMMON_H_
> +
> +#include "platform.h"
> +
> +/* 函数声明 */

Super, to teraz będziemy pisać w naszych własnych językach? Uważam to za
niedopuszczalne.


...

> +static const struct i2c_device_id nfc_i2c_dev_id[] = {
> +	{ "nxpnfc", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, nfc_i2c_dev_id);
> +
> +static const struct of_device_id nfc_i2c_dev_match_table[] = {
> +	{ .compatible = "nxp,nxpnfc" },

Undocumented ABI. Really, you could have followed basic guidelines. This
is not really acceptable.

NAK

Best regards,
Krzysztof

Powered by blists - more mailing lists