use callbacks to access UART_DLL/UART_DLM

PATCH Superseded Sebastian Frias 4 participants 12 messages Dec 17, 2015 – Dec 18, 2015 TTY layer and serial drivers

lore

Share & tools

lore

Sebastian Frias

author

2015-12-17 15:45 UTC #0 lore raw

PATCH 1/1 use callbacks to access UART_DLL/UART_DLM

---
resending as plain-text

---
 drivers/tty/serial/8250/8250_core.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

drivers/tty/serial/8250/8250_core.c +7 ~~-10~~

index 2c46a21..9ca863c 100644
@@ -791,22 +791,19 @@ static int size_fifo(struct uart_8250_port *up)
  */
 static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
 {
-    unsigned char old_dll, old_dlm, old_lcr;
+    unsigned char old_lcr;
     unsigned int id;
+    unsigned int old_dl;
 
     old_lcr = serial_in(p, UART_LCR);
-    serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
-
-    old_dll = serial_in(p, UART_DLL);
-    old_dlm = serial_in(p, UART_DLM);
 
-    serial_out(p, UART_DLL, 0);
-    serial_out(p, UART_DLM, 0);
+    serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
 
-    id = serial_in(p, UART_DLL) | serial_in(p, UART_DLM) << 8;
+    old_dl = serial_dl_read(p);
+    serial_dl_write(p, 0);
+    id = serial_dl_read(p);
+    serial_dl_write(p, old_dl);
 
-    serial_out(p, UART_DLL, old_dll);
-    serial_out(p, UART_DLM, old_dlm);
     serial_out(p, UART_LCR, old_lcr);
 
     return id;

Signature · 1 line

1.7.10.4

Måns Rullgård

2015-12-17 16:02 UTC #1 lore raw

Quoted 12 lines · Sebastian Frias

---
resending as plain-text
---
  drivers/tty/serial/8250/8250_core.c |   17 +++++++----------
  1 file changed, 7 insertions(+), 10 deletions(-)
drivers/tty/serial/8250/8250_core.c +0 -0
index 2c46a21..9ca863c 100644

drivers/tty/serial/8250/8250_core.c static int size_fifo(struct uart_8250_port *up) · line 792

Quoted 2 lines

drivers/tty/serial/8250/8250_core.c +0 -0

@@ -791,22 +791,19 @@ static int size_fifo(struct uart_8250_port *up)
   */
  static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)

What tree are you working against? Current mainline has that function in 8250_port.c.

drivers/tty/serial/8250/8250_core.c static int size_fifo(struct uart_8250_port *up) · line 796

Quoted 5 lines

