Re: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line, option

[Peter Rosin]

On 2019-01-07 09:59, Peter Rosin wrote:
> On 2019-01-07 09:40, Geert Uytterhoeven wrote:
>> Hi Peter,
>>
>> On Mon, Jan 7, 2019 at 9:26 AM Peter Rosin <peda@xxxxxxxxxx> wrote:
>>> On 2019-01-06 10:33, Geert Uytterhoeven wrote:
>>>> On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin <peda@xxxxxxxxxx> wrote:
>>>>> If there are extra logos (CONFIG_FB_LOGO_EXTRA) the heights of these
>>>>> extra logos are not considered when centering the first logo vertically.
>>>>>
>>>>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
>>>>
>>>>> --- a/drivers/video/logo/Kconfig
>>>>> +++ b/drivers/video/logo/Kconfig
>>>>> @@ -10,6 +10,15 @@ menuconfig LOGO
>>>>>
>>>>>  if LOGO
>>>>>
>>>>> +config FB_LOGO_CENTER
>>>>> +       bool "Center the logo"
>>>>> +       depends on FB=y
>>>>> +       help
>>>>> +         When this option is selected, the bootup logo is centered both
>>>>> +         horizontally and vertically. If more than one logo is displayed
>>>>> +         due to multiple CPUs, the collected line of logos is centered
>>>>> +         as a whole.
>>>>> +
>>>>
>>>> Isn't a kernel command line option more suitable to configure the position
>>>> of the logo?
>>>
>>> That didn't occur to me previously, but it does make sense now that you
>>> mention it. This is on top of v5.0-rc1, and if applied before v5.0 we
>>> can avoid possible regressions for folks who might start to rely on
>>> CONFIG_FB_LOGO_CENTER if v5.0 is released w/o this.
>>>
>>> Cheers,
>>> Peter
>>>
>>> From de7353ab519ba9b5c9ea3f62d607bb8e94b687cc Mon Sep 17 00:00:00 2001
>>> From: Peter Rosin <peda@xxxxxxxxxx>
>>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
>>> Cc: Jonathan Corbet <corbet@xxxxxxx>
>>> Cc: Peter Rosin <peda@xxxxxxxxxx>
>>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>> Cc: linux-fbdev@xxxxxxxxxxxxxxx
>>> Cc: linux-doc@xxxxxxxxxxxxxxx
>>> To: linux-kernel@xxxxxxxxxxxxxxx
>>> Date: Mon, 7 Jan 2019 08:35:26 +0100
>>> Subject: [PATCH] fbdev: fbmem: convert CONFIG_FB_LOGO_CENTER into a cmd line
>>>  option
>>>
>>> A command line option is much more flexible than a config option and
>>> the supporting code is small. Gets rid of #ifdefs in the code too...
>>>
>>> Suggested-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
>>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
>>
>> Thanks for your patch!
>>
>>> --- a/Documentation/fb/fbcon.txt
>>> +++ b/Documentation/fb/fbcon.txt
>>> @@ -163,6 +163,12 @@ C. Boot options
>>>         be preserved until there actually is some text is output to the console.
>>>         This option causes fbcon to bind immediately to the fbdev device.
>>>
>>> +7. fbcon=center-logo
>>> +
>>> +       When this option is selected, the bootup logo is centered both
>>> +       horizontally and vertically. If more than one logo is displayed due to
>>> +       multiple CPUs, the collected line of logos is centered as a whole.
>>> +
>>
>> What about making this more generic, than an option specific for centering?
>> Else people want fbcon=left-logo or fbcon=right-logo soon?
>>
>>     fbcon=logo-pos:<pos>
>>
>> With <pos> being "center", "left", "top", "right", "bottom", and combinations
>> like "top,center"? Or is that complicating stuff too much?
> 
> I'm not too keen on implementing all that when I have zero use for it. I can
> however rename the trigger for the existing fb_center_logo bool to
> 
> 	fbcon=logo-pos:center
> 
> and add a check for "top,left" to reset the bool to false. Then the other
> variants can be added later by whomever and when needed.
> 
> Is that good enough?

Ouch, I just realized that using a comma is a no-go, as that is already
separating fbcon suboptions, so I suggest top-left instead.

Cheers,
Peter