[U-Boot] [PATCH 02/21] common/lcd: Add command for setting cursor within lcd-framework

[Nikita Kiryanov]

Hi Hannes,

On 02/02/2015 09:32 AM, Hannes Petermaier wrote:
> "U-Boot" <u-boot-bounces at lists.denx.de> schrieb am 01.02.2015 15:48:51:
>
>> From: Nikita Kiryanov <nikita at compulab.co.il>
>> To: Hannes Petermaier <oe5hpm at oevsv.at>, u-boot at lists.denx.de
>> Date: 01.02.2015 15:49
>> Subject: Re: [U-Boot] [PATCH 02/21] common/lcd: Add command for setting
> cursor
>> within lcd-framework
>> Sent by: "U-Boot" <u-boot-bounces at lists.denx.de>
>>
>> Hi Hannes,
> Hi Nikita,
>
>>
>> On 01/30/2015 03:25 PM, Hannes Petermaier wrote:
>>> We need this function if we want to make some outputs i.e position the
> writing
>>> cursor out of u-boot scripts.
>>
>> This commit message is inaccurate. Positioning the writing cursor is not
> in
>> itself output.
>> Also, what is the use case for such a command?
>>
> I want to set the "cursor" on the screen to a specific position and write
> there something
> with "puts".
>
> For example:
>
> setcurs 1 9;            sets the cursor to first column and 9th row
> puts "Hello World!"     writes the text to the cursor.

Alright... So setcurs does not output anything, just prepares us to do output.
I think that's how the commit message should be phrased.

>
>>>
>>> Signed-off-by: Hannes Petermaier <oe5hpm at oevsv.at>
>>> ---
>>>    common/lcd.c |   21 +++++++++++++++++++++
>>>    1 file changed, 21 insertions(+)
>>>
>>> diff --git a/common/lcd.c b/common/lcd.c
>>> index cc34b8a..f418da9 100644
>>> --- a/common/lcd.c
>>> +++ b/common/lcd.c
>>> @@ -279,12 +279,33 @@ static int do_lcd_clear(cmd_tbl_t *cmdtp, int
> flag, int argc,
>>>       return 0;
>>>    }
>>>
>>> +static int do_lcd_setcursor(cmd_tbl_t *cmdtp, int flag, int argc,
>>> +             char *const argv[])
>>> +{
>>> +   unsigned int col, row;
>>> +
>>> +   if (argc != 3)
>>> +      return CMD_RET_USAGE;
>>> +
>>> +   col = simple_strtoul(argv[1], NULL, 10);
>>> +   row = simple_strtoul(argv[2], NULL, 10);
>>> +   lcd_position_cursor(col, row);
>>> +
>>> +   return 0;
>>> +}
>>> +
>>>    U_BOOT_CMD(
>>>       cls,   1,   1,   do_lcd_clear,
>>>       "clear screen",
>>>       ""
>>>    );
>>>
>>> +U_BOOT_CMD(
>>> +   setcurs, 3,   1,   do_lcd_setcursor,
>>> +   "sets cursor for 'puts'",

Another thing: this function is not really limited to working with puts. It
can do preparation for any other command that outputs to console. I recommend
rewriting the description string to not mention it, maybe something along the
lines of "set cursor on the screen".

>>> +   "    <col> <row> in character"
>>> +);
>>> +
>>
>> I think it would be better if the U_BOOT_CMD macros were adjacent to the
> functions they
>> use. Also, I think this command is better suited for the lcd_console.c
> file.
>
> You're maybe right. Only thing for decission was, that existing commands
> are allready defined within
> lcd.c.

That's ok; we should place code where it's most relevant, and your command
is related to lcd console functionality.

> But i've no problem to move it. What do you say? let's move?

Yes, please move it to lcd_console.c

-- 
Regards,
Nikita Kiryanov