netdev - Re: [PATCH] nfc: s3fwrn82: Add driver for Samsung S3FWRN82 NFC Chip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20201112171855.0f942722@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 12 Nov 2020 17:18:55 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Bongsu Jeon <bongsu.jeon@...sung.com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] nfc: s3fwrn82: Add driver for Samsung S3FWRN82 NFC Chip

On Thu, 12 Nov 2020 17:20:47 +0900 Bongsu Jeon wrote:
> Add driver for Samsung S3FWRN82 NFC controller.
> S3FWRN82 is using NCI protocol and I2C communication interface.
> 
> Signed-off-by: bongsujeon <bongsu.jeon@...sung.com>

Please put [PATCH net-next] in the subject so we know this will go into
the net-next tree.

> diff --git a/Documentation/devicetree/bindings/net/nfc/s3fwrn82.txt b/Documentation/devicetree/bindings/net/nfc/s3fwrn82.txt
> new file mode 100644
> index 000000000000..03ed880e1c7f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nfc/s3fwrn82.txt
> @@ -0,0 +1,30 @@
> +* Samsung S3FWRN82 NCI NFC Controller
> +
> +Required properties:
> +- compatible: Should be "samsung,s3fwrn82-i2c".
> +- reg: address on the bus
> +- interrupts: GPIO interrupt to which the chip is connected
> +- en-gpios: Output GPIO pin used for enabling/disabling the chip
> +- wake-gpios: Output GPIO pin used to enter firmware mode and
> +  sleep/wakeup control
> +
> +Example:
> +
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c4 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        s3fwrn82@27 {
> +            compatible = "samsung,s3fwrn82-i2c";
> +            reg = <0x27>;
> +
> +            interrupt-parent = <&gpa1>;
> +            interrupts = <3 IRQ_TYPE_LEVEL_HIGH>;
> +
> +            en-gpios = <&gpf1 4 GPIO_ACTIVE_HIGH>;
> +            wake-gpios = <&gpj0 2 GPIO_ACTIVE_HIGH>;
> +        };
> +    };

AFAIK the device tree bindings need to be in a separate patch, and you
need to CC the device tree mailing list and maintainers on that patch.

> +config NFC_S3FWRN82
> +	tristate
> +	help
> +	  Core driver for Samsung S3FWRN82 NFC chip. Contains core utilities
> +	  of chip. It's intended to be used by PHYs to avoid duplicating lots
> +	  of common code.

If this is only selected by other drivers you can skip the help and
make this option invisible in kconfig.

> +config NFC_S3FWRN82_I2C
> +	tristate "Samsung S3FWRN82 I2C support"
> +	depends on NFC_NCI && I2C
> +	select NFC_S3FWRN82
> +	default n

default n is unnecessary, when default is not specified 'n' is already
the default

> +	help
> +	  This module adds support for an I2C interface to the S3FWRN82 chip.
> +	  Select this if your platform is using the I2C bus.
> +
> +	  To compile this driver as a module, choose m here. The module will
> +	  be called s3fwrn82_i2c.ko.
> +	  Say N if unsure.

> +#define S3FWRN82_NFC_PROTOCOLS  (NFC_PROTO_JEWEL_MASK | \
> +				NFC_PROTO_MIFARE_MASK | \
> +				NFC_PROTO_FELICA_MASK | \
> +				NFC_PROTO_ISO14443_MASK | \
> +				NFC_PROTO_ISO14443_B_MASK | \
> +				NFC_PROTO_ISO15693_MASK)
> +
> +static int s3fwrn82_nci_open(struct nci_dev *ndev)
> +{
> +	struct s3fwrn82_info *info = nci_get_drvdata(ndev);
> +
> +	if (s3fwrn82_get_mode(info) != S3FWRN82_MODE_COLD)
> +		return  -EBUSY;

double space


> +int s3fwrn82_probe(struct nci_dev **ndev, void *phy_id, struct device *pdev,
> +	const struct s3fwrn82_phy_ops *phy_ops)

Please align the continuation lines properly. Please use checkpatch
--strict to check your patches.

> +{
> +	struct s3fwrn82_info *info;
> +	int ret;
> +
> +	info = devm_kzalloc(pdev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->phy_id = phy_id;
> +	info->pdev = pdev;
> +	info->phy_ops = phy_ops;
> +	mutex_init(&info->mutex);
> +
> +	s3fwrn82_set_mode(info, S3FWRN82_MODE_COLD);
> +
> +	info->ndev = nci_allocate_device(&s3fwrn82_nci_ops,
> +		S3FWRN82_NFC_PROTOCOLS, 0, 0);

same here

> +	if (!info->ndev)
> +		return -ENOMEM;
> +
> +	nci_set_parent_dev(info->ndev, pdev);
> +	nci_set_drvdata(info->ndev, info);
> +
> +	ret = nci_register_device(info->ndev);
> +	if (ret < 0) {
> +		nci_free_device(info->ndev);
> +		return ret;
> +	}
> +
> +	*ndev = info->ndev;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(s3fwrn82_probe);

> +static int s3fwrn82_i2c_parse_dt(struct i2c_client *client)
> +{
> +	struct s3fwrn82_i2c_phy *phy = i2c_get_clientdata(client);
> +	struct device_node *np = client->dev.of_node;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	phy->gpio_en = of_get_named_gpio(np, "en-gpios", 0);
> +	if (!gpio_is_valid(phy->gpio_en)) {
> +		return -ENODEV;
> +	}

brackets around single line statements are unnecessary

> +	phy->gpio_fw_wake = of_get_named_gpio(np, "wake-gpios", 0);
> +	if (!gpio_is_valid(phy->gpio_fw_wake)) {
> +		return -ENODEV;
> +	}

here as well

> +	return 0;
> +}

> +static inline int s3fwrn82_set_mode(struct s3fwrn82_info *info,
> +	enum s3fwrn82_mode mode)
> +{
> +	if (!info->phy_ops->set_mode)
> +		return -ENOTSUPP;

EOPNOTSUPP is a better error code, ENOTSUPP is internal to the kernel
and best avoided