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

[Geert Uytterhoeven]

Hi Peter,

On Mon, Jan 7, 2019 at 10:03 AM Peter Rosin <peda@xxxxxxxxxx> 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@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.

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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds