diff mbox

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

Message ID 1422624324-15431-3-git-send-email-oe5hpm@oevsv.at
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Hannes Schmelzer Jan. 30, 2015, 1:25 p.m. UTC
We need this function if we want to make some outputs out of u-boot scripts.

Signed-off-by: Hannes Petermaier <oe5hpm@oevsv.at>
---
 common/lcd.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Nikita Kiryanov Feb. 1, 2015, 2:53 p.m. UTC | #1
Hi Hannes,

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?

> 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

> +	"print string on lcd-framebuffer",
> +	"    <string>"
> +);
> +
>   /*----------------------------------------------------------------------*/
>
>   static int lcd_init(void *lcdbase)
>
Hannes Schmelzer Feb. 2, 2015, 7:37 a.m. UTC | #2
> From: Nikita Kiryanov <nikita@compulab.co.il>
> To: Hannes Petermaier <oe5hpm@oevsv.at>, u-boot@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.

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

Only thing for decission was, that existing commands are allready defined 
within
lcd.c. But i've no problem to move it. What do you say? let's move?

> 
> > +   "print string on lcd-framebuffer",
> > +   "    <string>"
> > +);
> > +
> > 
/*----------------------------------------------------------------------*/
> >
> >   static int lcd_init(void *lcdbase)
> >
> 
> -- 
> Regards,
> Nikita Kiryanov
best regards,
Hannes
Nikita Kiryanov Feb. 3, 2015, 11:53 a.m. UTC | #3
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@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
diff mbox

Patch

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,
+	"print string on lcd-framebuffer",
+	"    <string>"
+);
+
 /*----------------------------------------------------------------------*/
 
 static int lcd_init(void *lcdbase)