* [PATCH 4/4] auxdisplay: charlcd: make home command unshift display
2018-02-25 23:54 ` [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands Robert Abel
@ 2018-02-25 23:54 ` Robert Abel
2018-02-26 17:16 ` Miguel Ojeda
2018-02-26 8:46 ` [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands Geert Uytterhoeven
` (2 subsequent siblings)
3 siblings, 1 reply; 51+ messages in thread
From: Robert Abel @ 2018-02-25 23:54 UTC (permalink / raw)
To: linux-kernel
Cc: Robert Abel, Miguel Ojeda, Willy Tarreau, Geert Uytterhoeven,
Andy Shevchenko
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
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 4/4] auxdisplay: charlcd: make home command unshift display
2018-02-25 23:54 ` [PATCH 4/4] auxdisplay: charlcd: make home command unshift display Robert Abel
@ 2018-02-26 17:16 ` Miguel Ojeda
0 siblings, 0 replies; 51+ messages in thread
From: Miguel Ojeda @ 2018-02-26 17:16 UTC (permalink / raw)
To: Robert Abel
Cc: linux-kernel, Willy Tarreau, Geert Uytterhoeven, Andy Shevchenko
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
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-25 23:54 ` [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands Robert Abel
2018-02-25 23:54 ` [PATCH 4/4] auxdisplay: charlcd: make home command unshift display Robert Abel
@ 2018-02-26 8:46 ` Geert Uytterhoeven
2018-02-26 22:29 ` Robert Abel
2018-02-26 11:44 ` Andy Shevchenko
2018-02-26 16:49 ` Miguel Ojeda
3 siblings, 1 reply; 51+ messages in thread
From: Geert Uytterhoeven @ 2018-02-26 8:46 UTC (permalink / raw)
To: Robert Abel; +Cc: linux-kernel, Miguel Ojeda, Willy Tarreau, Andy Shevchenko
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 8:46 ` [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands Geert Uytterhoeven
@ 2018-02-26 22:29 ` Robert Abel
0 siblings, 0 replies; 51+ messages in thread
From: Robert Abel @ 2018-02-26 22:29 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-kernel, Miguel Ojeda, Willy Tarreau, Andy Shevchenko
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-25 23:54 ` [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands Robert Abel
2018-02-25 23:54 ` [PATCH 4/4] auxdisplay: charlcd: make home command unshift display Robert Abel
2018-02-26 8:46 ` [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands Geert Uytterhoeven
@ 2018-02-26 11:44 ` Andy Shevchenko
2018-02-26 16:54 ` Miguel Ojeda
2018-02-26 22:38 ` Robert Abel
2018-02-26 16:49 ` Miguel Ojeda
3 siblings, 2 replies; 51+ messages in thread
From: Andy Shevchenko @ 2018-02-26 11:44 UTC (permalink / raw)
To: Robert Abel; +Cc: linux-kernel, Miguel Ojeda, Willy Tarreau, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 11:44 ` Andy Shevchenko
@ 2018-02-26 16:54 ` Miguel Ojeda
2018-02-26 17:09 ` Andy Shevchenko
2018-02-26 22:38 ` Robert Abel
1 sibling, 1 reply; 51+ messages in thread
From: Miguel Ojeda @ 2018-02-26 16:54 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Robert Abel, linux-kernel, Willy Tarreau, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 16:54 ` Miguel Ojeda
@ 2018-02-26 17:09 ` Andy Shevchenko
2018-02-26 17:26 ` Miguel Ojeda
0 siblings, 1 reply; 51+ messages in thread
From: Andy Shevchenko @ 2018-02-26 17:09 UTC (permalink / raw)
To: Miguel Ojeda; +Cc: Robert Abel, linux-kernel, Willy Tarreau, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 17:09 ` Andy Shevchenko
@ 2018-02-26 17:26 ` Miguel Ojeda
2018-02-26 17:56 ` Andy Shevchenko
2018-02-26 23:00 ` Robert Abel
0 siblings, 2 replies; 51+ messages in thread
From: Miguel Ojeda @ 2018-02-26 17:26 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Robert Abel, linux-kernel, Willy Tarreau, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 17:26 ` Miguel Ojeda
@ 2018-02-26 17:56 ` Andy Shevchenko
2018-02-26 23:00 ` Robert Abel
1 sibling, 0 replies; 51+ messages in thread
From: Andy Shevchenko @ 2018-02-26 17:56 UTC (permalink / raw)
To: Miguel Ojeda; +Cc: Robert Abel, linux-kernel, Willy Tarreau, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 17:26 ` Miguel Ojeda
2018-02-26 17:56 ` Andy Shevchenko
@ 2018-02-26 23:00 ` Robert Abel
1 sibling, 0 replies; 51+ messages in thread
From: Robert Abel @ 2018-02-26 23:00 UTC (permalink / raw)
To: Miguel Ojeda, Andy Shevchenko
Cc: linux-kernel, Willy Tarreau, Geert Uytterhoeven, torvalds, akpm,
eldad, me, corbet, pantelis.antoniou
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 11:44 ` Andy Shevchenko
2018-02-26 16:54 ` Miguel Ojeda
@ 2018-02-26 22:38 ` Robert Abel
2018-02-26 23:06 ` Miguel Ojeda
1 sibling, 1 reply; 51+ messages in thread
From: Robert Abel @ 2018-02-26 22:38 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-kernel, Miguel Ojeda, Willy Tarreau, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 22:38 ` Robert Abel
@ 2018-02-26 23:06 ` Miguel Ojeda
0 siblings, 0 replies; 51+ messages in thread
From: Miguel Ojeda @ 2018-02-26 23:06 UTC (permalink / raw)
To: Robert Abel
Cc: Andy Shevchenko, linux-kernel, Willy Tarreau, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-25 23:54 ` [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands Robert Abel
` (2 preceding siblings ...)
2018-02-26 11:44 ` Andy Shevchenko
@ 2018-02-26 16:49 ` Miguel Ojeda
2018-02-26 16:57 ` Miguel Ojeda
` (2 more replies)
3 siblings, 3 replies; 51+ messages in thread
From: Miguel Ojeda @ 2018-02-26 16:49 UTC (permalink / raw)
To: Robert Abel
Cc: linux-kernel, Willy Tarreau, Geert Uytterhoeven, Andy Shevchenko
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
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 16:49 ` Miguel Ojeda
@ 2018-02-26 16:57 ` Miguel Ojeda
2018-02-26 22:43 ` Robert Abel
2018-02-26 23:05 ` Robert Abel
2 siblings, 0 replies; 51+ messages in thread
From: Miguel Ojeda @ 2018-02-26 16:57 UTC (permalink / raw)
To: Robert Abel
Cc: linux-kernel, Willy Tarreau, Geert Uytterhoeven, Andy Shevchenko
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
>>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 16:49 ` Miguel Ojeda
2018-02-26 16:57 ` Miguel Ojeda
@ 2018-02-26 22:43 ` Robert Abel
2018-02-27 5:19 ` Willy Tarreau
2018-02-26 23:05 ` Robert Abel
2 siblings, 1 reply; 51+ messages in thread
From: Robert Abel @ 2018-02-26 22:43 UTC (permalink / raw)
To: Miguel Ojeda
Cc: linux-kernel, Willy Tarreau, Geert Uytterhoeven, Andy Shevchenko
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 22:43 ` Robert Abel
@ 2018-02-27 5:19 ` Willy Tarreau
2018-02-27 19:31 ` Miguel Ojeda
2018-02-27 23:29 ` Robert Abel
0 siblings, 2 replies; 51+ messages in thread
From: Willy Tarreau @ 2018-02-27 5:19 UTC (permalink / raw)
To: Robert Abel
Cc: Miguel Ojeda, linux-kernel, Geert Uytterhoeven, Andy Shevchenko
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-27 5:19 ` Willy Tarreau
@ 2018-02-27 19:31 ` Miguel Ojeda
2018-02-27 23:29 ` Robert Abel
1 sibling, 0 replies; 51+ messages in thread
From: Miguel Ojeda @ 2018-02-27 19:31 UTC (permalink / raw)
To: Willy Tarreau
Cc: Robert Abel, linux-kernel, Geert Uytterhoeven, Andy Shevchenko
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-27 5:19 ` Willy Tarreau
2018-02-27 19:31 ` Miguel Ojeda
@ 2018-02-27 23:29 ` Robert Abel
2018-02-28 0:05 ` [PATCH RFC 0/2] auxdisplay: charlcd: fix movement and home commands Robert Abel
2018-02-28 4:21 ` [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands Willy Tarreau
1 sibling, 2 replies; 51+ messages in thread
From: Robert Abel @ 2018-02-27 23:29 UTC (permalink / raw)
To: Willy Tarreau
Cc: Miguel Ojeda, linux-kernel, Geert Uytterhoeven, Andy Shevchenko
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH RFC 0/2] auxdisplay: charlcd: fix movement and home commands
2018-02-27 23:29 ` Robert Abel
@ 2018-02-28 0:05 ` Robert Abel
2018-02-28 0:05 ` [PATCH 1/2] auxdisplay: charlcd: fix x/y address commands Robert Abel
2018-02-28 4:21 ` [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands Willy Tarreau
1 sibling, 1 reply; 51+ messages in thread
From: Robert Abel @ 2018-02-28 0:05 UTC (permalink / raw)
To: linux-kernel
Cc: Robert Abel, miguel.ojeda.sandonis, w, geert, andy.shevchenko
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 1/2] auxdisplay: charlcd: fix x/y address commands
2018-02-28 0:05 ` [PATCH RFC 0/2] auxdisplay: charlcd: fix movement and home commands Robert Abel
@ 2018-02-28 0:05 ` Robert Abel
2018-02-28 0:05 ` [PATCH 2/2] auxdisplay: charlcd: make home command unshift display Robert Abel
0 siblings, 1 reply; 51+ messages in thread
From: Robert Abel @ 2018-02-28 0:05 UTC (permalink / raw)
To: linux-kernel
Cc: Robert Abel, miguel.ojeda.sandonis, w, geert, andy.shevchenko
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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 2/2] auxdisplay: charlcd: make home command unshift display
2018-02-28 0:05 ` [PATCH 1/2] auxdisplay: charlcd: fix x/y address commands Robert Abel
@ 2018-02-28 0:05 ` Robert Abel
0 siblings, 0 replies; 51+ messages in thread
From: Robert Abel @ 2018-02-28 0:05 UTC (permalink / raw)
To: linux-kernel
Cc: Robert Abel, miguel.ojeda.sandonis, w, geert, andy.shevchenko
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
^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-27 23:29 ` Robert Abel
2018-02-28 0:05 ` [PATCH RFC 0/2] auxdisplay: charlcd: fix movement and home commands Robert Abel
@ 2018-02-28 4:21 ` Willy Tarreau
1 sibling, 0 replies; 51+ messages in thread
From: Willy Tarreau @ 2018-02-28 4:21 UTC (permalink / raw)
To: Robert Abel
Cc: Miguel Ojeda, linux-kernel, Geert Uytterhoeven, Andy Shevchenko
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 16:49 ` Miguel Ojeda
2018-02-26 16:57 ` Miguel Ojeda
2018-02-26 22:43 ` Robert Abel
@ 2018-02-26 23:05 ` Robert Abel
2018-02-27 5:20 ` Willy Tarreau
2 siblings, 1 reply; 51+ messages in thread
From: Robert Abel @ 2018-02-26 23:05 UTC (permalink / raw)
To: Miguel Ojeda
Cc: linux-kernel, Willy Tarreau, Geert Uytterhoeven, Andy Shevchenko
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 3/4] auxdisplay: charlcd: fix x/y address commands
2018-02-26 23:05 ` Robert Abel
@ 2018-02-27 5:20 ` Willy Tarreau
0 siblings, 0 replies; 51+ messages in thread
From: Willy Tarreau @ 2018-02-27 5:20 UTC (permalink / raw)
To: Robert Abel
Cc: Miguel Ojeda, linux-kernel, Geert Uytterhoeven, Andy Shevchenko
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
^ permalink raw reply [flat|nested] 51+ messages in thread