[PATCH 0/3] auxdisplay: charlcd: miscellaneous patches

[PATCH 0/3] auxdisplay: charlcd: miscellaneous patches

[Robert Abel]

While looking at charlcd I noticed some little bits here and there that might
be corrected or improved.

Robert Abel (3):
  auxdisplay: charlcd: fix hex literal ranges for graphics command
  auxdisplay: charlcd: use null character instead of zero literal to
    terminate strings
  auxdisplay: charlcd: replace octal literal with form-feed escape
    sequence

 drivers/auxdisplay/charlcd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.11.0

[PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

The graphics command expects 16 hexadecimal literals, but would allow characters in range [0-9a-zA-Z] instead of [0-9a-fA-F].

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 drivers/auxdisplay/charlcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 642afd88870b..324d02f9f1c5 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -441,9 +441,9 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
 			shift ^= 4;
 			if (*esc >= '0' && *esc <= '9') {
 				value |= (*esc - '0') << shift;
-			} else if (*esc >= 'A' && *esc <= 'Z') {
+			} else if (*esc >= 'A' && *esc <= 'F') {
 				value |= (*esc - 'A' + 10) << shift;
-			} else if (*esc >= 'a' && *esc <= 'z') {
+			} else if (*esc >= 'a' && *esc <= 'f') {
 				value |= (*esc - 'a' + 10) << shift;
 			} else {
 				esc++;
-- 
2.11.0

[PATCH 2/3] auxdisplay: charlcd: use null character instead of zero literal to terminate strings

[Robert Abel]

Using '\0' instead of plain 0 makes the intent clearer that this is indeed a string and not a series of integers.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 drivers/auxdisplay/charlcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 324d02f9f1c5..92549c8344a4 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -527,7 +527,7 @@ static void charlcd_write_char(struct charlcd *lcd, char c)
 	if ((c != '\n') && priv->esc_seq.len >= 0) {
 		/* yes, let's add this char to the buffer */
 		priv->esc_seq.buf[priv->esc_seq.len++] = c;
-		priv->esc_seq.buf[priv->esc_seq.len] = 0;
+		priv->esc_seq.buf[priv->esc_seq.len] = '\0';
 	} else {
 		/* aborts any previous escape sequence */
 		priv->esc_seq.len = -1;
@@ -536,7 +536,7 @@ static void charlcd_write_char(struct charlcd *lcd, char c)
 		case LCD_ESCAPE_CHAR:
 			/* start of an escape sequence */
 			priv->esc_seq.len = 0;
-			priv->esc_seq.buf[priv->esc_seq.len] = 0;
+			priv->esc_seq.buf[priv->esc_seq.len] = '\0';
 			break;
 		case '\b':
 			/* go back one char and clear it */
-- 
2.11.0

[PATCH 3/3] auxdisplay: charlcd: replace octal literal with form-feed escape sequence

[Robert Abel]

There is no need to resort to octal escape sequence for the form feed character when an established escape sequence exists.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 drivers/auxdisplay/charlcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 92549c8344a4..a3486db03d81 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -555,7 +555,7 @@ static void charlcd_write_char(struct charlcd *lcd, char c)
 			/* back one char again */
 			lcd->ops->write_cmd(lcd, LCD_CMD_SHIFT);
 			break;
-		case '\014':
+		case '\f':
 			/* quickly clear the display */
 			charlcd_clear_fast(lcd);
 			break;
-- 
2.11.0

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Miguel Ojeda]

On Sat, Feb 10, 2018 at 12:50 AM, Robert Abel <rabel@robertabel.eu> wrote:
> The graphics command expects 16 hexadecimal literals, but would allow characters in range [0-9a-zA-Z] instead of [0-9a-fA-F].
>
> Signed-off-by: Robert Abel <rabel@robertabel.eu>
> ---
>  drivers/auxdisplay/charlcd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
> index 642afd88870b..324d02f9f1c5 100644
> --- a/drivers/auxdisplay/charlcd.c
> +++ b/drivers/auxdisplay/charlcd.c
> @@ -441,9 +441,9 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
>                         shift ^= 4;
>                         if (*esc >= '0' && *esc <= '9') {
>                                 value |= (*esc - '0') << shift;
> -                       } else if (*esc >= 'A' && *esc <= 'Z') {
> +                       } else if (*esc >= 'A' && *esc <= 'F') {
>                                 value |= (*esc - 'A' + 10) << shift;
> -                       } else if (*esc >= 'a' && *esc <= 'z') {
> +                       } else if (*esc >= 'a' && *esc <= 'f') {

Willy, Geert: this seems obvious, but do you know if the broader range
was intended for some reason? In that case, adding a comment to the
code would be good. I found some related docs at
Documentation/misc-devices/lcd-panel-cgram.txt by Willy (which, by the
way, maybe now we should move them to Documentations/auxdisplay); but
the paragraph does indeed say they have to be hex:

'''
Some LCDs allow you to define up to 8 characters, mapped to ASCII
characters 0 to 7. The escape code to define a new character is
'\e[LG' followed by one digit from 0 to 7, representing the character
number, and up to 8 couples of hex digits terminated by a semi-colon
(';').
'''

>                                 value |= (*esc - 'a' + 10) << shift;
>                         } else {
>                                 esc++;
> --
> 2.11.0
>

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Willy Tarreau]

Hi Miguel,

On Sat, Feb 10, 2018 at 09:58:44AM +0100, Miguel Ojeda wrote:
> On Sat, Feb 10, 2018 at 12:50 AM, Robert Abel <rabel@robertabel.eu> wrote:
> > The graphics command expects 16 hexadecimal literals, but would allow characters in range [0-9a-zA-Z] instead of [0-9a-fA-F].
> >
> > Signed-off-by: Robert Abel <rabel@robertabel.eu>
> > ---
> >  drivers/auxdisplay/charlcd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
> > index 642afd88870b..324d02f9f1c5 100644
> > --- a/drivers/auxdisplay/charlcd.c
> > +++ b/drivers/auxdisplay/charlcd.c
> > @@ -441,9 +441,9 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
> >                         shift ^= 4;
> >                         if (*esc >= '0' && *esc <= '9') {
> >                                 value |= (*esc - '0') << shift;
> > -                       } else if (*esc >= 'A' && *esc <= 'Z') {
> > +                       } else if (*esc >= 'A' && *esc <= 'F') {
> >                                 value |= (*esc - 'A' + 10) << shift;
> > -                       } else if (*esc >= 'a' && *esc <= 'z') {
> > +                       } else if (*esc >= 'a' && *esc <= 'f') {
> 
> Willy, Geert: this seems obvious, but do you know if the broader range
> was intended for some reason?

No, I think it was simply a brain fart from me 14 years ago, as I can
find it as well in the original 0.9.0 patch for kernel 2.4!

Ack from me on this patch.

Willy

Re: [PATCH 2/3] auxdisplay: charlcd: use null character instead of zero literal to terminate strings

[Miguel Ojeda]

On Sat, Feb 10, 2018 at 12:50 AM, Robert Abel <rabel@robertabel.eu> wrote:
> Using '\0' instead of plain 0 makes the intent clearer that this is indeed a string and not a series of integers.
>
> Signed-off-by: Robert Abel <rabel@robertabel.eu>
> ---
>  drivers/auxdisplay/charlcd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
> index 324d02f9f1c5..92549c8344a4 100644
> --- a/drivers/auxdisplay/charlcd.c
> +++ b/drivers/auxdisplay/charlcd.c
> @@ -527,7 +527,7 @@ static void charlcd_write_char(struct charlcd *lcd, char c)
>         if ((c != '\n') && priv->esc_seq.len >= 0) {
>                 /* yes, let's add this char to the buffer */
>                 priv->esc_seq.buf[priv->esc_seq.len++] = c;
> -               priv->esc_seq.buf[priv->esc_seq.len] = 0;
> +               priv->esc_seq.buf[priv->esc_seq.len] = '\0';
>         } else {
>                 /* aborts any previous escape sequence */
>                 priv->esc_seq.len = -1;
> @@ -536,7 +536,7 @@ static void charlcd_write_char(struct charlcd *lcd, char c)
>                 case LCD_ESCAPE_CHAR:
>                         /* start of an escape sequence */
>                         priv->esc_seq.len = 0;
> -                       priv->esc_seq.buf[priv->esc_seq.len] = 0;
> +                       priv->esc_seq.buf[priv->esc_seq.len] = '\0';

Trivial. Compile-tested.

Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>

>                         break;
>                 case '\b':
>                         /* go back one char and clear it */
> --
> 2.11.0
>

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Miguel Ojeda]

On Sat, Feb 10, 2018 at 10:20 AM, Willy Tarreau <w@1wt.eu> wrote:
> Hi Miguel,
>
> On Sat, Feb 10, 2018 at 09:58:44AM +0100, Miguel Ojeda wrote:
>> On Sat, Feb 10, 2018 at 12:50 AM, Robert Abel <rabel@robertabel.eu> wrote:
>> > The graphics command expects 16 hexadecimal literals, but would allow characters in range [0-9a-zA-Z] instead of [0-9a-fA-F].
>> >
>> > Signed-off-by: Robert Abel <rabel@robertabel.eu>
>> > ---
>> >  drivers/auxdisplay/charlcd.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
>> > index 642afd88870b..324d02f9f1c5 100644
>> > --- a/drivers/auxdisplay/charlcd.c
>> > +++ b/drivers/auxdisplay/charlcd.c
>> > @@ -441,9 +441,9 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
>> >                         shift ^= 4;
>> >                         if (*esc >= '0' && *esc <= '9') {
>> >                                 value |= (*esc - '0') << shift;
>> > -                       } else if (*esc >= 'A' && *esc <= 'Z') {
>> > +                       } else if (*esc >= 'A' && *esc <= 'F') {
>> >                                 value |= (*esc - 'A' + 10) << shift;
>> > -                       } else if (*esc >= 'a' && *esc <= 'z') {
>> > +                       } else if (*esc >= 'a' && *esc <= 'f') {
>>
>> Willy, Geert: this seems obvious, but do you know if the broader range
>> was intended for some reason?
>
> No, I think it was simply a brain fart from me 14 years ago, as I can
> find it as well in the original 0.9.0 patch for kernel 2.4!
>
> Ack from me on this patch.

Thanks Willy for the quick answer! Then:

Compile-tested.

Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>

>
> Willy

Re: [PATCH 3/3] auxdisplay: charlcd: replace octal literal with form-feed escape sequence

[Miguel Ojeda]

On Sat, Feb 10, 2018 at 12:50 AM, Robert Abel <rabel@robertabel.eu> wrote:
> There is no need to resort to octal escape sequence for the form feed character when an established escape sequence exists.
>
> Signed-off-by: Robert Abel <rabel@robertabel.eu>
> ---
>  drivers/auxdisplay/charlcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
> index 92549c8344a4..a3486db03d81 100644
> --- a/drivers/auxdisplay/charlcd.c
> +++ b/drivers/auxdisplay/charlcd.c
> @@ -555,7 +555,7 @@ static void charlcd_write_char(struct charlcd *lcd, char c)
>                         /* back one char again */
>                         lcd->ops->write_cmd(lcd, LCD_CMD_SHIFT);
>                         break;
> -               case '\014':
> +               case '\f':

Normally I wouldn't bother with this kind of change, but since the
other switch cases all use escape sequences, this improves
consistency, so agreed.

Trivial. Compile-tested.

Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>

>                         /* quickly clear the display */
>                         charlcd_clear_fast(lcd);
>                         break;
> --
> 2.11.0
>

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Geert Uytterhoeven]

On Sat, Feb 10, 2018 at 9:58 AM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Sat, Feb 10, 2018 at 12:50 AM, Robert Abel <rabel@robertabel.eu> wrote:
>> The graphics command expects 16 hexadecimal literals, but would allow characters in range [0-9a-zA-Z] instead of [0-9a-fA-F].
>>
>> Signed-off-by: Robert Abel <rabel@robertabel.eu>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

>> --- a/drivers/auxdisplay/charlcd.c
>> +++ b/drivers/auxdisplay/charlcd.c
>> @@ -441,9 +441,9 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
>>                         shift ^= 4;
>>                         if (*esc >= '0' && *esc <= '9') {
>>                                 value |= (*esc - '0') << shift;
>> -                       } else if (*esc >= 'A' && *esc <= 'Z') {
>> +                       } else if (*esc >= 'A' && *esc <= 'F') {
>>                                 value |= (*esc - 'A' + 10) << shift;
>> -                       } else if (*esc >= 'a' && *esc <= 'z') {
>> +                       } else if (*esc >= 'a' && *esc <= 'f') {
>
> Willy, Geert: this seems obvious, but do you know if the broader range
> was intended for some reason? In that case, adding a comment to the
> code would be good. I found some related docs at
> Documentation/misc-devices/lcd-panel-cgram.txt by Willy (which, by the
> way, maybe now we should move them to Documentations/auxdisplay); but
> the paragraph does indeed say they have to be hex:
>
> '''
> Some LCDs allow you to define up to 8 characters, mapped to ASCII
> characters 0 to 7. The escape code to define a new character is
> '\e[LG' followed by one digit from 0 to 7, representing the character
> number, and up to 8 couples of hex digits terminated by a semi-colon
> (';').
> '''

Hadn't noticed this before. Probably a stupid thinko, as Willy said.

The redefinition feature definitely works with hex characters.
I've used it in the past, cfr. the picture on my Google+ profile ;-)
https://plus.google.com/u/0/+GeertUytterhoeven

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Willy Tarreau]

On Sat, Feb 10, 2018 at 07:31:54PM +0100, Geert Uytterhoeven wrote:
> The redefinition feature definitely works with hex characters.
> I've used it in the past, cfr. the picture on my Google+ profile ;-)
> https://plus.google.com/u/0/+GeertUytterhoeven

I've used it as well which is why I never noticed. However it's the
first time I see the driver used with a 4-lines device, so apparently
it works :-)

Willy

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Miguel Ojeda]

On Sat, Feb 10, 2018 at 10:41 AM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Sat, Feb 10, 2018 at 10:20 AM, Willy Tarreau <w@1wt.eu> wrote:
>> Hi Miguel,
>>
>> On Sat, Feb 10, 2018 at 09:58:44AM +0100, Miguel Ojeda wrote:
>>> On Sat, Feb 10, 2018 at 12:50 AM, Robert Abel <rabel@robertabel.eu> wrote:
>>> > The graphics command expects 16 hexadecimal literals, but would allow characters in range [0-9a-zA-Z] instead of [0-9a-fA-F].
>>> >
>>> > Signed-off-by: Robert Abel <rabel@robertabel.eu>
>>> > ---
>>> >  drivers/auxdisplay/charlcd.c | 4 ++--
>>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>>> >
>>> > diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
>>> > index 642afd88870b..324d02f9f1c5 100644
>>> > --- a/drivers/auxdisplay/charlcd.c
>>> > +++ b/drivers/auxdisplay/charlcd.c
>>> > @@ -441,9 +441,9 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
>>> >                         shift ^= 4;
>>> >                         if (*esc >= '0' && *esc <= '9') {
>>> >                                 value |= (*esc - '0') << shift;
>>> > -                       } else if (*esc >= 'A' && *esc <= 'Z') {
>>> > +                       } else if (*esc >= 'A' && *esc <= 'F') {
>>> >                                 value |= (*esc - 'A' + 10) << shift;
>>> > -                       } else if (*esc >= 'a' && *esc <= 'z') {
>>> > +                       } else if (*esc >= 'a' && *esc <= 'f') {
>>>
>>> Willy, Geert: this seems obvious, but do you know if the broader range
>>> was intended for some reason?
>>
>> No, I think it was simply a brain fart from me 14 years ago, as I can
>> find it as well in the original 0.9.0 patch for kernel 2.4!
>>
>> Ack from me on this patch.
>
> Thanks Willy for the quick answer! Then:
>
> Compile-tested.
>
> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>

To clarify, I have picked this one up (and the other two) in my queue
(sorry for any confusion!).

>
>>
>> Willy

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Andy Shevchenko]

On Sat, Feb 10, 2018 at 11:41 AM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Sat, Feb 10, 2018 at 10:20 AM, Willy Tarreau <w@1wt.eu> wrote:
>> On Sat, Feb 10, 2018 at 09:58:44AM +0100, Miguel Ojeda wrote:
>>> On Sat, Feb 10, 2018 at 12:50 AM, Robert Abel <rabel@robertabel.eu> wrote:

>>> >                         shift ^= 4;
>>> >                         if (*esc >= '0' && *esc <= '9') {
>>> >                                 value |= (*esc - '0') << shift;
>>> > -                       } else if (*esc >= 'A' && *esc <= 'Z') {
>>> > +                       } else if (*esc >= 'A' && *esc <= 'F') {
>>> >                                 value |= (*esc - 'A' + 10) << shift;
>>> > -                       } else if (*esc >= 'a' && *esc <= 'z') {
>>> > +                       } else if (*esc >= 'a' && *esc <= 'f') {
>>>
>>> Willy, Geert: this seems obvious, but do you know if the broader range
>>> was intended for some reason?
>>
>> No, I think it was simply a brain fart from me 14 years ago, as I can
>> find it as well in the original 0.9.0 patch for kernel 2.4!

I understand that we have a huge and hopefully nice library in the
kernel, but the question still the same, what prevents a developer or
maintainer to look at it from time to time?

For, I dare to say, ages we have hex_to_bin() and hex2bin().
Can we use it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Willy Tarreau]

On Tue, Feb 13, 2018 at 03:36:45PM +0200, Andy Shevchenko wrote:
> On Sat, Feb 10, 2018 at 11:41 AM, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> > On Sat, Feb 10, 2018 at 10:20 AM, Willy Tarreau <w@1wt.eu> wrote:
> >> On Sat, Feb 10, 2018 at 09:58:44AM +0100, Miguel Ojeda wrote:
> >>> On Sat, Feb 10, 2018 at 12:50 AM, Robert Abel <rabel@robertabel.eu> wrote:
> 
> >>> >                         shift ^= 4;
> >>> >                         if (*esc >= '0' && *esc <= '9') {
> >>> >                                 value |= (*esc - '0') << shift;
> >>> > -                       } else if (*esc >= 'A' && *esc <= 'Z') {
> >>> > +                       } else if (*esc >= 'A' && *esc <= 'F') {
> >>> >                                 value |= (*esc - 'A' + 10) << shift;
> >>> > -                       } else if (*esc >= 'a' && *esc <= 'z') {
> >>> > +                       } else if (*esc >= 'a' && *esc <= 'f') {
> >>>
> >>> Willy, Geert: this seems obvious, but do you know if the broader range
> >>> was intended for some reason?
> >>
> >> No, I think it was simply a brain fart from me 14 years ago, as I can
> >> find it as well in the original 0.9.0 patch for kernel 2.4!
> 
> I understand that we have a huge and hopefully nice library in the
> kernel, but the question still the same, what prevents a developer or
> maintainer to look at it from time to time?

It's always the same, nobody knows that some code appeared somewhere to
solve some problem they already solved a long time ago and that they are
not even aware of anymore. That's why code cleanups like this session are
useful.

> For, I dare to say, ages we have hex_to_bin() and hex2bin().
> Can we use it?

I definitely think so after taking a quick look at hex2bin().

Thanks!
Willy

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

On 13 Feb 2018 14:36, Andy Shevchenko wrote:
> I understand that we have a huge and hopefully nice library in the
> kernel, but the question still the same, what prevents a developer or
> maintainer to look at it from time to time?
> 
> For, I dare to say, ages we have hex_to_bin() and hex2bin().
> Can we use it?

hex_to_bin look fine to me, although personally I'm not a big fan of its
use of tolower.

The current parser implementation is much more lenient than hex2bin,
however. hex2bin won't parse strings containing illegal characters
(which are currently skipped) or hexadecimal strings with an odd number
of digits (which are currently allowed and the final digit will be ignored).

I noticed the only part of the code that does make use of library
functions, parsing x and y coordinates using kstrtoul, is broken.
Apparently it used to use simple_strtoul, which worked and then got
replaced. So apparently looking over the kernel lib from time to time
can also do some harm ;)
Patch incoming :)

Regards,

Robert

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Andy Shevchenko]

On Thu, Feb 15, 2018 at 1:17 AM, Robert Abel <rabel@robertabel.eu> wrote:
> On 13 Feb 2018 14:36, Andy Shevchenko wrote:
>> I understand that we have a huge and hopefully nice library in the
>> kernel, but the question still the same, what prevents a developer or
>> maintainer to look at it from time to time?
>>
>> For, I dare to say, ages we have hex_to_bin() and hex2bin().
>> Can we use it?
>
> hex_to_bin look fine to me, although personally I'm not a big fan of its
> use of tolower.

Let's duplicate then over and over?

> The current parser implementation is much more lenient than hex2bin,
> however. hex2bin won't parse strings containing illegal characters
> (which are currently skipped) or hexadecimal strings with an odd number
> of digits (which are currently allowed and the final digit will be ignored).

Can you point to the documentation where user can easily (w/o reading
the code) get how it suppose to be?

Besides that, I'm a fan of making things cleaner and stricter.
Allowing garbage in the middle of hex digits is making odd and
unflexible interface.

> I noticed the only part of the code that does make use of library
> functions, parsing x and y coordinates using kstrtoul, is broken.
> Apparently it used to use simple_strtoul, which worked and then got
> replaced.

By which commit?

> So apparently looking over the kernel lib from time to time
> can also do some harm ;)

Disagree. The careless (semi-)automated patches and / or negligent
review make this so.
See Markus Elfting phenomena. He dumbly doing routine work w/o paying
attention to the details and breaking things. That's why half of
maintainers already banned him.

> Patch incoming :)

Can you Cc me?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

Hi Andy,

On 15 Feb 2018 11:57, Andy Shevchenko wrote:
> On Thu, Feb 15, 2018 at 1:17 AM, Robert Abel <rabel@robertabel.eu> wrote:
>> hex_to_bin look fine to me, although personally I'm not a big fan of its
>> use of tolower.
> 
> Let's duplicate then over and over?

I was speaking more generally about performance here. There is a reason
for kstrtox.c:57
(https://elixir.bootlin.com/linux/v4.14.7/source/lib/kstrtox.c#L57)
> unsigned int lc = c | 0x20; /* don't tolower() this line */

> Can you point to the documentation where user can easily (w/o reading
> the code) get how it suppose to be?

Unfortunately not. I read the code myself to know how it is supposed to
work. That's definitely a gap in documentation.

> On Thu, Feb 15, 2018 at 1:17 AM, Robert Abel <rabel@robertabel.eu> wrote:
>> I noticed the only part of the code that does make use of library
>> functions, parsing x and y coordinates using kstrtoul, is broken.
>> Apparently it used to use simple_strtoul, which worked and then got
>> replaced.
> By which commit?

129957069e6af42a6e021d90679c56662c95f7e1
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=129957069e6af42a6e021d90679c56662c95f7e1)

I'll try to answer to this email with relevant patches for charlcd.c.

Regards,

Robert

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

The following patch set fixes the x/y coordinate addressing issue I mentioned earlier
as well as not marking the two-line command sequence as processed resulting in a confusing
state.

I implemented the logic around using kstrtoul by terminating the substrings that contain
numbers for each command separately and then restoring the command character following the
number that was overwritten.

Additionally, the code now doesn't write live to the x and y address fields and will
bail out if an unknown command character or an invalid number was encountered.

I did try to keep it backwards compatible, so sequences of the form ^[[Lx0y1x2y4; will
still work (and result in x2y4).

Additionally, I noticed that the ^[[H home function was just resetting the x/y coordinates
but not the display shift, which results in a situation where the display cannot be
programmatically unshifted (except for init ^[[LI). So I implemented the HOME command 0x02
for charlcd_home. The old behavior can be simulated using ^[[Lx0y0; (just reset x/y, don't
unshift).

I tested on my Raspberry Pi Zero W with a clone 1602 display.

Robert Abel (4):
  auxdisplay: charlcd: fix two-line command ^[[LN not marked as
    processed
  auxdisplay: charlcd: name x/y address struct
  auxdisplay: charlcd: fix x/y address commands
  auxdisplay: charlcd: make home command unshift display

 drivers/auxdisplay/charlcd.c | 76 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 60 insertions(+), 16 deletions(-)

-- 
2.11.0

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

The following patch set fixes the x/y coordinate addressing issue I mentioned earlier
as well as not marking the two-line command sequence as processed resulting in a confusing
state.

I implemented the logic around using kstrtoul by terminating the substrings that contain
numbers for each command separately and then restoring the command character following the
number that was overwritten.

Additionally, the code now doesn't write live to the x and y address fields and will
bail out if an unknown command character or an invalid number was encountered.

I did try to keep it backwards compatible, so sequences of the form ^[[Lx0y1x2y4; will
still work (and result in x2y4).

Additionally, I noticed that the ^[[H home function was just resetting the x/y coordinates
but not the display shift, which results in a situation where the display cannot be
programmatically unshifted (except for init ^[[LI). So I implemented the HOME command 0x02
for charlcd_home. The old behavior can be simulated using ^[[Lx0y0; (just reset x/y, don't
unshift).

I tested on my Raspberry Pi Zero W with a clone 1602 display.

Robert Abel (4):
  auxdisplay: charlcd: fix two-line command ^[[LN not marked as
    processed
  auxdisplay: charlcd: name x/y address struct
  auxdisplay: charlcd: fix x/y address commands
  auxdisplay: charlcd: make home command unshift display

 drivers/auxdisplay/charlcd.c | 76 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 60 insertions(+), 16 deletions(-)

-- 
2.11.0

[PATCH 1/4] auxdisplay: charlcd: fix two-line command ^[[LN not marked as processed

[Robert Abel]

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 drivers/auxdisplay/charlcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index a3486db03d81..e3b2fd15c5a3 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -362,6 +362,7 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
 		break;
 	case 'N':	/* Two Lines */
 		priv->flags |= LCD_FLAG_N;
+		processed = 1;
 		break;
 	case 'l':	/* Shift Cursor Left */
 		if (priv->addr.x > 0) {
-- 
2.11.0

[PATCH 2/4] auxdisplay: charlcd: name x/y address struct

[Robert Abel]

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 drivers/auxdisplay/charlcd.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index e3b2fd15c5a3..a3d364e6c666 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -67,6 +67,11 @@
 #define LCD_ESCAPE_LEN		24	/* Max chars for LCD escape command */
 #define LCD_ESCAPE_CHAR		27	/* Use char 27 for escape command */
 
+struct charlcd_priv_addr {
+	unsigned long int x;
+	unsigned long int y;
+};
+
 struct charlcd_priv {
 	struct charlcd lcd;
 
@@ -80,12 +85,9 @@ struct charlcd_priv {
 	unsigned long int flags;
 
 	/* Contains the LCD X and Y offset */
-	struct {
-		unsigned long int x;
-		unsigned long int y;
-	} addr;
+	struct charlcd_priv_addr addr;
 
-	/* Current escape sequence and it's length or -1 if outside */
+	/* Current escape sequence and its length or -1 if outside */
 	struct {
 		char buf[LCD_ESCAPE_LEN + 1];
 		int len;
-- 
2.11.0

[PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

NUL-terminate each individual number to be parsed.
To do this, the next command character and a pointer to its argument
are found and stored. The command character is then overwritten by NUL
before kstr* functions are called on the buffer.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 drivers/auxdisplay/charlcd.c | 53 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index a3d364e6c666..24cabe88c7f0 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -471,28 +471,63 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
 		break;
 	}
 	case 'x':	/* gotoxy : LxXXX[yYYY]; */
-	case 'y':	/* gotoxy : LyYYY[xXXX]; */
+	case 'y': {	/* gotoxy : LyYYY[xXXX]; */
+
+		char* nxt_esc;
+		char  nxt_cmd;
+		char  cmd;
+		struct charlcd_priv_addr tmp_addr;
+
 		if (!strchr(esc, ';'))
 			break;
 
-		while (*esc) {
-			if (*esc == 'x') {
-				esc++;
-				if (kstrtoul(esc, 10, &priv->addr.x) < 0)
+		/* sequence is processed whether legal or illegal */
+		processed = 1;
+
+		/* copy current address to temporary buffer */
+		tmp_addr = priv->addr;
+
+		nxt_cmd = *esc++;
+		nxt_esc = esc;
+
+		while ('\0' != *esc) {
+
+			cmd = nxt_cmd;
+			esc = nxt_esc;
+			nxt_esc = strpbrk(esc, "xy;");
+			if (NULL != nxt_esc) {
+				nxt_cmd = *nxt_esc;
+				/* terminate current sequence with NUL */
+				*nxt_esc++ = '\0';
+			}
+
+			if ('x' == cmd) {
+				if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
 					break;
-			} else if (*esc == 'y') {
-				esc++;
-				if (kstrtoul(esc, 10, &priv->addr.y) < 0)
+			} else if ('y' == cmd) {
+				if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
 					break;
 			} else {
+				/* break on unknown command or ';' */
 				break;
 			}
+
 		}
 
+		/* unknown commands in sequence will be followed by at least ';' */
+		if ('\0' != *esc)
+			break;
+
+		/* clamp new x/y coordinates */
+		if (tmp_addr.x >= lcd->width)
+			tmp_addr.x = lcd->width - 1;
+		tmp_addr.y %= lcd->height;
+
+		priv->addr = tmp_addr;
 		charlcd_gotoxy(lcd);
-		processed = 1;
 		break;
 	}
+	}
 
 	/* TODO: This indent party here got ugly, clean it! */
 	/* Check whether one flag was changed */
-- 
2.11.0

[PATCH 4/4] auxdisplay: charlcd: make home command unshift display

[Robert Abel]

A user has no way of unshifting the display programmatically once shifted.
Users cannot rely on ^[[H (home) to result in their message being seen either.
Use the actual HOME command 0x02 instead of only returning to x0/y0.
Users can still do ^[[Lx0y0; (go to x/y) if they wish to use the 'old' home
function.
Implement fast clearing of LCD by going to x0/y0 first, clearing display and
then calling home to possibly unshift display possibly avoiding artifacts by
not unshifting before clearing display.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 drivers/auxdisplay/charlcd.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 24cabe88c7f0..41f9aa4a73d4 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -43,6 +43,8 @@
 /* LCD commands */
 #define LCD_CMD_DISPLAY_CLEAR	0x01	/* Clear entire display */
 
+#define LCD_CMD_HOME            0x02    /* Set DDRAM address to 0 and unshift display */
+
 #define LCD_CMD_ENTRY_MODE	0x04	/* Set entry mode */
 #define LCD_CMD_CURSOR_INC	0x02	/* Increment cursor */
 
@@ -182,7 +184,8 @@ static void charlcd_home(struct charlcd *lcd)
 
 	priv->addr.x = 0;
 	priv->addr.y = 0;
-	charlcd_gotoxy(lcd);
+	lcd->ops->write_cmd(lcd, LCD_CMD_HOME);
+	long_sleep(2);
 }
 
 static void charlcd_print(struct charlcd *lcd, char c)
@@ -202,9 +205,12 @@ static void charlcd_print(struct charlcd *lcd, char c)
 
 static void charlcd_clear_fast(struct charlcd *lcd)
 {
+	struct charlcd_priv *priv = to_priv(lcd);
 	int pos;
 
-	charlcd_home(lcd);
+	priv->addr.x = 0;
+	priv->addr.y = 0;
+	charlcd_gotoxy(lcd);
 
 	if (lcd->ops->clear_fast)
 		lcd->ops->clear_fast(lcd);
-- 
2.11.0

Re: [PATCH 1/4] auxdisplay: charlcd: fix two-line command ^[[LN not marked as processed

[Geert Uytterhoeven]

On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
> Signed-off-by: Robert Abel <rabel@robertabel.eu>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] auxdisplay: charlcd: name x/y address struct

[Geert Uytterhoeven]

On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
> Signed-off-by: Robert Abel <rabel@robertabel.eu>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Geert Uytterhoeven]

Hi Robert,

On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
> NUL-terminate each individual number to be parsed.
> To do this, the next command character and a pointer to its argument
> are found and stored. The command character is then overwritten by NUL
> before kstr* functions are called on the buffer.
>
> Signed-off-by: Robert Abel <rabel@robertabel.eu>

Thanks for your patch!

> --- a/drivers/auxdisplay/charlcd.c
> +++ b/drivers/auxdisplay/charlcd.c
> @@ -471,28 +471,63 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
>                 break;
>         }
>         case 'x':       /* gotoxy : LxXXX[yYYY]; */
> -       case 'y':       /* gotoxy : LyYYY[xXXX]; */
> +       case 'y': {     /* gotoxy : LyYYY[xXXX]; */
> +
> +               char* nxt_esc;
> +               char  nxt_cmd;
> +               char  cmd;
> +               struct charlcd_priv_addr tmp_addr;
> +
>                 if (!strchr(esc, ';'))
>                         break;
>
> -               while (*esc) {
> -                       if (*esc == 'x') {
> -                               esc++;
> -                               if (kstrtoul(esc, 10, &priv->addr.x) < 0)
> +               /* sequence is processed whether legal or illegal */
> +               processed = 1;
> +
> +               /* copy current address to temporary buffer */
> +               tmp_addr = priv->addr;
> +
> +               nxt_cmd = *esc++;
> +               nxt_esc = esc;
> +
> +               while ('\0' != *esc) {
> +
> +                       cmd = nxt_cmd;
> +                       esc = nxt_esc;
> +                       nxt_esc = strpbrk(esc, "xy;");
> +                       if (NULL != nxt_esc) {
> +                               nxt_cmd = *nxt_esc;
> +                               /* terminate current sequence with NUL */
> +                               *nxt_esc++ = '\0';
> +                       }

So if none of "x", "y", or ";" is found, nxt_cmd will still contain
the current command?
Shouldn't it be reset to '\0' or so?

> +
> +                       if ('x' == cmd) {
> +                               if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
>                                         break;
> -                       } else if (*esc == 'y') {
> -                               esc++;
> -                               if (kstrtoul(esc, 10, &priv->addr.y) < 0)
> +                       } else if ('y' == cmd) {
> +                               if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
>                                         break;
>                         } else {
> +                               /* break on unknown command or ';' */
>                                 break;
>                         }
> +
>                 }
>
> +               /* unknown commands in sequence will be followed by at least ';' */
> +               if ('\0' != *esc)
> +                       break;
> +
> +               /* clamp new x/y coordinates */
> +               if (tmp_addr.x >= lcd->width)
> +                       tmp_addr.x = lcd->width - 1;
> +               tmp_addr.y %= lcd->height;
> +
> +               priv->addr = tmp_addr;
>                 charlcd_gotoxy(lcd);
> -               processed = 1;
>                 break;
>         }
> +       }
>
>         /* TODO: This indent party here got ugly, clean it! */
>         /* Check whether one flag was changed */

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Andy Shevchenko]

On Mon, Feb 26, 2018 at 1:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
> NUL-terminate each individual number to be parsed.
> To do this, the next command character and a pointer to its argument
> are found and stored. The command character is then overwritten by NUL
> before kstr* functions are called on the buffer.

Can we avoid yoda style of programming?

>         case 'x':       /* gotoxy : LxXXX[yYYY]; */
> +       case 'y': {     /* gotoxy : LyYYY[xXXX]; */
> +
> +               char* nxt_esc;
> +               char  nxt_cmd;
> +               char  cmd;
> +               struct charlcd_priv_addr tmp_addr;
> +
>                 if (!strchr(esc, ';'))
>                         break;
>
> +               /* sequence is processed whether legal or illegal */
> +               processed = 1;
> +
> +               /* copy current address to temporary buffer */
> +               tmp_addr = priv->addr;
> +
> +               nxt_cmd = *esc++;
> +               nxt_esc = esc;
> +
> +               while ('\0' != *esc) {
> +
> +                       cmd = nxt_cmd;
> +                       esc = nxt_esc;
> +                       nxt_esc = strpbrk(esc, "xy;");
> +                       if (NULL != nxt_esc) {
> +                               nxt_cmd = *nxt_esc;
> +                               /* terminate current sequence with NUL */
> +                               *nxt_esc++ = '\0';
> +                       }
> +
> +                       if ('x' == cmd) {
> +                               if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
>                                         break;

> +                       } else if ('y' == cmd) {
> +                               if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
>                                         break;

Perhaps instead of dancing around kstrtox() better to switch to
simple_strtoul() ?

>                         } else {
> +                               /* break on unknown command or ';' */
>                                 break;
>                         }
> +
>                 }
>
> +               /* unknown commands in sequence will be followed by at least ';' */
> +               if ('\0' != *esc)
> +                       break;
> +
> +               /* clamp new x/y coordinates */
> +               if (tmp_addr.x >= lcd->width)
> +                       tmp_addr.x = lcd->width - 1;
> +               tmp_addr.y %= lcd->height;
> +
> +               priv->addr = tmp_addr;
>                 charlcd_gotoxy(lcd);
>                 break;

>         }
> +       }

Same indentation level or my mailer hides this from me?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] auxdisplay: charlcd: fix two-line command ^[[LN not marked as processed

[Miguel Ojeda]

On Mon, Feb 26, 2018 at 9:34 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
>> Signed-off-by: Robert Abel <rabel@robertabel.eu>
>
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Good catch Robert! Picking it up for 4.17.

Cheers,
Miguel

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH 2/4] auxdisplay: charlcd: name x/y address struct

[Miguel Ojeda]

On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
> Signed-off-by: Robert Abel <rabel@robertabel.eu>
> ---
>  drivers/auxdisplay/charlcd.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
> index e3b2fd15c5a3..a3d364e6c666 100644
> --- a/drivers/auxdisplay/charlcd.c
> +++ b/drivers/auxdisplay/charlcd.c
> @@ -67,6 +67,11 @@
>  #define LCD_ESCAPE_LEN         24      /* Max chars for LCD escape command */
>  #define LCD_ESCAPE_CHAR                27      /* Use char 27 for escape command */
>
> +struct charlcd_priv_addr {
> +       unsigned long int x;
> +       unsigned long int y;
> +};
> +
>  struct charlcd_priv {
>         struct charlcd lcd;
>
> @@ -80,12 +85,9 @@ struct charlcd_priv {
>         unsigned long int flags;
>
>         /* Contains the LCD X and Y offset */
> -       struct {
> -               unsigned long int x;
> -               unsigned long int y;
> -       } addr;
> +       struct charlcd_priv_addr addr;

Since this is a very small change and you use it in the 3rd patch, I
think it is clearer to have it there.

>
> -       /* Current escape sequence and it's length or -1 if outside */
> +       /* Current escape sequence and its length or -1 if outside */

While this could be left in its own patch ("fix typo...").

Thanks!
Miguel

>         struct {
>                 char buf[LCD_ESCAPE_LEN + 1];
>                 int len;
> --
> 2.11.0
>

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Miguel Ojeda]

On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
> NUL-terminate each individual number to be parsed.
> To do this, the next command character and a pointer to its argument
> are found and stored. The command character is then overwritten by NUL
> before kstr* functions are called on the buffer.

It would be useful to have this description in the code itself as a comment.

>
> Signed-off-by: Robert Abel <rabel@robertabel.eu>
> ---
>  drivers/auxdisplay/charlcd.c | 53 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
> index a3d364e6c666..24cabe88c7f0 100644
> --- a/drivers/auxdisplay/charlcd.c
> +++ b/drivers/auxdisplay/charlcd.c
> @@ -471,28 +471,63 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
>                 break;
>         }
>         case 'x':       /* gotoxy : LxXXX[yYYY]; */
> -       case 'y':       /* gotoxy : LyYYY[xXXX]; */
> +       case 'y': {     /* gotoxy : LyYYY[xXXX]; */
> +

Extra empty line.

> +               char* nxt_esc;
> +               char  nxt_cmd;

I think skipping the "e" in the names do not buy us much :)

> +               char  cmd;
> +               struct charlcd_priv_addr tmp_addr;
> +
>                 if (!strchr(esc, ';'))
>                         break;
>
> -               while (*esc) {
> -                       if (*esc == 'x') {
> -                               esc++;
> -                               if (kstrtoul(esc, 10, &priv->addr.x) < 0)
> +               /* sequence is processed whether legal or illegal */
> +               processed = 1;
> +
> +               /* copy current address to temporary buffer */
> +               tmp_addr = priv->addr;
> +
> +               nxt_cmd = *esc++;
> +               nxt_esc = esc;
> +
> +               while ('\0' != *esc) {

Please do not change the style of the code w.r.t to the rest of the
file, which writes tests with the non-lvalue on the right-hand side
and do not compare against '\0'. Same for the rest.

> +

Extra empty line.

> +                       cmd = nxt_cmd;
> +                       esc = nxt_esc;
> +                       nxt_esc = strpbrk(esc, "xy;");
> +                       if (NULL != nxt_esc) {
> +                               nxt_cmd = *nxt_esc;
> +                               /* terminate current sequence with NUL */
> +                               *nxt_esc++ = '\0';
> +                       }
> +
> +                       if ('x' == cmd) {
> +                               if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
>                                         break;
> -                       } else if (*esc == 'y') {
> -                               esc++;
> -                               if (kstrtoul(esc, 10, &priv->addr.y) < 0)
> +                       } else if ('y' == cmd) {
> +                               if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
>                                         break;
>                         } else {
> +                               /* break on unknown command or ';' */
>                                 break;
>                         }

{ } not needed (your patch doesn't touch this -- just pointing it out).

> +
>                 }
>
> +               /* unknown commands in sequence will be followed by at least ';' */
> +               if ('\0' != *esc)
> +                       break;
> +
> +               /* clamp new x/y coordinates */
> +               if (tmp_addr.x >= lcd->width)
> +                       tmp_addr.x = lcd->width - 1;

tmp_addr.x = min(tmp_addr.x, lcd->width - 1);

> +               tmp_addr.y %= lcd->height;
> +
> +               priv->addr = tmp_addr;
>                 charlcd_gotoxy(lcd);
> -               processed = 1;
>                 break;
>         }
> +       }

On a general note, the code seems a bit convoluted for what it does,
specially without the comment written in the commit message :-) Isn't
it simpler to use a tiny array in the stack and put the numbers to be
converted instead of modifying the input sequence and dancing with
pointers?

Thanks for the patch!

Cheers,
Miguel

>
>         /* TODO: This indent party here got ugly, clean it! */
>         /* Check whether one flag was changed */
> --
> 2.11.0
>

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Miguel Ojeda]

On Mon, Feb 26, 2018 at 12:44 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 1:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
>> NUL-terminate each individual number to be parsed.
>> To do this, the next command character and a pointer to its argument
>> are found and stored. The command character is then overwritten by NUL
>> before kstr* functions are called on the buffer.
>
> Can we avoid yoda style of programming?
>
>>         case 'x':       /* gotoxy : LxXXX[yYYY]; */
>> +       case 'y': {     /* gotoxy : LyYYY[xXXX]; */
>> +
>> +               char* nxt_esc;
>> +               char  nxt_cmd;
>> +               char  cmd;
>> +               struct charlcd_priv_addr tmp_addr;
>> +
>>                 if (!strchr(esc, ';'))
>>                         break;
>>
>> +               /* sequence is processed whether legal or illegal */
>> +               processed = 1;
>> +
>> +               /* copy current address to temporary buffer */
>> +               tmp_addr = priv->addr;
>> +
>> +               nxt_cmd = *esc++;
>> +               nxt_esc = esc;
>> +
>> +               while ('\0' != *esc) {
>> +
>> +                       cmd = nxt_cmd;
>> +                       esc = nxt_esc;
>> +                       nxt_esc = strpbrk(esc, "xy;");
>> +                       if (NULL != nxt_esc) {
>> +                               nxt_cmd = *nxt_esc;
>> +                               /* terminate current sequence with NUL */
>> +                               *nxt_esc++ = '\0';
>> +                       }
>> +
>> +                       if ('x' == cmd) {
>> +                               if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
>>                                         break;
>
>> +                       } else if ('y' == cmd) {
>> +                               if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
>>                                         break;
>
> Perhaps instead of dancing around kstrtox() better to switch to
> simple_strtoul() ?

It seems deprecated:

/* Obsolete, do not use.  Use kstrto<foo> instead */
extern unsigned long simple_strtoul(const char *,char **,unsigned int);

>
>>                         } else {
>> +                               /* break on unknown command or ';' */
>>                                 break;
>>                         }
>> +
>>                 }
>>
>> +               /* unknown commands in sequence will be followed by at least ';' */
>> +               if ('\0' != *esc)
>> +                       break;
>> +
>> +               /* clamp new x/y coordinates */
>> +               if (tmp_addr.x >= lcd->width)
>> +                       tmp_addr.x = lcd->width - 1;
>> +               tmp_addr.y %= lcd->height;
>> +
>> +               priv->addr = tmp_addr;
>>                 charlcd_gotoxy(lcd);
>>                 break;
>
>>         }
>> +       }
>
> Same indentation level or my mailer hides this from me?

It is the same, but it is also how the other 'case's do it -- which in
this case looks just wrong since it is the last one of the switch. I
am not sure what is the preferred way of doing these kind of blocks,
coding-style.rst does not seem to give an example for this case.

Cheers,
Miguel

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Miguel Ojeda]

On Mon, Feb 26, 2018 at 5:49 PM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
>> NUL-terminate each individual number to be parsed.
>> To do this, the next command character and a pointer to its argument
>> are found and stored. The command character is then overwritten by NUL
>> before kstr* functions are called on the buffer.
>
> It would be useful to have this description in the code itself as a comment.
>
>>
>> Signed-off-by: Robert Abel <rabel@robertabel.eu>
>> ---
>>  drivers/auxdisplay/charlcd.c | 53 ++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 44 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
>> index a3d364e6c666..24cabe88c7f0 100644
>> --- a/drivers/auxdisplay/charlcd.c
>> +++ b/drivers/auxdisplay/charlcd.c
>> @@ -471,28 +471,63 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
>>                 break;
>>         }
>>         case 'x':       /* gotoxy : LxXXX[yYYY]; */
>> -       case 'y':       /* gotoxy : LyYYY[xXXX]; */
>> +       case 'y': {     /* gotoxy : LyYYY[xXXX]; */
>> +
>
> Extra empty line.
>
>> +               char* nxt_esc;
>> +               char  nxt_cmd;
>
> I think skipping the "e" in the names do not buy us much :)
>
>> +               char  cmd;
>> +               struct charlcd_priv_addr tmp_addr;
>> +
>>                 if (!strchr(esc, ';'))
>>                         break;
>>
>> -               while (*esc) {
>> -                       if (*esc == 'x') {
>> -                               esc++;
>> -                               if (kstrtoul(esc, 10, &priv->addr.x) < 0)
>> +               /* sequence is processed whether legal or illegal */
>> +               processed = 1;
>> +
>> +               /* copy current address to temporary buffer */
>> +               tmp_addr = priv->addr;
>> +
>> +               nxt_cmd = *esc++;
>> +               nxt_esc = esc;
>> +
>> +               while ('\0' != *esc) {
>
> Please do not change the style of the code w.r.t to the rest of the
> file, which writes tests with the non-lvalue on the right-hand side
> and do not compare against '\0'. Same for the rest.
>
>> +
>
> Extra empty line.
>
>> +                       cmd = nxt_cmd;
>> +                       esc = nxt_esc;
>> +                       nxt_esc = strpbrk(esc, "xy;");
>> +                       if (NULL != nxt_esc) {
>> +                               nxt_cmd = *nxt_esc;
>> +                               /* terminate current sequence with NUL */
>> +                               *nxt_esc++ = '\0';
>> +                       }
>> +
>> +                       if ('x' == cmd) {
>> +                               if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
>>                                         break;
>> -                       } else if (*esc == 'y') {
>> -                               esc++;
>> -                               if (kstrtoul(esc, 10, &priv->addr.y) < 0)
>> +                       } else if ('y' == cmd) {
>> +                               if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
>>                                         break;
>>                         } else {
>> +                               /* break on unknown command or ';' */
>>                                 break;
>>                         }
>
> { } not needed (your patch doesn't touch this -- just pointing it out).

Disregard this one, just checked and coding-style.rst specifies one
must use braces if the other branches need to use them.

Cheers,
Miguel

>
>> +
>>                 }
>>
>> +               /* unknown commands in sequence will be followed by at least ';' */
>> +               if ('\0' != *esc)
>> +                       break;
>> +
>> +               /* clamp new x/y coordinates */
>> +               if (tmp_addr.x >= lcd->width)
>> +                       tmp_addr.x = lcd->width - 1;
>
> tmp_addr.x = min(tmp_addr.x, lcd->width - 1);
>
>> +               tmp_addr.y %= lcd->height;
>> +
>> +               priv->addr = tmp_addr;
>>                 charlcd_gotoxy(lcd);
>> -               processed = 1;
>>                 break;
>>         }
>> +       }
>
> On a general note, the code seems a bit convoluted for what it does,
> specially without the comment written in the commit message :-) Isn't
> it simpler to use a tiny array in the stack and put the numbers to be
> converted instead of modifying the input sequence and dancing with
> pointers?
>
> Thanks for the patch!
>
> Cheers,
> Miguel
>
>>
>>         /* TODO: This indent party here got ugly, clean it! */
>>         /* Check whether one flag was changed */
>> --
>> 2.11.0
>>

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Andy Shevchenko]

On Mon, Feb 26, 2018 at 6:54 PM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 12:44 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Mon, Feb 26, 2018 at 1:54 AM, Robert Abel <rabel@robertabel.eu> wrote:

>>> +                       if ('x' == cmd) {
>>> +                               if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
>>>                                         break;
>>
>>> +                       } else if ('y' == cmd) {
>>> +                               if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
>>>                                         break;
>>
>> Perhaps instead of dancing around kstrtox() better to switch to
>> simple_strtoul() ?
>
> It seems deprecated:
>
> /* Obsolete, do not use.  Use kstrto<foo> instead */
> extern unsigned long simple_strtoul(const char *,char **,unsigned int);

It has been discussed several times. The comment is simple wrong.

Because of the requirement of kstrtox() to have a \0 or \n followed by
\0 as "end of field".
simple_strto*() is suitable to be run in place.

>>>         }
>>> +       }
>>
>> Same indentation level or my mailer hides this from me?
>
> It is the same, but it is also how the other 'case's do it -- which in
> this case looks just wrong since it is the last one of the switch. I
> am not sure what is the preferred way of doing these kind of blocks,
> coding-style.rst does not seem to give an example for this case.

Comes to my mind
- using }}
- putting default in between
- ... ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] auxdisplay: charlcd: make home command unshift display

[Miguel Ojeda]

On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
> A user has no way of unshifting the display programmatically once shifted.
> Users cannot rely on ^[[H (home) to result in their message being seen either.
> Use the actual HOME command 0x02 instead of only returning to x0/y0.
> Users can still do ^[[Lx0y0; (go to x/y) if they wish to use the 'old' home
> function.
> Implement fast clearing of LCD by going to x0/y0 first, clearing display and
> then calling home to possibly unshift display possibly avoiding artifacts by
> not unshifting before clearing display.

Thanks for the patch! It seems reasonable. However, like in the
previous patch, I feel that these commit messages describe -- usefully
-- the current code and some of the decisions for its design, and as
such they should be part of the code. Could you please put some of
that reasoning in the actual code? i.e. as comments for the
charlcd_clear_fast() and charlcd_home() functions, explaining why each
of them is implemented the way it is. That would be great. Also
hinting that the users can also use the "old" home functionality (or
maybe implementing it as another third alternative). Then you can
leave the commit message explaining only why the change was done, i.e.
why it improves the previous situation, referring to the new
functions.

Cheers,
Miguel

>
> Signed-off-by: Robert Abel <rabel@robertabel.eu>
> ---
>  drivers/auxdisplay/charlcd.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
> index 24cabe88c7f0..41f9aa4a73d4 100644
> --- a/drivers/auxdisplay/charlcd.c
> +++ b/drivers/auxdisplay/charlcd.c
> @@ -43,6 +43,8 @@
>  /* LCD commands */
>  #define LCD_CMD_DISPLAY_CLEAR  0x01    /* Clear entire display */
>
> +#define LCD_CMD_HOME            0x02    /* Set DDRAM address to 0 and unshift display */
> +
>  #define LCD_CMD_ENTRY_MODE     0x04    /* Set entry mode */
>  #define LCD_CMD_CURSOR_INC     0x02    /* Increment cursor */
>
> @@ -182,7 +184,8 @@ static void charlcd_home(struct charlcd *lcd)
>
>         priv->addr.x = 0;
>         priv->addr.y = 0;
> -       charlcd_gotoxy(lcd);
> +       lcd->ops->write_cmd(lcd, LCD_CMD_HOME);
> +       long_sleep(2);
>  }
>
>  static void charlcd_print(struct charlcd *lcd, char c)
> @@ -202,9 +205,12 @@ static void charlcd_print(struct charlcd *lcd, char c)
>
>  static void charlcd_clear_fast(struct charlcd *lcd)
>  {
> +       struct charlcd_priv *priv = to_priv(lcd);
>         int pos;
>
> -       charlcd_home(lcd);
> +       priv->addr.x = 0;
> +       priv->addr.y = 0;
> +       charlcd_gotoxy(lcd);
>
>         if (lcd->ops->clear_fast)
>                 lcd->ops->clear_fast(lcd);
> --
> 2.11.0
>

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Miguel Ojeda]

On Mon, Feb 26, 2018 at 6:09 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 6:54 PM, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>> On Mon, Feb 26, 2018 at 12:44 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Mon, Feb 26, 2018 at 1:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
>
>>>> +                       if ('x' == cmd) {
>>>> +                               if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
>>>>                                         break;
>>>
>>>> +                       } else if ('y' == cmd) {
>>>> +                               if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
>>>>                                         break;
>>>
>>> Perhaps instead of dancing around kstrtox() better to switch to
>>> simple_strtoul() ?
>>
>> It seems deprecated:
>>
>> /* Obsolete, do not use.  Use kstrto<foo> instead */
>> extern unsigned long simple_strtoul(const char *,char **,unsigned int);
>
> It has been discussed several times. The comment is simple wrong.
>
> Because of the requirement of kstrtox() to have a \0 or \n followed by
> \0 as "end of field".
> simple_strto*() is suitable to be run in place.

I agree that in-place versions of these kind of string functions are
very useful, don't get me wrong! But unless someone changes the
"official" comment, we shouldn't add new code relying on them.

>
>>>>         }
>>>> +       }
>>>
>>> Same indentation level or my mailer hides this from me?
>>
>> It is the same, but it is also how the other 'case's do it -- which in
>> this case looks just wrong since it is the last one of the switch. I
>> am not sure what is the preferred way of doing these kind of blocks,
>> coding-style.rst does not seem to give an example for this case.
>
> Comes to my mind
> - using }}
> - putting default in between

That is a clever one :-) But gcc complains, so we would need default +
break, and that looks wrong as well. I would just move those two
blocks into their own static function. The function is already long
enough, specially with the new code. For small inside-switch blocks, I
would just create the block at the level of the code, just like you
would do inside functions. We can have another later patch to clean
that up (and also the stuff below, which even has a TODO comment
regarding it!).

Cheers,
Miguel

> - ... ?
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Andy Shevchenko]

On Mon, Feb 26, 2018 at 7:26 PM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 6:09 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Mon, Feb 26, 2018 at 6:54 PM, Miguel Ojeda
>> <miguel.ojeda.sandonis@gmail.com> wrote:
>>> On Mon, Feb 26, 2018 at 12:44 PM, Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> On Mon, Feb 26, 2018 at 1:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
>>
>>>>> +                       if ('x' == cmd) {
>>>>> +                               if (kstrtoul(esc, 10, &tmp_addr.x) < 0)
>>>>>                                         break;
>>>>
>>>>> +                       } else if ('y' == cmd) {
>>>>> +                               if (kstrtoul(esc, 10, &tmp_addr.y) < 0)
>>>>>                                         break;
>>>>
>>>> Perhaps instead of dancing around kstrtox() better to switch to
>>>> simple_strtoul() ?
>>>
>>> It seems deprecated:
>>>
>>> /* Obsolete, do not use.  Use kstrto<foo> instead */
>>> extern unsigned long simple_strtoul(const char *,char **,unsigned int);
>>
>> It has been discussed several times. The comment is simple wrong.
>>
>> Because of the requirement of kstrtox() to have a \0 or \n followed by
>> \0 as "end of field".
>> simple_strto*() is suitable to be run in place.
>
> I agree that in-place versions of these kind of string functions are
> very useful, don't get me wrong! But unless someone changes the
> "official" comment, we shouldn't add new code relying on them.

I don't know what you afraid of, but be my guest.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

Hi Geert,

On 26 Feb 2018 09:46, Geert Uytterhoeven wrote:
>> +
>> +               nxt_cmd = *esc++;
>> +               nxt_esc = esc;
>> +
>> +               while ('\0' != *esc) {
>> +
>> +                       cmd = nxt_cmd;
>> +                       esc = nxt_esc;
>> +                       nxt_esc = strpbrk(esc, "xy;");
>> +                       if (NULL != nxt_esc) {
>> +                               nxt_cmd = *nxt_esc;
>> +                               /* terminate current sequence with NUL */
>> +                               *nxt_esc++ = '\0';
>> +                       }
> 
> So if none of "x", "y", or ";" is found, nxt_cmd will still contain
> the current command?
> Shouldn't it be reset to '\0' or so?

I originally had a comment there, but it then felt kinda silly.
Since one of the conditions of the code is that a semicolon ';' is
detected at the end of the stream, the situation you describe never
occurs until the end of the command string.

nxt_esc == NULL only when cmd == ';' and *esc == NULL. Since there is
nothing to terminate and the code won't run again (due to while (*esc)),
I didn't reset nxt_cmd. Other than that, I also don't know that there is
a sensible reset value.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

Hi Andy, Hi Miguel,

On 26 Feb 2018 12:44, Andy Shevchenko wrote:
> Can we avoid yoda style of programming?
On 26 Feb 2018 17:49, Miguel Ojeda wrote:
> Please do not change the style of the code w.r.t to the rest of the
> file, which writes tests with the non-lvalue on the right-hand side
> and do not compare against '\0'. Same for the rest.

I am actually a fan of yoda-style programming, although its value is
diminished with modern IDEs, which we all use, right?

On 26 Feb 2018 17:54, Miguel Ojeda wrote:
> On Mon, Feb 26, 2018 at 12:44 PM, Andy Shevchenko
>> Perhaps instead of dancing around kstrtox() better to switch to
>> simple_strtoul() ?
>
> It seems deprecated:
>
> /* Obsolete, do not use.  Use kstrto<foo> instead */
> extern unsigned long simple_strtoul(const char *,char **,unsigned int);

I thought the whole point was that simple_strtoul is deprecated and on
the kill list? Isn't that kind of against the whole argument of
re-inventing the wheel?
If using simple_strtoul is an option, it might be best to bring it back
and not touch the buffer at all.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

On 26 Feb 2018 17:49, Miguel Ojeda wrote:
> On a general note, the code seems a bit convoluted for what it does,
> specially without the comment written in the commit message :-) Isn't
> it simpler to use a tiny array in the stack and put the numbers to be
> converted instead of modifying the input sequence and dancing with
> pointers?

That's what I felt at first, too. If we can drop the backwards
compatibility of repeated xy commands, the whole affair gets much
easier, but will unfortunately break existing use.

Ex. ^[[Lx004y002x006; --> x6y2, because repeats of x would just
overwrite earlier values. That's what the while loop allowed in the
first place.

I suspect the while loop to parse was just a clever way of parsing y
followed by x and x followed by y using the same code and the
overwriting behavior is actually an unaccounted-for side-effect.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

Hi @ll,

this is a discussion stemming from drivers/auxdisplay about the usage
and possible deprecation of simple_strto* set of functions found in
lib/vsprintf.
I cc'ed everybody who signed off and also everybody who
signed/authored/added/removed more than 20% of lib/vsprintf.

This has come up, because some auxdisplay functionality was broken by a
well-intentioned patch to switch from simple_strtoul to kstrtoul in
129957069e6af42a6e021d90679c56662c95f7e1
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=129957069e6af42a6e021d90679c56662c95f7e1)
and now there is discussion about whether the simple_strto* functions
are really obsolete.

On 26 Feb 2018 18:26, Miguel Ojeda wrote:
> On Mon, Feb 26, 2018 at 6:09 PM, Andy Shevchenko wrote:
>> On Mon, Feb 26, 2018 at 6:54 PM, Miguel Ojeda wrote:
>>> On Mon, Feb 26, 2018 at 12:44 PM, Andy Shevchenko wrote:
>>>> Perhaps instead of dancing around kstrtox() better to switch to
>>>> simple_strtoul() ?
>>>
>>> It seems deprecated:
>>>
>>> /* Obsolete, do not use.  Use kstrto<foo> instead */
>>> extern unsigned long simple_strtoul(const char *,char **,unsigned int);
>>
>> It has been discussed several times. The comment is simple wrong.
>>
>> Because of the requirement of kstrtox() to have a \0 or \n followed by
>> \0 as "end of field".
>> simple_strto*() is suitable to be run in place.
> 
> I agree that in-place versions of these kind of string functions are
> very useful, don't get me wrong! But unless someone changes the
> "official" comment, we shouldn't add new code relying on them.

So can we get clarity in form of a patch here, or not?
I'm hesitant to move back to simple_kstroul, because to me it seems that
general consensus (at least when the comment was put there) was to
remove it:

> commit 462e471107624fe9bd8b6353ac13e06305c3f3fd
> Author: Eldad Zack <>
> Date:   Mon Dec 17 16:03:05 2012 -0800
> 
>     simple_strto*: annotate function as obsolete
> 
>     Update the documentation for simple_strto* to reflect that it has been
>     obsoleted and advise the usage of kstrto*.
> 
>     Signed-off-by: Eldad Zack <>
>     Cc: J. Bruce Fields <>
>     Cc: Joe Perches <>
>     Cc: Randy Dunlap <>
>     Cc: Alexey Dobriyan <>
>     Cc: Rob Landley <>
>     Signed-off-by: Andrew Morton <>
>     Signed-off-by: Linus Torvalds <>

One big advantage simple_kstro* functions have over their kstrto*
cousins is that strings don't need to be null-terminated. Instead, they
will be parsed as far as they can be and a pointer to the end of the
respective number will be returned.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

Hi Miguel,

On 26 Feb 2018 17:49, Miguel Ojeda wrote:
> On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
>> +               /* clamp new x/y coordinates */
>> +               if (tmp_addr.x >= lcd->width)
>> +                       tmp_addr.x = lcd->width - 1;
> 
> tmp_addr.x = min(tmp_addr.x, lcd->width - 1);

Had throught of that, too. However, it introduces a warning about type
mismatch, because lcd->width is int while tmp_addr.x is unsigned long
int. I didn't fell like saving a line warranted much bigger changes to
the lcd struct.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Miguel Ojeda]

On Mon, Feb 26, 2018 at 11:38 PM, Robert Abel <rabel@robertabel.eu> wrote:
> Hi Andy, Hi Miguel,
>
> On 26 Feb 2018 12:44, Andy Shevchenko wrote:
>> Can we avoid yoda style of programming?
> On 26 Feb 2018 17:49, Miguel Ojeda wrote:
>> Please do not change the style of the code w.r.t to the rest of the
>> file, which writes tests with the non-lvalue on the right-hand side
>> and do not compare against '\0'. Same for the rest.
>
> I am actually a fan of yoda-style programming, although its value is
> diminished with modern IDEs, which we all use, right?

The issue here is the consistency with the rest of the file (and with
the vast majority of the kernel as well), not with the style in
itself. If every single developer would write patches in their own
style, everything would be a mess, so I can't accept a patch like
that, sorry.

In any case, most compilers warn in case of assignment (Wparentheses,
which is on by default for the kernel), no need for a fancy IDE for
this particular thing. :-)

>
> On 26 Feb 2018 17:54, Miguel Ojeda wrote:
>> On Mon, Feb 26, 2018 at 12:44 PM, Andy Shevchenko
>>> Perhaps instead of dancing around kstrtox() better to switch to
>>> simple_strtoul() ?
>>
>> It seems deprecated:
>>
>> /* Obsolete, do not use.  Use kstrto<foo> instead */
>> extern unsigned long simple_strtoul(const char *,char **,unsigned int);
>
> I thought the whole point was that simple_strtoul is deprecated and on
> the kill list? Isn't that kind of against the whole argument of
> re-inventing the wheel?

It is not about reinventing the wheel or not. Any internal kernel
interface can change at any point -- which happens usually whenever
someone comes up with a better way of doing things and others agree,
specially if the benefits outweigh the risks/costs.

> If using simple_strtoul is an option, it might be best to bring it back
> and not touch the buffer at all.

It isn't an option for me as a maintainer (unless there is really,
really no better way of doing it), as long as we don't first agree
(i.e. the kernel) what is obsoleted or not.

Cheers,
Miguel

>
> Regards,
>
> Robert

Re: [PATCH 1/3] auxdisplay: charlcd: fix hex literal ranges for graphics command

[Robert Abel]

On 26 Feb 2018 00:54, Robert Abel wrote:> Robert Abel (4):
>   auxdisplay: charlcd: fix two-line command ^[[LN not marked as
>     processed
>   auxdisplay: charlcd: name x/y address struct
>   auxdisplay: charlcd: fix x/y address commands
>   auxdisplay: charlcd: make home command unshift display

So I think as it stands, only the first patch is being picked up, while
the other three will need rework, which is fine.

I think I'm going to wait a little while with the x/y fix until the
simple_strto* situation has settled.

For the home command basically only more comments in code should be
necessary.
More comments will also be good for the x/y issue once I change the
implementation.

Regards,

Robert

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Willy Tarreau]

On Mon, Feb 26, 2018 at 11:43:36PM +0100, Robert Abel wrote:
> On 26 Feb 2018 17:49, Miguel Ojeda wrote:
> > On a general note, the code seems a bit convoluted for what it does,
> > specially without the comment written in the commit message :-) Isn't
> > it simpler to use a tiny array in the stack and put the numbers to be
> > converted instead of modifying the input sequence and dancing with
> > pointers?
> 
> That's what I felt at first, too. If we can drop the backwards
> compatibility of repeated xy commands, the whole affair gets much
> easier, but will unfortunately break existing use.
> 
> Ex. ^[[Lx004y002x006; --> x6y2, because repeats of x would just
> overwrite earlier values. That's what the while loop allowed in the
> first place.
> 
> I suspect the while loop to parse was just a clever way of parsing y
> followed by x and x followed by y using the same code and the
> overwriting behavior is actually an unaccounted-for side-effect.

Well actually I don't see a problem there at all. The principle is simply
to accept any sequence assigning x or y or both. If you write x4y2x6, it
simply means that you changed your mind regarding x and that the last
value (6) is the one you want. Just as if you wrote "^[[Lx4;^[[y2;^[[x6;".
The while loop doesn't even try to do anything clever, it simply parses
everything matching x and y followed by digits. I think the only reason
for having both x and y processed in the same loop was to call
charlcd_gotoxy() only once for both axes.

Regards,
Willy

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Willy Tarreau]

On Tue, Feb 27, 2018 at 12:05:10AM +0100, Robert Abel wrote:
> Hi Miguel,
> 
> On 26 Feb 2018 17:49, Miguel Ojeda wrote:
> > On Mon, Feb 26, 2018 at 12:54 AM, Robert Abel <rabel@robertabel.eu> wrote:
> >> +               /* clamp new x/y coordinates */
> >> +               if (tmp_addr.x >= lcd->width)
> >> +                       tmp_addr.x = lcd->width - 1;
> > 
> > tmp_addr.x = min(tmp_addr.x, lcd->width - 1);
> 
> Had throught of that, too. However, it introduces a warning about type
> mismatch, because lcd->width is int while tmp_addr.x is unsigned long
> int. I didn't fell like saving a line warranted much bigger changes to
> the lcd struct.

The you can use min_t() :

      tmp_addr.x = min_t(unsigned, tmp_addr.x, lcd->width - 1);

Willy

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Miguel Ojeda]

On Tue, Feb 27, 2018 at 6:19 AM, Willy Tarreau <w@1wt.eu> wrote:
> On Mon, Feb 26, 2018 at 11:43:36PM +0100, Robert Abel wrote:
>> On 26 Feb 2018 17:49, Miguel Ojeda wrote:
>> > On a general note, the code seems a bit convoluted for what it does,
>> > specially without the comment written in the commit message :-) Isn't
>> > it simpler to use a tiny array in the stack and put the numbers to be
>> > converted instead of modifying the input sequence and dancing with
>> > pointers?
>>
>> That's what I felt at first, too. If we can drop the backwards
>> compatibility of repeated xy commands, the whole affair gets much
>> easier, but will unfortunately break existing use.
>>
>> Ex. ^[[Lx004y002x006; --> x6y2, because repeats of x would just
>> overwrite earlier values. That's what the while loop allowed in the
>> first place.
>>
>> I suspect the while loop to parse was just a clever way of parsing y
>> followed by x and x followed by y using the same code and the
>> overwriting behavior is actually an unaccounted-for side-effect.
>
> Well actually I don't see a problem there at all. The principle is simply
> to accept any sequence assigning x or y or both. If you write x4y2x6, it
> simply means that you changed your mind regarding x and that the last
> value (6) is the one you want. Just as if you wrote "^[[Lx4;^[[y2;^[[x6;".
> The while loop doesn't even try to do anything clever, it simply parses
> everything matching x and y followed by digits. I think the only reason
> for having both x and y processed in the same loop was to call
> charlcd_gotoxy() only once for both axes.
>

Robert, Willy, Geert, Andy: what about this? (sending the patch
separately, otherwise gmail messes the code up).

Cheers,
Miguel

> Regards,
> Willy

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

Hi Willy,

On 27 Feb 2018 06:19, Willy Tarreau wrote:
> Well actually I don't see a problem there at all. The principle is simply
> to accept any sequence assigning x or y or both. If you write x4y2x6, it
> simply means that you changed your mind regarding x and that the last
> value (6) is the one you want. Just as if you wrote "^[[Lx4;^[[y2;^[[x6;".
> The while loop doesn't even try to do anything clever, it simply parses
> everything matching x and y followed by digits. I think the only reason
> for having both x and y processed in the same loop was to call
> charlcd_gotoxy() only once for both axes.

I didn't say it is a problem. It is however an edge case that incurs a
lot of code for little to no functionality.
I'd much prefer if we broke backwards compatibility here and actually
only parse the format that is indicated in the comment:

>         case 'x':       /* gotoxy : LxXXX[yYYY]; */
>         case 'y':       /* gotoxy : LyYYY[xXXX]; */
>  

Exactly one x command followed exactly by zero or one y command or
vice-versa.

If somebody changes their mind during the escape sequence, they can just
issue a new one instead of appending to the current one.

I'll post an example patch.

Regards,

Robert

[PATCH RFC 0/2] auxdisplay: charlcd: fix movement and home commands

[Robert Abel]

I reworked the previous patches 2-4 into these two patches.
The first patch proproses a different solution to the movement command
parse code that reduces the movement command two at most two movement
subcommands (x or y standalone, x followed by y or vice-versa).

The second patch add comments in code to the home and clear_fast functions
and explains their behavior a bit more.

Robert Abel (2):
  auxdisplay: charlcd: fix x/y address commands
  auxdisplay: charlcd: make home command unshift display

 drivers/auxdisplay/charlcd.c | 137 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 122 insertions(+), 15 deletions(-)

-- 
2.11.0

[PATCH 1/2] auxdisplay: charlcd: fix x/y address commands

[Robert Abel]

The current version does not parse x/y commands at all.
Simplify the x/y command syntax to the one indicated in
the comment all along and introduce a parsing function
that handles parsing a sequence of one or two subcommands
where each subcommand must appear at most once.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 drivers/auxdisplay/charlcd.c | 109 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 96 insertions(+), 13 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index e3b2fd15c5a3..ae078f414539 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -292,6 +292,96 @@ static int charlcd_init_display(struct charlcd *lcd)
 	return 0;
 }
 
+/**
+ * parse_xy() - Parse coordinates of a movement command.
+ * @s: Pointer to null-terminated movement command string.
+ * @x: Pointer to x position.
+ * @y: Pointer to y position.
+ *
+ * Parses a movement command of the form "([xy][0-9]+){1,2};",
+ * where each group must begin with a different subcommand.
+ *
+ * The positions will only be updated when their respective
+ * subcommand is encountered and when the whole movement
+ * command is valid.
+ *
+ * The movement command string must contain a ';' at the end.
+ *
+ * For instance:
+ *   - ";"          fails.
+ *   - "x1;"        returns (1, <original y>).
+ *   - "y2x1;"      returns (1, 2).
+ *   - "x12y34x56;" fails.
+ *   - ""           fails.
+ *   - "x"          illegal input.
+ *   - "x;"         fails.
+ *   - "x1"         illegal input.
+ *   - "xy12;"      fails.
+ *   - "x12yy12;"   fails.
+ *   - "xx"         illegal input.
+ *
+ * Return: Returns whether the command is valid. The position arguments are
+ * only written if the parsing was successful.
+ */
+static bool parse_xy(const char *s, unsigned long *x, unsigned long *y)
+{
+
+	unsigned long new_x = *x;
+	unsigned long new_y = *y;
+
+	char xcoord[LCD_ESCAPE_LEN];
+	char ycoord[LCD_ESCAPE_LEN];
+	const char *split = strpbrk(s + 1, "xy");
+	const char *end = strchr(s, ';');
+	char *coord0 = xcoord;
+	char *coord1 = ycoord;
+	unsigned long *new0 = &new_x;
+	unsigned long *new1 = &new_y;
+
+	memset(xcoord, 0, sizeof(xcoord));
+	memset(ycoord, 0, sizeof(ycoord));
+
+	/* validate input */
+	switch (*s) {
+	case 'x':
+		if (split != NULL && *split != 'y')
+			return false;
+		break;
+	case 'y':
+		/* swap coordinates */
+		coord0 = ycoord;
+		coord1 = xcoord;
+		new0 = &new_y;
+		new1 = &new_x;
+
+		if (split != NULL && *split != 'x')
+			return false;
+		break;
+	default:
+		return false;
+	}
+
+	/* parse coordinate 0 and 1 */
+	if (split == NULL) {
+		memcpy(coord0, s + 1, end - s - 1);
+		if (kstrtoul(coord0, 10, new0) < 0)
+			return false;
+	} else {
+		memcpy(coord0, s + 1, split - s - 1);
+		memcpy(coord1, split + 1, end - split - 1);
+		if (kstrtoul(coord0, 10, new0) < 0)
+			return false;
+		if (kstrtoul(coord1, 10, new1) < 0)
+			return false;
+	}
+
+	/* update coordinates on success */
+	*x = new_x;
+	*y = new_y;
+	return true;
+
+}
+
 /*
  * These are the file operation function for user access to /dev/lcd
  * This function can also be called from inside the kernel, by
@@ -473,21 +563,14 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
 		if (!strchr(esc, ';'))
 			break;
 
-		while (*esc) {
-			if (*esc == 'x') {
-				esc++;
-				if (kstrtoul(esc, 10, &priv->addr.x) < 0)
-					break;
-			} else if (*esc == 'y') {
-				esc++;
-				if (kstrtoul(esc, 10, &priv->addr.y) < 0)
-					break;
-			} else {
-				break;
-			}
+		/* If the command is valid, move to the new address */
+		if (parse_xy(esc, &priv->addr.x, &priv->addr.y)) {
+			priv->addr.x = min_t(unsigned long, priv->addr.x, lcd->bwidth - 1);
+			priv->addr.y %= lcd->height;
+			charlcd_gotoxy(lcd);
 		}
 
-		charlcd_gotoxy(lcd);
+		/* Regardless of its validity, mark as processed */
 		processed = 1;
 		break;
 	}
-- 
2.11.0

[PATCH 2/2] auxdisplay: charlcd: make home command unshift display

[Robert Abel]

A user has no way of unshifting the display programmatically once shifted.
Users cannot rely on ^[[H (home) to result in their message being seen either.
Use the actual HOME command 0x02 instead of only returning to x0/y0.
Users can still do ^[[Lx0y0; (go to x/y) if they wish to use the 'old' home
function.
Implement fast clearing of LCD by going to x0/y0 first, clearing display and
then calling home to possibly unshift display possibly avoiding artifacts by
not unshifting before clearing display.

Signed-off-by: Robert Abel <rabel@robertabel.eu>
---
 drivers/auxdisplay/charlcd.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index ae078f414539..fdb3aa6423bf 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -43,6 +43,8 @@
 /* LCD commands */
 #define LCD_CMD_DISPLAY_CLEAR	0x01	/* Clear entire display */
 
+#define LCD_CMD_HOME            0x02    /* Set DDRAM address to 0 and unshift display */
+
 #define LCD_CMD_ENTRY_MODE	0x04	/* Set entry mode */
 #define LCD_CMD_CURSOR_INC	0x02	/* Increment cursor */
 
@@ -174,13 +176,24 @@ static void charlcd_gotoxy(struct charlcd *lcd)
 	lcd->ops->write_cmd(lcd, LCD_CMD_SET_DDRAM_ADDR | addr);
 }
 
+/**
+ * charlcd_home() - Return to DDRAM address 0 and unshift the display.
+ * @lcd: LCD descriptor structure.
+ *
+ * Return to coordinates x0/y0 and unshift the display.
+ *
+ * Note: Use the movement command ^[[Lx0y0; instead of home
+ * in case the display should not be unshifted.
+ */
 static void charlcd_home(struct charlcd *lcd)
 {
 	struct charlcd_priv *priv = to_priv(lcd);
 
+	/* return to x0/y0 and unshift the display */
 	priv->addr.x = 0;
 	priv->addr.y = 0;
-	charlcd_gotoxy(lcd);
+	lcd->ops->write_cmd(lcd, LCD_CMD_HOME);
+	long_sleep(2);
 }
 
 static void charlcd_print(struct charlcd *lcd, char c)
@@ -198,11 +211,21 @@ static void charlcd_print(struct charlcd *lcd, char c)
 		charlcd_gotoxy(lcd);
 }
 
+/**
+ * charlcd_clear_fast() - Perform a fast clear of the display and return home.
+ * @lcd: LCD descriptor structure.
+ *
+ * The display will be unshifted and at coordinates x0/y0 after fast clear.
+ */
 static void charlcd_clear_fast(struct charlcd *lcd)
 {
+	struct charlcd_priv *priv = to_priv(lcd);
 	int pos;
 
-	charlcd_home(lcd);
+	/* avoid artifacts by going to x0/y0 without unshifting the display */
+	priv->addr.x = 0;
+	priv->addr.y = 0;
+	charlcd_gotoxy(lcd);
 
 	if (lcd->ops->clear_fast)
 		lcd->ops->clear_fast(lcd);
@@ -210,6 +233,7 @@ static void charlcd_clear_fast(struct charlcd *lcd)
 		for (pos = 0; pos < min(2, lcd->height) * lcd->hwidth; pos++)
 			lcd->ops->write_data(lcd, ' ');
 
+	/* return to x0/y0 and unshift the display */
 	charlcd_home(lcd);
 }
 
-- 
2.11.0

Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands

[Willy Tarreau]

Hi Robert,

On Wed, Feb 28, 2018 at 12:29:38AM +0100, Robert Abel wrote:
> It is however an edge case that incurs a
> lot of code for little to no functionality.
> I'd much prefer if we broke backwards compatibility here and actually
> only parse the format that is indicated in the comment:
> 
> >         case 'x':       /* gotoxy : LxXXX[yYYY]; */
> >         case 'y':       /* gotoxy : LyYYY[xXXX]; */
> >  
> 
> Exactly one x command followed exactly by zero or one y command or
> vice-versa.
> 
> If somebody changes their mind during the escape sequence, they can just
> issue a new one instead of appending to the current one.
> 
> I'll post an example patch.

I'm sorry but I think that your patch has simply proven that your point
above doesn't stand. Adding 90 lines of code full of strchr, strpbrk and
memcpy to replace 12 trivial lines, while possibly breaking compatibility
isn't considered an improvement. Reducing code is an improvement,
multiplying it by 7 is not, it adds maintenance burden for no benefit.

Let's stick to Miguel's last version. At least now we know that the
alternatives are worse, which is great.

Thanks,
Willy