Re: [yocto] [psplash][PATCH] Add fbdev option to set the proper /dev/fbX.

[Richard Leitner]

Hi Julien,

On 05/30/2016 11:55 PM, Julien Gueytat wrote:
> 
> For FBDEV, I have been told on #yocto that the FBENV variable is kind of
> useless at least for the startup as the environment is not read yet.
> This why I replaced it. But I will let it available in the next patch if
> this is what should be done.

Ok, sorry I didn't knew that. In that case you can of course remove that
"feature". But please add this description somewhere to the commit text
or create a separate patch for it. So that everybody can understand why
it was removed (also in the future).

> 
> For the psplash documentation, I did not find any. Where am I supposed
> to complete it?

I just took a second look at your patch and saw that you've added the
option to the "Usage text". This should be enough. Sorry that I didn't
see it the first time! :-(

Unfortunately there is no "real" documentation for psplash AFAIK...

> 
> Thanks for pointing out that a patch should serve a single purpose. I
> will correct the indentation in a separate commit.
> 

Thanks.


Furthermore please add yourself to the AUTHORS file and add a short
description to the ChangeLog when re-sending the patch(es).

Thank you!

kind regards,
Richard
-- 
___
yocto mailing list
yocto@yoctoproject.org
https://lists.yoctoproject.org/listinfo/yocto

Re: [yocto] [psplash][PATCH] Add fbdev option to set the proper /dev/fbX.

[Julien Gueytat]

Thanks for your reply Richard.

For FBDEV, I have been told on #yocto that the FBENV variable is kind of 
useless at least for the startup as the environment is not read yet.
This why I replaced it. But I will let it available in the next patch if 
this is what should be done.


For the psplash documentation, I did not find any. Where am I supposed 
to complete it?


Thanks for pointing out that a patch should serve a single purpose. I 
will correct the indentation in a separate commit.


Best Regards,
Julien


Le 25/05/2016 10:00, Richard Leitner a écrit :

Hi Julien,
comments on the code are below.
On 05/22/2016 07:46 PM, Julien Gueytat wrote:

It works exactly the same way than the angle parameter:
  * --angle <-> /etc/rotation file
  * --fbdev <-> /etc/fbdev file

Signed-off-by: Julien Gueytat 
---
  psplash-fb.c | 16 ++--
  psplash-fb.h |  4 ++--
  psplash.c| 57 -
  3 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/psplash-fb.c b/psplash-fb.c
index 8daaf6f..d344e5a 100644
--- a/psplash-fb.c
+++ b/psplash-fb.c
@@ -99,18 +99,20 @@ attempt_to_change_pixel_format (PSplashFB *fb,
  }
  
  PSplashFB*

-psplash_fb_new (int angle)
+psplash_fb_new (int angle, int fbdev_id)
  {
struct fb_var_screeninfo fb_var;
struct fb_fix_screeninfo fb_fix;
int  off;
-  char*fbdev;
+  char fbdev[9] = "/dev/fb0";
  
PSplashFB *fb = NULL;
  
-  fbdev = getenv("FBDEV");

-  if (fbdev == NULL)
-fbdev = "/dev/fb0";
+  if (fbdev_id > 0 && fbdev_id < 10)
+{
+// Conversion from integer to ascii.
+fbdev[7] = fbdev_id + 48;
+}

You removed the possiblity to get the fb device from FBDEV!
Please don't to that as people may rely on that feature!

If you like to add a fbdev commandline option make sure it can co-exist
with the already available methods for setting this option.
And don't forget to add documentation!

  
if ((fb = malloc (sizeof(PSplashFB))) == NULL)

  {

...


@@ -205,35 +205,41 @@ int
  main (int argc, char** argv)
  {
char  *tmpdir;
-  intpipe_fd, i = 0, angle = 0, ret = 0;
+  intpipe_fd, i = 0, angle = 0, fbdev_id = 0, ret = 0;
PSplashFB *fb;
bool   disable_console_switch = FALSE;
-
+
signal(SIGHUP, psplash_exit);
signal(SIGINT, psplash_exit);
signal(SIGQUIT, psplash_exit);
  
-  while (++i < argc)

-{
-  if (!strcmp(argv[i],"-n") || !strcmp(argv[i],"--no-console-switch"))
-{
- disable_console_switch = TRUE;
- continue;
-   }
+  while (++i < argc) {
+if (!strcmp(argv[i],"-n") || !strcmp(argv[i],"--no-console-switch"))
+  {
+disable_console_switch = TRUE;
+continue;
+  }
+
+if (!strcmp(argv[i],"-a") || !strcmp(argv[i],"--angle"))
+  {
+if (++i >= argc) goto fail;
+angle = atoi(argv[i]);
+continue;
+  }
+
+if (!strcmp(argv[i],"-f") || !strcmp(argv[i],"--fbdev"))
+  {
+if (++i >= argc) goto fail;
+fbdev_id = atoi(argv[i]);
+continue;
+  }
  
-  if (!strcmp(argv[i],"-a") || !strcmp(argv[i],"--angle"))

-{
- if (++i >= argc) goto fail;
- angle = atoi(argv[i]);
- continue;
-   }
-
  fail:
fprintf(stderr,
- "Usage: %s [-n|--no-console-switch][-a|--angle <0|90|180|270>]\n",
- argv[0]);
+  "Usage: %s [-n|--no-console-switch][-a|--angle <0|90|180|270>][-f|--fbdev 
<0..9>]\n",
+  argv[0]);
exit(-1);
-}
+  }

Please use the same coding style everywhere in psplash.

Furthermore please avoid/remove trailing whitespaces in new/changed code

  
tmpdir = getenv("TMPDIR");
  
@@ -245,10 +251,10 @@ main (int argc, char** argv)

if (mkfifo(PSPLASH_FIFO, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP))
  {
if (errno!=EEXIST)
-   {
- perror("mkfifo");
- exit(-1);
-   }
+   {
+ perror("mkfifo");
+ exit(-1);
+   }
  }

This change is irrelevant for new fbdev option.
Please send this whitespace fix as a separate patch.

  
pipe_fd = open (PSPLASH_FIFO,O_RDONLY|O_NONBLOCK);

@@ -262,10 +268,11 @@ main (int argc, char** argv)
if (!disable_console_switch)
  psplash_console_switch ();
  
-  if ((fb = psplash_fb_new(angle)) == NULL) {

+  if ((fb = psplash_fb_new(angle,fbdev_id)) == NULL)
+{
  ret = -1;
  goto fb_fail;
-  }
+}
  
/* Clear the background with #ecece1 */

psplash_fb_draw_rect (fb, 0, 0, fb->width, fb->height,


IMHO in this case fixing the whitespace is fine because the if clause
needs to be adopted for the new option.


--
___
yocto mailing list
yocto@yoctoproject.org
https://lists.yoctoproject.org/listinfo/yocto

Re: [yocto] [psplash][PATCH] Add fbdev option to set the proper /dev/fbX.

[Richard Leitner]

Hi Julien,
comments on the code are below.
On 05/22/2016 07:46 PM, Julien Gueytat wrote:
> It works exactly the same way than the angle parameter:
>  * --angle <-> /etc/rotation file
>  * --fbdev <-> /etc/fbdev file
> 
> Signed-off-by: Julien Gueytat 
> ---
>  psplash-fb.c | 16 ++--
>  psplash-fb.h |  4 ++--
>  psplash.c| 57 -
>  3 files changed, 44 insertions(+), 33 deletions(-)
> 
> diff --git a/psplash-fb.c b/psplash-fb.c
> index 8daaf6f..d344e5a 100644
> --- a/psplash-fb.c
> +++ b/psplash-fb.c
> @@ -99,18 +99,20 @@ attempt_to_change_pixel_format (PSplashFB *fb,
>  }
>  
>  PSplashFB*
> -psplash_fb_new (int angle)
> +psplash_fb_new (int angle, int fbdev_id)
>  {
>struct fb_var_screeninfo fb_var;
>struct fb_fix_screeninfo fb_fix;
>int  off;
> -  char*fbdev;
> +  char fbdev[9] = "/dev/fb0";
>  
>PSplashFB *fb = NULL;
>  
> -  fbdev = getenv("FBDEV");
> -  if (fbdev == NULL)
> -fbdev = "/dev/fb0";
> +  if (fbdev_id > 0 && fbdev_id < 10)
> +{
> +// Conversion from integer to ascii.
> +fbdev[7] = fbdev_id + 48;
> +}

You removed the possiblity to get the fb device from FBDEV!
Please don't to that as people may rely on that feature!

If you like to add a fbdev commandline option make sure it can co-exist
with the already available methods for setting this option.
And don't forget to add documentation!

>  
>if ((fb = malloc (sizeof(PSplashFB))) == NULL)
>  {

...

> @@ -205,35 +205,41 @@ int
>  main (int argc, char** argv) 
>  {
>char  *tmpdir;
> -  intpipe_fd, i = 0, angle = 0, ret = 0;
> +  intpipe_fd, i = 0, angle = 0, fbdev_id = 0, ret = 0;
>PSplashFB *fb;
>bool   disable_console_switch = FALSE;
> -  
> +
>signal(SIGHUP, psplash_exit);
>signal(SIGINT, psplash_exit);
>signal(SIGQUIT, psplash_exit);
>  
> -  while (++i < argc)
> -{
> -  if (!strcmp(argv[i],"-n") || !strcmp(argv[i],"--no-console-switch"))
> -{
> -   disable_console_switch = TRUE;
> -   continue;
> - }
> +  while (++i < argc) {
> +if (!strcmp(argv[i],"-n") || !strcmp(argv[i],"--no-console-switch"))
> +  {
> +disable_console_switch = TRUE;
> +continue;
> +  }
> +
> +if (!strcmp(argv[i],"-a") || !strcmp(argv[i],"--angle"))
> +  {
> +if (++i >= argc) goto fail;
> +angle = atoi(argv[i]);
> +continue;
> +  }
> +
> +if (!strcmp(argv[i],"-f") || !strcmp(argv[i],"--fbdev"))
> +  {
> +if (++i >= argc) goto fail;
> +fbdev_id = atoi(argv[i]);
> +continue;
> +  }
>  
> -  if (!strcmp(argv[i],"-a") || !strcmp(argv[i],"--angle"))
> -{
> -   if (++i >= argc) goto fail;
> -   angle = atoi(argv[i]);
> -   continue;
> - }
> -  
>  fail:
>fprintf(stderr, 
> -   "Usage: %s [-n|--no-console-switch][-a|--angle 
> <0|90|180|270>]\n", 
> -   argv[0]);
> +  "Usage: %s [-n|--no-console-switch][-a|--angle 
> <0|90|180|270>][-f|--fbdev <0..9>]\n", 
> +  argv[0]);
>exit(-1);
> -}
> +  }

Please use the same coding style everywhere in psplash.

Furthermore please avoid/remove trailing whitespaces in new/changed code

>  
>tmpdir = getenv("TMPDIR");
>  
> @@ -245,10 +251,10 @@ main (int argc, char** argv)
>if (mkfifo(PSPLASH_FIFO, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP))
>  {
>if (errno!=EEXIST) 
> - {
> -   perror("mkfifo");
> -   exit(-1);
> - }
> + {
> +   perror("mkfifo");
> +   exit(-1);
> + }
>  }

This change is irrelevant for new fbdev option.
Please send this whitespace fix as a separate patch.

>  
>pipe_fd = open (PSPLASH_FIFO,O_RDONLY|O_NONBLOCK);
> @@ -262,10 +268,11 @@ main (int argc, char** argv)
>if (!disable_console_switch)
>  psplash_console_switch ();
>  
> -  if ((fb = psplash_fb_new(angle)) == NULL) {
> +  if ((fb = psplash_fb_new(angle,fbdev_id)) == NULL)
> +{
> ret = -1;
> goto fb_fail;
> -  }
> +}
>  
>/* Clear the background with #ecece1 */
>psplash_fb_draw_rect (fb, 0, 0, fb->width, fb->height,
> 

IMHO in this case fixing the whitespace is fine because the if clause
needs to be adopted for the new option.
-- 
___
yocto mailing list
yocto@yoctoproject.org
https://lists.yoctoproject.org/listinfo/yocto

[yocto] [psplash][PATCH] Add fbdev option to set the proper /dev/fbX.

[Julien Gueytat]

It works exactly the same way than the angle parameter:
 * --angle <-> /etc/rotation file
 * --fbdev <-> /etc/fbdev file

Signed-off-by: Julien Gueytat 
---
 psplash-fb.c | 16 ++--
 psplash-fb.h |  4 ++--
 psplash.c| 57 -
 3 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/psplash-fb.c b/psplash-fb.c
index 8daaf6f..d344e5a 100644
--- a/psplash-fb.c
+++ b/psplash-fb.c
@@ -99,18 +99,20 @@ attempt_to_change_pixel_format (PSplashFB *fb,
 }
 
 PSplashFB*
-psplash_fb_new (int angle)
+psplash_fb_new (int angle, int fbdev_id)
 {
   struct fb_var_screeninfo fb_var;
   struct fb_fix_screeninfo fb_fix;
   int  off;
-  char*fbdev;
+  char fbdev[9] = "/dev/fb0";
 
   PSplashFB *fb = NULL;
 
-  fbdev = getenv("FBDEV");
-  if (fbdev == NULL)
-fbdev = "/dev/fb0";
+  if (fbdev_id > 0 && fbdev_id < 10)
+{
+// Conversion from integer to ascii.
+fbdev[7] = fbdev_id + 48;
+}
 
   if ((fb = malloc (sizeof(PSplashFB))) == NULL)
 {
@@ -124,7 +126,9 @@ psplash_fb_new (int angle)
 
   if ((fb->fd = open (fbdev, O_RDWR)) < 0)
 {
-  perror ("Error opening /dev/fb0");
+  fprintf(stderr,
+  "Error opening %s\n",
+  fbdev);
   goto fail;
 }
 
diff --git a/psplash-fb.h b/psplash-fb.h
index a4a0f4c..d0dce10 100644
--- a/psplash-fb.h
+++ b/psplash-fb.h
@@ -38,7 +38,7 @@ typedef struct PSplashFB
   char *data;
   char *base;
 
-  intangle;
+  intangle, fbdev_id;
   intreal_width, real_height;
 
   enum RGBMode   rgbmode;
@@ -55,7 +55,7 @@ void
 psplash_fb_destroy (PSplashFB *fb);
 
 PSplashFB*
-psplash_fb_new (int angle);
+psplash_fb_new (int angle, int fbdev_id);
 
 void
 psplash_fb_draw_rect (PSplashFB*fb, 
diff --git a/psplash.c b/psplash.c
index 04d3d49..992e199 100644
--- a/psplash.c
+++ b/psplash.c
@@ -205,35 +205,41 @@ int
 main (int argc, char** argv) 
 {
   char  *tmpdir;
-  intpipe_fd, i = 0, angle = 0, ret = 0;
+  intpipe_fd, i = 0, angle = 0, fbdev_id = 0, ret = 0;
   PSplashFB *fb;
   bool   disable_console_switch = FALSE;
-  
+
   signal(SIGHUP, psplash_exit);
   signal(SIGINT, psplash_exit);
   signal(SIGQUIT, psplash_exit);
 
-  while (++i < argc)
-{
-  if (!strcmp(argv[i],"-n") || !strcmp(argv[i],"--no-console-switch"))
-{
- disable_console_switch = TRUE;
- continue;
-   }
+  while (++i < argc) {
+if (!strcmp(argv[i],"-n") || !strcmp(argv[i],"--no-console-switch"))
+  {
+disable_console_switch = TRUE;
+continue;
+  }
+
+if (!strcmp(argv[i],"-a") || !strcmp(argv[i],"--angle"))
+  {
+if (++i >= argc) goto fail;
+angle = atoi(argv[i]);
+continue;
+  }
+
+if (!strcmp(argv[i],"-f") || !strcmp(argv[i],"--fbdev"))
+  {
+if (++i >= argc) goto fail;
+fbdev_id = atoi(argv[i]);
+continue;
+  }
 
-  if (!strcmp(argv[i],"-a") || !strcmp(argv[i],"--angle"))
-{
- if (++i >= argc) goto fail;
- angle = atoi(argv[i]);
- continue;
-   }
-  
 fail:
   fprintf(stderr, 
- "Usage: %s [-n|--no-console-switch][-a|--angle 
<0|90|180|270>]\n", 
- argv[0]);
+  "Usage: %s [-n|--no-console-switch][-a|--angle 
<0|90|180|270>][-f|--fbdev <0..9>]\n", 
+  argv[0]);
   exit(-1);
-}
+  }
 
   tmpdir = getenv("TMPDIR");
 
@@ -245,10 +251,10 @@ main (int argc, char** argv)
   if (mkfifo(PSPLASH_FIFO, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP))
 {
   if (errno!=EEXIST) 
-   {
- perror("mkfifo");
- exit(-1);
-   }
+   {
+ perror("mkfifo");
+ exit(-1);
+   }
 }
 
   pipe_fd = open (PSPLASH_FIFO,O_RDONLY|O_NONBLOCK);
@@ -262,10 +268,11 @@ main (int argc, char** argv)
   if (!disable_console_switch)
 psplash_console_switch ();
 
-  if ((fb = psplash_fb_new(angle)) == NULL) {
+  if ((fb = psplash_fb_new(angle,fbdev_id)) == NULL)
+{
  ret = -1;
  goto fb_fail;
-  }
+}
 
   /* Clear the background with #ecece1 */
   psplash_fb_draw_rect (fb, 0, 0, fb->width, fb->height,
-- 
1.9.1

-- 
___
yocto mailing list
yocto@yoctoproject.org
https://lists.yoctoproject.org/listinfo/yocto