DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4D8CF21526
Date:   Wed, 17 Oct 2018 13:53:57 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Lars Poeschel <poeschel@lemonage.de>
Cc:     Samuel Ortiz <sameo@linux.intel.com>, Jiri Slaby <jslaby@suse.com>,
        Kees Cook <keescook@chromium.org>,
        linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org
Subject: Re: [PATCH 1/3] nfc: pn533: add UART phy driver
Message-ID: <20181017115357.GC32302@kroah.com>
References: <20181017102037.28537-1-poeschel@lemonage.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181017102037.28537-1-poeschel@lemonage.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Wed, Oct 17, 2018 at 12:20:35PM +0200, Lars Poeschel wrote:
> This adds the UART phy interface for the pn533 driver.
> The pn533 driver can be used through UART interface this way.
> It is implemented as a tty line discipline.

Why not use the serdev interface instead?

> --- /dev/null
> +++ b/drivers/nfc/pn533/uart.c
> @@ -0,0 +1,399 @@
> +/*
> + * Driver for NXP PN532 NFC Chip - UART transport layer
> + *
> + * Copyright (C) 2018 Lemonage Software GmbH
> + * Author: Lars Poeschel <poeschel@lemonage.de>
> + * All rights reserved.

Please use checkpatch.pl on patches so you do not get grumpy maintainers
telling you to use checkpatch.pl on your patches :)

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.

These three paragraphs can all be dropped because you are going to add
the correct SPDX line at the top of this file, right?  :)


> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/nfc.h>
> +#include <linux/netdevice.h>
> +#include <net/nfc/nfc.h>
> +#include "pn533.h"
> +
> +#define VERSION "0.1"
> +
> +#define PN532_I2C_DRIVER_NAME "pn532_uart"
> +
> +#define PN532_MAGIC		0x162f

The whole old model of "magic" tty numbers can just be removed.  It
never really did anything except to try to protect you from other broken
kernel code.  You can drop it all if you really do end up making a tty
device here.

thanks,

greg k-h