Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1294006 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] use callbacks to access UART_DLL/UART_DLM Måns Rullgård <mans@mansr.com> - 2015-12-17 17:10 +0100
    Re: [PATCH] use callbacks to access UART_DLL/UART_DLM Måns Rullgård <mans@mansr.com> - 2015-12-17 19:10 +0100
      Re: [PATCH] use callbacks to access UART_DLL/UART_DLM Mason <slash.tmp@free.fr> - 2015-12-17 20:20 +0100
    Re: [PATCH] use callbacks to access UART_DLL/UART_DLM Sebastian Frias <sf84@laposte.net> - 2015-12-17 19:10 +0100
      [PATCH v2] use callbacks to access UART_DLL/UART_DLM Sebastian Frias <sf84@laposte.net> - 2015-12-18 11:50 +0100
        Re: [PATCH v2] use callbacks to access UART_DLL/UART_DLM Peter Hurley <peter@hurleysoftware.com> - 2015-12-18 17:10 +0100
          Re: [PATCH v2] use callbacks to access UART_DLL/UART_DLM Sebastian Frias <sf84@laposte.net> - 2015-12-18 17:30 +0100
            Re: [PATCH v2] use callbacks to access UART_DLL/UART_DLM Sebastian Frias <sf84@laposte.net> - 2015-12-18 17:40 +0100
            Re: [PATCH v2] use callbacks to access UART_DLL/UART_DLM Peter Hurley <peter@hurleysoftware.com> - 2015-12-18 17:40 +0100
          Re: [PATCH v3] use callbacks to access UART_DLL/UART_DLM Peter Hurley <peter@hurleysoftware.com> - 2015-12-18 17:50 +0100
          [PATCH v3] use callbacks to access UART_DLL/UART_DLM Sebastian Frias <sf84@laposte.net> - 2015-12-18 17:50 +0100

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1294108

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1294152

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1294109

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1294654 — [PATCH v2] use callbacks to access UART_DLL/UART_DLM

---

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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;

?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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"...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1295026 — [PATCH v3] use callbacks to access UART_DLL/UART_DLM

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web