[PATCH] use callbacks to access UART_DLL/UART_DLM

[PATCH] use callbacks to access UART_DLL/UART_DLM

[Sebastian Frias]

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

diff --git a/drivers/tty/serial/8250/8250_core.c 
b/drivers/tty/serial/8250/8250_core.c
index 2c46a21..9ca863c 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -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;
-- 
1.7.10.4

Re: [PATCH] use callbacks to access UART_DLL/UART_DLM

[Måns Rullgård]

Sebastian Frias <sf84@laposte.net> writes:

> ---
> resending as plain-text
> ---
>   drivers/tty/serial/8250/8250_core.c |   17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_core.c 
> b/drivers/tty/serial/8250/8250_core.c
> index 2c46a21..9ca863c 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -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.

>   {
> -    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.

>       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;
> -- 

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.

-- 
Måns Rullgård

Re: [PATCH] use callbacks to access UART_DLL/UART_DLM

[Sebastian Frias]

On 12/17/2015 05:02 PM, Måns Rullgård wrote:
> Sebastian Frias <sf84@laposte.net> writes:
>
>> ---
>> resending as plain-text
>> ---
>>    drivers/tty/serial/8250/8250_core.c |   17 +++++++----------
>>    1 file changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c
>> b/drivers/tty/serial/8250/8250_core.c
>> index 2c46a21..9ca863c 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -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.

>
>>    {
>> -    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.

>
>>        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;
>> --
>
> 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.

Re: [PATCH] use callbacks to access UART_DLL/UART_DLM

[Måns Rullgård]

Sebastian Frias <sf84@laposte.net> writes:

>>>        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;
>>> --
>>
>> 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.

-- 
Måns Rullgård

Re: [PATCH] use callbacks to access UART_DLL/UART_DLM

[Mason]

On 17/12/2015 19:09, Måns Rullgård wrote:

> 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.

[PATCH v2] use callbacks to access UART_DLL/UART_DLM

[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 <sf84@laposte.net>
---
  drivers/tty/serial/8250/8250_port.c |   17 ++++++-----------
  1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c 
b/drivers/tty/serial/8250/8250_port.c
index 52d82d2..827eb6f 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -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;
-- 
1.7.10.4

Re: [PATCH v2] use callbacks to access UART_DLL/UART_DLM

[Peter Hurley]

Hi Sebastian,

On 12/18/2015 02:46 AM, Sebastian Frias wrote:
> ---
> 
> 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

> Fixes: b32b19b8ffc0

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

> 
> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> ---
>  drivers/tty/serial/8250/8250_port.c |   17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 52d82d2..827eb6f 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -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;

> 
>      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;

Re: [PATCH v2] use callbacks to access UART_DLL/UART_DLM

[Sebastian Frias]

On 12/18/2015 05:06 PM, Peter Hurley wrote:
>> -    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;

?

Re: [PATCH v2] use callbacks to access UART_DLL/UART_DLM

[Peter Hurley]

On 12/18/2015 08:27 AM, Sebastian Frias wrote:
> On 12/18/2015 05:06 PM, Peter Hurley wrote:
>>> -    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;
> 
> ?

The divisor is a 16-bit value.

Re: [PATCH v2] use callbacks to access UART_DLL/UART_DLM

[Sebastian Frias]

On 12/18/2015 05:34 PM, Peter Hurley wrote:
> On 12/18/2015 08:27 AM, Sebastian Frias wrote:
>> On 12/18/2015 05:06 PM, Peter Hurley wrote:
>>>> -    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;
>>
>> ?
>
> The divisor is a 16-bit value.
>

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

[PATCH v3] use callbacks to access UART_DLL/UART_DLM

[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 ...")

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
  drivers/tty/serial/8250/8250_port.c |   18 ++++++------------
  1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c 
b/drivers/tty/serial/8250/8250_port.c
index 52d82d2..56ccbce 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -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;
-- 
1.7.10.4

Re: [PATCH v3] use callbacks to access UART_DLL/UART_DLM

[Peter Hurley]

On 12/18/2015 08:40 AM, Sebastian Frias wrote:
> 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 <peter@hurleysoftware.com>