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.j...@samsung.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