drivers/tty/serial/8250/8250_core.c +2 -1

  {
-    unsigned char old_dll, old_dlm, old_lcr;
+    unsigned char old_lcr;
      unsigned int id;
+    unsigned int old_dl;

Your patch is messed up. Consider using "git send-email" instead.

Quoted 12 lines

drivers/tty/serial/8250/8250_core.c +1 -7

      old_lcr = serial_in(p, UART_LCR);
-    serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
-
-    old_dll = serial_in(p, UART_DLL);
-    old_dlm = serial_in(p, UART_DLM);
 
-    serial_out(p, UART_DLL, 0);
-    serial_out(p, UART_DLM, 0);
+    serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
 
-    id = serial_in(p, UART_DLL) | serial_in(p, UART_DLM) << 8;

drivers/tty/serial/8250/8250_core.c static int size_fifo(struct uart_8250_port *up) · line 809

Quoted 10 lines

drivers/tty/serial/8250/8250_core.c +4 -2

+    old_dl = serial_dl_read(p);
+    serial_dl_write(p, 0);
+    id = serial_dl_read(p);
+    serial_dl_write(p, old_dl);
 
-    serial_out(p, UART_DLL, old_dll);
-    serial_out(p, UART_DLM, old_dlm);
      serial_out(p, UART_LCR, old_lcr);
 
      return id;

If you left the blank lines alone, the patch would end up much easier to understand. In this diff, some of the lines listed as added or removed have actually not changed, and that's not immediately obvious.

Signature · 1 line

Måns Rullgård

Sebastian Frias

author

2015-12-17 18:07 UTC #2 lore raw

Quoted 8 lines · Måns Rullgård

Quoted 6 lines · Sebastian Frias
---
resending as plain-text
---
   drivers/tty/serial/8250/8250_core.c |   17 +++++++----------
   1 file changed, 7 insertions(+), 10 deletions(-)

Quoted 11 lines

drivers/tty/serial/8250/8250_core.c static int size_fifo(struct uart_8250_port *up) · line 792
Quoted 8 lines
drivers/tty/serial/8250/8250_core.c +0 -0
index 2c46a21..9ca863c 100644
@@ -791,22 +791,19 @@ static int size_fifo(struct uart_8250_port *up)
    */
   static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
What tree are you working against? Current mainline has that function in 8250_port.c.

Sorry, wrong patch. On mainline is about the same, I'll post it.

Quoted 8 lines

drivers/tty/serial/8250/8250_core.c static int size_fifo(struct uart_8250_port *up) · line 796
Quoted 6 lines
drivers/tty/serial/8250/8250_core.c +2 -1
   {
-    unsigned char old_dll, old_dlm, old_lcr;
+    unsigned char old_lcr;
       unsigned int id;
+    unsigned int old_dl;
Your patch is messed up. Consider using "git send-email" instead.

I don't know what happened there, thanks for the suggestion.

Quoted 21 lines

drivers/tty/serial/8250/8250_core.c +5 -9

       old_lcr = serial_in(p, UART_LCR);
-    serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
-
-    old_dll = serial_in(p, UART_DLL);
-    old_dlm = serial_in(p, UART_DLM);
 
-    serial_out(p, UART_DLL, 0);
-    serial_out(p, UART_DLM, 0);
+    serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
 
-    id = serial_in(p, UART_DLL) | serial_in(p, UART_DLM) << 8;
+    old_dl = serial_dl_read(p);
+    serial_dl_write(p, 0);
+    id = serial_dl_read(p);
+    serial_dl_write(p, old_dl);
 
-    serial_out(p, UART_DLL, old_dll);
-    serial_out(p, UART_DLM, old_dlm);
       serial_out(p, UART_LCR, old_lcr);

Quoted 7 lines

drivers/tty/serial/8250/8250_core.c static int size_fifo(struct uart_8250_port *up) · line 812
Quoted 2 lines
drivers/tty/serial/8250/8250_core.c +0 -1
       return id;
--
If you left the blank lines alone, the patch would end up much easier to understand. In this diff, some of the lines listed as added or removed have actually not changed, and that's not immediately obvious.

The old code had some blank lines, which I suppose were there for clarity. I attempted to follow the same idea.
But I don't mind changing the code as you wish.

Måns Rullgård

2015-12-17 18:09 UTC #3 lore raw

Quoted 23 lines · Sebastian Frias

Quoted 23 lines

Quoted 22 lines

       old_lcr = serial_in(p, UART_LCR);

drivers/tty/serial/8250/8250_core.c +5 ~~-10~~

-    serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
-
-    old_dll = serial_in(p, UART_DLL);
-    old_dlm = serial_in(p, UART_DLM);
 
-    serial_out(p, UART_DLL, 0);
-    serial_out(p, UART_DLM, 0);
+    serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
 
-    id = serial_in(p, UART_DLL) | serial_in(p, UART_DLM) << 8;
+    old_dl = serial_dl_read(p);
+    serial_dl_write(p, 0);
+    id = serial_dl_read(p);
+    serial_dl_write(p, old_dl);
 
-    serial_out(p, UART_DLL, old_dll);
-    serial_out(p, UART_DLM, old_dlm);
       serial_out(p, UART_LCR, old_lcr);
 
       return id;
--

Quoted 8 lines

Quoted 4 lines
If you left the blank lines alone, the patch would end up much easier to understand. In this diff, some of the lines listed as added or removed have actually not changed, and that's not immediately obvious.
The old code had some blank lines, which I suppose were there for clarity. I attempted to follow the same idea.
But I don't mind changing the code as you wish.

It could well be that your patch results in a clearer final version, but the diff is harder to parse when unchanged lines have moved around.

Signature · 1 line

Måns Rullgård

Mason

2015-12-17 19:14 UTC #4 lore raw

Quoted 2 lines · Måns Rullgård

It could well be that your patch results in a clearer final version, but the diff is harder to parse when unchanged lines have moved around.

When all other things are equal, is the priority

1) making the patch as clear as possible?
2) making the resulting code as clear as possible?

On a related note, I remember spotting a problem in a phy driver commit, and was told (by you) that I should split my fix in 3 (!!) patches. I'm sorry, but that's a double standard, why would the initial committer be given the leeway to post a single patch, but someone willing to clean up his mess should work harder?

End result: I left the bug in there.

