Date: Fri, 8 Dec 2017 17:22:09 +0100
From: Karsten Merker <merker@debian.org>
To: Palmer Dabbelt <palmer@dabbelt.com>
Cc: patches@groups.riscv.org, gregkh@linuxfoundation.org, jslaby@suse.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tty: New RISC-V SBI console driver
Message-ID: <20171208162209.52bkebfvw7mirbit@excalibur.cnev.de>
References: <20171208001015.27735-1-palmer@sifive.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20171208001015.27735-1-palmer@sifive.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2372
Lines: 61

On Thu, Dec 07, 2017 at 04:10:15PM -0800, Palmer Dabbelt wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
> 
> The RISC-V ISA defines a simple console that is availiable via SBI calls
> on all systems.  This patch adds a driver for this console interface
> that can act as both a target for early printk and as the system
> console.  The core arch code already enables the early printk support
> when CONFIG_HVC_RISCV_SBI is defined.
> 
> There is one checkpatch.pl warning here: to check the MAINTAINERS file.
> They're all matched by the "K: riscv" line.
> 
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>

> diff --git a/arch/riscv/include/asm/hvc_riscv_sbi.h b/arch/riscv/include/asm/hvc_riscv_sbi.h
> new file mode 100644
> index 000000000000..41723ed7bd97
> --- /dev/null
> +++ b/arch/riscv/include/asm/hvc_riscv_sbi.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _ASM_RISCV_HVC_RISCV_SBI_H
> +#define _ASM_RISCV_HVC_RISCV_SBI_H
[...]
> diff --git a/drivers/tty/hvc/hvc_riscv_sbi.c b/drivers/tty/hvc/hvc_riscv_sbi.c
> new file mode 100644
> index 000000000000..83ef0dcc24e0
> --- /dev/null
> +++ b/drivers/tty/hvc/hvc_riscv_sbi.c
> @@ -0,0 +1,75 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <linux/console.h>
[...]

Hello Palmer,

just a non-technical nitpick: the SPDX-License-Identifier states
the license, but not the copyright holder(s).  Could you please
add an appropriate copyright line to each new file, i.e. 
something in the form of "copyright <year> <author>"?

Although one can try to determine authorship from the git
history, IMHO authorship should be noted explicitly in each file,
as that information then sticks to the file whatever happens to
it.  Files might be reused outside the kernel (e.g. in u-boot,
which reuses quite a bit of Linux driver code), and in those
cases the connection between the file and its original git
history gets lost.  The same is true for example for the kernel
releases in form of release tarballs which of course don't
contain any git metadata. I therefore think it is good practice
to always include the copyright/authorship information in
each file.

Regards,
Karsten
-- 
Gem. Par. 28 Abs. 4 Bundesdatenschutzgesetz widerspreche ich der Nutzung
sowie der Weitergabe meiner personenbezogenen Daten für Zwecke der
Werbung sowie der Markt- oder Meinungsforschung.