[PATCH v2 1/3] tty: Address checkpatch warnings in goldfish.c

[PATCH v2 1/3] tty: Address checkpatch warnings in goldfish.c

[rkir]

From: Roman Kiryanov <rkir@google.com>

checkpatch.pl complained about missing blank lines
(between variable definitions and executable code)
and using just "unsigned" instead of "unsigned int".

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v2:
 - Updated the commit message.

 drivers/tty/goldfish.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/goldfish.c b/drivers/tty/goldfish.c
index 37caba7c3aff..a92fcb2b0002 100644
--- a/drivers/tty/goldfish.c
+++ b/drivers/tty/goldfish.c
@@ -172,6 +172,7 @@ static void goldfish_tty_shutdown(struct tty_port *port)
 static int goldfish_tty_open(struct tty_struct *tty, struct file *filp)
 {
 	struct goldfish_tty *qtty = &goldfish_ttys[tty->index];
+
 	return tty_port_open(&qtty->port, tty, filp);
 }
 
@@ -201,11 +202,12 @@ static int goldfish_tty_chars_in_buffer(struct tty_struct *tty)
 {
 	struct goldfish_tty *qtty = &goldfish_ttys[tty->index];
 	void __iomem *base = qtty->base;
+
 	return readl(base + GOLDFISH_TTY_REG_BYTES_READY);
 }
 
 static void goldfish_tty_console_write(struct console *co, const char *b,
-								unsigned count)
+				       unsigned int count)
 {
 	goldfish_tty_do_write(co->index, b, count);
 }
@@ -219,7 +221,7 @@ static struct tty_driver *goldfish_tty_console_device(struct console *c,
 
 static int goldfish_tty_console_setup(struct console *co, char *options)
 {
-	if ((unsigned)co->index >= goldfish_tty_line_count)
+	if ((unsigned int)co->index >= goldfish_tty_line_count)
 		return -ENODEV;
 	if (!goldfish_ttys[co->index].base)
 		return -ENODEV;
-- 
2.18.0.233.g985f88cf7e-goog

[PATCH v2 2/3] tty: Make constants to be enums instead of #define in goldfish.c

[rkir]

From: Roman Kiryanov <rkir@google.com>

enums produce better compilation errors than defines.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v2:
 - Added the enum types (goldfish_tty_reg and goldfish_tty_cmd).

 drivers/tty/goldfish.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/goldfish.c b/drivers/tty/goldfish.c
index a92fcb2b0002..173383f2a4c1 100644
--- a/drivers/tty/goldfish.c
+++ b/drivers/tty/goldfish.c
@@ -19,18 +19,22 @@
 #include <linux/serial_core.h>
 
 /* Goldfish tty register's offsets */
-#define	GOLDFISH_TTY_REG_BYTES_READY	0x04
-#define	GOLDFISH_TTY_REG_CMD		0x08
-#define	GOLDFISH_TTY_REG_DATA_PTR	0x10
-#define	GOLDFISH_TTY_REG_DATA_LEN	0x14
-#define	GOLDFISH_TTY_REG_DATA_PTR_HIGH	0x18
-#define	GOLDFISH_TTY_REG_VERSION	0x20
+enum goldfish_tty_reg {
+	GOLDFISH_TTY_REG_BYTES_READY	= 0x04,
+	GOLDFISH_TTY_REG_CMD		= 0x08,
+	GOLDFISH_TTY_REG_DATA_PTR	= 0x10,
+	GOLDFISH_TTY_REG_DATA_LEN	= 0x14,
+	GOLDFISH_TTY_REG_DATA_PTR_HIGH	= 0x18,
+	GOLDFISH_TTY_REG_VERSION	= 0x20,
+};
 
 /* Goldfish tty commands */
-#define	GOLDFISH_TTY_CMD_INT_DISABLE	0
-#define	GOLDFISH_TTY_CMD_INT_ENABLE	1
-#define	GOLDFISH_TTY_CMD_WRITE_BUFFER	2
-#define	GOLDFISH_TTY_CMD_READ_BUFFER	3
+enum goldfish_tty_cmd {
+	GOLDFISH_TTY_CMD_INT_DISABLE	= 0,
+	GOLDFISH_TTY_CMD_INT_ENABLE	= 1,
+	GOLDFISH_TTY_CMD_WRITE_BUFFER	= 2,
+	GOLDFISH_TTY_CMD_READ_BUFFER	= 3,
+};
 
 struct goldfish_tty {
 	struct tty_port port;
-- 
2.18.0.233.g985f88cf7e-goog

[PATCH v2 3/3] tty: Replace goldfish_tty_line_count with a #define

[rkir]

From: Roman Kiryanov <rkir@google.com>

The driver never mutates this variable - no benefits of
keeping it mutable.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v2:
 - Replaced "const u32" with "#define".

 drivers/tty/goldfish.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/goldfish.c b/drivers/tty/goldfish.c
index 173383f2a4c1..eb88db4f243c 100644
--- a/drivers/tty/goldfish.c
+++ b/drivers/tty/goldfish.c
@@ -49,10 +49,11 @@ struct goldfish_tty {
 
 static DEFINE_MUTEX(goldfish_tty_lock);
 static struct tty_driver *goldfish_tty_driver;
-static u32 goldfish_tty_line_count = 8;
 static u32 goldfish_tty_current_line_count;
 static struct goldfish_tty *goldfish_ttys;
 
+#define GOLDFISH_TTY_LINE_COUNT 8
+
 static void do_rw_io(struct goldfish_tty *qtty,
 		     unsigned long address,
 		     unsigned int count,
@@ -225,7 +226,7 @@ static struct tty_driver *goldfish_tty_console_device(struct console *c,
 
 static int goldfish_tty_console_setup(struct console *co, char *options)
 {
-	if ((unsigned int)co->index >= goldfish_tty_line_count)
+	if ((unsigned int)co->index >= GOLDFISH_TTY_LINE_COUNT)
 		return -ENODEV;
 	if (!goldfish_ttys[co->index].base)
 		return -ENODEV;
@@ -251,14 +252,14 @@ static int goldfish_tty_create_driver(void)
 	int ret;
 	struct tty_driver *tty;
 
-	goldfish_ttys = kcalloc(goldfish_tty_line_count,
+	goldfish_ttys = kcalloc(GOLDFISH_TTY_LINE_COUNT,
 				sizeof(*goldfish_ttys),
 				GFP_KERNEL);
 	if (goldfish_ttys == NULL) {
 		ret = -ENOMEM;
 		goto err_alloc_goldfish_ttys_failed;
 	}
-	tty = alloc_tty_driver(goldfish_tty_line_count);
+	tty = alloc_tty_driver(GOLDFISH_TTY_LINE_COUNT);
 	if (tty == NULL) {
 		ret = -ENOMEM;
 		goto err_alloc_tty_driver_failed;
@@ -333,7 +334,7 @@ static int goldfish_tty_probe(struct platform_device *pdev)
 	else
 		line = pdev->id;
 
-	if (line >= goldfish_tty_line_count) {
+	if (line >= GOLDFISH_TTY_LINE_COUNT) {
 		pr_err("goldfish_tty: Reached maximum tty number of %d.\n",
 		       goldfish_tty_current_line_count);
 		ret = -ENOMEM;
-- 
2.18.0.233.g985f88cf7e-goog

Re: [PATCH v2 2/3] tty: Make constants to be enums instead of #define in goldfish.c

[Greg KH]

On Tue, Jul 24, 2018 at 05:51:32PM -0700, rkir@google.com wrote:
> From: Roman Kiryanov <rkir@google.com>
> 
> enums produce better compilation errors than defines.

Yes, they can, if you use them correctly.  Which you are not doing here,
so this patch really does nothing.  Why should we take it?

confused,

greg k-h

Re: [PATCH v2 1/3] tty: Address checkpatch warnings in goldfish.c

[Greg KH]

On Tue, Jul 24, 2018 at 05:51:31PM -0700, rkir@google.com wrote:
> From: Roman Kiryanov <rkir@google.com>
> 
> checkpatch.pl complained about missing blank lines
> (between variable definitions and executable code)
> and using just "unsigned" instead of "unsigned int".

That is two different things, please make two different patches.

thanks,

greg k-h

Re: [PATCH v2 3/3] tty: Replace goldfish_tty_line_count with a #define

[Greg KH]

On Tue, Jul 24, 2018 at 05:51:33PM -0700, rkir@google.com wrote:
> From: Roman Kiryanov <rkir@google.com>
> 
> The driver never mutates this variable - no benefits of
> keeping it mutable.
> 
> Signed-off-by: Roman Kiryanov <rkir@google.com>
> ---
> Changes in v2:
>  - Replaced "const u32" with "#define".
> 
>  drivers/tty/goldfish.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Does not apply to my tree :(

Re: [PATCH v2 3/3] tty: Replace goldfish_tty_line_count with a #define

[Alan Cox]

On Tue, 24 Jul 2018 17:51:33 -0700
rkir@google.com wrote:

> From: Roman Kiryanov <rkir@google.com>
> 
> The driver never mutates this variable - no benefits of
> keeping it mutable.
> 
> Signed-off-by: Roman Kiryanov <rkir@google.com>
> ---
> Changes in v2:
>  - Replaced "const u32" with "#define".

v1 was IMHO definitely better. And you are correct that gcc will not
allocate memory for a static const object unless you do things like take
its address. It will (unlike a define) however properly typecheck
everything.

Alan