Regards.

Sebastian Frias

author

2015-12-18 10:46 UTC #5 lore raw

PATCH v2 1/1 use callbacks to access UART_DLL/UART_DLM

In reply to Sebastian Frias

---

Some UART HW has a single register combining UART_DLL/UART_DLM (this was probably forgotten in the change that introduced the callbacks, commit b32b19b8ffc05cbd3bf91c65e205f6a912ca15d9)

Fixes b32b19b8ffc0

Signed-off-by Sebastian Frias <[email protected]>

---
 drivers/tty/serial/8250/8250_port.c |   17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

drivers/tty/serial/8250/8250_port.c +6 ~~-11~~

index 52d82d2..827eb6f 100644
@@ -713,22 +713,17 @@ static int size_fifo(struct uart_8250_port *up)
  */
 static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
 {
-	unsigned char old_dll, old_dlm, old_lcr;
+	unsigned char old_lcr;
 	unsigned int id;
+	unsigned int old_dl;
 
 	old_lcr = serial_in(p, UART_LCR);
 	serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
+	old_dl = serial_dl_read(p);
+	serial_dl_write(p, 0);
+	id = serial_dl_read(p);
+	serial_dl_write(p, old_dl);
 
-	old_dll = serial_in(p, UART_DLL);
-	old_dlm = serial_in(p, UART_DLM);
-
-	serial_out(p, UART_DLL, 0);
-	serial_out(p, UART_DLM, 0);
-
-	id = serial_in(p, UART_DLL) | serial_in(p, UART_DLM) << 8;
-
-	serial_out(p, UART_DLL, old_dll);
-	serial_out(p, UART_DLM, old_dlm);
 	serial_out(p, UART_LCR, old_lcr);
 
 	return id;

Signature · 1 line

1.7.10.4

Peter Hurley

2015-12-18 16:06 UTC #6 lore raw

Hi Sebastian,

Quoted 5 lines · Sebastian Frias

---
Some UART HW has a single register combining UART_DLL/UART_DLM (this was probably forgotten in the change that introduced the callbacks, commit b32b19b8ffc05cbd3bf91c65e205f6a912ca15d9)

Thanks for the fix.

Because of the heavy patchload here, it helps if the patches are don't need hand-editing by maintainers.

Please drop the '---' separator and newline from the beginning of the commit log.

Regards,
Peter Hurley

Quoted 1 line

Fixes b32b19b8ffc0

Fixes b32b19b8ffc0 ("[SERIAL] 8250: set divisor register correctly ...")

Quoted 11 lines

Signed-off-by Sebastian Frias <[email protected]>
---
 drivers/tty/serial/8250/8250_port.c |   17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)
drivers/tty/serial/8250/8250_port.c +0 -0
index 52d82d2..827eb6f 100644

drivers/tty/serial/8250/8250_port.c static int size_fifo(struct uart_8250_port *up) · line 718

Quoted 7 lines

drivers/tty/serial/8250/8250_port.c +2 -1

