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

[Julien Gueytat <contact@jgueytat.fr>]

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.