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@xxxxxxxxx> wrote:
> On Mon, Feb 26, 2018 at 6:54 PM, Miguel Ojeda
> <miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>> On Mon, Feb 26, 2018 at 12:44 PM, Andy Shevchenko
>> <andy.shevchenko@xxxxxxxxx> wrote:
>>> On Mon, Feb 26, 2018 at 1:54 AM, Robert Abel <rabel@xxxxxxxxxxxxx> 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