[U-Boot] [PATCH 03/21] common/lcd: Add command for writing to lcd-display

[Nikita Kiryanov <nikita@compulab.co.il>]

Hi Hannes,

On 02/02/2015 09:37 AM, Hannes Petermaier wrote:
>> From: Nikita Kiryanov <nikita@compulab.co.il>
>> To: Hannes Petermaier <oe5hpm@oevsv.at>, u-boot at lists.denx.de
>> Date: 01.02.2015 15:53
>> Subject: Re: [U-Boot] [PATCH 03/21] common/lcd: Add command for writing
> to lcd-display
>> Sent by: "U-Boot" <u-boot-bounces@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 out of u-boot
> scripts.
>>>
>>
>> I think this commit message is missing information. What makes this
> necessary?
>> Why can't your script use regular echo commands with the lcd console
> enabled?
>
> Since i don't want that stdout is redirected to LCD (customer isn't
> interested in watching
> all output of u-boot) i need something to write out of scripts to the
> screen.
>
> For example we write of script in which mode we are booting, maybe also
> some default ip settings
> in recovery mode.

Alright, then please explain in the commit message that this function is meant
to give user the option to output to lcd without enabling lcd console.

>
>>
>>> Signed-off-by: Hannes Petermaier <oe5hpm@oevsv.at>
>>> ---
>>>    common/lcd.c |   17 +++++++++++++++++
>>>    1 file changed, 17 insertions(+)
>>>
>>> diff --git a/common/lcd.c b/common/lcd.c
>>> index f418da9..755388f 100644
>>> --- a/common/lcd.c
>>> +++ b/common/lcd.c
>>> @@ -279,6 +279,17 @@ static int do_lcd_clear(cmd_tbl_t *cmdtp, int
> flag, int argc,
>>>       return 0;
>>>    }
>>>
>>> +static int do_lcd_puts(cmd_tbl_t *cmdtp, int flag, int argc,
>>> +             char *const argv[])
>>> +{
>>> +   if (argc != 2)
>>> +      return CMD_RET_USAGE;
>>> +
>>> +   lcd_puts(argv[1]);
>>> +
>>> +   return 0;
>>> +}
>>> +
>>>    static int do_lcd_setcursor(cmd_tbl_t *cmdtp, int flag, int argc,
>>>                 char *const argv[])
>>>    {
>>> @@ -306,6 +317,12 @@ U_BOOT_CMD(
>>>       "    <col> <row> in character"
>>>    );
>>>
>>> +U_BOOT_CMD(
>>> +   puts,   2,   1,   do_lcd_puts,
>>
>> "puts" is too generic for an lcd specific function. I would expect to
> see something
>> with an "lcd" prefix. Also, this code seems better suited for
> lcd_console.c
>
> Okay, thats right - should we rename it to "lcdputs" ?

That sounds better, yes.

>
> 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