@@ -713,22 +713,17 @@ static int size_fifo(struct uart_8250_port *up)
  */
 static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
 {
-    unsigned char old_dll, old_dlm, old_lcr;
+    unsigned char old_lcr;
     unsigned int id;
+    unsigned int old_dl;

	unsigned int id, old_dl;

Quoted 21 lines

drivers/tty/serial/8250/8250_port.c +4 ~~-10~~

     old_lcr = serial_in(p, UART_LCR);
     serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
+    old_dl = serial_dl_read(p);
+    serial_dl_write(p, 0);
+    id = serial_dl_read(p);
+    serial_dl_write(p, old_dl);
 
-    old_dll = serial_in(p, UART_DLL);
-    old_dlm = serial_in(p, UART_DLM);
-
-    serial_out(p, UART_DLL, 0);
-    serial_out(p, UART_DLM, 0);
-
-    id = serial_in(p, UART_DLL) | serial_in(p, UART_DLM) << 8;
-
-    serial_out(p, UART_DLL, old_dll);
-    serial_out(p, UART_DLM, old_dlm);
     serial_out(p, UART_LCR, old_lcr);
 
     return id;

Sebastian Frias

author

2015-12-18 16:27 UTC #7 lore raw

Quoted 6 lines · Peter Hurley

drivers/tty/serial/8250/8250_port.c static int size_fifo(struct uart_8250_port *up) · line 718
Quoted 4 lines
drivers/tty/serial/8250/8250_port.c +2 -1
-    unsigned char old_dll, old_dlm, old_lcr;
+    unsigned char old_lcr;
      unsigned int id;
+    unsigned int old_dl;
	unsigned int id, old_dl;

Ok, thanks for your comments.
By the way, should I just do:

- unsigned char old_dll, old_dlm, old_lcr;
+ unsigned char old_lcr, old_dl;

Peter Hurley

2015-12-18 16:34 UTC #8 lore raw

Quoted 14 lines · Sebastian Frias

Quoted 6 lines · Peter Hurley
drivers/tty/serial/8250/8250_port.c static int size_fifo(struct uart_8250_port *up) · line 718
Quoted 4 lines
drivers/tty/serial/8250/8250_port.c +2 -1
-    unsigned char old_dll, old_dlm, old_lcr;
+    unsigned char old_lcr;
      unsigned int id;
+    unsigned int old_dl;
    unsigned int id, old_dl;
Ok, thanks for your comments.
By the way, should I just do:
(quoted diff) +1 -1
-    unsigned char old_dll, old_dlm, old_lcr;
+    unsigned char old_lcr, old_dl;

Quoted 1 line

?

The divisor is a 16-bit value.

Sebastian Frias

author

2015-12-18 16:35 UTC #9 lore raw

Quoted 12 lines · Peter Hurley

Quoted 11 lines · Sebastian Frias
Quoted 6 lines · Peter Hurley
drivers/tty/serial/8250/8250_port.c static int size_fifo(struct uart_8250_port *up) · line 718
Quoted 4 lines
drivers/tty/serial/8250/8250_port.c +2 -1
-    unsigned char old_dll, old_dlm, old_lcr;
+    unsigned char old_lcr;
       unsigned int id;
+    unsigned int old_dl;
     unsigned int id, old_dl;
Ok, thanks for your comments.
By the way, should I just do:

Quoted 7 lines

Quoted 4 lines
(quoted diff) +1 -1
-    unsigned char old_dll, old_dlm, old_lcr;
+    unsigned char old_lcr, old_dl;
?
The divisor is a 16-bit value.

You are right, I was too eager to "reduce the amount of lines that change"...

Sebastian Frias

author

2015-12-18 16:40 UTC #10 lore raw

PATCH v3 1/1 use callbacks to access UART_DLL/UART_DLM

In reply to Peter Hurley

Some UART HW has a single register combining UART_DLL/UART_DLM (this was probably forgotten in the change that introduced the callbacks, commit b32b19b8ffc05cbd3bf91c65e205f6a912ca15d9)

Fixes b32b19b8ffc0 ("[SERIAL] 8250: set divisor register correctly ...")

Signed-off-by Sebastian Frias <[email protected]>

---
 drivers/tty/serial/8250/8250_port.c |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

drivers/tty/serial/8250/8250_port.c +6 ~~-12~~

index 52d82d2..56ccbce 100644
@@ -713,22 +713,16 @@ static int size_fifo(struct uart_8250_port *up)
  */
 static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
 {
-	unsigned char old_dll, old_dlm, old_lcr;
-	unsigned int id;
+	unsigned char old_lcr;
+	unsigned int id, old_dl;
 
 	old_lcr = serial_in(p, UART_LCR);
 	serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
+	old_dl = serial_dl_read(p);
+	serial_dl_write(p, 0);
+	id = serial_dl_read(p);
+	serial_dl_write(p, old_dl);
 
-	old_dll = serial_in(p, UART_DLL);
-	old_dlm = serial_in(p, UART_DLM);
-
-	serial_out(p, UART_DLL, 0);
-	serial_out(p, UART_DLM, 0);
-
-	id = serial_in(p, UART_DLL) | serial_in(p, UART_DLM) << 8;
-
-	serial_out(p, UART_DLL, old_dll);
-	serial_out(p, UART_DLM, old_dlm);
 	serial_out(p, UART_LCR, old_lcr);
 
 	return id;

Signature · 1 line

1.7.10.4

Peter Hurley

Reviewed-by on v3 1/1

2015-12-18 16:42 UTC #11 lore raw

Quoted 5 lines · Sebastian Frias

Some UART HW has a single register combining UART_DLL/UART_DLM (this was probably forgotten in the change that introduced the callbacks, commit b32b19b8ffc05cbd3bf91c65e205f6a912ca15d9)
Fixes b32b19b8ffc0 ("[SERIAL] 8250: set divisor register correctly ...")

Reviewed-by Peter Hurley <[email protected]>