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

[Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>]

On 01/07/2019 11:35 AM, Peter Rosin wrote:
> On 2019-01-07 10:11, Geert Uytterhoeven wrote:
>> Hi Peter,
>>
>> On Mon, Jan 7, 2019 at 10:03 AM Peter Rosin <peda@axentia.se> wrote:
>>> On 2019-01-07 09:59, Peter Rosin wrote:
>>>> On 2019-01-07 09:40, Geert Uytterhoeven wrote:
>>>>> On Mon, Jan 7, 2019 at 9:26 AM Peter Rosin <peda@axentia.se> wrote:
>>>>>> On 2019-01-06 10:33, Geert Uytterhoeven wrote:
>>>>>>> On Mon, Nov 26, 2018 at 10:59 PM Peter Rosin <peda@axentia.se> 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@axentia.se>
>>>>>>>
>>>>>>>> --- 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@axentia.se>
>>>>>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>>>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>>>>> Cc: Peter Rosin <peda@axentia.se>
>>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>>> Cc: linux-fbdev@vger.kernel.org
>>>>>> Cc: linux-doc@vger.kernel.org
>>>>>> To: linux-kernel@vger.kernel.org
>>>>>> 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@linux-m68k.org>
>>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>>
>>>>> 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.
>>
>> Sure, sounds fine to me.  I just want to avoid having a parameter system
>> that complicates future extension.
>> I think you can drop the check for top-left, as the bool defaults to false.
> 
> Right. So, here's an update...
> 
> Again, it would probably be best if this went in before 5.0 is released.
> 
> Changes since v1:
> - rename from fbcon=center-logo option to fbcon=logo-pos:center (and adjust
>   the help text accordingly).
> 
> Cheers,
> Peter
> 
> From ebfb2feb7ea4298ac9c222e6ea6f9c9a92452084 Mon Sep 17 00:00:00 2001
> From: Peter Rosin <peda@axentia.se>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Peter Rosin <peda@axentia.se>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> To: linux-kernel@vger.kernel.org
> Date: Mon, 7 Jan 2019 08:35:26 +0100
> Subject: [PATCH v2] 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@linux-m68k.org>
> Signed-off-by: Peter Rosin <peda@axentia.se>

Patch queued for 5.0, thanks.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics