Re: [Buildroot] [PATCH v2 2/2] package/psplash: add support to missing configure options

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

Hello,

On Mon, 15 Nov 2021 16:26:04 +0100
Kory Maincent <kory.maincent@bootlin.com> wrote:

> Add support to three configure options:

Here and in the commit title: "Add support *for*" (not to)

> diff --git a/package/psplash/Config.in b/package/psplash/Config.in
> index da99b264e0..e79e9e85c4 100644
> --- a/package/psplash/Config.in
> +++ b/package/psplash/Config.in
> @@ -35,6 +35,21 @@ config BR2_PACKAGE_PSPLASH_IMAGE
>  	  Use a personalized png image as boot splash.
>  	  Let it empty if you want to keep the psplash default image.
>  
> +config BR2_PACKAGE_PSPLASH_FULL_SCREEN
> +	bool "use fullscreen mode"
> +	help
> +	  Enable the psplash image in fullscreen mode.
> +
> +config BR2_PACKAGE_PSPLASH_NO_STARTUP_MSG
> +	bool "disable startup message"
> +	help
> +	  Disable text banner output on startup.

Please use positive logic in options, i.e options should "enable"
something, not "disable" something. If the previous default (when the
option didn't exist) was to have it enabled, add a "default y" in the
Config.in option so that it remains enabled by default.

> +
> +config BR2_PACKAGE_PSPLASH_NO_PROGRESS_BAR
> +	bool "disable progress bar"
> +	help
> +	  Disable the management of the progress bar.

Same.

> +ifeq ($(BR2_PACKAGE_PSPLASH_FULL_SCREEN),y)
> +PSPLASH_CONF_OPTS += --enable-img-fullscreen
> +endif

We like to have both sides of the condition, i.e:

ifeq ($(BR2_PACKAGE_PSPLASH_FULL_SCREEN),y)
PSPLASH_CONF_OPTS += --enable-img-fullscreen
else
PSPLASH_CONF_OPTS += --disable-img-fullscreen
endif

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot