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

[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 <contact@jgueytat.fr>
---
 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;
 
-  int            angle;
+  int            angle, fbdev_id;
   int            real_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;
-  int        pipe_fd, i = 0, angle = 0, ret = 0;
+  int        pipe_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

Re: [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 <contact@jgueytat.fr>
> ---
>  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;
> -  int        pipe_fd, i = 0, angle = 0, ret = 0;
> +  int        pipe_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.

Re: [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 <contact@jgueytat.fr>
>> ---
>>   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;
>> -  int        pipe_fd, i = 0, angle = 0, ret = 0;
>> +  int        pipe_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.

Re: [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